From 460b89e4c56ae080ed0d1f571d3993b14cd309cf Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Fri, 30 Oct 2026 11:39:11 +0800 Subject: [PATCH] fix(host): a meeting archives its decision before it creates cards A decide wrote the produced cards first and the decision file after, so an unwritable `decisions/` folder left the cards on the board while the meeting stayed `deliberating`, and a retry created a second card for the same task. The temp file is now claimed before any card exists, and an attempt that dies after the cards leaves their ids on the meeting record so the retry keeps that card instead of making another. --- ...1763-meetings-decide-orphan-cards.fixed.md | 1 + core/src/host/meetings.js | 58 +++++++++-- core/test/meetings-decide-paths.test.js | 96 ++++++++++++++++++- docs/subsystems/core-host.md | 5 +- patches/ds-bot/files/src/host/meetings.js | 58 +++++++++-- .../files/test/meetings-decide-paths.test.js | 96 ++++++++++++++++++- 6 files changed, 293 insertions(+), 21 deletions(-) create mode 100644 changelog.d/1763-meetings-decide-orphan-cards.fixed.md diff --git a/changelog.d/1763-meetings-decide-orphan-cards.fixed.md b/changelog.d/1763-meetings-decide-orphan-cards.fixed.md new file mode 100644 index 000000000..0f05104d3 --- /dev/null +++ b/changelog.d/1763-meetings-decide-orphan-cards.fixed.md @@ -0,0 +1 @@ +A meeting that cannot archive its decision no longer leaves a card behind: the archive folder is claimed before any produced card is created, so an unwritable `decisions/` folder fails the decide with no card, no decision file and the meeting still open, and an attempt that fails after the cards exist keeps their ids on the meeting record so a retry reuses that card instead of creating a second one ([#1763](https://github.com/ThinkFlowLab/FlyBot/issues/1763)). diff --git a/core/src/host/meetings.js b/core/src/host/meetings.js index 97201226d..57a48cf74 100644 --- a/core/src/host/meetings.js +++ b/core/src/host/meetings.js @@ -6,7 +6,7 @@ // meeting writes no file and produces no task. Decisions stay on the existing store // (`state.meetings`), so there is no second store. import { createHash, randomBytes } from 'node:crypto' -import { link, mkdir, readFile, rm, writeFile } from 'node:fs/promises' +import { link, mkdir, open, readFile, rm } from 'node:fs/promises' import { join } from 'node:path' import { defineTool } from './tool-schema.js' import { output } from './tools.js' @@ -237,6 +237,22 @@ export function install(rt) { }) } + // P-205: the archive folder is touched before any card exists. Claiming the temp file proves the + // folder takes a write, so a decide that cannot archive its file fails with no card created, + // instead of leaving cards behind for the retry to double. The handle is filled once the ids exist. + const claimTemp = async (dir, name) => { + const temp = `${join(dir, name)}.tmp-${randomBytes(4).toString('hex')}` + return { temp, handle: await open(temp, 'w') } + } + const dropTemp = async ({ temp, handle }) => { + try { + await handle.close() + } catch { + // closed already: the write path closes before it links + } + await rm(temp, { force: true }) + } + const decide = async (meeting, args, { now, actor }) => { if (!OPEN_STATES.has(meeting.status)) throw refuse('closed', `the meeting is ${meeting.status}`) if (actor?.role === ASSISTANT_ROLE) throw refuse('forbidden', 'a Project Assistant takes a position; it does not close the meeting') @@ -275,8 +291,27 @@ export function install(rt) { closedBy: actor?.name ?? candidate.owner, decisionFile: file, } - // The cards come first: the decision file appears only once every produced task exists. - closed.taskIds = await produceTasks(planned, { actor }) + // P-205: the step that fails most often — writing into `decisions/` — now runs before any card, + // so an unwritable folder leaves the meeting exactly as it was. The temp is claimed here. + let slot = await claimTemp(dir, file) + // P-205: the cards come first, but a retry must not double them. If an earlier attempt died + // between the cards and the finished file, its ids are on the meeting record and this attempt + // keeps that card instead of creating a second one. A re-opened meeting starts from no ids. + const pending = Array.isArray(meeting.pendingTaskIds) ? meeting.pendingTaskIds : [] + try { + if (pending.length > 0) { + closed.taskIds = pending.filter(id => rt.state.tasks?.[id] !== undefined) + } else { + closed.taskIds = await produceTasks(planned, { actor }) + // On the record before the file is written, while the meeting is still `deliberating`: a + // failure below (a full disk, a `link` the loop cannot retry) must not double the cards. + store()[meeting.id] = { ...meeting, pendingTaskIds: closed.taskIds } + } + } catch (error) { + await dropTemp(slot) + throw error + } + delete closed.pendingTaskIds // One render, and the bytes hashed are the bytes archived. `link` claims the name without // rewriting an existing file; a name lost to a concurrent writer takes the next one, and the @@ -284,21 +319,26 @@ export function install(rt) { let bytes let name = file for (let attempt = 0; ; attempt += 1) { - if (attempt === 8) throw refuse('exists', `no free decision file name left for ${file}`) + if (attempt === 8) { + await dropTemp(slot) + throw refuse('exists', `no free decision file name left for ${file}`) + } closed.decisionFile = name bytes = Buffer.from(decisionBody(closed, name, now), 'utf8') - const temp = `${join(dir, name)}.tmp-${randomBytes(4).toString('hex')}` - await writeFile(temp, bytes) try { - await link(temp, join(dir, name)) + // The temp claimed before the cards is filled here: its bytes are the bytes archived. + await slot.handle.writeFile(bytes) + await slot.handle.close() + await link(slot.temp, join(dir, name)) } catch (error) { - await rm(temp, { force: true }) + await dropTemp(slot) if (error?.code !== 'EEXIST') throw error name = await nextFreeFile(dir, now, candidate.slug) await repointTasks(closed.taskIds, name, { now, actor }) + slot = await claimTemp(dir, name) continue } - await rm(temp, { force: true }) + await rm(slot.temp, { force: true }) break } closed.decisionSha256 = createHash('sha256').update(bytes).digest('hex') diff --git a/core/test/meetings-decide-paths.test.js b/core/test/meetings-decide-paths.test.js index 360342f35..0bac58a3b 100644 --- a/core/test/meetings-decide-paths.test.js +++ b/core/test/meetings-decide-paths.test.js @@ -6,7 +6,7 @@ // - P-192: `blocked` had no way back; the meeting stayed blocked for good. import assert from 'node:assert/strict' import { createHash } from 'node:crypto' -import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -272,3 +272,97 @@ test('meetings: a host without the create gate refuses the produced task instead assert.deepEqual(filesIn(dir), [], 'no decision file is archived') assert.equal(rt.state.meetings[id].status, 'deliberating', 'the meeting did not close') }) + +// P-205: the archive step that fails leaves nothing behind. The `decisions` folder is writable by +// name but not by content here, and the decide must fail before a card exists — the base created +// the card first, so every retry added one more. +test('meetings: a decide that cannot archive its file creates no card, and its retry creates none either', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + const folder = join(dir, 'decisions') + t.after(async () => { + chmodSync(folder, 0o755) + await stop() + }) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + mkdirSync(folder) + chmodSync(folder, 0o555) + + for (const attempt of [1, 2]) { + const refusal = await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }) + .then(() => undefined, error => error) + assert.equal(refusal?.code, 'EACCES', `attempt ${attempt} fails on the folder itself`) + assert.equal(Object.keys(rt.state.tasks ?? {}).length, 0, `attempt ${attempt} creates no card`) + assert.deepEqual(tempsIn(dir), [], `attempt ${attempt} leaves no temp file behind`) + assert.deepEqual(filesIn(dir), [], `attempt ${attempt} archives no decision file`) + assert.equal(rt.state.meetings[id].status, 'deliberating', `attempt ${attempt} leaves the meeting open`) + } +}) + +// P-205: once the folder takes the write again the same decide lands on the name the failures +// would have used, and the ids its file lists are exactly the cards on the board. +test('meetings: a retry once the folder is writable archives the file and lists every card', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + const folder = join(dir, 'decisions') + t.after(async () => { + chmodSync(folder, 0o755) + await stop() + }) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + mkdirSync(folder) + chmodSync(folder, 0o555) + await assert.rejects(decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }), /EACCES: permission denied/) + chmodSync(folder, 0o755) + + await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }, { botId: 'r1', title: 'Review the record' }] }) + const record = rt.state.meetings[id] + const body = readFileSync(join(dir, record.decisionFile), 'utf8') + assert.match(record.decisionFile, /-which-storage-shape-should-stage-4-use\.md$/, 'the retry keeps the name the failures left free') + assert.deepEqual(filesIn(dir), [record.decisionFile.replace('decisions/', '')], 'exactly one file, under that name') + assert.equal(record.status, 'decided') + assert.equal(record.taskIds.length, 2, 'both produced tasks are on the record') + assert.equal(Object.keys(rt.state.tasks).length, 2, 'and both are on the board') + assert.equal(record.taskIds.length, body.split('\n').filter(line => line.startsWith('- Tasks: '))[0].slice('- Tasks: '.length).split(', ').length, 'the file lists as many ids as the record keeps') + assert.ok(body.includes(`- Tasks: ${record.taskIds.join(', ')}`), 'the file lists the kept ids in order') + assert.equal(record.decisionSha256, sha256(body), 'the recorded sha covers the archived bytes') + assert.equal(record.pendingTaskIds, undefined, 'a decided meeting carries no retry bookkeeping') + assert.deepEqual(tempsIn(dir), [], 'no temp file survives the success') +}) + +// P-205, the window no filesystem lever opens in-process: a decide that died between the cards and +// the finished file left its ids on the meeting record. The retry must keep those cards rather than +// create a second one for the same produced task — and the file it writes lists the kept card. +test('meetings: a retry after the cards exist keeps them instead of creating another', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + t.after(stop) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + // The card an earlier attempt created, and the record it left behind: ids on the meeting, the + // meeting still `deliberating`, no file. + const card = await rt.taskEndpoint('task-create', { botId: 'd1', title: 'Write the record' }, { name: 'Chief', role: 'Chief' }) + rt.state.meetings[id].pendingTaskIds = [card.id] + + // Two more attempts while the folder still refuses the write: neither adds a card, and neither + // leaves a file or a temp behind. + const folder = join(dir, 'decisions') + mkdirSync(folder) + chmodSync(folder, 0o555) + t.after(() => { try { chmodSync(folder, 0o755) } catch { /* the tree is gone with the test */ } }) + for (const attempt of [1, 2]) { + await assert.rejects(decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }), /EACCES: permission denied/, `attempt ${attempt} still fails on the folder`) + assert.deepEqual(Object.keys(rt.state.tasks), [card.id], `attempt ${attempt} adds no second card`) + assert.deepEqual(filesIn(dir), [], `attempt ${attempt} archives nothing`) + assert.deepEqual(tempsIn(dir), [], `attempt ${attempt} leaves no temp behind`) + } + chmodSync(folder, 0o755) + + await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }) + const record = rt.state.meetings[id] + const body = readFileSync(join(dir, record.decisionFile), 'utf8') + assert.deepEqual(Object.keys(rt.state.tasks), [card.id], 'the retry built no second card') + assert.deepEqual(record.taskIds, [card.id], 'the record keeps the card the first attempt made') + assert.ok(body.includes(`- Tasks: ${card.id}`), 'the file names that card') + assert.equal(record.pendingTaskIds, undefined, 'the bookkeeping is gone once the file lands') + assert.deepEqual(filesIn(dir), [record.decisionFile.replace('decisions/', '')], 'one file, no second temp') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index ce840ca93..191854266 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -634,7 +634,10 @@ no second store, no UI-only path and no cron. temp file and no decision file. A host that carries no such gate refuses the produced task `forbidden` rather than creating it ungated. The decision body is rendered once, after the cards exist, so the `- Tasks:` line names the ids the meeting keeps and the recorded sha256 is the - sha256 of the bytes archived. + sha256 of the bytes archived. The archive folder is claimed (a temp file opened) before any card + exists, so a folder that refuses the write fails the decide with no card created, and an attempt + that died after the cards leaves their ids on the meeting record so a retry keeps them rather + than creating a second card. - **Who may close.** A Project Assistant (`role: 'Project Assistant'`) may be invited and record a position under its own name; it may not write the decision file or close the meeting, which is refused `forbidden`. diff --git a/patches/ds-bot/files/src/host/meetings.js b/patches/ds-bot/files/src/host/meetings.js index 97201226d..57a48cf74 100644 --- a/patches/ds-bot/files/src/host/meetings.js +++ b/patches/ds-bot/files/src/host/meetings.js @@ -6,7 +6,7 @@ // meeting writes no file and produces no task. Decisions stay on the existing store // (`state.meetings`), so there is no second store. import { createHash, randomBytes } from 'node:crypto' -import { link, mkdir, readFile, rm, writeFile } from 'node:fs/promises' +import { link, mkdir, open, readFile, rm } from 'node:fs/promises' import { join } from 'node:path' import { defineTool } from './tool-schema.js' import { output } from './tools.js' @@ -237,6 +237,22 @@ export function install(rt) { }) } + // P-205: the archive folder is touched before any card exists. Claiming the temp file proves the + // folder takes a write, so a decide that cannot archive its file fails with no card created, + // instead of leaving cards behind for the retry to double. The handle is filled once the ids exist. + const claimTemp = async (dir, name) => { + const temp = `${join(dir, name)}.tmp-${randomBytes(4).toString('hex')}` + return { temp, handle: await open(temp, 'w') } + } + const dropTemp = async ({ temp, handle }) => { + try { + await handle.close() + } catch { + // closed already: the write path closes before it links + } + await rm(temp, { force: true }) + } + const decide = async (meeting, args, { now, actor }) => { if (!OPEN_STATES.has(meeting.status)) throw refuse('closed', `the meeting is ${meeting.status}`) if (actor?.role === ASSISTANT_ROLE) throw refuse('forbidden', 'a Project Assistant takes a position; it does not close the meeting') @@ -275,8 +291,27 @@ export function install(rt) { closedBy: actor?.name ?? candidate.owner, decisionFile: file, } - // The cards come first: the decision file appears only once every produced task exists. - closed.taskIds = await produceTasks(planned, { actor }) + // P-205: the step that fails most often — writing into `decisions/` — now runs before any card, + // so an unwritable folder leaves the meeting exactly as it was. The temp is claimed here. + let slot = await claimTemp(dir, file) + // P-205: the cards come first, but a retry must not double them. If an earlier attempt died + // between the cards and the finished file, its ids are on the meeting record and this attempt + // keeps that card instead of creating a second one. A re-opened meeting starts from no ids. + const pending = Array.isArray(meeting.pendingTaskIds) ? meeting.pendingTaskIds : [] + try { + if (pending.length > 0) { + closed.taskIds = pending.filter(id => rt.state.tasks?.[id] !== undefined) + } else { + closed.taskIds = await produceTasks(planned, { actor }) + // On the record before the file is written, while the meeting is still `deliberating`: a + // failure below (a full disk, a `link` the loop cannot retry) must not double the cards. + store()[meeting.id] = { ...meeting, pendingTaskIds: closed.taskIds } + } + } catch (error) { + await dropTemp(slot) + throw error + } + delete closed.pendingTaskIds // One render, and the bytes hashed are the bytes archived. `link` claims the name without // rewriting an existing file; a name lost to a concurrent writer takes the next one, and the @@ -284,21 +319,26 @@ export function install(rt) { let bytes let name = file for (let attempt = 0; ; attempt += 1) { - if (attempt === 8) throw refuse('exists', `no free decision file name left for ${file}`) + if (attempt === 8) { + await dropTemp(slot) + throw refuse('exists', `no free decision file name left for ${file}`) + } closed.decisionFile = name bytes = Buffer.from(decisionBody(closed, name, now), 'utf8') - const temp = `${join(dir, name)}.tmp-${randomBytes(4).toString('hex')}` - await writeFile(temp, bytes) try { - await link(temp, join(dir, name)) + // The temp claimed before the cards is filled here: its bytes are the bytes archived. + await slot.handle.writeFile(bytes) + await slot.handle.close() + await link(slot.temp, join(dir, name)) } catch (error) { - await rm(temp, { force: true }) + await dropTemp(slot) if (error?.code !== 'EEXIST') throw error name = await nextFreeFile(dir, now, candidate.slug) await repointTasks(closed.taskIds, name, { now, actor }) + slot = await claimTemp(dir, name) continue } - await rm(temp, { force: true }) + await rm(slot.temp, { force: true }) break } closed.decisionSha256 = createHash('sha256').update(bytes).digest('hex') diff --git a/patches/ds-bot/files/test/meetings-decide-paths.test.js b/patches/ds-bot/files/test/meetings-decide-paths.test.js index 360342f35..0bac58a3b 100644 --- a/patches/ds-bot/files/test/meetings-decide-paths.test.js +++ b/patches/ds-bot/files/test/meetings-decide-paths.test.js @@ -6,7 +6,7 @@ // - P-192: `blocked` had no way back; the meeting stayed blocked for good. import assert from 'node:assert/strict' import { createHash } from 'node:crypto' -import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' @@ -272,3 +272,97 @@ test('meetings: a host without the create gate refuses the produced task instead assert.deepEqual(filesIn(dir), [], 'no decision file is archived') assert.equal(rt.state.meetings[id].status, 'deliberating', 'the meeting did not close') }) + +// P-205: the archive step that fails leaves nothing behind. The `decisions` folder is writable by +// name but not by content here, and the decide must fail before a card exists — the base created +// the card first, so every retry added one more. +test('meetings: a decide that cannot archive its file creates no card, and its retry creates none either', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + const folder = join(dir, 'decisions') + t.after(async () => { + chmodSync(folder, 0o755) + await stop() + }) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + mkdirSync(folder) + chmodSync(folder, 0o555) + + for (const attempt of [1, 2]) { + const refusal = await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }) + .then(() => undefined, error => error) + assert.equal(refusal?.code, 'EACCES', `attempt ${attempt} fails on the folder itself`) + assert.equal(Object.keys(rt.state.tasks ?? {}).length, 0, `attempt ${attempt} creates no card`) + assert.deepEqual(tempsIn(dir), [], `attempt ${attempt} leaves no temp file behind`) + assert.deepEqual(filesIn(dir), [], `attempt ${attempt} archives no decision file`) + assert.equal(rt.state.meetings[id].status, 'deliberating', `attempt ${attempt} leaves the meeting open`) + } +}) + +// P-205: once the folder takes the write again the same decide lands on the name the failures +// would have used, and the ids its file lists are exactly the cards on the board. +test('meetings: a retry once the folder is writable archives the file and lists every card', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + const folder = join(dir, 'decisions') + t.after(async () => { + chmodSync(folder, 0o755) + await stop() + }) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + mkdirSync(folder) + chmodSync(folder, 0o555) + await assert.rejects(decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }), /EACCES: permission denied/) + chmodSync(folder, 0o755) + + await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }, { botId: 'r1', title: 'Review the record' }] }) + const record = rt.state.meetings[id] + const body = readFileSync(join(dir, record.decisionFile), 'utf8') + assert.match(record.decisionFile, /-which-storage-shape-should-stage-4-use\.md$/, 'the retry keeps the name the failures left free') + assert.deepEqual(filesIn(dir), [record.decisionFile.replace('decisions/', '')], 'exactly one file, under that name') + assert.equal(record.status, 'decided') + assert.equal(record.taskIds.length, 2, 'both produced tasks are on the record') + assert.equal(Object.keys(rt.state.tasks).length, 2, 'and both are on the board') + assert.equal(record.taskIds.length, body.split('\n').filter(line => line.startsWith('- Tasks: '))[0].slice('- Tasks: '.length).split(', ').length, 'the file lists as many ids as the record keeps') + assert.ok(body.includes(`- Tasks: ${record.taskIds.join(', ')}`), 'the file lists the kept ids in order') + assert.equal(record.decisionSha256, sha256(body), 'the recorded sha covers the archived bytes') + assert.equal(record.pendingTaskIds, undefined, 'a decided meeting carries no retry bookkeeping') + assert.deepEqual(tempsIn(dir), [], 'no temp file survives the success') +}) + +// P-205, the window no filesystem lever opens in-process: a decide that died between the cards and +// the finished file left its ids on the meeting record. The retry must keep those cards rather than +// create a second one for the same produced task — and the file it writes lists the kept card. +test('meetings: a retry after the cards exist keeps them instead of creating another', async (t) => { + const { rt, dir, open, decide, closeAll, stop } = await boot() + t.after(stop) + const id = await open('Which storage shape should Stage 4 use?') + await closeAll(id) + // The card an earlier attempt created, and the record it left behind: ids on the meeting, the + // meeting still `deliberating`, no file. + const card = await rt.taskEndpoint('task-create', { botId: 'd1', title: 'Write the record' }, { name: 'Chief', role: 'Chief' }) + rt.state.meetings[id].pendingTaskIds = [card.id] + + // Two more attempts while the folder still refuses the write: neither adds a card, and neither + // leaves a file or a temp behind. + const folder = join(dir, 'decisions') + mkdirSync(folder) + chmodSync(folder, 0o555) + t.after(() => { try { chmodSync(folder, 0o755) } catch { /* the tree is gone with the test */ } }) + for (const attempt of [1, 2]) { + await assert.rejects(decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }), /EACCES: permission denied/, `attempt ${attempt} still fails on the folder`) + assert.deepEqual(Object.keys(rt.state.tasks), [card.id], `attempt ${attempt} adds no second card`) + assert.deepEqual(filesIn(dir), [], `attempt ${attempt} archives nothing`) + assert.deepEqual(tempsIn(dir), [], `attempt ${attempt} leaves no temp behind`) + } + chmodSync(folder, 0o755) + + await decide(id, { tasks: [{ botId: 'd1', title: 'Write the record' }] }) + const record = rt.state.meetings[id] + const body = readFileSync(join(dir, record.decisionFile), 'utf8') + assert.deepEqual(Object.keys(rt.state.tasks), [card.id], 'the retry built no second card') + assert.deepEqual(record.taskIds, [card.id], 'the record keeps the card the first attempt made') + assert.ok(body.includes(`- Tasks: ${card.id}`), 'the file names that card') + assert.equal(record.pendingTaskIds, undefined, 'the bookkeeping is gone once the file lands') + assert.deepEqual(filesIn(dir), [record.decisionFile.replace('decisions/', '')], 'one file, no second temp') +})