[sonichi]

What changed, and why

tests/agy-start-cli.test.py has a production-path concurrency test for the atomic seed() writer and a mutation control for it (8 naive writers through one shared <path>.tmp). Both relied on the executor happening to interleave the eight writers' stage-then-rename steps. That race is hoped for, not guaranteed: with a small payload the write window is microseconds, a fast runner serialises the calls, nobody collides, and the control fails (failures == []) although the production writer is correct. It failed twice on #4976 (runs 36808730430, 36816482701) on a PR that touches no agy file, and passed on rerun; #4928 reports the same flake.

Root cause: a property of the form "this race must happen" was asserted; a CI lane cannot promise it. The interleaving is now forced by construction, on the real writer and on the control alike.

Fix (test-only, no src/ change): one shared harness, _run_writers_staged_then_renamed(writer, path), patches the module's os.replace (unittest.mock.patch.object(self.seed.os, "replace", …) — the same seam test_write_failure_cleans_up_staging_file_and_reraises already uses) with a wrapper that waits on an 8-party threading.Barrier before renaming. Every writer therefore stages before any writer renames, on every run. The two tests differ only in the writer passed in:

• test_concurrent_writers_do_not_collide_on_shared_staging_name runs the real self.seed.seed: asserts no exception, the 500 000-char unrelatedBigField intact, all three fields true, and no stray staging file left in the directory.
• test_concurrency_test_would_catch_a_naive_non_atomic_writer runs _naive_seed (the fixed shared .tmp writer seed() replaced) through the same harness: asserts exactly 7 of 8 fail, each a FileNotFoundError (only the first rename can win).

Why all eight real writers reach the barrier: no rename happens until all eight are waiting, and each writer reads before it stages, so all eight read the unseeded file, compute changed=True, and call os.replace. The barrier has a 30 s timeout so a broken schedule surfaces as BrokenBarrierError, not a hang.

Files to look at first

tests/agy-start-cli.test.py: _run_writers_staged_then_renamed, _naive_seed, and the two tests that call them (lines ~113-185).

What user behavior or bug does this prove

No user-visible behavior; it removes a flake that fails unrelated PRs in python standalone tests, and makes the production-path test deterministically catch a regression from unique mkstemp staging back to a fixed shared name.

Feature/documentation consistency

N/A — test-only change; no user-visible feature or capability changes.

Before/after evidence

Whole file run 20× under CPU load (yes > /dev/null & ×4) on a 10-core macOS host.

Before — parent commit 1be758ba5 (origin/main):

run 1: OK
run 2: OK
run 3: FAIL(control)
run 4: OK
run 5: FAIL(control)
... (runs 6-20 OK)
label=before pass=18 fail=2 control_failures=2

One of those failures (run 3):

FAIL: test_concurrency_test_would_catch_a_naive_non_atomic_writer (__main__.OnboardingSeedTests.test_concurrency_test_would_catch_a_naive_non_atomic_writer)
  File ".../tests/agy-start-cli.test.py", line 164, in test_concurrency_test_would_catch_a_naive_non_atomic_writer
    self.assertTrue(
        failures,
Ran 26 tests in 1.685s
FAILED (failures=1)

After — HEAD 34caa0de9:

run 1: OK
...
run 20: OK
label=after pass=20 fail=0 production_test_failures=0 control_failures=0

With the live worker's SUTANDO_* environment scrubbed (bash -c "env -u SUTANDO_… python3 tests/agy-start-cli.test.py; echo exit=$?"):

Ran 26 tests in 3.391s

OK
exit=0

The committed harness catches a fixed-staging-name production writer deterministically

Source mutation at HEAD in src/agent/agy/onboarding_seed.py, keeping the writer's mode handling and replacing only the unique staging:

-    fd, tmp = tempfile.mkstemp(dir=parent or ".", prefix=os.path.basename(path) + ".", suffix=".tmp")
+    tmp = path + ".tmp"  # MUTATION: one fixed staging name shared by every caller
+    fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)

Same 20× loop under load against the mutated source:

