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

Merged

zombor merged 1 commit from bd-bookshelf-di2zy into main 2026-08-11 13:55:35 +00:00

Conversation 2 Commits 1 Files changed 1 +12 -1

zombor commented 2026-08-11 13:18:23 +00:00

Owner

Copy link

Summary

• Fixes an intermittent Integration-job panic in internal/wfengine:
  [PANICKED!] LibraryScanWorkflow — transient cover failure retried [JustBeforeEach] returns no error.
• Root cause: this spec's cover activity genuinely retries (transient error →
  MaxAttempts=3 backoff), driving more executor events/goroutines through
  go-workflows' in-memory WorkflowTester than a single-attempt spec. The
  tester's default 10s idle timeout is a real wall-clock timer racing the
  event loop (tester.go Execute: time.NewTimer(TestTimeout) vs the
  callback channel) — under CPU starvation on a loaded CI runner the gap
  between events can exceed 10s even though nothing is actually deadlocked,
  triggering the panic.
• Same root cause + same fix already applied to BulkMetadataWorkflow's
  retry-heavy spec (bookshelf-19lt): pass tester.WithTestTimeout(60*time.Second).
  This spec in scan_workflow_test.go was the one retry-driving cover spec
  still missing that option.
• This is a test-infra timeout adjustment, not a loosened assertion, sleep, or
  retry-the-CI-job workaround — it corrects a tester construction option the
  spec should have had from the start, matching the sibling pattern already
  used elsewhere in the same package.

Test plan

• go build ./..., go vet ./... clean
• go test -tags integration -run TestWfengine ./internal/wfengine/ — full
  2106-spec suite green
• Isolated --ginkgo.focus="transient cover failure retried" run 15x in a
  row, clean
• Re-ran under synthetic CPU contention (GOMAXPROCS=2 + 20-30 concurrent
  yes loads) to approximate a loaded CI runner — no regression
• make test full suite green

Docs: N/A because test infra only, no user-facing change.

Closes bead bookshelf-di2zy on merge.

## Summary - Fixes an intermittent Integration-job panic in `internal/wfengine`: `[PANICKED!] LibraryScanWorkflow — transient cover failure retried [JustBeforeEach] returns no error`. - Root cause: this spec's cover activity genuinely retries (transient error → MaxAttempts=3 backoff), driving more executor events/goroutines through go-workflows' in-memory `WorkflowTester` than a single-attempt spec. The tester's default 10s idle timeout is a **real wall-clock timer** racing the event loop (`tester.go` `Execute`: `time.NewTimer(TestTimeout)` vs the callback channel) — under CPU starvation on a loaded CI runner the gap between events can exceed 10s even though nothing is actually deadlocked, triggering the panic. - Same root cause + same fix already applied to `BulkMetadataWorkflow`'s retry-heavy spec (bookshelf-19lt): pass `tester.WithTestTimeout(60*time.Second)`. This spec in `scan_workflow_test.go` was the one retry-driving cover spec still missing that option. - This is a test-infra timeout adjustment, not a loosened assertion, sleep, or retry-the-CI-job workaround — it corrects a tester construction option the spec should have had from the start, matching the sibling pattern already used elsewhere in the same package. ## Test plan - [x] `go build ./...`, `go vet ./...` clean - [x] `go test -tags integration -run TestWfengine ./internal/wfengine/` — full 2106-spec suite green - [x] Isolated `--ginkgo.focus="transient cover failure retried"` run 15x in a row, clean - [x] Re-ran under synthetic CPU contention (`GOMAXPROCS=2` + 20-30 concurrent `yes` loads) to approximate a loaded CI runner — no regression - [x] `make test` full suite green Docs: N/A because test infra only, no user-facing change. Closes bead bookshelf-di2zy on merge.

zombor added 1 commit 2026-08-11 13:18:24 +00:00

fix(wfengine): raise idle timeout for retry-heavy cover flake test (bookshelf-di2zy) ... 7b3fbe779c

The "LibraryScanWorkflow — transient cover failure retried" spec exercises a
genuine cover-activity retry (transient error → MaxAttempts=3 backoff), which
drives more executor events/goroutines through go-workflows' in-memory
WorkflowTester than a single-attempt spec. The tester's default 10s idle
timeout (tester.go: "No new events generated ... workflow blocked?") is a real
wall-clock timer racing the event loop, not a simulated one — under CPU
starvation on a loaded CI runner the gap between events can exceed 10s even
though nothing is actually deadlocked, causing an intermittent panic.

