Skip to content

fix(credential-proxy): stop one sandbox from exhausting the shared replay table - #3281

Merged
freeqaz merged 6 commits into
mainfrom
fix-2231
Oct 23, 2026
Merged

freeqaz merged 6 commits into
mainfrom
fix-2231

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The Credential Proxy keeps every request ID it has seen for 30 seconds, so a replayed request is refused. That table is one in-memory pool of 4096 entries for all Sandboxes. resolve reserved the ID before the installation lookup and route classification, and entries leave only by age. Requests refused at those checks (for example an unsupported operation) therefore still held an entry for the full window. When the table was full, every Sandbox's protected requests were refused with unavailable/resolver, so one Sandbox's traffic could stop protected egress for every other Agent. The bound also capped the whole API process at about 136 protected requests per second, which was not documented.

Change

  • Reserve later. resolve now reserves the request ID after the installation and route checks, which have no side effects (a state read and pure classification), and before exchange capture, the policy check and any Secret read. A request refused before that point uses no entry, and its ID was never used. A known replay is still refused before any lookup. The reservation checks the table again in the same turn, so two concurrent requests with the same ID cannot both proceed. A request whose lookup outlasts its deadline is refused before reserving. From exchange capture on, the ID stays reserved for the whole 30-second window, as before, even if the request is then refused.
  • Per-Sandbox share. Entries are counted per authenticated sender and Sandbox. One Sandbox may hold at most a quarter of the table. The table grows from 4096 to 8192 entries, so the share is 2048 IDs per 30 s (about 68 protected requests per second per Sandbox). The existing sustained-traffic test runs one Sandbox at 40 requests per second, and model and Git traffic from a normal Agent is far below that. A quarter of the old 4096 entries would have refused that test. The whole table now allows about 270 per second per API process, at a memory cost of a few MB. Before refusing at either bound, resolve drops entries whose window ended while the installation lookup was pending. Past either bound, it refuses with the existing unavailable/resolver, with no further detail.
  • Docs. A new "Request-time capacity" table in the Credential Proxy reference lists these bounds and the 1024 in-progress operation bound. It states that four busy Sandboxes can still fill the shared table. The request flow doc now says when the ID is reserved.

These are provisional decisions, made by precedent; a maintainer may overrule them. The choices are the share identity (sender and Sandbox), the quarter share and the 8192 table size.

Not changed here, and filed as follow-ups:

  • The 1024 in-progress operation bound is still shared. It is charged before authentication, when only the unauthenticated Sandbox parameter is known.
  • The OpenShell authority's 1024 pending exchange openings are also shared.
  • There is no per-Namespace share, so several Sandboxes in one Namespace can still fill the table together.

Verification

  • New tests in tests/integration/static-egress.test.mjs, using a new sandboxes() helper on the shared syntheticDispatchFixture:
    • requests refused before capture leave the replay table to every Sandbox: a full table's worth of refused requests from one Sandbox, after which another Sandbox's first request and the same Sandbox's next request are both accepted. An ID refused before capture can still be used. With the table size set to 4096, this test fails on main ("another Sandbox's first request").
    • one Sandbox's replay share bounds it without refusing other Sandboxes: a Sandbox at its share is refused while another Sandbox is accepted. Four full shares still hit the shared bound. Both bounds clear after the 30-second window, including when the window ends while the request's installation lookup is pending.
    • a request ID stays reserved once policy or Secret work can start: after a policy refusal and after a ready resolution, the same ID is refused. A known replay does no lookup. Concurrent duplicates both pass the early check, then yield exactly one ready resolution and one Secret read. A lookup that outlasts the deadline is refused with deadline_exceeded and starts no policy check.
  • Nine mutants are killed, each by at least one new test: reserve before the installation lookup; reserve after the policy check; no per-Sandbox cap; no shared cap; no re-check at reservation; no early replay check; no deadline re-check after the lookup; no prune before refusing at a bound; no share release on expiry.
  • The new tests pass 20 of 20 runs, each run pinned to 2 CPUs.
  • static-egress.test.mjs and credential-proxy-peer-pin.test.ts pass locally, including the existing replay, horizon, capacity and sustained-traffic tests.
  • tsc --build passes, and prettier and eslint pass on the touched files.
  • An internal review focused on replay safety and tenant isolation found no must-fix. It found no path where a request without a reserved ID reaches capture, the policy check, a Secret read or a send. Its two should-fixes are applied: the deadline re-check after the lookup, and the documented multi-Sandbox residual. The automated review's three items are also applied: the prune before refusing at a bound, the release rule for in-progress operations, and the no-reservation boundary in the reference.

Risks

  • An ID refused at the installation or route checks can now be resubmitted. Nothing ran for it: no capture, policy, Secret read or receipt.
  • The table is twice as large, so pruning scans up to 8192 entries per call, still a small constant cost.
  • A Sandbox that sends more than about 68 protected requests per second for a sustained period is now refused before the shared bound is reached. The reference documents this limit.

… each Sandbox's share

Requests refused by the installation or route checks no longer take a replay key,
and one Sandbox can hold at most a quarter of the replay table, so it cannot
refuse protected requests for every other Agent.
@freeqaz
freeqaz requested a review from a team as a code owner October 23, 2026 06:44
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 23, 2026
…dline before reserving

Internal review: the installation await now sits between the deadline check and the
reservation, so recheck the clock there. Document the multi-Sandbox residual and the
identity's accepted-exchange bound; assert both duplicates pass the early check.
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 23, 2026
@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR delays replay reservation until installation and route validation, adds a 2048-ID per-Sandbox share, and enlarges the shared table to 8192 IDs.

