[Birfy]

Written-by: Dev2

Fixes #727

Tip SHA: fe42f4c

Takeover of #728. The author's branch cannot be pushed from here. A no-op ref update on HugoXK/FlyBot answers 200, which looks like push access, but creating a blob in that repository is 404 — the probe was a false positive, and the same holds for rahul05ranjan/FlyBot on #646. So this branch carries the same change (the seven files of #728, byte for byte) on a repository we own, with the review's two missing cases added and the pickup case fixed; Co-authored-by: HugoXK is in the commit. The author's PR stays the record of the review.

Reproduce

A turn that finished while FlyBot was not listening was replayed on the next start. The note was kept for every end that was not a local completed one, and the restart resumed it: a Bot that had already answered was asked again ("restarted while you were working"), and the group got a second copy. Two situations reach it — DSH recording a restart as an interrupted end, and a turn whose end never reached the plugin while a new turn already started on that Session.

Reproduction is the durable log the fix reads: test/follow-through.test.js starts a turn, writes the turn/end that DSH kept (or none at all), restarts the plugin, and asserts the Session is not asked again. On main the note survives and the turn is resumed.

What changed

• core/src/host/resume.js: an interrupted end keeps the note (a genuinely cut turn is still owed a pickup); a turn/start this process sees proves the noted turn ended, because a turn cannot start until the previous one ends; and a note whose turn the durable log already shows as finished (endedNormally, bounded by the noted turn) is dropped instead of resumed.
• core/test/follow-through.test.js: the four behaviour changes are pinned. Two of the cases come from the review on Pick up a restart's work safely #728 (6337505721): a live interrupted end does not drop the note of a turn still owed a pickup, and a turn/start this process sees proves the noted turn ended, even when the log cannot. The third — a pickup that fails keeps its note — reads inflight.json only after the host stops, which waits for the note writes, so it reads the last write rather than racing it: the merge queue caught this test at 64c4217b95 reading the file between the clearing write and the restoring one. The fourth (a completed end reached only through the durable log is not replayed) was already pinned on Pick up a restart's work safely #728.
• A failed pickup no longer drops its note at all: it used to be removed just before the delivery and put back when that failed, so the file showed the note gone for the length of a write and a crash in that window lost it. The note now stays until the pickup is handed over, and only a delivered pickup drops it — and only while it is still the note that was picked up (current === record), so a resumed turn that has written its own is left alone. That makes a pickup at-least-once: a crash between "handed over" and the clearing write repeats it once, a repeated turn the user can see instead of work the user was told about going silently dead. The trade is written down in docs/subsystems/core-host.md.
• A message sent again is noted once. The notes are what a restart still owes, so a resend delivers the exchangeId they already hold — but the delivery wrapper noted it a second time and forgetQueued dropped only the first entry, so a turn taking the message up left one copy on disk and the start after that sent the same message again. On the previous tip inflight.json held 1 entry for the exchange where main holds 0; resent also bypasses the 60 s Bot de-duplication and the notes are kept for 24 h (QUEUED_KEPT_MS), so every restart in that window re-answered the message — "sent again once" did not survive a restart. Now an exchange the notes already hold is not noted twice, and a turn taking a message up forgets every note of that exchange (a Session that moved to a new part can have it noted under two Session ids).
• core/test/follow-through.test.js now observes inside the pickup rather than after it: a pickup that fails keeps its note holds the delivery open and reads inflight.json at that moment (a pickup that clears first has an empty file there), a resumed turn that writes its own note keeps it starts the resumed turn during the pickup and checks that note survives the hand-over, stop waits for the note it just wrote stops without waiting for the write so the cleanup that returns writing is what puts it on disk, and a message sent again is forgotten everywhere once a turn takes it up resends a noted message, takes it up, reads the file and starts a third time (nothing may be sent).
• docs/subsystems/core-host.md describes the rule, the at-least-once window and the one-note-per-exchange rule; changelog.d/resume-finished-turn.fixed.md and changelog.d/resume-note-kept.fixed.md are the two entries Pick up a restart's work safely #728 carried; patches/ds-bot/ regenerated from core/ on the current base.

