Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
A deadline race and missing concurrent regression coverage leave uncancellable credential-write behavior needing human review.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
This PR adds opt-in deadlines for keyring operations in the shared credential-store library, addressing basecamp/basecamp-cli#800 while leaving file storage and zero-value behavior unchanged.
Changes:
- Adds
OperationTimeoutand refuses further keyring calls after an in-progress call times out. - Adds regression tests and documents the deadline behavior.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
| README.md | Summarizes credential-store deadlines. |
| credstore/store.go | Adds the option and routes keyring calls through it. |
| credstore/probe.go | Makes keyring reads injectable for tests. |
| credstore/operation.go | Implements deadlines and post-timeout refusal. |
| credstore/operation_test.go | Tests timeout and provider behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
robzolkos
changed the title
Check the deadline again before accepting a provider result, including when timer delivery lags. Expired results discard data and keep the store unavailable without switching storage backends. Retain the stalled-provider regression with 64 queued Load/Save/Delete callers, and clarify that an unattempted queue timeout does not itself poison the store.
|
Fixed all three review findings in afacf59:
The companion Basecamp CLI PR basecamp/basecamp-cli#805 still pins the earlier shared-library commit |
What
Callers can now put a deadline on keyring reads, writes, deletes and migration writes using opt-in
OperationTimeout. It includes queue waiting; zero/negative values retain existing behavior, and file storage is unchanged.A later keyring timeout returns a deadline error, not missing credentials or stale plaintext fallback. If the provider call started, the store refuses further operations because that call cannot be canceled and may still complete. A queued call that never started reports that it was not attempted.
Why
Bounding only the availability probe leaves callers exposed to hangs after a successful probe. Timeout errors must also retain their identity so callers do not mistake an unreadable store for a missing login.
This is the shared-library prerequisite for basecamp/basecamp-cli#805, which opts Linux into 10-second probe and operation limits. Review and merge this PR first; preserve the red-test-before-fix history. Both PRs remain unmerged with auto-merge disabled.
Testing
make checkpasses at reviewed HEAD86242a69567b7735ea1bc157b6988c6cf802a465.84a8b96precedes fix60dca1a: read/write/delete/migration tests fail on behavior before the fix and pass afterward.bin/ciand hosted checks also pass.Review corrected timeout errors being mislabeled as missing credentials and a misleading late-completion warning for queued calls that never ran.
The validation tool's CI watcher could not parse the local
ghwrapper's setup chatter. It was stopped after the actual hosted checks were independently verified with the real GitHub CLI binary; no test or hosted check was waived.Related: basecamp/basecamp-cli#800
Earlier automated review and regression receipts
Intent
Fix basecamp/basecamp-cli#800 with an opt-in shared credstore OperationTimeout, separate from ProbeTimeout. Commit a behaviorally red regression before the fix. Preserve zero-value behavior for existing consumers and all file storage. After a real keyring operation times out, return a context deadline error, do NOT switch to plaintext or serve stale credentials, and refuse further operations on that store because writes/deletes can still complete. Keep the initial probe fallback as-is. Validate healthy calls, provider errors, read/write/delete/migration timeouts, late completion and queue deadlines, race safety and platform compilation. This is one of two dependent PRs; the Basecamp consumer change will opt Linux into 10-second probe and operation bounds. Open an unmerged PR and get checks green. Do NOT merge, enable auto-merge, close the issue, or squash the red-test-before-fix commit sequence; the user must review everything first.
What Changed
StoreOptions.OperationTimeout, separate fromProbeTimeout, to put a time limit on keyringLoad,SaveandDeleteafter a successful probe. BecauseMigrateToKeyringsaves throughSave, each migration write is covered too. The limit includes time spent waiting behind another operation on the same store. A zero or negative value keeps today's unbounded behavior, and file storage is never time-limited. The initial probe fallback is unchanged.context.DeadlineExceeded. The store never falls back to plaintext and never serves a possibly stale fallback file. go-keyring can't cancel a call that has already started, so a write or delete may still complete. For that reason the store refuses all further keyring operations.Loadno longer adds thecredentials not foundprefix to timeout errors. A timeout while waiting in the queue gets its own error saying the call was never attempted.credstore/operation_test.go, committed ahead of the fix, which fail without it. They cover healthy calls, provider errors, timeouts on read, write, delete and migration, late completion, and queue deadlines.keyringGetis now an injectable var alongsidekeyringSetandkeyringDelete. The README gets a short section on credential-store deadlines.Refs basecamp/basecamp-cli#800. A companion Basecamp CLI change will opt Linux into 10-second probe and operation limits.
Risk Assessment
✅ Low: The follow-up commit fixes both round-1 findings with minimal, tested changes.
Loadnow returns timeout and refusal errors without thecredentials not foundprefix, and a queued operation that never ran now reports that it was not attempted instead of saying it may still complete. The opt-in change stays race-safe and meets the intent: zero-value behavior is preserved, there is no plaintext fallback, and the store is refused after a real timeout.Testing
I ran the new credstore tests at test-only commit 84a8b96, where they fail on behaviour (not compilation): load, save, delete and migrate ignore the deadline. On HEAD I ran the targeted operation-timeout tests and the whole credstore package under -race, and cross-compiled the package and its tests for linux, darwin, windows and freebsd. I also ran a temporary scenario test (since deleted) five times under -race. It covers a healthy probe followed by a hung keyring, plus 64 concurrent callers. It confirmed the deadline errors, no switch to plaintext, no stale data returned, an unchanged fallback file, the store refusing further calls, only one abandoned provider call, and unchanged healthy and zero-value behaviour. Everything passed and the working tree is clean. This is a library change with no UI, so the evidence is test transcripts rather than screenshots.
Evidence: Test-only commit 84a8b96 fails on behaviour, not compilation
Evidence: HEAD operation-timeout tests under -race (x3)
Evidence: Consumer-style transcript: healthy probe, hung keyring, deadline error, store stays on keyring, fallback file untouched; 64-caller concurrency under -race
probe healthy: UsingKeyring=true FallbackWarning="" Load (hung keyring) after 200ms: data="" err=keyring read timed out after 200ms (the operation may still complete): context deadline exceeded Save after timeout: err=keyring unavailable after an earlier timeout: keyring read timed out after 200ms (the operation may still complete): context deadline exceeded UsingKeyring=true FallbackWarning="" provider calls=1 fallback file untouched: {"default":{"access_token":"stale-plaintext"}} 64 concurrent callers: all returned DeadlineExceeded, provider calls=1 64 concurrent healthy callers: no failuresEvidence: Cross-platform compilation of the credstore package and its tests
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
credstore/store.go:135- When a keyring read times out, or the store has been refused after an earlier timeout,Loadstill wraps the error ascredentials not found: keyring read timed out ... (the operation may still complete): context deadline exceeded. The new README says callers "should not ... treat it as a missing login", but the library's own error text says the credentials are missing. Any consumer or user who reads the message prefix, rather than checkingerrors.Is(err, context.DeadlineExceeded), will take a hung keyring for a logged-out state. The Basecamp consumer PR that opts Linux into 10s bounds is exactly where this would show up. Consider returning timeout and refusal errors fromLoadwithout thecredentials not foundprefix. The prefix is kept for provider errors today, so this is a user-visible message decision for the author.credstore/operation.go:20- The sametimeoutError()text,(the operation may still complete), is returned in three cases: the deadline expires while waiting onoperationGate(line 26), the deadline expires just after acquiring the gate (line 33), and a real call is abandoned (line 49). In the first two cases the provider was never called, so the write or delete certainly did not happen. The message still tells the caller the outcome is unknown, and the README tells callers not to retry an unknown write. The store is correctly not refused in the queued case. Only the wording is wrong, so it misleads callers or users about whether a Save or Delete may have landed. Use a distinct message for the queue-wait timeout, for examplekeyring %s timed out waiting for another operation, and keep wrappingctx.Err().🔧 Fix: Clarify keyring timeout errors for reads and queued operations
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
go test ./credstore/ -run 'TestKeyringOperationTimeout|TestBoundedKeyring|TestWaitingForKeyring|TestZeroOperationTimeout' -count=1at test-only commit 84a8b96 in a temporary detached worktree (all 4 subtests fail with 'keyring operation ignored OperationTimeout', so the failure is behavioural)go test -race -count=3 -v ./credstore/ -run 'TestKeyringOperationTimeout|TestBoundedKeyring|TestWaitingForKeyring|TestZeroOperationTimeout'on HEAD b3a9b86go test -race -count=1 ./credstore/(existing probe-fallback and file-storage tests still pass)GOOS={linux,darwin,windows,freebsd} go test -c ./credstore/cross-compile for linux/amd64, darwin/arm64, darwin/amd64, windows/amd64, freebsd/amd64Temporary scenario test credstore/zz_evidence_test.go (removed afterwards), run withgo test -race -count=5 -v -run TestEvidence: a Basecamp-style store (10s ProbeTimeout, healthy probe, then a hung keyring) records the errors a user would see, UsingKeyring/FallbackWarning and the fallback file contents; 64 concurrent Load/Save/Delete callers against a hung keyring all get DeadlineExceeded with exactly one provider call; 64 concurrent healthy callers see no failures✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.