[omp major] `PingHook` runs full backlog in the request goroutine #155

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] `PingHook` runs full backlog in the request goroutine

internal/notify/webhooks.go:665: ping replays the full backlog synchronously — 256 events × 10s POST ≈ 43 min worst case, holding the admin's HTTP request open and racing background delivery for the same hook.

Fix

Bound the ping (e.g. single synthetic ping event through the normal loop, or a capped recent-window replay with a short deadline), return promptly. Regression test (backlog present → ping returns fast, delivery proves URL+secret end to end). Coverage gate holds.

Acceptance criteria

Ping returns promptly regardless of backlog depth; test green -race.

# [omp major] `PingHook` runs full backlog in the request goroutine `internal/notify/webhooks.go:665`: ping replays the full backlog synchronously — 256 events × 10s POST ≈ 43 min worst case, holding the admin's HTTP request open and racing background delivery for the same hook. ## Fix Bound the ping (e.g. single synthetic `ping` event through the normal loop, or a capped recent-window replay with a short deadline), return promptly. Regression test (backlog present → ping returns fast, delivery proves URL+secret end to end). Coverage gate holds. ## Acceptance criteria - [ ] Ping returns promptly regardless of backlog depth; test green `-race`.

crueber referenced this issue from a commit

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

notify: bound PingHook to one synthetic event POST (fix #155)

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

2026-09-05 21:19:15 +00:00

Fix #155: bounded webhook ping #161

crueber commented

2026-09-05 21:19:18 +00:00

Author

Owner

Fix ready for review: PR #161 (branch fix/issue-155). PingHook now POSTs exactly the synthetic ping event instead of replaying the backlog in the request goroutine. internal/notify: -race green, coverage 96.0%, gofmt/vet clean.

crueber commented

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

Author

Owner

Reviewed PR #161 (e21a40c, fix for #155: bounded webhook ping) in scratch worktree /tmp/pr161 (removed afterward). Main worktree untouched (read-only).

FINDINGS — all review criteria hold, no fixes needed:

Ping contract preserved (internal/notify/webhooks.go:660-693): PingHook appends the synthetic ping event then calls the same s.postEvent path as deliverHook (same wire shape, X-Walgit-Delivery/X-Walgit-Event keeper headers, HMAC-SHA256 when secret set, same hookClientFor, same recordDelivery ring). TestPingBoundedWithDeepBacklog asserts HMAC + keeper headers on the wire. URL+secret proof unchanged. PASS
Backlog untouched (webhooks.go:686-688): cursor CAS only when readCursor == seq-1 (i.e. ping is the head). Deep-backlog test asserts cursor stays 0 with 300 queued events. Ping reserves its seq via the same CAS two-step so it slots at the head without disturbing backlog order; background deliverHook later delivers backlog + ping (ping bypasses the events filter, webhooks.go:411), deduped by X-Walgit-Delivery — at-least-once, never a skip. No shared mutation with the loop besides the CAS ring/log appends; the old deliverHook race in the request goroutine is gone. PASS
No-backlog cursor advance (webhooks.go:686-688, advanceCursor is monotonic CAS): TestPingAdvancesCursorWithoutBacklog asserts cursor moves 0->1 and a follow-up DeliverRepo produces no second POST (sink.count()==1). PASS
Failure shape unchanged: sink/transport failure returns (false, nil) with detail on the deliveries ring (webhooks.go:683-685); errors only for bad hook/config (unknown/inactive/corrupt seq). TestPingDeliveryFailure covers HTTP-500 + refused-port; TestPingCorruptSeqState covers seq failure. http.go untouched so the API {delivery:false} mapping is unchanged. PASS
Prompt return proven two ways: exactly-one-POST count assertion (deterministic, load-bearing) plus elapsed < 10s bound. New test runs in ~0.07s with a 300-event backlog against a 50ms sink (old code would replay 256 x 50ms ~= 13s). PASS
No new imports: PR diff adds zero import lines (grep of added quoted imports empty); production change reuses postEvent/recordDelivery/readCursor/advanceCursor. PASS
Coverage: internal/notify 96.1% statements with change included (PingHook itself 95.8%) — gate holds. gofmt clean, go vet clean. PASS
Doc entries accurate: docs/features/06_notifications.md §5.3 ping paragraph (append + single postEvent POST, no backlog replay, filter bypass in loop, no-backlog cursor advance) matches the code; Decisions entry (bounded ping, no deliverHook, monotonic no-backlog advance) matches. PASS
Pre-existing ping tests still pass: TestPingStoreErrors, TestPingEdges, TestPingBypassesEventFilter — filter bypass and store-error paths preserved.

VERIFICATION (scratch worktree @ e21a40c): go test -race -count=1 ./internal/notify/... -> ok (2.4s); -coverprofile -> 96.1% total; gofmt -l clean; go vet clean; -run TestPing -v -> all 7 PASS. No fixes pushed (nothing to fix). NOT merged, per instructions.

MERGE RECOMMENDATION: ready to merge.

Reviewed PR #161 (e21a40c, fix for #155: bounded webhook ping) in scratch worktree /tmp/pr161 (removed afterward). Main worktree untouched (read-only). FINDINGS — all review criteria hold, no fixes needed: 1. Ping contract preserved (internal/notify/webhooks.go:660-693): PingHook appends the synthetic ping event then calls the same s.postEvent path as deliverHook (same wire shape, X-Walgit-Delivery/X-Walgit-Event keeper headers, HMAC-SHA256 when secret set, same hookClientFor, same recordDelivery ring). TestPingBoundedWithDeepBacklog asserts HMAC + keeper headers on the wire. URL+secret proof unchanged. PASS 2. Backlog untouched (webhooks.go:686-688): cursor CAS only when readCursor == seq-1 (i.e. ping is the head). Deep-backlog test asserts cursor stays 0 with 300 queued events. Ping reserves its seq via the same CAS two-step so it slots at the head without disturbing backlog order; background deliverHook later delivers backlog + ping (ping bypasses the events filter, webhooks.go:411), deduped by X-Walgit-Delivery — at-least-once, never a skip. No shared mutation with the loop besides the CAS ring/log appends; the old deliverHook race in the request goroutine is gone. PASS 3. No-backlog cursor advance (webhooks.go:686-688, advanceCursor is monotonic CAS): TestPingAdvancesCursorWithoutBacklog asserts cursor moves 0->1 and a follow-up DeliverRepo produces no second POST (sink.count()==1). PASS 4. Failure shape unchanged: sink/transport failure returns (false, nil) with detail on the deliveries ring (webhooks.go:683-685); errors only for bad hook/config (unknown/inactive/corrupt seq). TestPingDeliveryFailure covers HTTP-500 + refused-port; TestPingCorruptSeqState covers seq failure. http.go untouched so the API {delivery:false} mapping is unchanged. PASS 5. Prompt return proven two ways: exactly-one-POST count assertion (deterministic, load-bearing) plus elapsed < 10s bound. New test runs in ~0.07s with a 300-event backlog against a 50ms sink (old code would replay 256 x 50ms ~= 13s). PASS 6. No new imports: PR diff adds zero import lines (grep of added quoted imports empty); production change reuses postEvent/recordDelivery/readCursor/advanceCursor. PASS 7. Coverage: internal/notify 96.1% statements with change included (PingHook itself 95.8%) — gate holds. gofmt clean, go vet clean. PASS 8. Doc entries accurate: docs/features/06_notifications.md §5.3 ping paragraph (append + single postEvent POST, no backlog replay, filter bypass in loop, no-backlog cursor advance) matches the code; Decisions entry (bounded ping, no deliverHook, monotonic no-backlog advance) matches. PASS 9. Pre-existing ping tests still pass: TestPingStoreErrors, TestPingEdges, TestPingBypassesEventFilter — filter bypass and store-error paths preserved. VERIFICATION (scratch worktree @ e21a40c): go test -race -count=1 ./internal/notify/... -> ok (2.4s); -coverprofile -> 96.1% total; gofmt -l clean; go vet clean; -run TestPing -v -> all 7 PASS. No fixes pushed (nothing to fix). NOT merged, per instructions. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 21:20:58 +00:00

crueber referenced this issue from a commit

2026-09-05 21:20:58 +00:00

Merge pull request 'Fix #155: bounded webhook ping' (#161) from fix/issue-155 into main

crueber commented

2026-09-05 21:20:58 +00:00

Author

Owner

Fixed by PR #161 (review: contract preserved, backlog untouched, prompt return proven; 96.1% coverage), merged. Closing.

crueber added the

webhooks