[codex major + omp major-1, corroborated] Notify fan-out drain drops sequences #72

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 + omp major-1, corroborated] Notify fan-out drain drops sequences

Two independent reviewers found the same dropped-work race in internal/notify/tasks.go: the drainer sees empty seqs (drain()) and breaks while a concurrent enqueueFanout attaches to the still-running entry and appends (tasks.go:228-237); the drainer removes the task (:257), the joiner already returned (:232) and starts no worker. end() (:80-102) never re-checks e.seqs. The sequence is never processed; Run sweeps webhooks/retention only, not unprocessed fan-out. Recipients permanently, silently miss notifications.

Fix

Close the race: re-check e.seqs in/after end() (drain-then-end atomically under the entry discipline), or make the joiner own leftover seqs, or sweep unprocessed fan-out from the activity log. Deterministic regression test (late-attach vs terminal-drain interleaving) failing pre-fix. Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

No interleaving loses a sequence; regression test green -race -count=20.
go test -race + coverage ≥95% on internal/notify.

# [codex major + omp major-1, corroborated] Notify fan-out drain drops sequences Two independent reviewers found the same dropped-work race in `internal/notify/tasks.go`: the drainer sees empty `seqs` (`drain()`) and breaks while a concurrent `enqueueFanout` attaches to the still-running entry and appends (`tasks.go:228-237`); the drainer removes the task (`:257`), the joiner already returned (`:232`) and starts no worker. `end()` (`:80-102`) never re-checks `e.seqs`. The sequence is never processed; `Run` sweeps webhooks/retention only, not unprocessed fan-out. Recipients permanently, silently miss notifications. ## Fix Close the race: re-check `e.seqs` in/after `end()` (drain-then-end atomically under the entry discipline), or make the joiner own leftover seqs, or sweep unprocessed fan-out from the activity log. Deterministic regression test (late-attach vs terminal-drain interleaving) failing pre-fix. Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] No interleaving loses a sequence; regression test green `-race -count=20`. - [ ] `go test -race` + coverage ≥95% on `internal/notify`.

crueber referenced this issue from a commit

2026-09-05 00:31:57 +00:00

Fix #72: close notify fan-out drain/end race

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

2026-09-05 00:32:01 +00:00

Fix #72: fan-out drain/end race #81

crueber commented

2026-09-05 00:32:04 +00:00

Author

Owner

Fixed by PR #81 (#81): two-sided close in internal/notify/tasks.go — leader ends only via endIfQuiescent (drain-then-end atomic, refuses while seqs pending), joiner re-checks current() and re-enqueues on a miss. Deterministic regression test (late-attach-vs-terminal-drain) fails pre-fix, green -race -count=20; package coverage 97.2%.

Fixed by PR #81 (https://git.packden.us/crueber/walhub/pulls/81): two-sided close in internal/notify/tasks.go — leader ends only via endIfQuiescent (drain-then-end atomic, refuses while seqs pending), joiner re-checks current() and re-enqueues on a miss. Deterministic regression test (late-attach-vs-terminal-drain) fails pre-fix, green -race -count=20; package coverage 97.2%.

crueber commented

2026-09-05 00:34:44 +00:00

Author

Owner

PR #81 review (fix/issue-72, e458976) — DROPPED-WORK scrutiny, verified in scratch worktrees (main worktree untouched, read-only).

VERDICT: ready to merge. No blocking issues; no fixes needed.

Lock discipline (law 3) — PASS

endIfQuiescent (tasks.go:131) is the ONLY nested acquisition, order table→entry throughout. All other paths take exactly one lock: begin/end/current/TaskStatus table-only; attach/drain entry-only. No entry→table path exists anywhere in the package (audited all mu.Lock sites on the branch) — no deadlock.
No lock held across store/network on any new path: the nested section touches only e.seqs + finishLocked (record fields, maps, wg.Done). drainFanout releases the entry lock before fanoutOne; enqueueFanout holds nothing across iterations.

Progress/livelock — PASS

Refusal always means seqs were pending, so the mandated drain-again processes >=1 seq per loop: progress each iteration, ends when attaches stop. Joiner re-enqueue loop terminates (a miss implies a task ended; re-begin either leads or joins-current).

No duplicates — PASS

