Conversation
A production smoke test ran the CLI as 10 parallel workers making 8
sequential calls each: 10 of 80 calls succeeded and the server never
answered 429. The failures were the CLI's own gate. Every invocation
shares one token bucket (50 tokens, refilled at 10/s) in the resilience
state file, and OnOperationGate rejected the moment the bucket was empty,
so the burst drained it in the first second and every later call exited
with "Rate limit exceeded". A re-exec'd reproduction of the same shape
against a temp state dir showed the mechanism directly: 80 calls in
0.46s, 54 allowed (50 tokens plus 0.46s of refill), 26 rejected by the
rate limiter, none by the bulkhead.
The rate limiter and bulkhead now queue instead of rejecting: a gated
operation waits for the refill it needs, or polls with jitter for a slot,
within one shared 10s budget before failing. A Retry-After block from the
server is still honoured; one that lifts within the budget is waited out,
one that outlasts it is reported at once with the remaining time. The
circuit breaker is unchanged, and every store error still fails open.
When a gate does give up it now says which limit it hit, how long it
waited, and what to do ("Too many concurrent basecamp processes (limit
10); waited 10s" / "Re-run, or lower parallelism."), carried by a
GateError that still unwraps to the SDK sentinel so errors.Is checks and
the rate-limit exit code hold.
The state-file lock budget rises from 100ms to 2s. The lock is held for
one read-modify-write of a small file, so a wait that long means a wedged
holder, not contention; a queue of polling invocations that timed out
would fall open and write back a stale copy, resurrecting a slot another
live process had just released.
github-actions
Bot
added
commands
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Jittered sleeps and lock acquisition can exceed the documented wait budget, and Retry-After guidance can understate the required delay.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Queues rate-limiter and bulkhead admission within a shared budget while preserving existing error codes and sentinel behavior.
Changes:
- Adds bounded, jittered gate waiting and actionable
GateErrordetails. - Extends lock contention tolerance.
- Adds unit and multi-process coverage.
[!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 summaries
| File | Description |
|---|---|
internal/resilience/store.go |
Extends lock timeout. |
internal/resilience/rate_limiter.go |
Adds token and Retry-After waiting. |
internal/resilience/hooks.go |
Applies the shared wait budget. |
internal/resilience/gate.go |
Defines gate errors and wait helpers. |
internal/resilience/gate_test.go |
Tests queueing and contention. |
internal/resilience/config.go |
Adds MaxWait configuration. |
internal/resilience/bulkhead.go |
Adds slot polling. |
internal/names/resolver.go |
Maps gate errors for resolvers. |
internal/commands/projects.go |
Maps gate errors for commands. |
internal/commands/projects_test.go |
Tests user-facing error conversion. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c0ceca8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The jitter that spreads retries could stretch a sleep past the gate's deadline: a 9s Retry-After under a 10s budget passed the deadline check and then slept up to 13.5s. The pause is now capped at the remaining budget, as the bulkhead's already was. A Retry-After reported to the user is rounded up to the next second, so "Wait 40s, then re-run" never sends the re-run into the last 400ms of the block.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Five unresolved moderate findings and one nit remain in deadline and cancellation handling.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
internal/resilience/bulkhead.go:102
- A poll that was capped to the remaining budget can wake at or after the deadline, and
Acquire()is attempted before any deadline check. If a slot is then available, this returns nil afterMaxWaithas expired, so the shared budget is not actually enforced. Check the deadline before accepting a successful acquisition and release the slot if the acquisition completed after the deadline.
if acquired, _ := b.Acquire(); acquired { //nolint:contextcheck // lock acquisition is context-independent by design
return nil
}
internal/resilience/bulkhead.go:100
- When
ctxis already canceled and a slot is free,Acquire()still reserves the PID andWaitreturns nil because cancellation is only checked inpause. This violates the documentedctx.Err()result and consumes shared slot capacity even though the caller has canceled. Checkctx.Err()before each acquire attempt.
if acquired, _ := b.Acquire(); acquired { //nolint:contextcheck // lock acquisition is context-independent by design
internal/resilience/bulkhead.go:105
- If the bulkhead deadline expires while
Acquireis blocked on the state lock, this branch returnsGateErroreven when the caller's context has already been canceled. That reports a concurrency-limit failure instead of the cancellation error; givectx.Err()precedence before constructing the gate error.
if remaining <= 0 {
return &GateError{
internal/resilience/hooks.go:79
- The adjacent comment still describes this as having “no state reservation” and being safe to reject, but
Waitnow blocks and consumes a token when it succeeds. Update the comment so it does not mislead maintainers about the new queueing and token-consumption behavior.
if h.rateLimiter != nil {
if err := h.rateLimiter.Wait(ctx, deadline); err != nil {
return ctx, err
internal/resilience/rate_limiter.go:123
- When the jittered sleep is capped to
remaining, it can sleep through the deadline even though the original refill delay fit within it (for example, a 95ms refill with a 100ms remainder and jitter over 5ms). The next loop callstake()before checking the deadline, so a token available at or after the deadline is consumed and this returns nil, violating the sharedMaxWaitbound. Reject anallowedresult once the deadline has passed (and account for the consumed token).
allowed, wait, blocked := rl.take() //nolint:contextcheck // lock acquisition is context-independent by design
if allowed {
return nil
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4503f0527
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…adline With the circuit open, an invocation no longer queues for a rate-limiter token it would only be refused with: the gate reads the tripped state first, without reserving anything, and the reserving check still runs last as before. A canceled caller now gets ctx.Err() before a slot is reserved or a token consumed, and ahead of the deadline error when both apply, which is what Wait documented. The test that guards the jitter cap pins the jitter to its maximum instead of relying on scheduling slack.
Member
Author
|
On the suppressed findings in the latest Copilot review:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical, moderate, and test-coverage findings remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/resilience/store.go:88
- The new 2s value is not actually exercised by
TestStoreLockContentionDoesNotLoseReleases: that test uses the production constant and completes its short churn, so it also passes with the old 100ms timeout (as the PR description notes). A regression here would stay green; make the timeout injectable or test-configurable and hold the lock for longer than 100ms but less than 2s before the competing update.
internal/resilience/rate_limiter.go:154
- When
TokensPerRequestis not 1, this reports the token refill rate as a request rate. For the supported configurationRefillRate=1andTokensPerRequest=5, requests can be admitted at only 0.2/s, but the CLI reports a 1/s client limit. ComputeRefillRate/TokensPerRequesthere, or describe the value as tokens/s, so the diagnostic is accurate.
Message: fmt.Sprintf("Too many requests (client limit %g/s); waited %s", rl.config.RefillRate, waited.Round(time.Second)),
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d3796ec23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the circuit The waits checked the budget only after a failed attempt, so a slot or token that became free as the final capped sleep ended was taken just past MaxWait. Both waits now check cancellation and the deadline before each attempt; an expired gate reserves nothing. Jitter no longer pushes a rate-limiter sleep to the deadline, which would have waited out a refill or a Retry-After block only to reject at the wire. OnOperationGate answers an already-canceled context before reading the circuit, as the primitives already did. The files auto-detection probes dereferenced basecamp.AsError on the first probe error before converting it. A gate rejection is not a *basecamp.Error (nor were the bare sentinels before this branch), so the nil result panicked instead of rendering the message and hint; the probes now treat a non-SDK error like any non-404 and convert it.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Gate-error conversion and lock-timeout test coverage require fixes; the rate display also needs correction.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
internal/resilience/gate.go:20
GateErroris only handled by the twoconvertSDKErrorhelpers, but the gating hook is installed for every SDK operation and several commands return SDK errors directly (for example,chat.goreturns the list error). Those paths go through the root'soutput.AsError, which does not inspectGateErroror itsHint; it can therefore lose the new hint and classify the wrapped sentinel as a generic API error instead ofrate_limit/exit 5. HandleGateErrorin central conversion, or route every gated SDK error throughconvertSDKError.
internal/resilience/rate_limiter.go:163- This reports
RefillRateas requests per second, but that field is tokens per second and each request consumesTokensPerRequest. With a valid configuration such as 10 tokens/s and 5 tokens/request, the effective client limit is 2 requests/s, so the new user-facing limit is inaccurate. Report the effective request rate instead.
internal/resilience/store.go:88
- The added contention test does not exercise the changed lock timeout: it uses the production constant and never holds
.locklong enough to forceacquireLockpast either 100ms or 2s. As a result it also remains green if this change is reverted to 100ms, so it cannot catch the stale read-modify-write regression this constant is intended to prevent. Add a deterministic lock-holder/controllable timeout and assert the old budget fails while the new budget preserves the release.
const LockTimeout = 2 * time.Second
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2199275163
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A gate rejection reaches the user through any SDK operation, and many commands return the SDK error straight to the root, whose output.AsError knew nothing of GateError: the hint was dropped and the wrapped sentinel rendered as a generic API error. AsError now converts it, and the two convertSDKError helpers share that conversion. The rate-limiter message reported RefillRate as requests per second; it is tokens per second, so a request costing five tokens at ten per second is a limit of two, which is what the message now says. LockTimeout becomes a variable so a test can hold the store lock from a second file description and show that the production budget waits for the holder while a shorter one falls open over it.
Member
Author
|
On the three suppressed findings in the Copilot review of 2199275, all addressed in c6e10bb:
|
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate findings remain around cancellation handling and total gate-wait reporting.
Review details
Suppressed comments (2)
internal/resilience/bulkhead.go:108
- This
startis local to the bulkhead phase, so the user-facingwaitedvalue omits any time already spent inRateLimiter.Waitunder the shared deadline. For example, after 9s waiting for a token and 1s waiting on a full bulkhead, the error reports onlywaited 1s(or0safter rounding), even though the gate exhausted its 10s budget. Carry the gate's original start/elapsed into the bulkhead error (or construct the final error inOnOperationGate) so the message reports the total gate wait.
start := b.now()
for {
if err := ctx.Err(); err != nil {
return err
}
remaining := deadline.Sub(b.now())
if remaining <= 0 {
return &GateError{
Message: fmt.Sprintf("Too many concurrent basecamp processes (limit %d); waited %s", b.config.MaxConcurrent, b.now().Sub(start).Round(time.Second)),
internal/resilience/hooks.go:103
- After this queued bulkhead phase succeeds,
OnOperationGateproceeds straight to the circuit breaker's reservingAllow()without anotherctx.Err()check. If cancellation arrives while waiting for the slot, the gate can reserve a half-open circuit attempt and return success with a canceled context; release this slot and return the cancellation before the final reservation (and cover the transition with a test).
if err := h.bulkhead.Wait(ctx, deadline); err != nil {
return ctx, err
}
// Store marker in context so OnOperationEnd knows to release the slot
ctx = context.WithValue(ctx, releaseKey{}, true)
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
The bulkhead's give-up message counted only its own phase, so a gate that spent nine seconds queueing for a token and one on a full bulkhead said it had waited a second. Both primitives now take the gate's start, and the message covers the total. A cancellation that lands while the slot is being taken no longer goes on to reserve a half-open circuit attempt: the gate checks the context after the slot phase, releases the slot, and returns the cancellation.
Member
Author
|
On the two suppressed findings in the Copilot review of c6e10bb, both addressed in 836c236:
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect cancellation safety, slot ownership, and error-chain preservation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
internal/output/errors.go:75
- This conversion drops the original
GateErrorfromCause, so the central conversion severs the sentinel chain:errors.Is(output.AsError(gateErr), basecamp.ErrRateLimited)is false even thoughGateError.Unwrappromises that match. Other SDK conversions preserve their cause, andoutput.ErrorsupportsUnwrap; retaingateErras the cause here.
return &Error{
Code: CodeRateLimit,
Message: gateErr.Message,
Hint: gateErr.Hint,
Retryable: true,
}
internal/resilience/gate.go:12
- The default rationale has incorrect rate math: a 50-token bucket refilling at 10 tokens/second takes about five seconds to refill, not “well under a second.” This understates the queueing time the 10-second bound is intended to absorb; please make the explanation quantitatively accurate.
// invocations (10 workers draining a 50-token bucket refill at 10/s in well
// under a second) and short enough that a genuinely saturated machine still
internal/resilience/hooks.go:110
- After the post-bulkhead cancellation check, the gate calls
Allow()without checkingctx.Err()again (and there is no such check whenbulkheadis nil). A cancellation in this window can reserve a half-open circuit attempt while the gate still returns success, so a canceled operation may proceed and the reservation can remain outstanding if the SDK skipsOnOperationEnd. Check cancellation immediately before the circuit reservation and release any acquired bulkhead slot on that path.
// A cancellation that landed while the slot was being taken must not
// go on to reserve a half-open attempt.
if err := ctx.Err(); err != nil {
_ = h.bulkhead.Release()
return ctx, err
}
// Store marker in context so OnOperationEnd knows to release the slot
ctx = context.WithValue(ctx, releaseKey{}, true)
internal/resilience/rate_limiter.go:137
- The context/deadline check only happens before
take, buttakeperforms a context-independentStore.Updateand can wait up toLockTimeoutbefore committing the token. If cancellation or the gate deadline arrives while that update is blocked,allowedis true and this returns success anyway; with a bulkhead phase, the token is already consumed before the next phase rejects. Make the reservation conditional on a still-valid context/deadline at commit, or otherwise roll it back safely.
allowed, wait, blocked := rl.take() //nolint:contextcheck // lock acquisition is context-independent by design
if allowed {
return nil
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
This was referenced Sep 13, 2026
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.
What
OnOperationGatequeues instead of rejecting: the rate limiter waits for the refill it needs and the bulkhead polls (25–37ms, jittered) for a slot, within one shared 10s budget (Config.MaxWait,DefaultMaxWait) before failing.Retry-Afterblocks are still honoured: a block that lifts within the budget is waited out; one that outlasts it is reported immediately with the remaining time rather than burning the budget.CircuitBreaker.Tripped, a read) before queueing for a token it would only be refused with; the reserving check still runs last.resilience.GateErrorcarrying the message and hint the user sees; it unwraps to the SDK sentinel (ErrRateLimited/ErrBulkheadFull) soerrors.Ischecks, therate_limitcode and exit 5 are unchanged.output.AsErrorconverts it centrally (output.AsGateError), so commands that return SDK errors straight to the root render it the same way as theconvertSDKErrorhelpers. Thefilesauto-detection probes no longer dereference a nilbasecamp.AsErroron a non-SDK first error.Allow()/Acquire()keep their non-waiting semantics for callers that want them.Why
The smoke test card "CLI bulkhead hard-fails parallel invocations: 10 of 80 calls survive at 10 workers" ran the CLI as 10 parallel workers × 8 sequential calls: 10 of 80 calls succeeded and the server never answered 429. The failures were the CLI's own gate.
Mechanism, from a re-exec'd reproduction of the same shape against a temp state dir (each child is one CLI invocation through the real
GatingHookswithDefaultConfig):54 = the 50-token bucket plus 0.46s of refill at 10/s; every rejection is the rate limiter, none the bulkhead (each worker's previous process is dead by the time its next one starts, so the PID table never fills). The token bucket is shared by every invocation on the machine through
state.json, and the gate failed the instant it was empty. A real invocation spends several gated operations (account resolution, the command itself, pagination), which is how a 50-token bucket comes out as "10 of 80": the first wave drains it and everything after starves, each failing invocation having already consumed tokens for the operations it completed before the one that failed.The SDK gives the hook exactly one call per operation and returns its error straight to the command (no retry, no backoff), so the CLI's hook is the only place waiting can happen.
Before / After
Before — a worker's later call, with the shared bucket already empty:
After — the same 80 calls all complete (the rate limiter paces the tail at 10/s, so the run takes ~3s instead of failing in 0.5s):
After — when a gate does give up (15 workers pinned against 10 slots for longer than the budget):
and for the rate limiter:
Too many requests (client limit 10/s); waited 10s/Re-run, or lower parallelism.; for a server block that outlasts the budget:Rate limited by the server; retry after 42s/Wait 42s, then re-run.Borrowed from Codex
Nothing here — Codex has no cross-process client-side gate; its rate limiting is the server's, retried with backoff inside the request loop, which is the same "wait, then fail with a reason" shape this adopts for the CLI's own gate.
Testing
Multi-process tests in
internal/resilience/gate_test.gore-exec the test binary (TestHelperProcess) so each invocation is a real process with its own PID against a shared temp state dir:TestGateQueuesTenParallelWorkersThroughTheDefaults— the smoke-test shape, 10 workers × 8 sequential calls throughDefaultConfig: 80/80 succeed, peak live holders ≤ 10, under the 10s budget. Fails onmainwith 26 rate-limiter rejections.TestGateQueuesOversubscribedInvocationsWithinTheSlotLimit— 15 simultaneous invocations holding 150ms each against 10 slots: 15/15 succeed, no child ever sees more than 10 holders, wall time ≥ 2 × hold (the overflow waited), slot table empty afterwards.TestStoreLockContentionDoesNotLoseReleases— 12 processes churn acquire/release under the lock, then stay alive while the parent checks the slot table is empty (a leaked PID would otherwise be swept as dead). Verified to fail with the lock budget squeezed to 1ms; at the old 100ms it passed on this machine, so the raise is margin, not the smoke-test mechanism.TestStoreLockTimeoutCoversAHeldLockholds the store lock from a second file description for 150ms: with the 2s budget anUpdatewaits for the holder; with a 20ms budget it falls open before the holder releases.RateLimiter.Wait(refill wait, give-up message with the effective request rate, short Retry-After waited out, long Retry-After reported at once and rounded up, jitter kept inside the budget, context cancellation without consuming a token, expired deadline without consuming a token),Bulkhead.Wait(queues until a slot frees, give-up message, cancellation and expired deadline without reserving), the hooks' shared budget with no leaked slot, fail-fast on an open circuit without spending a token, cancellation answered before the circuit,GateErrorunwrapping, and bothconvertSDKErrorandoutput.AsErrorcarrying the gate message/hint with the rate-limit code andretryable.Gates:
make fmt-check vet lint(0 issues),go test -tags dev ./internal/...,-race -count=3oninternal/resilience,make check-surface check-skill-drift,make test-e2e check-naming check-bare-groups check-lint-lockstep check-smoke-coverage provenance-check tidy-check.make checkstops atmake testin this environment on pre-existing pty-dependent tests (internal/stdinarg,internal/tui/resolve, and their callers) that fail identically on a pristined120b3acheckout here (/dev/ptmxis not a terminal in this shell); they do not depend on the changed packages andmainCI is green.