[omp blocking] Overflow drain marks seq complete without completing it #152

New issue

Closed

opened 2026-09-05 20:39:02 +00:00 by crueber · 3 comments

crueber commented

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

Owner

[omp blocking] Overflow drain marks seq complete without completing it

internal/notify/tasks.go:331-335, :353-391, :484-487: fanoutOne returns "event existed", not "recipients done" — 5s FanoutBudget timeout (:354) strands goroutines at the fctx precheck (:369-377), per-recipient createFailed swallowed (:384-386), yet markFanoutDone still writes the completion marker so the #77 redrain sweep skips the seq forever.

Fix

Report completion honestly: count createFailed, log, skip the marker on incomplete (re-drain is idempotent, so a retry converges). Deterministic regression test (fault-injected recipient failure / expired budget → no marker, redrain recovers). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

Incomplete drain never writes the done marker; redrain recovers; test green -race -count=20.

# [omp blocking] Overflow drain marks seq complete without completing it `internal/notify/tasks.go:331-335`, `:353-391`, `:484-487`: `fanoutOne` returns "event existed", not "recipients done" — 5s `FanoutBudget` timeout (`:354`) strands goroutines at the `fctx` precheck (`:369-377`), per-recipient `createFailed` swallowed (`:384-386`), yet `markFanoutDone` still writes the completion marker so the #77 redrain sweep skips the seq forever. ## Fix Report completion honestly: count `createFailed`, log, skip the marker on incomplete (re-drain is idempotent, so a retry converges). Deterministic regression test (fault-injected recipient failure / expired budget → no marker, redrain recovers). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] Incomplete drain never writes the done marker; redrain recovers; test green `-race -count=20`.

crueber referenced this issue from a commit

2026-09-05 20:42:42 +00:00

Fix #152: honest overflow-drain completion (skip done marker on incomplete)

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

2026-09-05 20:42:42 +00:00

Fix #152: honest drain completion #158

crueber commented

2026-09-05 20:42:45 +00:00

Author

Owner

Fix is up: #158 (branch fix/issue-152). fanoutOne returns (existed, complete); incomplete drains skip the done marker and the redrain converges idempotently. Regression tests verified FAILING pre-fix, green -race -count=20; coverage 96.2%. Not merging per instructions.

Fix is up: https://git.packden.us/crueber/walhub/pulls/158 (branch fix/issue-152). fanoutOne returns (existed, complete); incomplete drains skip the done marker and the redrain converges idempotently. Regression tests verified FAILING pre-fix, green -race -count=20; coverage 96.2%. Not merging per instructions.

crueber commented

2026-09-05 20:44:10 +00:00

Author

Owner

Review PR #158 (fix/issue-152, honest drain completion) — APPROVE.

(existed, complete) mapping (internal/notify/tasks.go:364-424) — all correct:

missing activity (ev==nil:367) -> (false,false); drain (332-334) skips marker via !existed. Gap semantics preserved.
success incl. empty-recipients (vacuous) -> (true,true) -> marker (340). Correct.
dedup-skip (createSkipped:412-414 returns without fail()) -> complete. SOUND: live unread entry already covers the triple; redrain would skip identically via hasUnread/indexClaim dedup (emit.go:527-568). No loss.
budget-stranded, both precheck (390) and slot-wait (397) -> fail() -> incomplete. Correct.
createFailed (409-411) -> fail() -> incomplete. Correct. 412-replay maps to createCreated (emit.go:555-564) -> complete. Correct.

Counter mutex (375-383): increment-only fail() closure, never held across store calls (createOne:408, publish:417 outside lock); single unnested mutex so no lock-order issue (AGENTS.md law 3 / 13 S2). Read of failed at 419 after wg.Wait is race-safe (writes happen-before Done, -race clean x10 below).

Honest marker + exactly-once redrain: drain skips marker on !complete (336-339); sweep re-drives via fanoutDone==false + FanoutPending (tasks.go:507+); retry idempotent via deterministic NotificationID + PutCreate-412 + index dedup + orphan-delete (emit.go:526-571). Test asserts amy stays 1, zed converges 0->1 (redrain_test.go:358-396). No double delivery.

Shortfall log (420-421): repo/seq/recipients/failed only; no principals/titles/actors. Uses parent ctx (live), not expired fctx. Clean.

