[omp major] No cap on hooks per repo; sweep does full hook scan every minute #156

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 major] No cap on hooks per repo; sweep does full hook scan every minute

internal/notify/webhooks.go:153-200, tasks.go:264-273: sweepWebhooks LISTs + GETs every hook of every repo every minute on the shared Run goroutine. Hook count per repo is uncapped, so one repo with many hooks (or many repos) makes every sweep O(total hooks) forever.

Fix

Cap hooks per repo (validated max, documented default) and/or make the sweep incremental (only hooks with pending cursors / recent activity). Regression test (N hooks → bounded sweep cost). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

Sweep cost bounded independent of total hook count; test green -race.

# [omp major] No cap on hooks per repo; sweep does full hook scan every minute `internal/notify/webhooks.go:153-200`, `tasks.go:264-273`: `sweepWebhooks` LISTs + GETs every hook of every repo every minute on the shared `Run` goroutine. Hook count per repo is uncapped, so one repo with many hooks (or many repos) makes every sweep O(total hooks) forever. ## Fix Cap hooks per repo (validated max, documented default) and/or make the sweep incremental (only hooks with pending cursors / recent activity). Regression test (N hooks → bounded sweep cost). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] Sweep cost bounded independent of total hook count; test green `-race`.

crueber referenced this issue from a commit

2026-09-05 21:33:06 +00:00

