From 10e87463cb16f7e335d926fe04b5db9f30262c38 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Thu, 22 Oct 2026 15:44:00 +0800 Subject: [PATCH] Pair a turn end with its own turn in the tree load tracker (#696) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The feed names the turn it is about (`turnId`), and the load tracker threw it away: it kept only start times, so `end()` dropped whichever turn was oldest. With two turns open and the later one ending first, `openFor` reported the newer turn's age — the exact state #624 exists to expose, hidden. - load.js keeps each open turn as `{ turnId?, at }`, and `end(id, turnId)` closes that turn by id. No match — an id never seen, or none at all — still drops exactly one turn, the oldest, so the count cannot drift. - tree.js and index.js hand the feed's `turnId` through instead of dropping it. - An end event that arrives without an id (an older feed, or a caller with only a Bot id) therefore keeps working exactly as before. --- docs/subsystems/tree.md | 4 +-- plugins/tree/index.js | 6 +++-- plugins/tree/src/load.js | 47 +++++++++++++++++++--------------- plugins/tree/src/tree.js | 4 +-- plugins/tree/test/load.test.js | 26 +++++++++++++++++++ plugins/tree/test/tree.test.js | 26 +++++++++++++++++++ 6 files changed, 87 insertions(+), 26 deletions(-) diff --git a/docs/subsystems/tree.md b/docs/subsystems/tree.md index 549b2efb2..2732368cf 100644 --- a/docs/subsystems/tree.md +++ b/docs/subsystems/tree.md @@ -88,7 +88,7 @@ A coordinator can see which of its children are busy right now, so it sends a ne A count alone cannot tell a long turn from a stuck one: `load: 1` read the same for a turn three seconds in and for one that had been open 47 minutes without consuming its queue. -- The tracker keeps each open turn's start time, so `openFor(id)` answers the age of the **oldest** open turn and `0` when the Bot is idle; `end(id)` drops the oldest one (end events may arrive in any order, only the count has to stay right). +- The tracker keeps each open turn's start time, so `openFor(id)` answers the age of the **oldest** open turn and `0` when the Bot is idle. An end closes **its own** turn: `index.js` hands the feed's `turnId` to `end(id, turnId)`, because dropping whichever turn was oldest left the *newer* turn's age behind when a later turn ended first (#696). No match — an id never seen, or none at all — still drops exactly one turn, the oldest, so the count cannot drift. - Past `STALLED_AFTER_MS` (12 minutes) the coordinator's line says the age instead of just the count — `Work: working (47 min, no turn ended)`, or `Work: busy (2 running, oldest open 47 min)`. `list_children` gains `openForMs` (and `stalled: true` past the threshold) only for a child with a turn open, so an idle child keeps exactly its old shape and every existing reader of `load` keeps a number. - **A hint for routing, never a gate.** The tree plugin cannot see a Bot's inbox, so a legitimately long turn ages too; nothing here refuses, ends or reassigns work. `loadLine`'s third argument is optional, so a caller that only has a count keeps the old line. @@ -113,7 +113,7 @@ The plugin's discarded `.catch(() => {})` calls carry a one-line reason comment ## Tests -Covered by the plugin's own tests under `plugins/tree/` / core test suite as wired. The `#447` tests cover reuse of a same-role Bot at depth 3 and `find_bots` (scope, fields, ranking), `create_child_bot(parent)` refusing a non-descendant (sibling, ancestor, outsider, unknown) and checking limits at the target under its lock, `create_team` (Chief → FlyBot Lead → four members, one slot; reuse of the lead and members; a flat member's slot at a full level, journal and undo), later Bots joining their group's lead (only down leads of groups they belong to), and orphan reattachment; `test/host.test.js` runs `find_bots`, `create_team` and `create_child_bot(parent)` on the real host. The `#446` tests in `test/tree.test.js` cover role grouping (Life Lead, FlyBot Lead), existing leads, no unrelated fallback, the cap at every step, the daily tidy (merge, idle, archive), journal, undo and the client notice; `test/host.test.js` runs `create_bot` and the `tree/reorg` / `tree/undo` endpoints on the real host. `#16 Option B` in `test/tree.test.js` (and the matching host test) asserts a non-Main tree Bot can create a child under itself, that `maxChildren`/depth still refuse, and that the child is not auto-granted. Runtime load (#507) is covered by `test/load.test.js` (pure) and a `test/tree.test.js` case: a child's running turns show in the coordinator's stored prompt and `list_children`, and drop to zero at `turn.end`. The age of an open turn (#624) is covered in both: `openFor` reads the oldest start and falls back to the newer turn once one ends, the line names the age only past the threshold and never calls an idle child stalled, and `list_children` adds `openForMs`/`stalled` for an open turn while an idle child keeps the old shape. +Covered by the plugin's own tests under `plugins/tree/` / core test suite as wired. The `#447` tests cover reuse of a same-role Bot at depth 3 and `find_bots` (scope, fields, ranking), `create_child_bot(parent)` refusing a non-descendant (sibling, ancestor, outsider, unknown) and checking limits at the target under its lock, `create_team` (Chief → FlyBot Lead → four members, one slot; reuse of the lead and members; a flat member's slot at a full level, journal and undo), later Bots joining their group's lead (only down leads of groups they belong to), and orphan reattachment; `test/host.test.js` runs `find_bots`, `create_team` and `create_child_bot(parent)` on the real host. The `#446` tests in `test/tree.test.js` cover role grouping (Life Lead, FlyBot Lead), existing leads, no unrelated fallback, the cap at every step, the daily tidy (merge, idle, archive), journal, undo and the client notice; `test/host.test.js` runs `create_bot` and the `tree/reorg` / `tree/undo` endpoints on the real host. `#16 Option B` in `test/tree.test.js` (and the matching host test) asserts a non-Main tree Bot can create a child under itself, that `maxChildren`/depth still refuse, and that the child is not auto-granted. Runtime load (#507) is covered by `test/load.test.js` (pure) and a `test/tree.test.js` case: a child's running turns show in the coordinator's stored prompt and `list_children`, and drop to zero at `turn.end`. The age of an open turn (#624) is covered in both: `openFor` reads the oldest start and falls back to the newer turn once one ends, an end that names its turn closes that one and not the oldest (#696, unknown and missing ids included), the line names the age only past the threshold and never calls an idle child stalled, and `list_children` adds `openForMs`/`stalled` for an open turn while an idle child keeps the old shape. ## Common mistakes diff --git a/plugins/tree/index.js b/plugins/tree/index.js index 05abc3f51..4dc695c9f 100644 --- a/plugins/tree/index.js +++ b/plugins/tree/index.js @@ -33,8 +33,10 @@ export function apply(ctx, config = {}) { // Runtime load: how many turns each Bot runs now, so a coordinator routes to a free child (#507). // A change is also written into the child's parent prompt, so the Child-load line a coordinator // reads stays current (writeBlock only rewrites the block when the line really changed). - fb.onFeed('turn.start', e => { tree.noteTurnStart(e?.botId); refreshParentPrompt(e?.botId) }), - fb.onFeed('turn.end', e => { tree.noteTurnEnd(e?.botId); refreshParentPrompt(e?.botId) }), + // The feed names the turn it is about (turnId); dropping it made an end close whichever turn was + // oldest, so with two open and the later one ending first, openFor reported the newer age (#696). + fb.onFeed('turn.start', e => { tree.noteTurnStart(e?.botId, e?.turnId); refreshParentPrompt(e?.botId) }), + fb.onFeed('turn.end', e => { tree.noteTurnEnd(e?.botId, e?.turnId); refreshParentPrompt(e?.botId) }), // The tree view: what the last reorganization did, one-click undo, dismiss. fb.handleApi('tree/reorg', () => tree.overview(), { read: true }), fb.handleApi('tree/undo', payload => tree.undo(typeof payload?.runId === 'string' ? payload.runId : undefined)), diff --git a/plugins/tree/src/load.js b/plugins/tree/src/load.js index 911eff113..c3af1eb10 100644 --- a/plugins/tree/src/load.js +++ b/plugins/tree/src/load.js @@ -5,44 +5,51 @@ // // How long a turn has been open (#624): a count alone cannot tell a long turn from a stuck one — // `load: 1` read identically for a 47-minute-old turn that never consumed its queue and for a -// turn three seconds in. Each open turn's start is kept, so `openFor()` answers the age of the -// oldest one and a reader can draw its own line. It is a hint for routing, never a gate: this +// turn three seconds in. Each open turn's start is kept, keyed by the feed's turnId when it has one +// so an end closes the turn that really ended (#696), and `openFor()` answers the age of the oldest +// one still open. A reader can draw its own line. It is a hint for routing, never a gate: this // plugin cannot see a Bot's inbox, so a legitimately long turn also ages. export const STALLED_AFTER_MS = 12 * 60_000 const minutes = ms => Math.max(1, Math.round(ms / 60_000)) export function createLoadTracker({ now = Date.now } = {}) { - /** @type {Map} start times of the open turns per Bot id, oldest first */ + /** @type {Map} open turns per Bot id, oldest first */ const open = new Map() - const timesOf = id => open.get(id) ?? [] + const turnsOf = id => open.get(id) ?? [] + const cleanTurn = value => (typeof value === 'string' && value !== '' ? value : undefined) return { - start(id) { + start(id, turnId) { if (typeof id !== 'string' || id === '') return - const times = timesOf(id) - times.push(now()) - open.set(id, times) + const turns = turnsOf(id) + const key = cleanTurn(turnId) + turns.push({ ...(key === undefined ? {} : { turnId: key }), at: now() }) + open.set(id, turns) }, - end(id) { + end(id, turnId) { if (typeof id !== 'string' || id === '') return - const times = timesOf(id) - // Oldest first, so an end drops the oldest still-open turn and what remains is the age of the - // oldest one left. The end events may arrive in any order; only the count has to stay right. - times.shift() - if (times.length === 0) open.delete(id) - else open.set(id, times) + const turns = turnsOf(id) + if (turns.length === 0) return + // An end that names its turn closes that one, so a later turn ending first no longer closes an + // earlier one and leaves the age of the wrong turn behind (#696). With no match — an id never + // seen, or none at all — exactly one turn goes, the oldest, so the count still falls by one. + const key = cleanTurn(turnId) + const at = key === undefined ? -1 : turns.findIndex(turn => turn.turnId === key) + turns.splice(at >= 0 ? at : 0, 1) + if (turns.length === 0) open.delete(id) + else open.set(id, turns) }, of(id) { - return timesOf(id).length + return turnsOf(id).length }, /** Age in ms of the oldest open turn, 0 when the Bot is idle. */ openFor(id, at = now()) { - const times = timesOf(id) - if (times.length === 0) return 0 - return Math.max(0, at - times[0]) + const turns = turnsOf(id) + if (turns.length === 0) return 0 + return Math.max(0, at - turns[0].at) }, snapshot() { - return Object.fromEntries([...open].map(([id, times]) => [id, times.length])) + return Object.fromEntries([...open].map(([id, turns]) => [id, turns.length])) }, } } diff --git a/plugins/tree/src/tree.js b/plugins/tree/src/tree.js index 28c51dc61..edaf0e9f6 100644 --- a/plugins/tree/src/tree.js +++ b/plugins/tree/src/tree.js @@ -309,8 +309,8 @@ export function createTree(fb, { limits, rootName = 'Root', rootBotId, adoptExis const { tidy: tidyNow, maybeTidy, undo, dismiss, overview, noteActivity, idle: settled, repairArchiveNames } = organizer const { findBots, createChild: allocateChild, createTeam } = allocator - const noteTurnStart = id => load.start(id) - const noteTurnEnd = id => load.end(id) + const noteTurnStart = (id, turnId) => load.start(id, turnId) + const noteTurnEnd = (id, turnId) => load.end(id, turnId) const loadOf = id => load.of(id) const openForOf = id => load.openFor(id) return { tidy: tidyNow, maybeTidy, undo, dismiss, overview, noteActivity, noteTurnStart, noteTurnEnd, loadOf, openForOf, refreshPrompt, parentOf, settled, repairArchiveNames, limits, forest, adoptUnattached, snapshot, childrenOf, depthOf, heightOf, traceOf, ensureRoot, adoptChief, findChief, migrateLegacyRoots, promptFor, createChild, allocateChild, createTeam, findBots, moveBot, findBotByName, isMember, isRoot, rootOf, isTreeEdge, onFieldChange, onBotRemoved, subscribe } diff --git a/plugins/tree/test/load.test.js b/plugins/tree/test/load.test.js index f5a833e8a..7765bc915 100644 --- a/plugins/tree/test/load.test.js +++ b/plugins/tree/test/load.test.js @@ -53,3 +53,29 @@ test('#624: loadLine names the age only past the threshold, and never calls a ch // The third argument is optional, so a caller with only a count keeps the old line. assert.match(loadLine(children, () => 1), /Dev: working\. Send/) }) + +test('#696: an end closes its own turn, not just the oldest open one', () => { + let clock = 0 + const load = createLoadTracker({ now: () => clock }) + load.start('a', 's#1') + clock += 10 * 60_000 + load.start('a', 's#2') + clock += 2 * 60_000 + assert.equal(load.of('a'), 2) + // The later turn is the one that ended, so the age left behind must be the earlier turn's. + load.end('a', 's#2') + assert.equal(load.of('a'), 1) + assert.equal(load.openFor('a'), 12 * 60_000, 'the turn that really ended is gone, the older one stays') + // An id we never saw still drops exactly one turn — the oldest — so the count cannot drift. + load.start('a', 's#3') + clock += 60_000 + load.end('a', 'never-seen') + assert.equal(load.of('a'), 1) + assert.equal(load.openFor('a'), 60_000, 'the unknown end dropped the oldest, not the newest') + // An end with no id at all is the same fallback. + load.start('a', 's#4') + load.end('a') + assert.equal(load.of('a'), 1) + load.end('a', 's#4') + assert.equal(load.of('a'), 0, 'the named turn is closed and nothing is left') +}) diff --git a/plugins/tree/test/tree.test.js b/plugins/tree/test/tree.test.js index 100fa181e..b95e085bd 100644 --- a/plugins/tree/test/tree.test.js +++ b/plugins/tree/test/tree.test.js @@ -1470,6 +1470,32 @@ test('#624: a turn that never ends shows how long it has been open, and load sta assert.equal(item.stalled, undefined) }) +test('#696: two turns open, the later one ends, and the age left is the older turn\'s', async () => { + let clock = Date.UTC(2026, 9, 22, 7) + const m = await setup({ now: () => clock }) + const work = await make(m, m.root, 'Work') + m.feed('turn.start', { botId: work, place: 'chat', sessionId: 's', turnId: 's#1', turn: 1, time: 1 }) + clock += 10 * 60_000 + m.feed('turn.start', { botId: work, place: 'chat', sessionId: 's', turnId: 's#2', turn: 2, time: 2 }) + clock += 2 * 60_000 + // 12 minutes: the older turn is the oldest open one, so the line names 12 whatever ends next. + let item = (await m.call(m.root, 'list_children')).children.find(c => c.id === work) + assert.equal(item.load, 2) + assert.equal(item.openForMs, 12 * 60_000) + // The turn that ends is the newer one: the older turn is still open, so its age must stay. + m.feed('turn.end', { botId: work, place: 'chat', sessionId: 's', turnId: 's#2', turn: 2, time: 3 }) + item = (await m.call(m.root, 'list_children')).children.find(c => c.id === work) + assert.equal(item.load, 1, 'only the turn that ended is gone') + assert.equal(item.openForMs, 12 * 60_000, 'not the 2 minutes of the turn that ended') + assert.equal(item.stalled, true) + m.tree.refreshPrompt(m.root) + assert.match(sp(m, m.root), /Work: working \(12 min, no turn ended\)/) + m.feed('turn.end', { botId: work, place: 'chat', sessionId: 's', turnId: 's#1', turn: 1, time: 4 }) + item = (await m.call(m.root, 'list_children')).children.find(c => c.id === work) + assert.equal(item.load, 0) + assert.equal(item.openForMs, undefined) +}) + test('#450: a merge carries the archived Bot\'s own memory into the kept Bot', async () => { const DAY = 24 * 60 * 60 * 1000 let clock = Date.UTC(2026, 9, 19, 12)