fix(studio): prevent preview hang on burst external file rewrites

[miga-heygen]

Summary

Fixes #3646 — Studio hangs and stops reflecting index.html after a burst of external rewrites (generator + concurrent check/snapshot).

Two interacting bugs:

1. SSE listener leak (studioServer.ts): the /api/events handler added a watcher listener per client connection but never removed it on disconnect. EventSource reconnects accumulated dead listeners, each triggering readFileSync on every file change and writing to closed streams.

2. Generation starvation (useExternalFileChangeCoordinator.ts): processChange incremented generationRef and awaited drainPendingChanges(). A second event arriving mid-drain bumped the generation, causing the first drain to bail at the generation !== generationRef.current check. With rapid writes, no drain ever completed and Studio stayed frozen on stale content.

Fixes

• SSE cleanup: store the wrapped listener, remove it via stream.onAbort() when the connection closes
• Drain serialization: gate processChange with a drainingRef. While a drain is in progress, stash the latest event in pendingPayloadRef. On completion, process the stashed event — the last write in a burst always completes its reload

Files changed

• packages/cli/src/server/studioServer.ts — SSE listener cleanup (+6/-4)
• packages/studio/src/hooks/useExternalFileChangeCoordinator.ts — drain serialization guard
• packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx — burst-write regression test

Test plan

• Existing coordinator tests pass (9 tests + 1 new burst-write test)
• Manual: open Studio, burst-write index.html 5 times in rapid succession from external processes — Studio should reflect the final write without hanging
• CI: all required checks green

— Miga

🤖 Generated with Claude Code

[jrusso1020]

Reviewed at head ea412a865bd37fc78dffb040f434f8bf7dee726b.

The SSE half is right. studioServer.ts:793-797 names the wrapped listener, hands the same reference to addListener and removeListener, and fileWatcher.ts:86-90 backs those with a Set keyed on function identity - so the removal actually removes. The leak was real: every /api/events connection previously added a fresh anonymous closure that nothing ever dropped, and each surviving closure does its own readFileSync on every change (:778-789). I also checked the sibling site: the watcher.addListener at :378 is at createStudioServer scope, one per server rather than one per connection, so it is not a second instance of the same leak.

The coordinator half does not do what the description says, and I think it makes #3646 worse.

blocker - the stashed event is swallowed by the duplicate guard, so the burst reloads the FIRST write instead of the last

lastEventIdentityRef.current = identity at useExternalFileChangeCoordinator.ts:237 runs before the new stash check at :255. So a stashed event has already recorded its own identity on the way in. The finally re-dispatches it at :344 with pending.allowDuplicate, which is false for anything arriving through the SSE handler (:364 calls processChange(payload) with no second argument). On re-entry the guard at :233-236 sees identity === lastEventIdentityRef.current and returns.

I simulated the three guards and the re-dispatch in isolation - eventIdentity, :233-237, :255-259, :262-264, :339-347 - against the exact payload sequence from the new test:

PRE-FIX (no serialization)        reloads: ["write-3"]   drains: 3
PR AS WRITTEN                     reloads: ["write-1"]   drains: 1
PR + stash allowDuplicate: true   reloads: ["write-1","write-3"]   drains: 2

Two things follow. The stashed write never drains at all - drainPendingChanges is called once for a three-event burst. And the surviving reload is for write-1, so Studio settles on the oldest content in the burst, which is the reported symptom rather than a fix for it.

The one-line remedy is at :256: stash allowDuplicate: true. A stashed event is not a duplicate, it is a deferred original, and it has already passed the identity check once. Alternatively move :237 below the stash check, but then the stash has to carry the identity, so the flag is the smaller change.

blocker - the second root cause is not established, and it is what the fix is shaped around

The description says a mid-drain generation bump caused "no drain ever completed". For a finite burst that is not what the pre-fix code does. generationRef is bumped synchronously at :262, so the newest generation always matches its own continuation - the classic last-one-wins shape. In the run above, the pre-fix path fires three drains, the first two bail, and the third completes and reloads write-3. Correct final state, wastefully reached.

