host: a Clear that cannot remove the rotated log leaves the leaf untouched and silent

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

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

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

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
 

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

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