fix(test): eliminate two wfengine integration flakes (bookshelf-7dfl + bookshelf-19lt) #397

Merged

zombor merged 2 commits from bd-bookshelf-7dfl into main 2026-06-07 11:30:29 +00:00

Conversation 2 Commits 2 Files changed 2 +21 -3

zombor commented 2026-06-07 02:21:59 +00:00

Owner

Copy link

Summary

Two flaky integration tests in internal/wfengine have been burning ~6 CI re-triggers per day on unrelated PRs.

Flake 1: diag_accessor_integration_test.go — WaitForCompletion hang (bookshelf-7dfl)

Root cause: DeferCleanup is LIFO. The test registered engine.Stop() and cancel() as separate entries, so Stop() ran before cancel(). WaitForCompletion blocks until the worker's context is cancelled and all poller goroutines exit; calling it before cancel() meant waiting for the full 3-second context timeout plus the 30-second poll() long-poll timeout. On a loaded CI runner this produced a 9+ minute hang (and job failure by overall CI timeout).

Fix: Combined cancel() and engine.Stop() into a single DeferCleanup closure that cancels the context first, then calls Stop(). Pollers exit immediately on cancel; WaitForCompletion returns in milliseconds.

Flake 2: bulk_workflow_test.go:601 — tester panic under CPU starvation (bookshelf-19lt)

Root cause: The go-workflows tester has a hardcoded 10-second idle-panic timer (tester.go:458). Under CPU starvation on a single-core CI runner, the callback delivering the activity-failure result was delayed past 10 seconds, causing the tester to panic("workflow blocked?") at tester.go:470.

Fix: Pass tester.WithTestTimeout(60*time.Second) to the BulkMetadataWorkflow NewWorkflowTester call. The tester API provides this option exactly for slow/loaded environments.

Test plan

• make test — green
• make integration — green
• make coverage — 100.0% gate passes
• go vet + golangci-lint --new-from-rev=origin/main ./internal/wfengine/... — 0 issues
• go build -tags e2e ./e2e/... — builds clean
• Stress run: 20 repeats of Engine.ListInstances + Engine.GetInstanceState integration specs with GOMAXPROCS=2 — 260 executions, 0 failures
• Stress run: 50 repeats of BulkMetadataWorkflow (all 5 specs) with GOMAXPROCS=2 — 250 executions, 0 failures

Closes beads bookshelf-7dfl and bookshelf-19lt on merge.

## Summary Two flaky integration tests in `internal/wfengine` have been burning ~6 CI re-triggers per day on unrelated PRs. ### Flake 1: `diag_accessor_integration_test.go` — WaitForCompletion hang (bookshelf-7dfl) **Root cause:** `DeferCleanup` is LIFO. The test registered `engine.Stop()` and `cancel()` as separate entries, so `Stop()` ran before `cancel()`. `WaitForCompletion` blocks until the worker's context is cancelled and all poller goroutines exit; calling it before `cancel()` meant waiting for the full 3-second context timeout *plus* the 30-second `poll()` long-poll timeout. On a loaded CI runner this produced a 9+ minute hang (and job failure by overall CI timeout). **Fix:** Combined `cancel()` and `engine.Stop()` into a single `DeferCleanup` closure that cancels the context first, then calls `Stop()`. Pollers exit immediately on cancel; `WaitForCompletion` returns in milliseconds. ### Flake 2: `bulk_workflow_test.go:601` — tester panic under CPU starvation (bookshelf-19lt) **Root cause:** The go-workflows tester has a hardcoded 10-second idle-panic timer (`tester.go:458`). Under CPU starvation on a single-core CI runner, the callback delivering the activity-failure result was delayed past 10 seconds, causing the tester to `panic("workflow blocked?")` at `tester.go:470`. **Fix:** Pass `tester.WithTestTimeout(60*time.Second)` to the `BulkMetadataWorkflow` `NewWorkflowTester` call. The tester API provides this option exactly for slow/loaded environments. ## Test plan - [x] `make test` — green - [x] `make integration` — green - [x] `make coverage` — 100.0% gate passes - [x] `go vet` + `golangci-lint --new-from-rev=origin/main ./internal/wfengine/...` — 0 issues - [x] `go build -tags e2e ./e2e/...` — builds clean - [x] Stress run: 20 repeats of `Engine.ListInstances` + `Engine.GetInstanceState` integration specs with GOMAXPROCS=2 — 260 executions, 0 failures - [x] Stress run: 50 repeats of `BulkMetadataWorkflow` (all 5 specs) with GOMAXPROCS=2 — 250 executions, 0 failures Closes beads bookshelf-7dfl and bookshelf-19lt on merge.

