fix(wfengine): worker-lifecycle hardening — e.stopped guard + generation/Stop docs (bookshelf-289dd) #1458

Merged

zombor merged 1 commit from bd-bookshelf-289dd into main 2026-08-28 18:09:31 +00:00

Conversation 3 Commits 1 Files changed 3 +78 -1

zombor commented 2026-08-28 17:30:18 +00:00

Owner

Copy link

Summary

Fixes a pre-existing P3 latent race in internal/wfengine: Engine.StartWorker only
checked e.workerStarted, not e.stopped, so a racing StartWorker call could start a
NEW worker generation after Engine.Stop() had begun closing wfDB/diagBackend.
StartWorker is now a permanent no-op once e.stopped is set.

Also documents (does not change behavior for) two pre-existing latent MINORs surfaced in
round-3 reviews of PR #1388 (bookshelf-81hq):

• engine.go's workerGen and worker_set.go's generation are two independent,
  uncoordinated counters at two layers. Rather than a risky unification for a P3, both
  sites now cross-reference the invariant between them in doc comments.
• Engine.Stop()'s doc now notes the accepted limitation that it does not guarantee an
  orphaned older-generation drain goroutine finishes before closing wfDB/diagBackend
  — this is intentional (a cross-generation test asserts the orphaned generation keeps
  running) and is now documented the way DrainWorker's doc documents its own behavior.

No workflow command-sequence changes — this is engine-internal worker lifecycle plumbing,
not a workflow definition, so no gowf.Version gate applies.

Test plan

• New failing-first Ginkgo spec in internal/wfengine/worker_lifecycle_test.go:
  StartWorker called after Stop() returns nil, does not invoke the underlying
  startWorker closure, and leaves workerStarted false.
• make test — full suite green.
• make lint — 0 issues, all policy checks pass.
• make coverage — internal/wfengine at 100.0%; check-coverage: OK.

Closes bead bookshelf-289dd on merge.

## Summary Fixes a pre-existing P3 latent race in `internal/wfengine`: `Engine.StartWorker` only checked `e.workerStarted`, not `e.stopped`, so a racing `StartWorker` call could start a NEW worker generation after `Engine.Stop()` had begun closing `wfDB`/`diagBackend`. `StartWorker` is now a permanent no-op once `e.stopped` is set. Also documents (does not change behavior for) two pre-existing latent MINORs surfaced in round-3 reviews of PR #1388 (bookshelf-81hq): - `engine.go`'s `workerGen` and `worker_set.go`'s `generation` are two independent, uncoordinated counters at two layers. Rather than a risky unification for a P3, both sites now cross-reference the invariant between them in doc comments. - `Engine.Stop()`'s doc now notes the accepted limitation that it does not guarantee an orphaned older-generation drain goroutine finishes before closing `wfDB`/`diagBackend` — this is intentional (a cross-generation test asserts the orphaned generation keeps running) and is now documented the way `DrainWorker`'s doc documents its own behavior. No workflow command-sequence changes — this is engine-internal worker lifecycle plumbing, not a workflow definition, so no `gowf.Version` gate applies. ## Test plan - New failing-first Ginkgo spec in `internal/wfengine/worker_lifecycle_test.go`: `StartWorker` called after `Stop()` returns nil, does not invoke the underlying `startWorker` closure, and leaves `workerStarted` false. - `make test` — full suite green. - `make lint` — 0 issues, all policy checks pass. - `make coverage` — `internal/wfengine` at 100.0%; `check-coverage: OK`. Closes bead bookshelf-289dd on merge.

zombor added 1 commit 2026-08-28 17:30:19 +00:00

fix(wfengine): worker-lifecycle hardening — e.stopped guard + generation/Stop docs (bookshelf-289dd) ... eb545281ab

StartWorker now checks e.stopped in addition to e.workerStarted, so a
racing StartWorker call that arrives after Engine.Stop has begun no
longer starts a new worker generation against pools Stop is closing.

Also documents two pre-existing, accepted latent MINORs from the
#1388/bookshelf-81hq review rounds:
  - engine.go's workerGen and worker_set.go's generation are two
    independent, uncoordinated counters at two layers; doc comments on
    both now cross-reference the invariant (or lack thereof) between
    them instead of unifying them (lower-risk for a P3).
  - Engine.Stop's doc now notes it does not guarantee an orphaned
    older-generation drain goroutine finishes before wfDB/diagBackend
    are closed — an accepted limitation already covered by a
    cross-generation test, mirroring how DrainWorker's doc documents
    its own generation-scoped behavior.

Closes bead bookshelf-289dd on merge.

zombor commented 2026-08-28 17:42:45 +00:00

Author

Owner

Copy link

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Reviewed the wfengine worker-lifecycle hardening diff (internal/wfengine/engine.go, internal/wfengine/worker_set.go, internal/wfengine/worker_lifecycle_test.go).

Summary: Clean. This is a genuine race-window fix, not new risk.

• Lock ordering / deadlock: StartWorker adds e.stopped || to the existing guard while holding e.mu, returning immediately with no additional lock acquisition in that branch. Stop() sets e.stopped = true under the same e.mu before releasing it and beginning the drain. No new lock is taken inside worker_set.go from this guarded path, so there is no lock-order inversion and no new deadlock/hang surface.
• Resource leak: the early return happens strictly before e.startWorker(ctx) is invoked, so no poller goroutine, Worker instance, or DB handle is created before the no-op return - nothing to unwind. This closes a real TOCTOU: previously a racing StartWorker could slip past Stop()'s lock release and start a worker generation against wfDB/diagBackend pools that Stop was about to close.
• Secrets/PII/auth: none - the change is internal lifecycle-only (no HTTP surface, no logging of sensitive data; all additions to engine.go/worker_set.go are doc comments except the one-line guard). Auth is untouched.
• Test (worker_lifecycle_test.go) exercises the regression directly (StartWorker after Stop is a no-op, underlying start never invoked, workerStarted stays false), matching project Ginkgo one-Expect-per-It convention.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor Reviewed the wfengine worker-lifecycle hardening diff (internal/wfengine/engine.go, internal/wfengine/worker_set.go, internal/wfengine/worker_lifecycle_test.go). Summary: Clean. This is a genuine race-window fix, not new risk. - Lock ordering / deadlock: StartWorker adds `e.stopped ||` to the existing guard while holding e.mu, returning immediately with no additional lock acquisition in that branch. Stop() sets e.stopped = true under the same e.mu before releasing it and beginning the drain. No new lock is taken inside worker_set.go from this guarded path, so there is no lock-order inversion and no new deadlock/hang surface. - Resource leak: the early return happens strictly before e.startWorker(ctx) is invoked, so no poller goroutine, Worker instance, or DB handle is created before the no-op return - nothing to unwind. This closes a real TOCTOU: previously a racing StartWorker could slip past Stop()'s lock release and start a worker generation against wfDB/diagBackend pools that Stop was about to close. - Secrets/PII/auth: none - the change is internal lifecycle-only (no HTTP surface, no logging of sensitive data; all additions to engine.go/worker_set.go are doc comments except the one-line guard). Auth is untouched. - Test (worker_lifecycle_test.go) exercises the regression directly (StartWorker after Stop is a no-op, underlying start never invoked, workerStarted stays false), matching project Ginkgo one-Expect-per-It convention. No findings.

zombor commented 2026-08-28 17:42:54 +00:00

Author

Owner

Copy link

Code Review: PR #1458 (bookshelf-289dd)

Reviewed the diff only (engine.go, worker_set.go, worker_lifecycle_test.go). No re-run of tests; CI is source of behavioral truth.

Phase 1 — Spec compliance: matches the bead exactly — e.stopped guard added to StartWorker (engine.go:701-706), doc notes added for the two independent generation counters (engine.go:145-158, worker_set.go:94-101) and Stop()'s orphan-drain limitation (engine.go:810-819). No extra scope.

Phase 2 — Code quality:

1. Concurrency correctness (checked): e.stopped and e.workerStarted/e.workerGen are read/written inside the same e.mu.Lock()/Unlock() critical section in StartWorker (engine.go:701-712), and Stop() sets e.stopped=true under the same e.mu (engine.go:822-826) before releasing the lock. No TOCTOU — a racing StartWorker either observes stopped=true and no-ops, or wins the lock first and completes its state transition before Stop can flip stopped. Correct.

2. No workflow command-sequence change — confirmed via grep: no ExecuteActivity/CreateSubWorkflowInstance/gowf.Select/gowf.Sleep/ContinueAsNew touched in this diff. Purely engine-internal lifecycle guard; no gowf.Version gate needed, correctly omitted.