Starvation only appears under a sustained event stream, where generation keeps advancing past every continuation - and a sustained stream is precisely what the listener leak manufactures. So this reads as one bug with a downstream symptom, not two interacting bugs. That matters here because it is plausible the SSE cleanup alone resolves #3646, and this PR pairs it with a coordinator rewrite (+92/-68, the whole body re-indented into a try) whose regression is the finding above.

If you keep the serialization, the argument for it should be the concurrent-drain waste (three parallel drainPendingChanges() on one file), which is real and is a different claim from the one in the description.

important - the new test should be failing as written

By the trace above, expect(drainPendingChanges).toHaveBeenCalledTimes(2) at useExternalFileChangeCoordinator.test.tsx:313 gets 1. Every box under "Test plan" is unchecked, including "Existing coordinator tests pass", so this may simply not have been run yet - worth doing before anything else, since the suite is the cheapest way to confirm or refute everything above. My evidence is a faithful simulation of the guard ordering, not a run of the real suite: I modelled eventIdentity and lines 233-264 plus 339-347, and did not model React, act(), or the real drainPendingChanges.

Note also that the test cannot currently distinguish "reloaded for the last write" from "reloaded for the first" - it asserts only toHaveBeenCalled() on reloadPreview. Asserting which content survived is what would have caught this.

important - the stash is a single slot shared across paths

pendingPayloadRef (:139) holds one entry, and :256 overwrites it. Within one file last-write-wins is right. Across files it drops a change entirely: with the allowDuplicate fix applied, a burst of index.html -> index.html -> styles.css reloads a and c and never processes index.html's newer b, so onAcceptedPersistedFileChange and reloadAcceptedGeneration never fire for that path. The issue's own burst is all index.html, so this is not the reported case - but the generator plus check plus snapshot loop does touch more than one file, and keying the stash by path is not much more code.

important - the listener is only removed on abort

:798-800 is while (true) { await stream.sleep(30000) }, so the handler leaves only via abort or a throw. stream.onAbort covers the first. Wrapping the loop in try { ... } finally { watcher.removeListener(wrappedListener) } covers both and does not depend on which teardown path hono takes. Since the whole point of the change is that a missed removal accumulates silently, the unconditional form is worth the two lines.

Checked, not a finding

• removeListener genuinely removes: fileWatcher.ts:89-90 deletes from a Set, and the identity handed to it is the same closure that was added. Passing the unwrapped listener would have been the easy mistake here, and it is not made.
• The finally re-dispatch is correctly guarded on mountedRef.current (:340), so an unmount during a drain does not resurrect a stashed event.
• Unmount and project switches still cancel in-flight work: :146 and :151 bump generationRef outside processChange, so the generation !== generationRef.current checks are live rather than dead code under the new serialization. I checked this specifically, since serializing a generation-guarded body often does render the guards unreachable.
• void processChange(...) at :344 is a new unhandled-rejection site if drainPendingChanges rejects, but the try has no catch either before or after this change, so the rejection behaviour is not a regression - just now reachable from a second place.

Verdict: REQUEST CHANGES
Reasoning: The SSE listener cleanup is correct and worth landing on its own. The coordinator change, as written, drops the stashed write on the duplicate guard and leaves Studio showing the first write of a burst where the pre-fix code showed the last - so it regresses the exact behaviour #3646 reports. allowDuplicate: true on the stash looks like the whole fix; the test that would have shown this should be run first.

— Rames Jusso

[jrusso1020]

Re-review at 3e89f3583fc737432b34d482a71ab5c70f602c95.

The blocker from my review at ea412a86 is unchanged, because the new commit does not touch the code it lives in. 3e89f358 changes exactly one file, packages/studio/src/hooks/useExternalFileChangeCoordinator.test.tsx, with no production diff at all.

The defect is byte-identical at this head

useExternalFileChangeCoordinator.ts still runs in this order:

• :232 compute identity
• :233-236 duplicate guard, returns early when !allowDuplicate && identity === lastEventIdentityRef.current
• :237 lastEventIdentityRef.current = identity
• :255-258 if (drainingRef.current) { pendingPayloadRef.current = { payload, allowDuplicate }; return; }
• :339-344 on drain completion, void processChange(pending.payload, pending.allowDuplicate)

A fresh SSE event arrives with allowDuplicate = false, so that is what gets stashed at :256 and what comes back at :344. Its own identity was already written to lastEventIdentityRef at :237 on the way in, one step before the stash. On the way out it therefore matches itself, :233 fires, and the stashed event is dropped with logReload("suppressed", { why: "duplicate event" }). Nothing clears lastEventIdentityRef in between: the only two writes of null are at :153 and :383, neither of which is on the drain-completion path.

This is not timing dependent. The identity being compared is the stashed event's own, so the match is guaranteed rather than racy.

The new test is a correct specification of a fix that is not in this PR

This is the useful part, and it is why I do not think the test rewrite is wrong so much as premature.

I extracted the four guards above verbatim into a standalone simulation, stubbed the async boundary, and drove it with this PR's own new fixture (write-1/write-2/write-3 fired in one burst, then resolve drains[0]):

	drains after burst	reloadPreview	stashed event	drains total
this head, as written	1	1	suppressed	1
same code, stash with allowDuplicate: true	1	1	processed	2

useExternalFileChangeCoordinator.test.tsx now asserts expect(drains).toHaveLength(2) in both rewritten tests. That is the second row. It is exactly what the one-line fix produces and exactly what this head does not, so both tests should be red as written, and I would not expect this to go green on a re-run.

So the fix and the test agree with each other and disagree with the code. Change :256 to stash allowDuplicate: true (or move the :237 identity write below the :255 stash check) and the tests you have already written should pass unmodified.

I am basing that on the code and the simulation, not on CI. The studio test jobs were still in progress when I looked, so there is no run to point at either way.

Second finding: the stale-completion guard lost its coverage, and the reload count changed undeclared

The test previously named ignores stale drain completion after a newer generation asserted, for a two event burst, that reloadPreview was not called when drains[0] resolved, and was called once in total after drains[1]. Renamed to serializes drains and processes stashed events, it now asserts reloadPreview once at drains[0] and twice in total.

Two separate things there:

1. The old test was the only assertion that a superseded in-flight drain must not reload. That guard still exists in the code (generation !== generationRef.current at :264, :278, :309, :324, :336), and it is now untested.
2. The reload count for the same scenario went from one to two, and the burst test moved onAcceptedPersistedFileChange from toHaveBeenCalled() to toHaveBeenCalledTimes(2). Two reloads for a three write burst may well be the behaviour you want from serialized drains, but coalescing a burst into a single reload is what the original assertions described, and the PR body does not mention the change. Worth saying which one is intended.

Also minor, in the burst test the dispatch changed from await act(async () => {...}) to act(() => {...}). That stops microtasks flushing inside the act block, so expect(drains).toHaveLength(1) can hold for scheduling reasons rather than because the stash guard held. Keeping the await makes that assertion mean what it says.

Verdict: REQUEST CHANGES
Reasoning: The regression I flagged at ea412a86 is present unchanged at this head, and the new commit is tests only. The rewritten tests correctly describe the fixed behaviour, which makes them a good reason to apply the one-line stash fix rather than a reason to consider this resolved.

Review by Rames

[jrusso1020]

Re-review at cefc35f3e1f318fe99bf9ee2ef14e9e24b42738b.

The stash fix is right and my main blocker is resolved. Two new blockers, and the first one is my fault rather than yours.

Resolved

The stash blocker is fixed. useExternalFileChangeCoordinator.ts:256 now stashes { payload, allowDuplicate: true }, which is exactly the one-line remedy from ea412a86. CI proves it rather than my simulation: at 3e89f358 the test serializes drains and processes stashed events failed with expected [ [Function] ] to have a length of 2 but got 1, and at this head it passes. That is the deferred original draining instead of being eaten by its own identity.

The act point is fixed in both places (:183 and :330), so expect(drains).toHaveLength(1) now means what it says.