Example: Sandbox A submits 4096 unsupported requests within 30 seconds, then Sandbox B submits a valid protected request.

  • Before: A's refused requests fill the shared replay table, so B receives unavailable/resolver.
  • After: Unsupported requests reserve no replay entries, so B can resolve normally.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The focused patch resolves every prior finding and has meaningful resolver coverage, while evidence for the changed scenarios remains harness-level.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored PR is outside the ordinary contributor proof gate; supplied verification exercises CredentialProxy directly with synthetic identity, policy and Secret dependencies rather than an installed transport.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: One Sandbox can exhaust replay capacity and prevent other Agents from resolving protected requests.
Reason: The collaborator-authored direction repairs a concrete isolation failure within the existing Credential Proxy owner and explicitly records its capacity tradeoffs.

Merge readiness

✅ Ready for maintainer review

All three prior findings are resolved, and no new actionable defect was found; this PR still addresses replay-table exhaustion present on current main.

Priority: P2
Reviewed head: a30f855572520c3891d56befca319211b16534b6

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: The new multi-Sandbox exhaustion and expiry scenarios are not shown through the shipped authenticated transport; base/head failures and repeated passes are author-reported.
Agent review details

How this fits together

The OCC Credential Proxy receives authenticated Sandbox requests, validates their installed route, reserves replay capacity, checks current authority, and selects credential material before separate send authorization.

flowchart TB
A[Authenticated Sandbox request] --> B[Installation and route checks]
B --> C[Replay reservation and Sandbox quota]
C --> D[Exchange custody and policy check]
D --> E[Credential material selection]
E --> F[Separate send authorization]
Loading

Technical review

Best possible solution:

Land the focused replay-accounting repair while retaining separate ownership of the acknowledged shared-operation and multi-Sandbox exhaustion limits.

Do we have a high-confidence way to reproduce the issue?

Current-main source deterministically retains IDs for unsupported requests until the shared 4096-entry table fills; tests were inspected but not executed during this read-only review.

Is this the best way to solve the issue?

The repair uses the existing replay owner, reserves atomically before effectful work, preserves duplicate rejection, and releases expired accounting before enforcing either limit.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against eea5257b9957.

Provenance checked

  • Replay reservation timing and capacity changes intended behavior with a stated reason (unknown: Main blame carries the existing behavior through Credential Proxy Implementation #2487; its inline comment reserves before installation and policy I/O to prevent failed attempts reusing IDs. Earlier introduction could not be established because historical-object reads failed locally.)
  • Request-flow documentation changes intended behavior with a stated reason (unknown: The existing flow describes reservation before asynchronous authorization; this PR explains and implements the narrower pre-capture boundary while retaining replay protection before policy and Secret access.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

  • Prior expiry finding resolved: Reservation now prunes expired entries after the installation lookup before refusing at either bound; the added scenario crosses the old entries' expiry during that lookup. (packages/occ/src/credential-proxy.ts:255, a30f85557252)
  • Prior documentation findings resolved: The capacity table retains active charges while dependent work is pending, and the no-reservation statement now names installation and route checks rather than all checks before policy. (docs/reference/drivers/credential-proxy.md:194, a30f85557252)
  • Current main retains the reported failure: Current main reserves each authenticated request ID before installation lookup and route classification, with one shared 4096-entry bound and no Sandbox share. (packages/occ/src/credential-proxy.ts:280, eea5257b9957)
  • Distinct resolver regressions covered: The added tests exercise unsupported-request exhaustion, per-Sandbox and shared limits, expiry during lookup, concurrent duplicates, policy refusal retention, and deadline expiry through the real resolver with synthetic dependencies. (tests/integration/static-egress.test.mjs:1658, a30f85557252)
  • Maintainer direction and capacity tradeoff: The collaborator-authored PR explicitly selects sender-plus-Sandbox accounting, an 8192-entry table and a 2048-entry share, acknowledges the lower per-Sandbox rate and remaining shared bounds, and requests re-review of the corrections. (a30f85557252)
  • Feature history and original intent: Main history and blame associate the resolver with the credential-proxy work in Credential Proxy Implementation #2487, whose description assigns protected delivery enforcement to Credential Proxy. Earlier historical-object reads were unavailable locally, so the original introduction is not asserted. (packages/occ/src/credential-proxy.ts:283, eea5257b9957)

Review metrics

Metric Value Why it matters
Change distribution Production +63/-8; tests and helpers +183/-1; documentation +22/-3 Most growth covers distinct replay and quota failures through the existing resolver and fixture.

Labels

Label changes:

  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: The change repairs cross-Sandbox protected-egress exhaustion on a scoped, explicitly qualified execution surface.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • For stronger confidence, demonstrate one Sandbox reaching its replay share while another completes a protected request through the authenticated transport.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (1 earlier review cycle)
  • reviewed 2026-10-23T07:12:31.505Z sha d4a1ba0 :: needs changes before merge. :: [P2] Prune expired replay entries before enforcing the Sandbox share | [P3] Describe active capacity as retained until pending work settles | [P3] Limit the no-reservation claim to checks before capture

Reviewed October 23, 2026, 3:43 AM ET / 07:43 UTC (Revision 2).

…t a bound

Review: entries can expire while the installation lookup is pending, so a full share
or table is pruned again before refusing. Correct the capacity table's release rule
and the no-reservation boundary (installation and route checks, not policy).
@freeqaz

freeqaz commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

Head a30f855 addresses the three items: expired replay entries are pruned before refusing at either bound (new test covers a window that ends during the installation lookup), the capacity table's release rule for in-progress operations is corrected, and the reference now says only installation and route refusals reserve no ID.

@clawsweeper

clawsweeper Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 23, 2026
@freeqaz
freeqaz merged commit 6fa1aa0 into main Oct 23, 2026
67 checks passed
@freeqaz
freeqaz deleted the fix-2231 branch October 23, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant