fix(dev-server): release watchers and reload subscriptions when the bind fails by kojiwakayama · Pull Request #3588 · veryfront/veryfront-code

kojiwakayama · 2026-08-11T12:59:11Z

The defect

DevServer.start() registers four things before it binds the HTTP port:

Registration	`server.ts` (pre-fix)
`setupFileWatchers()`	:164
`ReloadNotifier.subscribeInvalidate`	:167
`HMRHandler.registerExternalBroadcastSource()`	:177
`ReloadNotifier.subscribe`	:178
`adapter.serve()` — the bind	:258

All four are released only by stop(). But startDevServer() (index.ts:15-20) constructs the instance, awaits start(), and then returns it — so when start() rejects, the half-built instance is dropped with no handle and no caller can ever call stop(). All four registrations survive for the life of the process.

The leaked broadcast source is the worst of the three kinds: it decrements a process-wide counter, and while it is held HMRHandler skips its own broadcast (hmr.handler.ts:72-78). So one failed start can silently disable HMR for the dev server that starts next.

Reachable from the probe-then-bind race in veryfront dev, and from any adapter.serve() failure that is not a port collision (bad bind address, permission denied on a privileged port).

The fix

start() routes failures through stop() and rethrows.

Chosen over catching in startDevServer() because:

stop() is already the single teardown path, and already null-safe at every step (?.(), if (this.fileWatchSetup), if (this.server)) — it was written to tolerate a partially-built instance. Reusing it means there is no second cleanup list that can drift out of sync when someone adds a new registration to start().
It covers every caller. DevServer is exported directly and constructed outside startDevServer() (e.g. tests/integration/server/dev-server-handlers.test.ts:28); a fix in startDevServer() would leave those leaking.
It matches existing convention. bootstrap.ts (orchestrateOrDisposeFS, and the comment at :482-484) and the Deno http-server.ts adapter both release-then-rethrow at the registration site.

Cleanup errors are swallowed to a debug log so they can never mask the real reason start() failed — "port already in use" is what the developer needs to see.

stop() now clears all three release handles up front, before any is invoked, and runs each through a small release() helper. This is required, not cosmetic: the reload/invalidate unsubscribes are idempotent (Set.delete), but releaseExternalBroadcastSource is not — it decrements a global counter, so the double-stop() this change introduces (failed start() cleans up, caller still holds the instance and calls stop()) would otherwise corrupt an unrelated dev server's count. Clearing up front rather than per-call means a throwing release cannot leave a later handle set for a second stop() to re-invoke.

Regression test

src/server/dev-server/server-start-failure.integration.test.ts forces a real bind failure — another listener holds the port — after the subscriptions are registered, and asserts the registrations are released. It does not settle for asserting that start() rejects; the rejection already happened before this change.

Watchers are counted by wrapping the runtime registry adapter's fs.watch, which is the only seam that observes the watcher DevServer really opens (it builds its own adapter internally). The test asserts watchers.opened > 0 first, so it cannot pass vacuously if that seam ever stops being the one used.

Failing before the fix — all three leaks in one diff:

    {
-     invalidateListeners: 1,
-     openWatchers: 1,
-     reloadListeners: 1,
+     invalidateListeners: 0,
+     openWatchers: 0,
+     reloadListeners: 0,
    }

Passing after, with no sanitizer opt-outs — Deno's own op/resource sanitizers confirm the teardown is complete.

It is colocated with the module it covers per AGENTS.md, and named *.integration.test.ts because it needs a real project directory, a full bootstrapDev(), a real OS file watcher and a real TCP bind. That suffix is excluded from the unit shard by deno.json and follows src/proxy/routing-invalidation.integration.test.ts; it still runs in the coverage shards and the pre-push suite.

Verification

New test fails before the fix and passes after — re-confirmed after relocating it
src/server/dev-server/ + tests/integration/server/: 52 passed, 0 failed
deno fmt --check, deno lint, deno check clean
Pre-push gate passed; all 27 CI checks green

Notes

