The one-scan config test tolerates a snapshot TTL that elapsed during its own loop - #570
The one-scan config test tolerates a snapshot TTL that elapsed during its own loop#570emooreatx wants to merge 3 commits into
Conversation
…n loop main 55b8c7a failed on macOS and Windows: "150 keyed reads + one list must cost ONE scan; got 2". On a cold runner the 150 awaits outlived the 2 s snapshot TTL and the cache expired mid-loop — the cache working, not a scan per read. The budget is now 1 + elapsed / TTL, derived from the measured loop time; ubuntu still sees exactly 1. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de93f1777
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let budget = 1 + (elapsed.as_millis() / graph_config::CONFIG_SNAPSHOT_TTL.as_millis()) as u64; | ||
| let scans = after - before; | ||
| assert!( | ||
| (1..=budget).contains(&scans), |
There was a problem hiding this comment.
This elapsed-time budget still leaves the test flaky under the normal parallel test harness: SCANS is process-global, and the graph-config cache is a single process-wide slot, while the other #[tokio::test] cases in this same integration binary create different engines and call config reads/writes. If one of those tests scans or replaces/invalidates the cache between before and after, this test can record multiple scans even when elapsed < CONFIG_SNAPSHOT_TTL, so budget == 1 and the assertion fails for reasons unrelated to TTL expiry. Serialize this test or use per-engine/snapshot-local accounting rather than bounding the shared counter by this test's elapsed time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in a5782ad, at the root rather than by serialising the test: the cache is now one slot per engine (a bounded map keyed by engine identity, CACHE_SLOTS = 8) and the writers invalidate per engine (invalidate_engine), so a sibling test's engine can no longer evict this test's snapshot or count against it. The test compares its OWN engine's snapshot ordinals before and after the reads (tolerating one rescan per TTL elapsed) and no longer touches the process-wide SCANS. graph_config 3/3, conformance_gate (the keyed-cache gate) green.
…ine, and the one-scan test reads its own engine's ordinal (Codex on #570) The TTL budget alone left the test flaky under the parallel harness: the cache was ONE process-wide slot and SCANS a process-wide counter, so a sibling test's engine reading or writing its own config plane evicted this test's snapshot and counted a scan against it with no TTL involved. - `graph_config`: a small map of (engine identity → snapshot), CACHE_SLOTS = 8, oldest evicted; `invalidate_engine(&Engine)` drops one engine's slot and is what `set_config` and `attest::put` call — a write to one store says nothing about another's; `invalidate()` still clears all (compose's re-serve). `engine_identity` takes `&Engine` (the value's address, which is `Arc::as_ptr` for an Arc) so `attest::put`'s `&Engine` names the same slot. - The test compares ITS engine's snapshot ordinals before and after the 150 reads, tolerating one rescan per TTL elapsed; the process counter is no longer part of the assertion. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
graph_config.rs, attest.rs and node_key.rs were not in the ladder's path filter, so #570 (the per-engine snapshot cache) would have merged with the config plane proven only by unit tests. Every node writes config at boot through graph_config and peers through attest::put; the ladder is what proved 0.5.201's per-key leaf on real nodes. Now it runs for these files. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
915af08 adds |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 915af08e2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let after = graph_config::snapshot(&engine) | ||
| .await | ||
| .expect("snapshot") | ||
| .scan; |
There was a problem hiding this comment.
When a sibling test scans its engine after this ordinal is captured, the unchanged assertion at lines 434–438 compares the process-global SCANS counter with this engine-local after value and fails even though this engine reused its cache. Fresh evidence beyond the earlier review is that the new ordinal is still fed into that global equality check; compare the returned snapshot's scan instead so concurrent engines cannot affect the result.
Useful? React with 👍 / 👎.
| /// How many engines keep a snapshot at once. A node has one; the bound exists | ||
| /// so a process that churns engines (a test binary, the embedded fold | ||
| /// re-serving) cannot grow the map without limit. | ||
| const CACHE_SLOTS: usize = 8; |
There was a problem hiding this comment.
Under the normal parallel harness, this integration binary has ten Tokio tests that each create an engine, but the cache retains only eight engine slots and cache hits do not refresh FIFO order. If enough sibling tests populate slots while fifty_reads_are_one_scan_and_a_write_invalidates is running, slots.remove(0) can evict its still-active snapshot; its next keyed read then rescans before a TTL has elapsed, making rescans == 1 with budget == 0. Fresh evidence beyond the earlier review is the mismatch between this eight-slot bound and the ten engine-owning tests, so the claimed per-engine isolation remains conditional on scheduling.
Useful? React with 👍 / 👎.
| fn engine_identity(engine: &Engine) -> usize { | ||
| // The address of the `Engine` value itself — for an `Arc<Engine>` this is | ||
| // exactly `Arc::as_ptr`, and it lets a door that holds only `&Engine` | ||
| // (`attest::put`) name the same slot. | ||
| std::ptr::from_ref(engine) as usize |
There was a problem hiding this comment.
When engines are created and dropped in one process, an Engine allocation address can be reused while this new multi-slot cache still retains the prior owner's fresh snapshot. For example, after engine A is cached, engine B is cached, A is dropped, and engine C is allocated at A's address, C's first read can match A's slot and return A's configuration without scanning C's store; the former single-slot cache would contain only B in this sequence. Key the entry with an identity whose lifetime is tied to the engine (such as a Weak<Engine> checked by pointer equality) rather than a bare recyclable integer address.
Useful? React with 👍 / 👎.
| let elapsed = t0.elapsed(); | ||
| let budget = (elapsed.as_millis() / graph_config::CONFIG_SNAPSHOT_TTL.as_millis()) as u64; | ||
| let rescans = after - before; |
There was a problem hiding this comment.
If this test task is delayed after first is returned but before t0 is recorded, the baseline snapshot can already be near or beyond its TTL while the measured elapsed remains below one TTL. The first keyed read then legitimately rescans, producing rescans == 1 and budget == 0; even without a long pause there is always a boundary window equal to the unmeasured age. Start the timer before loading first, or otherwise include the baseline snapshot's age when calculating the allowance.
Useful? React with 👍 / 👎.
1 participant
main 55b8c7a failed on macOS and Windows: "150 keyed reads + one list must
cost ONE scan; got 2". On a cold runner the 150 awaits outlived the 2 s
snapshot TTL and the cache expired mid-loop — the cache working, not a scan
per read. The budget is now 1 + elapsed / TTL, derived from the measured
loop time; ubuntu still sees exactly 1.
Main 55b8c7a:
clippy + test (macos-latest)and(windows-latest)both red on this test; ubuntu green. Test-only change.🤖 Generated with Claude Code