[omp major] Task goroutines ignore ctx cancel #154

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] Task goroutines ignore ctx cancel

internal/notify/tasks.go:285-289, :330: StartWebhooks uses WithoutCancel, drainFanout uses Background(); neither runs in a tracked WaitGroup — a wedged store call hangs them forever, immune to drain/shutdown.

Fix

Thread the phase-1/drain context through (same shape as the #74 import fix); track in a WaitGroup or make hangs impossible (deadlines on store work). Regression test (wedged store + drain → prompt exit). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

Drain/shutdown terminates wedged task goroutines; test green -race.

# [omp major] Task goroutines ignore ctx cancel `internal/notify/tasks.go:285-289`, `:330`: `StartWebhooks` uses `WithoutCancel`, `drainFanout` uses `Background()`; neither runs in a tracked WaitGroup — a wedged store call hangs them forever, immune to drain/shutdown. ## Fix Thread the phase-1/drain context through (same shape as the #74 import fix); track in a WaitGroup or make hangs impossible (deadlines on store work). Regression test (wedged store + drain → prompt exit). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] Drain/shutdown terminates wedged task goroutines; test green `-race`.

crueber referenced this issue from a commit

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

Fix #154: cancellable notify task leaders (drainCtx + WaitGroup)

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

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

Fix #154: cancellable notify tasks #160

crueber commented

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

Author

Owner

Fixed by #160 (branch fix/issue-154): notify task leaders now run on the service drainCtx (cancelled by new Service.Drain, wired into serve.go phase 1 beside importSvc.Drain — same shape as the #74 import fix) and are tracked in Service.wg; new tasks refuse fast once draining. Regression tests (wedged store + drain → prompt exit) verified hanging pre-fix. Suite green -race, coverage 96.1%, gofmt/vet clean.

crueber referenced this issue from a commit

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

Review #160: DeliverRepo waits for in-flight hooks on drain-cancel

crueber commented

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

Author

Owner

PR #160 review (branch fix/issue-154, commit 83f0de4 + review fixup 1057838):

All six checks pass. Verified in scratch worktree /tmp/pr160 (since removed): go test -race ./internal/notify/... ./cmd/... green, notify coverage 96.1% (>=95 gate holds), gofmt/vet clean, no new third-party imports (only first-party internal/store in the new test).

Force-end vs #72 hole: SAFE. Force-end (tasks.end on dead drainCtx, tasks.go:376) only fires when ctx dead, i.e. process is draining and no worker will ever run again here. Every seq that could be dropped in-memory is redrain-durable: overflow/shortfall paths appendActivity(pending=true) BEFORE enqueueFanout (emit.go:230->236, 255->265), sweep path only enqueues verified FanoutPending+no-completion-record seqs (tasks.go:577-583). The in-flight seq resolves safe either way: fanoutOne on dead ctx reads nothing (existed=false, gap path, no marker) or fails recipients (complete=false, no marker per #152); markFanoutDone(ctx) with dead ctx fails silently so no false completion claim. Restart sweep re-drives idempotently (deterministic ids + Create-412). #72's live-process hole does not reopen.
enqueueFanout drop when draining: durable, confirmed ordering above (append precedes enqueue attempt in both emit paths). Sync-complete path (pending=false) never enqueues. Safe.
Mid-backlog re-attach (tasks.go:386-388): verified present; harmless (lands on the entry force-ended at loop top, in-memory lost but sweep-durable). Effectively a no-op — noted as nit, not a bug.
Drain idempotent + non-blocking: Drain() sets bool under drainMu, unlocks, then cancels (notify.go) — never calls cancel under lock, takes no task locks, so double-Drain and Drain-from-task cannot deadlock. wg Add-before-spawn + deferred Done on both leaders (tasks.go:303-308, 336-343). serve.go phase-1 order sane: notifySvc.Drain() beside importSvc.Drain(), before maintainerDone wait, after reg.Tasks().Drain() (serve.go:267-269). Observation: prod never Waits on notify wg (cancel-only, mirrors importSvc); acceptable since all dropped work is sweep-durable, but a bounded Wait in phase 1 would make shutdown completeness explicit.
Refuse-fast callers safe: StartWebhooks->nil callers (tasks.go:233,281) ignore return as statements; enqueueFanout is void and all callers ignore it. No nil deref introduced.
Tests genuine + docs accurate: pre-fix leaders ran on WithoutCancel (tasks.go:286 on main) / Background() (:330) with no Drain method and no serve wiring (main serve.go has no notifySvc.Drain) — a ctx-honoring wedged store hangs them past the test's 10s waitTaskFinished bound, so the new tests fail pre-fix / pass post-fix by construction. Doc entry (06_notifications.md) claims verified against code: WithoutCancel/Background history, wg tracking, refuse-fast, fanout_pending durability, force-end exception, per-seq ctx re-check — all accurate.

Small fix pushed by reviewer (1057838): DeliverRepo's ctx.Done arm used bare return, orphaning in-flight hook workers past wg.Wait (arm was dead code pre-fix under WithoutCancel, live now). Changed to labeled break so the loop stops launching but still Waits; workers observe dead ctx and fail fast, wait stays bounded. Re-tested race+coverage after.

MERGE RECOMMENDATION: ready to merge.

PR #160 review (branch fix/issue-154, commit 83f0de4 + review fixup 1057838): All six checks pass. Verified in scratch worktree /tmp/pr160 (since removed): go test -race ./internal/notify/... ./cmd/... green, notify coverage 96.1% (>=95 gate holds), gofmt/vet clean, no new third-party imports (only first-party internal/store in the new test). 1. Force-end vs #72 hole: SAFE. Force-end (tasks.end on dead drainCtx, tasks.go:376) only fires when ctx dead, i.e. process is draining and no worker will ever run again here. Every seq that could be dropped in-memory is redrain-durable: overflow/shortfall paths appendActivity(pending=true) BEFORE enqueueFanout (emit.go:230->236, 255->265), sweep path only enqueues verified FanoutPending+no-completion-record seqs (tasks.go:577-583). The in-flight seq resolves safe either way: fanoutOne on dead ctx reads nothing (existed=false, gap path, no marker) or fails recipients (complete=false, no marker per #152); markFanoutDone(ctx) with dead ctx fails silently so no false completion claim. Restart sweep re-drives idempotently (deterministic ids + Create-412). #72's live-process hole does not reopen. 2. enqueueFanout drop when draining: durable, confirmed ordering above (append precedes enqueue attempt in both emit paths). Sync-complete path (pending=false) never enqueues. Safe. 3. Mid-backlog re-attach (tasks.go:386-388): verified present; harmless (lands on the entry force-ended at loop top, in-memory lost but sweep-durable). Effectively a no-op — noted as nit, not a bug. 4. Drain idempotent + non-blocking: Drain() sets bool under drainMu, unlocks, then cancels (notify.go) — never calls cancel under lock, takes no task locks, so double-Drain and Drain-from-task cannot deadlock. wg Add-before-spawn + deferred Done on both leaders (tasks.go:303-308, 336-343). serve.go phase-1 order sane: notifySvc.Drain() beside importSvc.Drain(), before maintainerDone wait, after reg.Tasks().Drain() (serve.go:267-269). Observation: prod never Waits on notify wg (cancel-only, mirrors importSvc); acceptable since all dropped work is sweep-durable, but a bounded Wait in phase 1 would make shutdown completeness explicit. 5. Refuse-fast callers safe: StartWebhooks->nil callers (tasks.go:233,281) ignore return as statements; enqueueFanout is void and all callers ignore it. No nil deref introduced. 6. Tests genuine + docs accurate: pre-fix leaders ran on WithoutCancel (tasks.go:286 on main) / Background() (:330) with no Drain method and no serve wiring (main serve.go has no notifySvc.Drain) — a ctx-honoring wedged store hangs them past the test's 10s waitTaskFinished bound, so the new tests fail pre-fix / pass post-fix by construction. Doc entry (06_notifications.md) claims verified against code: WithoutCancel/Background history, wg tracking, refuse-fast, fanout_pending durability, force-end exception, per-seq ctx re-check — all accurate. Small fix pushed by reviewer (1057838): DeliverRepo's ctx.Done arm used bare return, orphaning in-flight hook workers past wg.Wait (arm was dead code pre-fix under WithoutCancel, live now). Changed to labeled break so the loop stops launching but still Waits; workers observe dead ctx and fail fast, wait stays bounded. Re-tested race+coverage after. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

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

crueber referenced this issue from a commit

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

Merge pull request 'Fix #154: cancellable notify tasks' (#160) from fix/issue-154 into main

crueber commented

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

Author

Owner

Fixed by PR #160 incl. review DeliverRepo break fix (#72 interaction verified safe; 96.1% coverage), merged. Closing.

crueber added the

webhooks