From 5699bdb466204e896fef959c312010da155220c0 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Tue, 10 Nov 2026 08:57:51 +0800 Subject: [PATCH] fix(host): roll a track_tasks batch back when the store refuses it (#2761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every write path committed the batch into `state.tasks` before `save()`, so a refused store write (a temp name that is not a plain file, a lost lock, a full disk) left cards the file never took in memory — and the next unrelated save persisted them. The claimed card also announced its room line before the save, so a refused claim posted a line for a card nobody held. Snapshot the board on each write path and put it back when the store refuses: the claim, release and rework paths mutate the live card in place, so the snapshot is deep, and the restore writes every key back in order so the board is byte-identical. `state.revision` is left where the store moved it. The claim line moves after a successful save, which also keeps `claimCue` out of that save's bytes (a later write of the same card still persists it). --- .../2761-track-tasks-rollback.fixed.md | 1 + core/src/host/tasks.js | 49 +++++- core/test/track-tasks-batch-refusal.test.js | 152 +++++++++++++++++- docs/subsystems/core-host.md | 2 +- patches/ds-bot/files/src/host/tasks.js | 49 +++++- .../test/track-tasks-batch-refusal.test.js | 152 +++++++++++++++++- 6 files changed, 390 insertions(+), 15 deletions(-) create mode 100644 changelog.d/2761-track-tasks-rollback.fixed.md diff --git a/changelog.d/2761-track-tasks-rollback.fixed.md b/changelog.d/2761-track-tasks-rollback.fixed.md new file mode 100644 index 000000000..4aec92a47 --- /dev/null +++ b/changelog.d/2761-track-tasks-rollback.fixed.md @@ -0,0 +1 @@ +A `track_tasks` batch whose store write was refused no longer leaves the cards it committed in memory: the board is restored before the error, so a ghost card cannot ride the next save onto disk (#2761). diff --git a/core/src/host/tasks.js b/core/src/host/tasks.js index 1db062532..51f5e96ec 100644 --- a/core/src/host/tasks.js +++ b/core/src/host/tasks.js @@ -295,6 +295,40 @@ export function install(rt) { await rt.considerTaskDrive?.() } + // #2761: every write here reaches `state.tasks` before the store takes it, and a refused store + // write (a temp name that is not a plain file, a lost lock, a full disk) used to leave the batch + // in memory for the next unrelated save to persist. A write path snapshots the board first and + // puts it back when the store refuses. The snapshot is deep, unlike the staged copy #2597 uses: + // the claim, release and rework paths mutate the live card in place, so a shallow copy would hold + // the very objects it has to undo. Restoring writes the old references back into the same keys, so + // a caller already holding a card keeps seeing that object; `state.revision` is never touched — + // the store moved it forward before the write, and a refusal does not rewind it. + const snapshotTasks = () => structuredClone(state.tasks ?? {}) + const restoreTasks = (before) => { + state.tasks ??= {} + const live = state.tasks + for (const id of Object.keys(live)) if (!(id in before)) delete live[id] + for (const [id, saved] of Object.entries(before)) { + const card = live[id] + if (!card) { live[id] = saved; continue } + // Every key, in the saved order: a key the failed write deleted comes back where it was, so + // the restored card is byte-identical to the snapshot and not only equal to it. + for (const key of Object.keys(card)) delete card[key] + Object.assign(card, saved) + } + } + const commitTasks = async (before) => { + try { + await rt.save() + } catch (error) { + restoreTasks(before) + throw error + } + // #1205: the wake is not the write. A failure there must not fail the batch, and it must not + // roll one back either — `considerTaskDrive` already catches its own. + await rt.considerTaskDrive?.() + } + const botRecord = id => state.bots?.[id] // The human who owns this Bot. With no signed-in member the single-human product // may see every local Bot, the same way the rest of the task API does. @@ -1022,6 +1056,7 @@ export function install(rt) { await rt.load() const live = find(pick.id) if (live === undefined || live.status !== 'open' || live.claimedBy !== undefined) return { item: null, reason: 'taken' } + const before = snapshotTasks() live.claimedBy = botId live.ownerBotId = botId delete live.retryAt @@ -1031,8 +1066,8 @@ export function install(rt) { // Every claim gets a worker nobody else has started (#1729). live.worker = newWorker() transition(live, 'claimed', null, { by: 'claimed from the queue' }) + await commitTasks(before) announceClaim(live) - await save() return { item: view(live) } } @@ -1064,6 +1099,7 @@ export function install(rt) { await rt.load() const live = find(pick.id) if (live === undefined || live.status !== 'open' || live.claimedBy !== undefined) return { item: null, reason: 'taken' } + const before = snapshotTasks() live.claimedBy = botId live.ownerBotId = botId delete live.retryAt @@ -1072,8 +1108,8 @@ export function install(rt) { live.lease = { holder: botId, takenAt: claimedAt, expiresAt: claimedAt + LEASE_TTL_SEC * 1000 } live.worker = newWorker() transition(live, 'claimed', null, { by: 'claimed from the handoff pool' }) + await commitTasks(before) announceClaim(live) - await save() return { item: view(live) } } @@ -1091,8 +1127,9 @@ export function install(rt) { if (reason === undefined) throw invalid('Returning a card to its producer needs a one-line reason') const producer = oneLine(task.handoff?.from ?? '', ROLE_MAX) if (producer === '' || producer === 'requester') throw refuse('no-producer', `card ${task.id} carries no producer role to return it to`) + const before = snapshotTasks() const child = openRework(task, reason, myRole) - await save() + await commitTasks(before) return view(child) } @@ -1112,8 +1149,9 @@ export function install(rt) { if (!Number.isInteger(seconds) || seconds < 0 || seconds > QUEUE_RETRY_MAX_SEC) { throw invalid(`retryInSec must be a whole number of seconds from 0 to ${QUEUE_RETRY_MAX_SEC}`) } + const before = snapshotTasks() requeue(task, reason, seconds) - await save() + await commitTasks(before) return view(task) } @@ -1296,10 +1334,11 @@ export function install(rt) { } finally { staging = null } + const before = snapshotTasks() for (const id of Object.keys(board)) if (!(id in staged)) delete board[id] for (const [id, task] of Object.entries(staged)) board[id] = task + await commitTasks(before) for (const task of claims) announceClaim(task) - await save() // The conversation path is a board write like task-create and task-update, so a plugin // has to see it: a card a Bot records itself with `workflow:` starts nothing // otherwise (#1462). Routing after the save also keeps a routed owner visible. diff --git a/core/test/track-tasks-batch-refusal.test.js b/core/test/track-tasks-batch-refusal.test.js index d47798c00..1e74269e2 100644 --- a/core/test/track-tasks-batch-refusal.test.js +++ b/core/test/track-tasks-batch-refusal.test.js @@ -4,7 +4,7 @@ // caller was told the batch failed while a card nobody asked for stayed on the board. // The whole store is compared, not one field, so any other early write shows up too. import assert from 'node:assert/strict' -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -66,9 +66,41 @@ async function start(t) { const saves = [] const save = rt.save rt.save = async () => { saves.push(1); return save() } - return { rt, saves } + return { rt, saves, home } } +// A refused *store* write, not a refused validation: the temp name the store stages through is a +// link, so `tempWritable` refuses before one byte lands and every save rejects with `Not saved:`. +// The target outside the home is read back to prove the refusal wrote nothing. +function breakStoreWrites(t, home) { + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-batch-outside-')) + const target = join(outside, 'outside.txt') + writeFileSync(target, 'OUTSIDE\n') + const temp = join(home, `state.json.${process.pid}.tmp`) + symlinkSync(target, temp) + const heal = () => { try { unlinkSync(temp) } catch { /* already gone */ } } + t.after(() => { heal(); rmSync(outside, { recursive: true, force: true }) }) + return { heal, target, home } +} + +const onDisk = home => JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')) + +// A card the board already holds, written straight into the state and saved, so the refusal tests +// start from a real stored board instead of a batch the refused save would also have created. +async function seed(rt, card) { + rt.state.tasks[card.id] = card + await rt.save() + return card +} + +const claimedCard = (over = {}) => ({ + id: 'task-held', status: 'claimed', createdAt: 10, updatedAt: 10, + requesterId: 'owner-1', ownerBotId: 'd1', claimedBy: 'd1', title: 'Held card', + source: 'conversation', origin: { roomId: 'war' }, + actors: [{ status: 'claimed', at: 10, by: 'claimed from the queue' }], + ...over, +}) + const snapshot = rt => JSON.stringify(rt.state.tasks) test('a refused batch leaves state.tasks byte-identical (#2597)', async (t) => { @@ -122,3 +154,119 @@ test('an accepted batch creates every card once (#2597)', async (t) => { assert.equal(cards[1].status, 'working') assert.deepEqual(Object.keys(rt.state.tasks).sort(), [cards[0].id, cards[1].id].sort(), 'both cards landed once') }) + +// #2761: the batch is committed into `state.tasks` before the store takes it, so a refused write +// left cards in memory the file never held, and the next unrelated save persisted them. Every +// write path snapshots the board and puts it back when the store refuses. +test('a refused save leaves a new batch out of state.tasks (#2761)', async (t) => { + const { rt, saves, home } = await start(t) + const before = snapshot(rt) + const revision = rt.state.revision + const store = breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ title: 'Ghost card' }, { title: 'Second ghost', column: 'in-progress' }] }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused save left the board byte-identical') + assert.equal(saves.length, 1, 'the batch was saved once') + assert.equal(rt.state.revision > revision, true, 'the rollback does not rewind revision') + assert.equal(readFileSync(store.target, 'utf8'), 'OUTSIDE\n', 'the refusal wrote no byte outside the home') + assert.deepEqual(Object.keys(rt.state.tasks), [], 'no ghost card stayed behind') + assert.deepEqual(await rt.trackBotTasks('d1', {}), [], 'a read after the refusal finds no card') +}) + +test('a refused save leaves an updated card exactly as it was (#2761)', async (t) => { + const { rt, home } = await start(t) + const [card] = await rt.trackBotTasks('d1', { tasks: [{ title: 'Existing card', group: 'War' }] }) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ id: card.id, title: 'Renamed', column: 'in-progress', note: 'A note' }] }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused save restored the card, not only its key') + assert.equal(rt.state.tasks[card.id].title, 'Existing card') + assert.equal(rt.state.tasks[card.id].note, undefined) +}) + +test('a ghost card cannot ride an unrelated save onto the disk (#2761)', async (t) => { + const { rt, home } = await start(t) + const store = breakStoreWrites(t, home) + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ title: 'Ghost card' }] }), + /Not saved/, + ) + store.heal() + const [real] = await rt.trackBotTasks('d1', { tasks: [{ title: 'Unrelated card' }] }) + const saved = onDisk(home) + assert.deepEqual(Object.keys(saved.tasks ?? {}), [real.id], 'only the card that was saved reached the file') + assert.equal(saved.tasks[real.id].title, 'Unrelated card') +}) + +test('a refused save posts no claim line, a taken one posts exactly one (#2761)', async (t) => { + const { rt, home } = await start(t) + const lines = [] + rt.postClaimLine = task => { lines.push(task.id) } + const batch = { tasks: [{ title: 'Room card', group: 'War', column: 'in-progress' }] } + const store = breakStoreWrites(t, home) + + await assert.rejects(() => rt.trackBotTasks('d1', batch), /Not saved/) + assert.equal(lines.length, 0, 'a card the store refused never reached the room') + + store.heal() + const [card] = await rt.trackBotTasks('d1', batch) + assert.deepEqual(lines, [card.id], 'the same card posts its claim line once it is saved') +}) + +test('a refused claim leaves the queued card open (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, { + id: 'task-queued', status: 'open', createdAt: 10, updatedAt: 10, + requesterId: 'owner-1', ownerBotId: 'a', title: 'Queued card', source: 'conversation', + origin: { roomId: 'war' }, queueRole: 'Dev', queueBy: 'a', queueCap: 8, + actors: [{ status: 'open', at: 10, by: 'conversation' }], + }) + const lines = [] + rt.postClaimLine = task => { lines.push(task.id) } + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects(() => rt.trackBotTasks('d1', { claim: true, id: card.id }, { roomId: 'war' }), /Not saved/) + assert.equal(snapshot(rt), before, 'the refused claim restored the board') + assert.equal(rt.state.tasks[card.id].status, 'open', 'the card is still in the queue') + assert.equal(rt.state.tasks[card.id].claimedBy, undefined, 'nobody holds the card') + assert.equal(lines.length, 0, 'a claim the store refused posts no room line') +}) + +test('a refused release leaves the card claimed (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, claimedCard()) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { release: { task: card.id, reason: 'needs a person', retryInSec: 60 } }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused release restored the board') + assert.equal(rt.state.tasks[card.id].status, 'claimed') + assert.equal(rt.state.tasks[card.id].claimedBy, 'd1') + assert.equal(rt.state.tasks[card.id].retryAt, undefined, 'no retry time was left behind') +}) + +test('a refused rework opens no child card (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, claimedCard({ handoff: { from: 'Reviewer', to: 'Dev', at: 10, reason: 'first pass' } })) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { rework: { task: card.id, reason: 'the fix does not cover it' } }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused rework restored the board') + assert.deepEqual(Object.values(rt.state.tasks).filter(task => task.reworkOf === card.id), [], 'no child card stayed behind') + assert.equal(rt.state.tasks[card.id].handoff.reason, 'first pass', 'the original card kept its own handoff') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 655bb1dde..7216a9e5f 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -278,7 +278,7 @@ Accepted/failed/cancelled tasks stay terminal. This records the requester's task disposition, not proof that an external runtime stopped; no runtime abort or side-effect rollback is implied. -A tracked request with a stable id (`task-`) and a small state machine, owned by `src/host/tasks.js` with `state.tasks` (additive; corrupt values reset, no version bump). States: `open → claimed → working → blocked ⇄ working → result-ready → accepted`, with `cancelled`/`failed` terminal and `accepted` reachable only from `result-ready` — a failed or cancelled task can never display as accepted (#54). Transitions after create are the requester's (server-derived `requesterId`, the signed-in actor or the single owner; `bot/forbidden` otherwise, `bot/stale` on invalid moves), recorded with per-status actor entries. Create does not require owning the Bot: a matching per-Bot grant, or both the requester and the Bot as current members of the named room, is enough; outgoing holds stay the owner's (#52 card). A working task's held send blocks **that** task (`meta.taskId` stamped on the send, matching origin when the Bot has several working tasks; `updatedAt` is strictly increasing, so two starts in one millisecond still order) with the approval id; the owner's decide resumes (approve) or cancels (reject) it; deleting the responsible Bot fails its open tasks. `result-ready` is protected from prune while it waits for acceptance. Endpoints `tasks` / `task-create` / `task-accept` / `task-start` / `task-return` / `task-accept-result` / `task-cancel` are an explicit authz allow-list and are record-checked in tasks.js. Records store pointers and a copied summary, never private transcripts. The three 2,000-unit text bounds — the summary a Done move copies from the card's note, the criteria stored at create, and the summary a return writes — cut on a code point boundary (`cut` in `src/shared/text-boundary.js`) rather than with `slice`, so a bound that lands inside a surrogate pair drops the pair whole instead of storing a lone surrogate the card would show as U+FFFD ([#1956](https://github.com/ThinkFlowLab/FlyBot/issues/1956)). The 300-unit `oneLine` bound those cards share with a room dispatch (`tasks.js` and `work-dispatch.js`) goes through the same `cut`, so a title, a note or a dispatch line that lands inside a pair keeps whole characters rather than half of one. Clarification and `workBotId` wait on #55's exchange wiring. Team mode stays disabled. After `task-create`, `task-update` and `track_tasks` save, the host asks `rt.onTaskWrite` (hooks exposes it as `flybot.tasks.onWrite`, [plugin-runtime](plugin-runtime.md)). A returned `ownerBotId` that names a different local Bot is stored and saved once more; that second save does not ask again. A missing, remote or identical id leaves the owner (`core/test/task-route.test.js`, #1318). `track_tasks` routes every card it saved, after its own save and before it answers, so a card a Bot records itself reaches the same listeners as a board write made through the channel API (#1462). +A tracked request with a stable id (`task-`) and a small state machine, owned by `src/host/tasks.js` with `state.tasks` (additive; corrupt values reset, no version bump). States: `open → claimed → working → blocked ⇄ working → result-ready → accepted`, with `cancelled`/`failed` terminal and `accepted` reachable only from `result-ready` — a failed or cancelled task can never display as accepted (#54). Transitions after create are the requester's (server-derived `requesterId`, the signed-in actor or the single owner; `bot/forbidden` otherwise, `bot/stale` on invalid moves), recorded with per-status actor entries. Create does not require owning the Bot: a matching per-Bot grant, or both the requester and the Bot as current members of the named room, is enough; outgoing holds stay the owner's (#52 card). A working task's held send blocks **that** task (`meta.taskId` stamped on the send, matching origin when the Bot has several working tasks; `updatedAt` is strictly increasing, so two starts in one millisecond still order) with the approval id; the owner's decide resumes (approve) or cancels (reject) it; deleting the responsible Bot fails its open tasks. `result-ready` is protected from prune while it waits for acceptance. Endpoints `tasks` / `task-create` / `task-accept` / `task-start` / `task-return` / `task-accept-result` / `task-cancel` are an explicit authz allow-list and are record-checked in tasks.js. Records store pointers and a copied summary, never private transcripts. The three 2,000-unit text bounds — the summary a Done move copies from the card's note, the criteria stored at create, and the summary a return writes — cut on a code point boundary (`cut` in `src/shared/text-boundary.js`) rather than with `slice`, so a bound that lands inside a surrogate pair drops the pair whole instead of storing a lone surrogate the card would show as U+FFFD ([#1956](https://github.com/ThinkFlowLab/FlyBot/issues/1956)). The 300-unit `oneLine` bound those cards share with a room dispatch (`tasks.js` and `work-dispatch.js`) goes through the same `cut`, so a title, a note or a dispatch line that lands inside a pair keeps whole characters rather than half of one. Clarification and `workBotId` wait on #55's exchange wiring. Team mode stays disabled. After `task-create`, `task-update` and `track_tasks` save, the host asks `rt.onTaskWrite` (hooks exposes it as `flybot.tasks.onWrite`, [plugin-runtime](plugin-runtime.md)). A returned `ownerBotId` that names a different local Bot is stored and saved once more; that second save does not ask again. A missing, remote or identical id leaves the owner (`core/test/task-route.test.js`, #1318). `track_tasks` routes every card it saved, after its own save and before it answers, so a card a Bot records itself reaches the same listeners as a board write made through the channel API (#1462). A `track_tasks` write is one batch in both halves: the list is validated on a staged copy, and a store write the store refuses — a temp name that is not a plain file, a lost lock, a full disk — puts `state.tasks` back before the tool answers `Not saved:`, so no card the file never took stays on the board for the next save to persist; the restore keeps the old card objects (a held reference still sees the card) and leaves `state.revision` where the store moved it, and the room claim line goes out only after the save succeeded, so a refused claim posts nothing and its `claimCue` is not part of that save (#2761). ### The typed task contract (#1720) diff --git a/patches/ds-bot/files/src/host/tasks.js b/patches/ds-bot/files/src/host/tasks.js index 1db062532..51f5e96ec 100644 --- a/patches/ds-bot/files/src/host/tasks.js +++ b/patches/ds-bot/files/src/host/tasks.js @@ -295,6 +295,40 @@ export function install(rt) { await rt.considerTaskDrive?.() } + // #2761: every write here reaches `state.tasks` before the store takes it, and a refused store + // write (a temp name that is not a plain file, a lost lock, a full disk) used to leave the batch + // in memory for the next unrelated save to persist. A write path snapshots the board first and + // puts it back when the store refuses. The snapshot is deep, unlike the staged copy #2597 uses: + // the claim, release and rework paths mutate the live card in place, so a shallow copy would hold + // the very objects it has to undo. Restoring writes the old references back into the same keys, so + // a caller already holding a card keeps seeing that object; `state.revision` is never touched — + // the store moved it forward before the write, and a refusal does not rewind it. + const snapshotTasks = () => structuredClone(state.tasks ?? {}) + const restoreTasks = (before) => { + state.tasks ??= {} + const live = state.tasks + for (const id of Object.keys(live)) if (!(id in before)) delete live[id] + for (const [id, saved] of Object.entries(before)) { + const card = live[id] + if (!card) { live[id] = saved; continue } + // Every key, in the saved order: a key the failed write deleted comes back where it was, so + // the restored card is byte-identical to the snapshot and not only equal to it. + for (const key of Object.keys(card)) delete card[key] + Object.assign(card, saved) + } + } + const commitTasks = async (before) => { + try { + await rt.save() + } catch (error) { + restoreTasks(before) + throw error + } + // #1205: the wake is not the write. A failure there must not fail the batch, and it must not + // roll one back either — `considerTaskDrive` already catches its own. + await rt.considerTaskDrive?.() + } + const botRecord = id => state.bots?.[id] // The human who owns this Bot. With no signed-in member the single-human product // may see every local Bot, the same way the rest of the task API does. @@ -1022,6 +1056,7 @@ export function install(rt) { await rt.load() const live = find(pick.id) if (live === undefined || live.status !== 'open' || live.claimedBy !== undefined) return { item: null, reason: 'taken' } + const before = snapshotTasks() live.claimedBy = botId live.ownerBotId = botId delete live.retryAt @@ -1031,8 +1066,8 @@ export function install(rt) { // Every claim gets a worker nobody else has started (#1729). live.worker = newWorker() transition(live, 'claimed', null, { by: 'claimed from the queue' }) + await commitTasks(before) announceClaim(live) - await save() return { item: view(live) } } @@ -1064,6 +1099,7 @@ export function install(rt) { await rt.load() const live = find(pick.id) if (live === undefined || live.status !== 'open' || live.claimedBy !== undefined) return { item: null, reason: 'taken' } + const before = snapshotTasks() live.claimedBy = botId live.ownerBotId = botId delete live.retryAt @@ -1072,8 +1108,8 @@ export function install(rt) { live.lease = { holder: botId, takenAt: claimedAt, expiresAt: claimedAt + LEASE_TTL_SEC * 1000 } live.worker = newWorker() transition(live, 'claimed', null, { by: 'claimed from the handoff pool' }) + await commitTasks(before) announceClaim(live) - await save() return { item: view(live) } } @@ -1091,8 +1127,9 @@ export function install(rt) { if (reason === undefined) throw invalid('Returning a card to its producer needs a one-line reason') const producer = oneLine(task.handoff?.from ?? '', ROLE_MAX) if (producer === '' || producer === 'requester') throw refuse('no-producer', `card ${task.id} carries no producer role to return it to`) + const before = snapshotTasks() const child = openRework(task, reason, myRole) - await save() + await commitTasks(before) return view(child) } @@ -1112,8 +1149,9 @@ export function install(rt) { if (!Number.isInteger(seconds) || seconds < 0 || seconds > QUEUE_RETRY_MAX_SEC) { throw invalid(`retryInSec must be a whole number of seconds from 0 to ${QUEUE_RETRY_MAX_SEC}`) } + const before = snapshotTasks() requeue(task, reason, seconds) - await save() + await commitTasks(before) return view(task) } @@ -1296,10 +1334,11 @@ export function install(rt) { } finally { staging = null } + const before = snapshotTasks() for (const id of Object.keys(board)) if (!(id in staged)) delete board[id] for (const [id, task] of Object.entries(staged)) board[id] = task + await commitTasks(before) for (const task of claims) announceClaim(task) - await save() // The conversation path is a board write like task-create and task-update, so a plugin // has to see it: a card a Bot records itself with `workflow:` starts nothing // otherwise (#1462). Routing after the save also keeps a routed owner visible. diff --git a/patches/ds-bot/files/test/track-tasks-batch-refusal.test.js b/patches/ds-bot/files/test/track-tasks-batch-refusal.test.js index d47798c00..1e74269e2 100644 --- a/patches/ds-bot/files/test/track-tasks-batch-refusal.test.js +++ b/patches/ds-bot/files/test/track-tasks-batch-refusal.test.js @@ -4,7 +4,7 @@ // caller was told the batch failed while a card nobody asked for stayed on the board. // The whole store is compared, not one field, so any other early write shows up too. import assert from 'node:assert/strict' -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { mkdtempSync, readFileSync, rmSync, symlinkSync, unlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -66,9 +66,41 @@ async function start(t) { const saves = [] const save = rt.save rt.save = async () => { saves.push(1); return save() } - return { rt, saves } + return { rt, saves, home } } +// A refused *store* write, not a refused validation: the temp name the store stages through is a +// link, so `tempWritable` refuses before one byte lands and every save rejects with `Not saved:`. +// The target outside the home is read back to prove the refusal wrote nothing. +function breakStoreWrites(t, home) { + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-batch-outside-')) + const target = join(outside, 'outside.txt') + writeFileSync(target, 'OUTSIDE\n') + const temp = join(home, `state.json.${process.pid}.tmp`) + symlinkSync(target, temp) + const heal = () => { try { unlinkSync(temp) } catch { /* already gone */ } } + t.after(() => { heal(); rmSync(outside, { recursive: true, force: true }) }) + return { heal, target, home } +} + +const onDisk = home => JSON.parse(readFileSync(join(home, 'state.json'), 'utf8')) + +// A card the board already holds, written straight into the state and saved, so the refusal tests +// start from a real stored board instead of a batch the refused save would also have created. +async function seed(rt, card) { + rt.state.tasks[card.id] = card + await rt.save() + return card +} + +const claimedCard = (over = {}) => ({ + id: 'task-held', status: 'claimed', createdAt: 10, updatedAt: 10, + requesterId: 'owner-1', ownerBotId: 'd1', claimedBy: 'd1', title: 'Held card', + source: 'conversation', origin: { roomId: 'war' }, + actors: [{ status: 'claimed', at: 10, by: 'claimed from the queue' }], + ...over, +}) + const snapshot = rt => JSON.stringify(rt.state.tasks) test('a refused batch leaves state.tasks byte-identical (#2597)', async (t) => { @@ -122,3 +154,119 @@ test('an accepted batch creates every card once (#2597)', async (t) => { assert.equal(cards[1].status, 'working') assert.deepEqual(Object.keys(rt.state.tasks).sort(), [cards[0].id, cards[1].id].sort(), 'both cards landed once') }) + +// #2761: the batch is committed into `state.tasks` before the store takes it, so a refused write +// left cards in memory the file never held, and the next unrelated save persisted them. Every +// write path snapshots the board and puts it back when the store refuses. +test('a refused save leaves a new batch out of state.tasks (#2761)', async (t) => { + const { rt, saves, home } = await start(t) + const before = snapshot(rt) + const revision = rt.state.revision + const store = breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ title: 'Ghost card' }, { title: 'Second ghost', column: 'in-progress' }] }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused save left the board byte-identical') + assert.equal(saves.length, 1, 'the batch was saved once') + assert.equal(rt.state.revision > revision, true, 'the rollback does not rewind revision') + assert.equal(readFileSync(store.target, 'utf8'), 'OUTSIDE\n', 'the refusal wrote no byte outside the home') + assert.deepEqual(Object.keys(rt.state.tasks), [], 'no ghost card stayed behind') + assert.deepEqual(await rt.trackBotTasks('d1', {}), [], 'a read after the refusal finds no card') +}) + +test('a refused save leaves an updated card exactly as it was (#2761)', async (t) => { + const { rt, home } = await start(t) + const [card] = await rt.trackBotTasks('d1', { tasks: [{ title: 'Existing card', group: 'War' }] }) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ id: card.id, title: 'Renamed', column: 'in-progress', note: 'A note' }] }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused save restored the card, not only its key') + assert.equal(rt.state.tasks[card.id].title, 'Existing card') + assert.equal(rt.state.tasks[card.id].note, undefined) +}) + +test('a ghost card cannot ride an unrelated save onto the disk (#2761)', async (t) => { + const { rt, home } = await start(t) + const store = breakStoreWrites(t, home) + await assert.rejects( + () => rt.trackBotTasks('d1', { tasks: [{ title: 'Ghost card' }] }), + /Not saved/, + ) + store.heal() + const [real] = await rt.trackBotTasks('d1', { tasks: [{ title: 'Unrelated card' }] }) + const saved = onDisk(home) + assert.deepEqual(Object.keys(saved.tasks ?? {}), [real.id], 'only the card that was saved reached the file') + assert.equal(saved.tasks[real.id].title, 'Unrelated card') +}) + +test('a refused save posts no claim line, a taken one posts exactly one (#2761)', async (t) => { + const { rt, home } = await start(t) + const lines = [] + rt.postClaimLine = task => { lines.push(task.id) } + const batch = { tasks: [{ title: 'Room card', group: 'War', column: 'in-progress' }] } + const store = breakStoreWrites(t, home) + + await assert.rejects(() => rt.trackBotTasks('d1', batch), /Not saved/) + assert.equal(lines.length, 0, 'a card the store refused never reached the room') + + store.heal() + const [card] = await rt.trackBotTasks('d1', batch) + assert.deepEqual(lines, [card.id], 'the same card posts its claim line once it is saved') +}) + +test('a refused claim leaves the queued card open (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, { + id: 'task-queued', status: 'open', createdAt: 10, updatedAt: 10, + requesterId: 'owner-1', ownerBotId: 'a', title: 'Queued card', source: 'conversation', + origin: { roomId: 'war' }, queueRole: 'Dev', queueBy: 'a', queueCap: 8, + actors: [{ status: 'open', at: 10, by: 'conversation' }], + }) + const lines = [] + rt.postClaimLine = task => { lines.push(task.id) } + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects(() => rt.trackBotTasks('d1', { claim: true, id: card.id }, { roomId: 'war' }), /Not saved/) + assert.equal(snapshot(rt), before, 'the refused claim restored the board') + assert.equal(rt.state.tasks[card.id].status, 'open', 'the card is still in the queue') + assert.equal(rt.state.tasks[card.id].claimedBy, undefined, 'nobody holds the card') + assert.equal(lines.length, 0, 'a claim the store refused posts no room line') +}) + +test('a refused release leaves the card claimed (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, claimedCard()) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { release: { task: card.id, reason: 'needs a person', retryInSec: 60 } }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused release restored the board') + assert.equal(rt.state.tasks[card.id].status, 'claimed') + assert.equal(rt.state.tasks[card.id].claimedBy, 'd1') + assert.equal(rt.state.tasks[card.id].retryAt, undefined, 'no retry time was left behind') +}) + +test('a refused rework opens no child card (#2761)', async (t) => { + const { rt, home } = await start(t) + const card = await seed(rt, claimedCard({ handoff: { from: 'Reviewer', to: 'Dev', at: 10, reason: 'first pass' } })) + const before = snapshot(rt) + breakStoreWrites(t, home) + + await assert.rejects( + () => rt.trackBotTasks('d1', { rework: { task: card.id, reason: 'the fix does not cover it' } }), + /Not saved/, + ) + assert.equal(snapshot(rt), before, 'the refused rework restored the board') + assert.deepEqual(Object.values(rt.state.tasks).filter(task => task.reworkOf === card.id), [], 'no child card stayed behind') + assert.equal(rt.state.tasks[card.id].handoff.reason, 'first pass', 'the original card kept its own handoff') +})