feat(pass): cancel run if the engine becomes unavailable mid-resolution - #611
Conversation
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The liveness monitor implementation looks correct. The goroutine lifecycle is clean: ctx.Done() exits the loop on any cancellation (from either the parent context or cancel(nil) deferred by RunCommand), so there is no goroutine leak. The context.WithCancelCause / context.Cause pattern correctly surfaces the "secrets engine became unavailable" error when the monitor fires, and passes through genuine resolution errors when the monitor hasn't fired yet. The ping uses context.WithoutCancel to deliberately decouple the ping deadline from the already-cancelled parent — that's the right call. The test suite covers both the happy path and the failure path with appropriate intervals.
No bugs introduced by this PR were found.
Benehiko
left a comment
Benehiko
left a comment
There was a problem hiding this comment.
Code review — liveness monitor (ca71693)
15 findings from a multi-angle review with adversarial verification (14 CONFIRMED, 1 PLAUSIBLE, 0 refuted). 14 are posted as inline comments below; one finding has no diff anchor, so it is included here.
Top risks: false-positive run cancellation (any Version() error is treated as engine death, a single slow ping kills the run, and the effective header deadline is 1s rather than the intended 3s), error masking by the context.Cause branch, and the monitor both outliving resolution and being structurally unable to catch the per-request wedge it was built for.
[CONFIRMED] correctness — run.go:92 (not in diff): feature premise contradicts client config — missing WithResponseTimeout(0)
run expects resolution to block for minutes on approval prompts, but builds the client without WithResponseTimeout(0), leaving the 1s header timeout that client.WithResponseTimeout's godoc says interactive flows must override.
If the engine does not flush response headers before blocking on approval, every approval taking longer than ~1s dies with a raw timeout awaiting response headers — before the monitor's first 5s tick — making the new mechanism and its curated error unreachable for the pre-header wedge class. If the engine does flush early, the unbounded post-header wait affects all client consumers and only pass run gets a (partial) guard. The timeout regimes (1s header / 3s ping / 5s interval / optional --timeout) live in different layers with no coordination.
🤖 Generated with Claude Code
| func TestStartLivenessMonitor(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("resolves normally when engine stays alive", func(t *testing.T) { |
There was a problem hiding this comment.
[CONFIRMED] test-coverage — the monitor's two core paths are untested: a hanging Version() and continue-after-successful-ping
Every mock versionFn returns instantly and ignores its ctx, so dropping the context.WithTimeout on pingCtx (or a client change that stops honoring ctx in Version) passes the suite while a truly wedged engine blocks the ping forever and the monitor never fires — the exact hang class this PR exists for. A versionFn that blocks on <-ctx.Done() would cover it.
Subtest 1 ("resolves normally when engine stays alive", 50ms interval) resolves in microseconds and usually cancels before the first tick, making it functionally identical to subtest 3 — a regression breaking continue-after-successful-ping passes CI.
| return []secrets.Envelope{{ID: secrets.MustParseID("tok"), Value: []byte("val")}}, nil | ||
| }, | ||
| versionFn: func(_ context.Context) (client.DaemonVersion, error) { | ||
| t.Error("Version() called unexpectedly") |
There was a problem hiding this comment.
[CONFIRMED] test-quality — t.Error inside versionFn runs on the monitor goroutine and is safe only because interval=time.Hour
If a future edit lowers the interval (e.g. copying subtest 1's 50ms "to actually exercise the path"), the ticker can fire after the subtest returns — and the in-flight ping is uncancellable because pingCtx derives from context.WithoutCancel (run.go:201) — so t.Error fires post-completion and the "Log in goroutine after test has completed" panic aborts the whole test binary.
The subtest name "does not fire when resolution completes first" also documents a contract production does not have: in RunCommand the monitor keeps running after resolution (see the defer cancel comment); the test passes only because an hour never elapses.
Run a single bounded Version() ping before resolving secrets, so an unreachable engine fails `pass run` fast instead of hanging resolution indefinitely. The ping only runs when the client request timeout is indefinite; a timeout configured via WithTimeout already bounds the wait. The Docker CLI bounds its daemon connection ping the same way (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). Also expose WithResponseTimeout and WithSocketPath run options so embedders can override the client's 1s response-header timeout and the engine socket path; the socket option also lets tests point the preflight ping at a dead socket. Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
ca71693 to
2e650c7
Compare
3 participants
Start a background liveness monitor alongside resolveEnv that pings Version() every 5s (3s deadline per ping). If the engine stops responding the monitor cancels the shared context, unblocking pass run instead of hanging indefinitely.
The engine is multi-threaded and services pings concurrently with in-flight secret resolution and open ACM approval prompts, so a failed ping reliably indicates a wedge rather than load.
This mirrors the pattern docker/cli settled on after indefinite hangs were reported against an unreachable daemon: