fix(agent-loop): second review pass — late-steering discard, dead-branch, catalog leaks
Address a second fresh-eye review of the review fixes: - MAJOR: late steering that lands after runTurn returns terminally stopped (e.g. during the post-turn flush) was drained by runLoop and dropped without a discard, leaving a dangling outstanding id the negative-only invariant can't catch. Emit agent/inbox/discard for it, symmetric with the in-turn terminal-stop drop. - remove the dead cancel() idle-settle branch: whenIdle's fast path already resolves for a lone quiet item, so no waiter is ever left for it to settle. Document why. - gen-cordis-api classShape now drops private/protected/#private members and strips getter/setter bodies, so Session no longer leaks private fields and getter bodies into the model catalog. - document that AgentMessage intentionally omits meta (durable-only). Adds a regression test for the late-steering discard.
This commit is contained in:
@@ -347,11 +347,6 @@ export class ReactLoopAgent extends Agent {
|
||||
agentEvents(this.loopCtx, this).emit('agent/cancel-requested', resolvedCause)
|
||||
}
|
||||
if (!keepInbox) {
|
||||
// Whether the parked driver was already scheduled to run: a waking item
|
||||
// woke `waitForQueued`, so the loop WILL resume and settle idle waiters
|
||||
// itself through the pre-run-cancel path (possibly after a replacement
|
||||
// prompt). Only a lone quiet item leaves the loop truly parked.
|
||||
const willResume = this.#inbox.hasWakingQueued
|
||||
// Snapshot before clearing so the discard notification carries the exact
|
||||
// dropped items; a replacement synchronously enqueued by an
|
||||
// `agent/cancel-requested` observer belongs to the next turn, not here.
|
||||
@@ -362,15 +357,12 @@ export class ReactLoopAgent extends Agent {
|
||||
const items = discarded.map(({ message, steering }) => agentMessage(message, steering))
|
||||
agentEvents(this.loopCtx, this).emit('agent/inbox/discard', items)
|
||||
}
|
||||
// Clearing a parked quiet (`wakeup:false`) item reaches quiescence with no
|
||||
// status transition and without waking the parked driver, so settle any
|
||||
// `whenIdle` waiter here. When a waking item was present the loop resumes
|
||||
// and settles itself; while `running` (including the post-turn flush
|
||||
// window) the driver still owns the eventual idle transition. So settle
|
||||
// only for a parked, non-running agent whose sole cleared work was quiet.
|
||||
if (cancellation === undefined && !willResume && this._status !== 'running') {
|
||||
this.settleIdleWaiters()
|
||||
}
|
||||
// No idle-waiter settle here: a `whenIdle` waiter exists only while the
|
||||
// agent is `running` or a waking item is queued, and neither is left
|
||||
// quiescent by clearing the inbox — a lone quiet item takes `whenIdle`'s
|
||||
// fast path (no waiter), a waking item keeps the woken driver running,
|
||||
// and a running agent owns its own idle transition (including the
|
||||
// post-turn flush window).
|
||||
}
|
||||
cancellation?.request(resolvedCause)
|
||||
}
|
||||
|
||||
@@ -264,9 +264,17 @@ export async function runLoop(ctx: Context, handle: LoopHandle): Promise<void> {
|
||||
handle.clearTurnCancellation(cancellation)
|
||||
}
|
||||
|
||||
// Late steering becomes queued input unless terminal policy stopped the turn.
|
||||
for (const message of handle.inbox.drainSteering()) {
|
||||
if (!terminalStopped) handle.inbox.enqueue(message)
|
||||
// Late steering (arriving after runTurn returns, e.g. during the post-turn
|
||||
// flush) becomes queued input — unless terminal policy stopped the turn, in
|
||||
// which case it is dropped and must publish a discard so its enqueue is
|
||||
// still matched (the invariant only catches a NEGATIVE count, not a leak).
|
||||
const lateSteering = handle.inbox.drainSteering()
|
||||
if (terminalStopped) {
|
||||
if (lateSteering.length > 0) {
|
||||
events.emit('agent/inbox/discard', lateSteering.map(message => agentMessage(message, true)))
|
||||
}
|
||||
} else {
|
||||
for (const message of lateSteering) handle.inbox.enqueue(message)
|
||||
}
|
||||
|
||||
// Park at idle unless a waking item still wants the model to run; a lone
|
||||
|
||||
@@ -117,4 +117,39 @@ describe('inbox FIFO-conservation invariant', () => {
|
||||
expect(warn.mock.calls.flat().some(arg => String(arg).includes('agent/inbox'))).toBe(false)
|
||||
expect(warn.mock.calls.flat().some(arg => String(arg).includes('INVARIANT'))).toBe(false)
|
||||
})
|
||||
|
||||
it('stays balanced when late steering lands after a terminal stop (post-turn flush window)', async () => {
|
||||
const adapter = new MockAdapter([textResponse('done')])
|
||||
const ctx = await harness(adapter)
|
||||
const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined)
|
||||
const agent = ctx.agentLoop.create(SessionId('a1'), { provider: 'mock', model: 'mock' })
|
||||
|
||||
let enqueues = 0
|
||||
const discards: number[] = []
|
||||
ctx.on('agent/inbox/enqueue', (subject) => { if (subject === agent) enqueues += 1 })
|
||||
ctx.on('agent/inbox/discard', (subject, messages) => { if (subject === agent) discards.push(messages.length) })
|
||||
|
||||
// Terminal-stop the turn, then steer during the post-turn flush window
|
||||
// (status is still running). That late steer is drained by runLoop and
|
||||
// dropped because the turn terminally stopped; it must still be discarded so
|
||||
// its enqueue is matched (the drain sits on a different code path than the
|
||||
// in-turn terminal-stop drop).
|
||||
ctx.on('agent/turn-stop', subject => (subject === agent ? { action: 'stop' as const } : undefined))
|
||||
let steered = false
|
||||
ctx.on('session/flush', (session) => {
|
||||
if (session !== agent.session || steered) return
|
||||
steered = true
|
||||
agent.steer([{ type: 'text', text: 'late' }], { source: { kind: 'plugin', plugin: 'late' } })
|
||||
})
|
||||
|
||||
agent.send([{ type: 'text', text: 'go' }])
|
||||
await waitForIdle(ctx, agent)
|
||||
|
||||
// The prompt plus the late steer both enqueued; both are matched (the prompt
|
||||
// dequeued, the late steer discarded) so no id is left outstanding.
|
||||
expect(enqueues).toBe(2)
|
||||
expect(discards).toEqual([1])
|
||||
expect(warn.mock.calls.flat().some(arg => String(arg).includes('agent/inbox'))).toBe(false)
|
||||
expect(warn.mock.calls.flat().some(arg => String(arg).includes('INVARIANT'))).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user