diff --git a/changelog.d/2351-journal-lazy-open.fixed.md b/changelog.d/2351-journal-lazy-open.fixed.md new file mode 100644 index 000000000..d6822e9a0 --- /dev/null +++ b/changelog.d/2351-journal-lazy-open.fixed.md @@ -0,0 +1 @@ +A window that opens the host state read-only no longer creates or rewrites the state journal: opening the writer is itself a write, so it now happens on the first save that got past the write lock instead of at host start, while `rt.journal.status()` and the diagnostics line answer as before on a host that may write ([#2351](https://github.com/ThinkFlowLab/FlyBot/issues/2351)). diff --git a/core/src/host/store.js b/core/src/host/store.js index fcf0533bd..3083d875a 100644 --- a/core/src/host/store.js +++ b/core/src/host/store.js @@ -45,7 +45,19 @@ export function install(rt) { let queued = 0 const { warn } = rt // Opt-in (#511): every saved change is also recorded for replay; off by default. - const journal = config.stateJournal === true ? openJournal({ path: join(home, 'journal', 'state.jsonl'), warn }) : undefined + // Opening the writer is itself a write — it creates `journal/` (0700) and repairs a half-written + // last line by replacing the file with its prefix — so it waits for the first save that got past + // `writable()`. A window another dsh keeps read-only never gets there, and used to leave the + // folder behind and rewrite the holder's journal (#2351). + const journalPath = join(home, 'journal', 'state.jsonl') + let journalWriter + const journal = config.stateJournal === true + ? { + path: journalPath, + record: (snapshot) => (journalWriter ??= openJournal({ path: journalPath, warn })).record(snapshot), + status: () => journalWriter?.status() ?? { recording: true }, + } + : undefined const EMPTY = JSON.stringify(state) // The other dsh host that holds state.lock while this one only reads (see home-lock.js); diff --git a/core/test/store.test.js b/core/test/store.test.js index 1d3360ef0..5ab052dbc 100644 --- a/core/test/store.test.js +++ b/core/test/store.test.js @@ -27,8 +27,8 @@ function patchSync(method, replace) { } } -function runtime(t, saved) { - const home = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) +function runtime(t, saved, { home: at, config } = {}) { + const home = at ?? fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) const statePath = join(home, 'state.json') if (saved !== undefined) fs.writeFileSync(statePath, typeof saved === 'string' ? saved : JSON.stringify(saved)) const warnings = [] @@ -45,6 +45,7 @@ function runtime(t, saved) { ctx, home, statePath, + config, autoRoomName: () => 'Room', retitleRoom: async () => {}, warn: (...args) => warnings.push(args), @@ -247,6 +248,44 @@ test('a read-only host resolves awaited saves without writing', async (t) => { assert.equal(fs.readFileSync(statePath, 'utf8'), ownerState) }) +// Opening the state journal is itself a write: it creates `journal/` (0700) and repairs a +// half-written last line by replacing the file with its complete prefix. A window another dsh +// keeps read-only never gets to record, so opening it there leaves the folder behind and rewrites +// the holder's own journal — a window that may not change the team changed the team's home. +test('a read-only window neither creates nor rewrites the state journal', async (t) => { + // Someone else's home: this window may not write, so no `journal/` may appear at all. + const held = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) + t.after(() => fs.rmSync(held, { recursive: true, force: true, maxRetries: 5 })) + fs.writeFileSync(join(held, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' })) + const reader = runtime(t, undefined, { home: held, config: { stateJournal: true } }) + assert.equal(fs.existsSync(join(held, 'journal')), false, 'a window that may not write creates no journal folder') + await reader.rt.load() + assert.match(reader.rt.readOnly(), /another dsh/) + reader.rt.state.prefs.marker = 'not saved' + await reader.rt.save() + assert.equal(fs.existsSync(join(held, 'journal')), false, 'a refused save opens no journal') + + // The holder's journal keeps its bytes, half-written last line included: repairing it is a + // write, and this window may not write. + const shared = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) + t.after(() => fs.rmSync(shared, { recursive: true, force: true, maxRetries: 5 })) + const writer = runtime(t, undefined, { home: shared, config: { stateJournal: true } }) + await writer.rt.load() + await writer.rt.save() + const path = join(shared, 'journal', 'state.jsonl') + fs.appendFileSync(path, '{"seq":2,"t":1,"op":se') + // Another process, so this one is a reader: a same-pid lock file would be a lock it can take. + fs.writeFileSync(join(shared, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' })) + const before = fs.readFileSync(path) + const second = runtime(t, undefined, { home: shared, config: { stateJournal: true } }) + await second.rt.load() + assert.match(second.rt.readOnly(), /another dsh/) + assert.deepEqual(fs.readFileSync(path), before, 'opening a journal this window may not write leaves its bytes alone') + second.rt.state.prefs.marker = 'not saved' + await second.rt.save() + assert.deepEqual(fs.readFileSync(path), before, 'a refused save leaves the journal bytes alone') +}) + test('dispose waits for a pending save and prevents later writes', async (t) => { const { rt, home, statePath, dispose } = runtime(t) await rt.load() diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 257adbaa3..dc989dfe7 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -556,7 +556,7 @@ DSH's catalog carries `lastDelivery`, an inbox receipt (the reminder reached the A local, replayable record of every change to the saved team, for multi-Bot bugs that reproduce only sometimes. Owner decision 2026-10-20 (option a): state mutations only (decisions stay in #396), off by default, 7 days or 20 MB, dev-only replay, local only. - **On.** `stateJournal: true` in the `bot` config. Off (the default) nothing is opened or written, so every other test runs as before. -- **What.** `src/host/journal.js`. After each successful `state.json` write, `store.js` hands the exact snapshot text to the journal, which appends the difference from the last recorded snapshot to `/journal/state.jsonl` (file mode 0600, folder 0700). Lines are `{ seq, t, op, key, prev, next, botId?, actor }`; `op` is `base` (the whole team: the first line, and any change only a key reorder explains), `set`, `delete`, or `append` (`prevLength`, `drop`, the new items: a group log or other list that grew or slid). Paths go at most three keys deep (`bots..`); `botId` is set on a `bots.` path; `actor` is `host` for a save and `retention` for a folded base (per-call attribution is not recorded). Only the host that holds the state lock writes, so a read-only window records nothing. +- **What.** `src/host/journal.js`. After each successful `state.json` write, `store.js` hands the exact snapshot text to the journal, which appends the difference from the last recorded snapshot to `/journal/state.jsonl` (file mode 0600, folder 0700). Lines are `{ seq, t, op, key, prev, next, botId?, actor }`; `op` is `base` (the whole team: the first line, and any change only a key reorder explains), `set`, `delete`, or `append` (`prevLength`, `drop`, the new items: a group log or other list that grew or slid). Paths go at most three keys deep (`bots..`); `botId` is set on a `bots.` path; `actor` is `host` for a save and `retention` for a folded base (per-call attribution is not recorded). Only the host that holds the state lock writes, so a read-only window records nothing. Opening the writer is itself a write — it creates `journal/` (mode 0700) and repairs a half-written last line by replacing the file with its prefix — so `store.js` opens it on the first save that got past `writable()`, not when the host starts: a window another dsh keeps read-only creates no folder and leaves the holder's lines as they are, and the report still says `paused (this window is read-only)`. - **Retention.** When a write would pass 20 MB, or the oldest change is older than 7 days, the oldest lines are dropped down to 90% of each cap and folded into the base line (atomic rewrite), so the kept lines still replay. A team that alone exceeds the cap stops recording. - **Fail closed.** Replay (`replayTo(text, seq)`) refuses the whole journal on an unreadable or incomplete line, an unknown op, a gap in `seq`, a first line that is not a base, or a `prev` that does not match what the earlier lines built; a `seq` outside the file is refused, not approximated. On start the writer replays the existing file the same way; a journal that does not replay, a path that is not a plain file, or a failed write stops recording with a warning and leaves the file as it is. A last line left half written by a crash is the one shape the writer repairs instead of refusing: when the complete prefix before it replays, the file is replaced with that prefix (the same atomic temp-and-rename as a rotation) with one warning, `dropped an incomplete last line left by a crash`, and recording continues from it ([#2166](https://github.com/ThinkFlowLab/FlyBot/issues/2166)) — a broken line above the last one, a `seq` gap, a `prev` mismatch or an unknown op still stops recording and leaves the file as it is. A writer that stopped is asked once more on the next save: it runs the start-up `open()` again, so a folder or file repaired meanwhile resumes recording — appending the change that save carried, or starting a new file with a base when the journal was removed — while a journal that still refuses stays stopped and that save still lands; the asking is silent, so a refused start-up or write is warned once, not once per save ([#2154](https://github.com/ThinkFlowLab/FlyBot/issues/2154)). Saving the team never depends on the journal. The folder above the journal must be a real directory too (#2056): a link, a plain file or a check that cannot run refuses before the `mkdir`/`chmod` that would follow it, so neither the permissions nor a write reach the folder a link names; a folder that does not exist yet is still this writer's to create, as before. Since [#2074](https://github.com/ThinkFlowLab/FlyBot/issues/2074) the same question is asked again when the write runs: a journal folder swapped for a link is refused with one warning and recording stops, and a `state.jsonl` swapped for a link is never written through. A hard link reads as a plain file to `lstat` while its bytes are also a name outside this home, so the start-up read, the append and — since [#2146](https://github.com/ThinkFlowLab/FlyBot/issues/2146) — the rotation also require the file to carry only this one name (`nlink === 1`): a shared inode is refused exactly like a link — one warning, nothing read, chmod-ed or appended — and a journal with one name records as before. The append path refuses the write, and so does the rotation, before it reads: a link or a second name there stops recording with one warning, so another file's lines are never read as the team's history and the `rename` never drops the other name. A writer that stopped says so off the host too: `rt.journal.status()` answers `{ recording, because }`, and the diagnostics report adds one line — `state journal: recording`, `state journal: stopped ()`, or `state journal: paused (this window is read-only)` in a window another dsh keeps read-only, which never reaches the write that records ([#2153](https://github.com/ThinkFlowLab/FlyBot/issues/2153)) — while a host without the journal adds none ([#2134](https://github.com/ThinkFlowLab/FlyBot/issues/2134)). - **Replay.** `core/scripts/replay.mjs` (dev-only, not in the package, see [install](install.md)) prints or writes (`--out`, new file only, 0600) the team after a given `--to `; it makes no network or process calls. Test: `core/test/state-journal.test.js` (a 3-Bot round replays byte-identical at every step). diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index 239908e1c..b1ba63fc0 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -784,7 +784,7 @@ diff --git a/src/host/roster.js b/src/host/roster.js diff --git a/src/host/store.js b/src/host/store.js --- a/src/host/store.js +++ b/src/host/store.js -@@ -1,21 +1,51 @@ +@@ -1,21 +1,63 @@ // Store: the saved team (state.json), its migrations, the lock that keeps a second dsh // host on the same data from writing it, and the indexes that map a Session to the Bot // or group chat it belongs to. @@ -838,7 +838,19 @@ diff --git a/src/host/store.js b/src/host/store.js + let queued = 0 const { warn } = rt + // Opt-in (#511): every saved change is also recorded for replay; off by default. -+ const journal = config.stateJournal === true ? openJournal({ path: join(home, 'journal', 'state.jsonl'), warn }) : undefined ++ // Opening the writer is itself a write — it creates `journal/` (0700) and repairs a half-written ++ // last line by replacing the file with its prefix — so it waits for the first save that got past ++ // `writable()`. A window another dsh keeps read-only never gets there, and used to leave the ++ // folder behind and rewrite the holder's journal (#2351). ++ const journalPath = join(home, 'journal', 'state.jsonl') ++ let journalWriter ++ const journal = config.stateJournal === true ++ ? { ++ path: journalPath, ++ record: (snapshot) => (journalWriter ??= openJournal({ path: journalPath, warn })).record(snapshot), ++ status: () => journalWriter?.status() ?? { recording: true }, ++ } ++ : undefined const EMPTY = JSON.stringify(state) // The other dsh host that holds state.lock while this one only reads (see home-lock.js); diff --git a/patches/ds-bot/0014-flybot-branding.patch b/patches/ds-bot/0014-flybot-branding.patch index f656c4537..6515f104b 100644 --- a/patches/ds-bot/0014-flybot-branding.patch +++ b/patches/ds-bot/0014-flybot-branding.patch @@ -2130,6 +2130,25 @@ diff --git a/test/secrets.test.js b/test/secrets.test.js diff --git a/test/store.test.js b/test/store.test.js --- a/test/store.test.js +++ b/test/store.test.js +@@ -27,8 +27,8 @@ function patchSync(method, replace) { + } + } + +-function runtime(t, saved) { +- const home = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) ++function runtime(t, saved, { home: at, config } = {}) { ++ const home = at ?? fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) + const statePath = join(home, 'state.json') + if (saved !== undefined) fs.writeFileSync(statePath, typeof saved === 'string' ? saved : JSON.stringify(saved)) + const warnings = [] +@@ -45,6 +45,7 @@ function runtime(t, saved) { + ctx, + home, + statePath, ++ config, + autoRoomName: () => 'Room', + retitleRoom: async () => {}, + warn: (...args) => warnings.push(args), @@ -68,7 +68,7 @@ test('a failed state write rejects and leaves the saved file unchanged', async ( const original = fs.readFileSync(statePath) fs.mkdirSync(`${statePath}.${process.pid}.tmp`) @@ -2148,6 +2167,51 @@ diff --git a/test/store.test.js b/test/store.test.js rt.state.prefs.marker = 'not saved' await rt.save() assert.equal(fs.readFileSync(statePath, 'utf8'), original) +@@ -247,6 +247,44 @@ test('a read-only host resolves awaited saves without writing', async (t) => { + assert.equal(fs.readFileSync(statePath, 'utf8'), ownerState) + }) + ++// Opening the state journal is itself a write: it creates `journal/` (0700) and repairs a ++// half-written last line by replacing the file with its complete prefix. A window another dsh ++// keeps read-only never gets to record, so opening it there leaves the folder behind and rewrites ++// the holder's own journal — a window that may not change the team changed the team's home. ++test('a read-only window neither creates nor rewrites the state journal', async (t) => { ++ // Someone else's home: this window may not write, so no `journal/` may appear at all. ++ const held = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) ++ t.after(() => fs.rmSync(held, { recursive: true, force: true, maxRetries: 5 })) ++ fs.writeFileSync(join(held, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' })) ++ const reader = runtime(t, undefined, { home: held, config: { stateJournal: true } }) ++ assert.equal(fs.existsSync(join(held, 'journal')), false, 'a window that may not write creates no journal folder') ++ await reader.rt.load() ++ assert.match(reader.rt.readOnly(), /another dsh/) ++ reader.rt.state.prefs.marker = 'not saved' ++ await reader.rt.save() ++ assert.equal(fs.existsSync(join(held, 'journal')), false, 'a refused save opens no journal') ++ ++ // The holder's journal keeps its bytes, half-written last line included: repairing it is a ++ // write, and this window may not write. ++ const shared = fs.mkdtempSync(join(tmpdir(), 'ds-bot-store-')) ++ t.after(() => fs.rmSync(shared, { recursive: true, force: true, maxRetries: 5 })) ++ const writer = runtime(t, undefined, { home: shared, config: { stateJournal: true } }) ++ await writer.rt.load() ++ await writer.rt.save() ++ const path = join(shared, 'journal', 'state.jsonl') ++ fs.appendFileSync(path, '{"seq":2,"t":1,"op":se') ++ // Another process, so this one is a reader: a same-pid lock file would be a lock it can take. ++ fs.writeFileSync(join(shared, 'state.lock'), JSON.stringify({ pid: process.ppid, host: hostname(), token: 'other' })) ++ const before = fs.readFileSync(path) ++ const second = runtime(t, undefined, { home: shared, config: { stateJournal: true } }) ++ await second.rt.load() ++ assert.match(second.rt.readOnly(), /another dsh/) ++ assert.deepEqual(fs.readFileSync(path), before, 'opening a journal this window may not write leaves its bytes alone') ++ second.rt.state.prefs.marker = 'not saved' ++ await second.rt.save() ++ assert.deepEqual(fs.readFileSync(path), before, 'a refused save leaves the journal bytes alone') ++}) ++ + test('dispose waits for a pending save and prevents later writes', async (t) => { + const { rt, home, statePath, dispose } = runtime(t) + await rt.load() @@ -285,3 +285,94 @@ test('dispose waits for a pending save and prevents later writes', async (t) => restore() }