feat(client): move the preflight liveness ping into the client - #615
Open
Benehiko wants to merge 1 commit into
Open
feat(client): move the preflight liveness ping into the client#615Benehiko wants to merge 1 commit into
Benehiko wants to merge 1 commit into
Conversation
The preflight ping that guards against an unreachable or wedged engine lived in the pass plugin's run command (#611), so every other consumer of the client - including ones that configure an indefinite request timeout precisely because requests may wait on user interaction - was left to reimplement the same guard. Move it into the client: when the request timeout is indefinite, every GetSecrets call is preceded by a Version() ping bounded by DefaultClientPreflightPingTimeout (3s), distinguishing "engine is busy asking the user" from "engine is gone". A bounded request timeout disables the ping by default since the request itself cannot hang. The new WithPreflightPing option makes the choice explicit in either direction: it forces the ping alongside any request timeout, and a zero timeout disables it entirely. pass run drops its local preflightPing and inherits the client's. One behavioral nuance: the ping now runs per secret fetch rather than once up front, so a run that resolves no se:// references no longer requires a live engine. The Docker CLI bounds its daemon connection ping the same way (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
docker-agent
left a comment
Contributor
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
This PR cleanly moves the preflight liveness ping from the pass plugin into the client layer. The implementation is correct:
- Ping enable/disable logic is sound:
WithPreflightPing(0)stores a non-nil pointer to zero, hits the first switch case, setspingTimeout = 0, andpreflightPingreturns early on<= 0. ✓ - Error wrapping avoids double-wrapping
ErrSecretsEngineNotAvailablecorrectly viaerrors.Ischeck before re-wrapping. ✓ - Context propagation: the child context with the ping timeout properly bounds the
Version()call without affecting the parent. ✓ - Behavioral change in
pass run(ping per secret fetch vs. once up front) is intentional and correctly documented. ✓ - Test coverage: the dead-socket subprocess test correctly injects
GO_PASS_RUN_PING_REF=se://tokinto the process environment somergeEnv(os.Environ(), ...)picks it up, triggering a resolution (and thus the ping) against the dead socket. ✓ WithResponseTimeout(0)in the wedged-engine test correctly disables the response header timeout so the ping's 50ms deadline is the only bound. ✓
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
#611 added a preflight liveness ping to
pass run: a boundedVersion()ping before resolution, run only when the client request timeout is indefinite, so an unreachable or wedged engine fails fast instead of hanging resolution forever.That guard lives in the pass plugin, so every other consumer of the Go client — including ones that configure an indefinite request timeout precisely because requests may block on user interaction (see #614 for wield's case) — has to reimplement it.
Change
The client now owns the ping:
GetSecretscall is preceded by aVersion()ping bounded byapi.DefaultClientPreflightPingTimeout(3s). The engine services pings concurrently with in-flight resolution and open approval prompts, so a failed ping distinguishes "engine is busy asking the user" from "engine is gone".client.WithPreflightPing(timeout)option makes the choice explicit in either direction: it forces the ping regardless of the request timeout, and0disables it entirely. Negative values are rejected, consistent with the other timeout options.preflight ping: ...wrappingErrSecretsEngineNotAvailable.pass rundrops its localpreflightPingand inherits the client's. One behavioral nuance: the ping now runs per secret fetch rather than once up front, so a run that resolves nose://references no longer requires a live engine.The unit coverage from #611 moved to the client package and runs against a real transport (mock engine over a unix socket) instead of an interface mock, including the wedged-engine case (server accepts but never responds). The
pass rundead-socket subprocess test now injects anse://reference to force a resolution.Like #611, this mirrors the Docker CLI's bounded daemon ping (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652).
Note: touches lines adjacent to #614 in
x/api/defaults.go; whichever merges second has a trivial rebase.Testing
go build ./...+go test ./...inclient,x,plugin, andplugins/pass— all pass;gofmt/go vetclean.🤖 Generated with Claude Code