[omp major] Unbounded goroutine spawn: acquire-after-`go` in fan-out paths #153

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] Unbounded goroutine spawn: acquire-after-`go` in fan-out paths

internal/notify/emit.go:322-342, tasks.go:364-389, webhooks.go:360-376: one goroutine per recipient/hook spawned BEFORE the cap-8 semaphore acquire; recipient set and hook count are uncapped. A burst (large team mention, many hooks) spawns unbounded goroutines first and throttles second.

Fix

Acquire before go (or errgroup SetLimit): bound in-flight fan-out work by construction. Regression test (large recipient set → bounded goroutine delta). Coverage gate holds.

Acceptance criteria

Goroutine count bounded independent of recipient/hook count; test green -race.

# [omp major] Unbounded goroutine spawn: acquire-after-`go` in fan-out paths `internal/notify/emit.go:322-342`, `tasks.go:364-389`, `webhooks.go:360-376`: one goroutine per recipient/hook spawned BEFORE the cap-8 semaphore acquire; recipient set and hook count are uncapped. A burst (large team mention, many hooks) spawns unbounded goroutines first and throttles second. ## Fix Acquire before `go` (or errgroup `SetLimit`): bound in-flight fan-out work by construction. Regression test (large recipient set → bounded goroutine delta). Coverage gate holds. ## Acceptance criteria - [ ] Goroutine count bounded independent of recipient/hook count; test green `-race`.

crueber referenced this issue from a commit

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

Fix #153: acquire fan-out slot before spawn (bound goroutines by construction)

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

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

Fix #153: bound fan-out goroutines #159

crueber commented

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

Author

Owner

Fix ready for review: #159 (branch fix/issue-153). Acquire-before-spawn on resolve/createAll/fanoutOne/DeliverRepo; regression tests fail pre-fix (deltas 201/201/201/89) and pass after; package suite green -race, coverage 96.0%.

crueber commented

2026-09-05 21:01:34 +00:00

Author

Owner

Review of PR #159 (fix/issue-153, 0704ebf): acquire-before-go on all four fan-out sites.

Acquire-before-go + release on every path — PASS (all four):

internal/notify/emit.go:325-329 (resolve): parent select sem<- vs ctx.Done before wg.Add/go; worker releases via defer <-sem (emit.go:333). All returns (actor/empty/author/invalid gates) run the defers; panic still unwinds defers so no slot leak. ctx.Done path holds no slot (continue, nothing to release).
internal/notify/emit.go:476-483 (createAll): same shape; parent ctx.Done path takes mu only for failed=true (no slot held); worker defer-release at :487 covers createOne returns + ctx.Err early return.
internal/notify/tasks.go:390-395 (fanoutOne): parent select sem<- vs fctx.Done + fail() before spawn; worker defer-release at :399 covers fctx.Err/fail/createFailed/publish paths.
internal/notify/webhooks.go:368-372 (DeliverRepo): parent select sem<- vs ctx.Done + return; worker defer-release at :376 covers all deliverHook returns.
No leaked-slot path found: every acquired slot has exactly one paired deferred release in the spawned goroutine; every unacquired path releases nothing.

Budget-expiry accounting preserved — PASS: compared against origin/main. createAll pre-fix counted semaphore-stranded workers failed inside the goroutine; post-fix counts never-launched recipients failed in the parent (same failed=true). fanoutOne identical via fail() on both parent-expiry and worker fctx.Err paths. DeliverRepo best-effort skip semantics unchanged (unlaunched hooks abandoned either way). resolve has no failed flag; dropping remaining recipients on request-cancel (vs pre-fix blocking sem<- with no ctx escape, which leaked) is strictly better.
No deadlock — PASS: no site holds any lock across the parent acquire (mu taken only briefly for failed++/add, never across sem<-; team-expansion path continues before acquire). sem is per-call local, never re-acquired by createOne/deliverHook/validPrincipal (no nested acquire, no pool-exhaustion cycle). Callers (emit :243 with fctx, drainFanout tasks.go:332 with Background, StartWebhooks tasks.go:287 detached ctx) hold no repo/task locks across the call. notify takes no syncMu/packMu/rw at all. Parent acquire always has a ctx/fctx escape, so no indefinite block.
chan-sem vs errgroup — SOUND: keeps the existing primitive (minimal diff, same cap-8 bound §4 permits both flavors). One doc nit (non-blocking): docs/features/06_notifications.md entry says 'the §4 errgroup.SetLimit(8) bound now holds literally' but the code is a chan-sem implementing the same limit — conceptually accurate, mechanism wording slightly loose. Left as-is; not worth a churn push.
Negative controls — GENUINE by construction: pre-fix resolve used blocking sem<- with no ctx escape, so all N goroutines provably coexisted (200 parked/blocked); createAll/fanoutOne spawned N before any select; DeliverRepo spawned 64. Deltas ~200/200/200/64+slack vs bound 40 (fanout_bound_test.go:fanoutGoroutineBound) fail pre-fix, pass fixed (~8 + slack). Claimed 201/201/201/89 all exceed 40 — plausible, wide margin either way.
Hygiene — PASS: coverage 96.0% (gate >=95%); no import-block changes in emit.go/tasks.go/webhooks.go/notify.go; test file imports stdlib + internal/identity + internal/store only (no new third-party; dependency budget intact). gofmt clean, go vet clean. Sender-owns-closes respected (sem never closed — correct for a counting semaphore; wg owns completion). wg.Add precedes go, Wait after loop at all sites.

