Skip to content

fix(server): correct process-handler and shutdown lifecycle#35

Merged
lesquel merged 1 commit into
mainfrom
fix/process-shutdown-lifecycle
May 17, 2026
Merged

fix(server): correct process-handler and shutdown lifecycle#35
lesquel merged 1 commit into
mainfrom
fix/process-shutdown-lifecycle

Conversation

@lesquel

@lesquel lesquel commented May 17, 2026

Copy link
Copy Markdown
Owner

Summary

Resolves the process/shutdown lifecycle items of #33 (Tier-1/2). The plugin factory runs once per workspace/worktree in one process, so these defects compound per invocation. Verified by hand.

# Defect Fix
D1 process.on for uncaught/unhandled → listeners accumulate per invocation (dup emits, MaxListenersWarning, feedback loop) Module-scoped install-once guard (extracted to server/lifecycle.ts). Deliberately not process.onceonce removes the listener after one fire, leaving a 2nd rejection unhandled. Handlers route through shared singletons.
D2 shutdown() never closed SSE clients → keepalive setIntervals leak after server.stop() shutdown() calls eventBus.closeAll() before server.stop(); closeAll() now clears each client's pingInterval explicitly (stored on SSEClient) rather than relying on controller.close() → stream cancel() (a Bun impl detail)
D3 No re-entrancy guard → SIGINT+SIGTERM/exit ran cleanup twice Closure-scoped guard; original shutdown step order preserved exactly (only closeAll prepended)
D4 process.once("exit", () => void shutdown()) dropped all async cleanup (fires during event-loop drain) Listener removed; SIGINT/SIGTERM (intact) are the correct graceful hooks

Design notes

  • Install-once + shutdown-guard extracted to server/lifecycle.ts (testable seam; server/ may compose core/ — no dependency-rule violation; documented in server/AGENTS.md).
  • The D2 robustness hardening (explicit pingInterval clear in closeAll) was added in response to the fresh review's nit — it makes the interval-leak fix self-contained instead of depending on undocumented Bun ReadableStream behavior.

Testing

  • +9 behavioral tests (listener-count-stable-across-invocations; idempotent/concurrent shutdown; closeAll invoked; no exit listener). Non-flaky, no leaked process listeners.
  • bun test 633/0 · bunx tsc --noEmit clean (bus.ts keepalive change did not regress existing bus tests)
  • Independent fresh-context adversarial review: APPROVE, no blocking issues. Shutdown step-ordering exhaustively diffed (original vs new, side by side) — every step/condition preserved. All 3 review nits fixed in this PR.

Pre-existing, out of scope (noted, not fixed): SIGINT/SIGTERM use process.once, so in the multi-instance model only the first invocation's shutdown runs on a signal — a separate multi-instance design question.

Refs #33 (Tier-1/2, partial — does not close).

Second-wave audit (#33), verified by hand. The plugin factory runs once
per workspace/worktree in the same process; these defects compound per
invocation.

- D1: uncaughtException/unhandledRejection used process.on, appending a
  new pair every invocation (N duplicate emits/audits per error, Max
  ListenersExceededWarning, feedback loop). Fixed via a module-scoped
  install-once guard (extracted to server/lifecycle.ts). NOT process.once
  — once() removes the listener after the first fire, leaving a second
  rejection unhandled. Handlers route through the shared singletons.
- D2: shutdown() never closed SSE clients → per-client keepalive
  setIntervals kept firing after server.stop(). shutdown now calls
  eventBus.closeAll() before server.stop(); closeAll() clears each
  client's pingInterval explicitly (stored on SSEClient) instead of
  relying on controller.close() propagating to the stream cancel()
  callback (a Bun impl detail, not a WhatWG guarantee).
- D3: shutdown() had no re-entrancy guard → SIGINT+SIGTERM/exit ran
  cleanup twice. Added a closure-scoped guard; original step order is
  preserved exactly (only closeAll prepended).
- D4: removed process.once("exit", () => void shutdown()) — "exit" fires
  during event-loop drain so all async cleanup was silently dropped;
  SIGINT/SIGTERM (intact) are the correct graceful hooks.

Extracted the install-once + shutdown-guard logic to server/lifecycle.ts
(testable seam; documented in server/AGENTS.md). +9 tests. bun test
633/0, tsc clean. Independent fresh-context review: APPROVE, no blocking
issues (shutdown step-ordering exhaustively diffed — preserved); the 3
nits it raised are fixed here.

Pre-existing multi-instance note (out of scope): SIGINT/SIGTERM use
process.once so only the first invocation's shutdown runs on a signal.

Refs #33
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