Repository navigation
fix(opencode): make the notice tracker's dispose terminal - #1540
Merged
Merged
Conversation
`stop()` aborts the controller and leaves it `undefined`, which is exactly the state the watcher starts from, so aborting alone could not express "never again": a `notifyUrl` that outlived `dispose()` re-opened a host event subscription with no owner left to abort it — the leak #1536 added `dispose()` to prevent. Both call sites dispose in a `finally` after the review returns and `toastPlannotatorUrl` never awaits `notifyUrl`, so nothing in the type orders the two. Unreachable on main today (the CLI child's pumps are torn down inside `runCli`'s own `finally`), latent the moment notice delivery outlives the command. Add a `disposed` latch, set only by `dispose` and never by `settle`: the watcher returns early and `admitted()` arms nothing, since after dispose no watcher is left that could lower the flag again. Also close the pre-existing arming window carried over from #1518. `pending` was raised only after the un-awaited `session.synthetic` round-trip, so feedback delivered while the notice was still posting read as not pending and queued behind a notice that is then promoted alone as its own model turn — the #1515 symptom. `watch` becomes `posting`, which starts the watch AND arms provisionally; the notifier wraps `synthetic` so a refusal calls the new `rejected()` and lowers the arm again, which the existing "a rejected notice leaves the feedback queued" test still pins.
1 task
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
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.
Verification QA follow-up on #1536. Two findings on
createNoticePendingTracker, both about its lifecycle rather than its event reading.LOW —
dispose()was not terminalstop()aborts the controller and sets it back toundefined, which is exactlythe state
watch()treats as "not watching yet":So aborting alone cannot express "never again". Both production call sites
dispose in a
finallyAFTER the review returns (native-commands.ts,server.ts) andtoastPlannotatorUrlinvokesnotifyUrlfire-and-forgetwithout awaiting it, so a notice that outlives the command would re-open a fresh
ctx.event.subscribe()on the host with no owner left to abort it — the exactleak
dispose()was added to prevent.Unreachable on main today (the CLI child's stderr pump and ready-file poller are
torn down inside
runCli's ownfinallybeforehandleCliCommandresolves), sothis is latent: a detached ready poller, a notice retry, or any future reuse of
one
V2BridgeClientacross two invocations makes it live.Fix. A
disposedlatch, set only bydispose(never bysettle, whichlegitimately just stops the watch).
posting()returns early when it is set, andadmitted()arms nothing — after dispose there is no watcher left that couldever lower the flag again, which is the stale "still pending" the tracker exists
to rule out.
INFO (pre-existing) — the arm happened after the host round-trip
pendingwas raised only inadmitted(), i.e. AFTERawait synthetic({...}),while
toastPlannotatorUrlnever awaits the returned promise. For the wholeround-trip the flag read false, so feedback delivered in that window would be
admitted with
queueand the notice promoted alone as its own model turn withthe feedback stuck behind it — the #1515 symptom. Carried over unchanged from
#1518; practically unreachable (the notice is posted when the review server
prints its URL, the feedback arrives when the human finishes reviewing), but it
is the one remaining way the co-promotion can silently fail.
It was safe to tighten, so it is tightened:
watchon the tracker is nowposting, which starts the watch AND arms provisionally, and the notifier wrapssyntheticso a refusal calls the newrejected()and lowers the arm again.Both corrections keep the tracker from staying armed on a guess:
admitted()sets the real answer (including thealready-settled case, where it lowers the arm);
rejected()lowers it, which the pre-existing "a rejectednotice leaves the feedback queued" test already pinned and still pins.
Testing
bun test apps/opencode-plugin→ 184 pass / 0 fail (was 181), 3 new cases:a notice posted after dispose opens no new subscription, feedback delivered
while the notice is still posting is co-promoted, and a refused notice lowers
the provisional arm. The first two were confirmed to fail against the
unmodified
v2-client.tsand pass with it.bun run typecheck→ clean.bun run build:opencode→ clean (after the requiredapps/review+build:hookprerequisites in a fresh worktree).No OpenCode server was started; the behaviour is exercised through the existing
createTestEventStreamharness.