Skip to content

fix(shutdown): dispose the goal loop at quit, after sessions and the MCP host stop - #1449

Merged
Juliusolsson05 merged 3 commits into
integration/batch-2026-09-27-pfrom
fix/goal-loop-shutdown-dispose
Sep 27, 2026
Merged

Juliusolsson05 merged 3 commits into
integration/batch-2026-09-27-pfrom
fix/goal-loop-shutdown-dispose

Conversation

@Juliusolsson05

@Juliusolsson05 Juliusolsson05 commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

Fixes #1372.

Plan (first commit): docs/plans/2026-09-27-goal-loop-shutdown-dispose.md.

Problem

The mechanism is recorded in #1341 (CI run 36156560842): a fire-and-forget persist() that outlives its owner's teardown fails. There were 13 persisting loop state failed: ENOENT … rename warnings, with a test's afterEach as the owner. At quit the owner is process exit, which leaves no log line. No production quit that lost loop state has been recorded; this closes that window.

#1371 gave GoalLoopService a dispose() that cancels its timers and drains an in-flight loop-state persist or continuation. The shutdown inventory (installApplicationShutdown) never called it, so a persist started just before quit could be cut off and the loop state lost.

What merges

  • applicationShutdown.ts: a disposeGoalLoop entry in the inventory. It runs in the support-disposal wave with control and caffeinate:
    • after sessions stop, so no turn boundary can start a persist after dispose;
    • after the built-in MCP host stops, so no goal_loop tool call reaches a disposed service.
      Quit waits for its drain.
  • index.ts: startup publishes disposeGoalLoop once the service exists (the disposeControlHost pattern). If startup never got that far, there is nothing to drain.

Tests

  • applicationShutdown.test.ts: the goal loop is not disposed while sessions or the MCP host are still stopping, and quit waits for its drain. Checked at head 8d646502:
    • head: 11/11 pass;
    • the pre-change inventory (applicationShutdown.ts at the merge base) fails it (disposeGoalLoop called 0 times);
    • a mutation that disposes it in the first wave, beside sessions, fails it (called while sessions were still stopping).
      The existing veto test now also covers it (an editor veto disposes nothing). The test models the inventory with deferred services, not a real quit.
  • Gate: npx tsc -b is clean; vitest run src/main/applicationShutdown.test.ts src/main/goalLoop src/renderer/src/features/global-editor passes 14 files / 150 tests at fb3542b5 (the gate's exact paths; the whole global-editor folder is included, not only its before-unload guard).

Review a round 1 (fixed at fb3542b)

dispose() waited only for TRACKED work. The public mutators (goal_loop_start / goal_loop_complete over MCP) awaited persist() without tracking it, so quit could still cut off their write. persist() now tracks every write. Pinned by "dispose waits for a public complete/startLoop whose write is in flight", both red without the tracking.

Overlap

W3's #1430 work also edits src/main/index.ts, in different hunks (resolveRepoRoot / conversation setup). Per B6 (q125), whichever is READY first merges first and they never share a batch. The second merges origin/main after the first lands, reruns tsc plus the startup and shutdown tests, and states that combined check here.

🤖 Generated with Claude Code

@Juliusolsson05 Juliusolsson05 added the type:bug Something works wrong label Sep 27, 2026
Juliusolsson05 and others added 2 commits September 27, 2026 08:46
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…MCP host stop (#1372)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… MCP mutators' (#1372, #1449 review a)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Juliusolsson05

Copy link
Copy Markdown
Owner Author

Review disposition (head fb3542b)

Reviewer Verdict Findings and disposition
a r1 FIX The public MCP mutators' writes were untracked, so dispose did not wait: fixed fail-first at fb3542b (persist() tracks every write)
a r2 MERGE-READY Verified: the round-1 probe now holds dispose, and removing the tracking fails both new tests
b MERGE-READY Minor: the index.ts wiring is untested. Accepted residual (see below)
c MERGE-READY Minor 1: the same index.ts residual. Minor 2: the body's gate count did not reproduce; fixed in the body with the gate's exact paths (150 tests at fb3542b). Note 3: carry() has no dedicated gated-write test; its write goes through the same tracked persist(), which the tests pin

Residual: src/main/index.ts has no composition test for any shutdown-inventory entry, so a mutation that no-ops disposeGoalLoop at the wiring survives (it survives for disposeControl and the rest too). The inventory itself (applicationShutdown.ts) and the service drain are pinned. A composition test for index.ts is out of scope under the freeze.

Merge order (q125): W3's #1430 work also edits src/main/index.ts (different hunks). Whichever is READY first merges first; the second merges main and states the combined check.

@Juliusolsson05
Juliusolsson05 marked this pull request as ready for review September 27, 2026 18:12
@Juliusolsson05
Juliusolsson05 changed the base branch from main to integration/batch-2026-09-27-p September 27, 2026 20:36
@Juliusolsson05
Juliusolsson05 merged commit 4ced4c1 into integration/batch-2026-09-27-p Sep 27, 2026
2 checks passed
@Juliusolsson05
Juliusolsson05 deleted the fix/goal-loop-shutdown-dispose branch September 27, 2026 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

class:C3-silent-failure The app knows it failed and does not say sev:P3 Minor type:bug Something works wrong

Projects

None yet

Development

Successfully merging this pull request may close these issues.

goal-loop: dispose GoalLoopService in the app shutdown inventory (last persist can be lost at quit)

1 participant