Negative controls genuine: pre-fix drain (main: if fanoutOne single-bool { markFanoutDone }) marked faulted drains, so TestFanoutDrainSkipsDoneMarkerOnIncomplete fails pre-fix at the fanoutDone==true assertion for the right reason (marker on partial drain). TestFanoutOneExpiredBudgetIsIncomplete pins (true,false)->(true,true) convergence (redrain_test.go:398-430); pre-fix single-bool could not distinguish stranded from done (would mark-and-lose).

Checks (scratch /tmp/pr158 @ e9cdb66): go test -race ./internal/notify/... PASS; new tests -count=10 PASS; coverage 96.2% (>=95%); gofmt clean; go vet clean. No new non-stdlib imports (tasks.go: none; redrain_test.go adds stdlib strings only).

Doc (docs/features/06_notifications.md:493-508) accurate: tuple semantics, dedup-complete rationale, idempotent retry, budget/createFailed incomplete, regression names. Nit (non-blocking): 'No new locks (one counter mutex...)' reads contradictory — suggest 'One new mutex (counter-only...)'.

No fix pushed (nothing structural/broken). MERGE RECOMMENDATION: ready to merge.

Review PR #158 (fix/issue-152, honest drain completion) — APPROVE. (existed, complete) mapping (internal/notify/tasks.go:364-424) — all correct: - missing activity (ev==nil:367) -> (false,false); drain (332-334) skips marker via !existed. Gap semantics preserved. - success incl. empty-recipients (vacuous) -> (true,true) -> marker (340). Correct. - dedup-skip (createSkipped:412-414 returns without fail()) -> complete. SOUND: live unread entry already covers the triple; redrain would skip identically via hasUnread/indexClaim dedup (emit.go:527-568). No loss. - budget-stranded, both precheck (390) and slot-wait (397) -> fail() -> incomplete. Correct. - createFailed (409-411) -> fail() -> incomplete. Correct. 412-replay maps to createCreated (emit.go:555-564) -> complete. Correct. Counter mutex (375-383): increment-only fail() closure, never held across store calls (createOne:408, publish:417 outside lock); single unnested mutex so no lock-order issue (AGENTS.md law 3 / 13 S2). Read of failed at 419 after wg.Wait is race-safe (writes happen-before Done, -race clean x10 below). Honest marker + exactly-once redrain: drain skips marker on !complete (336-339); sweep re-drives via fanoutDone==false + FanoutPending (tasks.go:507+); retry idempotent via deterministic NotificationID + PutCreate-412 + index dedup + orphan-delete (emit.go:526-571). Test asserts amy stays 1, zed converges 0->1 (redrain_test.go:358-396). No double delivery. Shortfall log (420-421): repo/seq/recipients/failed only; no principals/titles/actors. Uses parent ctx (live), not expired fctx. Clean. Negative controls genuine: pre-fix drain (main: if fanoutOne single-bool { markFanoutDone }) marked faulted drains, so TestFanoutDrainSkipsDoneMarkerOnIncomplete fails pre-fix at the fanoutDone==true assertion for the right reason (marker on partial drain). TestFanoutOneExpiredBudgetIsIncomplete pins (true,false)->(true,true) convergence (redrain_test.go:398-430); pre-fix single-bool could not distinguish stranded from done (would mark-and-lose). Checks (scratch /tmp/pr158 @ e9cdb66): go test -race ./internal/notify/... PASS; new tests -count=10 PASS; coverage 96.2% (>=95%); gofmt clean; go vet clean. No new non-stdlib imports (tasks.go: none; redrain_test.go adds stdlib strings only). Doc (docs/features/06_notifications.md:493-508) accurate: tuple semantics, dedup-complete rationale, idempotent retry, budget/createFailed incomplete, regression names. Nit (non-blocking): 'No new locks (one counter mutex...)' reads contradictory — suggest 'One new mutex (counter-only...)'. No fix pushed (nothing structural/broken). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 20:44:25 +00:00

crueber commented

2026-09-05 20:44:25 +00:00

Author

Owner

Fixed by PR #158 (review: completion mapping + mutex scope + negative controls verified; 96.2% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 20:44:26 +00:00

Merge pull request 'Fix #152: honest drain completion' (#158) from fix/issue-152 into main

crueber referenced this issue

2026-09-05 21:12:16 +00:00