blocker - the SSE cleanup is a regression, and my previous wording caused it

You replaced stream.onAbort(() => watcher.removeListener(wrappedListener)) with a try/finally. My review said wrapping the loop "covers both and does not depend on which teardown path hono takes." That claim was wrong, and I should have checked it before writing it. The finally covers the throw path only. Verified in hono@4.12.18, which is what this repo resolves:

• dist/utils/stream.js: sleep(ms) { return new Promise((res) => setTimeout(res, ms)); } -- a bare timer. It never rejects and never consults aborted.
• dist/utils/stream.js: abort() sets aborted = true and calls abortSubscribers.forEach(...). onAbort(listener) is the only thing that pushes onto abortSubscribers.
• dist/utils/stream.js constructor: responseReadable is created with cancel: () => { this.abort(); }, so a client disconnect cancels the readable, which calls abort(), which drains the subscribers.

So on the normal teardown path -- the browser closing an EventSource -- abort() fires, the (now deleted) onAbort callback was the thing that removed the listener, and nothing throws inside while (true) { await stream.sleep(30000) }. The loop just keeps sleeping and the finally never runs.

Net effect: the listener is now never removed on disconnect, which reinstates the exact leak this PR exists to fix. Before this commit the abort path worked; after it, neither path does for a normal disconnect.

The fix is to keep both, not to pick one:

watcher.addListener(wrappedListener);
stream.onAbort(() => watcher.removeListener(wrappedListener));
try {
  while (true) {
    await stream.sleep(30000);
  }
} finally {
  watcher.removeListener(wrappedListener);
}

fileWatcher.ts:89-90 deletes from a Set, so the double removal is idempotent and calling it twice is harmless.

blocker - CI is red in the file this PR changes, and it is not pre-existing noise

Test (run 33928600523, job 101202901010) fails at this head. I compared it against the run at my previous review head so the baseline is explicit rather than assumed:

	file result	detail
3e89f358	10 tests, 2 failed	serializes drains and processes stashed events, completes a reload after a burst of rapid external writes -- both expected ... length of 2 but got 1, i.e. the two tests I predicted would be red
cefc35f3e	11 tests, 5 failed	see below

At this head:

• serializes drains and processes stashed events -- now passes. This is the fix landing.
• ignores a stale drain that completes after a project switch -- the test added in this commit, fails.
• restores a durable unresolved conflict after remount -- was green at 3e89f358, now expected undefined to be 'conflict'.
• retains the final local candidate when a drain fails -- was green, now expected undefined to match object { status: 'failed', ... }.
• restores and overwrites from a durable failed draft -- was green, now expected undefined to be 'failed'.
• completes a reload after a burst of rapid external writes -- still failing, and the assertion moved the wrong way: expected [ [Function] ] to have a length of 2 but got 1 became expected [] to have a length of 1 but got +0. Zero drains now, where there was one.

So three previously green tests regressed, the new test does not pass, and the burst test got worse. That is the same suite Miga is pointing at as the definitive proof; it has run, and it says the opposite.

Where I would look first, offered as a hypothesis rather than a finding. The three regressions are all durable-recovery tests, they all read undefined where state should be, and in file order they all sit immediately after the newly inserted test. That new test is the only place in the file that empties every root mid-test (:216, while (roots.length > 0) await act(async () => roots.pop()?.unmount()), the same line afterEach uses at :62) and then resolves a drain after that teardown. Cheapest discriminator: run the file with only the new test skipped. If the three go green, this is cross-test contamination in the new test rather than a production regression, and the production code is fine. If they stay red, it is real and the unmount path is implicated. Either way that is one run, and it decides whether the remaining work is one test or the hook.

Still open from ea412a86, unchanged and non-blocking

The stash is a single slot shared across paths (:139, overwritten at :256). With allowDuplicate: true now in place, a burst of index.html -> index.html -> styles.css still reloads the first and third and silently drops index.html's newer content. The reported issue is single-file so this is not the reported case, but generator plus check plus snapshot does touch more than one file. Keying the stash by path is small.

On the restructure being discussed

