flake(wfengine): extend tester timeout so deadlock-detection stops false-firing under CI load (bookshelf-v8o1o) #1462

Merged

zombor merged 1 commit from bd-bookshelf-v8o1o into main 2026-08-31 01:12:42 +00:00

Conversation 3 Commits 1 Files changed 28 +264 -241

zombor commented 2026-08-30 00:07:37 +00:00

Owner

Copy link

Root cause

The go-workflows tester's deadlock-detection heuristic (tester.go:458-470 in
github.com/cschleiden/go-workflows/tester@v1.4.1) waits TestTimeout
(default 10s wall-clock) for the next event before panicking with
"No new events generated during workflow execution and no pending timers, workflow blocked?". Activities are executed on real goroutines
(scheduleActivity) even though workflow time is simulated via a mock clock,
so under CI CPU starvation that 10s wall-clock budget can be exhausted even
though the workflow is NOT actually deadlocked — producing an intermittent
false-positive panic. Confirmed by reading tester.go around line 470 and
tester/options.go (WithTestTimeout directly sets options.TestTimeout,
the value read at line 458).

Observed on CI Coverage job for PR #1461 (internal/wfengine
TemplateGenerateWorkflow "when activity fails returns an error from the
workflow" spec). Already documented in-code that the error-propagation specs
use large RetryOptions timers, but the timers themselves are not the
problem — this is the tester's real-time idle-wait budget, not workflow
simulated time.

Fix

