[codex major] Per-user notify SSE leaks one goroutine per disconnected stream #73

New issue

Closed

opened 2026-09-05 00:15:03 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 00:15:03 +00:00

Owner

[codex major] Per-user notify SSE leaks one goroutine per disconnected stream

internal/notify/stream.go:252 ranges over s.ka.C; close() (:297) only calls Ticker.Stop(). Stopping a ticker does not close its channel, so the goroutine blocks forever after the handler's defer s.close() (internal/notify/http.go:169). Violates the context-exit rule (every goroutine exits via context).

Fix

Make stream teardown actually terminate the keepalive goroutine (close a done channel / context-cancel the range / stop ranging on close — pick the shape matching the codebase's channel rule: sender owns and closes). Regression test proving goroutine exit after handler close (leak check via runtime.NumGoroutine orDone-channel assertion). Coverage gate holds.

Acceptance criteria

No leaked goroutine on stream disconnect; regression test green -race.

# [codex major] Per-user notify SSE leaks one goroutine per disconnected stream `internal/notify/stream.go:252` ranges over `s.ka.C`; `close()` (`:297`) only calls `Ticker.Stop()`. Stopping a ticker does not close its channel, so the goroutine blocks forever after the handler's `defer s.close()` (`internal/notify/http.go:169`). Violates the context-exit rule (every goroutine exits via context). ## Fix Make stream teardown actually terminate the keepalive goroutine (close a done channel / context-cancel the range / stop ranging on close — pick the shape matching the codebase's channel rule: sender owns and closes). Regression test proving goroutine exit after handler close (leak check via runtime.NumGoroutine orDone-channel assertion). Coverage gate holds. ## Acceptance criteria - [ ] No leaked goroutine on stream disconnect; regression test green `-race`.

crueber referenced this issue from a commit

2026-09-05 00:38:59 +00:00

Fix #73: terminate SSE keepalive goroutines on stream teardown

crueber referenced this issue from a pull request that will close it,

2026-09-05 00:39:02 +00:00

Fix #73: notify SSE goroutine lifecycle #82

crueber commented

2026-09-05 00:39:05 +00:00

Author

Owner

Fixed by PR #82 (#82): sender-owned done channel + request-context select replaces the range over ka.C in internal/notify/stream.go; same latent shape fixed in sibling internal/api/sse.go. Regression tests fail pre-fix (5 leaked goroutines) and pass post-fix; notify coverage 97.2%, -race green.

Fixed by PR #82 (https://git.packden.us/crueber/walhub/pulls/82): sender-owned done channel + request-context select replaces the range over ka.C in internal/notify/stream.go; same latent shape fixed in sibling internal/api/sse.go. Regression tests fail pre-fix (5 leaked goroutines) and pass post-fix; notify coverage 97.2%, -race green.

crueber commented

2026-09-05 00:41:08 +00:00

Author

Owner

Review of PR #82 (fix/issue-73, commit f2ac4d1) — verified in scratch worktree /tmp/wt73, main worktree untouched.

DONE-CHANNEL LIFECYCLE — PASS

internal/notify/stream.go:235-236,318-324: done is created in newSSEWriter, closed in exactly one place (stopLocked:321), guarded by ended under mu. All stopLocked call sites (event:288, comment:304, close:315) hold mu. No double-close path: Close racing ctx-cancel serializes on mu; ctx-cancel alone never closes done (goroutine exits via ctx arm instead). No panic possible.
internal/api/sse.go:27,144-150: identical shape — single close site (sse.go:147) under ended guard, all callers (Event:81/85/89, Close:138) hold mu. stopLocked is lock-free (no re-lock deadlock: comment->write and Event->write/stopLocked never re-acquire mu).
Sender-closes holds in both: done is close-only (never sent on); only the writer side closes; receivers (keepalive select) never close. Matches 13 §5 rules 1-2.

KEEPALIVE LOOP / STUCK-WRITE (omp minor #9) — PASS

Both loops (stream.go:258-272, sse.go:54-68) select on done AND ctx AND ka.C — no path blocks forever on the channel.
The write itself is bounded: notify comment/event set a 15s SetWriteDeadline (stream.go:286,302); api write() sets 15s (sse.go:124). A TCP zero-window client pins the goroutine at most 15s, then the error path returns false and the goroutine exits. Concern addressed.
Observation (non-blocking): api comment() (sse.go:108-120) does not call stopLocked on write failure — the goroutine still exits via 'return', and deferred Close() reaps done/ticker. No leak; notify's comment does stop eagerly. Equivalent outcomes, leave as is.

SIBLING-FIX SAFETY (task streams) — PASS

Event() behavior change is nil-observable to pump() (sse.go:173-201): pre-fix, post-failure Events re-hit ctx/write errors and returned false; post-fix they short-circuit on ended and return false. Terminal result/error path sets ended identically. pump's range/return logic unchanged.

REGRESSION TESTS — PASS, GENUINE

stream_leak_test.go + sse_leak_test.go: deadline-poll to baseline (5s, 10ms cadence, full stack dump on timeout) — no fixed sleep-then-assert; spawn-count guard (got >= base+N) prevents vacuous pass.
Negative controls verified by swapping pre-fix files back: TestSSEWriterKeepaliveExitsOnClose FAILS pre-fix (goroutines stuck chan-receive at stream.go:253, the 'for range ka.C'), TestSSEKeepaliveExitsOnClose FAILS pre-fix (stuck at sse.go:53). Both fail for the right reason (Stop never closes ka.C).

GATES

go test -race ./internal/notify/... ./internal/api/... : ok both. New tests -count=5: ok both.
Coverage: notify 97.2%, api 95.5% — both >= 95%. gofmt clean, go vet clean.
Imports: only addition is stdlib 'context' in notify/stream.go (stores r.Context()); zero third-party additions. Doc bullet in docs/features/06_notifications.md:360-366 is accurate (sender-owned done, ended guard, sibling fix, 10s contract unchanged).

MERGE RECOMMENDATION: ready to merge (no fixes pushed — nothing to fix).

Review of PR #82 (fix/issue-73, commit f2ac4d1) — verified in scratch worktree /tmp/wt73, main worktree untouched. DONE-CHANNEL LIFECYCLE — PASS - internal/notify/stream.go:235-236,318-324: done is created in newSSEWriter, closed in exactly one place (stopLocked:321), guarded by ended under mu. All stopLocked call sites (event:288, comment:304, close:315) hold mu. No double-close path: Close racing ctx-cancel serializes on mu; ctx-cancel alone never closes done (goroutine exits via ctx arm instead). No panic possible. - internal/api/sse.go:27,144-150: identical shape — single close site (sse.go:147) under ended guard, all callers (Event:81/85/89, Close:138) hold mu. stopLocked is lock-free (no re-lock deadlock: comment->write and Event->write/stopLocked never re-acquire mu). - Sender-closes holds in both: done is close-only (never sent on); only the writer side closes; receivers (keepalive select) never close. Matches 13 §5 rules 1-2. KEEPALIVE LOOP / STUCK-WRITE (omp minor #9) — PASS - Both loops (stream.go:258-272, sse.go:54-68) select on done AND ctx AND ka.C — no path blocks forever on the channel. - The write itself is bounded: notify comment/event set a 15s SetWriteDeadline (stream.go:286,302); api write() sets 15s (sse.go:124). A TCP zero-window client pins the goroutine at most 15s, then the error path returns false and the goroutine exits. Concern addressed. - Observation (non-blocking): api comment() (sse.go:108-120) does not call stopLocked on write failure — the goroutine still exits via 'return', and deferred Close() reaps done/ticker. No leak; notify's comment does stop eagerly. Equivalent outcomes, leave as is. SIBLING-FIX SAFETY (task streams) — PASS - Event() behavior change is nil-observable to pump() (sse.go:173-201): pre-fix, post-failure Events re-hit ctx/write errors and returned false; post-fix they short-circuit on ended and return false. Terminal result/error path sets ended identically. pump's range/return logic unchanged. REGRESSION TESTS — PASS, GENUINE - stream_leak_test.go + sse_leak_test.go: deadline-poll to baseline (5s, 10ms cadence, full stack dump on timeout) — no fixed sleep-then-assert; spawn-count guard (got >= base+N) prevents vacuous pass. - Negative controls verified by swapping pre-fix files back: TestSSEWriterKeepaliveExitsOnClose FAILS pre-fix (goroutines stuck chan-receive at stream.go:253, the 'for range ka.C'), TestSSEKeepaliveExitsOnClose FAILS pre-fix (stuck at sse.go:53). Both fail for the right reason (Stop never closes ka.C). GATES - go test -race ./internal/notify/... ./internal/api/... : ok both. New tests -count=5: ok both. - Coverage: notify 97.2%, api 95.5% — both >= 95%. gofmt clean, go vet clean. - Imports: only addition is stdlib 'context' in notify/stream.go (stores r.Context()); zero third-party additions. Doc bullet in docs/features/06_notifications.md:360-366 is accurate (sender-owned done, ended guard, sibling fix, 10s contract unchanged). MERGE RECOMMENDATION: ready to merge (no fixes pushed — nothing to fix).

crueber closed this issue

2026-09-05 00:41:25 +00:00

crueber referenced this issue from a commit

2026-09-05 00:41:25 +00:00

Merge pull request 'Fix #73: notify SSE goroutine lifecycle' (#82) from fix/issue-73 into main

crueber commented

2026-09-05 00:41:25 +00:00

Author

Owner

Fixed by PR #82 (review: exactly-once close + write deadlines verified, negative controls genuine; 97.2%/95.5% coverage), merged. Closing.

crueber added the

webhooks