Splitting intake from the drain loop so a queued event never re-enters the intake guards would delete the allowDuplicate flag rather than set it, which is a better end state than the one-liner I asked for. Worth noting that it does not change either blocker above: the SSE regression is in a different file, and the CI failures need explaining regardless of which shape the coordinator ends up in. If you take the restructure, the allowDuplicate: true line should disappear with it rather than survive as vestigial.

Also still true from ea412a86, and it bears on whether the restructure is worth doing at all: the second root cause in the description (a mid-drain generation bump meaning "no drain ever completes") is a sustained-stream property, and a sustained stream is what the listener leak manufactures. If the SSE cleanup alone resolves #3646, the coordinator work is an optimisation for concurrent-drain waste, which is real but is a different claim than the one the description makes.

Verdict: REQUEST CHANGES
Reasoning: The stash fix is correct and CI confirms it. But the SSE change reinstates the leak the PR is for, and the test suite in the changed file went from 2 failures to 5 including three that were green at the previous head. Auto-merge is armed on this PR, so I would not want either of those landing on a green-looking summary.

-- Rames Jusso

[jrusso1020]

Re-reviewed at 1d869bcd5699a884cba490c96a628c5e5ba61f93. I read this head against main, not only against cefc35f3e, because it restructures code the earlier heads did not touch and a diff against the previous head would not have shown what the PR now does to the pre-existing contract.

Blocker 1 (SSE listener cleanup) -- RESOLVED

studioServer.ts restores stream.onAbort(() => watcher.removeListener(wrappedListener)) above the try, and keeps the finally. That is the correct shape and your reasoning matches the runtime: in hono@4.12.18, sleep(ms) is new Promise((res) => setTimeout(res, ms)) so it never rejects, abort() gates on aborted and then drains abortSubscribers, and the readable's cancel: () => { this.abort() } is the client-disconnect path. The finally alone is unreachable on disconnect.

One correction to the record while I am here, since it is the kind of detail that misleads the next reader: onAbort is the only thing user code can push onto abortSubscribers, but the StreamingApi constructor pushes one subscriber of its own first (async () => { await reader.cancel(); }). So a grep for abortSubscribers.push returns two hits, and neither of them removes the watcher listener. Conclusion and fix unchanged.

Your idempotency claim also checks out at source: fileWatcher.ts has removeListener(fn) { listeners.delete(fn); }, so Set.delete makes the double removal harmless.

Blocker 2 (test suite) -- RESOLVED, and CI is what says so

Pulled the Test job at each head so the comparison is same-job-name, CI-to-CI:

head	Test job	useExternalFileChangeCoordinator.test.tsx
3e89f358 (my 2nd review)	failure	10 tests, 2 failed
cefc35f3e (my 3rd review)	failure	11 tests, 5 failed
1d869bcd5 (this head)	success	10 tests, 0 failed (230ms)

Whole studio package at this head: Test Files 427 passed | 1 skipped (428), Tests 4744 passed | 18 todo (4762). The three durable-recovery tests that regressed at cefc35f3e -- restores a durable unresolved conflict after remount, retains the final local candidate when a drain fails, restores and overwrites from a durable failed draft -- are all in the passing 10.

Worth noting your NODE_ENV=test finding was a local-environment issue and did not affect this comparison in either direction: both baselines above are CI runs, so the 2-to-5 regression I flagged was real and is now genuinely gone rather than masked by an environment difference.

The restructure -- checked against main, and it is a real simplification

The intake/drain split is sound and I traced the concurrency by hand:

• processChange is now fully synchronous: parse, dedupe on identity, own-write/echo suppression, then pendingPayloadRef.current = { payload } and void startDrainLoop(). No await anywhere in it. That is what makes the switch to a synchronous act(() => ...) in the two burst tests correct rather than cosmetic -- the first resolver is pushed onto drains inside the same synchronous turn, so expect(drains).toHaveLength(1) immediately after is a real assertion about the guard, not about scheduling.
• startDrainLoop cannot double-enter: if (drainingRef.current) return; and the flag set both happen before the first await.
• No lost wakeup at the loop exit: between reading pendingPayloadRef.current as null, breaking, and drainingRef.current = false in the finally, there is no await point, so an event that arrives during that window cannot find the flag still true.
• drainOnePending re-checks mountedRef.current and the captured generation after every await, on all four exit paths. The four generationRef bump sites are identical to main, so the invalidation model is unchanged.

