fix(test): scope the notification-copy ordering triggers to the destination open - #8992
Conversation
…nation open (kirodotdev#8893) Both ordering tests in TestNotificationCopyWhenNoLiveFileExists patch os.open process-wide and fired their trigger on the FIRST O_CREAT open seen anywhere. Locally that open is the copy's own destination, taken while the copy occupies the single notification worker, so the queued append cannot run and the wait times out as the test intends. On a loaded CI shard the first creating open can belong to a foreign thread at a moment when the worker is still free -- the append then runs immediately and the assertion reads a scheduling accident as a broken ordering guarantee (observed on shards for PRs whose diffs never touch this path). Scope the trigger to the destination path itself: it is the one open the copy performs while it holds the worker, so it is the only trigger that measures the serialisation these tests are about. No retry, no sleep -- the fix removes the foreign-trigger case instead of tolerating it, and a new test keeps the loaded-shard condition deterministic: a churn thread is confirmed to have done a creating open before the merge starts, guaranteed-foreign under the old shape, invisible under the scoped one.
github-actions
Bot
added
fork
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of The base code confirms the claims: Design-Verdict: PASS Root-cause fix at the right layer: the trigger now measures the destination open it was always meant to, and the flake condition itself became a deterministic test. [DESIGN-REVIEWED] 599c5cf |
github-actions
Bot
added
readiness: checking
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsEverything checks out. The production path confirms No findings. [OPUS-REVIEWED] 599c5cf |
github-actions
Bot
added
readiness: passed
iamwhatever
left a comment
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: test (1 file). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-only change to test/test_snapshot.py scoping the process-wide os.open trigger to the copy's own destination path so a foreign creating open on a loaded shard no longer fires the ordering assertion (#8893); no production file touched. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.
github-actions
Bot
removed
the
readiness: passed
Both notification-copy ordering tests measured the guarantee AFTER `_do_merge` returned, with `assert not ran_during_copy.is_set()`. By that point the copy has released the single `notif-io` worker and the worker runs the queued append -- which is the CORRECT behaviour the tests exist to prove, and it sets the same event. So the assertion was really measuring whether the main thread reached it before that legitimate run: a footrace between the main thread finishing `_do_merge` and the worker draining one trivial job. On a contended shard the main thread loses, and shard 4 reddened for PRs whose diffs never touch this path. #8893/#8992 found and fixed a different cause in the same two tests -- the trigger fired on the first `O_CREAT` seen anywhere, so a foreign thread's open submitted the append while the worker was still free. That scoping is correct and is kept. It narrowed the flake without closing it because the observation point was left alone. The fix is the shape #8992 already introduced in `test_the_ordering_trigger_ignores_a_foreign_O_CREAT_open` and did not retrofit: capture the wait's own return value INSIDE the trigger, while the copy still holds the worker, and assert on that. In that window only the defect can set the event, so the measurement no longer competes with correct behaviour. Production is unchanged and sound: `_copy_notifications` wraps its whole body in `_serialise_with_notification_writes`, so the destination `O_CREAT|O_EXCL` open genuinely happens on the worker the copy occupies. The assertion is not weakened -- with the serialisation removed all three tests fail `[True] == [False]`.
2 participants
Problem / Motivation
Two ordering tests in
TestNotificationCopyWhenNoLiveFileExists(test/test_snapshot.py) failed on a loadedBackend Tests (3.12, 4)shard for PR #8887, whose diff never touches the snapshot/notification path:The class passes locally on the same commit. A second occurrence fired on shard 4 for PR #8896 (run 34014923783) while the same test passed 5/5 locally at that PR's exact sha — and a third on the very next round of the same PR (run 34017247801,
test_a_FRESH_gateway_still_orders_the_copy_against_a_deliveryagain, taking the Coverage Gate down with it via the missing shard upload). Three firings across three unrelated diffs in under 24 hours: the failure follows shard load, not the diff.Why it matters
Every PR in the repo runs these tests; a scheduling-dependent trigger turns busy CI days into false-negative red boards and burns contributor CI rounds on failures their diffs cannot cause. The class is one day old (#8576), so this is its first hardening pass — left alone, it becomes a standing flake with a growing rerun tax.
What changed (motivation → approach → change)
os.openprocess-wide and fire their delivery trigger on the firstO_CREATopen observed anywhere (if flags & os.O_CREAT and not fired). Locally that first open is the copy's own destination — taken while the copy occupies the single notification worker, so the queued append cannot run and the bounded wait times out exactly as intended. On a loaded shard, the first creating open can come from a foreign thread while the worker is still free: the append runs immediately,ran_during_copysets, and the assertion reads a scheduling accident as a broken ordering guarantee.os.fspath(path) == dst). The destination is opened by full path at a single site (snapshot.py::_install_notifications,os.open(dst_path, O_CREAT|O_EXCL|...)), and it is the one open the copy performs while it holds the worker — so it is the only trigger that measures the serialisation these tests exist to prove. Per the issue's ask and the testing conventions: no retry, no sleep — the foreign-trigger case is removed, not tolerated.Tests
test_the_ordering_trigger_ignores_a_foreign_O_CREAT_open(new): keeps the loaded-shard condition as a deterministic test. A churn thread is confirmed to have completed a creating open before the merge starts, so under the old first-O_CREAT-anywhere shape the trigger is guaranteed foreign and the test fails every run (reproduced before the fix: the churn open fires the trigger and the pin fails deterministically). Under the scoped shape the churn is invisible however the scheduler interleaves it; asserts the trigger fired on exactly the destination, the worker-held measurement stayed false (captured inside the trigger from the wait's return value, where the append's legitimate post-copy run cannot race it), and the note survived. Soaked 20/20 solo runs; an event-re-read variant of the same assertion failed the identical soak, which is what forced the in-trigger capture.test_a_note_delivered_during_the_copy_survives_the_READERandtest_a_FRESH_gateway_still_orders_the_copy_against_a_delivery(updated): destination-scoped triggers; their assertions — including theassert firedproved-something guard — are unchanged.test_dashboard.py,test_notification_phase5.py): 68 passed.Manual verification
N/A — unit coverage sufficient: the flake condition itself is now a deterministic in-suite test, and the affected surface is test-only (no production code changed).
Related Issues
Fixes #8893
Pattern harvest
Rule candidate: review-prompt
Pattern: "a process-wide patch of a filesystem/syscall entry point must scope its trigger to the specific path or descriptor under test — 'the first call observed' is a scheduling assertion on loaded runners, not a behavioral one". Flag new tests that monkeypatch
os.open/builtins.openand branch on flags alone without binding to the path they mean.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).