fix(test): poll job store off the event loop in test_job_routes

[dwu96]

Symptom

Backend Tests (Windows) shard 2 intermittently fails
test/test_job_routes.py::test_get_existing_run_is_200 with
PermissionError: [Errno 13] Permission denied on the run's record JSON under the pytest tmp
(app-data/jobs/<hex>.json). Non-deterministic: the same shard over the same backend code passed on
#7686 and failed on #7696 minutes apart, with no Python in either diff. Linux shards never see it.

Root cause

The test file's polling helpers _wait_terminal / _wait_status were plain def helpers calling
sdk.get(run_id) + time.sleep(0.02). Every caller is an @pytest.mark.asyncio coroutine, so the
poll read the job record on the event loop. On that path read_bytes_with_retry
(src/kiro_crew/atomic_write.py) deliberately re-raises PermissionError instead of sleeping the
one loop for its retry budget (on_event_loop() gate) — so the retry budget is effectively one
attempt. When the job worker's concurrent os.replace (the last step of atomic_write) happened to
be in flight, the Windows sharing violation surfaced as an uncaught PermissionError out of
sdk.get and reddened the test. JobStore.read catches only
(FileNotFoundError, NotADirectoryError, ValueError), which is why the traceback names the final
.json, not a .tmp.

Production never sees this because every route offloads the read
(await asyncio.to_thread(sdk.get, …) at job_routes.py:256, 287, 295, 311) — and JobStore.read's
docstring states that off-loop invariant explicitly. The test file was the only caller breaking it.

POSIX permits reading a file mid-replace, which is the entire "Windows only" observation — there is
no Windows-specific logic branch involved.

The earlier triage hypothesis on #7703 ("a background JobSDK worker still holds an open handle
while the test/teardown replaces or unlinks it") is superseded: the failure is on the READER side,
not the unlink, and the writer's replace is already retried (replace_with_retry runs on the worker
thread with its full budget). #7296 is not a duplicate (the issue-summary bot suggested the
link): that one is annotation-only, has no test identity, and an empty failed-log archive.

Fix (test-only, by design)

Following the in-tree precedent the routes already set (job_routes.py's asyncio.to_thread
offload):

• Convert _wait_terminal / _wait_status to async def, polling via
  await asyncio.to_thread(sdk.get, run_id) and await asyncio.sleep(0.02) (the await also stops
  the poll from monopolising the loop thread, which time.sleep did). Deadline semantics and the
  AssertionError message shapes are unchanged.
• await them at all 15 call sites — the 7 that existed when the branch was cut plus the 8 added
  by fix(jobs): let a requested cancel survive a fresh read of the run #7680, which merged mid-flight and reused the helpers synchronously (caught as 3 failing tests
  on rebase; a textual-clean, semantically-conflicting merge). The task spec counted "9 call sites"
  from a grep that included the two def lines; the re-verified count at branch time was 7.
• Audited the file for any other synchronous sdk.* read from an async test (get, list_active,
  list_recent, iter_runs): none — the only other match is a monkeypatch.setattr(sdk, "list_recent", …), not a read. The identically-named helpers in
  test_workflows_nudge_wiring.py / test_workflows_registry.py are already async and unrelated
  (out of scope per spec).
• Recorded the convention in docs/system-specs/common/testing-conventions.md (§ Async tests):
  never poll a synchronous store read from an async test; offload like the routes do. The invariant
  previously lived only in JobStore.read's docstring, which is what let this happen.

Deliberately not done, per the spec's correctness analysis: adding PermissionError to
JobStore.read's except clause (would turn an existing record into None — _wait_terminal reads
that as "not terminal yet" and list_active as a vanished record, the silent-skip hazard
iter_runs' docstring warns about), and weakening atomic_write.py's on_event_loop() gate (would
put a sleep on the gateway loop). Both trade a loud test failure for a silent production one.

Verification

• Red-before-green, without a Windows shard —
  test_on_loop_read_propagates_permission_error_offloaded_read_retries proves the invariant
  directly: with platform_compat.IS_WINDOWS shimmed true (module attribute only — never a global
  os.name patch, which breaks Path.home() on POSIX) and the record's first Path.read_bytes
  raising PermissionError, a synchronous sdk.get from the loop propagates the error, while both
  converted helpers retry off-loop and succeed.
