diff --git a/changelog.d/2134-journal-status.added.md b/changelog.d/2134-journal-status.added.md new file mode 100644 index 000000000..cbaffe5f0 --- /dev/null +++ b/changelog.d/2134-journal-status.added.md @@ -0,0 +1 @@ +With `stateJournal` on, the diagnostics report now says whether the state journal is recording or why it stopped, and `journal.status()` answers the same question to a host ([#2134](https://github.com/ThinkFlowLab/FlyBot/issues/2134)). diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index 400c800b5..bc9379b81 100644 --- a/core/src/host/diagnostics.js +++ b/core/src/host/diagnostics.js @@ -81,7 +81,10 @@ export function reportMarkdown({ facts, client = {}, entries = [], clientEntries row('Read-only', facts.readOnly), ].filter(Boolean) const recent = entries.slice(-REPORT_ENTRIES) - const out = [...lines, ''] + // The journal is only mentioned while the host runs one: an off journal is not a fact about + // this host, and a stopped one is exactly what a report about a bug it was meant to catch misses. + const journal = facts.stateJournal === undefined ? [] : ['', `state journal: ${facts.stateJournal}`] + const out = [...lines, ...journal, ''] out.push(recent.length === 0 ? 'No host errors recorded.' : `Recent host errors (${recent.length} of ${entries.length}, oldest first):`) if (recent.length > 0) out.push('```', ...recent.map(entryLine), '```') const stacks = recent.filter(entry => entry.stack).slice(-3) @@ -221,6 +224,7 @@ 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() return { botVersion: ownVersion(), dshVersion: dshVersion(), @@ -229,6 +233,7 @@ export function install(rt) { 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)})`, } } diff --git a/core/src/host/journal.js b/core/src/host/journal.js index 128c2bdf4..d17cec18f 100644 --- a/core/src/host/journal.js +++ b/core/src/host/journal.js @@ -228,6 +228,8 @@ export function pruneEntries(entries, { now, maxBytes = JOURNAL_MAX_BYTES, maxAg // and leaves the file as it is; it never stops the team from saving. export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX_BYTES, maxAgeMs = JOURNAL_MAX_AGE_MS }) { let stopped = false + // Why recording stopped, in the words `stop()` already had; `status()` hands it on. + let because let lastText let seq = 0 let bytes = 0 @@ -235,8 +237,10 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX let queue = Promise.resolve() const stop = (error) => { + if (stopped) return stopped = true - warn('ds-bot: state journal %s is not recording: %s', path, error) + because = `${path} is not recording: ${error}` + warn('ds-bot: state journal %s', because) } const open = async () => { @@ -322,5 +326,7 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX queue = queue.then(() => write(snapshot, actor)).catch(stop) return queue } - return { record, path } + // Whether this writer still records, and why not — the host can ask instead of reading stderr. + const status = () => (stopped ? { recording: false, because } : { recording: true }) + return { record, path, status } } diff --git a/core/src/host/store.js b/core/src/host/store.js index e53ba802b..fcf0533bd 100644 --- a/core/src/host/store.js +++ b/core/src/host/store.js @@ -1070,7 +1070,7 @@ export function install(rt) { const isAdminOf = (room, id) => id !== undefined && room.admin === id Object.assign(rt, { - state, load, save, refresh, readOnly, mutate, savesQueued, savesSettled, + state, load, save, refresh, readOnly, mutate, savesQueued, savesSettled, journal, botOf, roomOf, isMain, chatOf, selfOf, isBotSession, chatOwners, groupOwners, pastOwners, groupSessionsOf, dropGroupSession, findBot, findRooms, isAdminOf, diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index 00685b834..9a5f8b7cb 100644 --- a/core/test/diagnostics.test.js +++ b/core/test/diagnostics.test.js @@ -6,6 +6,7 @@ import { homedir, tmpdir } from 'node:os' import { join } from 'node:path' import { test } from 'node:test' import { ISSUES_URL, cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' +import { openJournal, stateText } from '../src/host/journal.js' import { failureLine, install as installTurns, turnFailure } from '../src/host/turns.js' import { errorCode } from '../src/host/channel.js' import { SOURCE_KIND } from '../src/host/constants.js' @@ -273,6 +274,32 @@ test('diagnostics append fallback redacts an error message', async (t) => { assert.match(stderr.join('\n'), /api_key=\[hidden\]/) }) +// #2134: the report is where a user looks after a bug the journal was turned on for, so it says +// whether the journal is still recording or why it stopped, and says nothing while it is off. +test('the diagnostics report says whether the state journal is recording or why it stopped (#2134)', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) + t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) + const off = await host(home, { config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.equal(off.facts.stateJournal, undefined, 'a host without the journal says nothing about it') + assert.doesNotMatch(off.markdown, /state journal:/) + + const target = join(home, 'someone-else.txt') + writeFileSync(target, 'PRIVATE LINE\n') + mkdirSync(join(home, 'journal'), { recursive: true }) + const path = join(home, 'journal', 'state.jsonl') + symlinkSync(target, path) + const journal = openJournal({ path, warn: () => {} }) + await journal.record(stateText({ version: 2, revision: 1 })) + const stopped = await host(home, { journal, config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.match(stopped.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) + assert.equal(stopped.facts.stateJournal.startsWith('stopped ('), true) + + const recording = openJournal({ path: join(home, 'live', 'state.jsonl'), warn: () => {} }) + const live = await host(home, { journal: recording, config: { diagnosticsFile: false } }).diagnostics.report({}) + assert.match(live.markdown, /^state journal: recording$/m) + assert.equal(live.facts.stateJournal, 'recording') +}) + 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/core/test/state-journal.test.js b/core/test/state-journal.test.js index d527a87a3..3b03b03e1 100644 --- a/core/test/state-journal.test.js +++ b/core/test/state-journal.test.js @@ -472,3 +472,31 @@ test('a journal that is still its own folder and file keeps recording (#2074 con if (posix) assert.equal(statSync(path).mode & 0o777, 0o600) rmSync(home, { recursive: true, force: true }) }) + +// #2134: the journal can stop for good — a link where its file belongs, a folder the host does not +// own, a write that failed — and until now only a stderr warning said so. `status()` answers the +// question a host can ask, with the reason `stop()` already received and the journal path in it. +test('the journal says whether it is still recording, and why it stopped, path included (#2134)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.txt') + writeFileSync(target, 'PRIVATE LINE\n') + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + symlinkSync(target, path) + const stopped = openJournal({ path, warn: () => {} }) + await stopped.record(stateText({ version: 2, revision: 1 })) + const answer = stopped.status() + assert.equal(answer.recording, false) + assert.ok(answer.because.includes(path), `the reason names the journal: ${answer.because}`) + assert.match(answer.because, /not a plain file/) + assert.equal(readFileSync(target, 'utf8'), 'PRIVATE LINE\n', 'nothing is written through the link') + + // The control: a journal that is still its own file answers recording, before and after a write. + const own = openJournal({ path: join(home, 'own', 'state.jsonl'), warn: () => {} }) + assert.deepEqual(own.status(), { recording: true }) + await own.record(stateText({ version: 2, revision: 1 })) + assert.deepEqual(own.status(), { recording: true }) + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index c479c2fae..26026afb2 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -556,7 +556,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 for that run with a warning and leaves the file as it is. 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 both the start-up read and the append 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; the prune path reads the current file and replaces the link with a fresh file in the verified folder, so nothing is written through it. +- **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 for that run with a warning and leaves the file as it is. 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 both the start-up read and the append 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; the prune path reads the current file and replaces the link with a fresh file in the verified folder, so nothing is written through it. 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)). - **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 ab106edb2..161e5ca46 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -4783,7 +4783,15 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js row('DSH', facts.dshVersion), row('Surface', client.surface), row('OS', facts.os), -@@ -82,7 +82,7 @@ export function reportMarkdown({ facts, client = {}, entries = [], clientEntries +@@ -78,11 +78,14 @@ export function reportMarkdown({ facts, client = {}, entries = [], clientEntries + row('Read-only', facts.readOnly), + ].filter(Boolean) + const recent = entries.slice(-REPORT_ENTRIES) +- const out = [...lines, ''] ++ // The journal is only mentioned while the host runs one: an off journal is not a fact about ++ // this host, and a stopped one is exactly what a report about a bug it was meant to catch misses. ++ const journal = facts.stateJournal === undefined ? [] : ['', `state journal: ${facts.stateJournal}`] ++ const out = [...lines, ...journal, ''] out.push(recent.length === 0 ? 'No host errors recorded.' : `Recent host errors (${recent.length} of ${entries.length}, oldest first):`) if (recent.length > 0) out.push('```', ...recent.map(entryLine), '```') const stacks = recent.filter(entry => entry.stack).slice(-3) @@ -4856,7 +4864,20 @@ 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', -@@ -204,7 +204,7 @@ export function install(rt) { +@@ -192,6 +192,7 @@ 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() + return { + botVersion: ownVersion(), + dshVersion: dshVersion(), +@@ -200,11 +200,12 @@ export function install(rt) { + 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)})`, + } } const sanitizeClient = (client) => { diff --git a/patches/ds-bot/0002-flybot-api-host.patch b/patches/ds-bot/0002-flybot-api-host.patch index f1488739b..239908e1c 100644 --- a/patches/ds-bot/0002-flybot-api-host.patch +++ b/patches/ds-bot/0002-flybot-api-host.patch @@ -1695,7 +1695,7 @@ diff --git a/src/host/store.js b/src/host/store.js Object.assign(rt, { - state, load, save, refresh, readOnly, -+ state, load, save, refresh, readOnly, mutate, savesQueued, savesSettled, ++ state, load, save, refresh, readOnly, mutate, savesQueued, savesSettled, journal, botOf, roomOf, isMain, chatOf, selfOf, isBotSession, chatOwners, groupOwners, pastOwners, groupSessionsOf, dropGroupSession, findBot, findRooms, isAdminOf, diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index 8f60ca274..e93a8d87f 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -795,7 +795,7 @@ diff --git a/scripts/lint.mjs b/scripts/lint.mjs diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js --- a/test/diagnostics.test.js +++ b/test/diagnostics.test.js -@@ -1,11 +1,11 @@ +@@ -1,11 +1,12 @@ import assert from 'node:assert/strict' -import { mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' @@ -806,6 +806,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js import { test } from 'node:test' -import { cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' +import { ISSUES_URL, cleanText, install as installDiagnostics, reportMarkdown } from '../src/host/diagnostics.js' ++import { openJournal, stateText } from '../src/host/journal.js' import { failureLine, install as installTurns, turnFailure } from '../src/host/turns.js' import { errorCode } from '../src/host/channel.js' import { SOURCE_KIND } from '../src/host/constants.js' @@ -1002,6 +1003,39 @@ 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) => { + assert.match(stderr.join('\n'), /api_key=\[hidden\]/) + }) + ++// #2134: the report is where a user looks after a bug the journal was turned on for, so it says ++// whether the journal is still recording or why it stopped, and says nothing while it is off. ++test('the diagnostics report says whether the state journal is recording or why it stopped (#2134)', async (t) => { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) ++ t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) ++ const off = await host(home, { config: { diagnosticsFile: false } }).diagnostics.report({}) ++ assert.equal(off.facts.stateJournal, undefined, 'a host without the journal says nothing about it') ++ assert.doesNotMatch(off.markdown, /state journal:/) ++ ++ const target = join(home, 'someone-else.txt') ++ writeFileSync(target, 'PRIVATE LINE\n') ++ mkdirSync(join(home, 'journal'), { recursive: true }) ++ const path = join(home, 'journal', 'state.jsonl') ++ symlinkSync(target, path) ++ const journal = openJournal({ path, warn: () => {} }) ++ await journal.record(stateText({ version: 2, revision: 1 })) ++ const stopped = await host(home, { journal, config: { diagnosticsFile: false } }).diagnostics.report({}) ++ assert.match(stopped.markdown, /^state journal: stopped \(.*not a plain file.*\)$/m) ++ assert.equal(stopped.facts.stateJournal.startsWith('stopped ('), true) ++ ++ const recording = openJournal({ path: join(home, 'live', 'state.jsonl'), warn: () => {} }) ++ const live = await host(home, { journal: recording, config: { diagnosticsFile: false } }).diagnostics.report({}) ++ assert.match(live.markdown, /^state journal: recording$/m) ++ assert.equal(live.facts.stateJournal, 'recording') ++}) ++ + test('reportMarkdown says when nothing went wrong', () => { + const text = reportMarkdown({ facts: { botVersion: '0.1.0', os: 'linux' } }) + assert.match(text, /No host errors recorded\./) @@ -155,6 +155,29 @@ test('turnFailure drops AUTH messages and keeps the facts', () => { assert.equal(failureLine({ code: 'SERVER', status: 503, message: 'busy' }), 'SERVER 503: busy') }) diff --git a/patches/ds-bot/files/src/host/journal.js b/patches/ds-bot/files/src/host/journal.js index 128c2bdf4..d17cec18f 100644 --- a/patches/ds-bot/files/src/host/journal.js +++ b/patches/ds-bot/files/src/host/journal.js @@ -228,6 +228,8 @@ export function pruneEntries(entries, { now, maxBytes = JOURNAL_MAX_BYTES, maxAg // and leaves the file as it is; it never stops the team from saving. export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX_BYTES, maxAgeMs = JOURNAL_MAX_AGE_MS }) { let stopped = false + // Why recording stopped, in the words `stop()` already had; `status()` hands it on. + let because let lastText let seq = 0 let bytes = 0 @@ -235,8 +237,10 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX let queue = Promise.resolve() const stop = (error) => { + if (stopped) return stopped = true - warn('ds-bot: state journal %s is not recording: %s', path, error) + because = `${path} is not recording: ${error}` + warn('ds-bot: state journal %s', because) } const open = async () => { @@ -322,5 +326,7 @@ export function openJournal({ path, warn, now = Date.now, maxBytes = JOURNAL_MAX queue = queue.then(() => write(snapshot, actor)).catch(stop) return queue } - return { record, path } + // Whether this writer still records, and why not — the host can ask instead of reading stderr. + const status = () => (stopped ? { recording: false, because } : { recording: true }) + return { record, path, status } } diff --git a/patches/ds-bot/files/test/state-journal.test.js b/patches/ds-bot/files/test/state-journal.test.js index d527a87a3..3b03b03e1 100644 --- a/patches/ds-bot/files/test/state-journal.test.js +++ b/patches/ds-bot/files/test/state-journal.test.js @@ -472,3 +472,31 @@ test('a journal that is still its own folder and file keeps recording (#2074 con if (posix) assert.equal(statSync(path).mode & 0o777, 0o600) rmSync(home, { recursive: true, force: true }) }) + +// #2134: the journal can stop for good — a link where its file belongs, a folder the host does not +// own, a write that failed — and until now only a stderr warning said so. `status()` answers the +// question a host can ask, with the reason `stop()` already received and the journal path in it. +test('the journal says whether it is still recording, and why it stopped, path included (#2134)', async () => { + const home = tempHome() + const outside = mkdtempSync(join(tmpdir(), 'ds-bot-journal-outside-')) + const target = join(outside, 'someone-else.txt') + writeFileSync(target, 'PRIVATE LINE\n') + mkdirSync(join(home, 'journal')) + const path = join(home, 'journal', 'state.jsonl') + symlinkSync(target, path) + const stopped = openJournal({ path, warn: () => {} }) + await stopped.record(stateText({ version: 2, revision: 1 })) + const answer = stopped.status() + assert.equal(answer.recording, false) + assert.ok(answer.because.includes(path), `the reason names the journal: ${answer.because}`) + assert.match(answer.because, /not a plain file/) + assert.equal(readFileSync(target, 'utf8'), 'PRIVATE LINE\n', 'nothing is written through the link') + + // The control: a journal that is still its own file answers recording, before and after a write. + const own = openJournal({ path: join(home, 'own', 'state.jsonl'), warn: () => {} }) + assert.deepEqual(own.status(), { recording: true }) + await own.record(stateText({ version: 2, revision: 1 })) + assert.deepEqual(own.status(), { recording: true }) + rmSync(home, { recursive: true, force: true }) + rmSync(outside, { recursive: true, force: true }) +})