[minor-6] Unread-dedup check-then-act race mints duplicate tray entries #91

New issue

Closed

opened 2026-09-05 02:40:26 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 02:40:26 +00:00

Owner

[minor-6] Unread-dedup check-then-act race mints duplicate tray entries

internal/notify/emit.go:454-459 (hasUnread at :456) + :482-497: createOne checks hasUnread then Creates; the id embeds the reserved seq, so two concurrent emissions for the same (user, thread, reason) both pass the check and Create distinct objects + two unread index rows. CAS doesn't save you — different ids. Violates "one live notification per (user, thread, reason)" under concurrency.

Fix

Make the dedup atomic under concurrency (entry-level discipline: re-check under the index CAS loop, or reason-keyed reservation with Create-arbitration). Deterministic regression test (concurrent same-triple emissions → single unread row). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

Concurrent same-triple emissions converge to one unread entry; test green -race -count=20.

# [minor-6] Unread-dedup check-then-act race mints duplicate tray entries `internal/notify/emit.go:454-459` (`hasUnread` at `:456`) + `:482-497`: `createOne` checks `hasUnread` then Creates; the id embeds the reserved seq, so two concurrent emissions for the same (user, thread, reason) both pass the check and Create distinct objects + two unread index rows. CAS doesn't save you — different ids. Violates "one live notification per (user, thread, reason)" under concurrency. ## Fix Make the dedup atomic under concurrency (entry-level discipline: re-check under the index CAS loop, or reason-keyed reservation with Create-arbitration). Deterministic regression test (concurrent same-triple emissions → single unread row). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] Concurrent same-triple emissions converge to one unread entry; test green `-race -count=20`.

crueber referenced this issue from a commit

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

Fix #91: atomic unread dedup via index-CAS re-check

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

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

Fix #91: atomic unread dedup #100

crueber commented

2026-09-05 02:50:13 +00:00

Author

Owner

Fixed by PR #100 (branch fix/issue-91): unread dedup re-checked inside the unread-index CAS loop (indexClaim); loser deletes its orphan object; fanoutOne delegates to createOne. Regression test TestEmitConcurrentSameTripleDedups fails pre-fix (14 rows), green -race -count=20 post-fix; coverage 97.3%.

crueber commented

2026-09-05 02:53:34 +00:00

Author

Owner

PR #100 review (fix/issue-91, atomic unread dedup) — APPROVED, ready to merge.

Linearization (emit.go:556 indexUpsert, notify.go:531 casUpdate): two concurrent same-triple creates carry distinct seq-derived ids, so both Creates succeed and arbitration falls to the index CAS. CAS serializes the PutUpdate: winner commits, loser's stale-version write 412s, retry re-reads and now sees the winner's unread same-triple row with a different id → errDedupLive abort → (live=false, nil). Exactly one row lands either way (if the 'loser' commits first it is the winner). Read-flip interleave is linearizable: abort is decided on the read's snapshot; a concurrent mark-read that lands first simply lets the second row land, which is the correct 'a read does not block a new one' outcome. Callback errors return immediately from casUpdate (no retry), 8 attempts preserved — no infinite loop, no new locks (errors sentinel only, law 3 clean).

Orphan Delete (emit.go:504-506): targets NotifKey(principal, own-seq id) only — never another principal's row, never another emission's id. The aborted claim means no index row references it, so it can never be a LIVE row; worst case a concurrent LIST-overflow reader 404s (documented harmless). fresh flag correctly skips Delete on 412 (pre-existing object may back a live row; same-id retry-orphan leftover is the pre-existing crash class). Best-effort failure mode = same orphan class the pre-fix Create→index crash window already had. Doc states this accurately.

Propagation: createAll treats createSkipped as complete (no frame, no task arm — matches the 'dedup-skips never arm the task' decision); fanoutOne 'status != createCreated → return' preserves the prior best-effort drain (seq still marked done, identical to the pre-fix per-recipient error path). indexAdd id-only path is byte-identical to pre-fix (only the dedupTriple branch added); retention uses direct casUpdate, unaffected. hasUnread remains a side-effect-free fast path; the CAS re-check is authoritative.

Evidence (scratch worktree @b89e44a): gofmt clean, go vet clean, go test -race ./internal/notify/... ok, new TestEmitConcurrentSameTripleDedups -count=10 -race ok, coverage 97.3% (≥95% gate). Negative control genuine: new test vs origin/main emit+tasks fails as created=16 skipped=0 (the bug), passes 1/15/0 on the fix. Doc Decisions entry (§2/§4 arbitration, 412/fresh, indexAdd, fanoutOne delegation, lock/order/crash notes) matches the code.

No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

PR #100 review (fix/issue-91, atomic unread dedup) — APPROVED, ready to merge. Linearization (emit.go:556 indexUpsert, notify.go:531 casUpdate): two concurrent same-triple creates carry distinct seq-derived ids, so both Creates succeed and arbitration falls to the index CAS. CAS serializes the PutUpdate: winner commits, loser's stale-version write 412s, retry re-reads and now sees the winner's unread same-triple row with a different id → errDedupLive abort → (live=false, nil). Exactly one row lands either way (if the 'loser' commits first it is the winner). Read-flip interleave is linearizable: abort is decided on the read's snapshot; a concurrent mark-read that lands first simply lets the second row land, which is the correct 'a read does not block a new one' outcome. Callback errors return immediately from casUpdate (no retry), 8 attempts preserved — no infinite loop, no new locks (errors sentinel only, law 3 clean). Orphan Delete (emit.go:504-506): targets NotifKey(principal, own-seq id) only — never another principal's row, never another emission's id. The aborted claim means no index row references it, so it can never be a LIVE row; worst case a concurrent LIST-overflow reader 404s (documented harmless). fresh flag correctly skips Delete on 412 (pre-existing object may back a live row; same-id retry-orphan leftover is the pre-existing crash class). Best-effort failure mode = same orphan class the pre-fix Create→index crash window already had. Doc states this accurately. Propagation: createAll treats createSkipped as complete (no frame, no task arm — matches the 'dedup-skips never arm the task' decision); fanoutOne 'status != createCreated → return' preserves the prior best-effort drain (seq still marked done, identical to the pre-fix per-recipient error path). indexAdd id-only path is byte-identical to pre-fix (only the dedupTriple branch added); retention uses direct casUpdate, unaffected. hasUnread remains a side-effect-free fast path; the CAS re-check is authoritative. Evidence (scratch worktree @b89e44a): gofmt clean, go vet clean, go test -race ./internal/notify/... ok, new TestEmitConcurrentSameTripleDedups -count=10 -race ok, coverage 97.3% (≥95% gate). Negative control genuine: new test vs origin/main emit+tasks fails as created=16 skipped=0 (the bug), passes 1/15/0 on the fix. Doc Decisions entry (§2/§4 arbitration, 412/fresh, indexAdd, fanoutOne delegation, lock/order/crash notes) matches the code. No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 02:53:49 +00:00

crueber commented

2026-09-05 02:53:49 +00:00

Author

Owner

Fixed by PR #100 (review: race closure + orphan safety verified, negative control genuine; 97.3% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 02:53:51 +00:00

Merge pull request 'Fix #91: atomic unread dedup' (#100) from fix/issue-91 into main

crueber added the

webhooks