fix(tree): clamp jobTimeoutMs to the timer ceiling so a job is never abandoned after 1 ms

changelog.d/2811-tree-jobtimeout-ceiling.fixed.md

@@ -0,0 +1 @@
+The tree plugin's `jobTimeoutMs` is capped at 2147483647 ms (about 24.8 days): a larger value — 30 days is the natural way to ask for "effectively no deadline" — was handed to Node's `setTimeout`, which silently replaced it with 1 ms, so every queued reorganization job was reported abandoned on a healthy host while the host call it was waiting for kept running and writing. The value is now clamped to the ceiling with one warning that names the deadline actually used, so a job whose host call answers inside it resolves normally. See [#2811](https://github.com/ThinkFlowLab/FlyBot/issues/2811).

docs/subsystems/tree.md

@@ -76,7 +76,7 @@ The tree organizes itself by role instead of leaving Bots outside it or a parent
 - **Placement** of a Bot outside the tree: its namesake, else the tree Bot that leads a group it belongs to (most specific key first, then the shallower lead; an area only through a lead made for it, e.g. "Work Lead"), else the root. At the first of those with room it is placed. A full level is **regrouped**: the most specific group of two or more children (the newcomer counted) moves under a suitable existing member, or under a new lead. Inside a full lead of its own group with no finer group, the member closest to its role takes it (all members share the group). There is **no "least-loaded child" fallback**: a Bot that fits no group while every level it matches is full stays outside, a warning names it and the sidebar tags it "outside the tree"; the next attempt (any `bot.created`, startup) tries again.
 - **Leads.** A suitable existing lead is a member already marked as that group's lead, already coordinating that group (it has children and the key, project and domain only), or named for it ("Life", "FlyBot Lead") or as a lead ("Kitchen Manager", 负责人). It gets `lead { key, label }`. Only when none fits a new lead is created with a clear role (`<Label> Lead`, role "Lead of the <Label> group: coordinates …"); its members move under it before it joins the level. A lead's own group key, and broad areas below any lead, are ignored inside it, so there is no second "Dev Lead" under "Dev Lead".
 - **Repair**: a parent over `maxChildren` (after a deletion re-attached a deleted Bot's children) is regrouped the same way, at once when the lead exists, else in the queue.
-- **One queue, bounded per job (#2751):** placement, repair and tidy run one at a time on the reorganization queue. A job whose host call never answers is abandoned after 10 minutes (`flybot-tree: reorganization job <id> was abandoned after …`): the next job still runs and `settled()` still resolves, and an `exclusive()` job's promise rejects with `abandoned: true` and its job id instead of hanging. The deadline is the plugin's `jobTimeoutMs` config key — a finite number of milliseconds above 0; `0`, a negative number, a numeric string and `NaN` all keep the ten-minute default. The abandoned work cannot be cancelled, so it may still write afterwards.
+- **One queue, bounded per job (#2751):** placement, repair and tidy run one at a time on the reorganization queue. A job whose host call never answers is abandoned after 10 minutes (`flybot-tree: reorganization job <id> was abandoned after …`): the next job still runs and `settled()` still resolves, and an `exclusive()` job's promise rejects with `abandoned: true` and its job id instead of hanging. The deadline is the plugin's `jobTimeoutMs` config key — a finite number of milliseconds above 0, capped at 2147483647 ms (Node's `setTimeout` ceiling, about 24.8 days): a larger value is clamped to that ceiling and one warning names the effective deadline, because Node would otherwise silently shorten the delay to 1 ms; `0`, a negative number, a numeric string and `NaN` all keep the ten-minute default. The abandoned work cannot be cancelled, so it may still write afterwards.
 - **Daily tidy** (checked hourly, runs once a day; the first check after install only records the time; `dailyTidy: false` turns it off): (1) leaf Bots with no activity for `tidy.idleDays` (14) get `idle` (any turn or tool call clears it) and are **archived** after `tidy.archiveIdleDays` (30) more days; Bots with children, granted Bots and the root are never idle; (2) duplicates: two Bots with essentially the same role (same name once generic words are dropped and no conflicting description, or near-identical descriptions in one domain), both at least `tidy.mergeGraceDays` (1) in the tree, are **merged**: the newer one's children move to the kept Bot, which lists it in `mergedFrom` and is told in its prompt; the old one is archived; (3) project and domain groups of three or more (or two with an existing lead) on every level, never a group of a whole level without a lead.
 - **Archive, never delete**: an archived Bot leaves the tree (`archived { at, reason, parent, treeRole, into? }`, tree fields and block removed) but keeps its chat, memory and grants. It is **hidden from the default Bot list** (`bot.hidden` / structure `include`; Search still finds it) (#452), and the sidebar's `flybot-tree-archived` section is the way back to its chat (#810): collapsed by default, header `Archived (N)`, drawn only while N > 0, one row per archived Bot with the same tag `statusOf()` gives (`archived`, `archived · merged into <name>`) opening the chat through `actions.openSession`. `include()` still returns false, so the list and `botTree()` never see it. Deleting it stays the owner's own click. That section is registered at `order: 88` and the reorganization notice below at `order: 89`; a slot renders ascending, so the archived Bots sit **above** the notice. On a merge (#450 item 1), the archived Bot's own memory is carried into the kept Bot through `flybot.memory` (at most 20 entries, tagged `source: merged`), so a merge loses nothing; without the host's memory seam the merge still stands.
 - **Retire from the core (#638):** the Main Bot's `archive_bot` writes `fields['flybot-tree'].retired = true` (never `archived`, which is the tidy's own record) and sets `bot.hidden`, so the Bot leaves the tree and the default list the way an archived one does while its history stays readable; adoption skips a retired record and `forestOf` no longer counts it, and since #2430 the model itself does too: `snapshot()` and `isMember()` leave the record out (so it occupies no `maxChildren` slot, no `list_children` row and no prompt trace), and the field change that retires it hands its children to its own parent — or to the current Main for a retired root — with every moved Bot's prompt refreshed, instead of stranding them under a Bot the host refuses to deliver to. The tree panel (`treeOf`) skips a retired Bot the same way (#1793), and the `flybot-tree-archived` section lists it with the `retired` tag so its chat stays reachable; a record that also carries `archived` keeps the archived tag.

plugins/tree/src/organize.js

@@ -19,6 +19,10 @@ import { systemPrompt, withTreeBlock, BLOCK_START } from './prompt.js'
 const DAY = 24 * 60 * 60 * 1000
 // #2751: how long one reorganization job may wait on the host before the queue gives up on it.
 const DEFAULT_JOB_TIMEOUT_MS = 10 * 60 * 1000
+// #2811: Node's `setTimeout` cannot hold a delay above a signed 32-bit millisecond count — it
+// rewrites it to 1ms and prints TimeoutOverflowWarning — so a deadline meant as "effectively none"
+// would give up on the next tick. Anything above the ceiling is clamped to it.
+const MAX_JOB_TIMEOUT_MS = 2_147_483_647
 const DOMAIN_AREA = new Map(DOMAINS.map(d => [d.id, d.area]))
 export const DEFAULT_TIDY = Object.freeze({ idleDays: 14, archiveIdleDays: 30, mergeGraceDays: 1, pinDays: 14, keepRuns: 5 })
 /** Mis-archive rename prefix (Chief update_bot / manual): 归档-Name or Archive-Name. Reorg never writes this. */
@@ -313,7 +317,11 @@ export function createOrganizer({ fb, limits, snapshot, childrenOf, depthOf, war
   // host does not answer in time, the queue records the abandoned job, warns with its id and moves
   // on. The work behind it cannot be cancelled, so it may still write later; the queue no longer
   // waits for it.
-  const jobTimeoutMs = Number.isFinite(timeoutConfig) && timeoutConfig > 0 ? timeoutConfig : DEFAULT_JOB_TIMEOUT_MS
+  const configuredTimeoutMs = Number.isFinite(timeoutConfig) && timeoutConfig > 0 ? timeoutConfig : undefined
+  const jobTimeoutMs = configuredTimeoutMs === undefined ? DEFAULT_JOB_TIMEOUT_MS : Math.min(configuredTimeoutMs, MAX_JOB_TIMEOUT_MS)
+  if (configuredTimeoutMs > MAX_JOB_TIMEOUT_MS) {
+    warn(`flybot-tree: jobTimeoutMs=${configuredTimeoutMs}ms is above Node's ${MAX_JOB_TIMEOUT_MS}ms timer ceiling; using ${MAX_JOB_TIMEOUT_MS}ms`)
+  }
   let queue = Promise.resolve()
   let pending = 0
   let jobSeq = 0

plugins/tree/test/job-timeout-ceiling.test.js

@@ -0,0 +1,154 @@
+// #2811: the deadline one reorganization job may wait on the host is validated as "a finite number
+// above 0", but Node's `setTimeout` cannot hold a delay above 2**31 - 1 ms (2147483647, about 24.8
+// days): it silently rewrites the delay to 1 ms and prints `TimeoutOverflowWarning`. A user who
+// writes 30 days to mean "effectively no deadline" therefore got the deadline on the next tick —
+// every queued job reported abandoned on a healthy host, while the host call kept running and
+// writing. These cases pin the ceiling: a healthy job must resolve, and the reported deadline must
+// be the one the timer really uses.
+import assert from 'node:assert/strict'
+import { test } from 'node:test'
+import { createOrganizer } from '../src/organize.js'
+
+const CEILING = 2 ** 31 - 1
+const DEFAULT_JOB_TIMEOUT_MS = 10 * 60 * 1000
+const TOO_LONG = 30 * 24 * 60 * 60 * 1000 // 2592000000ms
+
+// The queue needs nothing from the tree itself: no job below reads a Bot or a snapshot, so the
+// organizer is built with the smallest host that satisfies its constructor.
+function organizerAt({ jobTimeoutMs } = {}) {
+  const warnings = []
+  const fields = new Map()
+  const fb = {
+    getField: (id, key) => fields.get(`${id}:${key}`),
+    setField: (id, key, value) => { if (value == null) fields.delete(`${id}:${key}`); else fields.set(`${id}:${key}`, value) },
+    getBot: id => ({ id, name: id }),
+    listBots: () => [],
+    setSystemPrompt: () => {},
+    createBot: () => new Promise(() => {}),
+    updateBot: async () => {},
+    memoryRead: () => [],
+    memoryAppend: async () => ({ ok: true }),
+    isRemote: () => false,
+    hasAnyGrant: () => false,
+  }
+  const organizer = createOrganizer({
+    fb,
+    limits: { maxChildren: 8, maxDepth: 4 },
+    snapshot: () => ({}),
+    childrenOf: () => [],
+    depthOf: () => 0,
+    warn: (...args) => warnings.push(args.map(String).join(' ')),
+    refreshAll: () => {},
+    writeBlock: () => {},
+    findRootId: () => 'root',
+    now: () => 1_700_000_000_000,
+    classify: () => ({}),
+    jobTimeoutMs,
+  })
+  return { organizer, warnings }
+}
+
+const sleep = ms => new Promise(resolve => setTimeout(resolve, ms))
+
+// Node reports the overflow as a process warning rather than an error, so a case that only reads
+// the queue's own warning would miss the 1 ms timer behind it.
+async function captureOverflowWarnings(run) {
+  const overflow = []
+  const onWarning = warning => { if (warning.name === 'TimeoutOverflowWarning') overflow.push(warning.message) }
+  process.on('warning', onWarning)
+  try {
+    const value = await run()
+    await sleep(0) // a warning is emitted on a later tick
+    return { value, overflow }
+  } finally {
+    process.off('warning', onWarning)
+  }
+}
+
+test('#2811 a deadline above the timer ceiling does not abandon a healthy job', { timeout: 10_000 }, async () => {
+  const { organizer, warnings } = organizerAt({ jobTimeoutMs: TOO_LONG })
+  const started = process.hrtime.bigint()
+  let value, failure
+  try {
+    ;({ value } = await captureOverflowWarnings(() => organizer.shared.exclusive(async () => {
+      await sleep(50)
+      return 'host answered'
+    })))
+  } catch (error) {
+    failure = error
+  }
+  const elapsedMs = Number(process.hrtime.bigint() - started) / 1e6
+  const warned = warnings.join('\n')
+
+  assert.equal(
+    failure,
+    undefined,
+    `the host answered in 50ms, but the job ${failure ? `was given up on after ${elapsedMs.toFixed(1)}ms: ${failure.message}` : ''}` +
+      `; warnings: ${warned || '(none)'}`,
+  )
+  assert.equal(value, 'host answered')
+  assert.ok(elapsedMs >= 40, `the job resolved after ${elapsedMs.toFixed(1)}ms, so its deadline fired almost at once`)
+  assert.doesNotMatch(warned, /abandoned/, `a job the host answered must not be warned about as abandoned; warnings: ${warned}`)
+})
+
+test('#2811 a value above the ceiling is clamped, and the give-up names the deadline the timer uses', { timeout: 10_000 }, async () => {
+  const { organizer, warnings } = organizerAt({ jobTimeoutMs: TOO_LONG })
+  const atConstruction = warnings.slice()
+
+  // A host that never answers: timers are capped so the case does not wait 24.8 days, but the
+  // number in the give-up must still be the one the configured deadline really is.
+  const realSetTimeout = globalThis.setTimeout
+  globalThis.setTimeout = (fn, ms, ...rest) => realSetTimeout(fn, Math.min(ms, 250), ...rest)
+  let failure
+  try {
+    await organizer.shared.exclusive(async () => new Promise(() => {}))
+  } catch (error) {
+    failure = error
+  } finally {
+    globalThis.setTimeout = realSetTimeout
+  }
+
+  assert.equal(failure?.abandoned, true, `a host call that never answers must still be abandoned; got ${failure?.message ?? 'a resolved job'}`)
+  assert.match(
+    failure.message,
+    new RegExp(`abandoned after ${CEILING}ms`),
+    `the give-up must name the clamped deadline the timer uses, not the value it could not hold: ${failure.message}`,
+  )
+  const ceilingWarnings = atConstruction.filter(line => line.includes(String(CEILING)))
+  assert.equal(ceilingWarnings.length, 1, `exactly one warning must name the effective deadline; warnings: ${atConstruction.join(' | ') || '(none)'}`)
+  assert.match(ceilingWarnings[0], new RegExp(`jobTimeoutMs=${TOO_LONG}ms`), 'the warning also names the value the user configured')
+})
+
+test('#2811 the ceiling itself is a valid deadline, and one millisecond more is clamped', async () => {
+  const atCeiling = organizerAt({ jobTimeoutMs: CEILING })
+  assert.deepEqual(atCeiling.warnings, [], '2147483647ms is a deadline setTimeout can hold, so it needs no warning')
+
+  const above = organizerAt({ jobTimeoutMs: CEILING + 1 })
+  assert.equal(above.warnings.length, 1, `one warning above the ceiling; warnings: ${above.warnings.join(' | ') || '(none)'}`)
+  assert.match(above.warnings[0], new RegExp(`${CEILING + 1}ms`))
+  assert.match(above.warnings[0], new RegExp(`using ${CEILING}ms`))
+})
+
+test('#2811 values the docs already reject still fall back to ten minutes', async () => {
+  for (const ignored of [0, -5, '30', Number.NaN, Infinity, null, undefined]) {
+    const { warnings } = organizerAt({ jobTimeoutMs: ignored })
+    assert.deepEqual(
+      warnings,
+      [],
+      `jobTimeoutMs=${String(ignored)} falls back silently to the ten-minute default; warnings: ${warnings.join(' | ')}`,
+    )
+  }
+  // The fallback is the default deadline, not the ceiling: a hanging job reports 600000ms.
+  const { organizer, warnings } = organizerAt({ jobTimeoutMs: Infinity })
+  const realSetTimeout = globalThis.setTimeout
+  globalThis.setTimeout = (fn, ms, ...rest) => realSetTimeout(fn, Math.min(ms, 50), ...rest)
+  let failure
+  try {
+    await organizer.shared.exclusive(async () => new Promise(() => {}))
+  } catch (error) {
+    failure = error
+  } finally {
+    globalThis.setTimeout = realSetTimeout
+  }
+  assert.match(failure?.message ?? '', new RegExp(`abandoned after ${DEFAULT_JOB_TIMEOUT_MS}ms`), `warnings: ${warnings.join(' | ')}`)
+})