fix: retrospective review fixes for merged PR #92

[ikeikeikeike]

Retrospective /code-review sweep of merged PR #92 (which itself fixed #74: don't hard-fail Up on a reused elasticsearch datadir owned by the container uid). That PR shipped without pre-merge review, per the repo's established retrospective-review pattern (see PRs #56-58, #63, #84, #85, #88, #108-#112).

What / Why

Reviewed ensureDatadirWritableByContainer (plugins/engine/elasticsearch/docker.go) from every angle in the fix's own risk class (ownership-detection edge cases across OSes, the original chmod-fallback path staying reachable, TOCTOU, error actionability). Two confirmed, still-live issues, both introduced by PR #92 itself:

1. False doc claim about macOS, empirically verified false. The doc comment asserted "On macOS Docker Desktop the VirtioFS proxy papers over the uid mismatch and chown also succeeds." I disproved this on a real Darwin host: os.Chown to an arbitrary uid is _POSIX_CHOWN_RESTRICTED on macOS exactly like non-root Linux — it EPERMs regardless of Docker Desktop, since the call happens on the host filesystem before any container touches the path. I further ran a real elasticsearch:7.17.29 container bind-mounted through Docker Desktop for Mac and confirmed what actually makes macOS work: a plain 0o755, host-owned datadir is already sufficient (no AccessDeniedException), and files the container writes as uid 1000 are reflected back to the host as owned by the host user, not uid 1000 — the opposite of native Linux bind-mount behavior. This is what the pre-PR-92 comment correctly said ("VirtioFS hides the uid mismatch, plain 0o755 works") before PR fix(elasticsearch): don't hard-fail Up on a reused datadir owned by the container uid (#74) #92 rewrote it into the false "chown succeeds" claim. Restored an accurate description for both platforms.

2. Chown error silently dropped from the "actionable" hard-fail message. The final error claimed "chown and chmod both failed (%w)" but only wrapped the chmod error via %w — the chown error was computed in an inner if err := ...; err != nil scope and never reached the message-building branch. Extracted the message into actionableDatadirError(datadir, chownErr, chmodErr) and wrap both (Go 1.20+ supports multiple %w in one fmt.Errorf), so errors.Is and log correlation retain the true cause of both failed syscalls, not just one.

No functional regression: traced both the fresh-datadir and reused-datadir paths against the corrected mental model and confirmed ensureDatadirWritableByContainer still returns nil in both cases exactly as before.

Test plan

• go build ./...
• go test ./... (full suite, all packages green)
• golangci-lint run ./... (0 issues)
• make conformance-local PLUGIN=elasticsearch — real Docker containers, two full Up→Down iterations (exercises the reused-datadir path), all green
• New TestActionableDatadirError unit test, deterministic via synthetic errors (the underlying "datadir owned by a third uid" case can't be constructed portably without root)
• Empirically verified the macOS chown/chmod claims against this real Darwin host + a real elasticsearch:7.17.29 container via Docker Desktop (not just code reasoning)