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') +})