fix(agent): don't resolve whenIdle() early on pre-step cancel + requeue (Codex review)
Codex's converge pass found a quiescence-contract violation: a whenIdle() waiter registered for prompt A, then cancel() clears A, then prompt B is queued BEFORE the loop resumes from the idle wait. The window-1 cancel branch called settleIdle() UNCONDITIONALLY, resolving the waiter while B was still queued-and-unrun — whenIdle() resolved with zero events, then B ran afterward. Fix: in window 1, only settleIdle() + re-park when NO new work is queued. If a send() raced in after the cancel, the marker was for the cancelled work only — clear it and fall through to run the new prompt's turn, letting THAT turn's running→idle settle the waiter (so whenIdle() waits for B to actually run). Adds a regression test reproducing the exact interleaving (send A → whenIdle → cancel → send B): whenIdle() now resolves only after B's turn ran (B's user message + a turn/end in the log), and A was dropped.
This commit is contained in:
@@ -169,16 +169,24 @@ export async function runLoop(ctx: Context, agent: ReactLoopAgent, handle: LoopH
|
|||||||
if (handle.isDisposed()) break
|
if (handle.isDisposed()) break
|
||||||
|
|
||||||
// Pre-step cancel (window 1): a `cancel()` landed after a `send()` woke the
|
// Pre-step cancel (window 1): a `cancel()` landed after a `send()` woke the
|
||||||
// idle wait but before we flip to `running`. Drop the about-to-run turn: the
|
// idle wait but before we flip to `running`. The cancelled queued/steering
|
||||||
// queued/steering work is already cleared by `cancel()`, and we settle any
|
// work is already cleared by `cancel()`. Clear the marker, then:
|
||||||
// `whenIdle()` waiter DIRECTLY (no status transition fires here, so the
|
// - if NOTHING new is queued, drop the about-to-run turn and re-park,
|
||||||
// running→idle settle never runs) WITHOUT emitting `agent/status` (an ACP
|
// settling any `whenIdle()` waiter DIRECTLY (no running→idle transition
|
||||||
// listener must not see a spurious idle that resolves a freshly-queued prompt
|
// fires here to settle it) and WITHOUT emitting `agent/status` (an ACP
|
||||||
// as cancelled). Clear the marker and re-park.
|
// listener must not see a spurious idle that resolves a freshly-queued
|
||||||
|
// prompt as cancelled);
|
||||||
|
// - if a NEW prompt was queued AFTER the cancel (a send() that raced in
|
||||||
|
// before the loop resumed), the marker was for the cancelled work only —
|
||||||
|
// fall through and run the new prompt's turn. Do NOT settle waiters here:
|
||||||
|
// a whenIdle() waiter must wait for that new turn's running→idle, not
|
||||||
|
// resolve before it runs (the quiescence contract).
|
||||||
if (handle.isCancelled()) {
|
if (handle.isCancelled()) {
|
||||||
handle.clearCancel()
|
handle.clearCancel()
|
||||||
handle.settleIdle()
|
if (!agent.inbox.hasQueued) {
|
||||||
continue
|
handle.settleIdle()
|
||||||
|
continue
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
handle.setStatus('running')
|
handle.setStatus('running')
|
||||||
|
|||||||
@@ -249,6 +249,31 @@ describe('Agent.cancel()', () => {
|
|||||||
expect(agent.session.events.some(e => e.type === 'turn/start')).toBe(false)
|
expect(agent.session.events.some(e => e.type === 'turn/start')).toBe(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('whenIdle() does NOT resolve early when a new prompt is queued during a pre-step cancel', async () => {
|
||||||
|
// The subtle race: a whenIdle() waiter is registered for prompt A; cancel()
|
||||||
|
// clears A; prompt B is queued BEFORE the loop resumes from the idle wait.
|
||||||
|
// The window-1 cancel branch must NOT settle the waiter while B is still
|
||||||
|
// queued-and-unrun — whenIdle() must wait for B's turn to actually run and
|
||||||
|
// settle (the quiescence contract), not resolve before B's first event.
|
||||||
|
const adapter = new MockAdapter([textResponse('A reply'), textResponse('B reply')])
|
||||||
|
const ctx = await harness(adapter)
|
||||||
|
const agent = ctx.agentLoop.create('a1', { model: 'mock' })
|
||||||
|
|
||||||
|
send(agent, 'A') // queues A (status still idle, loop microtask pending)
|
||||||
|
const idle = agent.whenIdle() // registers a waiter (idle + hasQueued → no fast path)
|
||||||
|
agent.cancel('drop A') // arms marker, clears A
|
||||||
|
send(agent, 'B') // B races in before the loop resumes
|
||||||
|
|
||||||
|
// whenIdle() must resolve only AFTER B's turn fully ran — by which point B's
|
||||||
|
// user message and a turn/end are in the log. (Before the fix it resolved
|
||||||
|
// immediately, with zero events, then B ran afterward.)
|
||||||
|
await idle
|
||||||
|
expect(userTexts(agent)).toContain('B')
|
||||||
|
expect(agent.session.events.some(e => e.type === 'turn/end')).toBe(true)
|
||||||
|
// A was dropped (never ran); only B's turn is recorded.
|
||||||
|
expect(userTexts(agent)).not.toContain('A')
|
||||||
|
})
|
||||||
|
|
||||||
it("cancel clears the turn's steering — it is not re-enqueued as a fresh turn", async () => {
|
it("cancel clears the turn's steering — it is not re-enqueued as a fresh turn", async () => {
|
||||||
const adapter = new MockAdapter(['hang'])
|
const adapter = new MockAdapter(['hang'])
|
||||||
const ctx = await harness(adapter)
|
const ctx = await harness(adapter)
|
||||||
|
|||||||
Reference in New Issue
Block a user