diff --git a/changelog.d/2358-diagnostics-clear-rm.fixed.md b/changelog.d/2358-diagnostics-clear-rm.fixed.md new file mode 100644 index 000000000..1df9cf1f1 --- /dev/null +++ b/changelog.d/2358-diagnostics-clear-rm.fixed.md @@ -0,0 +1 @@ +Settings → Feedback's **Clear** now truncates the log even when the rotated copy cannot be removed: that removal reports itself in one line on stderr instead of stopping the truncate, so "Cleared the error log." matches the file on disk again ([#2358](https://github.com/ThinkFlowLab/FlyBot/issues/2358)). diff --git a/core/src/host/diagnostics.js b/core/src/host/diagnostics.js index 4d4e6faf4..975e74563 100644 --- a/core/src/host/diagnostics.js +++ b/core/src/host/diagnostics.js @@ -291,8 +291,15 @@ export function install(rt) { if (await fileKind(logPath) === 'other') { refuseLeaf(); return } // Rotation moves the full log to `.1`, and the truncate alone left every rotated line // on disk after the user asked for the error log to be cleared. That one name belongs to this - // log, so it goes in the same chain and behind the same guard. - await rm(`${logPath}.1`, { force: true }) + // log, so it goes in the same chain and behind the same guard. The removal keeps a try of its + // own: a copy that cannot be removed (a directory under that name, a folder without its write + // bit) must not reject the chain before the truncate runs, or the window says "cleared" while + // the leaf still holds every line. The line names the leftover, never the folder. + try { + await rm(`${logPath}.1`, { force: true }) + } catch (error) { + console.error('[ds-bot] diagnostics log rotated copy not removed:', error?.code ?? clean(error?.message ?? String(error))) + } await writeFile(logPath, '', 'utf8') }).catch(() => {}) await writing diff --git a/core/test/diagnostics.test.js b/core/test/diagnostics.test.js index 7b905e402..f44c4d821 100644 --- a/core/test/diagnostics.test.js +++ b/core/test/diagnostics.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict' -import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import fsp from 'node:fs/promises' import { syncBuiltinESMExports } from 'node:module' import { homedir, hostname, tmpdir } from 'node:os' @@ -608,3 +608,82 @@ test('clear removes the rotated log, not only the leaf', async (t) => { assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated as before') assert.equal(existsSync(rotated), false, 'the rotated log is gone as well') }) + +// #2352 moved the rotated copy's removal into the clear chain, but outside a try of its own: `rm` on a +// name that is not a plain file of ours (a directory under the same name, a folder that refuses the +// removal) rejects the whole chain before the truncate runs, and the empty `catch` keeps it quiet — so +// the window answered "Cleared the error log." while every byte of the leaf stayed on disk. +test('#2358: clear truncates the leaf even when the rotated copy cannot be removed', async (t) => { + const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) + t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) + mkdirSync(join(home, 'logs'), { recursive: true }) + const logPath = join(home, 'logs', 'diagnostics.jsonl') + const rotated = `${logPath}.1` + writeFileSync(logPath, 'OLD LINE\n'.repeat(4)) + mkdirSync(rotated) // `rm` without `recursive` refuses a directory, so the removal throws EISDIR + + const stderr = [] + const quiet = t.mock.method(console, 'error', (...args) => stderr.push(args.join(' '))) + const rt = host(home) + rt.warn('ds-bot: written before the clear') + await rt.dispose() + assert.match(readFileSync(logPath, 'utf8'), /written before the clear/) + + await rt.diagnostics.clear() + quiet.mock.restore() + + assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated although the rotated copy stays') + assert.equal(lstatSync(rotated).isDirectory(), true, 'the name that could not be removed is still there') + assert.deepEqual(await rt.diagnostics.entries(), [], 'the window reads no entries after a clear') + const noted = stderr.filter(line => /rotated copy/.test(line)) + assert.equal(noted.length, 1, 'one line says the rotated copy was not removed') + assert.equal(noted[0].includes(home), false, 'the line does not print the home folder') + + // The control: with the obstructing name gone the same clear removes both, as the previous test asks. + rmSync(rotated, { recursive: true }) + writeFileSync(rotated, 'ROTATED\n') + writeFileSync(logPath, 'OLD LINE\n') + await rt.diagnostics.clear() + assert.equal(readFileSync(logPath, 'utf8'), '') + assert.equal(existsSync(rotated), false) +}) + +// The same defect through the folder instead of the name: a `logs/` without its write bit refuses the +// removal (EACCES) while the leaf itself is still an ordinary file the host may truncate, so before the +// fix the truncate never ran at all. Root ignores the folder's mode, so the case is skipped there. +test('#2358: clear truncates the leaf when the folder refuses the removal', async (t) => { + if (process.getuid?.() === 0) { + t.skip('root removes from a folder without its write bit') + return + } + const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) + const logs = join(home, 'logs') + t.after(() => { + try { chmodSync(logs, 0o755) } catch { /* the folder may already be gone */ } + rmSync(home, { recursive: true, force: true, maxRetries: 5 }) + }) + mkdirSync(logs, { recursive: true }) + const logPath = join(logs, 'diagnostics.jsonl') + const rotated = `${logPath}.1` + + const stderr = [] + const quiet = t.mock.method(console, 'error', (...args) => stderr.push(args.join(' '))) + const rt = host(home) + rt.warn('ds-bot: written before the clear') + await rt.dispose() + writeFileSync(rotated, 'ROTATED\n'.repeat(8)) + chmodSync(logs, 0o555) + + await rt.diagnostics.clear() + quiet.mock.restore() + + assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated although the folder refused the removal') + assert.equal(readFileSync(rotated, 'utf8'), 'ROTATED\n'.repeat(8), 'the copy that could not be removed keeps its bytes') + const noted = stderr.filter(line => /rotated copy/.test(line)) + assert.equal(noted.length, 1, 'one line says the rotated copy was not removed') + assert.equal(noted[0].includes(home), false, 'the line does not print the home folder') + + chmodSync(logs, 0o755) + await rt.diagnostics.clear() + assert.equal(existsSync(rotated), false, 'once the folder allows it, the same clear removes the copy') +}) diff --git a/docs/subsystems/core-host.md b/docs/subsystems/core-host.md index 31ab91501..340afeeff 100644 --- a/docs/subsystems/core-host.md +++ b/docs/subsystems/core-host.md @@ -406,7 +406,7 @@ Work a member hands a Bot with `message_bot` from its group Session runs in a hi - Host diagnostics never depend on `ctx.logger`: DSH may export no logger, and a test or plugin-local host may carry one without `warn`. `rt.warn` still writes stderr and the diagnostics log in that case (`src/host/diagnostics.js`). Plugins reach the same log through `flybot.warn` (#68); see [plugin-runtime](plugin-runtime.md). - The `Read-only` row of a report repeats this host's own answer rather than one constant: `rt.readOnly()` is `undefined` while the window writes, a sentence about a live `state.lock` when another dsh holds the team, and a sentence about the team file when it is `stuck` (a newer version, a file that will not parse, an unreadable path). The report shows that sentence — `clean(cut(answer, 200))`, so saved keys, the home folder and `|` are handled like any other report text — the same one the sidebar shows, so a newer team file reads `update FlyBot` and an unreadable one reads `could not be read`, never `another dsh holds the team`; a writable window still adds no row ([#2167](https://github.com/ThinkFlowLab/FlyBot/issues/2167)). -- The log itself is one leaf file under the home, and a write asks it before it appends ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `logs/diagnostics.jsonl` is a link, a hard link or anything other than a plain file of its own — the line is dropped with one line on stderr that carries no home folder, the entries stay readable in memory, and `clear` leaves a file behind such a link untouched rather than truncating it. Past 1 MB the leaf moves aside to `diagnostics.jsonl.1`, and `clear` removes that name as well — in the same write chain, behind the same leaf guard — so a cleared log leaves no rotated lines behind. +- The log itself is one leaf file under the home, and a write asks it before it appends ([#2089](https://github.com/ThinkFlowLab/FlyBot/issues/2089)): when `logs/diagnostics.jsonl` is a link, a hard link or anything other than a plain file of its own — the line is dropped with one line on stderr that carries no home folder, the entries stay readable in memory, and `clear` leaves a file behind such a link untouched rather than truncating it. Past 1 MB the leaf moves aside to `diagnostics.jsonl.1`, and `clear` removes that name as well — in the same write chain, behind the same leaf guard — so a cleared log leaves no rotated lines behind. The removal keeps a `try` of its own, and the truncate runs whether or not it succeeded: a copy the host cannot remove (a directory under that name, a folder without its write bit) keeps its bytes, is named on one stderr line that carries no folder, and never turns a Clear that emptied the leaf into one that leaves every line on disk. ### Untrusted fences in every shared room diff --git a/patches/ds-bot/0001-core-fixes.patch b/patches/ds-bot/0001-core-fixes.patch index a1e794e13..0bab9cce4 100644 --- a/patches/ds-bot/0001-core-fixes.patch +++ b/patches/ds-bot/0001-core-fixes.patch @@ -5100,7 +5100,7 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js const list = await entries() return list.map(entry => (entry.stack ? `${entryLine(entry)}\n${entry.stack.replace(/^/gm, ' ')}` : entryLine(entry))).join('\n') } -@@ -241,7 +241,16 @@ export function install(rt) { +@@ -241,7 +241,23 @@ export function install(rt) { cleared = Date.now() fresh = [] if (!toFile) return @@ -5111,8 +5111,15 @@ diff --git a/src/host/diagnostics.js b/src/host/diagnostics.js + if (await fileKind(logPath) === 'other') { refuseLeaf(); return } + // Rotation moves the full log to `.1`, and the truncate alone left every rotated line + // on disk after the user asked for the error log to be cleared. That one name belongs to this -+ // log, so it goes in the same chain and behind the same guard. -+ await rm(`${logPath}.1`, { force: true }) ++ // log, so it goes in the same chain and behind the same guard. The removal keeps a try of its ++ // own: a copy that cannot be removed (a directory under that name, a folder without its write ++ // bit) must not reject the chain before the truncate runs, or the window says "cleared" while ++ // the leaf still holds every line. The line names the leftover, never the folder. ++ try { ++ await rm(`${logPath}.1`, { force: true }) ++ } catch (error) { ++ console.error('[ds-bot] diagnostics log rotated copy not removed:', error?.code ?? clean(error?.message ?? String(error))) ++ } + await writeFile(logPath, '', 'utf8') + }).catch(() => {}) await writing diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index d829fecb8..d08251a50 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -798,7 +798,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js @@ -1,11 +1,13 @@ import assert from 'node:assert/strict' -import { mkdtempSync, readFileSync, rmSync } from 'node:fs' -+import { existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' ++import { chmodSync, existsSync, linkSync, lstatSync, mkdirSync, mkdtempSync, readFileSync, readlinkSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import fsp from 'node:fs/promises' import { syncBuiltinESMExports } from 'node:module' -import { homedir, tmpdir } from 'node:os' @@ -1175,7 +1175,7 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js const short = issueUrl(base, { what: 'Writer failed\nin the group', diagnostics: '| DS Bot | 0.1.0 |' }) assert.equal(short.inline, true) const params = new URL(short.url).searchParams -@@ -253,3 +253,78 @@ test('a browser error is noted once, repeats are counted, and clear empties the +@@ -253,3 +253,157 @@ test('a browser error is noted once, repeats are counted, and clear empties the clearClientErrors() assert.equal(clientErrors().length, 0) }) @@ -1254,3 +1254,82 @@ diff --git a/test/diagnostics.test.js b/test/diagnostics.test.js + assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated as before') + assert.equal(existsSync(rotated), false, 'the rotated log is gone as well') +}) ++ ++// #2352 moved the rotated copy's removal into the clear chain, but outside a try of its own: `rm` on a ++// name that is not a plain file of ours (a directory under the same name, a folder that refuses the ++// removal) rejects the whole chain before the truncate runs, and the empty `catch` keeps it quiet — so ++// the window answered "Cleared the error log." while every byte of the leaf stayed on disk. ++test('#2358: clear truncates the leaf even when the rotated copy cannot be removed', async (t) => { ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) ++ t.after(() => rmSync(home, { recursive: true, force: true, maxRetries: 5 })) ++ mkdirSync(join(home, 'logs'), { recursive: true }) ++ const logPath = join(home, 'logs', 'diagnostics.jsonl') ++ const rotated = `${logPath}.1` ++ writeFileSync(logPath, 'OLD LINE\n'.repeat(4)) ++ mkdirSync(rotated) // `rm` without `recursive` refuses a directory, so the removal throws EISDIR ++ ++ const stderr = [] ++ const quiet = t.mock.method(console, 'error', (...args) => stderr.push(args.join(' '))) ++ const rt = host(home) ++ rt.warn('ds-bot: written before the clear') ++ await rt.dispose() ++ assert.match(readFileSync(logPath, 'utf8'), /written before the clear/) ++ ++ await rt.diagnostics.clear() ++ quiet.mock.restore() ++ ++ assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated although the rotated copy stays') ++ assert.equal(lstatSync(rotated).isDirectory(), true, 'the name that could not be removed is still there') ++ assert.deepEqual(await rt.diagnostics.entries(), [], 'the window reads no entries after a clear') ++ const noted = stderr.filter(line => /rotated copy/.test(line)) ++ assert.equal(noted.length, 1, 'one line says the rotated copy was not removed') ++ assert.equal(noted[0].includes(home), false, 'the line does not print the home folder') ++ ++ // The control: with the obstructing name gone the same clear removes both, as the previous test asks. ++ rmSync(rotated, { recursive: true }) ++ writeFileSync(rotated, 'ROTATED\n') ++ writeFileSync(logPath, 'OLD LINE\n') ++ await rt.diagnostics.clear() ++ assert.equal(readFileSync(logPath, 'utf8'), '') ++ assert.equal(existsSync(rotated), false) ++}) ++ ++// The same defect through the folder instead of the name: a `logs/` without its write bit refuses the ++// removal (EACCES) while the leaf itself is still an ordinary file the host may truncate, so before the ++// fix the truncate never ran at all. Root ignores the folder's mode, so the case is skipped there. ++test('#2358: clear truncates the leaf when the folder refuses the removal', async (t) => { ++ if (process.getuid?.() === 0) { ++ t.skip('root removes from a folder without its write bit') ++ return ++ } ++ const home = mkdtempSync(join(tmpdir(), 'ds-bot-diag-')) ++ const logs = join(home, 'logs') ++ t.after(() => { ++ try { chmodSync(logs, 0o755) } catch { /* the folder may already be gone */ } ++ rmSync(home, { recursive: true, force: true, maxRetries: 5 }) ++ }) ++ mkdirSync(logs, { recursive: true }) ++ const logPath = join(logs, 'diagnostics.jsonl') ++ const rotated = `${logPath}.1` ++ ++ const stderr = [] ++ const quiet = t.mock.method(console, 'error', (...args) => stderr.push(args.join(' '))) ++ const rt = host(home) ++ rt.warn('ds-bot: written before the clear') ++ await rt.dispose() ++ writeFileSync(rotated, 'ROTATED\n'.repeat(8)) ++ chmodSync(logs, 0o555) ++ ++ await rt.diagnostics.clear() ++ quiet.mock.restore() ++ ++ assert.equal(readFileSync(logPath, 'utf8'), '', 'the leaf is truncated although the folder refused the removal') ++ assert.equal(readFileSync(rotated, 'utf8'), 'ROTATED\n'.repeat(8), 'the copy that could not be removed keeps its bytes') ++ const noted = stderr.filter(line => /rotated copy/.test(line)) ++ assert.equal(noted.length, 1, 'one line says the rotated copy was not removed') ++ assert.equal(noted[0].includes(home), false, 'the line does not print the home folder') ++ ++ chmodSync(logs, 0o755) ++ await rt.diagnostics.clear() ++ assert.equal(existsSync(rotated), false, 'once the folder allows it, the same clear removes the copy') ++})