notify: cap hooks per repo + incremental webhook sweep (fix #156)

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

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

Fix #156: hooks cap + bounded sweep #162

crueber commented

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

Author

Owner

Fix ready for review: PR #162 (#162) — branch fix/issue-156. Cap (20/repo, 409 plain-text) + incremental sweep (quiet repos 1 GET/pass, lagging repos re-pass, create/activate arms). Tests: new sweep_cap_test.go (bound + retry + arming + edges), notify 95.9% cover, -race green.

Fix ready for review: PR #162 (https://git.packden.us/crueber/walhub/pulls/162) — branch fix/issue-156. Cap (20/repo, 409 plain-text) + incremental sweep (quiet repos 1 GET/pass, lagging repos re-pass, create/activate arms). Tests: new sweep_cap_test.go (bound + retry + arming + edges), notify 95.9% cover, -race green.

crueber commented

2026-09-05 21:36:51 +00:00

Author

Owner

PR #162 review (branch fix/issue-156, ccd3c3c + review fixup 1eb40f2):

CAP SEMANTICS — sound. Default 20 via maxHooks() (zero/negative fail open to default, never uncapped — notify.go). Over-cap CreateHook refuses with ErrConflict → 409 plain-text (statusFor mapping verified; HTTP test asserts code + text/plain). Grandfathering verified: cap enforced only in CreateHook; DeliverRepo/ListHooks have no cap check so pre-existing over-cap repos deliver all hooks, only new creates blocked. 25-hook seed bypass is the documented grandfathering path (test comment); added the same statement to 06 §1.4 in 1eb40f2 since the doc previously overclaimed the sweep bound for such repos. 64-hook fanout test opt-out (MaxHooks=65) legitimate — it measures goroutine bounding, not the cap. Minor note (not fixed): concurrent creates can TOCTOU-overshoot the cap by racers (check-then-act, no CAS) — benign for a cost bound.

WATERMARKS — correct. hookMu hold sites (tasks.go:302,327,344,364; webhooks.go:223,339,466) all touch only the two maps, never held across store/network I/O. Sweep↔delivery race walked (NextSeq = last-allocated, activity.go:50): every interleaving fails toward a pass — gate-skip then reserve → next sweep sees advance + wake fires; event lands mid-pass → finishHookPass sees delivered<head → pending → re-pass; reserved-but-unwritten → probeAhead misses → pending → re-pass; head-read failure/corrupt state → pending stays true / watermarks untouched → retry. finishHookPass converges (delivered==head clears pending; no spurious re-pass). DeleteHook dropping repo watermarks when sibling hooks remain is safe (fails toward one extra pass, self-heals).

QUIET SWEEP — really 1 GET. Gate = one collab_state GET; hookless/no-advance returns before ListHooks; repos without activity log never reach LIST. Test asserts exactly 1 GET + 0 LIST/HEAD/PUT/DEL (+2 shared enumeration prefix-LISTs) for a 25-hook drained repo. At-least-once preserved: failed delivery holds cursor, sets pending, next sweep re-passes (test asserts deliveries ring grows + pending stays). Fast path unchanged: emit still wakeRepo per event; armHookSweep adds non-blocking wake + pending backstop; only adds one head GET at end of pass. Inactive create arms nothing; (re-)activation arms; all-inactive healing pass covered.

VERIFY (scratch worktree /tmp/pr162, since removed): gofmt clean, go vet clean, go test -race ./internal/notify/... ok, new sweep/cap tests -count=10 ok, coverage 96.0% (≥95% gate holds). No import changes (zero new non-stdlib deps). Doc entries accurate after 1eb40f2.

FIXES PUSHED to origin/fix/issue-156 (1eb40f2): 06 §1.4 grandfathering sentence; removed dead mkConflict helper in sweep_cap_test.go. Re-tested after.

MERGE RECOMMENDATION: ready to merge.

PR #162 review (branch fix/issue-156, ccd3c3c + review fixup 1eb40f2): CAP SEMANTICS — sound. Default 20 via maxHooks() (zero/negative fail open to default, never uncapped — notify.go). Over-cap CreateHook refuses with ErrConflict → 409 plain-text (statusFor mapping verified; HTTP test asserts code + text/plain). Grandfathering verified: cap enforced only in CreateHook; DeliverRepo/ListHooks have no cap check so pre-existing over-cap repos deliver all hooks, only new creates blocked. 25-hook seed bypass is the documented grandfathering path (test comment); added the same statement to 06 §1.4 in 1eb40f2 since the doc previously overclaimed the sweep bound for such repos. 64-hook fanout test opt-out (MaxHooks=65) legitimate — it measures goroutine bounding, not the cap. Minor note (not fixed): concurrent creates can TOCTOU-overshoot the cap by racers (check-then-act, no CAS) — benign for a cost bound. WATERMARKS — correct. hookMu hold sites (tasks.go:302,327,344,364; webhooks.go:223,339,466) all touch only the two maps, never held across store/network I/O. Sweep↔delivery race walked (NextSeq = last-allocated, activity.go:50): every interleaving fails toward a pass — gate-skip then reserve → next sweep sees advance + wake fires; event lands mid-pass → finishHookPass sees delivered<head → pending → re-pass; reserved-but-unwritten → probeAhead misses → pending → re-pass; head-read failure/corrupt state → pending stays true / watermarks untouched → retry. finishHookPass converges (delivered==head clears pending; no spurious re-pass). DeleteHook dropping repo watermarks when sibling hooks remain is safe (fails toward one extra pass, self-heals). QUIET SWEEP — really 1 GET. Gate = one collab_state GET; hookless/no-advance returns before ListHooks; repos without activity log never reach LIST. Test asserts exactly 1 GET + 0 LIST/HEAD/PUT/DEL (+2 shared enumeration prefix-LISTs) for a 25-hook drained repo. At-least-once preserved: failed delivery holds cursor, sets pending, next sweep re-passes (test asserts deliveries ring grows + pending stays). Fast path unchanged: emit still wakeRepo per event; armHookSweep adds non-blocking wake + pending backstop; only adds one head GET at end of pass. Inactive create arms nothing; (re-)activation arms; all-inactive healing pass covered. VERIFY (scratch worktree /tmp/pr162, since removed): gofmt clean, go vet clean, go test -race ./internal/notify/... ok, new sweep/cap tests -count=10 ok, coverage 96.0% (≥95% gate holds). No import changes (zero new non-stdlib deps). Doc entries accurate after 1eb40f2. FIXES PUSHED to origin/fix/issue-156 (1eb40f2): 06 §1.4 grandfathering sentence; removed dead mkConflict helper in sweep_cap_test.go. Re-tested after. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 21:38:13 +00:00

crueber commented

2026-09-05 21:38:13 +00:00

Author

Owner

Fixed by PR #162 incl. review doc/test fixups (cap 20 + incremental sweep, grandfathering verified; 96.0% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 21:38:14 +00:00

Merge pull request 'Fix #156: hooks cap + bounded sweep' (#162) from fix/issue-156 into main

crueber added the

webhooks