Repository navigation
fix(protocol): share the probe's round so a bound family evaluates once per revision - #303
Merged
Merged
Conversation
…n and make the sharing tests deterministic bound_subscriptions_share_one_evaluation_and_get_only_their_rows failed on CI with zero shared evaluations for the write it counted (left 1, right 2). A lab run pinned to 4 cores under CPU load reproduced it: 4 of 40 runs of the standing_query_tests binary. Traces showed the probe judged too slow on one wall-clock sample (72.6 ms against twice a 29.6 ms own average, while own evaluations ranged 11-65 ms), so the family never shared. Sharing is cost-based by design; the test asserted that verdict on the host's timing, and then slept a fixed 200 ms hoping the probe had been judged. The traces also showed an engine gap. A probe's evaluation was not the round of its revision: a view refreshing at that revision after the verdict, which two-relation commits make likely (one refresh per relation), evaluated the lifted query again. Two evaluations at one revision, against the documented one, and the +2 the issue describes. - A probe now evaluates its revision's round, so views that then share at that revision read it. A round judged too slow is let go at once rather than kept, rows and all, until the family's next probe. - A shared evaluation counts once judged: whoever sees the count sees the verdict, which gives tests a signal in place of the sleep. - Handler::with_sharing_regardless_of_cost lets a test or benchmark keep the probe and rounds but set the cost comparison aside. The integration tests that count evaluations use it, wait for the judged count, and assert one evaluation for the probe's revision, then one for the next write. Cost judgment stays covered by the unit tests, which set the costs. - a_probe_too_slow_to_share_never_makes_a_view_wait pinned the own cost before an own evaluation folded a measured sample into the average, and failed 18 of 200 runs under load; it now pins it after. After: the standing_query_tests binary 40 of 40 under the same load, and the parameterized unit tests 200 of 200. Fixes #289
…pass behind test-support
…e was a flake in `backup_restore_tests::online_export_of_a_running_server_restores_what_it_served`, and this PR's subscription changes did not cause it. The root cause is a real bug in `DataDirLock`, so I fixed it there rather than in the test. **Invariant violated:** once a `DataDirLock` is dropped, the data directory can be locked again at once. **Why it broke:** `DataDirLock` released its `flock` only by closing its file handle. When another thread spawns a child process, the child briefly shares every open file until its exec. During that window the lock stays held even though our handle is closed. In CI, the concurrent test `cli_refuses_a_live_directory…` was spawning the `inputlayer-backup` command-line tool. Meanwhile this test had just dropped its `populate` engine, and its `open(&data)` failed with `DataDirLocked`, owner being the test's own process. In a scratch experiment with a thread spawning processes, 10,560 of 20,000 acquire-right-after-drop calls were refused. **Fix** (`src/storage/data_dir_lock.rs`): - Added `impl Drop for DataDirLock` that calls `fs4::FileExt::unlock(&self.file)`. This releases the lock on the shared file, so a child spawned at the same moment no longer keeps it held. - Renamed the field `_file` to `file`, because clippy rejects using an underscore-prefixed field. - Updated the module and field docs. `DataDirLock` has a single drop path, and every user of the lock (the engine and the backup CLI) goes through it. So this one change covers all of them. **Regression test:** `drop_releases_lock_while_processes_spawn` runs 2,000 acquire-and-drop cycles while another thread keeps spawning processes, and asserts that none are refused. It failed without the fix (1,031 refused) and passed with it in 6 runs. **Verified locally:** - `cargo fmt --check` passes. - `cargo clippy --workspace --all-targets --all-features -- -D warnings` is clean. - `cargo clippy -p inputlayer -- -D warnings` is clean. - `cargo test -p inputlayer --test backup_restore_tests` passes (3 of 3). - The `storage` library unit tests pass (483)
…was a flake in `credential_revocation_tests::revocation_during_a_proof_withholds_it`, a test already on main. This PR's changes (subscription sharing, DataDirLock, the test-support feature) did not cause it; the bug was in how the test installed its tracing hook. **Invariant violated:** once the test installs its hook, the server's `ws_execute_end` event must reach the hook on the test's thread, so the key is revoked at the output boundary. **Root cause:** the test installed its hook as a thread-local subscriber (`set_default`), making it the only registered dispatcher. In that case tracing-core 0.1.36 takes its `JustOne` path, which works out a callsite's interest from whatever subscriber the registering thread has. The other 13 tests in this binary run in parallel. If one of them reached `ws_execute_end` first, after the hook made the max level TRACE, interest came from that thread's `NoSubscriber`, was `never`, and was cached for every thread. The hook then never ran, the key was never revoked, the result went out, and `drain` waited the full 20s, giving CI's `timed out waiting for a frame` at line 242. **Reproduced:** a temporary edit ran an execute on another thread's server right after the hook was installed. It failed with the same timeout panic before the fix and passed after it. The edit was removed. **Fix** (`tests/credential_revocation_tests.rs` only): - The hook is now installed with `set_global_default`, so callsite interest no longer depends on which thread registers first. - It is armed through a `static ARMED: Mutex<Option<(ThreadId, Arc<Handler>)>>` and acts only on the test's own thread. It revokes once and releases the lock before revoking. - A per-layer `Targets` filter enables only INFO events from `inputlayer::protocol::rest::handlers::ws::execute`, so the global subscriber does not turn on tracing in other tests' code. - The test still uses the current-thread runtime so the server task runs on the test's thread, and its doc comment now explains why the subscriber is global. **Verified:** - `cargo fmt --check` passes. - `cargo clippy -p inputlayer --all-targets --all-features -- -D warnings` is clean. - The full `credential_revocation_tests` binary passed 10 of 10 runs (about 8.5s each). One earlier run of 10, before I added the target filter, failed in a different timing-based test, `revocation_during_evaluation_withholds_its_rows`, and that run took 117s against the usual ~9s. The likely cause is load on the shared machine, but a global subscriber with no filter would also add tracing overhead. With the filter in place, no failures occurred
This was referenced Oct 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
I want you to really hyper focus on making the symbolic engine, you know, crisp and really truly production ready. (Context: #289: bound_subscriptions_share_one_evaluation_and_get_only_their_rows counts 2 shared evaluations instead of 1 on CI, intermittently, and now fails CI on unrelated PRs (#287, #293, #294), blocking the merge train. Workers suspect the sharing decision is cost-dependent.)
What Changed
Roundinstead of running a separate evaluation. Views that start sharing at the probe's revision read the probe's result, so the lifted query runs at most once per revision. Before this, the probe and a refreshing view could each evaluate the same revision, which is how test: bound_subscriptions_share_one_evaluation_and_get_only_their_rows flakes on slow CI runners #289 saw 2 shared evaluations where 1 was expected. A round the family stops sharing after judging it (too slow or failed) is released fromlatestright away, so its rows don't stay in memory until the next probe. Theshared_evaluationsmetric is now recorded after a round is judged, so anyone who reads the count also sees the sharing decision.test-supportCargo feature, turned on only for test targets through a self dev-dependency. It gatesHandler::with_sharing_regardless_of_cost(), which keeps a family sharing whatever its rounds cost (it still probes, and still stops on a failed round). Shipped builds don't include it and always use the cost-based decision.bound_subscriptions_share_one_evaluation_and_get_only_their_rows) now run on a server built with this option through the newstart_server_adjustedharness. They wait for a given number of judged evaluations instead of sleeping for a fixed time, and assert exact counts. New unit tests cover a view reading the probe's round and the release of a round judged too slow.Risk Assessment
✅ Low: The fix is narrow and fixes the real cause: views that start sharing at the probe's revision now read the probe's round from
latest, so they no longer evaluate a second round at that revision. The metric is now counted after the verdict, so tests can wait on it reliably. The cost bypass is limited to test builds behind thetest-supportfeature, and rounds the family doesn't share are released for probe rounds and rounds evaluated for waiting views alike. Both match the recorded decisions, and tracing the probe/view/sharing state transitions found no reachable wrong result.Testing
I ran the five standing-query sharing integration tests, which start a real server and subscribe real clients over WebSocket. All five passed in a normal run. I then repeated them in a loop while 40 CPU-hog processes loaded the 32-core host, to mimic a busy CI machine. At the last check, 11 of 15 repeats had finished and none had failed, including the test that used to flake. I did not wait for the last 4 runs. I also did not reproduce the old flake on the base commit, or confirm that a build without the test-support feature hides the new method. The evidence is the load log and a summary file in the evidence directory; there is no UI to screenshot.
cargo test -p inputlayer --test standing_query_tests parameterized: bound_subscriptions_share_one_evaluation_and_get_only_their_rows okEvidence: Repeated sharing-test runs under CPU load (raw log)
~/.no-mistakes/evidence/01M45W886NYM2TDN3WQB69R2KS/head-under-load-summary.txt)Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
cargo test -p inputlayer --test standing_query_tests parameterized: bound_subscriptions_share_one_evaluation_and_get_only_their_rows okcargo test -p inputlayer --test standing_query_tests parameterized(5 integration tests, each against a real server over HTTP/WebSocket)target/debug/deps/standing_query_tests-* parameterized:: --test-threads=5run 15 times in a loop while 40yes > /dev/nullprocesses loaded the host (11 runs finished at the last check)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.