• Shape guard — test_wait_helpers_are_coroutine_functions asserts both helpers are coroutine
  functions, so a future revert to the synchronous shape fails loudly instead of reintroducing an
  unreproducible Windows-only flake.
• Mutation checks (all three killed, both directions — green on fix, red on mutant):
  1. _wait_terminal's poll made synchronous (await asyncio.to_thread(sdk.get, run_id) →
     sdk.get(run_id); the helper-internal read IS the offload — call sites hold no to_thread):
     injected-PermissionError test FAILED with the propagated PermissionError.
  2. Same mutation on _wait_status: injected-PermissionError test FAILED on its _wait_status leg.
  3. Both helpers fully reverted to main's synchronous bodies (guard tests kept): shape guard FAILED
     (assert False on iscoroutinefunction).
• Affected suite in full: test/test_job_routes.py = 28 passed, 0 failed (22 pre-existing +
  4 from fix(jobs): let a requested cancel survive a fresh read of the run #7680 + 2 new) on the fix branch; 26 passed, 0 failed on an origin/main worktree.
  Failing-test id sets byte-identical both directions (both empty).
• Full backend suite both directions (fix branch vs origin/main git worktree, -n auto):
  fix 225 failed + 2 errors, main 224 failed + 2 errors; id-set diff = exactly one test
  (test_pod_e2e_harness_paths.py::test_health_refuses_a_foreign_port_holder_and_names_the_conflict),
  which passes standalone on the fix branch — a load flake of its 1-second
  POD_E2E_HEALTH_TIMEOUT under two concurrent full suites, not a regression. Excluding it, the
  226 remaining failing ids are byte-identical both directions, all pre-existing environmental
  (AF_UNIX path length, missing host node for installer tests, platform-context composition).
• Frontend cross-surface guards: 197 spec files / 5358 tests, all passed (node 22).
• Lint/format/types: black 26.3.1 (repo gate script), isort, flake8, mypy — clean.

No frontend surface changes — backend test file + docs only; a still frame cannot show a
concurrency fix. Evidence is the injected-PermissionError invariant test above.

Pattern harvest

Rule candidate: an invariant that callers must uphold ("only call this off the event loop") must
be enforced or conventionalised, not just documented at the callee — JobStore.read documented
the off-loop contract but nothing checked it, so the first synchronous test-side caller compiled,
passed on POSIX, and flaked only on Windows CI. This PR adds the convention to
testing-conventions.md and a shape guard in the test file. Knowingly out of scope (already async,
different subject): the _wait_terminal helpers in test/test_workflows_nudge_wiring.py and
test/test_workflows_registry.py; no other sync store-read polls from async tests were found in
test/test_job_routes.py, and other test modules were not audited per the spec's
no-opportunistic-widening constraint.

Fixes #7703

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of c2cd6611846be9b37f12c0332bf471167fc7e3e5 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Root-cause test-only fix matching the production offload pattern, with the invariant now falsifiable on POSIX; alternatives were weighed and correctly rejected.

Suggestions

• The off-loop contract is still only convention + a one-file shape guard; a test-mode assertion inside JobStore.read (fail loudly when called on a running loop under pytest) would enforce it for every future test module — worth a follow-up, as your own pattern-harvest note implies.

[DESIGN-REVIEWED] c2cd661

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed c2cd6611846be9b37f12c0332bf471167fc7e3e5 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] c2cd661

Verdict parsed from the review's SHA-scoped output markers for commit c2cd6611846be9b37f12c0332bf471167fc7e3e5.

False positive or not applicable? A repository writer can comment:
/ai-review override fable c2cd6611846be9b37f12c0332bf471167fc7e3e5: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of c2cd6611846be9b37f12c0332bf471167fc7e3e5 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] c2cd661

False positive or not applicable? A repository writer can comment:
/ai-review override gpt c2cd6611846be9b37f12c0332bf471167fc7e3e5: <one-sentence reason>