refactor(claude): make the pipeline the sole owner of the WarmQuery (handoff)#326561
Draft
TylerLeonhardt wants to merge 4 commits into
Draft
refactor(claude): make the pipeline the sole owner of the WarmQuery (handoff)#326561TylerLeonhardt wants to merge 4 commits into
TylerLeonhardt wants to merge 4 commits into
Conversation
…handoff) Follows up the ownership question on C8: the session built the WarmQuery AND disposed it (the pre-construction abort gate + the ctor-throw catch), so `warm` had two lifecycle touchers. Now the session hands `warm` to the pipeline as its SOLE owner and never async-disposes it again — except the one genuine case where the pipeline constructor throws and no owner ever came into existence. _installPipeline now constructs the pipeline unconditionally, then runs the abort/dispose gate: on a raced abort it disposes the just-built pipeline (whose teardown async-disposes `warm`) instead of disposing the orphan `warm` directly. The pipeline is a cheap object to build-then-discard, and this removes the session's direct `warm` teardown from the common abort path. Scoped deliberately to `warm`: the abortController's dual-touch is NOT cleaned up here because during a rebuild `this._pipeline` is the OLD pipeline while the in-flight build uses the NEW controller, so routing abort through the pipeline would cancel the wrong one — that needs C7's provisional/materialized split. This is a down-payment on C7. No behavior change: the raced-abort paths still dispose `warm` exactly once and reject with CancellationError, now via the pipeline's teardown. Covered by the existing gate tests (dispose-races-materialize; abort-inside-a-rebuild's-startup) with no test changes needed. Suites green: claudeAgent 197, pipeline 13, queue 12; typecheck-client / eslint / valid-layers clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The suite was a coin flip because of two unrelated harness issues (not product bugs):
1. Aborts missed the turn: an enumeration prompt could be shortcut by the model, so
the turn sometimes finished before the abort landed. Replaced it with a blocking
Bash `sleep` tool call the model cannot shortcut — the turn stays in-flight (parked
on approval, or running the sleep) until aborted. `_abort_inflight` also presses
⌘Escape as a stop fallback. Result: scenario 3 + every churn cycle now abort reliably.
2. The `activeClientSet` materialize storm: the automation client's tool-sync can race
the first turn, rebuilding-to-resume before the conversation is written ("No
conversation found"), which poisons the session and pollutes the cumulative log.
Since a poisoned session can't be recovered in place, launch mode now retries the
whole run on a FRESH window (up to 3x), keyed on materialize failing. A real
regression fails every attempt; the flaky storm does not. Also bumps the post-attach
settle to 5s.
Verified: 19/19 green, with scenario 3 and all churn cycles aborting cleanly.
Follows the review question on the handoff change ("can the creation of this object
really throw?"). Traced the full construction chain — ClaudeSdkPipeline → queue,
router → file-edit-observer → edit-tracker — and it is pure field-assignment plus
`createInstance` of the next object on always-registered services (ILogService,
IInstantiationService). No file I/O, parsing, or validation; the only external call
is `WarmQuery.query()`, a synchronous bind we make once on a fresh warm.
So the catch guarded a case that cannot happen in a correctly-wired app, and it was
incomplete even if it had (the orphaned half-built pipeline's already-`_register`'d
queue/router/observer would leak, and the `openDatabase(...)` one line above the
`try` is itself unguarded). Removing it makes the pipeline the TRULY sole owner of
`warm` + `dbRef` — the session no longer touches `warm` on any path.
No behavior change on any path the app or tests exercise (the catch never fired).
Suites green: claudeAgent 197, claudeSdkPipeline 13, claudePromptQueue 12;
typecheck-client / eslint clean. Net -8 lines.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
C9 deleted the rematerializer, but four JSDocs in claudeAgentSession.ts still
described it ("attach the rematerializer", "rematerializer flow", "yield-restart
rematerialize", "on every rematerializer call"). Retarget them to the current
vocabulary (rebuild / session-orchestrated `_installPipeline`). Surfaced while
reviewing the bottom PR's Copilot comments (which flagged this exact class of
stale historical narrative) against the top of the stack. JSDoc-only.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
You flagged on C8 that
warmis created by the session but owned/torn-down by the pipeline — two lifecycle touchers. This makes the pipeline the sole owner of theWarmQuery:_installPipelinenow constructs the pipeline unconditionally, then runs the abort/dispose gate. On a raced abort it disposes the just-built pipeline (whose C8 teardown async-disposeswarm) instead of the session async-disposing the orphanwarm.warmdispose left is the genuine "constructor threw, so no owner ever existed" case.Scope — deliberately just
warmThe abortController's dual-touch is not cleaned up here. During a rebuild,
this._pipelineis still the old pipeline while the in-flight build uses the new controller — so routingsession.abort()throughpipeline.abort()would cancel the wrong controller and leave the rebuild'sstartup()un-cancellable. That needs C7's provisional/materialized split. So the controller stays as-is; this is a down-payment on C7.Why it's safe
No behavior change: the raced-abort paths still dispose
warmexactly once and reject withCancellationError— now routed through the pipeline's teardown. The change is covered by the existing gate tests with no test changes needed:agent.dispose() during a racing first sendMessage … disposes the WarmQueryabort landing inside a rebuild's startup() … the half-built WarmQuery is disposed(C9 edge test)Validation
claudeAgent197,claudeSdkPipeline13,claudePromptQueue12.typecheck-client/eslint/valid-layers-checkclean.<id>.jsonlerrors, session usable), with zero teardown/dispose/orphan errors. (Two runs flaked on unrelated harness-timing issues —activeClientSet-materialize and model-finishing-before-abort — which I'm hardening separately.)🤖 Generated with Claude Code