test(sessions): gate snapshot reads instead of wall-clock sleeps - #557
Conversation
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c730e4ab1d
ℹ️ 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".
| while state.entered == 0 { | ||
| state = self.changed.wait(state).unwrap(); |
There was a problem hiding this comment.
Bound the wait for a gated reader
If a spawned recovery or publication returns or panics before reaching verified_snapshot, no thread increments entered or notifies this condition variable. Each converted test waits here before joining its worker, so the test suite hangs indefinitely instead of reporting the underlying failure; use a timed wait or coordination that also observes worker completion.
AGENTS.md reference: AGENTS.md:L108-L110
Useful? React with 👍 / 👎.
| store.graph.await_gated_snapshot_reader(); | ||
| store.graph.release_gated_snapshot_reads(); |
There was a problem hiding this comment.
Prove both publishers contend before releasing the gate
The gate is released after only one publisher enters, while the barrier does not prove that the other publisher has attempted to acquire the publication lock. If that second worker is delayed and the lock is removed, the first can publish before the second reaches the permanently open gate; the second then reads the committed snapshot and all merge assertions still pass, so this test does not deterministically catch the lock-removal regression it targets.
AGENTS.md reference: AGENTS.md:L108-L110
Useful? React with 👍 / 👎.
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
c730e4a to
086f8e7
Compare
a46d1ad
into
cursor/agent-hosts-retain-graph-port-cb93
Summary
thread::sleep(snapshot_delay_millis)inMemoryEvidenceGraphRuntime::verified_snapshotwith a condvar-backedSnapshotReadGate(enable / entered-read await / release) in the sessions crate test support.sleep(10ms)timing with a deterministic ordering plus an explicit entered-before-cancel assertion.Motivation
Coordinator / build-perf named these wall-clock waits: 50 ms fake snapshot IO plus 10 ms "hope the reader started" sleeps made the cancellation tests slow and timing-dependent. Gates make the ordering explicit and the assertions stronger (the reader is provably inside the read before cancellation).
Changes
crates/tracedecay-sessions/src/runtime/git_correlation/test_support.rs—set_snapshot_read_delayremoved;SnapshotReadGateadded withgate_snapshot_reads/await_gated_snapshot_reader/release_gated_snapshot_reads/gated_snapshot_readers_entered. Release is the only wake-up, so tests store the cancellation signal before releasing and the reader's post-gate check observes it deterministically.crates/tracedecay-sessions/src/runtime/git_correlation/tests.rs—recovery_observes_request_cancellation_while_reading_a_snapshotnow awaits the gated reader, asserts it entered, cancels, releases.crates/tracedecay-sessions/src/runtime/ingest/tests.rs—admitted_bounded_publication_observes_live_operation_cancellationconverted the same way;concurrent_same_session_observations_merge_under_the_publication_lockuses the gate to hold the first recovery read open so lock-removal regressions still lose an observation and fail the merge asserts.Production
tokio::time::sleep(CONTROL_POLL_INTERVAL)polling inbackfill/bounded/blocking.rsis untouched (production budget, out of scope). No asserts weakened; no tests removed.Test plan
cargo test -p tracedecay-sessions— full crate suite green (git_correlation targets: 57 passed; ingest targets: 63 passed; 0 failed anywhere)cargo fmt -p tracedecay-sessions --checkcleanChecklist
CHANGELOG.mdupdated (test-support only; no user-facing behavior change).envfiles included