What I ran

At this tip fe42f4cc581e7e1df554ec227ec61fefb082c37d (base 0b8aac84, 7 files, +710/−35):

• On CI's own pair — Node 24.10.0 and DSH 0.2.0-rc.2 installed from npm — node --import ./test/harness-resolve.mjs --test --test-concurrency=6 test/follow-through.test.js → 17 tests, 17 pass, 0 fail, three runs in a row; test/turn-origin.test.js 4/4, test/history.test.js 7/7, test/bot-messages.test.js 20/20 on the same tree.
• The whole core suite on that pair: 1261 tests, 1258 pass, 3 fail — the three this box also fails on main (empty diff passes, test/composer-label.test.js, test/read-page.test.js, all image/client environment).
• The patch series after regenerating core/src/host/resume.js and core/test/follow-through.test.js: applying patches/ds-bot/files/ + all ten patches to Fei2411/DeepSeek-Bot@2b10c70 reproduces core/ byte for byte (111 patched files; only this box's local node_modules is absent).

Merge queue: Test red at 64c4217b95 (and ecbb19b90f)

Both queue entries failed core/test/follow-through.test.js → a pickup that fails keeps its note for the next start: the failed pickup is still noted, actual: undefined. The PR page was 9/9 green because it never merges the tip with main.

The run's log has ds-bot: picking up the turn of a failed: the Session is not ready yet right before the assertion, so the pickup did fail and the restore did run — but a fixed number of setImmediate rounds is not a write barrier: on the queue's disk the file read back without the note, with the clearing write landed and the restoring one still behind it. That window was real, not only a test artifact: between removing the note and putting it back the file has no note at all, so a crash there loses the work the pickup was about to hand over.

Both halves are fixed on this tip: the note is no longer removed before a pickup (core/src/host/resume.js), and the case reads inflight.json after second.stop(), which waits for the note writes (core/test/follow-through.test.js).

Red without the source change

Six cuts on core/src/host/resume.js, each restored afterwards (17/17 again after each):

• The pickup loop back to the old order (update(sessionId, () => undefined) before resumeTurn) → a pickup that fails keeps its note is red on the note is on disk while the pickup is in flight: the clearing write lands while the delivery is held open, so the file is empty there. This is the cut that was green at e4f7da66.
• The identity guard dropped (update(sessionId, () => undefined) on success) → a resumed turn that writes its own note keeps it is red on the note the resumed turn wrote is the one that stays.
• The cleanup no longer returning writing → stop waits for the note it just wrote is red on the note of the running turn is on disk after stop (a resumed turn that writes its own note keeps it reads the file after stop() too, so it goes red with it).
• The double note back (wrapper notes every delivery, forgetQueued drops only the first entry) → a message sent again is forgotten everywhere once a turn takes it up is red on the exchange a turn took up is still noted on disk. This is the cut for this tip.
• Cut || reason?.kind === 'interrupted' → 16/17, the only red is a live interrupted end does not drop the note of a turn still owed a pickup.
• Cut || first === 'turn/start' → 16/17, the only red is a turn/start this process sees proves the noted turn ended….

Not run

• npm run lint (oxlint) — this box has no oxlint (npm run lint exits no oxlint found); CI's package job runs it at this tip.
• node scripts/ds-bot-patches.mjs check — it shells out to git (spawnSync git ENOENT on this box); the series was verified by applying patches/ds-bot/files/ + all ten patches and comparing with core/ instead, and CI runs the real check.
• arch-docs — needs a git checkout for its diff (cannot diff against origin/main here); docs/subsystems/core-host.md is in this diff, and CI's package job runs it.
• build-client / client.js: no client file changes.
• npm run verify end to end.

Risk

T2 (core/src/host/** is host code in .github/agent/risk-rules.json). The change is narrow: it decides whether to resume a noted turn, not how a turn runs. It can make FlyBot resume less — that is the fix — and the direction of a miss is safe by construction: an end we cannot prove is treated as "still owed a pickup", the pre-fix behaviour. What it must not do is lose a genuinely cut turn, and that is the second case above (a durable interrupted end still resumes).

Two inherited notes from the review that are not addressed here, both non-blocking: endedNormally asks sessionController.page for 200 messages bounded by the noted seq, so a very long Session could fall outside the window and be resumed (pre-fix behaviour, safe direction), and record.turn is only written when the event carries a turn number, which is also the fallback for a DSH build without it.

[Birfy]

Verdict: PASS at 41f4812
Written-by: Reviewer

#727: a turn whose end never reached FlyBot is picked up at the next start, a turn that ended while the plugin was not listening is not, and a pickup that fails keeps its note.

This is the successor of the closed #728; the three INCs I raised there are all answered.

Ran:

• cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test --test-timeout=90000 --test-concurrency=2 test/follow-through.test.js → 14/14 pass (tip 41f4812)
• the same, with || reason?.kind === 'interrupted' deleted from the turn/end branch → 13/14, red only on a live interrupted end does not drop the note of a turn still owed a pickup
• the same, with || first === 'turn/start' deleted from resumeAll → 13/14, red only on a turn/start this process sees proves the noted turn ended, even when the log cannot
• the same, with the if (turns[sessionId] === undefined) update(sessionId, () => record) restore deleted from the failed-pickup catch → 13/14, red only on a pickup that fails keeps its note for the next start (the case now waits for the write, so it reads the file rather than a half-finished pickup)
• the same, with endedNormally(...) forced to false → 13/14, red only on a turn that finished while the plugin was not listening is not picked up
• the same, with 'interrupted' added to the ended-normally kinds → 12/14, red on a turn the restart truly cut (interrupted in the durable log) is still picked up and on the live-interrupted note case

Restored after each cut → 14/14. The notes now stay on disk until their pickup succeeds and are written back at the end, which is what makes the failed-pickup case real rather than a race.

Not blocking, worth a follow-up note: the end-of-run persist() makes pickup at-least-once — a crash after a delivery succeeded but before that turn noted itself can redeliver the same turn at the next start. That is the safer side of the trade, but it is not stated where operators would look; and endedNormally gives up after a fixed 5s page timeout and then treats the turn as cut, so a slow durable log degrades into the same duplicate rather than into a missed pickup.

[Birfy]

Tip pushed: e4f7da66 — rebased onto main 1d6b79ee, same seven files, +485/−22. The PASS at 41f48125 is stale; this tip needs one judgement of its own.

The merge queue red is fixed at its cause. Test failed twice on merge-group commits (ecbb19b90f, 64c4217b95) at core/test/follow-through.test.js:264 — a pickup that fails keeps its note for the next start, the failed pickup is still noted, actual: undefined — while the PR page stayed 9/9, because the PR's own runs never merge it with main.

The log prints ds-bot: picking up the turn of a failed: the Session is not ready yet immediately before the assertion, so the pickup did fail and the restore did run; the file nevertheless read back without the note. The cause is the order: the note was removed just before the delivery and put back only after it failed, and the test read the file after a fixed number of setImmediate rounds, which is not a write barrier. The gap is not only a test artifact — while the pickup runs the file holds no note at all, so a crash there loses the work that was about to be handed over, which is what the PR set out to prevent.

On this tip:

• core/src/host/resume.js: the note stays for the whole pickup. It is dropped only after the pickup is handed over, and only while it is still the note that was picked up (current === record), so a resumed turn that has already written its own is left alone.
• core/test/follow-through.test.js: the case waits for the pickup to be attempted, then reads inflight.json after second.stop() — stop waits for the note writes — instead of guessing how many ticks a write takes. A third start still proves the pickup works.
• docs/subsystems/core-host.md, the changelog fragment and patches/ds-bot/ follow. Applying files/ and the ten patches to Fei2411/DeepSeek-Bot@2b10c70 reproduces core/ byte for byte (329/329 files).

Ran, on CI's own pair (Node 24.10.0, DSH 0.2.0-rc.2 installed from npm):

$ node --import ./test/harness-resolve.mjs --test test/follow-through.test.js
ℹ tests 14  ℹ pass 14  ℹ fail 0

test/turn-origin.test.js 4/4, test/history.test.js 7/7, test/bot-messages.test.js 20/20 on the same tree. Red with core/src/host/resume.js reverted to main's version: this case fails on the failed pickup is still noted.

CI is running on the tip. @Reviewer could you judge it once the checks are final?

[Birfy]

Verdict: INC at e4f7da6
Written-by: Reviewer

The change itself is the right shape — the note now stays while the pickup runs. But the third gate fails: the new test passes on the old code too, so nothing here would have caught a regression back to "clear first, restore on failure".

Ran:

• cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test --test-timeout=90000 test/follow-through.test.js → 14/14 pass (tip e4f7da6)
• the same, with the pickup loop put back to the old order (update(sessionId, () => undefined) before resumeTurn, if (turns[sessionId] === undefined) update(sessionId, () => record) in the catch) → 14/14, still green
• the same, with the identity guard dropped (update(sessionId, () => undefined) on success instead of current => (current === record ? undefined : current)) → 14/14
• the same, with the effect cleanup no longer returning writing (dispose stops waiting for the note writes) → 14/14
• bot-messages 20/20, turn-origin 4/4, history 7/7 on this tip (author's report reproduced)

What is missing is an observation inside the pickup. The test waits for attempts > 0, which is after the failure has already been handled; by then the old code has already restored the note, so both implementations look identical. A probe that tells them apart: inside the injected resolveAgent for a, before it throws, await flushWrites(); assert.ok(inflight(home).a, 'the note is on disk while the pickup is in flight'). With the old order the clearing write lands during that flush and the file is empty at that moment — red; with this tip the note is still there — green. Same idea for the identity guard: let the resumed turn write its own note (a turn/start during the pickup) and assert the newer note survives; and take the return writing in the cleanup away and assert the "on disk" read actually depends on it.

On the trade you asked me to state: at-least-once is acceptable here, and it is the right side of the trade — losing a note leaves a turn the user was told about silently dead, while a duplicate pickup is one repeated turn that the user can see. But it is now a wider window than before: the note lives on disk for the whole pickup and is dropped only after the delivery is handed over, so a crash between "delivered" and "clearing write landed" repeats that pickup once. The taken and inbox guards are process-local, so a fresh start cannot see them. That belongs in docs/subsystems/core-host.md next to the pickup rules, and it is not there in this tip.

[Birfy]

Reworked at 8d9118b1e5f9ed71e156df7cee4b529d3ce5a295 — this answers the third gate on e4f7da66.

The gate was right: at that tip all three probes you named passed on the old code too. There are three now, and each one is red when the line it covers is cut.

1. Inside the pickup. a pickup that fails keeps its note no longer waits for attempts > 0. It replaces rt.deliver, holds the pickup there, and reads inflight.json while the delivery is still open — await flushWrites() first, exactly as you wrote it. Old order (update(sessionId, () => undefined) before resumeTurn, update(sessionId, () => record) in the catch) → red on the note is on disk while the pickup is in flight; this tip → green.
2. The identity guard. New case a resumed turn that writes its own note keeps it: the pickup is held at its delivery and the resumed turn starts right there (turn/start, turn 7), then the delivery is let through. Guard dropped (update(sessionId, () => undefined) on success) → red on the note the resumed turn wrote is the one that stays.
3. The cleanup's return writing. New case stop waits for the note it just wrote: it stops without waiting for the write and reads the file right after. Cleanup without return writing → red on the note of the running turn is on disk after stop.

The two earlier cuts still hold: || reason?.kind === 'interrupted' → 15/16, the only red being the live-interrupted case; || first === 'turn/start' → 15/16, the only red being the turn/start case.

The at-least-once trade is written where you asked, in docs/subsystems/core-host.md next to the pickup rule: a crash between handing the turn over and the write that drops its note repeats that pickup once, because taken and the inbox that would tell the next start otherwise are process-local — a repeated turn the user can see is the side to take over work the user was told about going silently dead.

Ran: Node 24.10.0 + DSH 0.2.0-rc.2, TZ=UTC:

$ node --import ./test/harness-resolve.mjs --test test/follow-through.test.js
ℹ tests 16  ℹ pass 16  ℹ fail 0        (ten runs at --test-concurrency=6, all green)

bot-messages 20/20, turn-origin 4/4, history 7/7 on the same tree; the patch series reproduces core/ after regenerating resume.js and follow-through.test.js.

[Birfy]

Verdict: INC at 8d9118b
Written-by: Reviewer

The three pins are real and the tip is stable. What blocks it is a note this change leaves behind, which sends the same message again on a later restart — the case that names itself "sent again once" no longer holds across restarts.

Ran:

• cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test --test-timeout=90000 test/follow-through.test.js → 16/16, six runs under --test-concurrency=6
• … test/bot-messages.test.js → 20/20, three runs under --test-concurrency=6
• whole core at --test-concurrency=6 → 1111 tests / 1078 pass / 33 fail; main 0b8aac84 → 1110 / 1077 / 33 with the identical red list (client-bundle, git-install and the three known local ones), so no regression there
• the same file with core/src/host/resume.js replaced by main's → 11/16: a pickup that fails keeps its note for the next start fails with the note is on disk while the pickup is in flight, plus the four older pickup cases — the in-flight pin is genuine
• the same file with the current === record guard replaced by () => undefined → exactly one red: a resumed turn that writes its own note keeps it, message the note the resumed turn wrote is the one that stays (undefined !== 7)
• the same file with return writing removed from the effect cleanup → stop waits for the note it just wrote reds with the note of the running turn is on disk after stop
• the at-least-once paragraph is in docs/subsystems/core-host.md, next to the pickup rule

Blocking: resumeAll seeds queued from the file, and resendQueued then calls rt.deliver, whose wrapper appends a second entry for the same exchangeId. forgetQueued deletes only the first match and returns, so one entry survives the take-up and is persisted:

restart resends a cancelled message, then a turn takes it up:
  main 0b8aac84 → inflight.json queued.b = 0, next start delivers nothing
  this tip      → inflight.json queued.b = 1 (same exchangeId twice), next start delivers "[Message from Chief] Draft the plan" again

That entry is kept for QUEUED_KEPT_MS (24 h), and resent messages are exempt from the 60 s Bot-to-Bot dedupe, so every later restart in that window makes the Bot answer the same request again. main cleared the notes before resending, which is why it does not do this.

Fix, either is enough:

// forgetQueued: drop every entry with that exchange, not just the first one
const kept = list.filter(item => item.args.exchangeId !== exchangeId)
if (kept.length === list.length) continue

or make the wrapper not note an exchange it already holds:

if ((queued[sessionId] ?? []).some(item => item.args.exchangeId === noted.exchangeId) === false) {
  queued = { ...queued, [sessionId]: [...(queued[sessionId] ?? []), { at: Date.now(), args: noted }].slice(-QUEUED_MAX) }
  persist()
}

Pin it in core/test/follow-through.test.js: resend a cancelled message, let its turn take it up (turn/start + user/message carrying that source), then assert the file holds no entry with that exchangeId and that a third start delivers nothing. That test passes on main too — unlike the three pins, it guards the new seeding from leaking rather than a behaviour change, which is the right shape for a regression inside this PR.

Not blocking: endedNormally adds one sessionController.page call per noted turn at start (5 s bound, a failure keeps the pickup). Fine as is; worth knowing on a slow host.

[Birfy]

Written-by: Dev

Fixed at fe42f4c — the INC at 8d9118b1 was right, and it was my miss: the resend path noted an exchange the notes already held, and forgetQueued dropped only the first entry.

Measured the same way you did, before the fix: after a resent message was taken up by a turn, inflight.json held 1 entry for its exchangeId where main holds 0, and a third start sent [Message from Chief] Draft the plan again. With the fix the file holds 0 and the third start sends nothing.

• core/src/host/resume.js: the delivery wrapper does not note an exchange the notes already hold (the resend carries the id it already has); forgetQueued forgets every entry of that exchangeId, not the first — a Session that moved to a new part can have the same exchange noted under two Session ids.
• core/test/follow-through.test.js: new a message sent again is forgotten everywhere once a turn takes it up — send, stop with the note on disk, second start resends it under the same exchange, a turn takes it up, assert no note of that exchange is on disk after stop(), then a third start must send nothing. This one is green on main too, as you said; it is the guard against re-seeding a note, not the revert-red case.
• docs/subsystems/core-host.md: the pickup rule now says the one-note-per-exchange rule and that a take-up forgets every note of the exchange.

Re-verified your three pins at this tip, each cut restored afterwards (17/17 again after each):

• pickup back to clearing first → a pickup that fails keeps its note red on the note is on disk while the pickup is in flight;
• current === record dropped → a resumed turn that writes its own note keeps it red on the note the resumed turn wrote is the one that stays;
• cleanup not returning writing → stop waits for the note it just wrote red;
• the double note back → the new case red on the exchange a turn took up is still noted on disk (16/17).

Ran: follow-through 17/17 three runs at --test-concurrency=6 (Node 24.10.0, DSH 0.2.0-rc.2); whole core suite 1261 / 1258 pass / 3 fail (empty diff passes, composer-label, read-page — the same three on main here); turn-origin 4/4, history 7/7, bot-messages 20/20; patch series re-applied over Fei2411/DeepSeek-Bot@2b10c70 reproduces core/ byte for byte. Not run on this box: oxlint (no oxlint found), ds-bot-patches.mjs check (spawnSync git ENOENT), arch-docs (needs a git checkout); CI's package job runs those.

[Birfy]

Verdict: PASS at fe42f4c
Written-by: Reviewer

The one leak from the last review is fixed, and a test fails when the fix is reverted. The three pickup pins still hold on this tip.

Ran:

• cd core && TZ=UTC DSH_HARNESS=/opt/flybot node --import ./test/harness-resolve.mjs --test --test-timeout=90000 test/follow-through.test.js → 17/17, three runs under --test-concurrency=6
• … test/bot-messages.test.js → 20/20 under --test-concurrency=6
• whole core at --test-concurrency=6 → 1112 tests / 1079 pass / 33 fail; the 33 are the same client-bundle, git-install and three known local ones as main 0b8aac84 (1110 / 1077 / 33)
• the fix reverted (note a second copy of an exchange already in queued, and forgetQueued deleting only the first match) → exactly one red: a message sent again is forgotten everywhere once a turn takes it up, the exchange a turn took up is still noted on disk (1 !== 0)
• an independent probe of that path: restart resends [Message from Chief] Draft the plan, a turn takes it up, inflight.json holds no entry for that exchangeId, third start delivers nothing. On 8d9118b1 the same probe left 1 entry and the third start sent the message again, which is the regression this tip closes
• the three earlier pins, re-checked here: resume.js replaced by main's → 12/17 with the note is on disk while the pickup is in flight among the reds; the current === record guard replaced by () => undefined → exactly one red, the note the resumed turn wrote is the one that stays; return writing removed from the effect cleanup → the note of the running turn is on disk after stop
• patches/ds-bot/0001-core-fixes.patch carries both hunks, and the pickup rule in docs/subsystems/core-host.md now says an exchange is noted once and every note of it is forgotten when a turn takes it up

Not blocking: the new test's settle can read an empty file while a write is in flight, so it may stop waiting early; the assertion after stop() (which waits for the writes) is what decides the case, so the red stays the right one.