label=mutated pass=0 fail=20 production_test_failures=20 control_failures=0
FAIL: test_concurrent_writers_do_not_collide_on_shared_staging_name (__main__.OnboardingSeedTests.test_concurrent_writers_do_not_collide_on_shared_staging_name)
AssertionError: Lists differ: [FileNotFoundError(2, 'No such file or dir[310 chars]ry')] != []
FAILED (failures=1)

The production-path test fails 20/20 and nothing else does; the control passes 20/20 because it tests the naive writer, not seed(). Mutation then reverted (git checkout -- src/agent/agy/onboarding_seed.py; the PR diff contains no src/ line) and the file passes:

Ran 26 tests in 2.444s
OK

Tests run

python3 tests/agy-start-cli.test.py                      # Ran 26 tests ... OK  (×41 incl. the two 20× loops)
git diff origin/main...HEAD | bash scripts/review-checks.sh
# prose-cap: PASS (comment blocks within cap 2, exts .py)
# review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)
git diff --check                                          # clean

Live path: N/A — test file only.

Closes #4977
Closes #4928

🤖 Generated with Claude Code

[john-the-dev]

Approving at exact head d7964fac7662613b13796cb453ee7fdcc97f11ca. This is test-only, one file.

Why it's deterministic now:

• staged.wait() sits between the shared .tmp write and os.replace, so all 8 writers have staged before any of them renames.
• The first os.replace consumes the single shared p + ".tmp", and the other 7 find it gone. That gives exactly writers - 1 FileNotFoundError on POSIX, every run.
• No ordering assumption is left for disk speed to break.

What I ran, from a detached worktree at the head:

• python3 tests/agy-start-cli.test.py passed 10/10 (26 tests, OK).
• Mutant with staged.wait() removed: 10/10 runs fail. The exact-count assertion depends on the barrier, so the control can't pass vacuously.

Failure mode: if a writer dies before the barrier, the other writers raise BrokenBarrierError after at most the 30 s timeout. The count/type assertion then fails and prints repr of the failures, so a broken run fails loudly rather than hanging. Returning the exception object (not its repr) is what makes the assertIsInstance check possible.

Non-blocking: this relies on POSIX os.replace semantics for a missing source. On a Windows runner, concurrent opens of one .tmp could raise PermissionError instead. The Windows lane is skipped today, so this is only a note if that ever changes.

CI at review time: 15 pass, 8 pending, 1 skipping. This approval doesn't cover the pending lanes.

[keweichen]

Requesting changes on d7964fac7662613b13796cb453ee7fdcc97f11ca.

Blocker: force the schedule through the production-path test, not only its synthetic stand-in

The barrier makes the local naive_seed deterministic, but it no longer proves the contract stated immediately above it: “the naive shared-tmp-name writer this replaced must make the assertion above actually fail.” The production-path test at tests/agy-start-cli.test.py:113-137 still calls self.seed.seed(path) under ordinary executor scheduling. The new barrier exists only inside the separate local writer at :139-177. Therefore a future production regression from unique mkstemp names back to one fixed .tmp can serialize and pass the real assertion while this synthetic control independently passes.

I verified the distinction with an exact-head source mutation that kept the production writer's mode handling but replaced unique staging with one fixed .tmp: the real production-path test failed 16/20 but passed 4/20, while the new synthetic control passed 20/20 regardless. So the committed control does not make the sibling test's source-mutation discrimination deterministic; the PR body's three mutated runs happened to take the colliding schedule.

Please put the forced interleaving on the actual self.seed.seed path (for example, wrap/patch its os.replace with an 8-party barrier in the production concurrency test), then show the same committed harness passes with unique sources and deterministically fails with a fixed shared source. Reshape or remove the separate control so it exercises that same path rather than asserting a stronger relationship than it proves.

Evidence and scope

• Confirmed the bug on parent/current main 1be758ba5: the old focused control failed on my run 131 with failures == []; the two cited #4976 CI attempts failed identically and passed on rerun at the same SHAs.
• The new local barrier logic itself is sound: exactly one rename succeeds and seven raise FileNotFoundError; the focused test passed 250/250 here and an independent run passed 1,000/1,000. The full file collected all 26 tests and passed.
• git diff --check and review-checks.sh pass; the diff is test-only, so there is no live-path or user-facing blast radius.
• Current CI's changed agy-start-cli.test.py passed. The red browser-persistent and slack-proactive-claim-requires-owner suites are untouched and are not this finding, but the required aggregate gate still needs a green rerun before merge.

