fix: do not re-record requests served from the replay snapshot - #8
Merged
Conversation
In `Mode::Record`, the request lifecycle recorded every served exchange unconditionally — including responses served from the replay snapshot. Because the per-upstream storage backend is both the replay source and the recording sink, replaying an already-recorded request appended a duplicate entry on every hit, so re-runs multiplied records for requests already in the file. `SPECIFICATION.md` §8.3/§20.1 require the snapshot to act as a deduplicating cache: an already-seen request is replayed "rather than re-recording it". This threads the terminal's `ResponseSource` into the recording decision and skips recording when the response came from the replay snapshot (`ResponseSource::Snapshot`). Stub and forwarded responses are unaffected. Also reconciles the §6 wording that said "every served exchange is appended back", which contradicted the §8.3/§20.1 dedup rule, and updates the replay test that encoded the old behaviour. Regression test: record_mode_does_not_re_record_request_already_in_snapshot
JosiahBull
commented
Jun 21, 2026
JosiahBull
commented
Jun 21, 2026
JosiahBull
commented
Jun 21, 2026
JosiahBull
commented
Jun 21, 2026
JosiahBull
marked this pull request as ready for review
June 21, 2026 23:29
- Move ndjson_line_count / seed_snapshot / unreachable_addr into a shared tests/common/mod.rs so they can be reused across integration test files. - ndjson_line_count now streams line-by-line instead of reading the whole file, so it stays cheap on large snapshot files. - Reuse unreachable_addr in unreachable_upstream_records_error_outcome. - Drop the low-value banner comment and the doc-comment lead-in.
hardbyte
approved these changes
Jun 22, 2026
JosiahBull
marked this pull request as draft
June 22, 2026 00:04
spawn_echo, http_client and cfg(url) were copy-pasted across nearly every integration test file; unreachable_addr existed in two forms. Move the shared versions into tests/common/mod.rs and have each test binary import them, dropping the local copies (and now-unused imports). - spawn_echo / http_client: identical across assertions, control_plane_tcp, forward, middleware, record, replay, stubs. - cfg(url): identical across assertions, replay, stubs; middleware's byte-identical `upstream_cfg` folded into it; forward::spawn_proxy now uses it too. - unreachable_addr: unified on the sync form; forward's inline block and record's two call sites now use it. File-specific helpers (rt, make_recorded, in_memory_store, TrackingStorage, shutdown/tls fixtures) are left in place. No behaviour change.
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
2 participants
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.
Bug
In
Mode::Record, the request lifecycle recorded every served exchange unconditionally — including responses served from the replay snapshot. Since each upstream's storage backend is both the replay source and the recording sink, replaying an already-recorded request appended a duplicate on every hit, so re-runs (and any request already present in the snapshot) multiplied records for existing requests.This contradicts
SPECIFICATION.md§8.3/§20.1, which require the snapshot to act as a deduplicating cache: an already-seen request is replayed "rather than re-recording it".Fix
ResponseSourceinto the recording decision and skip recording when the response came from the replay snapshot (ResponseSource::Snapshot). Stub and forwarded responses are unaffected.Test
New regression test
record_mode_does_not_re_record_request_already_in_snapshot: a request pre-seeded into the snapshot is served from it (proved via an unreachable upstream — a forward would 502) and the file stays at one line instead of growing to two. Full lib suite + clippy + rustfmt clean.Note
This is part 1 of 2. The empty-file / within-run duplication (
jo/fix-empty-file-dedup) builds on this branch — fully resolving that symptom needs both the skip-on-hit logic here and index promotion there.