zombor added 1 commit 2026-06-07 02:22:00 +00:00

fix(test): eliminate two wfengine integration test flakes (bookshelf-7dfl + bookshelf-19lt) ... 201cd5f5a5

Flake 1 (bookshelf-7dfl) — diag_accessor_integration_test.go:
Root cause: DeferCleanup entries were registered in separate calls, leaving
engine.Stop() and cancel() as independent LIFO entries where Stop ran before
cancel. WaitForCompletion blocks until the context's poller goroutines exit;
calling it before cancel() meant we had to wait for the full 3-second context
timeout (plus the 30-second long-poll timeout inside the poller), which under
a loaded CI runner caused a 9+ minute hang.
Fix: combine cancel+Stop into a single DeferCleanup closure that calls cancel()
first, then engine.Stop(). The pollers exit immediately on cancel; Stop returns
promptly with no in-flight tasks.

Flake 2 (bookshelf-19lt) — bulk_workflow_test.go:
Root cause: the go-workflows tester has a hardcoded 10-second idle-panic timer
(tester.go:458). Under CPU starvation on a single-core CI runner, the callback
delivering the activity-failure result to the workflow event loop was delayed
past 10 seconds, causing tester.go:470 to panic with "workflow blocked?".
Fix: pass tester.WithTestTimeout(60*time.Second) to the BulkMetadataWorkflow
NewWorkflowTester call, giving the failure-path event delivery enough headroom
on a contended runner.

Stress-tested: 20 repeats of the diag integration suite (13 specs × 20 = 260
executions), 50 repeats of BulkMetadataWorkflow (5 specs × 50 = 250 executions),
all green with GOMAXPROCS=2.

Closes beads bookshelf-7dfl and bookshelf-19lt on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-06-07 02:41:18 +00:00

ci: re-trigger after Forgejo 503/500 outage ... 3672e586c3

The prior run (SHA 201cd5f5) recorded Integration + E2E API + E2E Browser
failures that coincided exactly with a git.zombor.net 503→500 outage; the three
fast jobs that finished before the outage (Lint, JS Unit, Test) passed. Empty
commit to re-run CI on a healthy server. No code change.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor force-pushed bd-bookshelf-7dfl from 3672e586c3 to a16ffede65 2026-06-07 02:42:11 +00:00 Compare

zombor commented 2026-06-07 02:51:27 +00:00

Author

Owner

Copy link

