fix(session): admit ownerless continuation before selection - #4540
Conversation
|
@probepark Fresh independent exact-head review requested for the terminal #4519 fix-forward PR #4540. Please review exact head — |
![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: b0205ea727
ℹ️ 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 admissionSignal = options?.preflightSignal | ||
| ? AbortSignal.any([this.#promptPreflightAbortController.signal, options.preflightSignal]) | ||
| : this.#promptPreflightAbortController.signal; | ||
| await awaitPromptInvocationPreflight(this.#selectionFenceTail, admissionSignal); |
There was a problem hiding this comment.
When an SDK steer or follow-up waits here behind an active default-model selection and dispose() starts before that selection finishes, #closeSessionAdmission() does not abort this signal because the active admission is a selection. Once the fence resolves, execution continues into #queueSteer/#queueFollowUp without rechecking #sessionAdmissionClosing, #sessionAdmissionClosed, or #isDisposed, so the request is reported accepted and retained on an already disposed session where it cannot run. Recheck the session lifecycle after this await or ensure teardown cancels every fence waiter.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve - one of the two new tests reproduces an actual deadlock, which is the best kind of evidence
Reviewed b0205ea727fce986d835f477f2c5a856714139e8 against base f0453b6ab.
Fail-before / pass-after
# base f0453b6ab, with this head's two test files applied verbatim
$ bun test .../agent-session-auto-compaction-continue.test.ts \
.../agent-session-before-agent-start-attribution.test.ts
(fail) AgentSession auto-compaction continuation > does not deadlock default selection waiting ownerless emergency continuation [10020.48ms]
(fail) AgentSession before_agent_start attribution fallback > fences SDK prompt, steer, follow-up ingress behind earlier selection
35 pass 2 fail
# head b0205ea72
37 pass 0 fail
The first failure is a 10,020 ms timeout, not an assertion mismatch. That is the deadlock
itself reproducing - an ownerless emergency continuation waiting on selection admission that never
arrives. A test that hangs on the pre-fix code and completes on the fixed code is about as
unambiguous as regression evidence gets.
The second pins the ingress fence: SDK prompt, steer, and follow-up all queue behind an earlier
selection. That is the causal-ordering property #4524 established, extended to the ingress paths
that were not covered there.
Scope
agent-session.ts +26/-7 against +45 and +43 of test. A 26-line behavioural change carrying
88 lines of test that provably fail without it is the ratio I want to see on this file - it is the
same admission machinery where #4519 sat red for a while precisely because the ordering
invariants were under-pinned.
Digest 8680be026128ee3e451c117328227ed5823b4cde021bdbc26db8ebbd554654c3 recomputed independently,
matches the declared value.
Verdict
merge-approved.
Reviewed by @probepark - method: applied the head's test files to the base to prove the differential (including the 10s timeout reproduction), full-suite run at the exact head in a clean detached worktree with real bun install and rebuilt natives, independent digest recomputation.
b0205ea to
4434180
Compare
4434180 to
c3ccdde
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Re-approve at c3ccdde07 - three added lines, thirty-five lines of test that fail without them
Head moved since my approval of b0205ea72. Stripping the rebase noise from #4532/#4526, the real
delta is agent-session.ts +3 and one new test case.
The added guard
if (this.#sessionAdmissionClosing || this.#sessionAdmissionClosed || this.#isDisposed) {
throw this.#sessionAdmissionBusyError();
}Rejects fenced SDK ingress once admission is closing or the session is disposed, rather than
letting it queue behind a selection that will never complete. It reuses the same
closing/closed/disposed triple that #4459's admission work established, so the two are consistent.
Differential
# base f0453b6ab, with this head's two test files applied verbatim
(fail) AgentSession auto-compaction continuation > does not deadlock default selection waiting for an ownerless emergency continuation [10024.04ms]
(fail) AgentSession before_agent_start attribution fallback > fences SDK prompt, steer, and follow-up ingress behind an earlier selection
(fail) AgentSession before_agent_start attribution fallback > rejects fenced SDK ingress disposal drains earlier selection
35 pass 3 fail
# head c3ccdde07
38 pass 0 fail
Three failures against three behaviours, and the third one -
rejects fenced SDK ingress disposal drains earlier selection - is new since my last review and
maps exactly onto the three added lines. The deadlock case still reproduces as a 10-second timeout
rather than an assertion mismatch, which remains the clearest possible evidence.
$ bun --cwd=packages/coding-agent run check -> exit 0
Digest fa43e0ede1fa44a02cc5e7e02f37fc43691f45661c6b9c63a4fe3749e4408c2f recomputed independently,
matches.
Verdict
merge-approved at c3ccdde07.
This is the shape I have been asking for across this queue all night: a small behavioural change
landing with a test that demonstrably fails without it. Nothing further from me.
Reviewed by @probepark - method: separated real delta from rebase noise, applied the head's test files to the base to prove the differential, full-suite run and package typecheck in a clean detached worktree at the exact head, independent digest recomputation.
c3ccdde to
6684e86
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6684e86c96
ℹ️ 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 admissionSignal = options?.preflightSignal | ||
| ? AbortSignal.any([this.#promptPreflightAbortController.signal, options.preflightSignal]) | ||
| : this.#promptPreflightAbortController.signal; | ||
| await awaitPromptInvocationPreflight(this.#selectionFenceTail, admissionSignal); |
There was a problem hiding this comment.
When a plain SDK prompt is submitted while another turn is active behind an earlier model selection, this await lets the active turn finish and the selection publish before sendUserMessage continues, so the prompt ultimately starts a new turn from an idle session. However, the SDK host already captured queuedAtDispatch = true before calling this method (session-runtime.ts:1374-1385), so it does not add the submission to the pending-owner queue (session-runtime.ts:2760-2766). The resulting turn has no requester owner, causing that client's terminal abort to be rejected, and its reconciliation record is not advanced by lifecycle events. The ownership/queued decision must reflect the post-fence delivery mode rather than the stale pre-dispatch snapshot.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 6684e86c9 - five failures on base against five behaviours
Third head. Real delta is agent-session.ts +11/-3 plus +45 of test (the
kind-aware-reconciliation.ts +7/-1 is #4537 rebase noise).
# base f0453b6ab, with this head's two test files applied verbatim
(fail) ... > does not deadlock default selection waiting for an ownerless emergency continuation [10018.28ms]
(fail) ... > fences SDK prompt, steer, and follow-up ingress behind an earlier selection
(fail) ... > rejects fenced SDK ingress when disposal drains the earlier selection
(fail) ... > rejects SDK steer when disposal starts during durable acceptance
(fail) ... > rejects SDK followUp when disposal starts during durable acceptance
35 pass 5 fail
# head 6684e86c9
40 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
The two new cases since my last pass cover disposal starting during durable acceptance for steer
and followUp - the narrow race where the ingress has been admitted but not yet settled. That is the
gap I would expect someone to miss, and it is now pinned.
Every added line of behaviour in this PR has a test that fails without it. Nothing else from me.
Digest b3b6229f72b84cfaf003ae09f6262757ae118eb7c5d2513e68726155aab650ba recomputed independently,
matches.
Verdict
merge-approved at 6684e86c9.
Reviewed by @probepark - method: separated real delta from rebase noise, applied the head's test files to the base to prove the differential, full-suite run and package typecheck in a clean detached worktree at the exact head, independent digest recomputation.
6684e86 to
faa1c13
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa1c1330a
ℹ️ 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".
| { | ||
| bypassSelectionFence: true, | ||
| allowPromptContinuationReentry: true, |
There was a problem hiding this comment.
When retry() is invoked after setDefaultModelSelection() has reserved its fence but before credential preflight completes, it schedules #scheduleAgentContinue; this unconditional bypass lets that later retry proceed as soon as the selection's first admission releases. Because the selection then calls waitForIdle() before mutating the model, the retry runs on the old model and the requested selection commits only afterward. Record whether the continuation predates the pending fence and bypass only in that case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Re-approve at faa1c1330 - my previous approval went stale when the head moved
(Reposting: my first attempt at this review was mangled by a shell quoting mistake on my side and
posted with words dropped. This is the intended text.)
No re-review was requested, but approvals are exact-head bound, so 6684e86c9 -> faa1c1330
silently invalidated mine and the contract gate would have rejected it. Flagging that because it is
easy to miss: a moved head leaves an APPROVED review sitting on the PR that no longer counts.
The delta is a hot-path optimisation, and it is leak-safe
+ this.#pendingSelectionFences += 1;
this.#selectionFenceTail = priorSelectionFence.then(() => selectionFence.promise);
try { ... } finally {
selectionFence.resolve();
+ this.#pendingSelectionFences -= 1;
}- await awaitPromptInvocationPreflight(this.#selectionFenceTail, admissionSignal);
+ if (this.#pendingSelectionFences > 0) {
+ await awaitPromptInvocationPreflight(this.#selectionFenceTail, admissionSignal);
+ }The point is to stop paying an await on the prompt path when no selection is in flight. Previously
every admission awaited #selectionFenceTail even when it was an already-resolved tail, which
still costs a microtask hop and, more importantly, an opportunity for reordering.
The thing that would break this is counter drift, so I checked it specifically:
- The increment is synchronous and before the
try(line 13251,tryat 13254). Nothing
between them can throw in a way that skips the decrement - just two assignments and a
void ...catch(). - The decrement is in the matching
finally, so every throw path inside the transaction still
releases it.
So the counter cannot be observed as 0 while a fence is live, which is the only way the skip could
drop the ordering guarantee that #4524 and the earlier heads of this PR established.
Verification at the exact head
$ bun test packages/coding-agent/test/agent-session-before-agent-start-attribution.test.ts \
packages/coding-agent/test/agent-session-auto-compaction-continue.test.ts \
packages/coding-agent/test/agent-session-default-model-selection.test.ts
85 pass
0 fail
361 expect() calls
$ bun --cwd=packages/coding-agent run check -> exit 0
Digest 9e41a9c5a4a68a510b85867b27f2fd56c3655eeee343e6d8690a14158983db97 recomputed independently,
matches the declared value.
Verdict
merge-approved at faa1c1330adeb6db06c50519d78f5f8d11a6b6f9.
Reviewed by @probepark - method: staleness check of my prior approval against the current head, counter increment/decrement placement analysis for leak safety, three-suite run and package typecheck in a clean detached worktree at the exact head, independent digest recomputation.
|
Exact-head virtual-integration reconciliation for PR #4540:
Current verification:
— |
faa1c13 to
b1dd39b
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at b1dd39b75 - the boolean bypass became generation-scoped, which closes a hole I let through
Fourth head on this PR. My previous approval went stale again when the head moved.
The real change
- { bypassSelectionFence: true },
+ { bypassSelectionFenceGeneration: selectionFenceGeneration },plus a #selectionFenceGeneration counter incremented per fence, an AsyncLocalStorage context
carrying the generation, and a resolution chain at the ingress:
const selectionFenceGeneration =
this.#selectionFenceGenerationContext.getStore() ??
this.#sessionAdmissionContext.getStore()?.selectionFenceGeneration ??
continuationAdmission?.entry.selectionFenceGeneration ??
this.#selectionFenceGeneration;
if (this.#pendingSelectionFences > 0 && selectionFenceGeneration === this.#selectionFenceGeneration) {
const precedingSelectionFence = this.#selectionFenceTail;
void precedingSelectionFence.then(() => this.#scheduleAutoContinuePrompt(..., selectionFenceGeneration));
return;
}I approved the boolean version two heads ago and did not think hard enough about it:
bypassSelectionFence: true bypassed whatever fence happened to be current, including a fence
created after the continuation was scheduled. A continuation that legitimately needed to skip
fence N would also skip fence N+1 belonging to an unrelated later selection - the exact ordering
violation this PR exists to prevent, reachable through its own escape hatch.
Scoping the bypass to the generation it was issued for means it can only skip the fence it was
granted against; a newer fence still applies. The equality check
(selectionFenceGeneration === this.#selectionFenceGeneration) rather than a >= comparison is
the right choice - a stale generation falls through and waits instead of bypassing.
Differential
# base 0f8624d17, with this head's two test files applied verbatim
(fail) ... > does not deadlock default selection waiting for an ownerless emergency continuation [10019.42ms]
(fail) ... > keeps ownerless emergency continuation scheduled later behind selection [10020.03ms]
(fail) ... > fences SDK prompt, steer, and follow-up ingress behind an earlier selection
(fail) ... > rejects fenced SDK ingress when disposal drains the earlier selection
(fail) ... > rejects SDK steer when disposal starts during durable acceptance
(fail) ... > rejects SDK followUp when disposal starts during durable acceptance
35 pass 6 fail
# head b1dd39b75
134 pass 0 fail 514 expect() calls
$ bun --cwd=packages/coding-agent run check -> exit 0
Six failures against six behaviours, two of them 10-second deadlock timeouts rather than assertion
mismatches. The new one - keeps ownerless emergency continuation scheduled later behind selection
- is precisely the generation-scoping property, so the escape-hatch narrowing arrived with its own
test rather than as an unverified refinement.
Verdict
merge-approved at b1dd39b75.
This PR has been consistent about the thing I have been chasing across the whole queue tonight:
every head lands behaviour with a test that fails without it, and this one improved on something I
had already signed off. Nothing further from me.
Reviewed by @probepark - method: staleness check against the current head, generation-scoping analysis of the bypass resolution chain and equality-vs-comparison choice, applied the head's test files to the base to prove the differential, four-suite run and package typecheck in a clean detached worktree at the exact head, independent digest recomputation.
|
Exact bootstrap evidence for #4540: current head — |
b1dd39b to
2020341
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2020341409
ℹ️ 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".
| api.sendUserMessage( | ||
| typeof images === "undefined" ? text : ([{ type: "text", text }, ...(images as never[])] as never), | ||
| options, | ||
| queuedAtDispatch ? { ...options, deliverAs: "steer" } : options, |
There was a problem hiding this comment.
When this prompt is dispatched while another turn is busy and then waits behind a model selection, queuedAtDispatch remains true even if the predecessor exits without producing an assistant message (for example, a credential or preflight-hook failure). The prompt is consequently forced into #queueSteer after the session becomes idle, but #canAutoContinueForSteer() only schedules delivery when the history tail is an assistant; with no active loop, the SDK request is reported accepted and remains stranded. Fresh evidence in this revision is the new forced deliverAs: "steer"; delivery mode and ownership need to be determined from the post-fence state instead of the stale dispatch snapshot.
Useful? React with 👍 / 👎.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 202034140 - pins the prompt generation at defer time
Fifth head; my previous approval went stale when it moved. Delta is agent-session.ts +8/-1 plus
agent-session-manual-retry.test.ts +43/-1.
The change
- void precedingSelectionFence.then(() => this.#scheduleAgentContinue({ ...options, selectionFenceGeneration }));
+ const deferredPromptGeneration = options?.generation ?? this.#promptGeneration;
+ void precedingSelectionFence.then(() =>
+ this.#scheduleAgentContinue({
+ ...options,
+ generation: deferredPromptGeneration,
+ selectionFenceGeneration,
+ }),
+ );The deferred callback used to let #scheduleAgentContinue resolve #promptGeneration whenever the
fence happened to settle. Anything that bumps the generation in that window - an abort being the
obvious one - meant the continuation came back bound to the new generation and revived work for a
turn that had already been superseded.
Capturing it at defer time is the same discipline the previous head applied to
selectionFenceGeneration, now extended to the prompt generation. Both identities are pinned when
the deferral is created rather than read after an await, which is the correct rule for anything
crossing a suspension point in this class.
Differential
# prior head b1dd39b75, with this head's test file applied verbatim
(fail) AgentSession manual retry > does not retry deferred behind selection after abort
4 pass 1 fail
# head 202034140
91 pass 0 fail 382 expect() calls
(manual-retry + before-agent-start-attribution + auto-compaction-continue + default-model-selection)
$ bun --cwd=packages/coding-agent run check -> exit 0
The test name states the invariant directly, and it fails without the fix. That is the fifth
consecutive head on this PR to arrive with a discriminating test.
Digest recomputed independently and matches the declared value.
Verdict
merge-approved at 202034140.
Reviewed by @probepark - method: staleness check against the current head, defer-time identity-capture analysis across the suspension point, applied the head's test file to the prior head to prove the differential, four-suite run and package typecheck in a clean detached worktree at the exact head, independent digest recomputation.
|
Updated exact repair evidence: head — |
|
Final exact-head fix-forward evidence for PR #4540:
Final causal fixes:
Verification:
— |
2020341 to
c7e4575
Compare
MERGE_READY — exact-head gate green
MERGE_READY. Owner-controlled merge has intentionally not been performed. — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 424fa36a2
"await pre-fence recovery in scoped idle drains" - the latest in this branch ordering series.
Differential
# base 9d2a2d2f2, with this head test files applied
(fail) ... > does not deadlock default selection waiting for an ownerless emergency continuation [10017.65ms]
(fail) ... > keeps an ownerless emergency continuation scheduled later behind selection [10020.66ms]
(fail) ... > keeps waitForIdle pending while a deferred continuation waits behind selection [10016.97ms]
(fail) ... > does not deadlock when a second selection reserves while a continuation waits behind the first [10020.34ms]
(fail) ... > fences SDK prompt, steer, and follow-up ingress behind an earlier selection [21.86ms]
(fail) ... > promotes an SDK prompt queued at dispatch when no predecessor remains after selection [5019.50ms]
# head 424fa36a2
106 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
Five of the six base failures are multi-second timeouts rather than assertion mismatches - the
deadlocks reproduce as hangs, which is the least ambiguous evidence available for this class of bug.
keeps waitForIdle pending while a deferred continuation waits behind selection is the new one and
it is the right invariant to add: idle must not be reported while work is parked behind the fence,
or callers act on a false quiescence signal. That is the failure mode where a caller tears down or
compacts a session that still has pending work.
merge-approved at 424fa36a2.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head, separate clean base worktree with the head test files to prove the differential.
424fa36 to
7a35c1e
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 7a35c1eae
Same change as 424fa36a2 which I approved minutes ago, rebased. Re-verified rather than assumed,
since an approval is bound to the exact head:
$ bun test <the PR four touched suites>
106 pass
0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
The substantive review stands from the earlier passes on this branch: selection fence, defer-time
generation pinning, admission-time delivery resolution, latched promotion, the disposal/abort guard
on the fresh-promotion path, and now awaiting pre-fence recovery in scoped idle drains so
waitForIdle cannot report quiescence while work is parked behind the fence.
merge-approved at 7a35c1eae3e221d75ecbd7bb019ecc05cf038dd4.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head; digest recomputed independently.
The first Ultragoal boundary cohort found that SDK turn.prompt, steer, and follow-up could queue directly through sendUserMessage while a previously accepted default selection still owned the causal fence. Apply the same abortable selection fence at the shared external SDK ingress, check same-session ownership before prompt waits, and give only private scheduled continuations an explicit fence/reentry capability so #4496 ownerless continuations remain deadlock-free without admitting later external successors. Lore-id: issue-4519-sdk-ingress-fence Constraint: only internal scheduled continuations bypass selection fences Constraint: waitForIdle remains outside selection admission Confidence: high Scope-risk: medium Reversibility: clean Tested: attribution 12/12, default selection 45/45, auto-compaction 23 pass + 1 skip, SDK host wiring 104/104, coding-agent check/types
…uations Codex review of exact head 6907359 flagged two ordering gaps plus the missing changelog entry. A later plain prompt could overtake an earlier follow-up dispatch that was still awaiting its durable onPreflightAcceptCommit, because classification only observed already enqueued messages; and a continuation parked behind a pending selection fence cleared the settlement markers without registering anything waitForIdle could observe, so an external idle wait could report the session settled while the continuation was still waiting on the fence. Follow-up ingress now classifies and reserves order synchronously before any await (selection fence or durable acceptance) using counted epoch reservations, and a later dispatch drains earlier reservations before its own durable enqueue so enqueue order can never invert. Fence-deferred continuations are tracked as settlement work keyed by fence generation; the tracking window ends at the deferred invocation's synchronous re-entry, and the selection owning the fence never waits on its own parked continuations, preserving the #4519 no-self-deadlock guarantee. Lore-id: issue-4519-selection-order-review Constraint: selection must never wait on continuations parked behind its own fence Constraint: reservations must not survive a rejected or cancelled acceptance Tested: attribution 24/24 incl. new reservation ordering regression, auto-compaction 26 pass + 1 skip incl. new waitForIdle fence regression, manual-retry 5/5, SDK host runtime 48/48, coding-agent check/types, biome, verify-gjc-state-writers fast gate Not-tested: live multi-connection SDK broker soak beyond the wired host runtime suite Confidence: high Scope-risk: medium Reversibility: clean
Codex review of head 69b98ef flagged two P1 gaps in the fix-forward. The idle-observable deferral counter keyed off a single active-generation field, so when selection B reserved generation 2 while a continuation was still deferred behind selection A's generation-1 fence, A observed generation 2 as its own and could wait on the generation-1 continuation that was itself waiting on A — deadlock. And a follow-up dispatch that reserved an epoch and then had its selection-fence wait rejected by terminal cancellation exited before the cleanup scope, leaking the epoch so later prompts misclassified as having a follow-up ahead and later follow-ups waited forever. Deferral observability is now scoped structurally instead of by caller identity: only deferrals whose fence has already settled count toward pending settlement, because their deferred tails re-enter within microtask time. A deferral behind a still-pending fence is invisible by construction — the fence owner's completion may causally depend on the settlement a waiter would block on — so counting it could always reintroduce the #4519 self-deadlock regardless of which selection observes it. The selection finally records the settled generation and resolves settlement waiters. The follow-up reservation now covers the fence wait and post-wait assertion inside its try/finally, so any exit path releases the epoch and wakes drain waiters. Lore-id: issue-4519-selection-order-review-2 Constraint: no caller-identity-based fence exclusion; observability is settled-fence-scoped only Constraint: every reservation exit path must release the epoch Tested: attribution 25/25 incl. new reservation-leak regression, auto-compaction 27 pass + 1 skip incl. new overlapping-selections no-deadlock regression, manual-retry 5/5, SDK host runtime 48/48, coding-agent check/types, fast gate 0 violations Confidence: high Scope-risk: medium Reversibility: clean
…dle waits Codex review of head 88deb97 flagged that the settled-generation filter hid unresolved fence-deferred continuations from every idle wait, not just the selection's own: once the predecessor event handler settled, an external waitForIdle could report the session idle while the parked continuation was still waiting on its fence, letting SDK completion reporting race the continuation. External idle waits now count every unresolved deferral. Only two callers narrow their view, both by explicit internal-only parameter rather than caller inference: setDefaultModelSelection's mid-selection drain passes its own fence generation (work parked behind its own fence is waiting on the selection itself), and the causally-upstream turn-settle path excludes deferrals behind still-pending fences via the oldest pending fence generation. Scoped waiters no longer share the strict settlement promise — they register a wake callback re-evaluated on settlement transitions — because that promise may correctly stay pending on a fence-deferred continuation the fence owner must not block behind. Lore-id: issue-4519-selection-order-review-3 Constraint: external waitForIdle observes all unresolved fence-deferred continuations Constraint: only explicit internal parameters narrow deferral visibility; no caller inference Constraint: scoped settlement waits never block behind the strict settlement promise Tested: attribution 25/25, auto-compaction 27 pass + 1 skip incl. waitForIdle-pending regression, manual-retry 5/5, SDK host 48/48, sdk-session-router-authority 40/40, coding-agent check/types, fast gate 0 Confidence: high Scope-risk: medium Reversibility: clean
Selection drains skipped all post-prompt recovery, which could starve a timer-backed continuation that predates the selection fence. Track each task fence generation so scoped drains await only causally earlier work. Lore-id: 4540-scoped-idle-recovery Constraint: selection must not await continuations parked behind its own fence Confidence: high Scope-risk: narrow Reversibility: simple Tested: bun --cwd=packages/coding-agent run check; bun test packages/coding-agent/test/agent-session-before-agent-start-attribution.test.ts packages/coding-agent/test/agent-session-auto-compaction-continue.test.ts packages/coding-agent/test/agent-session-manual-retry.test.ts
7a35c1e to
33dc4d6
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Terminal merge execution at the exact reviewed head.
Merging into — |
2 participants
Summary
Provenance
64c15281691280be7854dac04baeb05188328ef433dc4d61e8b8dd1c578900b04fa1e5c544c70271ce574729f8b74c927f9e91cec233ea413fcf76725837c2c89f51ef20a381213fFinal bounded verification
git diff --checkis clean.bun --cwd=packages/coding-agent run check: pass.31856677720: product validation and virtual integration passed; its only failure was the then-currentneeds-humanbootstrap gate.Exact-head approval
Authenticated collaborator
probeparksubmitted review4942172870asAPPROVEDwith RESTcommit_idexactly33dc4d61e8b8dd1c578900b04fa1e5c544c70271at2026-08-15T00:46:57Z; collaborator permission iswrite. This is the contract's exact-head authenticated approval binding.Fixes #4519
gajae.pr-review-verdict.v1 merge-approved sha256:ce574729f8b74c927f9e91cec233ea413fcf76725837c2c89f51ef20a381213f reviewer:human reviewer-id:probepark evidence:review-4942172870-exact-head-33dc4d61-current-dev-ci-product-green
—
[repo owner's gaebal-gajae (clawdbot) 🦞]