Skip to content

fix(server): refcount heartbeat clients instead of exiting on first disconnect - #1

Draft
jscheid wants to merge 4 commits into
mainfrom
fix/heartbeat-refcount
Draft

fix(server): refcount heartbeat clients instead of exiting on first disconnect#1
jscheid wants to merge 4 commits into
mainfrom
fix/heartbeat-refcount

Conversation

@jscheid

@jscheid jscheid commented Sep 4, 2026

Copy link
Copy Markdown

Replaces "exit when any one browser disconnects" with a refcounted, grace-period idle decision, and stops the client suppressing its own SSE reconnect.

Groundwork for the one-way review handoff (docs/design/one-way-review-handoff.md §5–6): an agent can only be told a review is finished if the server can tell that nobody is looking at it any more.

Why

Two live bugs in main:

  • The /api/heartbeat close handler shut the whole server down when any single stream closed. With two tabs open, closing one ended the other's session.
  • The client called eventSource.close() in onerror, which disables the browser's built-in EventSource retry. One transient drop made a live tab look permanently gone. Rare on loopback; routine behind a reverse proxy with an idle timeout.

Approach

The decision logic is a pure, clock-injected ReviewLifecycle value in a new module — stateAt(now) never reads the wall clock, so the grace period is tested without sleeping. The Express handler keeps only the imperative parts: counting, and one timer that asks the pure value whether the review has gone terminal.

New module rather than more logic in src/server/server.ts (~1150 lines, changes often upstream) to keep rebase conflict surface small.

Notes for the reviewer

  • disconnects counts transitions to zero clients, not individual stream closes. Counting every close would make a reviewer closing three tabs look identical to three network drops. Nothing consumes it yet; a follow-up branch will.
  • useFileWatch still hand-rolls reconnection for /api/watch with a five-attempt cap, after which the client goes permanently stale. Deliberately not copied here, and deliberately not fixed here — separate defect.
  • Behaviour change: with --keep-alive, the "staying alive" log now prints after the grace period rather than immediately on disconnect.
  • The grace timer is unref'd and cleared on server.close(), so it cannot outlive its server. afterEach in the test suite restores the real process.exit before closing, so a leaked timer would have been able to kill the test runner.

Verification

pnpm test 879 passed / 2 skipped · pnpm check clean · pnpm build clean.

Run everything under the pinned toolchain (mise exec -- pnpm …). Node 26 produces ~70 spurious localStorage failures against this repo's happy-dom/vitest pairing and has segfaulted tsgolint; mise.toml pins Node 24.

jscheid and others added 4 commits September 4, 2026 21:55
Previously the heartbeat handler shut the server down as soon as any
one SSE connection closed, so a second open tab killed the review for
everyone. Wire in the ReviewLifecycle from Task 1: track connect/
disconnect counts, and only shut down once the client count has been
zero for idleGraceMs (default 10s, overridable via ServerOptions).
Closing on error disabled EventSource's built-in retry, so a single
transient drop made a live tab look permanently gone to the server's
refcount-based idle detection. Log and let the browser reconnect.
Adds a --idle-grace <seconds> option that maps to ServerOptions.idleGraceMs
(converted to milliseconds), so the heartbeat-refcount grace period is
tunable without editing code.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant