fix(session): restore synchronous append for uncontended canonical admission - #4564
Conversation
…mission The merged #4521 admission lane awaited its predecessor unconditionally, so every message_end cost a microtask even when nothing was contended. External emitters (host bridges, the 2261 successor flow) read the persisted branch immediately after emitExternalEvent, and the deep-interview continuation scheduler reads stop state across that boundary; both broke. Track released-ness on the admission slot itself. The append site takes a synchronous fast path when the predecessor slot is already released and only awaits when an admission is genuinely in flight, so FIFO ordering under a gated spill is preserved exactly. Lore-id: issue-4536-postmerge-regressions Constraint: preserve handler-local reservation ownership (duplicate-emission wedge fix) Constraint: FIFO admission must hold under real contention Rejected: unconditional await skip analysis | await yields even on settled promises, verified empirically Rejected: blanket test expectation edits | 2261/deep-interview encode a real product visibility contract Confidence: high Scope-risk: narrow Reversibility: easy Tested: 2261 17/17; deep-interview 22/22; spill 7/7 incl. new sync-visibility and gated-FIFO tests Not-tested: multi-host bridge traffic emitting duplicates in production Fixes #4536
T2's expectation encoded the pre-#4536 defective canonical order, under which the seeded orphan tool results landed inside the protectRecentTurns fence and forced compaction. With canonical order equal to emission order they sit outside the fence and prune — the cheaper preferred rewrite — legitimately wins; the surviving assertions prove the protected paired result and steering messages are preserved either way. Adds two discriminating tests to the spill suite: uncontended message_end appends must be visible synchronously after emitExternalEvent (no microtask), and a gated spill must hold the continuation's admission in FIFO order behind the tool result. Lore-id: issue-4536-postmerge-regressions Constraint: expectation updates only where behavior is intentionally changed and product-safe Confidence: high Scope-risk: narrow Reversibility: easy Tested: maintenance 19/19; spill 7/7; midrun-compaction 18/18
Lore-id: issue-4536-postmerge-regressions Confidence: high
|
Exact-head CI + local evidence for review — human merge gate pending CI at exact head
Local evidence at the same head: maintenance 19/19 · 2261 17/17 · deep-interview 22/22 · spill 7/7 · midrun-compaction 18/18 · message-pipeline 24/24 · #4558 interaction 101/101 · #4542 interaction 40/40 · 11-suite broad sweep 176/176 on re-run · What the fix is not: no blanket expectation edits. 2261 + deep-interview encode a real synchronous-visibility product contract (both go green only with the fast path; a new test pins append visibility immediately after Reviewer @probepark: on approval I will embed the — |
|
Evidence handoff from the retired duplicate lane PR #4565 (closed unmerged; this lane stays canonical):
Supersession receipt: #4565 head — |
|
Terminal-green evidence + full regression-family coverage at exact head CI (run 31822780509, exact head): 17/17 product jobs green — affected shards All three dev regression families covered at this head (local, deterministic):
Controls: T3 ordered Cursor projection green; duplicate-emission spill test green (wedge fix preserved); FIFO-under-contention green (new gated test: empty while gated, Adversarial follow-up on #4565's unique reasoning, in progress on this PR only: #4565's claim is that an externally emitted terminal's Reviewers @probepark @HaD0Yun: on first exact-head approval I will embed the — |
|
@probepark Independent exact-head review requested for — |
Externally emitted terminals dispatch agent_end immediately after message_end. With the admission predecessor contended (gated spill), the message_end handler parked before its post-persistence capture site, so the agent_end handler's post-turn read resolved #lastAssistantMessage to the PREVIOUS turn's assistant: a mid-loop toolUse capture short-circuited stop handling via hasToolCalls and silently skipped the terminal's deep-interview continuation; an errored capture misclassified the terminal for retry. Adversarial provenance: identified by closed duplicate PR #4565's terminal-assistant-before-wait reasoning; proven real on #4564's head with an instrumented probe (agent_end resolved the previous turn's timestamp while the terminal admission was parked) and pinned by a discriminating test that is red without this capture and green with it. Subsumes #4565 alongside the uncontended fast path: contended and uncontended admissions now both expose the terminal synchronously. Lore-id: issue-4536-postmerge-regressions Constraint: single-source capture; post-persistence block keeps side effects only Constraint: do not regress the duplicate-emission wedge fix Confidence: high Scope-risk: narrow Reversibility: easy Tested: discriminating capture test red-without/green-with; deep-interview 22/22; 2261 17/17; maintenance 19/19; spill 7/7; midrun-compaction 18/18; message-pipeline 24/24; check green
|
Head advanced to Adversarial validation of closed duplicate #4565's terminal-assistant-before-wait reasoning, executed on this PR only:
Verification at CI is re-running at the new exact head; the — |
![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: 7f32140b6f
ℹ️ 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 (event.type === "message_end" && event.message.role === "assistant") { | ||
| this.#lastAssistantMessage = event.message; |
There was a problem hiding this comment.
When an assistant admission is blocked behind a slow tool-result spill, assigning #lastAssistantMessage here allows an immediately emitted agent_end to begin post-turn work before either the tool result or terminal assistant has reached SessionManager. In the new gated deep-interview scenario, the continuation reminder can therefore be persisted before the tool result and assistant it responds to; threshold compaction can likewise inspect or rewrite an incomplete branch. This breaks the FIFO persistence guarantee on reload, so capture the terminal per event but gate its agent_end processing on completion of the corresponding canonical admission.
Useful? React with 👍 / 👎.
|
Verdict-digest defect fixed at exact head The PR body's verdict line briefly carried a with the digest recomputed exactly as the contract validator computes it ( Exact-head evidence at Reviewers @probepark @HaD0Yun are both requested. No merge until an authenticated exact-head approval flips this verdict to — |
…al admission The pre-wait terminal capture fixed identity but not ordering: under a contended predecessor, an immediately emitted terminal let agent_end begin post-turn work while the terminal assistant (and the tool result it follows) had not yet reached SessionManager, so a deep-interview continuation reminder or compaction rewrite could persist ahead of the branch entries it responds to — breaking FIFO on reload. Track the admission slot per assistant message; the agent_end handler joins the terminal's admission before any post-turn write, using the same released fast path so uncontended terminals stay synchronous. Review provenance: Codex P1 on 7f32140. The discriminating test now also pins reminder-after-terminal branch ordering and remains red without the fix. Lore-id: issue-4536-postmerge-regressions Constraint: uncontended terminals must stay fully synchronous Constraint: per-message keying so re-wrapped duplicate events still join Confidence: high Scope-risk: narrow Reversibility: easy Tested: capture test red-without/green-with incl branch-order assertion; deep-interview 22/22; 2261 17/17; maintenance 19/19; spill 7/7; midrun-compaction 18/18; message-pipeline 24/24; 10-suite matrix 137 pass; check green
|
Head advanced to Codex's P1 on Fix ( Discriminating test strengthened and re-proven: still red without the fix, green with it, and now additionally asserts the branch order Verification at Reviewers @probepark @HaD0Yun: first exact-head approval at — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbccf267f4
ℹ️ 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 spillGate = new Promise<void>(resolve => { | ||
| resolveSpill = resolve; | ||
| }); |
There was a problem hiding this comment.
Replace this manually constructed promise and definite-assignment resolver with Promise.withResolvers<void>(); the repository contract explicitly requires that primitive instead of new Promise(...), and this newly added test otherwise establishes a nonconforming pattern.
AGENTS.md reference: AGENTS.md:L119-L119
Useful? React with 👍 / 👎.
| const spillGate = new Promise<void>(resolve => { | ||
| resolveSpillWrite = resolve; | ||
| }); |
There was a problem hiding this comment.
Replace this manual promise/resolver capture with Promise.withResolvers<void>(); this added gate violates the repository-wide requirement to avoid new Promise(...) and unnecessarily relies on a definite-assignment assertion for the resolver.
AGENTS.md reference: AGENTS.md:L119-L119
Useful? React with 👍 / 👎.
Terminal-green reminder — PR #4564Exact head:
One authenticated APPROVED review on exact head — |
|
@probepark @HaD0Yun — concise review request for #4564 at exact head What it does (3 commits): (1) restores synchronous canonical append for uncontended Evidence: CI terminal at this head — 18 green, all 5 affected shards pass (contended-terminal-capture, spill, midrun-maintenance, notifications-live-stream, session-manager-resident-cache); sole red is Verdict line is staged: — |
Addresses the two Codex review notes: the repository requires Promise.withResolvers() over manually constructed promise/resolver captures (AGENTS.md L119). Behavior identical; gates now self-document. Lore-id: issue-4536-postmerge-regressions Confidence: high Tested: capture 1/1; spill 7/7; check green
|
Head The two style findings are fixed: both spill gates in the test files now use Product-fix heads: — |
|
Recovery evidence — head
gajae.pr-review-verdict.v1 needs-human sha256:7473766943882f941a61af13f42f2d96165b2b5a9667caa4031ec125f836a38f reviewer:human reviewer-id:pending-independent-review evidence:current-head-product-ci-green-awaiting-authenticated-independent-review-e8a956ae0b718b3893475d5e75c2fff4b7dc0616 Resumption is bounded to PR #4564: refetch — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve - this cleans up fallout from a change I approved
Reviewed e8a956ae0 against base 9d2a2d2f2.
I approved the canonicalAdmission FIFO lane in #4521. It fixed the canonical ordering defect I
filed as #4536, and it opened two windows I did not think through. This closes both.
1. Uncontended append is synchronous again
if (canonicalAdmission && !canonicalAdmission.predecessor.released) {
await canonicalAdmission.predecessor.promise;
}The lane made every message_end await its predecessor, including the overwhelmingly common case
where nothing is contended. The released flag check restores a synchronous append there.
I checked the flag in the dangerous direction, because a fast path that skips a barrier is only as
good as its predicate:
const release = () => {
if (released) return;
released = true;
slot.released = true;
settled.resolve();
};slot.released is set before settled.resolve() and the whole thing is idempotent. So the flag
can never lag the promise - if the predecessor's promise is resolved, released is already true, and
released === true implies release() ran, which happens after that predecessor's canonical append.
There is no window where the fast path skips a wait that was actually needed.
2. agent_end no longer outruns a parked terminal admission
// an externally emitted terminal dispatches agent_end while its own
// admission may still be parked behind a contended predecessor, and a
// continuation reminder or compaction rewrite that runs first would
// persist ahead of the branch entries it responds to.
const terminalAdmission = msg ? this.#lastAssistantAdmissionByMessage.get(msg) : undefined;
if (msg) this.#lastAssistantAdmissionByMessage.delete(msg);
if (terminalAdmission && !terminalAdmission.predecessor.released) {
await terminalAdmission.predecessor.promise;
}This is the sharper of the two. The lane ordered canonical appends against each other but not against
post-turn work, so a continuation reminder or compaction rewrite could persist ahead of the entries
it is responding to - the same class of defect as #4536, one layer up. Tracking the admission per
message in a WeakMap and joining it before post-turn writes is the right fix, and the WeakMap
avoids pinning messages alive.
Moving the #lastAssistantMessage capture to before the admission wait is the necessary companion -
otherwise a parked handler would lose the auto-compaction tracking for that message.
Differential
# base 9d2a2d2f2, with this head's test files applied
(fail) AgentSession contended terminal assistant capture (#4565 finding) > still schedules deep-interview continuation when the terminal's admission is contended behind a gated spill [153.10ms]
(fail) AgentSession pre-admission artifact spill > appends canonical messages synchronously when no admission is contended [1.00ms]
25 pass 2 fail
# head e8a956ae0
45 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
One failure per window, both real. The second one is notable for asserting a synchronous property
rather than an outcome - that is the only way to pin a fast path, since a correct-but-slow
implementation would otherwise pass.
Verdict
merge-approved.
For the record: #4521 was correct about the defect it fixed and I would approve it again, but "add a
FIFO lane to an event handler" had a wider blast radius than my review credited. The lesson I am
taking is that when a change serializes something, the review needs to enumerate what else was
previously free to run in any order - not just check that the newly serialized pair is correct.
Reviewed by @probepark - method: source read of the released-flag ordering inside release() to validate the fast-path predicate in the unsafe direction, WeakMap admission tracking review, fresh-worktree run and package typecheck at the exact head, separate clean base worktree with the head's test files to prove the differential.
2 participants
Summary
Fix-forward for the deterministic dev regressions left by merged #4521 (issue #4536), which shipped the canonical Cursor-ordering fix without updating its dependent suites:
message_endadmissions — the admission lane now tracksreleasedon its own slot, so the canonical append site takes a synchronous fast path when the predecessor is already released and only awaits when an admission is genuinely in flightprotectRecentTurnsfence and prune (the designed cheaper preference) wins over compaction; expectation + rationale updatedemitExternalEvent; gated spill holds the continuation's admission in FIFO order behind the tool resultThe merged handler-local reservation closure (the duplicate-emission wedge fix from
e1dd34d3dd) is preserved untouched.Root-cause analysis (not blanket expectation edits)
agent-session-issue-2261(2 fails) andagent-session-deep-interview-continuation(4 fails) share one cause: the merged lane'sawait canonicalAdmission?.predecessoryields a microtask even when the predecessor promise is already settled (verified empirically —awaiton a resolved promise never continues synchronously). The successor-finalization flow assertsappendMessageimmediately afteremitExternalEvent, and the deep-interview continuation scheduler reads stop state across that boundary. Both suites go green the moment the synchronous fast path lands — synchronous visibility is a real product contract, and the new test pins it.agent-session-midrun-maintenanceT2 (1 fail) is the intentional behavior change from fix(session): Cursor split persists tool result after its continuation in canonical order #4536's own fix: corrected canonical order places the three oversized orphan tool results before the recent user turns, i.e. outside the recency fence (fence math verified for both orderings), so they become prunable and the prune branch wins. Data preservation was already asserted and still passes underpruned.Verification
9d2a2d2f2d8f074333bba92fe1fc4d6902f5b2df: T2 1 fail, 2261 2 fail, deep-interview 4 fail; controls green (T3 Cursor, spill incl. duplicate-emission)sdk-session-router-authority40/40bun --cwd=packages/coding-agent run checkgreen (biome + tsc)e8a956ae0b718b3893475d5e75c2fff4b7dc06169d2a2d2f2d8f074333bba92fe1fc4d6902f5b2df(currentorigin/dev)Subsumes closed duplicate #4565
#4565's terminal-assistant-before-wait capture is correct and identified a real uncovered path on this PR's first head: with the predecessor contended, my synchronous fast path cannot shortcut the wait, and the
agent_endhandler resolved#lastAssistantMessageto the previous turn's assistant (proven with an instrumented probe; a mid-loop toolUse capture then skipped the terminal's deep-interview continuation viahasToolCalls). This PR now captures the terminal assistant synchronously before the admission wait, and a discriminating test (agent-session-contended-terminal-capture.test.ts) is red without that capture and green with it. Both #4565 files' concerns are therefore owned here.Fixes #4536. Successor to #4521 (not a reopen).
gajae.pr-review-verdict.v1 merge-approved sha256:7473766943882f941a61af13f42f2d96165b2b5a9667caa4031ec125f836a38f reviewer:human reviewer-id:probepark evidence:base 25pass-2fail -> head 45pass-0fail; released-flag fast path verified sound; check exit0