Tests (scratch worktree /tmp/pr159 @0704ebf, removed after): go vet ./internal/notify/... clean; gofmt -l clean; go test -race -run TestFanout.*Bounded -count=5 PASS; full go test -race ./internal/notify/... PASS; coverage 96.0%.

MERGE RECOMMENDATION: ready to merge.

Review of PR #159 (fix/issue-153, 0704ebf): acquire-before-go on all four fan-out sites. 1) Acquire-before-go + release on every path — PASS (all four): - internal/notify/emit.go:325-329 (resolve): parent select sem<- vs ctx.Done before wg.Add/go; worker releases via defer <-sem (emit.go:333). All returns (actor/empty/author/invalid gates) run the defers; panic still unwinds defers so no slot leak. ctx.Done path holds no slot (continue, nothing to release). - internal/notify/emit.go:476-483 (createAll): same shape; parent ctx.Done path takes mu only for failed=true (no slot held); worker defer-release at :487 covers createOne returns + ctx.Err early return. - internal/notify/tasks.go:390-395 (fanoutOne): parent select sem<- vs fctx.Done + fail() before spawn; worker defer-release at :399 covers fctx.Err/fail/createFailed/publish paths. - internal/notify/webhooks.go:368-372 (DeliverRepo): parent select sem<- vs ctx.Done + return; worker defer-release at :376 covers all deliverHook returns. No leaked-slot path found: every acquired slot has exactly one paired deferred release in the spawned goroutine; every unacquired path releases nothing. 2) Budget-expiry accounting preserved — PASS: compared against origin/main. createAll pre-fix counted semaphore-stranded workers failed inside the goroutine; post-fix counts never-launched recipients failed in the parent (same failed=true). fanoutOne identical via fail() on both parent-expiry and worker fctx.Err paths. DeliverRepo best-effort skip semantics unchanged (unlaunched hooks abandoned either way). resolve has no failed flag; dropping remaining recipients on request-cancel (vs pre-fix blocking sem<- with no ctx escape, which leaked) is strictly better. 3) No deadlock — PASS: no site holds any lock across the parent acquire (mu taken only briefly for failed++/add, never across sem<-; team-expansion path continues before acquire). sem is per-call local, never re-acquired by createOne/deliverHook/validPrincipal (no nested acquire, no pool-exhaustion cycle). Callers (emit :243 with fctx, drainFanout tasks.go:332 with Background, StartWebhooks tasks.go:287 detached ctx) hold no repo/task locks across the call. notify takes no syncMu/packMu/rw at all. Parent acquire always has a ctx/fctx escape, so no indefinite block. 4) chan-sem vs errgroup — SOUND: keeps the existing primitive (minimal diff, same cap-8 bound §4 permits both flavors). One doc nit (non-blocking): docs/features/06_notifications.md entry says 'the §4 errgroup.SetLimit(8) bound now holds literally' but the code is a chan-sem implementing the same limit — conceptually accurate, mechanism wording slightly loose. Left as-is; not worth a churn push. 5) Negative controls — GENUINE by construction: pre-fix resolve used blocking sem<- with no ctx escape, so all N goroutines provably coexisted (200 parked/blocked); createAll/fanoutOne spawned N before any select; DeliverRepo spawned 64. Deltas ~200/200/200/64+slack vs bound 40 (fanout_bound_test.go:fanoutGoroutineBound) fail pre-fix, pass fixed (~8 + slack). Claimed 201/201/201/89 all exceed 40 — plausible, wide margin either way. 6) Hygiene — PASS: coverage 96.0% (gate >=95%); no import-block changes in emit.go/tasks.go/webhooks.go/notify.go; test file imports stdlib + internal/identity + internal/store only (no new third-party; dependency budget intact). gofmt clean, go vet clean. Sender-owns-closes respected (sem never closed — correct for a counting semaphore; wg owns completion). wg.Add precedes go, Wait after loop at all sites. Tests (scratch worktree /tmp/pr159 @0704ebf, removed after): go vet ./internal/notify/... clean; gofmt -l clean; go test -race -run TestFanout.*Bounded -count=5 PASS; full go test -race ./internal/notify/... PASS; coverage 96.0%. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 21:03:29 +00:00

crueber commented

2026-09-05 21:03:30 +00:00

Author

Owner

Fixed by PR #159 (review: release-on-all-paths + no-deadlock + genuine controls verified; 96.0% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 21:03:30 +00:00

Merge pull request 'Fix #153: bound fan-out goroutines' (#159) from fix/issue-153 into main

crueber added the

webhooks