Repository navigation
Conversation
|
Verdict: INC at c65f041 The fix is the right one and its main case is pinned: withdraw the 1. The test('a live interrupted end does not drop the note of a turn still owed a pickup', async (t) => {
t.mock.timers.enable({ apis: ['setTimeout'] })
const home = freshHome(t)
const first = await start(home)
run(first, 'a', 1, { content: [{ type: 'text', text: 'hello' }], source: { kind: 'user' } })
await settle(() => inflight(home).a !== undefined)
first.emit('a', 'turn/end', { turn: 1, reason: { kind: 'interrupted' } })
assert.notEqual(inflight(home).a, undefined, 'the note survives an interrupted end')
await first.stop()
const second = await start(home)
t.mock.timers.tick(3000)
await settle(() => second.agentOf('a').calls.length === 1)
})( 2. The new test('a turn/start this process sees proves the noted turn ended, even when the log cannot', async (t) => {
t.mock.timers.enable({ apis: ['setTimeout'] })
const home = freshHome(t)
const first = await start(home)
run(first, 'a', 1, { content: [{ type: 'text', text: 'hello' }], source: { kind: 'user' } })
await settle(() => inflight(home).a !== undefined)
await first.stop() // the crash wrote no turn/end
const second = await start(home)
second.emit('a', 'turn/start', { turn: 2 })
t.mock.timers.tick(3000)
await new Promise(resolve => setImmediate(resolve))
assert.equal(second.agentOf('a').calls.length, 0)
})Both use the fixtures this PR already added ( Non-blocking.
Ran (tip Red, four cuts, each restored afterwards: Not run: |
github-actions
Bot
added
the
risk:T2
resume.js decided a noted turn was cut from the first turn event this process saw; a turn that ended `completed` while the plugin was not listening was resumed as a restart and its work repeated. Record the turn number and ask the durable Session log (only `interrupted`, an abort caused by `disposed`, or no end is picked up), and let a `turn/start` or an `interrupted` end close the note's turn. The pickup also cleared inflight.json before delivering: one failed delivery (or a crash during the pickup) lost the note for good. Seed the live notes from what was read, drop a turn's note just before its pickup and put it back if that fails, and keep a queued message whose resend failed. Fixes ThinkFlowLab#727 Fixes ThinkFlowLab#747
c65f041 to
4399d1f
Compare
HugoXK
changed the title
|
Verdict: INC at 4399d1f Two of the four 1. The live 2. The 3. The new Already good: a Ran:
Not run: |
|
Written-by: Dev2 Takeover, and the reason it is a takeover rather than a new commit here. Your branch cannot be pushed to from this side, and the obvious probe says otherwise. A no-op ref update on So the change continues on a branch we own: #759,
Closing this PR as superseded by #759 — your commits and this review history stay as the record. If you would rather own the branch, push a rebase yourself and say so: we would then take your tip and this PR could be reopened instead of the takeover. Sorry for the extra hop: the earlier "we can push to forks" finding was that same false positive. |
) Takeover of #728: the author's fork cannot be pushed to from here (a ref update to the same SHA answers 200, but creating a blob in it is 404, so the obvious probe was a false positive), so this carries the same change on a branch we own. A turn that finished while FlyBot was not listening was replayed: the note was kept for anything that was not a local `completed` end, and a restart picked it up. Now an `interrupted` end keeps the note, a `turn/start` this process sees proves the noted turn ended, and a note whose turn the durable log already shows as finished is dropped. The merge queue showed the other half: a failed pickup dropped its note, which was removed before the delivery and only put back after it failed — the file could be read without it, and a crash in that window lost it for good. The note now stays until the pickup is handed over, and only a delivered pickup drops it. Reviewed before this branch existed, on #728: `Verdict: INC at 4399d1f` — two branches were unpinned and the case for a failed pickup read the note file too early. Both cases now come from that review and each turns red when its branch is cut. Co-authored-by: HugoXK <HugoXK@users.noreply.github.com>
) (#759) 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 #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 #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 #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.
Pick up a restart's work safely
Fixes #727
Fixes #747
Two failures in the restart pickup (
core/src/host/resume.js), both losing or repeating work.A finished turn was replayed.
resumeAlldecided "cut off" from the firstturn/start/turn/endthis process saw. Events a reloaded plugin missed are not replayed, so a turn that ended while the plugin was not listening (a plugin reload, or a crash right after DSH wroteturn/end) was resumed as "DeepSeek Harness restarted while you were working", repeating finished work. The note now records the turn number andresumeAllasks the durable Session log:interrupted, an abort caused bydisposed, or no end is picked up;completed/blocked/max-tokens/forked, or an abort by the user, hook or parent, is not repeated. Aturn/startseen after the start proves the noted turn ended, and aninterruptedend now keeps the note instead of clearing it. Notes withoutturnkeep the old behaviour.A failed pickup dropped the note.
resumeAllclearedinflight.jsonand persisted the empty file before resending or picking anything up, so one failed delivery (a Session DSH has not opened yet, a transient error) or a crash during the pickup lost the note forever. The notes read at start are now seeded into the liveturns/queued(the file keeps everything owed), a turn's note is removed just before its pickup and put back if that fails before a new turn notes itself, and a queued message whose resend fails is kept for the next start.core/test/follow-through.test.jskeeps a durable per-home log and a mockedsessionController.page: a turn that endscompletedwhile no host listens is not replayed and one that endsinterruptedstill is; a start whose Session cannot open keeps its note and a third start picks it up. Each new case fails without its change.Ran at the tip:
npm run verify(green,TZ=UTC),ARCH_DOCS_BASE=upstream/main node core/scripts/check-arch-docs.mjs,ds-bot-patches generate+check. Not run: smoke (this macOS box has nosetsid, sodev/bench.shcannot startdsh web), Desktop, real-model steward.