Orphaned copy sits on an unreferenced detached entry no worker ever drains (GC'd, never processes). Double-drain would still be a no-op: deterministic NotificationID + putCreate-412 (fanoutOne), hasUnread guard (emit.go:482), indexAdd id-dedup (emit.go:503) — all confirmed.

finishLocked refactor — PASS: byte-identical to old end() body (state/stamp/note, wg.Done, delete, recent/order/128-bound). Fanout tasks never touched cursors/metrics; nothing lost. Bare end() retained only for the seqless webhooks task — safe.
Bad-repo path — PASS: drains+discards, still ends quiescent; a concurrent attach costs one more drain-discard round, then ends.

Negative control — GENUINE: adapted interleaving (begin→drain-empty→join+attach(7)→bare end) on pre-fix d8c0e33 finishes the task with seq 7 pending on the detached entry — the exact silent loss; new test asserts the refusal at that step. (Direct backport can't compile pre-fix — new symbols — so the control ran adapted; passed, temp file removed.)

Gate: full notify suite -race PASS (count=1), new tests -race -count=20 PASS, coverage 97.2% (>=95%), gofmt/vet clean, zero import changes. Doc entries (06 Decisions, notify.go header, tasks.go Concurrency subsection) match the implementation.

Footnote (pre-existing, non-blocking): StartWebhooks returns the live e.rec pointer readable without the table lock — predates this PR, untouched by it.

PR #81 review (fix/issue-72, e458976) — DROPPED-WORK scrutiny, verified in scratch worktrees (main worktree untouched, read-only). VERDICT: ready to merge. No blocking issues; no fixes needed. Lock discipline (law 3) — PASS - endIfQuiescent (tasks.go:131) is the ONLY nested acquisition, order table→entry throughout. All other paths take exactly one lock: begin/end/current/TaskStatus table-only; attach/drain entry-only. No entry→table path exists anywhere in the package (audited all mu.Lock sites on the branch) — no deadlock. - No lock held across store/network on any new path: the nested section touches only e.seqs + finishLocked (record fields, maps, wg.Done). drainFanout releases the entry lock before fanoutOne; enqueueFanout holds nothing across iterations. Progress/livelock — PASS - Refusal always means seqs were pending, so the mandated drain-again processes >=1 seq per loop: progress each iteration, ends when attaches stop. Joiner re-enqueue loop terminates (a miss implies a task ended; re-begin either leads or joins-current). No duplicates — PASS - Orphaned copy sits on an unreferenced detached entry no worker ever drains (GC'd, never processes). Double-drain would still be a no-op: deterministic NotificationID + putCreate-412 (fanoutOne), hasUnread guard (emit.go:482), indexAdd id-dedup (emit.go:503) — all confirmed. finishLocked refactor — PASS: byte-identical to old end() body (state/stamp/note, wg.Done, delete, recent/order/128-bound). Fanout tasks never touched cursors/metrics; nothing lost. Bare end() retained only for the seqless webhooks task — safe. Bad-repo path — PASS: drains+discards, still ends quiescent; a concurrent attach costs one more drain-discard round, then ends. Negative control — GENUINE: adapted interleaving (begin→drain-empty→join+attach(7)→bare end) on pre-fix d8c0e33 finishes the task with seq 7 pending on the detached entry — the exact silent loss; new test asserts the refusal at that step. (Direct backport can't compile pre-fix — new symbols — so the control ran adapted; passed, temp file removed.) Gate: full notify suite -race PASS (count=1), new tests -race -count=20 PASS, coverage 97.2% (>=95%), gofmt/vet clean, zero import changes. Doc entries (06 Decisions, notify.go header, tasks.go Concurrency subsection) match the implementation. Footnote (pre-existing, non-blocking): StartWebhooks returns the live e.rec pointer readable without the table lock — predates this PR, untouched by it.

crueber closed this issue

2026-09-05 00:35:40 +00:00

crueber referenced this issue from a commit

2026-09-05 00:35:40 +00:00

Merge pull request 'Fix #72: fan-out drain/end race' (#81) from fix/issue-72 into main

crueber commented

2026-09-05 00:35:40 +00:00

Author

Owner

Fixed by PR #81 (review: lock order + no-dup + negative control verified; 97.2% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 21:09:11 +00:00