Same root cause and same fix already applied to BulkMetadataWorkflow's
retry-heavy spec (bookshelf-19lt): pass tester.WithTestTimeout(60*time.Second)
so the idle window has enough headroom on a heavily loaded single-core runner.
Not a retry/sleep workaround — it corrects a tester option that this spec
should have set from the start, matching the sibling pattern already in
bulk_workflow_test.go / recalc_scores_workflow_test.go.

Reproduced by reading tester.go's Execute loop (time.NewTimer(TestTimeout)
racing the callback channel) and confirming this Describe was the only
retry-driving cover spec in scan_workflow_test.go missing the option; ran the
suite (2106 specs) and the isolated spec 15x clean, plus under synthetic CPU
contention (GOMAXPROCS=2 + 20-30 `yes` loads), with no regression.

Docs: N/A because test infra only, no user-facing change.

zombor commented 2026-08-11 13:35:38 +00:00

Author

Owner

Copy link

Security review (bookshelf-di2zy)

Reviewed the diff for PR #1432 — a test-only de-flake of the "LibraryScanWorkflow — transient cover failure retried" spec in internal/wfengine/scan_workflow_test.go.

Scope confirmed: the entire diff is a single hunk adding tester.WithTestTimeout(60*time.Second) to the existing tester.NewWorkflowTester[error](wfengine.LibraryScanWorkflow, ...) call plus an explanatory comment and a time import. No production (non-_test.go) code is touched, no assertion is removed or weakened, and the change matches the existing sibling pattern already used in bulk_workflow_test.go / recalc_scores_workflow_test.go. No secrets, credentials, or PII are introduced.

No findings.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review (bookshelf-di2zy) Reviewed the diff for PR #1432 — a test-only de-flake of the "LibraryScanWorkflow — transient cover failure retried" spec in `internal/wfengine/scan_workflow_test.go`. Scope confirmed: the entire diff is a single hunk adding `tester.WithTestTimeout(60*time.Second)` to the existing `tester.NewWorkflowTester[error](wfengine.LibraryScanWorkflow, ...)` call plus an explanatory comment and a `time` import. No production (non-`_test.go`) code is touched, no assertion is removed or weakened, and the change matches the existing sibling pattern already used in `bulk_workflow_test.go` / `recalc_scores_workflow_test.go`. No secrets, credentials, or PII are introduced. No findings. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor commented 2026-08-11 13:38:43 +00:00

Author

Owner

Copy link

Code Review — PR #1432 (bookshelf-di2zy)

No blockers or majors. This is a legitimate root-cause fix, not a mask.

Verification of root cause (not a race, not a masked assertion):

• The bead hypothesized either (a) a race/nil-deref on state mutated by an async activity stub, or (b) a tester timing issue. I traced the actual panic message "No new events generated during workflow execution and no pending timers, workflow blocked?" to go-workflows@v1.4.1/tester/tester.go:470, which fires from a hardcoded TestTimeout: 10s idle-wait (tester.go:197) inside the tester's own event loop (tester.go:458-470) — this is a wall-clock idle-detection heuristic inside the third-party test harness, not a data race in scan_workflow_test.go's own state.
• coverCallCount/coverSucceeded (scan_workflow_test.go:298-299, 325-330) are correctly atomic-incremented inside the activity stub and read only after wt.Execute() returns synchronously in JustBeforeEach (scan_workflow_test.go:335-337) — i.e. after the tester has fully drained the event loop. No async-side-effect race here; this confirms the panic is the harness's idle timer, not a test-state race.
• This exact mechanism and fix (tester.WithTestTimeout) was already established and accepted for the identical failure mode in bookshelf-19lt / PR #397 (BulkMetadataWorkflow, June 2026) — same tester.go:470 panic under CI runner CPU contention, same fix. This PR is consistent precedent, not a novel workaround.
• The retry-heavy nature of this specific spec (transient cover error → MaxAttempts=3 fan-out retry with backoff, scan_workflow_test.go:291-351) genuinely schedules more executor goroutines/events than a single-attempt spec, which is a plausible reason this spec — and not its siblings in the same file — hits the 10s ceiling under load. The 60s bump is scoped to only this Describe block (scan_workflow_test.go:315), not applied file-wide.

Confirmed NOT masking:

