feat(runtime): present the delivery nonce on ack; 409 stops the turn (D6 consumer) - #1349
Conversation
…stops the turn (ADR-026 D6 consumer) Pairs with #1347. The claim's deliveryId is echoed on ack; a 409 stale_delivery raises StaleDeliveryError — the DO drops its staged reply and moves on, never retries or posts twice. Additive: today's server ignores the body and never 409s. 2 tests; 22/22. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK
…9; document that D6 makes acks single-winner, not posts (Otto) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK
…uard on staged entries (Otto) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK
07c3ca2 to
c6a7821
Compare
lilyshen0722
left a comment
There was a problem hiding this comment.
Gate at c6a7821574dd0936c29519ad5391b974037944a0 (CLEAN, 10/10 green, base main). Reviewed against #1347 at 18d9b7a66, since this PR is only correct relative to that producer.
Blocking: the nonce is read from a key the producer never writes, so this consumer presents nothing.
src/agent-do.ts:108 — await ackEvent(cfg, event._id, event.deliveryId), and CapEvent declares deliveryId?: string at the top level of the event. #1347 puts it in the payload:
// agentEventService.ts list(), #1347 @ 18d9b7a66
const enrichedPayload = {
...basePayload, ...digestBundle,
...(event?.deliveryNonce ? { deliveryId: event.deliveryNonce } : {}),
};
return { ...event, payload: enrichedPayload };The claim uses .lean() with no .select(), and GET /api/agents/runtime/events is a raw res.json({ events }), so the wire object is:
{ _id, type, podId, deliveryNonce: "<hex>", payload: { ..., deliveryId: "<hex>" } }
Top-level deliveryId does not exist on it. event.deliveryId is undefined, ackEvent sends {}, and this runtime acks nonce-less forever. Under Phase A that is invisible — it just increments withoutNonce, which is the counter #1347 gates the Phase B flip on, so this PR's presence actively holds that gate shut. Under Phase B the route answers 400 delivery_id_required, ackEvent throws ackEvent 400, and no event is ever acked. Fix is event.payload?.deliveryId.
The suite cannot see this: cap.test.ts passes 'nonce-abc' into ackEvent by hand, so it pins the client's serialization and never the extraction one call frame up. agent-do has no test that hands it a realistically-shaped event. One fixture with the real wire shape catches it.
The comment at agent-do.ts:111-113 claims a guarantee the ordering cannot provide. "drop the staged reply so nothing posts twice" — but handleEvent (which posts) runs at :104, before the ack at :108. By the time the 409 arrives the duplicate is already in the pod; the replacement child posts its own. And the id was pushed to processed at :105, so the processed.includes guard is what prevents a re-post, not the staging delete. Deleting the staged entry is, if anything, mildly the wrong direction: it discards the memoized reply that would have been the second line of defence once the id ages out of the 200-entry ring.
I don't think that's fatal to the PR — this is an ack-side signal and #1347 scopes D6 to the ack. But the comment should say what the 409 actually buys (stop recording it as our error, stop retrying) rather than a double-post guarantee it doesn't deliver. If a genuine pre-post supersession check is wanted, that's a design question for D6, not a comment fix.
Non-blocking: a systematically superseded runtime reports as healthy. The continue at :115 skips batchErrors += 1 — correct, it isn't our error — but also records nothing, so lastError is cleared at :125 and lastPollAt is fresh. A runtime whose claims expire every single cycle looks identical on /status to one working perfectly. That is the exact inverse of the latched-lastError fix reasoned about two lines below it. A staleDeliveries counter in storage, surfaced on /status, costs one line.
What's right. Branching on body.code rather than the bare 409 is the correct call and the negative test for it (something_else and a throwing json()) is the one I'd have asked for. The staging shape guard plus its test is a clean fix for a real reply: undefined TypeError.
Blocking on the wiring bug only.
|
TASK-090 correction at 91e641e: the claim nonce is in event.payload.deliveryId, not event.deliveryId. The hosted runtime now forwards that exact value to ackEvent; the regression test fails on the old top-level read. Worker suite 30/30 and typecheck pass. Please independently re-gate the new head. |
lilyshen0722
left a comment
There was a problem hiding this comment.
PASS at 91e641e6. My block is cleared exactly at the seam, and the extra deletion is the part that matters most.
agent-do.ts:108 now reads event.payload?.deliveryId, which is where list() actually writes it, and the string guard means a malformed value degrades to undefined (Phase-A accepted) rather than being echoed as a nonce that cannot match.
Removing deliveryId from CapEvent is what stops this recurring. The mis-wiring was not a typo — the type said the field lived at the top level, so reading it there was the correct move against a wrong contract. Deleting the declaration means the next person cannot make the same reasonable mistake. That is a better fix than the one I recommended, which only corrected the read site.
The new test pins the contract at the right altitude: it asserts ackEvent is called with the nonce taken from a polled event's payload, so a future producer that moves the field reddens here instead of going quiet.
The StaleDeliveryError comment correction is right too — the processed-id ring, not the staged-reply delete, is what prevents a double post. The old comment claimed the delete was doing that work.
10/10 CI green. behind=0.
One note carried from the other side of this pair: this PR is now correct against list(), and #1347 also pushes a nonce over the WebSocket at a different nesting. That is a finding on #1347, not here — filed there.
Consumer half of ADR-026 D6, pairing with #1347: the hosted DO echoes the claim's
deliveryIdon ack and treats 409stale_deliveryas 'you were replaced' — drops its staged reply, does not post, does not retry. Additive and safe in either merge order (today's ack route ignores the body and never returns 409). 2 tests. Merge gate: @otto.🤖 Generated with Claude Code
https://claude.ai/code/session_013pc6nGXRS8mHvrwcXMSRDK