mosaicstack/stack
1
0
Fork 0
Code Issues 85 Pull Requests Actions Packages Projects Releases 15 Wiki Activity

test(wake): #923 de-flake T10 concurrent-enqueue race (deterministic barrier) #926

Merged
Mos merged 1 commits from fix/wake-923-t10-deflake into main 2026-07-26 10:23:08 +00:00
Conversation 1 Commits 1 Files Changed 1 +50 -5
jason.woltje commented 2026-07-26 10:04:44 +00:00
Owner
Copy Link

Closes #923; Part of #892

The race (diagnosed)

T10 launched its two concurrent store.sh enqueue calls with plain
cmd & cmd & wait "$p1"; wait "$p2". Nothing guaranteed the two children were
ever genuinely in-flight together — the scheduler is free to run one to
completion before the other is even forked, so the flock contention T10 exists
to prove was sometimes never really exercised. Under CI load this manifested
as the intermittent fail-then-pass-on-rerun behavior in #923.

The fix (test determinism only, invariant unchanged)

Added a deterministic start barrier (an flock'd gate file — no sleep):

  • both children write a "ready" marker, then block trying to take a shared
    lock on a gate file the parent holds exclusively;
  • the parent busy-polls (not sleeps) until both ready markers exist, proving
    both children are truly launched and blocked;
  • the parent then releases its exclusive lock, so both children's pending
    shared-lock acquisitions are granted together and they race into
    store.sh's own enqueue lock for real, at (as close to) the same instant as
    the scheduler allows.

Assertions are unchanged: distinct + gapless {1,2} seqs (order-independent),
both durably enqueued (pending_depth=2), only after both PIDs have exited.

Verification (all foreground)

  • 30/30 sequential runs of test-wake-store-ack.sh pass (previously
    intermittent).
  • RED-catch: with _wake_lock_acquire temporarily stubbed to a no-op
    (uncommitted, local-only mutation, reverted after), T10 reliably aliases
    (both enqueues get seq 1) and fails loud, 5/5 runs — proving the
    de-flaked T10 still catches a real aliasing regression. Restored and
    reconfirmed GREEN.
  • pnpm run test:framework-shell: rc=0 normally, and rc=0 with openssl
    masked from PATH (command -v openssl verified empty in that run).
  • shellcheck 0.11.0: clean.
  • git diff --stat: only test-wake-store-ack.sh (no manifest bump — test
    determinism only, no behavior change, matching #918/#861 precedent).

Note for follow-up (out of scope for this PR)

While diagnosing, I found and deterministically reproduced (100% repeatable,
independent of test-file timing) a genuine product-level TOCTOU race,
separate from the test-harness issue this PR fixes:

_wake_clean_stale_tmp() (_wake-common.sh) unconditionally
find -deletes every .wake.tmp.* file in the state dir. It's invoked by
_wake_init_dir(), which cmd_enqueue calls at store.sh:166 — before
_wake_lock_acquire at store.sh:174. So a second enqueue process, still in
its pre-lock setup, can delete a first process's live in-flight atomic-write
temp file even while that first process holds the enqueue lock the entire
time, causing a spurious "durable pending write FAILED" abort. Reproduced
deterministically by holding the lock + creating a tmp file in one process
while a concurrent process runs _wake_init_dir() with no lock at all.

This can plausibly account for at least some of the real-world T10 CI flake
history (not just the test-harness timing gap fixed here), and is a genuine
concurrent-enqueue reliability issue independent of any test. Recommend a
follow-up: move the pre-lock _wake_init_dir cleanup either inside the
enqueue lock, or restrict _wake_clean_stale_tmp to non-hot-path callers
(e.g. cmd_init only) rather than running it unconditionally on every
cmd_enqueue invocation. I did not touch store.sh/_wake-common.sh in this
PR to keep the diff scoped to the test-determinism fix as directed.

Closes #923; Part of #892 ## The race (diagnosed) T10 launched its two concurrent `store.sh enqueue` calls with plain `cmd & cmd & wait "$p1"; wait "$p2"`. Nothing guaranteed the two children were ever genuinely in-flight together — the scheduler is free to run one to completion before the other is even forked, so the flock contention T10 exists to prove was sometimes never really exercised. Under CI load this manifested as the intermittent fail-then-pass-on-rerun behavior in #923. ## The fix (test determinism only, invariant unchanged) Added a deterministic start barrier (an `flock`'d gate file — no `sleep`): - both children write a "ready" marker, then block trying to take a *shared* lock on a gate file the parent holds *exclusively*; - the parent busy-polls (not sleeps) until both ready markers exist, proving both children are truly launched and blocked; - the parent then releases its exclusive lock, so both children's pending shared-lock acquisitions are granted together and they race into `store.sh`'s own enqueue lock for real, at (as close to) the same instant as the scheduler allows. Assertions are unchanged: distinct + gapless `{1,2}` seqs (order-independent), both durably enqueued (`pending_depth=2`), only after both PIDs have exited. ## Verification (all foreground) - 30/30 sequential runs of `test-wake-store-ack.sh` pass (previously intermittent). - RED-catch: with `_wake_lock_acquire` temporarily stubbed to a no-op (uncommitted, local-only mutation, reverted after), T10 reliably aliases (both enqueues get seq `1`) and fails loud, 5/5 runs — proving the de-flaked T10 still catches a real aliasing regression. Restored and reconfirmed GREEN. - `pnpm run test:framework-shell`: rc=0 normally, and rc=0 with `openssl` masked from `PATH` (`command -v openssl` verified empty in that run). - `shellcheck` 0.11.0: clean. - `git diff --stat`: only `test-wake-store-ack.sh` (no manifest bump — test determinism only, no behavior change, matching #918/#861 precedent). ## Note for follow-up (out of scope for this PR) While diagnosing, I found and deterministically reproduced (100% repeatable, independent of test-file timing) a genuine **product-level** TOCTOU race, separate from the test-harness issue this PR fixes: `_wake_clean_stale_tmp()` (`_wake-common.sh`) unconditionally `find -delete`s every `.wake.tmp.*` file in the state dir. It's invoked by `_wake_init_dir()`, which `cmd_enqueue` calls at `store.sh:166` — **before** `_wake_lock_acquire` at `store.sh:174`. So a second enqueue process, still in its pre-lock setup, can delete a first process's live in-flight atomic-write temp file even while that first process holds the enqueue lock the entire time, causing a spurious "durable pending write FAILED" abort. Reproduced deterministically by holding the lock + creating a tmp file in one process while a concurrent process runs `_wake_init_dir()` with no lock at all. This can plausibly account for at least some of the real-world T10 CI flake history (not just the test-harness timing gap fixed here), and is a genuine concurrent-enqueue reliability issue independent of any test. Recommend a follow-up: move the pre-lock `_wake_init_dir` cleanup either inside the enqueue lock, or restrict `_wake_clean_stale_tmp` to non-hot-path callers (e.g. `cmd_init` only) rather than running it unconditionally on every `cmd_enqueue` invocation. I did not touch `store.sh`/`_wake-common.sh` in this PR to keep the diff scoped to the test-determinism fix as directed.
jason.woltje added 1 commit 2026-07-26 10:04:44 +00:00
test(wake): de-flake T10 concurrent-enqueue race — deterministic start-barrier + wait-both (#923)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Details
f9175c99b6 
T10 ("two concurrent store.sh enqueue get DISTINCT seqs via flock") launched
its two background enqueues with plain `cmd & cmd & wait` — no guarantee the
scheduler ever actually overlapped them, so the flock contention the test
exists to prove was sometimes never really exercised (or the assertion could
race a not-yet-durable second write), producing an intermittent CI-load flake.

Replace the launch with a deterministic start barrier (flock'd gate file, no
sleep): both children signal a ready marker then block on a shared exclusive
lock; the parent busy-polls (not sleeps) until both markers exist, THEN
releases the gate so both children's pending shared-lock waits are granted
together and they race into store.sh's own enqueue lock for real. Assertions
(distinct+gapless {1,2} seqs, order-independent, both durably enqueued) are
unchanged and still run only after both PIDs have exited.

Verified: 30/30 sequential foreground runs pass (previously intermittent).
RED-catch reproduced 5/5: with _wake_lock_acquire temporarily stubbed to a
no-op, T10 reliably aliases (both enqueues get seq '1') and fails loud,
proving the de-flaked test still catches a real aliasing regression;
restored and reconfirmed GREEN. shellcheck 0.11.0 clean.

Closes #923; Part of #892
jason.woltje commented 2026-07-26 10:22:44 +00:00
Author
Owner
Copy Link

Record of Review — PR #926 (issue #923): T10 concurrency-test de-flake — FINAL, CI-GREEN

VERDICT: APPROVE — GO. REVIEWED-HEAD (full-40): f9175c99b6b8e1be51ee6aa6edec5f910d43e02d. CI: TERMINAL-GREEN (pipeline 2072).

Review basis

Independent reviewer ae6831d2 (≠ builder mosaic-coder) — APPROVE, all 4 criteria PASS, reproduced (not narrative):

  1. Scope/provenance — head==f9175c99…; mergeable; PR-author id2 ≠ commit-author mosaic-coder; body Closes #923 + Part of #892; delta = ONLY test-wake-store-ack.sh (+50/-5); store.sh/_wake-common.sh BYTE-UNCHANGED vs main (no product touch); no manifest bump (test-only, #918/#861 precedent).
  2. De-flake correct — the flake was cmd & cmd & wait never guaranteeing both enqueues concurrently in-flight; fix = deterministic flock START BARRIER (no sleep; both children block on a shared lock, parent busy-polls until both ready-markers then releases → both race into store.shs enqueue lock together). Reviewer ran the harness 30× → 30/30 pass; assertions (distinct+gapless {1,2}, order-independent, both durable) unchanged.
  3. Invariant NOT weakened — reviewer stubbed _wake_lock_acquire to no-op → T10 RED 5/5 (both alias seq 1); reverted → GREEN. Still catches a real flock/aliasing regression.
  4. Gates — shellcheck 0.11 clean; test:framework-shell rc=0 normal AND openssl-masked (command -v openssl empty, HMAC groups SKIP); firewall 0; diff test-only.

Non-blocking observations

(a) a small ready-marker/flock ordering window (proven fine by 30/30 + 5/5); (b) spin-cap magic number (fires sub-second). Neither blocks.

Important context (separate, tracked)

The builder deterministically reproduced + PROMINENTLY DISCLOSED a genuine product-level enqueue TOCTOU (_wake_clean_stale_tmp pre-lock) — correctly OUT OF SCOPE for this test-only PR (store.sh untouched) — filed + Mos-ELEVATED as #927 (fix in flight). Scope discipline exemplary.

6-check — GO

  1. open/mergeable base main ✓ · 2. CI TERMINAL-GREEN @ full-40 f9175c99b6b8e1be51ee6aa6edec5f910d43e02d ✓ · 3. reviewer(ae6831d2)≠builder durable RoR ✓ · 4. reviewed==CI==merge-head all f9175c99 ✓ · 5. body Closes #923 + Part of #892 ✓ · 6. queue-guard at merge.
    GO for id-11 + squash-merge at FULL-40 f9175c99b6b8e1be51ee6aa6edec5f910d43e02d. Closes #923. NOTE: #927 (in flight) may add a store-ack test group → union-rebase #927 onto the new main after this merge if so. Non-executor.
# Record of Review — PR #926 (issue #923): T10 concurrency-test de-flake — FINAL, CI-GREEN **VERDICT: APPROVE — GO.** **REVIEWED-HEAD (full-40):** `f9175c99b6b8e1be51ee6aa6edec5f910d43e02d`. **CI: TERMINAL-GREEN** (pipeline 2072). ## Review basis Independent reviewer `ae6831d2` (≠ builder `mosaic-coder`) — **APPROVE, all 4 criteria PASS**, reproduced (not narrative): 1. **Scope/provenance** — head==`f9175c99…`; mergeable; PR-author id2 ≠ commit-author `mosaic-coder`; body `Closes #923` + `Part of #892`; delta = ONLY test-wake-store-ack.sh (+50/-5); **store.sh/_wake-common.sh BYTE-UNCHANGED vs main** (no product touch); no manifest bump (test-only, #918/#861 precedent). 2. **De-flake correct** — the flake was `cmd & cmd & wait` never guaranteeing both enqueues concurrently in-flight; fix = deterministic flock START BARRIER (no sleep; both children block on a shared lock, parent busy-polls until both ready-markers then releases → both race into store.shs enqueue lock together). Reviewer ran the harness 30× → 30/30 pass; assertions (distinct+gapless {1,2}, order-independent, both durable) unchanged. 3. **Invariant NOT weakened** — reviewer stubbed `_wake_lock_acquire` to no-op → T10 RED 5/5 (both alias seq 1); reverted → GREEN. Still catches a real flock/aliasing regression. 4. **Gates** — shellcheck 0.11 clean; test:framework-shell rc=0 normal AND openssl-masked (`command -v openssl` empty, HMAC groups SKIP); firewall 0; diff test-only. ## Non-blocking observations (a) a small ready-marker/flock ordering window (proven fine by 30/30 + 5/5); (b) spin-cap magic number (fires sub-second). Neither blocks. ## Important context (separate, tracked) The builder deterministically reproduced + PROMINENTLY DISCLOSED a genuine product-level enqueue TOCTOU (`_wake_clean_stale_tmp` pre-lock) — correctly OUT OF SCOPE for this test-only PR (store.sh untouched) — filed + Mos-ELEVATED as **#927** (fix in flight). Scope discipline exemplary. ## 6-check — GO 1. open/mergeable base main ✓ · 2. **CI TERMINAL-GREEN @ full-40 `f9175c99b6b8e1be51ee6aa6edec5f910d43e02d`** ✓ · 3. reviewer(`ae6831d2`)≠builder durable RoR ✓ · 4. reviewed==CI==merge-head all `f9175c99` ✓ · 5. body `Closes #923` + `Part of #892` ✓ · 6. queue-guard at merge. **GO for id-11 + squash-merge at FULL-40 `f9175c99b6b8e1be51ee6aa6edec5f910d43e02d`. Closes #923. NOTE: #927 (in flight) may add a store-ack test group → union-rebase #927 onto the new main after this merge if so. Non-executor.**
Mos approved these changes 2026-07-26 10:23:05 +00:00
Mos left a comment
First-time contributor
Copy Link

GO — Gate-16 id-11 stamp, pinned to exact FULL head f9175c99b6.
Basis: verbatim FINAL RoR (comment 19171) — independent review APPROVE 4/4: determinism reproduced 30/30 via the flock start-barrier, the flock-disabled RED preserved 5/5 (the aliasing catch keeps its teeth), store.sh and _wake-common byte-unchanged (test-only). CI terminal-green. Closes #923 — the T10 timing-granularity flake joins T7 in the fixed column, and this de-flake's build is also what surfaced #927 (the builder's scope discipline turning a test fix into a tracked product-correctness find). Part of #892. Named executor: Mos (id-11), squash pinned.

GO — Gate-16 id-11 stamp, pinned to exact FULL head f9175c99b6b8e1be51ee6aa6edec5f910d43e02d. Basis: verbatim FINAL RoR (comment 19171) — independent review APPROVE 4/4: determinism reproduced 30/30 via the flock start-barrier, the flock-disabled RED preserved 5/5 (the aliasing catch keeps its teeth), store.sh and _wake-common byte-unchanged (test-only). CI terminal-green. Closes #923 — the T10 timing-granularity flake joins T7 in the fixed column, and this de-flake's build is also what surfaced #927 (the builder's scope discipline turning a test fix into a tracked product-correctness find). Part of #892. Named executor: Mos (id-11), squash pinned.
Mos merged commit 90265ef550 into main 2026-07-26 10:23:08 +00:00
Mos deleted branch fix/wake-923-t10-deflake 2026-07-26 10:23:09 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Mos
Labels
Clear labels
fleet-enhancement
Fleet-wide reliability/speed/cost improvements surfaced by the nightly enhance pass
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaicstack/stack#926
Delete Branch "fix/wake-923-t10-deflake"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?