Added a shared newWorkflowTester[TResult] wrapper in
internal/wfengine/test_helpers_test.go that passes
tester.WithTestTimeout(60 * time.Second) to every tester construction, then
mechanically switched all 240 call sites across 26 internal/wfengine/*_test.go
files from tester.NewWorkflowTester[T](...) to newWorkflowTester[T](...)
(same call syntax, so every existing test keeps its exact assertions — no
weakening, no deleted specs, no shrunk retry timers). replay_golden_test.go
had its now-unused tester import removed.

Test plan

• go vet ./internal/wfengine/... — clean
• go test ./internal/wfengine/... — 2072 specs pass
• ginkgo -repeat=2 ./internal/wfengine/... with GOMAXPROCS=1 (simulates CI
  CPU starvation) — 2072 specs x2 runs, all green, no tester panics
• make test, make lint — pass
• make coverage — 100% on internal/wfengine (and repo-wide gate passes)

Closes bead bookshelf-v8o1o on merge.

## Root cause The go-workflows tester's deadlock-detection heuristic (`tester.go:458-470` in `github.com/cschleiden/go-workflows/tester@v1.4.1`) waits `TestTimeout` (default 10s **wall-clock**) for the next event before panicking with `"No new events generated during workflow execution and no pending timers, workflow blocked?"`. Activities are executed on real goroutines (`scheduleActivity`) even though workflow time is simulated via a mock clock, so under CI CPU starvation that 10s wall-clock budget can be exhausted even though the workflow is NOT actually deadlocked — producing an intermittent false-positive panic. Confirmed by reading `tester.go` around line 470 and `tester/options.go` (`WithTestTimeout` directly sets `options.TestTimeout`, the value read at line 458). Observed on CI Coverage job for PR #1461 (`internal/wfengine` `TemplateGenerateWorkflow` "when activity fails returns an error from the workflow" spec). Already documented in-code that the error-propagation specs use large `RetryOptions` timers, but the timers themselves are not the problem — this is the tester's real-time idle-wait budget, not workflow simulated time. ## Fix Added a shared `newWorkflowTester[TResult]` wrapper in `internal/wfengine/test_helpers_test.go` that passes `tester.WithTestTimeout(60 * time.Second)` to every tester construction, then mechanically switched all 240 call sites across 26 `internal/wfengine/*_test.go` files from `tester.NewWorkflowTester[T](...)` to `newWorkflowTester[T](...)` (same call syntax, so every existing test keeps its exact assertions — no weakening, no deleted specs, no shrunk retry timers). `replay_golden_test.go` had its now-unused `tester` import removed. ## Test plan - `go vet ./internal/wfengine/...` — clean - `go test ./internal/wfengine/...` — 2072 specs pass - `ginkgo -repeat=2 ./internal/wfengine/...` with `GOMAXPROCS=1` (simulates CI CPU starvation) — 2072 specs x2 runs, all green, no tester panics - `make test`, `make lint` — pass - `make coverage` — 100% on `internal/wfengine` (and repo-wide gate passes) Closes bead bookshelf-v8o1o on merge.

zombor added 1 commit 2026-08-30 00:07:38 +00:00

flake(wfengine): extend go-workflows tester idle-wait timeout (bookshelf-v8o1o) ... 92758b2c3e

The tester's deadlock-detection heuristic (tester.go:458-470 in
github.com/cschleiden/go-workflows/tester) waits TestTimeout (default 10s
wall-clock) for the next event before panicking with "No new events
generated ... workflow blocked?". Activities run on real goroutines even
under the simulated workflow clock, so under CI CPU starvation that 10s
budget can be exhausted even though the workflow isn't actually deadlocked
-- causing an intermittent false-positive panic on error-propagation specs
(observed on the Coverage job, e.g. TemplateGenerateWorkflow "when activity
fails returns an error from the workflow").

Fix: add a shared newWorkflowTester[T] wrapper in test_helpers_test.go that
passes tester.WithTestTimeout(60*time.Second) to every wfengine tester
construction, and switch all 240 call sites (26 files) from
tester.NewWorkflowTester[T] to it. No assertions were weakened -- every
error-propagation spec still asserts the workflow returns the error.

Verified: `go test ./internal/wfengine/...` and `ginkgo -repeat=2` (2072
specs x2, GOMAXPROCS=1 to simulate CPU starvation) both green; make test,
make lint, make coverage (100% internal/wfengine) all pass.

zombor commented 2026-08-30 00:15:26 +00:00

Author

Owner

Copy link

Security review (adversarial, read-only) — PR #1462 (bd-bookshelf-v8o1o)

Scope confirmed: all 28 changed files match internal/wfengine/*_test.go; no production code (internal/, cmd/, etc. outside *_test.go) is touched. Diffed non-mechanical lines and confirmed the ONLY substantive additions are in the new test_helpers_test.go: a testerIdleTimeout = 60 * time.Second constant and a newWorkflowTester[TResult] wrapper around tester.NewWorkflowTester that appends tester.WithTestTimeout(testerIdleTimeout). Every other changed line across the 27 other files is a 1:1 mechanical substitution of tester.NewWorkflowTester[...](...) → newWorkflowTester[...](...) — no Expect/assertion lines, stub behavior, retry policy, permanent-error classification, version-gating, or fan-out-bound assertions were touched.

Checks:

1. Change confined to test files — CONFIRMED. git diff --name-only origin/main...origin/bd-bookshelf-v8o1o returns only internal/wfengine/*_test.go paths.
2. No test weakened/skipped — CONFIRMED. This only extends the tester's idle-wait budget (a wall-clock generosity to reduce CI false positives under CPU starvation), not a shortened timeout or loosened assertion. No Skip/XIt/PIt/removed Expect found. Black-box discipline preserved (package wfengine_test, per project convention).
3. No secrets/credentials introduced — CONFIRMED. New helper file adds no config/credentials, only a time constant and a slog test-capture helper (moved to the same file).

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review (adversarial, read-only) — PR #1462 (bd-bookshelf-v8o1o) Scope confirmed: all 28 changed files match `internal/wfengine/*_test.go`; no production code (`internal/`, `cmd/`, etc. outside `*_test.go`) is touched. Diffed non-mechanical lines and confirmed the ONLY substantive additions are in the new `test_helpers_test.go`: a `testerIdleTimeout = 60 * time.Second` constant and a `newWorkflowTester[TResult]` wrapper around `tester.NewWorkflowTester` that appends `tester.WithTestTimeout(testerIdleTimeout)`. Every other changed line across the 27 other files is a 1:1 mechanical substitution of `tester.NewWorkflowTester[...](...)` → `newWorkflowTester[...](...)` — no `Expect`/assertion lines, stub behavior, retry policy, permanent-error classification, version-gating, or fan-out-bound assertions were touched. Checks: 1. Change confined to test files — CONFIRMED. `git diff --name-only origin/main...origin/bd-bookshelf-v8o1o` returns only `internal/wfengine/*_test.go` paths. 2. No test weakened/skipped — CONFIRMED. This only extends the tester's idle-wait budget (a wall-clock generosity to reduce CI false positives under CPU starvation), not a shortened timeout or loosened assertion. No `Skip`/`XIt`/`PIt`/removed `Expect` found. Black-box discipline preserved (`package wfengine_test`, per project convention). 3. No secrets/credentials introduced — CONFIRMED. New helper file adds no config/credentials, only a time constant and a slog test-capture helper (moved to the same file). No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-30 00:15:32 +00:00

Author

Owner

Copy link

Security review (adversarial, read-only) — PR #1462 (bd-bookshelf-v8o1o)

Scope confirmed: all 28 changed files match internal/wfengine/*_test.go; no production code (internal/, cmd/, etc. outside *_test.go) is touched. Diffed non-mechanical lines and confirmed the ONLY substantive additions are in the new test_helpers_test.go: a testerIdleTimeout = 60 * time.Second constant and a newWorkflowTester[TResult] wrapper around tester.NewWorkflowTester that appends tester.WithTestTimeout(testerIdleTimeout). Every other changed line across the 27 other files is a 1:1 mechanical substitution of tester.NewWorkflowTester[...](...) → newWorkflowTester[...](...) — no Expect/assertion lines, stub behavior, retry policy, permanent-error classification, version-gating, or fan-out-bound assertions were touched.

Checks:

1. Change confined to test files — CONFIRMED. git diff --name-only origin/main...origin/bd-bookshelf-v8o1o returns only internal/wfengine/*_test.go paths.
2. No test weakened/skipped — CONFIRMED. This only extends the tester's idle-wait budget (a wall-clock generosity to reduce CI false positives under CPU starvation), not a shortened timeout or loosened assertion. No Skip/XIt/PIt/removed Expect found. Black-box discipline preserved (package wfengine_test, per project convention).
3. No secrets/credentials introduced — CONFIRMED. New helper file adds no config/credentials, only a time constant and a slog test-capture helper (moved to the same file).

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review (adversarial, read-only) — PR #1462 (bd-bookshelf-v8o1o) Scope confirmed: all 28 changed files match `internal/wfengine/*_test.go`; no production code (`internal/`, `cmd/`, etc. outside `*_test.go`) is touched. Diffed non-mechanical lines and confirmed the ONLY substantive additions are in the new `test_helpers_test.go`: a `testerIdleTimeout = 60 * time.Second` constant and a `newWorkflowTester[TResult]` wrapper around `tester.NewWorkflowTester` that appends `tester.WithTestTimeout(testerIdleTimeout)`. Every other changed line across the 27 other files is a 1:1 mechanical substitution of `tester.NewWorkflowTester[...](...)` → `newWorkflowTester[...](...)` — no `Expect`/assertion lines, stub behavior, retry policy, permanent-error classification, version-gating, or fan-out-bound assertions were touched. Checks: 1. Change confined to test files — CONFIRMED. `git diff --name-only origin/main...origin/bd-bookshelf-v8o1o` returns only `internal/wfengine/*_test.go` paths. 2. No test weakened/skipped — CONFIRMED. This only extends the tester's idle-wait budget (a wall-clock generosity to reduce CI false positives under CPU starvation), not a shortened timeout or loosened assertion. No `Skip`/`XIt`/`PIt`/removed `Expect` found. Black-box discipline preserved (`package wfengine_test`, per project convention). 3. No secrets/credentials introduced — CONFIRMED. New helper file adds no config/credentials, only a time constant and a slog test-capture helper (moved to the same file). No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-30 00:19:21 +00:00

Author

Owner

Copy link

Code Review: PR #1462 (bd-bookshelf-v8o1o)

Reviewed diff only (per policy, CI is behavioral truth -- did not re-run tests).

Summary: Purely mechanical test-only fix. Introduces newWorkflowTester[TResult] in test_helpers_test.go (package wfengine_test) that wraps tester.NewWorkflowTester[TResult] and appends tester.WithTestTimeout(60*time.Second) via variadic opts = append(opts, ...). All 240 call sites across 26 files renamed tester.NewWorkflowTester[T] -> newWorkflowTester[T], with no other change. Diff stat matches the bead description exactly: 28 files, +264/-241, entirely within internal/wfengine/.

Verification performed

• Confirmed tester.options.TestTimeout default is 10s (tester.go:197) and the timeout is a per-iteration idle-wait budget reset every time a new event/callback/timer fires (tester.go:436-471 in go-workflows@v1.4.2), not a hard wall-clock for the whole test. So the 60s change cannot silently extend a genuinely deadlocked test past 60s of actual idleness -- it only tolerates CPU-starvation-induced idle gaps between real events. This matches the in-code rationale comment.
• Confirmed the wrapper is a pure passthrough: variadic opts are preserved and only the timeout option is appended (append-after, not prepend/replace), and since WorkflowTesterOption funcs are applied in order with last-write-wins semantics on TestTimeout, appending after caller-supplied opts means the wrapper's 60s always "wins" for TestTimeout specifically -- no other option field is affected.
• Spot-checked pre-existing call sites that already passed an explicit tester.WithTestTimeout(60*time.Second) (bulk_workflow_test.go:809, recalc_scores_workflow_test.go x8, scan_workflow_test.go:340, simple_workflows_test.go:305) -- all already used the same 60s value, so the append is a harmless no-op duplicate at those sites, not a conflict.
• Grepped every changed file's diff for lines other than the NewWorkflowTester renames (variable names wt, wt2, wt3, wtEmpty) -- zero unrelated edits found (no assertion changes, no skipped specs, no Register/Execute alterations).
• replay_golden_test.go's removed "github.com/cschleiden/go-workflows/tester" import is correctly dead after all 15 call sites in that file switched to the local helper; rest of import list left intact.
• newWorkflowTester lives in a _test.go file under package wfengine_test (black-box convention) -- not exported to production code.
• Confirmed the diff touches only internal/wfengine/*_test.go -- no production code, no other package.

Findings

[MINOR] internal/wfengine/bulk_workflow_test.go:119, recalc_scores_workflow_test.go:186,195,204,213,222,231,240,249,258, scan_workflow_test.go:316, simple_workflows_test.go:374 -- now-redundant explicit tester.WithTestTimeout(60*time.Second) option
These call sites still pass an explicit tester.WithTestTimeout(60*time.Second) alongside the new newWorkflowTester wrapper, which now also appends the same 60s value. Functionally harmless (last-applied-wins, same value), but it's dead/confusing code post-refactor -- a future reader may wonder whether it's double-counted. Suggest a fast-follow cleanup PR to drop the now-redundant explicit options at these ~12 sites, keeping only cases that ever need a different timeout than the shared default (none currently do).

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## Code Review: PR #1462 (bd-bookshelf-v8o1o) Reviewed diff only (per policy, CI is behavioral truth -- did not re-run tests). **Summary:** Purely mechanical test-only fix. Introduces `newWorkflowTester[TResult]` in `test_helpers_test.go` (package `wfengine_test`) that wraps `tester.NewWorkflowTester[TResult]` and appends `tester.WithTestTimeout(60*time.Second)` via variadic `opts = append(opts, ...)`. All 240 call sites across 26 files renamed `tester.NewWorkflowTester[T]` -> `newWorkflowTester[T]`, with no other change. Diff stat matches the bead description exactly: 28 files, +264/-241, entirely within `internal/wfengine/`. ### Verification performed - Confirmed `tester.options.TestTimeout` default is 10s (`tester.go:197`) and the timeout is a **per-iteration idle-wait budget** reset every time a new event/callback/timer fires (`tester.go:436-471` in `go-workflows@v1.4.2`), not a hard wall-clock for the whole test. So the 60s change cannot silently extend a genuinely deadlocked test past 60s of *actual* idleness -- it only tolerates CPU-starvation-induced idle gaps between real events. This matches the in-code rationale comment. - Confirmed the wrapper is a pure passthrough: variadic `opts` are preserved and only the timeout option is appended (append-after, not prepend/replace), and since `WorkflowTesterOption` funcs are applied in order with last-write-wins semantics on `TestTimeout`, appending after caller-supplied opts means the wrapper's 60s always "wins" for `TestTimeout` specifically -- no other option field is affected. - Spot-checked pre-existing call sites that already passed an explicit `tester.WithTestTimeout(60*time.Second)` (bulk_workflow_test.go:809, recalc_scores_workflow_test.go x8, scan_workflow_test.go:340, simple_workflows_test.go:305) -- all already used the same 60s value, so the append is a harmless no-op duplicate at those sites, not a conflict. - Grepped every changed file's diff for lines other than the `NewWorkflowTester` renames (variable names `wt`, `wt2`, `wt3`, `wtEmpty`) -- zero unrelated edits found (no assertion changes, no skipped specs, no Register/Execute alterations). - `replay_golden_test.go`'s removed `"github.com/cschleiden/go-workflows/tester"` import is correctly dead after all 15 call sites in that file switched to the local helper; rest of import list left intact. - `newWorkflowTester` lives in a `_test.go` file under `package wfengine_test` (black-box convention) -- not exported to production code. - Confirmed the diff touches only `internal/wfengine/*_test.go` -- no production code, no other package. ### Findings [MINOR] internal/wfengine/bulk_workflow_test.go:119, recalc_scores_workflow_test.go:186,195,204,213,222,231,240,249,258, scan_workflow_test.go:316, simple_workflows_test.go:374 -- now-redundant explicit `tester.WithTestTimeout(60*time.Second)` option These call sites still pass an explicit `tester.WithTestTimeout(60*time.Second)` alongside the new `newWorkflowTester` wrapper, which now also appends the same 60s value. Functionally harmless (last-applied-wins, same value), but it's dead/confusing code post-refactor -- a future reader may wonder whether it's double-counted. Suggest a fast-follow cleanup PR to drop the now-redundant explicit options at these ~12 sites, keeping only cases that ever need a *different* timeout than the shared default (none currently do). REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor merged commit 574a112ed0 into main 2026-08-31 01:12:42 +00:00

zombor referenced this pull request from a commit 2026-08-31 01:12:46 +00:00

Merge pull request 'flake(wfengine): extend tester timeout so deadlock-detection stops false-firing under CI load (bookshelf-v8o1o)' (#1462) from bd-bookshelf-v8o1o 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!1462

No description provided.