Fail a credential lookup when the store stops answering, instead of hanging - #1588
Open
GeiserX wants to merge 3 commits into
Open
Fail a credential lookup when the store stops answering, instead of hanging#1588GeiserX wants to merge 3 commits into
GeiserX wants to merge 3 commits into
Conversation
…ils, not hangs A credential provider is frequently remote — the 1Password backend talks to a service over HTTP, and any custom provider may be a network store — so "stopped answering" is one of its ordinary failure modes rather than an exotic one. Nothing bounded the call, so a store that went away did not fail a tool invocation, it hung it, and nothing in the resulting silence named the provider. Measured before changing anything: with a provider whose `get` never returns, seeding and connection creation both succeed and the resolution never comes back. A control provider resolves normally, so the hang is the provider call and not the harness. `CredentialProvider` documents nothing about timing — no expectation that `get` returns promptly, no note that the caller will not bound it — so neither side owned this. Executor already bounds its other remote calls the same way, in OAuth discovery and in the MCP plugin's probes; credential resolution was the one that did not. Bounded once at the registration funnel rather than at each call site, so a method added later is bounded by default instead of by whoever remembers. Optional methods stay optional: a provider that cannot enumerate must not appear to. The failure names the provider and the operation, so the diagnostic points at the store rather than at whatever the caller happened to be doing. Thirty seconds is a backstop against a dead dependency, not a latency budget. The tests advance a virtual clock past it rather than waiting.
This was referenced Aug 14, 2026
The wrapper destructured the four optional methods and called the bindings bare, which drops `this`. `get` was already called on the provider, so the two disagreed. Every provider in the tree is an object literal and cannot notice; a provider written as a class — which is exactly what "wrap any provider" invites — threw TypeError on its first optional call. Covered by a class-based provider test, with an object-literal control that is identical except for that one difference, so a red result can only mean the receiver. Also pins the operation in the message-shape test, which asserted the provider and the phrasing but not the operation it is named for, and uses Exit.isFailure rather than inspecting _tag, which the repo's own no-manual-tag-check rule rejects.
…mbers The wrapper spread the provider. A spread copies only own ENUMERABLE properties, so everything on a class's prototype — its methods, and accessors like `writable` — was dropped silently. Nothing raised: the wrapper simply appeared not to have the capability, and the caller took a path the provider meant to own. A class-based provider whose `writable` is an accessor stops being seen as a writable store at all, so creating a connection from a pasted value fails with "provider not registered: default". It now inherits through Object.create and shadows only the five methods it bounds, which also means a capability added to CredentialProvider later survives the wrapper without anyone remembering to list it here. Covered by a class-based provider whose `writable` lives on the prototype, verified failing before the change with exactly that error.
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
1 participant
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.
A credential provider is frequently remote — the 1Password backend talks to a service over
HTTP, and any custom provider may be a network store — so "stopped answering" is one of its ordinary
failure modes rather than an exotic one.
Worth saying up front, because it is the strongest argument for this change: the 1Password plugin
already bounds every SDK call itself at
DEFAULT_TIMEOUT_MS = 15_000. The one remote provider in thetree found a bound necessary and added its own. Executor does not give that to the providers which do
not, and nothing in the interface tells their authors it is their job.
Nothing bounds the call. Measured with a provider whose
getnever returns:So a store that goes away does not fail a tool invocation, it hangs it — and nothing in the
resulting silence names the provider.
Why this looks like a gap rather than a deliberate choice
CredentialProviderdocuments nothing about timing: no expectation thatgetreturns promptly, nonote that the caller will not bound it. So a provider author has no reason to think they own the
timeout, and Executor does not add one — which is how a remote dependency ends up with no bound at all.
Executor already bounds its other remote calls this way:
oauth-discovery.ts—Effect.timeoutOrElsepackages/plugins/mcp/src/sdk/probe-shape.ts—Effect.timeout(Duration.millis(timeoutMs)), inseveral places
Credential resolution is the one remote dependency without a bound.
Shape of the change
Bounded once at the registration funnel rather than at each call site, so every provider passes
through one place on its way in rather than every call site remembering. The wrapper names the five
methods
CredentialProviderhas today, so adding a sixth means adding it here too — worth a note onthe interface if you would rather that were automatic. Optional methods stay optional — a provider
that cannot enumerate must not appear to. The failure names the provider and the operation, so the
diagnostic points at the store rather than at whatever the caller happened to be doing.
Thirty seconds is a backstop against a dead dependency, not a latency budget; a store legitimately
slower than that is better served by the operator hearing about it than by the request waiting
indefinitely. Happy to make it configurable, or to pick a different number.
Tests
provider-call-timeout.test.ts— the hang now fails, the message names the provider and operation, anda control provider still resolves. Removing the bound reddens the first two and leaves the control
green. The tests advance a virtual clock past the bound rather than waiting thirty seconds.
Part of the review in #1585; see #1564 for the first of this series.