• No assertion was loosened. It("returns no error", ...) (scan_workflow_test.go:340-342) still asserts the workflow SUCCEEDED (workflowErr).NotTo(HaveOccurred())), not merely "an error occurred" or "did not panic."
• No sleep was added to pad timing; WithTestTimeout only raises the harness's own idle-panic ceiling, it does not change when real events fire.
• No assertion was deleted or skipped.
• Test file remains black-box: package wfengine_test (scan_workflow_test.go:1). No new white-box/internal access introduced.
• Diff is scoped to exactly the flaky Describe block (scan_workflow_test.go:294-351); no unrelated changes.

[MINOR] internal/wfengine/scan_workflow_test.go:315-320 — The 60s timeout is generous but matches the existing accepted precedent (bookshelf-19lt used the same 60s value for a similar retry-heavy spec), so consistency is good; no action needed. Worth a passing note only: if this class of flake recurs in additional retry-heavy specs, consider making WithTestTimeout(60*time.Second) the default for any wfengine spec that exercises MaxAttempts > 1 fan-out, rather than fixing them one at a time as they flake (tracked informally, not blocking this PR).

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

**Code Review — PR #1432 (bookshelf-di2zy)** No blockers or majors. This is a legitimate root-cause fix, not a mask. **Verification of root cause (not a race, not a masked assertion):** - The bead hypothesized either (a) a race/nil-deref on state mutated by an async activity stub, or (b) a tester timing issue. I traced the actual panic message `"No new events generated during workflow execution and no pending timers, workflow blocked?"` to `go-workflows@v1.4.1/tester/tester.go:470`, which fires from a hardcoded `TestTimeout: 10s` idle-wait (`tester.go:197`) inside the tester's own event loop (`tester.go:458-470`) — this is a wall-clock idle-detection heuristic *inside the third-party test harness*, not a data race in `scan_workflow_test.go`'s own state. - `coverCallCount`/`coverSucceeded` (scan_workflow_test.go:298-299, 325-330) are correctly atomic-incremented inside the activity stub and read only after `wt.Execute()` returns synchronously in `JustBeforeEach` (scan_workflow_test.go:335-337) — i.e. after the tester has fully drained the event loop. No async-side-effect race here; this confirms the panic is the harness's idle timer, not a test-state race. - This exact mechanism and fix (`tester.WithTestTimeout`) was already established and accepted for the identical failure mode in bookshelf-19lt / PR #397 (BulkMetadataWorkflow, June 2026) — same tester.go:470 panic under CI runner CPU contention, same fix. This PR is consistent precedent, not a novel workaround. - The retry-heavy nature of *this specific* spec (transient cover error → MaxAttempts=3 fan-out retry with backoff, scan_workflow_test.go:291-351) genuinely schedules more executor goroutines/events than a single-attempt spec, which is a plausible reason this spec — and not its siblings in the same file — hits the 10s ceiling under load. The 60s bump is scoped to only this `Describe` block (scan_workflow_test.go:315), not applied file-wide. **Confirmed NOT masking:** - No assertion was loosened. `It("returns no error", ...)` (scan_workflow_test.go:340-342) still asserts the workflow SUCCEEDED (`workflowErr).NotTo(HaveOccurred())`), not merely "an error occurred" or "did not panic." - No `sleep` was added to pad timing; `WithTestTimeout` only raises the harness's own idle-panic ceiling, it does not change when real events fire. - No assertion was deleted or skipped. - Test file remains black-box: `package wfengine_test` (scan_workflow_test.go:1). No new white-box/internal access introduced. - Diff is scoped to exactly the flaky `Describe` block (scan_workflow_test.go:294-351); no unrelated changes. **[MINOR]** internal/wfengine/scan_workflow_test.go:315-320 — The 60s timeout is generous but matches the existing accepted precedent (bookshelf-19lt used the same 60s value for a similar retry-heavy spec), so consistency is good; no action needed. Worth a passing note only: if this class of flake recurs in additional retry-heavy specs, consider making `WithTestTimeout(60*time.Second)` the default for any wfengine spec that exercises `MaxAttempts > 1` fan-out, rather than fixing them one at a time as they flake (tracked informally, not blocking this PR). REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor merged commit 94c347c204 into main 2026-08-11 13:55:35 +00:00

zombor referenced this pull request from a commit 2026-08-11 13:55:40 +00:00

Merge pull request 'De-flake wfengine transient cover-retry spec (bookshelf-di2zy)' (#1432) from bd-bookshelf-di2zy 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!1432

No description provided.