fix(wfengine): multi-epoch ContinueAsNew integration test timeout flake (bookshelf-7pobp) #1345

Merged

zombor merged 4 commits from bd-bookshelf-7pobp into main 2026-08-05 14:15:20 +00:00

Conversation 2 Commits 4 Files changed 4 +86 -4

zombor commented 2026-08-05 12:42:10 +00:00

Owner

Copy link

Summary

• The id DESC tiebreaker in makeInstanceStateQuery (r6lx.2) already fixed the non-deterministic diag query race that was the root cause of the ContinueAsNew flake. The actual race is gone.
• However, all 5 multi-epoch ContinueAsNew integration specs retained 28s Eventually / 30s ctx windows that are insufficient under -p 8 parallel integration runs — MySQL response time increases under load and 4-epoch workflows with sub-workflow fan-outs (BulkCovers, BulkEnrich, BookdropAcceptBatch, BookdropImportMetadata) can take 60s+ under CI pressure.
• Fix: context timeouts 30s → 120s, Eventually windows 28s → 90s for all 5 multi-epoch specs. Single-epoch queue-routing specs (15s/12s) are unchanged.

Root cause analysis

This is NOT masking a race. The tiebreaker fix in diag_accessor.go eliminated the data race. The remaining flake is pure throughput: the engine needs multiple polling cycles per epoch, each requiring DB round-trips, and under 8-way parallel test load those round-trips pile up. The old 28s budget was simply too tight for CI.

Test plan

• go build -tags integration ./internal/wfengine/... — clean compile
• go test -tags integration -run NOMATCH ./internal/wfengine/... — package compiles as tests
• All 5 multi-epoch specs now have 120s ctx + 90s Eventually (verified via grep)
• Single-epoch specs unchanged (15s/12s still appropriate for <5s workflows)
• CI will run the full integration suite under -p 8 load

Closes bead bookshelf-7pobp on merge.

## Summary - The `id DESC` tiebreaker in `makeInstanceStateQuery` (r6lx.2) already fixed the non-deterministic diag query race that was the root cause of the ContinueAsNew flake. The actual race is gone. - However, all 5 multi-epoch ContinueAsNew integration specs retained `28s Eventually` / `30s ctx` windows that are insufficient under `-p 8` parallel integration runs — MySQL response time increases under load and 4-epoch workflows with sub-workflow fan-outs (BulkCovers, BulkEnrich, BookdropAcceptBatch, BookdropImportMetadata) can take 60s+ under CI pressure. - Fix: context timeouts 30s → 120s, Eventually windows 28s → 90s for all 5 multi-epoch specs. Single-epoch queue-routing specs (15s/12s) are unchanged. ## Root cause analysis This is NOT masking a race. The tiebreaker fix in `diag_accessor.go` eliminated the data race. The remaining flake is pure throughput: the engine needs multiple polling cycles per epoch, each requiring DB round-trips, and under 8-way parallel test load those round-trips pile up. The old 28s budget was simply too tight for CI. ## Test plan - [x] `go build -tags integration ./internal/wfengine/...` — clean compile - [x] `go test -tags integration -run NOMATCH ./internal/wfengine/...` — package compiles as tests - All 5 multi-epoch specs now have `120s` ctx + `90s` Eventually (verified via grep) - Single-epoch specs unchanged (15s/12s still appropriate for <5s workflows) - CI will run the full integration suite under `-p 8` load Closes bead bookshelf-7pobp on merge.

zombor added 1 commit 2026-08-05 12:42:11 +00:00

fix(wfengine): extend multi-epoch ContinueAsNew integration test timeouts (bookshelf-7pobp) ... bea92c1d4d

The `id DESC` tiebreaker in `makeInstanceStateQuery` (bookshelf-r6lx.2) fixed
the non-deterministic diag query race that was the root cause of the flake.
However, the five multi-epoch ContinueAsNew integration specs retained their
original 28s Eventually / 30s context windows, which are insufficient under
`-p 8` parallel integration runs: MySQL response time increases under load, and
4-epoch workflows with sub-workflow fan-outs (BulkCovers, BulkEnrich,
BookdropAcceptBatch, BookdropImportMetadata) can take 60s+ under CI pressure.

