[minor-10] Watch DELETE order can strand watcher in `watcher_list` #94

New issue

Closed

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

crueber commented

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

Owner

[minor-10] Watch DELETE order can strand watcher in `watcher_list`

internal/notify/watch.go:61-68: SetWatch(off) deletes the record BEFORE the socialWatch CAS. If the CAS fails (or the process dies between), the record is gone while watcher_list still contains the principal — they keep receiving subscribed-class fan-out with no record to delete (repair only via deliberate re-PUT+DELETE).

Fix

Reverse the order: list CAS first, then record delete (a crash between then leaves a record without list membership — fail-closed, self-heals on next toggle). Regression test (fault-injected CAS failure → no phantom watcher). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

No failure ordering strands a watcher; test green -race.

# [minor-10] Watch DELETE order can strand watcher in `watcher_list` `internal/notify/watch.go:61-68`: `SetWatch(off)` deletes the record BEFORE the `socialWatch` CAS. If the CAS fails (or the process dies between), the record is gone while `watcher_list` still contains the principal — they keep receiving subscribed-class fan-out with no record to delete (repair only via deliberate re-PUT+DELETE). ## Fix Reverse the order: list CAS first, then record delete (a crash between then leaves a record without list membership — fail-closed, self-heals on next toggle). Regression test (fault-injected CAS failure → no phantom watcher). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] No failure ordering strands a watcher; test green `-race`.

crueber referenced this issue from a commit

2026-09-05 03:06:55 +00:00

Fix #94: watch unwatch order (list CAS before record delete)

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

2026-09-05 03:06:56 +00:00

Fix #94: watch unwatch order #103

crueber commented

2026-09-05 03:06:59 +00:00

Author

Owner

Fixed by #103 (#103): SetWatch(off) now CASes watcher_list before deleting the record — fail-closed + self-healing. Regression test + Decisions entry included; -race green, coverage 97.3%.

Fixed by #103 (https://git.packden.us/crueber/walhub/pulls/103): SetWatch(off) now CASes watcher_list before deleting the record — fail-closed + self-healing. Regression test + Decisions entry included; -race green, coverage 97.3%.

crueber commented

2026-09-05 03:08:22 +00:00

Author

Owner

PR #103 review (fix/issue-94, commit 6967d10) — APPROVED, ready to merge.

Order reversal correct (watch.go:69-78): off-path is now repoAlive probe → socialWatch CAS(false) → record Delete. Ghost-repo path (70-73) deletes-then-skips-CAS with zero writes to social.json — resurrection impossible (repoAlive at notify.go:614 is a read-only Exists probe; Delete cannot create).

Crash-between fail-closed confirmed: fan-out resolve() (emit.go:341) consumes s.watchers() (emit.go:405-414), which reads social.json watcher_list only — never the userspace record. Record-without-membership notifies no one.

Self-heal walked: retry off → socialWatch removes member (or !on&&!have reconcile no-op at watch.go:105-115) → Delete → converges to record-gone+list-gone. Retry on → putCreate 412-tolerated + CAS re-add. Both converge.

on=true path unchanged (diff touches only the else-branch) and already safe: CAS failure there leaves record+no-member — the same fail-closed shape, retry converges via the 412-tolerated putCreate.

CAS-failure return correct: socialWatch err → SetWatch returns WatchState{},err; http.go:300-306 propagates via statusFor (no swallow) — caller sees the error and can retry.

Regression test genuine: TestSetWatchUnwatchCASFailureNoPhantom (cover2_test.go:571) FAILS on the pre-fix order at line 624 ('phantom watcher stranded in watcher_list') and PASSES with the fix. Retry-convergence asserted (629-637).

Hygiene: no new imports/locks (diff reorders two statements + comment); gofmt clean; go vet clean; go test -race ./internal/notify/... green; coverage 97.3% (≥95% gate holds). Doc entry (06_notifications.md:466) accurate — matches code, law-4 rationale correct.

No changes pushed (nothing to fix). MERGE RECOMMENDATION: ready to merge.

PR #103 review (fix/issue-94, commit 6967d10) — APPROVED, ready to merge. Order reversal correct (watch.go:69-78): off-path is now repoAlive probe → socialWatch CAS(false) → record Delete. Ghost-repo path (70-73) deletes-then-skips-CAS with zero writes to social.json — resurrection impossible (repoAlive at notify.go:614 is a read-only Exists probe; Delete cannot create). Crash-between fail-closed confirmed: fan-out resolve() (emit.go:341) consumes s.watchers() (emit.go:405-414), which reads social.json watcher_list only — never the userspace record. Record-without-membership notifies no one. Self-heal walked: retry off → socialWatch removes member (or !on&&!have reconcile no-op at watch.go:105-115) → Delete → converges to record-gone+list-gone. Retry on → putCreate 412-tolerated + CAS re-add. Both converge. on=true path unchanged (diff touches only the else-branch) and already safe: CAS failure there leaves record+no-member — the same fail-closed shape, retry converges via the 412-tolerated putCreate. CAS-failure return correct: socialWatch err → SetWatch returns WatchState{},err; http.go:300-306 propagates via statusFor (no swallow) — caller sees the error and can retry. Regression test genuine: TestSetWatchUnwatchCASFailureNoPhantom (cover2_test.go:571) FAILS on the pre-fix order at line 624 ('phantom watcher stranded in watcher_list') and PASSES with the fix. Retry-convergence asserted (629-637). Hygiene: no new imports/locks (diff reorders two statements + comment); gofmt clean; go vet clean; go test -race ./internal/notify/... green; coverage 97.3% (≥95% gate holds). Doc entry (06_notifications.md:466) accurate — matches code, law-4 rationale correct. No changes pushed (nothing to fix). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 03:08:46 +00:00

crueber commented

2026-09-05 03:08:46 +00:00

Author

Owner

Fixed by PR #103 (review: order, fail-closed, self-heal verified, negative control genuine; 97.3% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 03:08:46 +00:00

Merge pull request 'Fix #94: watch unwatch order' (#103) from fix/issue-94 into main

crueber added the

webhooks