From 6273704741e1499e67e59e363df2d8427879dd89 Mon Sep 17 00:00:00 2001 From: Day Dreamer <87640074+HugoXK@users.noreply.github.com> Date: Mon, 2 Nov 2026 20:23:20 +0800 Subject: [PATCH] diagnostics: do not claim the journal is recording in a read-only window The state journal writer is opened before the state lock is asked for, and a window another dsh keeps read-only returns from save() before journal.record(), so no line is ever written there. The report said 'state journal: recording' anyway, promising a line the file would not get. It now says the journal is paused in that window, and keeps a genuinely stopped journal's own reason. --- .../2153-journal-readonly-paused.fixed.md | 1 + core/src/host/diagnostics.js | 11 ++++++-- core/test/diagnostics.test.js | 26 +++++++++++++++++ docs/subsystems/core-host.md | 2 +- patches/ds-bot/0001-core-fixes.patch | 17 ++++++++--- patches/ds-bot/0007-version-and-tooling.patch | 28 ++++++++++++++++++- 6 files changed, 77 insertions(+), 8 deletions(-) create mode 100644 changelog.d/2153-journal-readonly-paused.fixed.md diff --git a/changelog.d/2153-journal-readonly-paused.fixed.md b/changelog.d/2153-journal-readonly-paused.fixed.md new file mode 100644 index 000000000..fca840188 --- /dev/null +++ b/changelog.d/2153-journal-readonly-paused.fixed.md @@ -0,0 +1 @@ +A diagnostics report from a window that another dsh keeps read-only no longer says `state journal: recording`: that window never reaches the journal write, so the line now says `state journal: paused (this window is read-only)` instead, while a journal that really stopped keeps its own reason ([#2153](https://github.com/ThinkFlowLab/FlyBot/issues/2153)). diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index bc9379b81..4ffe17586 100644 --- a/core/src/host/diagnostics.js +++ b/core/src/host/diagnostics.js @@ -225,6 +225,7 @@ export function install(rt) { const count = (n, one, many) => `${n} ${n === 1 ? one : many}` const groups = Object.keys(rt.state.rooms).length const journal = rt.journal?.status() + const readOnly = rt.readOnly?.() return { botVersion: ownVersion(), dshVersion: dshVersion(), @@ -232,8 +233,14 @@ export function install(rt) { node: process.version, team: `${count(bots.length, 'Bot', 'Bots')}, ${rt.state.mainBotIds.length} Main, ${count(groups, 'group chat', 'group chats')}`, models: models.length > 0 ? models.join(', ') : undefined, - readOnly: rt.readOnly?.() ? 'yes, another dsh holds the team' : undefined, - stateJournal: journal === undefined ? undefined : journal.recording ? 'recording' : `stopped (${clean(journal.because)})`, + readOnly: readOnly ? 'yes, another dsh holds the team' : undefined, + // The writer is opened before the lock is asked for, so in a window another dsh keeps + // read-only `save()` returns before `journal.record()` and no line is written at all; + // claiming `recording` there promises a line the file will not get. A journal that + // genuinely stopped keeps its own reason, which is the fault a report is usually for. + stateJournal: journal === undefined ? undefined + : journal.recording ? (readOnly ? 'paused (this window is read-only)' : 'recording') + : `stopped (${clean(journal.because)})`, } } diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index 9a5f8b7cb..c1d75bca9 100644 --- a/core/test/diagnostics.test.js +++ b/core/test/diagnostics.test.js @@ -300,6 +300,32 @@ test('the diagnostics report says whether the state journal is recording or why assert.equal(live.facts.stateJournal, 'recording') }) +// The journal is created before the state lock is asked for, and a window another dsh keeps +// read-only never reaches `journal.record()` at all, so saying `recording` would promise a line +// the file will not get. The paused wording is this host's own state, not a journal fault. +test('a read-only window does not say the state journal is recording (#2153)', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) + t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) + const readOnly = () => 'Another dsh (pid 1 on host) holds the team' + const journal = openJournal({ path: join(home, 'journal', 'state.jsonl'), warn: () => {} }) + const held = await host(home, { journal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.match(held.markdown, /^\| Read-only \| yes, another dsh holds the team \|$/m) + assert.doesNotMatch(held.markdown, /state journal: recording/) + assert.match(held.markdown, /^state journal: paused \(this window is read-only\)$/m) + assert.equal(held.facts.stateJournal, 'paused (this window is read-only)') + + // A journal that did stop keeps its own reason: that is the fault a report is usually for. + const target = join(home, 'neighbour.txt') + writeFileSync(target, 'PRIVATE LINE\n') + mkdirSync(join(home, 'journal'), { recursive: true }) + const path = join(home, 'journal', 'state.jsonl') + symlinkSync(target, path) + const stoppedJournal = openJournal({ path, warn: () => {} }) + await stoppedJournal.record(stateText({ version: 2, revision: 1 })) + const stoppedHeld = await host(home, { journal: stoppedJournal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.match(stoppedHeld.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) +}) + test('reportMarkdown says when nothing went wrong', () => { const text = reportMarkdown({ facts: { botVersion: '0.1.0', os: 'linux' } }) assert.match(text, /No host errors recorded\./) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 328bf19e6..0c61d09d3 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -557,7 +557,7 @@ A local, replayable record of every change to the saved team, for multi-Bot bugs - **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. - **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 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` or `state journal: stopped ()` — while a host without the journal adds none ([#2134](https://github.com/ThinkFlowLab/FlyBot/issues/2134)). +- **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 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). ### Standing tool approvals (#540) diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index 0fd193a24..02315ecf4 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -4875,19 +4875,28 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js console.error(`[ds-bot] ${line}${frames.length > 0 ? `\n${clean(frames.join('\n'))}` : ''}`) note({ level: 'warn', -@@ -192,6 +192,7 @@ export function install(rt) { +@@ -192,6 +192,8 @@ export function install(rt) { const models = [...new Set(bots.map(bot => bot.model ?? bot.appliedModel).filter(Boolean))] const count = (n, one, many) => `${n} ${n === 1 ? one : many}` const groups = Object.keys(rt.state.rooms).length + const journal = rt.journal?.status() ++ const readOnly = rt.readOnly?.() return { botVersion: ownVersion(), dshVersion: dshVersion(), -@@ -200,11 +200,12 @@ export function install(rt) { +@@ -199,12 +199,19 @@ export function install(rt) { + node: process.version, team: `${count(bots.length, 'Bot', 'Bots')}, ${rt.state.mainBotIds.length} Main, ${count(groups, 'group chat', 'group chats')}`, models: models.length > 0 ? models.join(', ') : undefined, - readOnly: rt.readOnly?.() ? 'yes, another dsh holds the team' : undefined, -+ stateJournal: journal === undefined ? undefined : journal.recording ? 'recording' : `stopped (${clean(journal.because)})`, +- readOnly: rt.readOnly?.() ? 'yes, another dsh holds the team' : undefined, ++ readOnly: readOnly ? 'yes, another dsh holds the team' : undefined, ++ // The writer is opened before the lock is asked for, so in a window another dsh keeps ++ // read-only `save()` returns before `journal.record()` and no line is written at all; ++ // claiming `recording` there promises a line the file will not get. A journal that ++ // genuinely stopped keeps its own reason, which is the fault a report is usually for. ++ stateJournal: journal === undefined ? undefined ++ : journal.recording ? (readOnly ? 'paused (this window is read-only)' : 'recording') ++ : `stopped (${clean(journal.because)})`, } } diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index e93a8d87f..463312d4c 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -1003,7 +1003,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js test('diagnostics append fallback redacts an error message', async (t) => { const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) -@@ -133,6 +133,32 @@ test('diagnostics append fallback redacts an error message', async (t) => { +@@ -133,6 +133,58 @@ test('diagnostics append fallback redacts an error message', async (t) => { assert.match(stderr.join('\n'), /api_key=\[hidden\]/) }) @@ -1032,6 +1032,32 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + assert.match(live.markdown, /^state journal: recording$/m) + assert.equal(live.facts.stateJournal, 'recording') +}) ++ ++// The journal is created before the state lock is asked for, and a window another dsh keeps ++// read-only never reaches `journal.record()` at all, so saying `recording` would promise a line ++// the file will not get. The paused wording is this host's own state, not a journal fault. ++test('a read-only window does not say the state journal is recording (#2153)', async (t) => { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) ++ t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) ++ const readOnly = () => 'Another dsh (pid 1 on host) holds the team' ++ const journal = openJournal({ path: join(home, 'journal', 'state.jsonl'), warn: () => {} }) ++ const held = await host(home, { journal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) ++ assert.match(held.markdown, /^\| Read-only \| yes, another dsh holds the team \|$/m) ++ assert.doesNotMatch(held.markdown, /state journal: recording/) ++ assert.match(held.markdown, /^state journal: paused \(this window is read-only\)$/m) ++ assert.equal(held.facts.stateJournal, 'paused (this window is read-only)') ++ ++ // A journal that did stop keeps its own reason: that is the fault a report is usually for. ++ const target = join(home, 'neighbour.txt') ++ writeFileSync(target, 'PRIVATE LINE\n') ++ mkdirSync(join(home, 'journal'), { recursive: true }) ++ const path = join(home, 'journal', 'state.jsonl') ++ symlinkSync(target, path) ++ const stoppedJournal = openJournal({ path, warn: () => {} }) ++ await stoppedJournal.record(stateText({ version: 2, revision: 1 })) ++ const stoppedHeld = await host(home, { journal: stoppedJournal, readOnly, config: { diagnosticsFile: false } }).diagnostics.report({}) ++ assert.match(stoppedHeld.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) ++}) + test('reportMarkdown says when nothing went wrong', () => { const text = reportMarkdown({ facts: { botVersion: '0.1.0', os: 'linux' } })