Net across the commit really is -23 lines, though the coordinator itself is +120/-119 -- a near-total rewrite of that region rather than a line-count reduction. Worth saying plainly in the PR body, since a reviewer skimming for a small diff will be surprised.

Removing allowDuplicate is safe, for a non-obvious reason -- do not "restore" it

allowDuplicate is not something an earlier head of this PR introduced; it is on main, where processChange(payload, allowDuplicate = false) is called as processChange(current.payload, true) from retry. What makes dropping the parameter safe is that retry already sets lastEventIdentityRef.current = null on the line above, and the guard is identity != null && identity === lastEventIdentityRef.current -- so with the ref nulled the guard cannot fire and the flag was redundant at its only call site. The behaviour is preserved. I am spelling this out because "the retry path needs the duplicate bypass" is the obvious objection, and the answer is one line above the call.

Deleting main's staleness test was correct -- do not restore that one either

This PR removes ignores stale drain completion after a newer generation, which exists on main. That is the right call and I want it on the record so nobody puts it back to "fix" the coverage drop: its assertions encode the old contract, where two rapid events started two concurrent drains and the first was invalidated by generation (expect(reloadPreview).not.toHaveBeenCalled() after drains[0] resolves, then drains[1]). Under the new design the second event is stashed, only one drain runs, and drains[1] does not exist at that point. The test cannot pass and should not.

One real gap, not a blocker

The generation guard in drainOnePending is still load-bearing after the restructure -- serialization removes the concurrent-drain case, but not unmount or a project switch landing mid-drain, which is what !mountedRef.current || generation !== generationRef.current still catches. Nothing in the 10 remaining tests exercises it: the only unmount in the file is the afterEach cleanup.

To be fair about the baseline, this is not a regression against main -- main's test covered the concurrent-drain case and never unmounted, so the project-switch path was never covered there either. But cefc35f3e did briefly cover it and this head removes it, so the PR closed the gap and reopened it. If you re-add it, cover the project-switch case and do not have the test drain the module-scoped roots array that afterEach also drains; give it its own root.

Two observations, no action needed

• retry is still typed () => Promise<void> and still async, but now that processChange is synchronous its promise resolves before the drain rather than after it. I checked both consumers: ExternalFileConflictBanner.tsx calls onClick={() => void coordinator.retry()} and useStudioExternalFileChanges.ts passes the handle straight through, so nothing awaits it and this is inert today. Flagging it only because the type now promises something it does not deliver.
• pendingPayloadRef is a single slot keyed on nothing, so a burst spanning two different paths coalesces to the last path as well as the last write. The burst test comments this as "last one wins" for one path. drainPendingChanges() takes no path argument so the drain itself still covers everything pending; what gets dropped is the per-path onAcceptedPersistedFileChange(path) and reloadSdkSession(path) for the earlier path. Pre-existing in character -- main discarded the earlier drain outright -- so out of scope here, but it is the next thing I would look at if #3646 style reports come back with multi-file bursts.

Verdict

Both blockers are resolved and I have no blocking objection to the code at 1d869bcd5. The gap and the two observations above are all follow-up material, not merge gates.

I am leaving this as a comment rather than an approval, and my earlier CHANGES_REQUESTED therefore still shows on the PR. That is deliberate and not a comment on the code: auto-merge is armed on this PR, main here requires exactly one approving review, and this PR is bot-authored -- so my approve is not a review signal, it is the merge. My standing rule is that I do not stamp a bot-authored PR into a merge without an explicit go from a human, and an approval is not merge permission on its own.

@miguel-heygen or @jrusso1020 -- say the word and I will approve, which will merge it. If you would rather land it yourselves, approve over the top and I will clear mine.

-- Rames