Non-blocking bookkeeping: #4928 predates #4977 and reports this exact flake with the same barrier suggestion. Please link/close that duplicate too so the original report is not left open after this PR lands.

Verdict: changes requested; not merge-ready until the committed test proves the production-path mutation deterministically and required CI is green on the resulting head.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[sonichi]

CI at d7964fac7: the two red lanes were tests/browser-persistent.test.py and tests/slack-proactive-claim-requires-owner.test.py — neither touched by this PR (which changes only tests/agy-start-cli.test.py, green in its lane); both passed on the failed-jobs rerun (07:4x PDT). Tracked separately so the rerun is not read as a fix.

[github-actions]

@cla-assistant check

[sonichi]

@keweichen — addressed at 34caa0de9 (PR body rewritten to match).

What changed: both tests now go through one harness, _run_writers_staged_then_renamed(writer, path), which patches the module's os.replace (the seam test_write_failure_cleans_up_staging_file_and_reraises already uses) with a wrapper that waits on an 8-party threading.Barrier before renaming. So all eight writers stage before any rename, on the real self.seed.seed as well as on the naive writer; the two tests differ only in the writer passed in. The production test asserts zero exceptions, intact 500 000-char JSON, all fields true, and no stray staging file; the control runs the naive fixed-.tmp writer through the same harness and asserts exactly 7 FileNotFoundError. No src/ change.

Evidence (20× each, whole file, under yes ×4 load):

• real seed() at HEAD: pass=20 fail=0 production_test_failures=0 control_failures=0
• seed() mutated to a fixed .tmp with mode handling kept (your mutation): pass=0 fail=20 production_test_failures=20 control_failures=0 — the production-path test now fails every run, not 16/20.

Also added Closes #4928 per your bookkeeping note.

[john-the-dev]

Re-approving at exact head 34caa0de9b11e3b79e7cae394130d8ca2a9facb7. The delta from my d7964fac7 approval is one commit in tests/agy-start-cli.test.py (+44/−41).

What changed: both concurrency tests now go through _run_writers_staged_then_renamed. It patches the module's os.replace to wait on an 8-way Barrier before renaming. The production test and the mutation control therefore face the identical forced interleaving and differ only in the writer. The production test also now asserts that no staging files are left behind.

What I ran, from a detached worktree at head:

• The test file: 10/10 pass.
• Mutant: src/agent/agy/onboarding_seed.py:69, with tempfile.mkstemp(...) replaced by a fixed path + '.tmp' opened via os.open. Result: 5/5 runs fail, on test_concurrent_writers_do_not_collide_on_shared_staging_name. So the production test, not just the control, now catches the shared-staging-name bug deterministically.
• My first mutation attempt used a regex that silently missed the line, and that run showed 0/5 failing. I re-applied it by line and confirmed with git diff --stat (1 line changed) before trusting the 5/5.

Hazard: if seed() ever calls os.replace more or fewer than once per writer, the Barrier breaks after its 30 s timeout. The test then fails rather than hangs, which is the right failure mode.

[keweichen]

Re-reviewed exact head 34caa0de9b11e3b79e7cae394130d8ca2a9facb7: APPROVE. This replaces my stale request-changes review on d7964fac7.

The prior mutation-discrimination blocker is resolved. Both the real self.seed.seed path and the naive fixed-.tmp control now use the same helper and the same patched-os.replace eight-way barrier; only the writer differs.

Evidence:

• python3 tests/agy-start-cli.test.py -v — 26/26 passed.
• Unmutated production test — 250/250 passed; naive control — 250/250 passed.
• With production mutated from unique mkstemp staging to path + ".tmp", the production-path test failed 250/250 (exactly seven FileNotFoundErrors per representative failure), while the naive control stayed green 250/250. The full mutant suite had exactly the intended one failure.
• After reverting the mutation, the production test passed 100/100 and the checkout was clean.
• git diff --check, scripts/review-checks.sh, current CI, and CLA are green.

No blocking findings remain. The change is test-only; the barrier's failure mode is bounded by its 30-second timeout.