From 74ba7a021320b4e3e2592d9f4d1430933f2d708d Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Wed, 15 Jul 2026 14:07:19 +0200 Subject: [PATCH] crash-ding: page cos + the worker's spawner, not the whole permanent crew (Nathan-flagged) + fix double-ding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The crash-ding paged EVERY --permanent agent on one worker crash — permanence was used as a proxy for "orchestrator," but every repo-owner runs --permanent, so a throwaway worker crash paged the entire standing crew (13 agents; pulled app-apple off task). TARGETING (crashDingTargets): now targets (a) the CoS — always, via a convoy.tier=cos tag (network-wide backstop, no hardcoded id); (b) the crashed agent's ACTUAL supervisor — its convoy.spawner tag (bus id of whoever ran convoy add). NOT the permanent set. notify + no-self-ding preserved. WIRING (writePtyToml/nativeLaunch): stamp convoy.tier=cos (role=chief-of-staff) + convoy.spawner= on the HARNESS session (never the ding sidecar). convoy up re-asserts both across a permanent respawn (pty restart strips tags) so the cos backstop survives cos restarting. DOUBLE-DING (found while diagnosing the crashtest test): a worker's DING sidecar fired its own crash-ding alongside the agent (both resolve the same busId) -> 2 dings/crash. Ding sessions are now excluded from crash/flapping dings (still respawned for permanents, silently). One crash = one ding. Preserved: clean exit 0 = silent; nonzero/vanished/null = crash (workerCrashed unchanged). tsc clean, 163 green — incl the acceptance (worker crash pages ONLY cos + its spawner, NOT unrelated permanents like app-apple), the clean-exit-silent negative control, and the writePtyToml tag wiring. --- src/launch.test.ts | 27 ++++++++++++++++ src/launch.ts | 19 ++++++++--- src/up.test.ts | 64 +++++++++++++++++++------------------ src/up.ts | 80 ++++++++++++++++++++++++++++++---------------- 4 files changed, 127 insertions(+), 63 deletions(-) diff --git a/src/launch.test.ts b/src/launch.test.ts index 8db90c5..1f490aa 100644 --- a/src/launch.test.ts +++ b/src/launch.test.ts @@ -143,6 +143,33 @@ describe("writePtyToml (pinned hostname-prefixed ids, cold start)", () => { rmSync(dir, { recursive: true, force: true }); } }); + + it("crash-ding tags: cos gets convoy.tier=cos + spawner gets convoy.spawner, on the HARNESS session only (not the ding)", () => { + const dir = mkdtempSync(join(tmpdir(), "convoy-ptytoml-tags-")); + try { + // a worker spawned by a supervisor: convoy.spawner recorded, no convoy.tier (not cos) + writePtyToml(dir, spec({ role: "worker", identity: "wk-claude" }), { spawner: "sup-claude" }); + const wkToml = readFileSync(join(dir, "pty.toml"), "utf8"); + expect(wkToml).toContain('"convoy.spawner" = "sup-claude"'); + expect(wkToml).not.toContain("convoy.tier"); + // the spawner tag is on the harness session, NOT the ding (else a crash double-dings the same busId) + expect(wkToml.match(/convoy\.spawner/g)?.length).toBe(1); + + // the CoS: convoy.tier=cos stamped (the always-ding backstop) + writePtyToml(dir, spec({ role: "chief-of-staff", identity: "cos-claude" })); + const cosToml = readFileSync(join(dir, "pty.toml"), "utf8"); + expect(cosToml).toContain('"convoy.tier" = "cos"'); + expect(cosToml).not.toContain("convoy.spawner"); // no spawner passed + + // no spawner passed + not cos → neither tag (a human-spawned worker → cos-only ding downstream) + writePtyToml(dir, spec({ role: "worker", identity: "wk2-claude" })); + const plainToml = readFileSync(join(dir, "pty.toml"), "utf8"); + expect(plainToml).not.toContain("convoy.spawner"); + expect(plainToml).not.toContain("convoy.tier"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); }); describe("discoverSmalltalkDir (fresh-install hook discovery, no SMALLTALK_DIR needed)", () => { diff --git a/src/launch.ts b/src/launch.ts index 14087b2..c45212d 100644 --- a/src/launch.ts +++ b/src/launch.ts @@ -119,14 +119,23 @@ export function dingCommand(busId: string, claudeSessionId: string, root?: strin /** Serialize the per-agent pty.toml (pty's manifest format — NOT a convoy.toml). Pins the session ids * to `.` (claude) and `..ding` so the ding + name refs stay - * stable across respawns; prefix defaults to the short hostname. */ -export function writePtyToml(dir: string, spec: AgentSpec): void { + * stable across respawns; prefix defaults to the short hostname. `opts.spawner` (the bus id of whoever ran + * `convoy add`, from their ST_AGENT) is stamped on the HARNESS session so a crash-ding reaches this agent's + * ACTUAL supervisor, not the whole permanent crew (see up.ts crashDingTargets). */ +export function writePtyToml(dir: string, spec: AgentSpec, opts?: { spawner?: string | null }): void { const busId = spec.identity; // the bus identity, e.g. convoy-claude const root = spec.networkRoot; const harnessId = sessionId(spec); // e.g. silber.convoy (agentShort strips the -claude/-codex suffix) const dingId = `${harnessId}.ding`; // e.g. silber.convoy.ding const permanent = specPermanent(spec); const stTag = root ? { "st.network": root } : {}; + // Crash-ding targeting tags (read by up.ts crashDingTargets), HARNESS session only — never the ding sidecar + // (else a crash double-dings the same busId): `convoy.tier=cos` marks the CoS (the always-ding backstop); + // `convoy.spawner` records who spawned this agent (its supervisor) so a worker crash pages the parent. + const agentTags: Record = { + ...(spec.role === "chief-of-staff" ? { "convoy.tier": "cos" } : {}), + ...(opts?.spawner ? { "convoy.spawner": opts.spawner } : {}), + }; const env: Record = { ST_AGENT: busId }; if (root) { env["ST_ROOT"] = root; @@ -141,7 +150,7 @@ export function writePtyToml(dir: string, spec: AgentSpec): void { [HARNESS_SESSION_KEY[spec.harness]]: { id: harnessId, command: harnessCommand(spec.harness, specPermissionMode(spec), bootPrompt(spec.role)), - tags: { role: "agent", ...(permanent ? { strategy: "permanent" } : {}), ...stTag }, + tags: { role: "agent", ...(permanent ? { strategy: "permanent" } : {}), ...stTag, ...agentTags }, env: harnessEnv, }, ...(usesDing(spec) @@ -255,7 +264,9 @@ export async function nativeLaunch(spec: AgentSpec): Promise<{ spawned: string[] writeContextFiles(dir, spec); writeHooks(dir); - writePtyToml(dir, spec); + // The spawner = whoever ran `convoy add` (their bus id, from ST_AGENT) — stamped so a crash-ding reaches + // this agent's actual supervisor, not the whole permanent crew. Null when a human spawns it (→ cos-only ding). + writePtyToml(dir, spec, { spawner: process.env["ST_AGENT"] ?? null }); // Create the agent's bus member folder BEFORE spawning `st ding`. The ding watcher errors at startup // if `$ST_ROOT//{inbox,archive}` is missing → the worker is never poked → it parks. `st diff --git a/src/up.test.ts b/src/up.test.ts index 6dcf195..bb356bb 100644 --- a/src/up.test.ts +++ b/src/up.test.ts @@ -6,37 +6,39 @@ const sess = (name: string, tags: Record): SupervisedSession => // Test resolver: read the bus id from a plain "busId" tag (the real one reads ST_AGENT out of the pty.toml). const resolve = (s: SupervisedSession): string | null => s.tags["busId"] ?? null; -describe("crashDingTargets — who gets the crash/flap ding", () => { - it("dings the permanent convoy agents (cos + supervisors); excludes workers, non-agents, and the crasher", () => { - const sessions = [ - sess("cos", { "ptyfile.session": "claude", strategy: "permanent", busId: "cos-claude" }), - sess("sup", { "ptyfile.session": "claude", strategy: "permanent", busId: "sup-claude" }), - sess("wk", { "ptyfile.session": "claude", busId: "wk-claude" }), // worker: not permanent → not an orchestrator - sess("bare", { strategy: "permanent", busId: "bare" }), // not a convoy agent (no ptyfile.session) - sess("crasher", { "ptyfile.session": "claude", strategy: "permanent", busId: "crasher-claude" }), - ]; - expect(crashDingTargets(sessions, "crasher-claude", [], resolve).sort()).toEqual(["cos-claude", "sup-claude"]); - }); - - it("adds --notify ids, dedups them, and still excludes the crasher", () => { - const sessions = [sess("cos", { "ptyfile.session": "claude", strategy: "permanent", busId: "cos-claude" })]; - expect(crashDingTargets(sessions, "crasher", ["extra", "cos-claude", "crasher"], resolve).sort()).toEqual(["cos-claude", "extra"]); - }); - - it("dedups a member's two sessions (harness + ding) to one bus id", () => { - const sessions = [ - sess("cos.claude", { "ptyfile.session": "claude", strategy: "permanent", busId: "cos-claude" }), - sess("cos.ding", { "ptyfile.session": "ding", strategy: "permanent", busId: "cos-claude" }), - ]; - expect(crashDingTargets(sessions, null, [], resolve)).toEqual(["cos-claude"]); - }); - - it("a session whose bus id can't be resolved is skipped (never dings a null/empty target)", () => { - const sessions = [ - sess("cos", { "ptyfile.session": "claude", strategy: "permanent", busId: "cos-claude" }), - sess("mystery", { "ptyfile.session": "claude", strategy: "permanent" }), // no busId → resolve() null - ]; - expect(crashDingTargets(sessions, null, [], resolve)).toEqual(["cos-claude"]); +describe("crashDingTargets — cos + the crashed one's spawner (NOT the whole permanent crew)", () => { + const cos = sess("cos", { "ptyfile.session": "claude", strategy: "permanent", "convoy.tier": "cos", busId: "cos-claude" }); + // Unrelated repo-owner agents — long-lived, so they run --permanent, but they are NOT orchestrators. + const appApple = sess("app-apple", { "ptyfile.session": "claude", strategy: "permanent", busId: "app-apple-claude" }); + const evals = sess("evals", { "ptyfile.session": "claude", strategy: "permanent", busId: "evals-claude" }); + const sup = sess("sup", { "ptyfile.session": "claude", strategy: "permanent", busId: "sup-claude" }); + + it("ACCEPTANCE: a worker crash pages ONLY cos + the worker's spawner — NOT unrelated permanent agents (Nathan's bug)", () => { + const crashed = sess("wk", { "ptyfile.session": "claude", busId: "crashtest", "convoy.spawner": "sup-claude" }); + const targets = crashDingTargets(crashed, [cos, sup, appApple, evals, crashed], [], resolve).sort(); + expect(targets).toEqual(["cos-claude", "sup-claude"]); // app-apple-claude / evals-claude NOT paged + expect(targets).not.toContain("app-apple-claude"); + }); + + it("dings ONLY cos when the crashed worker has no spawner tag (human-spawned → cos backstop only)", () => { + const crashed = sess("wk", { "ptyfile.session": "claude", busId: "crashtest" }); // no convoy.spawner + expect(crashDingTargets(crashed, [cos, appApple, crashed], [], resolve)).toEqual(["cos-claude"]); + }); + + it("dedups when the spawner IS cos (cos spawned it directly) → a single cos ding", () => { + const crashed = sess("wk", { "ptyfile.session": "claude", busId: "crashtest", "convoy.spawner": "cos-claude" }); + expect(crashDingTargets(crashed, [cos, crashed], [], resolve)).toEqual(["cos-claude"]); + }); + + it("adds --notify ids, dedups, and NEVER self-dings the crasher (even if it is the cos-tier)", () => { + // cos itself crashes: excluded from its own ding despite being cos-tier; notify still delivered + deduped. + expect(crashDingTargets(cos, [cos], ["extra", "cos-claude", "cos-claude"], resolve).sort()).toEqual(["extra"]); + }); + + it("skips an unresolvable cos-tier session (never dings a null/empty target)", () => { + const cosNoBus = sess("cos2", { "ptyfile.session": "claude", "convoy.tier": "cos" }); // no busId → resolve() null + const crashed = sess("wk", { "ptyfile.session": "claude", busId: "crashtest", "convoy.spawner": "sup-claude" }); + expect(crashDingTargets(crashed, [cos, cosNoBus, sup, crashed], [], resolve).sort()).toEqual(["cos-claude", "sup-claude"]); }); }); diff --git a/src/up.ts b/src/up.ts index 4ada30f..9adb4eb 100644 --- a/src/up.ts +++ b/src/up.ts @@ -68,24 +68,34 @@ export function workerCrashed(status: SupervisedSession["status"], exitCode: num return status === "vanished" || (status === "exited" && exitCode !== 0); // !== 0 dings nonzero (incl. an agent OOM = 137 via pty ≥ #72) AND a no-record null; only a reaped-grandchild OOM (Case B) escapes — OS-level follow-up } -/** The ORCHESTRATORS to ding when a session crash-loops or gives up: convoy can't read a role off a pty session, - * but permanence is a clean proxy — the CoS + supervisors run `--permanent`, workers don't — so the permanent - * convoy agents ARE the cos+supervisor tier. Plus any explicit `notify` ids. The crashing agent itself is - * excluded (no self-ding). Targets are BUS IDENTITIES (via `resolve`, injectable for tests), deduped. Pure. */ +/** Who to ding when `crashed` crash-loops / vanishes: (a) the CoS — ALWAYS (a network-wide backstop), + * resolved by its `convoy.tier=cos` tag (no hardcoded id); plus (b) `crashed`'s ACTUAL supervisor — the + * `convoy.spawner` bus id recorded on it at `convoy add` (whoever spawned it owns it); plus any explicit + * `notify` ids. NOT every `--permanent` agent: permanence is long-lived-ness, NOT "orchestrator" — every + * repo-owner runs `--permanent`, so the old permanent-set target paged the ENTIRE standing crew on one + * throwaway worker crash (Nathan-flagged). The crashing agent itself is excluded (no self-ding). Targets + * are BUS IDENTITIES (via `resolve`, injectable for tests), deduped. Pure. */ export function crashDingTargets( + crashed: SupervisedSession, sessions: readonly SupervisedSession[], - crasherBusId: string | null, notify: readonly string[], resolve: (s: SupervisedSession) => string | null, ): string[] { const ids = new Set(); + // (a) the CoS — always, resolved by tag so it survives naming/id churn. for (const s of sessions) { - if (s.tags["ptyfile.session"] === undefined) continue; // a convoy agent, not a bare pty session - if (s.tags["strategy"] !== "permanent") continue; // orchestrators are permanent (cos + supervisors) + if (s.tags["convoy.tier"] !== "cos") continue; const bid = resolve(s); - if (bid && bid !== crasherBusId) ids.add(bid); + if (bid) ids.add(bid); } - for (const n of notify) if (n && n !== crasherBusId) ids.add(n); + // (b) the crashed agent's actual supervisor — the spawner recorded at add-time. + const spawner = crashed.tags["convoy.spawner"]; + if (spawner) ids.add(spawner); + // explicit extras. + for (const n of notify) if (n) ids.add(n); + // never self-ding. + const crasherBusId = resolve(crashed); + if (crasherBusId) ids.delete(crasherBusId); return [...ids]; } @@ -176,7 +186,7 @@ export async function up(opts: UpOptions): Promise { const supervised = new Set(); const workerDinged = new Set(); // gone workers aren't respawned → dedup so we ding each once const notify = opts.notify ?? []; - const dingTargets = (sessions: readonly SupervisedSession[], crasherBusId: string | null): string[] => crashDingTargets(sessions, crasherBusId, notify, busIdOf); + const dingTargets = (crashed: SupervisedSession, sessions: readonly SupervisedSession[]): string[] => crashDingTargets(crashed, sessions, notify, busIdOf); const tick = async (): Promise => { const now = new Date(); @@ -185,19 +195,20 @@ export async function up(opts: UpOptions): Promise { if (isPermanent(s)) permanentKeys.add(s.name); const permanent = isPermanent(s) || permanentKeys.has(s.name); - // WORKER-CRASH: a gone NON-permanent convoy agent (a worker). convoy up does NOT respawn it — workers are - // ephemeral (Nomad no-respawn) — but a CRASH is ding-worthy so its supervisor + cos know. Gate on the exit - // (workers have no fast-fail loop, since they're never respawned): a nonzero exitCode or a hard `vanished` - // death dings; a CLEAN exit (code 0 = the worker finished its task) stays SILENT (routine). Dedup: a gone - // worker re-appears every tick, so ding ONCE. Target = cos + all permanent supervisors (no spawner tag - // exists to derive the specific owner yet — a follow-up). - if (!permanent && s.tags["ptyfile.session"] !== undefined) { + // WORKER-CRASH: a gone NON-permanent convoy agent (a worker's HARNESS session — NOT its ding sidecar, + // which would double-ding the same busId). convoy up does NOT respawn it — workers are ephemeral (Nomad + // no-respawn) — but a CRASH is ding-worthy so its supervisor + cos know. Gate on the exit (workers have no + // fast-fail loop, since they're never respawned): a nonzero exitCode or a hard `vanished` death dings; a + // CLEAN exit (code 0 = the worker finished its task) stays SILENT (routine). Dedup: a gone worker re-appears + // every tick, so ding ONCE. Target = cos + the worker's actual supervisor (its `convoy.spawner` tag) — see + // crashDingTargets. + if (!permanent && s.tags["ptyfile.session"] !== undefined && s.tags["ptyfile.session"] !== "ding") { if (!gone(s) || workerDinged.has(s.name)) continue; workerDinged.add(s.name); // mark regardless — a clean-exit worker must not be re-checked either if (!workerCrashed(s.status, s.exitCode)) continue; // routine clean exit (code 0) → silent const id = busIdOf(s) ?? logicalId(s); const reason = s.status === "vanished" ? "vanished (hard death — no exit record)" : s.exitCode === null ? "killed with no exit code (hard kill / OOM)" : `exit ${s.exitCode}`; - const targets = dingTargets(sessions, busIdOf(s)); + const targets = dingTargets(s, sessions); const body = `Worker ${id} CRASHED (${reason}) on network ${root} — it is NOT auto-respawned (workers are ephemeral). NEEDS ATTENTION: its supervisor should decide whether to re-spawn or redirect it. Inspect: pty peek ${s.name}.`; for (const t of targets) await sendDing(root, t, `worker crash: ${id}`, body); emit( @@ -234,17 +245,26 @@ export async function up(opts: UpOptions): Promise { state.set(key, decision.tags); const ok = await host.respawn(s); // pty restart -y — preserves the real command (strips tags) // Re-assert permanence + the counter AFTER the restart (it strips runtime tags) — for pty's - // display + so a fresh host still recognizes this session. convoy's own store is authoritative. - host.setTags(s.name, { ...writtenTags(decision.tags), strategy: "permanent" }); + // display + so a fresh host still recognizes this session. convoy's own store is authoritative. Also + // re-assert the crash-ding targeting tags (convoy.tier/convoy.spawner) from the pre-respawn session, so + // a permanent respawn (e.g. cos restarting) doesn't drop the CoS backstop / a supervisor's spawner link. + host.setTags(s.name, { + ...writtenTags(decision.tags), + strategy: "permanent", + ...(s.tags["convoy.tier"] ? { "convoy.tier": s.tags["convoy.tier"] } : {}), + ...(s.tags["convoy.spawner"] ? { "convoy.spawner": s.tags["convoy.spawner"] } : {}), + }); emit( { type: "respawn", identity: logicalId(s), session: s.name, reason: "exited", attempt: decision.tags.consecutiveFastFails, cap: limit, ok, ts: isoString(now) }, `[convoy-up] respawn ${logicalId(s)} session=${s.name} reason=exited attempt=${decision.tags.consecutiveFastFails}/${limit}${ok ? "" : " (spawn FAILED)"}`, ); // Ding the orchestrators on a fast-fail CRASH (consecutiveFastFails ≥ 1) — the agent died fast + is being - // respawned; gate OUT routine respawns (counter 0 = a normal/slow exit, not a crash) as noise. - if (decision.tags.consecutiveFastFails >= 1) { + // respawned; gate OUT routine respawns (counter 0 = a normal/slow exit, not a crash) as noise. A ding + // SIDECAR is still respawned above, but never generates its own crash-ding (the agent's covers it → no + // double-ding). + if (decision.tags.consecutiveFastFails >= 1 && s.tags["ptyfile.session"] !== "ding") { const id = busIdOf(s) ?? logicalId(s); - const targets = dingTargets(sessions, busIdOf(s)); + const targets = dingTargets(s, sessions); const body = `Agent ${id} CRASHED (fast-fail ${decision.tags.consecutiveFastFails}/${limit}) on network ${root} — convoy up is auto-respawning it. NEEDS ATTENTION if it keeps crashing: pty peek ${s.name} to see why.`; for (const t of targets) await sendDing(root, t, `crash: ${id}`, body); } @@ -257,11 +277,15 @@ export async function up(opts: UpOptions): Promise { `[convoy-up] flapping ${logicalId(s)} session=${s.name} — parked after ${e.counter} fast fails (cap ${e.limit}/${e.window}s). \`pty tag ${s.name} --rm strategy.status\` to retry.`, ); // Ding the orchestrators on a GAVE-UP (flapping) — the strongest signal: convoy up stopped respawning it. - // This fires once (the tick that transitions to flapping; subsequent ticks `skip`), so no ding spam. - const id = busIdOf(s) ?? logicalId(s); - const targets = dingTargets(sessions, busIdOf(s)); - const body = `Agent ${id} GAVE UP — flapping/parked after ${e.counter} fast fails (cap ${e.limit}/${e.window}s) on network ${root}. NEEDS ATTENTION: it is crash-looping and convoy up stopped respawning it. Inspect (pty peek ${s.name}), fix the cause, then clear its strategy.status to retry.`; - for (const t of targets) await sendDing(root, t, `flapping: ${id}`, body); + // This fires once (the tick that transitions to flapping; subsequent ticks `skip`), so no ding spam. A ding + // SIDECAR is excluded (the agent's crash/flap ding covers the incident → no double-ding; independent + // ding-health monitoring is a separate concern). + if (s.tags["ptyfile.session"] !== "ding") { + const id = busIdOf(s) ?? logicalId(s); + const targets = dingTargets(s, sessions); + const body = `Agent ${id} GAVE UP — flapping/parked after ${e.counter} fast fails (cap ${e.limit}/${e.window}s) on network ${root}. NEEDS ATTENTION: it is crash-looping and convoy up stopped respawning it. Inspect (pty peek ${s.name}), fix the cause, then clear its strategy.status to retry.`; + for (const t of targets) await sendDing(root, t, `flapping: ${id}`, body); + } } } };