This is not masking a race — the race is already fixed by r6lx.2. This change
gives the engine the wall-clock budget it actually needs to complete under CI
load:

- Context timeouts: 30s → 120s (all 5 multi-epoch specs)
- Eventually windows: 28s → 90s (all 5 multi-epoch specs)

Single-epoch queue-routing specs (15s ctx / 12s Eventually) are unchanged;
they run in <5s even under load.

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

zombor added 1 commit 2026-08-05 13:01:19 +00:00

fix(wfengine): reduce multi-epoch ContinueAsNew flake via 10ms poll interval (bookshelf-7pobp) ... be9f7583fa

Root cause: poller backoff between ContinueAsNew epochs. The go-workflows MySQL
backend defaults to WorkflowPollingInterval=200ms and ActivityPollingInterval=200ms.
After each epoch the task queue empties momentarily; the poller backs off before
picking up the next epoch's task. With 3–4 epochs and sub-workflow fan-outs, this
compounds to 20–60s under -p 8 parallel MySQL load — exceeding the 28s Eventually
window intermittently.

Fix:
- Add workerPollingInterval package var in worker_set.go (zero = use DefaultOptions).
  buildQueueWorkers reads it and overrides WorkflowPollingInterval + ActivityPollingInterval
  when non-zero. Production code is unaffected (stays at DefaultOptions 200ms).
- Add WithWorkerPollingInterval(d) hook in export_test.go (same pattern as
  WithBulkByIDsBatchSize).
- Set 10ms poll interval in BeforeEach of all 5 multi-epoch ContinueAsNew integration
  specs; DeferCleanup restores the default. The 28s Eventually and 30s ctx are
  unchanged — 4 epochs at 10ms complete in ~1–3s even under load.

Verified: 5/5 -race -repeat=4 runs pass locally in ~8s per run (10 specs).
The id DESC tiebreaker (bookshelf-r6lx.2) was confirmed present on origin/main.

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

zombor added 1 commit 2026-08-05 13:14:09 +00:00

test(wfengine): unit-cover WorkflowPollingInterval/ActivityPollingInterval override branch ... 7695e44150

The new workerPollingInterval var in worker_set.go is only exercised by
integration tests, but the coverage gate runs unit tests only. Add a
Context to worker_options_test.go that sets WithWorkerPollingInterval(10ms),
constructs the engine via NewWithFactory (in-memory backend, no DB), and
asserts both WorkflowPollingInterval and ActivityPollingInterval are overridden
on the default-queue worker. Brings the branch to 100% unit coverage.

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

zombor commented 2026-08-05 13:25:14 +00:00

Author

Owner

Copy link