3. Guard semantics: post-Stop StartWorker silently returns nil, consistent with the existing workerStarted idempotency no-op one line above it (engine.go:702). All three production call sites (cmd/pergamum/worker.go:153, internal/app/app.go:1210, internal/app/app.go:1401) call StartWorker only at startup, before Stop is ever invoked, so the silent no-op doesn't mask a real caller misuse today. Acceptable.

4. Test quality (worker_lifecycle_test.go:257-299): the new Context("when StartWorker is called after Stop") calls engine.Stop() in BeforeEach, then StartWorker in JustBeforeEach, and asserts via atomic.Int32 startCalls that the injected startWorker stub was never invoked, plus WorkerStarted() stays false. Verified this fails without the guard: removing e.stopped || from the if would let StartWorker call the stub (startCalls → 1, workerStarted → true), failing 2 of the 3 Its. The third It ("returns nil without error") is weak in isolation (the stub itself returns nil, so it can't distinguish guarded vs unguarded) but the other two Its in the same Context provide real regression coverage — not a can't-fail test overall. One assertion per It, matches package Ginkgo style. NewTestEngine positional args used correctly.

5. Docs: both new comment blocks (engine.go:145-158 on workerGen, worker_set.go:94-101 on generation, and the Stop() ACCEPTED LIMITATION block at engine.go:810-819) accurately describe the real invariant per the source read — workerGen increments on every StartWorker call, workerSet.generation only increments when start() finds drained==true, and Stop's bounded drain doesn't force-join an orphaned older-generation drain goroutine. Matches the code and the bead's stated limitation. Not misleading.

No blockers, no majors found.

[MINOR] internal/wfengine/worker_lifecycle_test.go:290 — the "returns nil without error" It is self-validating in isolation (stub always returns nil regardless of the guard) — harmless here since sibling Its in the same Context carry the real regression signal, but worth noting for future copy-paste.

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review: PR #1458 (bookshelf-289dd) Reviewed the diff only (engine.go, worker_set.go, worker_lifecycle_test.go). No re-run of tests; CI is source of behavioral truth. **Phase 1 — Spec compliance:** matches the bead exactly — `e.stopped` guard added to `StartWorker` (engine.go:701-706), doc notes added for the two independent generation counters (engine.go:145-158, worker_set.go:94-101) and Stop()'s orphan-drain limitation (engine.go:810-819). No extra scope. **Phase 2 — Code quality:** 1. **Concurrency correctness (checked):** `e.stopped` and `e.workerStarted`/`e.workerGen` are read/written inside the same `e.mu.Lock()`/`Unlock()` critical section in `StartWorker` (engine.go:701-712), and `Stop()` sets `e.stopped=true` under the same `e.mu` (engine.go:822-826) before releasing the lock. No TOCTOU — a racing `StartWorker` either observes `stopped=true` and no-ops, or wins the lock first and completes its state transition before `Stop` can flip `stopped`. Correct. 2. **No workflow command-sequence change** — confirmed via grep: no `ExecuteActivity`/`CreateSubWorkflowInstance`/`gowf.Select`/`gowf.Sleep`/`ContinueAsNew` touched in this diff. Purely engine-internal lifecycle guard; no `gowf.Version` gate needed, correctly omitted. 3. **Guard semantics:** post-Stop `StartWorker` silently returns `nil`, consistent with the existing `workerStarted` idempotency no-op one line above it (engine.go:702). All three production call sites (`cmd/pergamum/worker.go:153`, `internal/app/app.go:1210`, `internal/app/app.go:1401`) call `StartWorker` only at startup, before `Stop` is ever invoked, so the silent no-op doesn't mask a real caller misuse today. Acceptable. 4. **Test quality (worker_lifecycle_test.go:257-299):** the new `Context("when StartWorker is called after Stop")` calls `engine.Stop()` in `BeforeEach`, then `StartWorker` in `JustBeforeEach`, and asserts via `atomic.Int32` `startCalls` that the injected `startWorker` stub was never invoked, plus `WorkerStarted()` stays false. Verified this fails without the guard: removing `e.stopped ||` from the `if` would let `StartWorker` call the stub (`startCalls` → 1, `workerStarted` → true), failing 2 of the 3 `It`s. The third `It` ("returns nil without error") is weak in isolation (the stub itself returns nil, so it can't distinguish guarded vs unguarded) but the other two `It`s in the same `Context` provide real regression coverage — not a can't-fail test overall. One assertion per `It`, matches package Ginkgo style. `NewTestEngine` positional args used correctly. 5. **Docs:** both new comment blocks (engine.go:145-158 on `workerGen`, worker_set.go:94-101 on `generation`, and the `Stop()` ACCEPTED LIMITATION block at engine.go:810-819) accurately describe the real invariant per the source read — `workerGen` increments on every `StartWorker` call, `workerSet.generation` only increments when `start()` finds `drained==true`, and Stop's bounded drain doesn't force-join an orphaned older-generation drain goroutine. Matches the code and the bead's stated limitation. Not misleading. No blockers, no majors found. [MINOR] internal/wfengine/worker_lifecycle_test.go:290 — the "returns nil without error" `It` is self-validating in isolation (stub always returns nil regardless of the guard) — harmless here since sibling `It`s in the same `Context` carry the real regression signal, but worth noting for future copy-paste. REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-28 17:42:55 +00:00

Author

Owner

Copy link

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Reviewed the wfengine worker-lifecycle hardening diff (internal/wfengine/engine.go, internal/wfengine/worker_set.go, internal/wfengine/worker_lifecycle_test.go).

Summary: Clean. This is a genuine race-window fix, not new risk.

• Lock ordering / deadlock: StartWorker adds e.stopped || to the existing guard while holding e.mu, returning immediately with no additional lock acquisition in that branch. Stop() sets e.stopped = true under the same e.mu before releasing it and beginning the drain. No new lock is taken inside worker_set.go from this guarded path, so there is no lock-order inversion and no new deadlock/hang surface.
• Resource leak: the early return happens strictly before e.startWorker(ctx) is invoked, so no poller goroutine, Worker instance, or DB handle is created before the no-op return - nothing to unwind. This closes a real TOCTOU: previously a racing StartWorker could slip past Stop()'s lock release and start a worker generation against wfDB/diagBackend pools that Stop was about to close.
• Secrets/PII/auth: none - the change is internal lifecycle-only (no HTTP surface, no logging of sensitive data; all additions to engine.go/worker_set.go are doc comments except the one-line guard). Auth is untouched.
• Test (worker_lifecycle_test.go) exercises the regression directly (StartWorker after Stop is a no-op, underlying start never invoked, workerStarted stays false), matching project Ginkgo one-Expect-per-It convention.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor Reviewed the wfengine worker-lifecycle hardening diff (internal/wfengine/engine.go, internal/wfengine/worker_set.go, internal/wfengine/worker_lifecycle_test.go). Summary: Clean. This is a genuine race-window fix, not new risk. - Lock ordering / deadlock: StartWorker adds `e.stopped ||` to the existing guard while holding e.mu, returning immediately with no additional lock acquisition in that branch. Stop() sets e.stopped = true under the same e.mu before releasing it and beginning the drain. No new lock is taken inside worker_set.go from this guarded path, so there is no lock-order inversion and no new deadlock/hang surface. - Resource leak: the early return happens strictly before e.startWorker(ctx) is invoked, so no poller goroutine, Worker instance, or DB handle is created before the no-op return - nothing to unwind. This closes a real TOCTOU: previously a racing StartWorker could slip past Stop()'s lock release and start a worker generation against wfDB/diagBackend pools that Stop was about to close. - Secrets/PII/auth: none - the change is internal lifecycle-only (no HTTP surface, no logging of sensitive data; all additions to engine.go/worker_set.go are doc comments except the one-line guard). Auth is untouched. - Test (worker_lifecycle_test.go) exercises the regression directly (StartWorker after Stop is a no-op, underlying start never invoked, workerStarted stays false), matching project Ginkgo one-Expect-per-It convention. No findings.

zombor merged commit 13ff97a871 into main 2026-08-28 18:09:31 +00:00

zombor referenced this pull request from a commit 2026-08-28 18:09:34 +00:00

Merge pull request 'fix(wfengine): worker-lifecycle hardening — e.stopped guard + generation/Stop docs (bookshelf-289dd)' (#1458) from bd-bookshelf-289dd into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1458

No description provided.