Repository navigation
fix(agent-sessions): quota refusal delivery + storage-measurement generation CAS - #2253
2witstudios wants to merge 3 commits into
Conversation
… an enum code
The plan-limit refusal had a carefully-worded message that no user could
ever see, and a delivery path that showed nothing at all.
1. `{ error: detail ?? SESSION_QUOTA_MESSAGE }` never fell through.
`detail` is `quota.reason` — `'concurrency_limit'`, always set — so
both human strings on this path were unreachable and a user over
their limit received `{"error":"concurrency_limit"}`. The socket
surface printed the real sentence for the identical refusal: the
three-ways-to-say-one-thing the module was extracted to prevent.
`detail` is now what it always was, a diagnostic code, recorded in
the audit row and never in the body.
2. The provisioner's dead prose fallback becomes a code, so no caller
can read `detail` as user copy again.
3. `handleAddShell` was try/finally with no catch, so the 429 rejected
out of an onClick as an unhandled rejection: spinner stopped, no tab,
no explanation. A limit the product enforces but never states is
indistinguishable from a bug. Both shell handlers now surface the
failure via the toast convention already used in this directory.
The copy had a test; the path to it did not. Added: the response body
is the human sentence whatever code the provisioner names, the code
still reaches the audit trail, the button re-enables after a failure,
and both shell handlers report rather than swallow. Verified by
mutation — restoring `detail ??` fails two of them.
Found by re-reading the files no automated reviewer has seen: Codex
reviewed 36 commits ago and CodeRabbit skipped this PR entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
…ration
Reported by Codex on the re-review. Confirmed, reproduced against real
Postgres, and fixed.
`recordStorageMeasurement` wrote on `(conversationId, spriteTornDownAt
IS NULL)` — no generation guard, in a store where every other write
CASes. The measurement is fire-and-forget, so a `du` against generation
A can still be running when A is torn down and B provisioned. B's own
stamps clear `spriteTornDownAt` and null the measurement columns
(plan-session-lifecycle.ts:218), so A's late write finds a live row,
lands A's bytes on B, and stamps a fresh `storageMeasuredAt` that
suppresses B's real measurement for the whole throttle window while the
reconcile bills B's interval against A's disk.
The guard has to be `spriteInstanceId`, not `sandboxId`: the name is
HMAC-derived from the session and is identical across generations.
`SandboxHandle` already says so — "anything that must act on THIS VM
(a kill, a CAS against a tracking row) keys on this".
The interface comment claimed this protection ("would bill the next
generation against a dead disk") while the predicate delivered it only
for the never-revived case.
- store: CAS on `spriteInstanceId`, `eqOrIsNull` so a driver reporting
no instance id still matches its own null row.
- the instance travels with the bytes through the pure measure module
to all four call sites (web create + warm, realtime create + resume);
the required field is what surfaced them.
- the in-memory fake honours the CAS — a fake that accepts writes the
real store rejects makes every test using it agree with the bug.
Tests: three integration cases on real Postgres — stale generation
dropped (bytes AND timestamp), same generation persists despite a reused
NAME, null-instance driver still persists. Verified by mutation:
removing the predicate fails the stale-generation case and nothing else.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
📝 WalkthroughWalkthroughStorage measurements now carry Sprite instance identifiers and use generation-scoped persistence guards. Quota responses separate public messages from audit diagnostics, and agent shell failures now produce toast feedback with tested loading-state recovery. ChangesGeneration-scoped storage measurements
Quota error response handling
Shell operation error feedback
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentView
participant ShellSession
participant SonnerToast
AgentView->>ShellSession: addShell()
ShellSession-->>AgentView: error
AgentView->>SonnerToast: error title and description
AgentView->>AgentView: reset isAddingShell
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 82aacbfa8e
ℹ️ 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".
| // If a teardown + re-provision lands while the walk runs, the row's | ||
| // instance moves on and the write is dropped rather than billing the new | ||
| // Sprite for the old one's disk. | ||
| spriteInstanceId: row.spriteInstanceId ?? null, |
There was a problem hiding this comment.
When a tool finishes on generation A while that session is torn down and reprovisioned as B before this function reads the row, row.spriteInstanceId is B even though input.attach() still returns the already-acquired sandbox A (see sandbox-tools-runtime.ts, where the closure executes on the captured sandbox). The resulting du reads A but persists with B's instance ID, so the new CAS succeeds against B and still records A's bytes and throttle timestamp on the replacement generation. Carry the captured sandbox's spriteInstanceId through attach and CAS using that measured handle identity instead.
Useful? React with 👍 / 👎.
… row's Codex, reviewing the previous commit: the CAS was only as good as the id handed to it, and I handed it the wrong one. `measureWarmSessionStorage` read `spriteInstanceId` off the session row. But this path is fed a sandbox the tool run ALREADY acquired, and the measurement is fire-and-forget — so between acquiring the handle and reading the row, the session can be torn down and re-provisioned. The row then says generation B while `du` walks A's disk, and the CAS "succeeds" against B carrying A's bytes and A's throttle timestamp: precisely the write the CAS was added to reject, waved through by its own guard. `ExecutableSandbox extends SandboxHandle`, so the acquired sandbox knows its own generation. `attach()` now returns it alongside `exec`, and the CAS uses that — the identity of the disk actually measured. The row is still read, but only for the throttle, which is all it can honestly say. Same correction on the realtime resume path: CAS on the fetched sandbox's instance, not the row's. There the two usually agree, since the sandbox is fetched right after the row — but "usually" is what a CAS is for. Test: the seam supplies the ACQUIRED sandbox's instance through `attach`. It was previously mocked wholesale, so nothing asserted what the seam passed — which is where this hid. Mutation-verified: pinning the supplied id to null fails it and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Re: [P1] Use the measured sandbox's instance for the CAS — correct, fixed in
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Point-guard triage (post-#2258 audit swarm): this branch predates the restore merge and edits files #2258 deleted (
Recommend closing this PR once the re-landed equivalents merge; keeping it open until then as the reference for the mutation-verified test matrix. |
…ing, sidebar polish Fixes the follow-up audit findings from PR #2258 (issue #2264), plus a live P1 the point guard surfaced from stale PR #2253. - useResolvedConversation: gate first resolution on authLoading === false. On a hard refresh, useAuth starts out loading, so canUseSessions reads false before the role is known — resolving anyway would mint a fresh agent's first conversation as permanently session-less (thread→session binding is congenital). A startedForAgentId ref ensures this only waits out the FIRST load, so a later auth-loading blip (token refresh) never restarts resolution and clobbers a conversation the user is in. - Split spawn refusals into quota (429, worth interrupting the user for — they have the capability and simply ran out of allowance) vs capability (everything else — the existing silent degrade to a plain conversation). New spawn-refusal.ts pure module + ApiRequestError (auth-fetch.ts) to carry the status code through the throw. - quota-response.ts P1: `{ error: detail ?? SESSION_QUOTA_MESSAGE }` used `detail` for both a diagnostic enum (quota.reason, e.g. 'concurrency_limit', ALWAYS set on denial) and an occasional human override — so the enum always won and users saw 'concurrency_limit' rendered as their error message, with the human sentence unreachable. Split into {reasonCode, message}: reasonCode goes only into the audit row, the response body is always human-worded. Addresses the quota-delivery half of PR #2253. Verified the new UI's 429 consumers (AgentPanes.tsx handlePickShell, AgentsSidebar.tsx spawn/newConversation/ endSession) already catch and toast the rejection — no repeat of the deleted AgentView.tsx handleAddShell's silent-spinner bug. - usePermissionsCheck: reset isReadOnly at effect start so a page change doesn't inherit the previous page's read-only verdict when its own check errors. - AgentsSidebar: extracted session-groups.ts (groupSessionsByDrive) — the Assistant group now sorts first deterministically instead of depending on Map insertion order during the sessions fetch. Status dot gets role="img" so it's announced by screen readers (a bare aria-label span is not). Coordination note: useResolvedConversation now takes an authLoading param; AgentPageView.tsx (owned by the pane-system agent, issue #2263) got the smallest possible touch — destructuring isLoading from useAuth and passing it through — per the task's stated seam. Closes #2264 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WZ55BHXrze46tyZDA9eNhU
|
Superseded in full: the quota-delivery fix re-landed in #2270, and the generation-CAS + handle-sourced instance id re-landed in #2274 (adapted to the post-#2258 session-keyed model, same mutation-verified test matrix). #2254 fixed by #2274; #2255 was closed earlier as superseded by the pane-system restore (#2268 added shell reattach). Nothing from this branch remains unlanded. |
1 participant
Follow-up to #2249, which merged at 14:47 UTC today while a review pass was still running. These fixes were verified after the merge, so they never landed with it. All are on code #2249 introduced.
Two of the three are P1s reported by Codex on post-merge re-reviews — the second confirmed and reproduced against real Postgres, the third a defect Codex found in my fix for the second.
1. The plan-limit refusal reached the user as an enum code
sessionQuotaExceededanswered{ error: detail ?? SESSION_QUOTA_MESSAGE }.detailisquota.reason—'concurrency_limit', and always set — so the??never fell through: both human strings on that path were unreachable and a user over their sandbox limit received{"error":"concurrency_limit"}rendered straight into the product, while the socket surface printed the real sentence for the identical refusal. That is the "one event should not read three different ways depending on which layer said no" the module was extracted to prevent.
Compounding it,
handleAddShellwastry/finallywith nocatch, so the 429 rejected out of an onClick as an unhandled rejection — spinner stopped, no tab, nothing said. A limit the product enforces but never states is indistinguishable from a bug.detailis now what it always was: a diagnostic code, recorded in the audit row, never in the body.detailas user copy again.The copy had a test; the path to it did not. Added: the body is the human sentence whatever code the provisioner names, the code still reaches the audit trail, the button re-enables after failure, and both handlers report rather than swallow. Mutation-verified — restoring
detail ??fails two of them.2. [P1] Storage measurements could bill the next Sprite generation for the previous one's disk
recordStorageMeasurementwrote on(conversationId, spriteTornDownAt IS NULL)— no generation guard, in a store where every other write CASes.The measurement is fire-and-forget, so a
duagainst generation A can still be running when A is torn down and B provisioned. B's own stamps clearspriteTornDownAtand null the measurement columns (plan-session-lifecycle.ts:218), so A's late write finds a live, freshly-reset row: it lands A's bytes on B and stamps a freshstorageMeasuredAtthat suppresses B's real measurement for the whole throttle window while the reconcile bills B's interval against A's disk.The guard has to be
spriteInstanceId, notsandboxId— the name is HMAC-derived from the session and is identical across generations, so a CAS on it would have looked like a fix and caught nothing.SandboxHandlealready states the rule: "anything that must act on THIS VM (a kill, a CAS against a tracking row) keys on this."The interface comment claimed this protection ("would bill the next generation against a dead disk") while the predicate delivered it only for the never-revived case.
spriteInstanceId, viaeqOrIsNullso a driver reporting no instance id still matches its own null row.{measuredBytes, measuredAt}and would have dropped it silently.3. The CAS was reading its generation from the wrong place
Codex, reviewing commit 2 — and correct. A CAS is only as good as the id handed to it, and I handed it the row's.
measureWarmSessionStoragereadspriteInstanceIdoff the session row. But this path is fed a sandbox the tool run already acquired, and the measurement is fire-and-forget — so between capturing that handle and reading the row, the session can be torn down and re-provisioned. The row then names generation B whileduwalks A's disk, and the CAS succeeds against B carrying A's bytes and A's throttle timestamp: the exact write the CAS was added to reject, waved through by its own guard.I had considered this window in commit 2 and called it a residual. It isn't residual on this path — the handle is captured before the row read, so the mismatch is the normal ordering, not a rare interleaving. A guard that is wrong in the common case is worse than none, because it reads as covered.
ExecutableSandbox extends SandboxHandle, so the acquired sandbox already knows its own generation — no contract widening, just carrying what was there:attach()returns{ exec, spriteInstanceId }; the CAS uses the handle's id — the identity of the disk actually measured.All five measurement call sites now source the id from a handle; none from a row. Create-arm ordering re-checked:
updateSpriteIdentityis confirmed (recorded === true) before measurement fires, so tightening the predicate does not drop the baseline measurement.Validation
typecheck,lintclean across the monorepo.packages/libagent-sessions + sandbox: 928 tests.apps/realtime: 937.apps/webagent-sessions + agents + tools: 1530 passing (1 unrelated pre-existing failure,activity-tools, which needs a test DB and fails onFailed queryin any local env).detail ??fails two quota tests; removing the CAS predicate fails the stale-generation case; pinning the supplied instance id to null fails the seam test. Each fails that test and nothing else.The timestamp assertion is the one worth keeping: a stale
storageMeasuredAtsilences the next real measurement for a full window without ever recording a real number, so dropping only the bytes would leave half the bug.Review coverage
Stated explicitly because #2249 taught the lesson: a green bot check is not evidence of review.
9a0d8f634) — no issues. Its two findings on earlier commits are fixed and left open for verification.Still open from the Codex re-review (not in this PR)
Two findings against merged code, now filed so they outlive this PR:
0234_phase8_teardown_machines_world.sql— the four rescueINSERTs useON CONFLICT DO NOTHING, while the 0209/0219/0229 triggers useDO UPDATE SET spriteInstanceId = COALESCE(...)precisely because "a newer generation took this name; the pointer must chase the VM that is actually alive now." If the outbox already held thatsandboxIdfrom an older generation, the rescue keeps the stale instance id while the migration drops the current generation's tracking row;agent-session-orphan-reconcile-runtime.ts:132then returns{ok: true}onSandboxSpriteReplacedError, deleting the outbox row and orphaning a live Sprite. This migration is already on master, so the fix is not an edit to it — needs a decision between a corrective migration and hardening the reconciler's replaced-instance handling for outbox rows, which is where the host's own comment says the safe behaviour is to keep the pointer.useSessionShells.ts— agent-invokedspawn_shell/kill_shellmutate shell rows without touching this SWR cache, which has no polling or socket invalidation and disables focus revalidation, so agent-created shells never appear as tabs.🤖 Generated with Claude Code
https://claude.ai/code/session_017V3eqwRX5cFy3Tdu2Syoro
Summary by CodeRabbit
Bug Fixes
Tests