HOMELAB-1544: fix(subagents): make async dispatch the enforceable default and stop teardown eating sibling results #14
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "plane/HOMELAB-1544-async-only-dispatch"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes HOMELAB-1544.
Problem
Three paths could freeze the orchestrator's session loop on subagent work. All three live in
pi-swarm-suite, not pi core:core/index.tsawait manager.spawnAndWait(...)— no timeoutget_subagent_result wait:truecore/index.tsawait record.promise— unboundedagent_swarmfan-outswarm/swarm-tool.tsawait collector.allTerminal()(unchanged here)The foreground path was the one that bit on 2026-09-13 (3425s, 34 tool uses on a single scout, chat frozen, user killed it and lost two siblings' results): its effective default was hardcoded
falseininvocation-config.ts, so any LLM call that omittedrun_in_backgroundblocked.Changes
defaultRunInBackground(new,subagents.json) — defaults tofalse, so behavior is unchanged until explicitly enabled. Whentrue,Agentreturns an agent id immediately. Precedence stays frontmatter → explicit tool param → setting, sorun_in_background: falseremains an escape hatch for callers that need the result inline.toolWaitCapSec(new, default120) — capsget_subagent_result's wait. On expiry the tool returns the agent's status snapshot, and re-arms the completion notification (resultConsumedreset) so the result is still announced when the agent actually finishes rather than being silently swallowed.runBudgetMinutes(new, default10,0disables) — soft wall-clock budget. The existing 60s manager sweep now also flags agents past budget, once each, via asetBudgetHookcallback. The agent keeps running — the budget makes a runaway visible, it does not kill work.dispose()no longer callsagents.clear()— it discarded every finished agent's result along with the live ones, which is how killing one agent took two completed siblings with it. Only records that could still run are removed.Latent bug fixed in passing
notifyApplied()persistedsnapshotSettings(), which carries only the keys the/agentsmenu manages — so any settings change silently dropped every other knob in the project file (including the pre-existingtaskMaxAttempts). It now merges over the existing project settings, matching what the suite settings page already did.extractCoreSettingsgained matching range checks for the same reason: a value it drops is silently erased on save.All three knobs are surfaced on
/agents → Settingsand the unifiedpi-swarm-suitesettings page.Verification
Baseline on
mainwas 48 files / 389 tests, so the new suite adds 16 cases and nothing regressed.New tests in
tests/core/async-dispatch.test.tscover the four ticket requirements: background-by-default precedence (including both escape-hatch directions), wait-cap clamping, the budget firing exactly once and never stopping the agent, and sibling results surviving bothdispose()andabort(id).Not covered here: scope item 1 asked for an audit of the
agent_swarmsynchronous fan-out. It does block by design (allTerminal()), but it's a synchronous tool whose contract is "run this fan-out and report" — making it async would need a job-id + notification model of its own, so it's left as-is rather than half-converted. Flagging in case that should become its own ticket.Note for review
~/.pi/agent/extensionsis the live extensions directory, and it had 20 uncommitted files (HOMELAB-1534/1542 result-contract work) plus 3 failing tests when this work started. Those are parked onrescue/HOMELAB-1534-1542-inflight(with the failingtiers.test.tsfixed — it still expecteddeepseek/deepseek-v4-flashafter the rename todeepseek-flash) and are not part of this PR. This branch is based cleanly onmain.A subagent could freeze the orchestrator's session loop indefinitely, and killing one discarded its siblings' finished work. Blocking paths found (all in this extension, not pi core): - Agent's foreground branch awaited spawnAndWait() with no timeout (core/index.ts). It was reached whenever an LLM call omitted run_in_background, because the effective default was hardcoded `false`. - get_subagent_result({ wait: true }) awaited record.promise unbounded. - agent_swarm always awaits collector.allTerminal() (unchanged here). Changes: 1. `defaultRunInBackground` (new, subagents.json) — default false, so behavior is unchanged until explicitly enabled. When true, Agent returns an agent id immediately instead of awaiting spawnAndWait(). Precedence stays frontmatter -> explicit tool param -> this setting, so run_in_background: false remains an escape hatch for callers that need the result inline. 2. `toolWaitCapSec` (new, default 120s) — caps get_subagent_result's wait. On expiry the tool returns the agent's current status snapshot and re-arms the completion notification (resultConsumed is reset) so the orchestrator is still told when the agent actually finishes rather than having that notification silently suppressed. 3. `runBudgetMinutes` (new, default 10, 0 disables) — soft wall-clock budget for a running agent. The existing 60s manager sweep now also flags agents past budget, once each, via a setBudgetHook callback wired in core/index.ts. The agent keeps running: the budget makes a runaway visible, it does not kill work. 4. dispose() no longer calls agents.clear(). It dropped every finished agent's result along with the live ones, which is how killing one agent took two completed siblings with it. Only records that could still be running are removed now. Also fixes a latent settings-clobber: notifyApplied() persisted snapshotSettings(), which only carries the keys the /agents menu manages, so any settings change silently dropped every other knob in the project file (including taskMaxAttempts). It now merges over the existing project settings, matching what the suite page already did. All three knobs are surfaced on /agents -> Settings and the unified pi-swarm-suite settings page (extractCoreSettings gained matching range checks, since a value it drops would otherwise be silently erased on save). New tests (tests/core/async-dispatch.test.ts, 16 cases) cover the four ticket requirements: background-by-default precedence, wait-cap clamping, the budget firing once and never stopping the agent, and sibling results surviving both dispose() and abort(id). Verified: tsc --noEmit clean; vitest 49 files / 405 tests passing (was 48/389 on main).Heads-up from HOMELAB-1548 (merged as
142a866):mainmoved while this PR was open, so Forgejo now reports the branch as not mergeable.I ran a merge of
maininto this branch locally to check the damage:pi-swarm-suite/README.md— both branches append rows to the same settings table. Take both sets: keepdefaultRunInBackground/toolWaitCapSec/runBudgetMinutes(this PR), keep thedefaultModelrow and the updatedtier-*rows (main).tsc --noEmitclean,vitest run→ 51 files / 419 tests passing.deepseek/deepseek-flash(HOMELAB-1548). If you carry test fixtures that pin tier model strings (e.g. from therescue/HOMELAB-1534-1542-inflightstash), update those to the new default.Recommended:
git merge forgejo/mainon the branch, resolve the README table as above, and re-run typecheck + tests before merging.Conflict resolved — branch is mergeable again
origin/mainmerged into this branch as7286d36(normal fast-forward push of one merge commit; no force-push, no rebase). Forgejo now reportsMergeable: true.The 2026-09-14 comment (written against HOMELAB-1548) described a single README conflict.
mainhad since moved ~20 merges (HOMELAB-1557 → 1575), so the merge had six conflicts, one of them semantic. Resolutions:pi-swarm-suite/README.md— union as prescribed: the three HOMELAB-1544 rows (defaultRunInBackground,toolWaitCapSec,runBudgetMinutes) plus main'swatchPane/persistChildSessionsrows, then a single tier block on main'sdeepseek/deepseek-flashdefaults. Also dropped main's stray trailing| |on thetier-premiumrow, which broke that row's column count.src/core/settings.ts— union of the interface fields,SettingsApplierssetters,sanitizeSettingsranges andapplySettingsapplier calls. Merge adaptation: the three HOMELAB-1544 getters are decision-time reads, so they now callrefreshSettingsIfStale()like every other HOMELAB-1569 participant — otherwise these would have been the only knobs a second session's write could not reach.src/core/agent-manager.ts— the one semantic conflict:validateResultForLanding/ContractWarning(the merged body callsvalidateResultForLanding, notvalidateResult), keptgetRunBudgetMinutesand the HOMELAB-1558 child-session imports;dispose(): kept the HOMELAB-1544 fix — the targetedrunning/queued-only removal — and did not reinstate main's blanketagents.clear(); drivers.clear(); lastStatus.clear().removeRecord()already deletes each removed record's driver/last-status entries, and a blanket clear would have stripped the labels of exactly the terminal records this PR keeps.OnAgentComplete/OnAgentStarttype aliases (this branch had them concatenated on one line — cosmetic, no behavior).src/core/index.ts/src/suite-settings.ts— union of the settings imports,coreAppliers, the unified-page descriptors and theextractCoreSettingsranges.Test fixtures from main newer than this PR (
tests/core/settings-freshness.test.ts,tests/suite-settings-watchpane.test.ts,tests/suite-settings-persist-child-sessions.test.ts) constructSettingsAppliersliterals, so they gained the three HOMELAB-1544 setters. I deliberately did not make those setters optional to dodge this: they stay required, so a missing wire-up still failstsc.Validation (local, on the merged tree)
CI on
7286d36pr-title✅ ·pi-swarm-suite✅ ·pi-otel-telemetry✅For the reviewer
The
dispose()resolution above is the one judgement call worth a second pair of eyes: this branch previously asserted "no stash/other-branch work is included", and that still holds — the merge only unions both sides of the same shared files. Everything else is mechanical.Fresh-context review — head
7286d36, vsorigin/main(a02fe1f)Reviewed from primary sources only: the PR diff vs current
main, the pre-merge head7994424, the ticket's acceptance criteria, and the repo's own code paths. No implementer reasoning was consulted. Nothing was pushed, committed, or merged.Verification run locally on
7286d36(pi-swarm-suite/):tsc --noEmit→ clean (exit 0).vitest run→ 92 files / 1197 tests passing, 0 failures. CI on7286d36green (pr-title,pi-swarm-suite,pi-otel-telemetry).1. Merge delta (
7994424..7286d36) — CLEANOnly 26 lines are removed across the PR-vs-main diff, and every one is accounted for: the three teardown
clear()calls, two replaced tool-description strings, the two supersededsettings.jsimport lines, the replaceddispose()loop,snapshotSettings()→ the merge-over-project call, the old?? falsebackground default, the old unlock message, and one README row (see below). Nothing frommainwas dropped:agent-manager.ts— took main'svalidateResultForLanding/ContractWarning(the merged body calls them), kept the HOMELAB-1558 child-session imports andgetRunBudgetMinutes.index.ts/suite-settings.ts— union of imports,coreAppliers, unified-page descriptors andextractCoreSettingsranges; the three new setters are deliberately non-optional so a missed wire-up failstsc.README.md— union table keeps main'sdefaultModel,toolDescriptionMode,watchPane,persistChildSessionsand the singledeepseek/deepseek-flashtier block, plus the three HOMELAB-1544 rows. The one deleted README line is main's malformedtier-premiumrow (| |trailing pipe), replaced by a well-formed row. Good catch, not a loss.settings-freshness,suite-settings-watchpane,suite-settings-persist-child-sessions,team-settings) were adapted with the three new setters — mechanical, andtscproves it.2. The
dispose()resolution — CONCERN (keeping it is right; its premise and its shipped test are not)(a) Blanket clear would have stripped kept terminal labels — CONFIRMED.
removeRecord()(agent-manager.ts:1240-1247) is the only placedrivers/lastStatusentries are deleted, and the roster row's label is read throughserializeAgentRecord→driverOf(id)→manager.getDriver()→drivers.get(id), with the field simply omitted when absent (roster-json.ts:93,:121). SoremoveRecord's per-record deletes are the correct discipline, and taking main's blanketdrivers.clear(); lastStatus.clear()alongside kept records would have dropped thedriverlabel off exactly those rows.removeRecord()also runsreleaseChildSession()first, which the blanket clear does not. The resolution itself is correct.(b) But on the only production teardown path the loop is dead code, and dispose() now removes nothing.
manager.dispose()has exactly one caller:disposeSession()(core/index.ts:864), and it always runs immediately aftermanager.abortAll()(:849), which first flips every running/queued record tostatus = "stopped"(agent-manager.ts:1312,:1334). The removal conditionstatus !== "running" && status !== "queued"(:1421) therefore matches nothing, so the loop deletes zero records andremoveRecord()— the only thing that nullsrecord.sessionand frees the maps — never runs.dispose()retains all records and every disposed session object.I confirmed this with a throwaway probe test (mimicking the real
abortAll()→dispose()order, then deleted): after teardown,getRecord(stuckId)still returns the record withsessionfield retainedtrue. The shipped test inasync-dispatch.test.ts("drops only the agents that cannot still run after dispose()") assertsgetRecord(stuckId)isundefined— an orderingdispose()is never called in, and its own comment ("the still-running one was stopped byabortAll()during teardown") describes the path it didn't take. So no test covers the real ordering, and the source comment "only records that could still run are dropped (abortAll() upstream has already stopped them)" is self-defeating: ifabortAll()already stopped them, none can still run, so none are dropped.Not a blocker — the session is ending,
disposeSession()has already nulledcurrentCtx(:838) and unregistered the manager handle (:840), so nothing reads those records afterwards, and retention is bounded by whatever still references the manager. Butdispose()'s teardown contract is now effectively "release nothing". Two small follow-ups would close it: (a) in the retain path, nullrecord.sessionafter disposing it (best of both — the result survives, the session object is released), or (b) if full retention is intended, say so and fix the test to exerciseabortAll()→dispose(). Nit for the same hunk: the new comment atcore/index.ts:847("dispose()below clears the roster") is now stale.3. Ticket
HOMELAB-1544Verify coverage — CONCERN (all four items need a live session; one is not delivered by merging)Read the ticket via
plane_list_items(itsHOMELAB-1544seq-id worked; the 1557/1558/1577 duplicates were avoided as instructed).toolWaitCapSec= 120 — implemented, CLEAN. Default120(settings.ts:580), clamped[5, 3600]in bothsanitizeSettingsandsetToolWaitCapSec(:599-603), and the/agentsmenu refuses< 5. The re-arm is genuinely wired: on expiryrecord.resultConsumed = false(core/index.ts:1757), and the individual-nudge send re-checks that flag at send time (:333, scheduled at:354), so the completion notification still fires. A promise that settles concurrently with the cap is handled too —waitTimedOutalso requiresrecord.status === "running", so a settled agent's result is returned inline. Ticket Verify #2 satisfied.runBudgetMinutes= 10, 0 disables — implemented, CLEAN. Exactly-once per agent viabudgetWarned; the hook only sends a follow-up notification and never touches the run;cleanup()drops the id when a terminal record is reaped so a resumed agent can warn again. Ticket Verify #4's guardrail satisfied.defaultRunInBackgrounddefault =false→ the ticket's headline is NOT delivered by this merge. The ticket is "subagent execution blocks the main session — need async-only dispatch", scope item 2: "Agent always returns immediately (run_in_background default/true)", Verify #1: "main session stays interactive throughout". This PR deliberately ships the mechanism inert (precedence frontmatter → explicit param → setting,invocation-config.ts:48), which is defensible for compat but means merging changes no behavior. The live~/.pi/agent/subagents.jsontoday is{maxConcurrent: 20, defaultModel: "deepseek/deepseek-flash", taskMaxAttempts: 2, taskLeaseTtlSec: 90, watchPane: "off"}— none of the three new knobs present. So post-merge someone must set"defaultRunInBackground": true(hand-edit,/agents → Settings, or the unified page; the getters read at decision time viarefreshSettingsIfStale(), so no restart is needed). Merging is not the same as fixing the ticket — and all four Verify items are runtime checks on a live session, none of which this PR or its tests perform. Also:agent_swarm's synchronous fan-out is explicitly left blocking, with no follow-up ticket — it is a real remaining block path named in the ticket's scope item 1.Bonus fix is real and worth keeping:
notifyAppliedused to persistsnapshotSettings()(the menu's subset), silently dropping every other knob in the file — which is precisely howdefaultRunInBackgroundwould have appeared to reset itself.{...loadProjectSettings(process.cwd()), ...snapshotSettings()}(core/index.ts:2674) fixes it, andextractCoreSettingsgained matching ranges so a dropped value can't be erased on save.4. README settings table (union) — CLEAN
Row-by-row against the code:
defaultRunInBackgroundboolfalse✓;toolWaitCapSecint 5–3600 default120✓ (matchesTOOL_WAIT_CAP_SEC_FLOOR/CEILING);runBudgetMinutesint ≥0, 0 = off, default10✓. Main's rows are intact and the single tier block is correct. The new prose section and thesuite-settings.tspage description mirror the same numbers and honestly state that the agent keeps running under budget. Nit: therunBudgetMinutescode ceiling (1440,settings.ts:201) is not documented, unliketoolWaitCapSec's range.5. Tests — CLEAN (with the one gap in 2b)
tsc --noEmitclean; 92 files / 1197 tests pass on7286d36(baselinemainis smaller; the PR's 16 new cases inasync-dispatch.test.tsassert behavior — precedence both directions, wait-cap clamping, budget firing once and never stopping the agent, sibling survival — not implementation). Green CI confirms it independently. Thedispose()case is the only place a test asserts a behavior the production ordering cannot produce.Verdict
mainlost; all six conflicts resolved as uniondispose()semanticsdispose()is never called indefaultRunInBackground/toolWaitCapSec/runBudgetMinutesimplementationagent_swarmstill blocksNo blocker findings. The merge resolution is safe to merge as-is; the
dispose()retention is a follow-up worth a small commit (nullrecord.sessionon the retain path, and a test that exercisesabortAll()→dispose()), and the human should treat the enablement decision ("defaultRunInBackground": true) as the actual close of HOMELAB-1544.Review concern [2b] addressed in
53f8392: dispose()'s retain path now nullsrecord.sessionafter disposing it, and the shipped dispose test was replaced with one exercising the realabortAll()→dispose()order (terminal records survive with labels intact + session released) — typecheck clean, vitest 92 files / 1197 tests green.Merging per Aaron's approval 2026-09-19.
Pre-flight verified live:
53f8392d8c868293722881c39ae5a6be6b537fd4, basemain, mergeable: true, unmerged.mainHEAD (a02fe1f) — merge-base == main, zero commits on main since, so no regression risk to the ~18 PRs merged after this one was opened.Branch
plane/HOMELAB-1544-async-only-dispatchis intentionally left in place for now (not deleted at merge).