fix(host): a meeting archives its decision before it creates cards

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)).

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,30 +291,54 @@ 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
     // cards follow it so their evidence pointer and the file cannot disagree.
     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')

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

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`.

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,30 +291,54 @@ 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
     // cards follow it so their evidence pointer and the file cannot disagree.
     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')