feat(lifecycle): bound concurrent tool-recall sessions by logical lane (#820) - #2611
Conversation
A parallel subagent fan-out is Codex's normal shape, and every child of one parent carries the SAME x-codex-parent-thread-id - that is exactly what codexPoolAffinityKey keys on so a fan-out pins to one account. A lane derived the same way inherits that coalescing, so the second and third sibling of any fan-out were rejected with 503. Reproduced before fixing: two thread_spawn requests differing only in thread-id resolved to one lane and the second tryAdmitTurn returned null. A lane wants the MOST specific identity, which is the opposite of what affinity wants. Keyed on the pair, the parent qualifies the lane rather than defining it: siblings separate, while two overlapping turns of one conversation still share a lane - the protocol rule this boundary exists to enforce. Both components are already fixed-size digests, so retained lane bytes stay bounded. Falsified: restoring the parent-first derivation fails the new test with 'Expected 3 distinct lanes, received 1'.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughChangesSession-aware turn admission now derives opaque lane IDs from request metadata, rejects overlapping turns and excess active lanes, tracks bounded metrics, and cleans lanes on lease release. HTTP, sideband, and WebSocket paths propagate the lane ID. A recall harness validates isolation, cleanup, memory bounds, and capacity. Session Lane Admission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestLog as sessionLaneIdFromRequest
participant Server
participant Lifecycle as lifecycle.tryAdmitTurn
participant Lease as ActiveTurnLease
Client->>Server: HTTP or WebSocket request
Server->>RequestLog: request headers
RequestLog-->>Server: session lane ID
Server->>Lifecycle: admit turn with lane ID
Lifecycle-->>Server: lease or 503 server_busy
Server-->>Client: response or stream
Server->>Lease: release
Lease->>Lifecycle: clear active lane
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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: 2cb088e906
ℹ️ 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".
| const turnAdmissionLease = tryAdmitTurn(ws.data.sessionLaneId); | ||
| if (!turnAdmissionLease) { |
There was a problem hiding this comment.
When a second response.create arrives while the prior frame is still running, line 1651 aborts the prior turn but its lease is released only later in the asynchronous finally at line 1771. This immediate admission therefore finds the same lane occupied, returns server_busy, and leaves both the aborted original and rejected replacement without a result. Either transfer/release the prior lane before admitting its replacement or reject the new frame without first cancelling the active turn.
Useful? React with 👍 / 👎.
| const specific = thread ?? session; | ||
| if (parent && specific) return `${parent}\u0000${specific}`; | ||
| return specific ?? parent; |
There was a problem hiding this comment.
For parentless Codex Desktop requests, this selects thread-id and discards session-id, while also accepting either partial identity alone. The established contract in structure/08_openai-provider-tiers.md:21-44 treats those individual components as weak and derives identity only from the complete pair, so independent Desktop sessions that reuse a partial value can collapse into one lane and receive spurious 503 responses. Combine the complete session-id/thread-id pair and leave incomplete Desktop identities unbound.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
lidge-jun#820) (lidge-jun#2611) * fix(server): bound logical session turn lanes * fix(lanes): key a session lane on parent+thread, not the parent alone A parallel subagent fan-out is Codex's normal shape, and every child of one parent carries the SAME x-codex-parent-thread-id - that is exactly what codexPoolAffinityKey keys on so a fan-out pins to one account. A lane derived the same way inherits that coalescing, so the second and third sibling of any fan-out were rejected with 503. Reproduced before fixing: two thread_spawn requests differing only in thread-id resolved to one lane and the second tryAdmitTurn returned null. A lane wants the MOST specific identity, which is the opposite of what affinity wants. Keyed on the pair, the parent qualifies the lane rather than defining it: siblings separate, while two overlapping turns of one conversation still share a lane - the protocol rule this boundary exists to enforce. Both components are already fixed-size digests, so retained lane bytes stay bounded. Falsified: restoring the parent-first derivation fails the new test with 'Expected 3 distinct lanes, received 1'.
lidge-jun#820) (lidge-jun#2611) * fix(server): bound logical session turn lanes * fix(lanes): key a session lane on parent+thread, not the parent alone A parallel subagent fan-out is Codex's normal shape, and every child of one parent carries the SAME x-codex-parent-thread-id - that is exactly what codexPoolAffinityKey keys on so a fan-out pins to one account. A lane derived the same way inherits that coalescing, so the second and third sibling of any fan-out were rejected with 503. Reproduced before fixing: two thread_spawn requests differing only in thread-id resolved to one lane and the second tryAdmitTurn returned null. A lane wants the MOST specific identity, which is the opposite of what affinity wants. Keyed on the pair, the parent qualifies the lane rather than defining it: siblings separate, while two overlapping turns of one conversation still share a lane - the protocol rule this boundary exists to enforce. Both components are already fixed-size digests, so retained lane bytes stay bounded. Falsified: restoring the parent-first derivation fails the new test with 'Expected 3 distinct lanes, received 1'.
1 participant
Summary
Closes #820. #829 already bounded per-call, per-turn and per-transport budgets and added a 256-turn global gate, but admission leases were still keyed by
AbortController, so two overlapping recalls from one logical session were indistinguishable from two independent turns. There was also no harness that actually drove 32 or 64 concurrent recall sessions.This adds a fixed-size opaque session lane derived at ingress (HTTP and the WebSocket upgrade) and claimed atomically before the existing global permit. Independent lanes stay parallel up to the existing cap; an overlapping turn on one identified lane gets the existing structured
server_busyresponse. This is admission, not scheduling — no waiters, no queue memory, no scheduler, which is the part #820 deferred and this deliberately does not build.The lane registry stores fixed-size digests only, so retained metadata is bounded by the already-bounded active-turn population times a constant. Anonymous requests keep today's independent-turn behavior: guessing an identity would be less protocol-safe than leaving them uncoordinated.
src/server/lifecycle.tsgains no import ofsrc/lab/and noawaitenters the startup window;tests/core-lab-boundary.test.tsis green.The bug this PR was one commit away from shipping
The original lane derivation preferred
x-codex-parent-thread-id, mirroringcodexPoolAffinityKey. That is right for affinity and wrong for a lane: every child of a parallel subagent fan-out carries the same parent thread id, so all siblings collapsed into one lane and every one after the first got a 503. A parallel fan-out is Codex's normal shape, not an abuse.I reproduced it before changing anything — two
thread_spawnrequests differing only inthread-idresolved to one lane, and the secondtryAdmitTurnreturnednull.A lane wants the most specific identity available, which is the opposite of what affinity wants. Keyed on the pair, the parent qualifies the lane rather than defining it: siblings separate, while two overlapping turns of one conversation still share a lane, which is the rule this boundary exists to enforce. Both components are already fixed-size digests, so the bound is unaffected.
Verification
Harness measurements at the merge head — 32 sessions:
lanePeakBytes=1024; 64 sessions:lanePeakBytes=2048. Both waves return active lanes and retained lane bytes to zero.Falsification, all four:
Expected 64, received 256; the 65th lane wrongly got a leaseExpected retainedBytes 1024, received 0Expected status 503, received 400Expected 3 distinct lanes, received 1The memory oracle is the lifecycle-owned byte accounting, not RSS. RSS is recorded as an observation but is not a valid mutation oracle here, since allocator retention is nondeterministic — a test that asserted on it would pass or fail for reasons unrelated to the bound.
Checklist
devdevlog/_plan/260826_session_lane_bounds/010_design.mdawaitadded to the startup windowSummary by CodeRabbit
New Features
Bug Fixes
Tests