docs/api-reference/veryfront/server.md carries a one-line source-link update: the teardown helper shifted export class DevServer from L57 to L70. Regenerated with deno task docs under the Deno version CI pins (2.7.7), not hand-edited.
The known flake src/transforms/esm/http-cache.test.ts ("returns a signal-less cache follower after its bounded wait", from fix: allow cold remote modules to finish fetching #3553) fired once on an earlier push and presented as three red checks (its coverage shard plus the dependent tests (unit) and coverage gate). Re-run without changes and it went green; it is untouched by this PR.
Out of scope, noted while working: when start() fails, this.ready never settles, so anything awaiting it hangs. Pre-existing and independent of this leak — left alone deliberately.

Surfaced by CodeRabbit on #3562 and declined there as out of scope, since that PR touched only cli/.

🤖 Generated with Claude Code

coderabbitai · 2026-08-11T12:59:46Z

📝 Walkthrough

Walkthrough

DevServer.start() now cleans up watchers and notifier subscriptions when startup fails. stop() makes teardown callbacks single-use. An integration test verifies cleanup when HTTP port binding fails with HMR enabled.

Changes

Dev server startup cleanup

Layer / File(s)	Summary
Startup failure lifecycle and teardown `src/server/dev-server/server.ts`, `tests/integration/server/dev-server-start-failure.test.ts`	`start()` delegates to `startAndBind()`, calls `stop()` after startup failure, logs cleanup errors, and rethrows the original error. `release()` guards teardown callbacks. `stop()` clears unsubscribe callbacks before invoking them. The integration test tracks watchers and notifier registrations during a port-binding failure.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DevServer
  participant startAndBind
  participant HTTPPort
  participant ReloadNotifier
  DevServer->>startAndBind: start startup
  startAndBind->>HTTPPort: bind port
  HTTPPort-->>startAndBind: binding failure
  DevServer->>ReloadNotifier: stop subscriptions
  DevServer-->>DevServer: rethrow startup error

Suggested reviewers: kwakayama, ariskemper

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix for releasing watchers and reload subscriptions after bind failure.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/dev-server-start-failure-leak

_{Comment @coderabbitai help to get the list of available commands.}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 29fd85ee43

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/server/dev-server/server.ts`:
- Around line 503-513: Update the cleanup sequence in stop() to clear each
resource handle before invoking its callback, and catch/debug-log failures
independently so one cleanup error does not stop subsequent cleanup. Apply this
to reloadUnsubscribe, invalidateUnsubscribe, releaseExternalBroadcastSource, and
later watcher resources while preserving the original start() bind error
behavior.

In `@tests/integration/server/dev-server-start-failure.test.ts`:
- Around line 81-146: Move the “DevServer start failure” test containing the
`DevServer` bind-failure scenario beside the `DevServer` source module, updating
imports and path-dependent setup as needed. Preserve the real occupied-port bind
failure coverage and all watcher and ReloadNotifier cleanup assertions.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d43630a-231d-4fb3-98cb-47e75392d0b4

📥 Commits

Reviewing files that changed from the base of the PR and between 46fe6da and 29fd85e.

📒 Files selected for processing (2)

src/server/dev-server/server.ts
tests/integration/server/dev-server-start-failure.test.ts

Addresses CodeRabbit on #3588: a release callback that throws would exit stop() before the remaining subscriptions and the watcher were released. All three handles are now cleared before any of them is invoked, so a throwing release cannot leave a handle set for a later stop() to invoke a second time — which for the external broadcast source would decrement a process-wide counter twice and suppress HMR for an unrelated dev server. Each release is then run through a helper that debug-logs a failure instead of stranding the steps after it. The three callbacks are all non-throwing closures today (two Set.delete unsubscribes and a counter decrement), so this is defence in depth on a teardown path rather than a fix for a reachable failure.

kojiwakayama · 2026-08-11T13:11:52Z

Declining this one, with reasoning on the record.

The AGENTS.md rule reads "Keep test files colocated as *.test.ts next to source", but it describes the unit tier. The repo has an established integration tree — 193 test files under tests/, run by a separate CI job (deno task test:integration, cicd.yml:93) — and this file sits alongside the five other DevServer integration tests already in tests/integration/server/: dev-server.test.ts, dev-server-handlers.test.ts, dev-server-debounce.test.ts, dev-server-debounce-simple.test.ts, parallel-requests.test.ts.

More importantly, this test cannot be a unit test as DevServer is currently written. It needs a real project directory, a full bootstrapDev(), a real OS file watcher and a real TCP bind that fails. DevServer.start() builds its own adapter via runtime.get() internally — there is no injection seam, which is exactly why the test has to reach the watcher by wrapping the runtime registry's fs.watch. Colocating it under src/server/dev-server/ would move an integration test into the unit shard without making it any more focused; giving it a genuine unit seam would mean adding adapter injection to DevServer, which is a larger production refactor than this leak fix warrants.

On the concern itself — that focused verification of src/server/dev-server/ could pass without exercising this path — that is equally true of the five sibling tests, and the pre-push hook and CI both run the full suite, so a reintroduced leak is caught before merge.

kojiwakayama · 2026-08-11T13:12:06Z

Applied in 189e815, with one change to the proposed shape.

The ordering point is the valuable half, and I took it further than the suggestion: all three handles are now cleared up front, before any of them is invoked. Clearing each handle immediately before its own call still leaves the later handles set if an earlier release throws, so a subsequent stop() would re-invoke them — and re-invoking releaseExternalBroadcastSource is the one genuinely harmful case, since it decrements a process-wide counter and would suppress HMR broadcasts for an unrelated dev server. Clearing all three first makes double-release impossible regardless of what throws.

Each release then runs through a module-level release() helper that debug-logs a failure instead of stranding the steps after it, so a throwing subscription can no longer prevent the file watcher from being cleaned up.

For the record on reachability: all three callbacks are non-throwing closures today — two are Set.delete unsubscribes from createSubscriberSet, and the third is count = Math.max(0, count - 1). So this is defence in depth on a teardown path rather than a fix for a currently reachable failure, which is why I kept it to one small helper rather than a per-call try/catch block.

Addresses CodeRabbit on #3588: a release callback that throws would exit stop() before the remaining subscriptions and the watcher were released. All three handles are now cleared before any of them is invoked, so a throwing release cannot leave a handle set for a later stop() to invoke a second time — which for the external broadcast source would decrement a process-wide counter twice and suppress HMR for an unrelated dev server. Each release is then run through a helper that debug-logs a failure instead of stranding the steps after it. The three callbacks are all non-throwing closures today (two Set.delete unsubscribes and a counter decrement), so this is defence in depth on a teardown path rather than a fix for a reachable failure.

…ind fails DevServer.start() registers the file watchers, both ReloadNotifier subscriptions and the HMR external broadcast source before it binds the HTTP port. Callers only ever receive the instance after start() resolves — startDevServer() constructs it, awaits start() and returns it — so a bind failure dropped the half-built instance with no handle and left nobody able to call stop(). Every registration then survived for the life of the process. The leaked broadcast source is the worst of the three: it decrements a process-wide counter, and while it is held HMRHandler skips its own broadcast, so a single failed start could silently disable HMR for the dev server that starts next. Reachable from the probe-then-bind race in `veryfront dev`, and from any adapter.serve() failure that is not a port collision (bad bind address, permission denied on a privileged port). start() now routes failures through stop() and rethrows. stop() is already null-safe at every step, so it tears down however far start() got, and it remains the single teardown path — a new registration added to start() cannot drift out of sync with a second cleanup list. Fixing it here rather than in startDevServer() also covers callers that construct DevServer directly. This matches the release-before-rethrow the codebase already uses in bootstrap.ts (orchestrateOrDisposeFS) and in the Deno http-server adapter. stop() now clears the release handles after calling them, so the double-stop this introduces cannot decrement the broadcast counter twice. Surfaced by CodeRabbit on #3562 and declined there as out of scope.

Addresses CodeRabbit on #3588: a release callback that throws would exit stop() before the remaining subscriptions and the watcher were released. All three handles are now cleared before any of them is invoked, so a throwing release cannot leave a handle set for a later stop() to invoke a second time — which for the external broadcast source would decrement a process-wide counter twice and suppress HMR for an unrelated dev server. Each release is then run through a helper that debug-logs a failure instead of stranding the steps after it. The three callbacks are all non-throwing closures today (two Set.delete unsubscribes and a counter decrement), so this is defence in depth on a teardown path rather than a fix for a reachable failure.

The teardown helper added above the class shifted the "export class DevServer" declaration from L57 to L70, so the generated source link went stale. Regenerated with "deno task docs" under the Deno version CI pins (2.7.7 per .github/actions/setup-deno/action.yml) rather than hand-edited; the local default 2.7.12 reflows every table and produces a 42-file diff.

Both reviewers on #3588 flagged that AGENTS.md requires test files to sit beside the source they cover, and they were right — the file is now at src/server/dev-server/server-start-failure.integration.test.ts, so a focused run of src/server/dev-server/ exercises the bind-failure cleanup. Named *.integration.test.ts rather than *.test.ts because it needs a real project directory, a full bootstrapDev(), a real OS file watcher and a real TCP bind. That suffix is excluded from the unit shard by deno.json and follows src/proxy/routing-invalidation.integration.test.ts, which is the existing precedent for a colocated integration-weight test importing tests/_helpers. It still runs in the coverage shards and the pre-push suite, so the regression stays gated.

kojiwakayama · 2026-08-11T13:41:43Z

Correcting my earlier reply on test placement — I declined this too quickly, and both reviewers were right. The test is now colocated at src/server/dev-server/server-start-failure.integration.test.ts.

What changed my mind was checking the two things I had assumed were blockers, and finding neither holds:

I assumed a colocated test could not reach tests/_helpers. It can — 18 files under src/ already import from there, so withTestContext works fine from its new home.
I assumed colocating would force an integration-weight test (real bootstrapDev(), real OS file watcher, real TCP bind) into the unit shard. It does not, because of the *.integration.test.ts suffix, which deno.json excludes from test:unit:parallel and test:coverage:unit. src/proxy/routing-invalidation.integration.test.ts is the existing precedent for exactly this: colocated, integration-weight, importing tests/_helpers.

So the suffix resolves my only real objection while satisfying the AGENTS.md rule, and it directly addresses the concern raised — a focused deno test src/server/dev-server/ now exercises the bind-failure cleanup, where before it would have passed without touching it. The test still runs in the coverage shards and the pre-push suite, so the regression stays gated.

Re-verified after the move: it still fails without the fix for the right reason (openWatchers: 1, reloadListeners: 1, invalidateListeners: 1 against a zero baseline) and passes with it.

kojiwakayama requested a review from kwakayama as a code owner August 11, 2026 12:59

chatgpt-codex-connector Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/server/dev-server/server-start-failure.integration.test.ts

coderabbitai Bot reviewed Aug 11, 2026

View reviewed changes

Comment thread src/server/dev-server/server.ts Outdated

Comment thread src/server/dev-server/server-start-failure.integration.test.ts

kojiwakayama force-pushed the fix/dev-server-start-failure-leak branch from 29fd85e to 189e815 Compare August 11, 2026 13:11

kojiwakayama force-pushed the fix/dev-server-start-failure-leak branch from 189e815 to 98b013c Compare August 11, 2026 13:24

kojiwakayama added 4 commits August 11, 2026 15:39

kojiwakayama force-pushed the fix/dev-server-start-failure-leak branch from 98b013c to 79e50d6 Compare August 11, 2026 13:41

kojiwakayama added this pull request to the merge queue Aug 11, 2026

Merged via the queue into main with commit 9c6f423 Aug 11, 2026
33 checks passed

kojiwakayama deleted the fix/dev-server-start-failure-leak branch August 11, 2026 15:07

Navigation Menu

fix(dev-server): release watchers and reload subscriptions when the bind fails - #3588

fix(dev-server): release watchers and reload subscriptions when the bind fails#3588
kojiwakayama merged 4 commits into
mainfrom
fix/dev-server-start-failure-leak

kojiwakayama commented Aug 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

kojiwakayama commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The defect

The fix

Regression test

Verification

Notes

Uh oh!

coderabbitai Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

kojiwakayama commented Aug 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kojiwakayama commented Aug 11, 2026 •

edited

Loading

coderabbitai Bot commented Aug 11, 2026 •

edited

Loading