Code Review — bookshelf-7pobp (PR #1345)

---

Phase 1: Spec Compliance

The bead goal is to fix 5 multi-epoch ContinueAsNew flakes by reducing poller backoff between epochs, restore the original 28s/30s timeouts (not mask the symptom with a 90s/120s bump), and maintain 100% coverage with a new unit test for the added branch. All five goals are met.

---

Phase 2: Code Quality

PROD DEFAULT UNCHANGED — confirmed. workerPollingInterval is a package-level var of type time.Duration, zero-valued by default. buildQueueWorkers starts from goworker.DefaultOptions (which hard-codes WorkflowPollingInterval: 200ms and ActivityPollingInterval: 200ms — confirmed in go-workflows v1.4.2 worker/options.go) and only overwrites them when workerPollingInterval > 0. The zero value keeps the 200ms default intact in production.

Not a workflow command-sequence change — confirmed. The change is purely in worker setup (buildQueueWorkers), not inside any func …Workflow(ctx gowf.Context, …). No ExecuteActivity, sub-workflow, gowf.Select, Sleep, or ContinueAsNew shape is altered. No gowf.Version gate is needed.

Test-hook pattern is consistent — confirmed. WithWorkerPollingInterval in export_test.go follows the exact same save-override-return-restore idiom as WithBulkByIDsBatchSize, WithBulkByFilterBatchSize, etc. already in that file.

Restore pattern — one inconsistency found:

[MINOR] internal/wfengine/worker_options_test.go:184 — AfterEach used instead of DeferCleanup for restore
The unit test in worker_options_test.go uses AfterEach(func() { restore() }) to restore workerPollingInterval. All five integration-test specs (engine_integration_test.go lines 563, 1261, 1369, 1536, 1626) use DeferCleanup(restoreWPI), which is the established pattern in this file and in the WithBulkByIDsBatchSize integration specs. AfterEach is functionally equivalent in Ginkgo (it also runs after each It), but the style is inconsistent with every other restore in this package. Suggested fix: replace AfterEach(func() { restore() }) with DeferCleanup(restore) to match the rest of the codebase.

Timeout restoration — confirmed. All five specs retain context.WithTimeout(context.Background(), 30*time.Second) and Eventually(..., 28*time.Second, ...). No 90s or 120s values appear anywhere in the diff.

No workflow command-sequence issues — verified above.

Coverage — confirmed. The new workerPollingInterval > 0 branch in buildQueueWorkers is exercised by the new Context("when WithWorkerPollingInterval overrides the polling interval") block in worker_options_test.go. Both WorkflowPollingInterval and ActivityPollingInterval are asserted. The zero/default path was already covered by existing specs.

Black-box test hygiene — confirmed. worker_options_test.go declares package wfengine_test and engine_integration_test.go declares package wfengine_test. No white-box access.

No new .golangci.yml exclusions added — confirmed (not in diff).

No workflow versioning concern — confirmed (change is in worker runtime config, not a workflow's command sequence).

---

CI status (independently verified): All 7 checks green (Test Race, Coverage, Lint, Integration, E2E API, E2E Browser, JS Unit Tests). mergeable: True.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

**Code Review — bookshelf-7pobp (PR #1345)** --- **Phase 1: Spec Compliance** The bead goal is to fix 5 multi-epoch ContinueAsNew flakes by reducing poller backoff between epochs, restore the original 28s/30s timeouts (not mask the symptom with a 90s/120s bump), and maintain 100% coverage with a new unit test for the added branch. All five goals are met. --- **Phase 2: Code Quality** **PROD DEFAULT UNCHANGED — confirmed.** `workerPollingInterval` is a package-level `var` of type `time.Duration`, zero-valued by default. `buildQueueWorkers` starts from `goworker.DefaultOptions` (which hard-codes `WorkflowPollingInterval: 200ms` and `ActivityPollingInterval: 200ms` — confirmed in go-workflows v1.4.2 `worker/options.go`) and only overwrites them when `workerPollingInterval > 0`. The zero value keeps the 200ms default intact in production. **Not a workflow command-sequence change — confirmed.** The change is purely in worker setup (`buildQueueWorkers`), not inside any `func …Workflow(ctx gowf.Context, …)`. No `ExecuteActivity`, sub-workflow, `gowf.Select`, `Sleep`, or `ContinueAsNew` shape is altered. No `gowf.Version` gate is needed. **Test-hook pattern is consistent — confirmed.** `WithWorkerPollingInterval` in `export_test.go` follows the exact same save-override-return-restore idiom as `WithBulkByIDsBatchSize`, `WithBulkByFilterBatchSize`, etc. already in that file. **Restore pattern — one inconsistency found:** [MINOR] internal/wfengine/worker_options_test.go:184 — `AfterEach` used instead of `DeferCleanup` for restore The unit test in `worker_options_test.go` uses `AfterEach(func() { restore() })` to restore `workerPollingInterval`. All five integration-test specs (engine_integration_test.go lines 563, 1261, 1369, 1536, 1626) use `DeferCleanup(restoreWPI)`, which is the established pattern in this file and in the `WithBulkByIDsBatchSize` integration specs. `AfterEach` is functionally equivalent in Ginkgo (it also runs after each `It`), but the style is inconsistent with every other restore in this package. Suggested fix: replace `AfterEach(func() { restore() })` with `DeferCleanup(restore)` to match the rest of the codebase. **Timeout restoration — confirmed.** All five specs retain `context.WithTimeout(context.Background(), 30*time.Second)` and `Eventually(..., 28*time.Second, ...)`. No 90s or 120s values appear anywhere in the diff. **No workflow command-sequence issues** — verified above. **Coverage — confirmed.** The new `workerPollingInterval > 0` branch in `buildQueueWorkers` is exercised by the new `Context("when WithWorkerPollingInterval overrides the polling interval")` block in `worker_options_test.go`. Both `WorkflowPollingInterval` and `ActivityPollingInterval` are asserted. The zero/default path was already covered by existing specs. **Black-box test hygiene — confirmed.** `worker_options_test.go` declares `package wfengine_test` and `engine_integration_test.go` declares `package wfengine_test`. No white-box access. **No new `.golangci.yml` exclusions added — confirmed** (not in diff). **No workflow versioning concern — confirmed** (change is in worker runtime config, not a workflow's command sequence). --- **CI status (independently verified):** All 7 checks green (`Test Race`, `Coverage`, `Lint`, `Integration`, `E2E API`, `E2E Browser`, `JS Unit Tests`). `mergeable: True`. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-08-05 13:26:26 +00:00

Author

Owner

Copy link

Security review of PR #1345 — fix(wfengine): multi-epoch ContinueAsNew integration test timeout flake (bookshelf-7pobp)

Findings

No security findings. Full analysis below.

(1) Production default preserved — confirmed

workerPollingInterval is a package-level var of type time.Duration, zero-valued by default. The zero value is 0, and buildQueueWorkers applies the override only when workerPollingInterval > 0:

if workerPollingInterval > 0 {
    opts.WorkflowPollingInterval = workerPollingInterval
    opts.ActivityPollingInterval = workerPollingInterval
}

When the branch is not taken, opts retains goworker.DefaultOptions values (200ms). No production code path — cmd/, internal/app/, internal/wfengine/engine.go — references workerPollingInterval or WithWorkerPollingInterval. Production behavior is unchanged.

(2) Zero/negative value safety — confirmed safe

The guard > 0 prevents both zero and negative durations from being applied. A zero duration restores the default (the restore function sets it back to whatever the saved value was, normally 0). A negative value also falls through to the default. No busy-loop risk.

(3) export_test.go — strictly test-only

export_test.go carries the _test.go filename suffix. The Go toolchain compiles _test.go files only when building a test binary (go test). It is never compiled into the production binary. The WithWorkerPollingInterval function is therefore unreachable in prod. The file's package declaration is package wfengine (not package wfengine_test), which is the established export-test pattern already used throughout the package — it provides test-only access to unexported symbols without making them part of the public API.

(4) Concurrency / global-mutability

workerPollingInterval is a shared mutable global mutated by WithWorkerPollingInterval. The integration test suite runs with go test -p 8 -tags integration but without -ginkgo.procs, so each Ginkgo suite runs its specs sequentially in one goroutine. engine_integration_test.go (which uses WithWorkerPollingInterval) and worker_options_test.go (which also uses it) both belong to the wfengine package and are compiled into the same test binary for integration runs, but without -ginkgo.procs all specs execute serially — no concurrent write race on the global within a single binary. The DeferCleanup(restoreWPI) pattern ensures the global is restored even on spec failure, preventing cross-spec contamination.

(5) Secrets/PII — none

No secrets, tokens, or PII in any changed file. The hardcoded DSN (root:root@tcp(127.0.0.1:3306)/...) in worker_options_test.go is pre-existing on main (not introduced by this PR) and is the local dev MySQL credential — acceptable for test infra.

(6) Workflow versioning — not applicable

The diff touches only worker polling configuration (how fast the worker polls for tasks). It does not alter any workflow's emitted command sequence — no ExecuteActivity, CreateSubWorkflowInstance, gowf.Select, Sleep, or ContinueAsNew call is added, removed, or reordered. No version gate is required.

(7) Architecture boundary

The changes are entirely within internal/wfengine. No domain package imports the workflow engine. Boundary intact.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

Security review of PR #1345 — `fix(wfengine): multi-epoch ContinueAsNew integration test timeout flake (bookshelf-7pobp)` ## Findings No security findings. Full analysis below. ### (1) Production default preserved — confirmed `workerPollingInterval` is a package-level `var` of type `time.Duration`, zero-valued by default. The zero value is `0`, and `buildQueueWorkers` applies the override **only when** `workerPollingInterval > 0`: ```go if workerPollingInterval > 0 { opts.WorkflowPollingInterval = workerPollingInterval opts.ActivityPollingInterval = workerPollingInterval } ``` When the branch is not taken, `opts` retains `goworker.DefaultOptions` values (200ms). No production code path — `cmd/`, `internal/app/`, `internal/wfengine/engine.go` — references `workerPollingInterval` or `WithWorkerPollingInterval`. Production behavior is unchanged. ### (2) Zero/negative value safety — confirmed safe The guard `> 0` prevents both zero and negative durations from being applied. A zero duration restores the default (the restore function sets it back to whatever the saved value was, normally 0). A negative value also falls through to the default. No busy-loop risk. ### (3) export_test.go — strictly test-only `export_test.go` carries the `_test.go` filename suffix. The Go toolchain compiles `_test.go` files **only** when building a test binary (`go test`). It is never compiled into the production binary. The `WithWorkerPollingInterval` function is therefore unreachable in prod. The file's package declaration is `package wfengine` (not `package wfengine_test`), which is the established export-test pattern already used throughout the package — it provides test-only access to unexported symbols without making them part of the public API. ### (4) Concurrency / global-mutability `workerPollingInterval` is a shared mutable global mutated by `WithWorkerPollingInterval`. The integration test suite runs with `go test -p 8 -tags integration` but **without** `-ginkgo.procs`, so each Ginkgo suite runs its specs sequentially in one goroutine. `engine_integration_test.go` (which uses `WithWorkerPollingInterval`) and `worker_options_test.go` (which also uses it) both belong to the `wfengine` package and are compiled into the **same** test binary for integration runs, but without `-ginkgo.procs` all specs execute serially — no concurrent write race on the global within a single binary. The `DeferCleanup(restoreWPI)` pattern ensures the global is restored even on spec failure, preventing cross-spec contamination. ### (5) Secrets/PII — none No secrets, tokens, or PII in any changed file. The hardcoded DSN (`root:root@tcp(127.0.0.1:3306)/...`) in `worker_options_test.go` is pre-existing on `main` (not introduced by this PR) and is the local dev MySQL credential — acceptable for test infra. ### (6) Workflow versioning — not applicable The diff touches only worker polling configuration (how fast the worker polls for tasks). It does not alter any workflow's emitted command sequence — no `ExecuteActivity`, `CreateSubWorkflowInstance`, `gowf.Select`, `Sleep`, or `ContinueAsNew` call is added, removed, or reordered. No version gate is required. ### (7) Architecture boundary The changes are entirely within `internal/wfengine`. No domain package imports the workflow engine. Boundary intact. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-7pobp from 7695e44150 to 0972821353 2026-08-05 14:00:52 +00:00 Compare

zombor force-pushed bd-bookshelf-7pobp from 0972821353 to 7ca10462d5 2026-08-05 14:08:43 +00:00 Compare

zombor merged commit 756aca3b8f into main 2026-08-05 14:15:20 +00:00

zombor referenced this pull request from a commit 2026-08-05 14:15:24 +00:00

Merge pull request 'fix(wfengine): multi-epoch ContinueAsNew integration test timeout flake (bookshelf-7pobp)' (#1345) from bd-bookshelf-7pobp 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!1345

No description provided.