Skip to content

ci(test): auto-enable storage stress rounds for WAL recovery race changes - #2474

Merged
Astro-Han merged 3 commits into
apache:mainfrom
GabrielDrapor:ci/storage-stress-wal-race
Aug 9, 2026
Merged

ci(test): auto-enable storage stress rounds for WAL recovery race changes#2474
Astro-Han merged 3 commits into
apache:mainfrom
GabrielDrapor:ci/storage-stress-wal-race

Conversation

@GabrielDrapor

Copy link
Copy Markdown
Contributor

Refs #2388 (stress-policy PR; the fixture-reduction changes are in #2473).

#2459 moved the 12-round fresh-WAL race amplification behind MAKA_STORAGE_STRESS=1, but sqlite-recovery-concurrency.test.ts was never added to STORAGE_STRESS_FILES in scripts/ci-test-plan.mjs — so CI would not re-enable the amplified rounds even for changes to the recovery race surface itself.

This wires the WAL race into the same auto-enable heuristic the other multi-process storage probes use, adding:

  • packages/storage/src/sqlite-runtime-store.ts (the store the race exercises)
  • packages/storage/src/__tests__/sqlite-recovery-concurrency.test.ts
  • packages/storage/src/__tests__/fixtures/sqlite-recovery-concurrency-child.ts (the spawned worker)

Verification

  • scripts/ci-test-plan.test.mjs: pass.
  • planTests flips storageStress=true for each of the three files and stays false for unrelated storage files (e.g. plan-store.ts).
  • MAKA_STORAGE_STRESS=1 node --test dist/__tests__/sqlite-recovery-concurrency.test.js: 12 pass / 0 fail (3.1s).

Default-suite behavior is unchanged: 1 deterministic round, stress rounds only when the flag is set.

Co-Authored-By: Claude noreply@anthropic.com

https://claude.ai/code/session_01Ac5rv6WUKWtMZgQb5QPN16

…nges
Refs apache#2388.
apache#2459 gated the 12-round fresh-WAL race amplification behind
MAKA_STORAGE_STRESS, but sqlite-recovery-concurrency.test.ts was not in
STORAGE_STRESS_FILES, so CI never re-enabled the amplified rounds even when
the recovery race surface itself changed. Add the test, its spawned worker
fixture, and sqlite-runtime-store.ts (the store the race exercises) to the
stress set, matching how the other multi-process storage probes are wired.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ac5rv6WUKWtMZgQb5QPN16
@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks — wiring this race back into the storage-stress route is the right follow-up to #2459. One coverage gap remains in the trigger set:

P2 — The amplified operational_open_only path does not only exercise sqlite-runtime-store.ts. Its child calls acquireOperationalStateDatabase(), so the fresh-WAL initialization, locking, and migration path is owned by:

  • packages/storage/src/operational-state-store.ts
  • packages/storage/src/sqlite-runtime-schema.ts

Changes to either file currently leave storageStress=false, even though they can directly affect the race this route is intended to amplify. Could we add both paths to STORAGE_STRESS_FILES?

P3 — It would also help to add a small table-driven planner test covering the five relevant paths (the three already added here plus the two production owners above), with one unrelated storage file as the negative case. The PR body says these paths were checked, but keeping that assertion executable will prevent the routing contract from drifting later.

The overall approach is good; this should be a small completion of the same seam rather than a redesign.

简体中文

感谢补上这条路由——把该 race 重新接回 storage stress,是 #2459 之后正确的补充。目前触发集合还漏了一个覆盖面:

P2 — 被放大的 operational_open_only 路径不只经过 sqlite-runtime-store.ts。Child 实际调用 acquireOperationalStateDatabase(),因此 fresh-WAL 初始化、加锁和迁移还由以下文件负责:

  • packages/storage/src/operational-state-store.ts
  • packages/storage/src/sqlite-runtime-schema.ts

这两个文件发生变化时,当前 storageStress 仍然是 false,但它们会直接影响本路由要放大的 race。建议把两者也加入 STORAGE_STRESS_FILES

P3 — 还建议补一个小型表驱动 planner 测试:覆盖本 PR 已加入的三个路径和上述两个生产 owner,并用一个无关 storage 文件作为负例。PR 正文虽然说明已经手动验证,但把它变成可执行契约,才能避免后续路由漂移。

整体方案是对的;这里只需要沿用同一个 seam 补完整,不需要重新设计。

…outing
Review follow-up: the amplified race path runs through
acquireOperationalStateDatabase(), so operational-state-store.ts and
sqlite-runtime-schema.ts own the fresh-WAL initialization, locking, and
migration it exercises. Add both to STORAGE_STRESS_FILES, and extend the
planner test's stress table with all five WAL-race paths so the routing
contract stays executable.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ac5rv6WUKWtMZgQb5QPN16
@GabrielDrapor

Copy link
Copy Markdown
ContributorAuthor

Both points addressed in df96d1c's follow-up commit:

P2: Confirmed — the child fixture calls acquireOperationalStateDatabase() (sqlite-recovery-concurrency-child.ts:31), and operational-state-store.ts pulls the schema/migration path from sqlite-runtime-schema.ts. Both are now in STORAGE_STRESS_FILES.

P3: Rather than a new test block, I extended the existing table-driven stress assertion in ci-test-plan.test.mjs ('stress and specialized script checks run only for their owning surfaces') with all five WAL-race paths — the three from this PR plus the two production owners. The negative case was already executable there: session-store.ts asserts storageStress === false, and the force-full path asserts the flag stays off. Planner suite: 6 pass / 0 fail.

@Astro-Han

Copy link
Copy Markdown
Contributor

Thanks for addressing the previous review exactly as requested. On a deeper pass through the full initialization call chain, I realized that my earlier list of production owners was incomplete — sorry for the extra round.

operational_open_only calls acquireOperationalStateDatabase(), which also runs migrations owned by:

  • sqlite-session-metadata-schema.ts
  • sqlite-core-execution-schema.ts
  • sqlite-workflow-schema.ts
  • sqlite-usage-schema.ts
  • sqlite-artifact-schema.ts
  • sqlite-automation-schema.ts

Changes to these files still leave storageStress=false, although their migrations execute inside the fresh-WAL race being amplified. Could we add these six paths and extend the existing table-driven planner cases accordingly?

One correction to my previous review: sqlite-runtime-store.ts is imported by the child fixture, but the amplified operational_open_only branch does not call createSqliteRuntimeStore(). Removing it from the trigger set would keep the routing boundary exact and avoid unrelated stress runs.

This is a completion of the same trigger set, not a request to redesign the approach. Thanks again, and apologies that I did not trace the full migration chain in the first pass.

…utes
Review follow-up: acquireOperationalStateDatabase() runs the migrations
owned by the six sqlite-*-schema modules inside the amplified fresh-WAL
race, so all six join STORAGE_STRESS_FILES and the planner's stress table.
sqlite-runtime-store.ts leaves the set - the amplified operational_open_only
branch never constructs it - and a negative planner case pins that boundary.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ac5rv6WUKWtMZgQb5QPN16
@GabrielDrapor

Copy link
Copy Markdown
ContributorAuthor

Done in the latest commit. Verified both corrections against the child fixture before changing anything: operational_open_only takes the acquireOperationalStateDatabase() branch (sqlite-recovery-concurrency-child.ts:30-34) and createSqliteRuntimeStore() belongs only to the other worker modes, whose behavior does not change under MAKA_STORAGE_STRESS — so removing it keeps the trigger exact. The six schema modules are all imported by operational-state-store.ts's migration path and are now in STORAGE_STRESS_FILES plus the planner's stress table, and a new negative case pins sqlite-runtime-store.ts to storageStress=false so the boundary stays executable. Planner suite: 6 pass / 0 fail.

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at ce93ae9d. The previous trigger-set findings are resolved: every migration owner exercised by acquireOperationalStateDatabase() now enables storage stress, while sqlite-runtime-store.ts is correctly excluded and pinned by a negative planner case. The focused planner suite passes 6/6. No remaining P0–P3 findings.\n\nThe failing Storybook check is an unrelated main-branch baseline issue fixed separately by #2546.

@Astro-Han
Astro-Han merged commit 54b702e into apache:mainAug 9, 2026
21 of 23 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@GabrielDrapor@Astro-Han