Conversation
Both probes kept their gates in a per-path map but signalled through a single process-wide Notify, so a sibling test whose write reached its own stall point woke a different test's waiter. That waiter aborted its save before the save had reached the gate at all, which surfaced as the lost-update assertion in store::fs::tests_recovery::a_racing_load_does_not_lose_an_orphaned_commits_update and, when the intended waiter got no notification of its own, as a test binary parked forever. Both are reproducible on main by running the four probe-driven tests together. arm/arm_commit now return a Gate owning the release sender and its own Arc<Notify>, created at arm time, and maybe_block wakes that path's Notify. A waiter that holds the Arc from arm time can neither miss nor steal a notification, and notify_one still stores its permit, so arm-then-park-then-wait stays race-free. The cleanup probe gets the same per-directory Notify; fs_send_probe already worked this way.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 3 days. Promotion and pricing detailsOn-demand reviews are free for the next 3 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 33 seconds for your next included review. Limit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0133 · 385,327 in / 11,347 out · 6,135 cached (2%) · ladder/vectors, gpt-5.6-luna, deepseek/deepseek-v4-flash · 722 embedded
critique: $0.0046 · 166,653 in / 4,537 out · 0 cached (0%) · gpt-5.6-luna, deepseek/deepseek-v4-flash
security: $0.0051 · 185,910 in / 2,253 out · 3,575 cached (2%) · gpt-5.6-luna
tests: $0.0020 · 20,113 in / 1,841 out · 1,280 cached (6%) · deepseek/deepseek-v4-flash
description: $0.0015 · 12,651 in / 2,716 out · 1,280 cached (10%) · deepseek/deepseek-v4-flash
| BLOCKED.notified().await; | ||
| /// Arms a one-shot stall for the next [`stage_atomic_bytes`] write | ||
| /// targeting `path`. Returns the gate the test waits on and releases. | ||
| pub(crate) fn arm(path: &Path) -> Gate { |
There was a problem hiding this comment.
Preserve the stall-probe API for existing callers
arm previously returned Sender<()>, while this signature returns Gate; likewise, wait_blocked and wait_blocked_commit were removed. The rest of the repository's existing probe tests still compile against the old return type and wait functions, so calls such as arm(...).send(()) and wait_blocked().await now fail to compile. Update every caller in the same change, or retain compatibility wrappers while migrating callers.
[RULE] api-compatibility ·
There was a problem hiding this comment.
Every caller is migrated in this same commit — the diff includes fs_recovery_tests.rs, fs_recovery2_tests.rs and fs_company_store_tests.rs, which are the only files that ever used these functions.
git grep for the old shapes (wait_blocked(), wait_blocked_commit(), arm(..).send() returns nothing across crates/; the call sites now read gate.wait().await and gate.release().
Compilation confirms it independently: Rust, Rust (mail), Rust (mongodb), Rust (openhuman, tinymemory) and Gated host binary are all green on this head, and cargo clippy --all-targets -- -D warnings exits 0 locally. A missed caller would fail to compile rather than pass.
Compatibility wrappers would defeat the fix: the defect is that a waiter cannot tell which path parked, so keeping a path-less wait_blocked() would preserve the exact race this removes.
tinysweeper
Bot
added
the
priority: p0
1 participant
Summary
mainis red on theRust (openhuman, tinymemory)lane:The store's lost-update guard is fine. The test probe that drives it is not.
fs_stall_probe.rskeys its gates by path, but signals "an armed write has reached its stall point" through a single process-wideNotifyper gate set. Six tests use the commit gate and six use the stage gate, across three files, and cargo runs them as parallel threads in one binary — so a sibling test's commit parking wakes a different test'swait_blocked_commit(). The waiter has no way to tell which path parked.In the failing test that plays out as: it arms its gate, spawns the stale
save, andwait_blocked_commit()returns on someone else's notification. It then aborts a task that has not yet reachedpath_lock(bundle.dir()), so the abort kills it outright — no lock held, nothing staged, no orphaned commit to race. Theassert!(joined.is_cancelled())guard still passes, because the task genuinely was cancelled. The fresh caller then correctly loadslifecycle: "running"and saves it back, producing the observed mismatch.The same theft has a second face: the intended waiter never receives its notification, and
wait_blocked*()has no timeout, so the test binary parks forever. That one surfaces as a job timeout rather than a red assertion — two runs wedged at 0% CPU for the better part of an hour while diagnosing this.Rarity is a function of how many tests share the binary. The full 8437-test suite passes; the probe tests only collide when few enough neighbours are scheduled alongside them.
API Or Behavior Changes
None — test-support code only. No product source changed;
fs.rsis touched for a module doc line alone.arm/arm_commitnow return aGateowning both the releaseSenderand its ownArc<Notify>, created at arm time. A waiter holds the signal it is waiting on, so it can neither miss a notification nor consume another path's.maybe_block/maybe_block_commitnotify theArcthey removed from the map. The two gate sets stay separate, for the reason already documented: stage and commit stall on the same destination path at different points in onesave.Call sites changed only in how they wait and release. No test assertion was altered, and there are no sleeps, no timeouts, no global serialisation of the probe tests, and no
#[ignore]— any of those would have hidden a race in a guard that exists to catch silent data loss on the persistence path.fs_cleanup_probe.rscarries the same structural defect and is fixed the same way. Stated plainly: it has one caller today, so it could not yet steal from a sibling — that change closes the family rather than a live failure.fs_send_probe.rsalready used the per-pathArc<Notify>shape, which is the precedent this follows.fs_append_probe.rsandfs_fault_probe.rshold noNotifyand are untouched.Tests
The point of this change is the before/after on one command:
--lib store::(439 tests): green on six runs.--lib(8437 tests) passed twice before the fix, which is why CI only catches this intermittently.cargo fmt --all -- --check— exit 0, unpiped.cargo clippy -p opencompany-core --features openhuman,tinymemory --all-targets -- -D warnings— exit 0.Honest limit: the failure is scheduling-dependent, so no iteration count proves absence. What carries it is the mechanism — after this change a waiter owns its own signal, and the theft that produced both the wrong assertion and the hang is not expressible.
Documentation
The existing doc comments on these functions explain real mechanism (the stored-permit ordering property, why the two gate sets are separate maps) and are preserved and updated to stay accurate.
🤖 Generated with Claude Code