Security Review — bd-bookshelf-7dfl (PR #397)

Diff scope: internal/wfengine/bulk_workflow_test.go, internal/wfengine/diag_accessor_integration_test.go — two test files only.

Findings

No security findings. Specifically verified:

• No production code touched (changes are 100% test-file-only, both files carry _test.go suffix).
• No secrets, tokens, API keys, or credentials introduced in fixtures.
• No new external network calls added (no HTTP clients, no dial, no external addresses).
• No security-relevant assertions weakened or removed — the test changes only adjust timeout headroom (WithTestTimeout(60*time.Second)) and DeferCleanup ordering (LIFO consolidation for race-free teardown). Neither change relaxes any security boundary.
• The timeout increase (10s → 60s) is test-infrastructure-only and guards against CI CPU-starvation flakes; it does not touch any production timeout or auth path.
• The DeferCleanup ordering fix correctly sequences cancel() before engine.Stop(), which is more correct (stops pollers before waiting on the engine), not less secure.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

**Security Review — bd-bookshelf-7dfl (PR #397)** Diff scope: `internal/wfengine/bulk_workflow_test.go`, `internal/wfengine/diag_accessor_integration_test.go` — two test files only. **Findings** No security findings. Specifically verified: - No production code touched (changes are 100% test-file-only, both files carry `_test.go` suffix). - No secrets, tokens, API keys, or credentials introduced in fixtures. - No new external network calls added (no HTTP clients, no dial, no external addresses). - No security-relevant assertions weakened or removed — the test changes only adjust timeout headroom (`WithTestTimeout(60*time.Second)`) and DeferCleanup ordering (LIFO consolidation for race-free teardown). Neither change relaxes any security boundary. - The timeout increase (10s → 60s) is test-infrastructure-only and guards against CI CPU-starvation flakes; it does not touch any production timeout or auth path. - The DeferCleanup ordering fix correctly sequences `cancel()` before `engine.Stop()`, which is more correct (stops pollers before waiting on the engine), not less secure. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-06-07 02:52:40 +00:00

Author

Owner

Copy link

Code Review — PR #397 (bookshelf-7dfl + bookshelf-19lt wfengine flake fixes)

Reviewed diff: origin/main...origin/bd-bookshelf-7dfl (head a16ffede). CI green. Test-only changes verified.

---

Claim 1 — diag_accessor_integration_test.go: DeferCleanup LIFO ordering (bookshelf-7dfl)

Verified correct. The fix combines cancel() + engine.Stop() into a single DeferCleanup closure, with cancel() first. This is the right ordering: cancelling the context stops the pollers so engine.Stop() / WaitForCompletion can return promptly instead of blocking until the 3-second timeout expires with pollers still active.

Sibling-bug sweep: all other integration tests that call StartWorker already use a single combined closure pattern:

• engine_integration_test.go lines 138–146, 217–225: combined cancel() + Stop() in one closure ✓
• scan_integration_test.go lines 166–179: combined cancel() + Stop() ✓

The second Describe block in diag_accessor_integration_test.go (line 119) keeps a standalone DeferCleanup(func() { _ = engine.Stop() }) — this is NOT the same bug. That block does not call StartWorker and has no context to cancel, so no combined closure is needed.

No split-DeferCleanup survivors remain in the package.

---

Claim 2 — bulk_workflow_test.go: WithTestTimeout(60s) on BulkMetadataWorkflow tester (bookshelf-19lt)

Verified correct. The timeout is applied to the single NewWorkflowTester call in the BulkMetadataWorkflow Describe block (line 573), which is the only tester that covers both the success path AND the failure path (via stubErr = errors.New("save failed") in a nested Context). The 60s headroom matches the CI comment: the activity-failure path must propagate through the go-workflows event loop before the default 10s idle panic fires on a loaded single-core runner.

Missed-sibling scan of all NewWorkflowTester call sites with failure paths:

Other workflow testers that exercise failure paths (a failing activity returning an error):

• EnrichWorkflow (simple_workflows_test.go:177): stubErr = errors.New("enrich error") → still on default 10s.
• DeleteBooksWorkflow (simple_workflows_test.go:264): stubErr = errors.New("delete failed") → still on default 10s.
• TemplateGenerateWorkflow (simple_workflows_test.go:76): stubErr = errors.New("template error") → still on default 10s.
• LibraryScanWorkflow — list activity error (scan_workflow_test.go:194): errors.New("list db failure") → still on default 10s.
• BulkCoversWorkflow — list activity error (bulk_workflow_test.go:184): errors.New("list failure") → still on default 10s.
• BulkEnrichWorkflow — list activity error (bulk_workflow_test.go:400): errors.New("db timeout") → still on default 10s.

The question is why BulkMetadataWorkflow was singled out. Looking at the bead description: "bookshelf-19lt" specifically targeted the metadata-workflow CI failure. The other failure-path testers listed above have presumably not flaked in CI under the same conditions — they may exercise shallower event-loop paths or simpler retry configurations. Without evidence that they share the same flake pattern, extending the timeout to them speculatively would be untargeted noise.

Assessment: not a missed sibling in the same category as the fix. The fix is targeted to the known flaking test. Other failure-path testers that haven't flaked in CI do not require treatment on this PR.

---

Standard checks

• Production code changed: none — diff is purely test files. ✓
• No skipped/deleted assertions: no Skip, no removed Expect. ✓
• No widened suite timeouts: NodeTimeout/SpecTimeout unchanged; only per-tester idle timeout increased for the specific flaking tester. ✓
• No sleeps added: the existing time.Sleep(200 * time.Millisecond) in diag_accessor_integration_test.go line 64 predates this PR. ✓
• 100% coverage gate: no exclusions added, no coverage configuration touched. ✓
• Ginkgo conventions (changed lines only): one Expect per It on new assertions — not applicable (no new It blocks added). ✓
• Vacuous/self-validating patterns: none. ✓

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Code Review — PR #397 (bookshelf-7dfl + bookshelf-19lt wfengine flake fixes) Reviewed diff: `origin/main...origin/bd-bookshelf-7dfl` (head a16ffede). CI green. Test-only changes verified. --- ### Claim 1 — `diag_accessor_integration_test.go`: DeferCleanup LIFO ordering (bookshelf-7dfl) **Verified correct.** The fix combines `cancel()` + `engine.Stop()` into a single `DeferCleanup` closure, with `cancel()` first. This is the right ordering: cancelling the context stops the pollers so `engine.Stop()` / `WaitForCompletion` can return promptly instead of blocking until the 3-second timeout expires with pollers still active. Sibling-bug sweep: all other integration tests that call `StartWorker` already use a single combined closure pattern: - `engine_integration_test.go` lines 138–146, 217–225: combined `cancel()` + `Stop()` in one closure ✓ - `scan_integration_test.go` lines 166–179: combined `cancel()` + `Stop()` ✓ The second `Describe` block in `diag_accessor_integration_test.go` (line 119) keeps a standalone `DeferCleanup(func() { _ = engine.Stop() })` — this is NOT the same bug. That block does not call `StartWorker` and has no context to cancel, so no combined closure is needed. **No split-DeferCleanup survivors remain in the package.** --- ### Claim 2 — `bulk_workflow_test.go`: `WithTestTimeout(60s)` on `BulkMetadataWorkflow` tester (bookshelf-19lt) **Verified correct.** The timeout is applied to the single `NewWorkflowTester` call in the `BulkMetadataWorkflow` Describe block (line 573), which is the only tester that covers both the success path AND the failure path (via `stubErr = errors.New("save failed")` in a nested `Context`). The 60s headroom matches the CI comment: the activity-failure path must propagate through the go-workflows event loop before the default 10s idle panic fires on a loaded single-core runner. **Missed-sibling scan of all `NewWorkflowTester` call sites with failure paths:** Other workflow testers that exercise failure paths (a failing activity returning an error): - `EnrichWorkflow` (simple_workflows_test.go:177): `stubErr = errors.New("enrich error")` → still on default 10s. - `DeleteBooksWorkflow` (simple_workflows_test.go:264): `stubErr = errors.New("delete failed")` → still on default 10s. - `TemplateGenerateWorkflow` (simple_workflows_test.go:76): `stubErr = errors.New("template error")` → still on default 10s. - `LibraryScanWorkflow — list activity error` (scan_workflow_test.go:194): `errors.New("list db failure")` → still on default 10s. - `BulkCoversWorkflow — list activity error` (bulk_workflow_test.go:184): `errors.New("list failure")` → still on default 10s. - `BulkEnrichWorkflow — list activity error` (bulk_workflow_test.go:400): `errors.New("db timeout")` → still on default 10s. The question is why `BulkMetadataWorkflow` was singled out. Looking at the bead description: "bookshelf-19lt" specifically targeted the metadata-workflow CI failure. The other failure-path testers listed above have presumably not flaked in CI under the same conditions — they may exercise shallower event-loop paths or simpler retry configurations. Without evidence that they share the same flake pattern, extending the timeout to them speculatively would be untargeted noise. **Assessment:** not a missed sibling in the same category as the fix. The fix is targeted to the known flaking test. Other failure-path testers that haven't flaked in CI do not require treatment on this PR. --- ### Standard checks - **Production code changed:** none — diff is purely test files. ✓ - **No skipped/deleted assertions:** no `Skip`, no removed `Expect`. ✓ - **No widened suite timeouts:** `NodeTimeout`/`SpecTimeout` unchanged; only per-tester idle timeout increased for the specific flaking tester. ✓ - **No sleeps added:** the existing `time.Sleep(200 * time.Millisecond)` in `diag_accessor_integration_test.go` line 64 predates this PR. ✓ - **100% coverage gate:** no exclusions added, no coverage configuration touched. ✓ - **Ginkgo conventions (changed lines only):** one Expect per It on new assertions — not applicable (no new It blocks added). ✓ - **Vacuous/self-validating patterns:** none. ✓ --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor merged commit 612c7dff92 into main 2026-06-07 11:30:29 +00:00

zombor referenced this pull request from a commit 2026-06-07 11:30:31 +00:00

Merge pull request 'fix(test): eliminate two wfengine integration flakes (bookshelf-7dfl + bookshelf-19lt)' (#397) from bd-bookshelf-7dfl into main

zombor referenced this pull request 2026-08-11 13:38:43 +00:00

De-flake wfengine transient cover-retry spec (bookshelf-di2zy) #1432

zombor referenced this pull request from a commit 2026-08-12 20:25:43 +00:00

fix(wfengine): guard EnrichWorkflow activity-failure test against tester idle-panic

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!397

No description provided.