test(export): pin that a full export reads inside one snapshot

[mforce]

Why

WriteZipAsync opens BeginConsistentReadAsync once, then reads all 20 datasets through the same IInsightsModule. ExportQueries points every read at a separate, non-retrying context inside a RepeatableRead transaction (#269), so every CSV sees the same instant. Before this PR no test checked that. TransientDbResilienceTests.Export_ExplicitRepeatableReadTransaction_StillWorksUnderTheRetryStrategy checks only that the call does not throw.

Three tests now cover it. Each test proves one link, and the table states only what that test proves.

Test	Proves
ExportTests.ConsistentRead_HidesAWriteCommittedInsideTheSnapshot (Integration, real Postgres)	A row committed after the snapshot's first read stays hidden until the snapshot closes, then becomes visible. It covers one dataset, expense-categories, through IInsightsModule from real DI.
ExportTests.FullBackup_BuildsAndEnumeratesEveryDatasetInsideTheSnapshot (Integration, HTTP)	GET /api/v1/export/all opens exactly one snapshot. For every dataset, the GetDataset call (which binds the query to the active context) and the row enumeration (which runs the SQL) both happen after the snapshot opens and before it closes. A test-only IExportQueries recorder wraps the real ExportQueries behind the real facade, registered via ConfigureTestServices.
ExportSnapshotSourceTests.EveryDatasetArm_NamesTheSnapshotFieldDirectly (Application, Roslyn syntax)	Each GetDataset arm names the activeDb field directly, names no other AppDbContext field or primary-constructor parameter, and the set of arms equals DatasetNames in any order. A Theory applies four mutations to the real source, so the guard is shown to fail.

Limit of the source test. It is a syntax check of a naming convention and does not resolve identifiers. Indirection passes it: a local that shadows activeDb, a helper that ignores its argument, or a ternary. Code review and the two behavioural tests are what cover those cases. Renaming activeDb makes the test fail, which is intentional.

Test only, no call graph change.

Mutation evidence

Each mutation was applied locally, run against the named test, and reverted. The unmutated source is GREEN for every test.

Mutation	Test	Result
InsightsModule.BeginConsistentReadAsync returns a no-op disposable	ConsistentRead_…	RED: Assert.DoesNotContain() Failure: Item found in set
ExportQueries opens ReadCommitted instead of RepeatableRead	ConsistentRead_…	RED: same assertion
Only the customers arm reads requestDb.Customers	ExportSnapshotSourceTests	RED: "customers" reads through [requestDb], not only activeDb
customers arm moved to the end of the switch (round 2 P3: a harmless reorder)	ExportSnapshotSourceTests	GREEN, as intended
Delete the BeginConsistentReadAsync line from WriteZipAsync	FullBackup_Builds…	RED: first event is query:flocks, not begin
WriteZipAsync disposes the snapshot before the dataset loop	FullBackup_Builds…	RED: last event is rows-end:audit-events, not end
WriteZipAsync builds every GetDataset query into a dictionary before opening the snapshot, then enumerates them inside it (round 2 prefetch)	FullBackup_Builds…	RED: first event is query:flocks, not begin

In CI, the Theory in ExportSnapshotSourceTests also runs requestDb.Customers, db.Customers, this.requestDb.AuditEvents, and a renamed arm label.

Test counts (dotnet test --list-tests)

Project	base 6e3f5845	this branch
Application	620	625 (+5: 1 Fact, 1 Theory with 4 cases)
Integration	1873	1875 (+2)

origin/main has moved on since 6e3f5845. None of these files changed upstream: src/Cluckwork.Infrastructure/Insights, src/Cluckwork.Api/Endpoints/Export, src/Cluckwork.Application/Features/Export, ExportTests.cs.

Local runs (final head)

dotnet build Cluckwork.sln reports 0 warnings and 0 errors. Application passed 644/644; that total counts each Theory case, which --list-tests does not. Integration passed 1875/1875.

pstack:deslop found nothing to remove in any round.

[mforce]

Review of record: Codex gpt-6-astra, round 1, at head 5ab616c7. Posted by the coordinator. Log and probe paths named below are local to the review host.

Review of PR #1025

Reviewed head 5ab616c7729465664d30da616ae6b8de88a8dd7c in the assigned detached checkout. Read the PR, diff, export implementation, facade, DI registration, fixture helpers, #269 decision, and AGENTS.md guard rules. No delegation.

Finding

P2 | CONFIRMED | tests/Cluckwork.Api.IntegrationTests/ExportTests.cs:284

The test claims to protect every dataset but exercises only expense-categories, so another dataset can bypass the snapshot without failing it.

Concrete failure scenario: change only ExportQueries.GetDataset("customers") to query requestDb.Customers. A concurrent transaction changes a customer and an order after the first CSV establishes the snapshot. The backup can then contain the customer's new state and the order's old state, while this test passes.

Evidence: I changed only activeDb.Customers to requestDb.Customers at src/Cluckwork.Infrastructure/Insights/ExportQueries.cs:160. The new test passed, 1/1. See mutation log. The switch has 20 independently written query branches; the new test never invokes the other 19. This is an additional gap beyond the endpoint gap already disclosed in the PR.

Either extend coverage to reject individual dataset bypasses, or narrow the method name, comment and PR claim to the facade's snapshot lifecycle using expense categories, explicitly documenting the untested branches. Merely enumerating the other tables without asserting their snapshot behavior would not close this gap.

This is a test coverage/claim defect, not a product defect in the reviewed head. All current dataset branches use activeDb.

Mutation evidence

Each mutation ran separately against the new test with a real PostgreSQL database. Each was reverted afterward. All six red results below failed at line 308 with Assert.DoesNotContain() Failure: Item found in set, not at compilation or setup.

Mutation	Result	Evidence
Facade BeginConsistentReadAsync returns a no-op disposable	RED	log
ReadCommitted replaces RepeatableRead	RED	log
Every GetDataset branch reads requestDb	RED	log
Snapshot context opens a connection but never starts a transaction	RED	log
activeDb resets to requestDb immediately after snapshot activation	RED	log
Facade receives separate transient query instances for begin and dataset reads	RED	log
Only customers reads requestDb	GREEN	log
Delete the begin/disposal statement from WriteZipAsync	GREEN	log

The exact mutations and runner are retained in 1025-mutations.py.

Determinism and isolation

No timing defect found. The first await foreach exhausts a real SQL query before the separate scope saves the inserted category. That first query fixes PostgreSQL's repeatable-read snapshot. SaveChangesAsync completes before the second query starts, so no scheduling race, sleep, backpressure assumption, or row-order dependency determines visibility. Hash sets remove ordering concerns.

Each test creates a fresh account and unique row IDs. The reader explicitly resolves that account; each writer uses a fresh tenant-resolved scope. The integration collection serializes its tests, and the test modifies no shared factory configuration. The final read confirms both the mid-snapshot and post-disposal inserts become visible. It does not assert tenant isolation, but no account leakage or shared-state dependency was found in this change.

Endpoint gap and recommendation

The documented gap is real at ExportTests.cs:296: the test calls begin itself, bypassing ExportEndpoints.WriteZipAsync:100. This is a known limitation, not a second undisclosed finding.

Recommend a small test-only IExportQueries decorator registered through WithWebHostBuilder and ConfigureTestServices. Keep the real facade and queries. Record successful begin, the start and completion of each dataset's row enumeration, and disposal. Request /api/v1/export/all, consume the response, and assert exactly one begin, all dataset enumerations, then disposal. Record enumeration rather than only GetDataset, because the SQL is deferred. Empty datasets still produce enumeration events.

I built and ran this approach as a throwaway prototype:

• Unmodified endpoint: GREEN, 1/1.
• Deleted begin call: RED, event sequence differs.
• Snapshot disposed before the dataset loop: RED, disposal precedes enumeration.

This uses no production hook, source-text assertion, timing dependency, or concurrent insert. It complements the real database visibility test. It does not by itself prove each query branch uses the snapshot context, so the P2 finding still needs its stated resolution. The prototype was removed from the checkout and retained only beside this report.

Style and final validation

The added usings are outside the namespace and the file retains its file-scoped namespace, satisfying #985. The test follows the file's factory helpers and domain qualification conventions. No style issue found.

Baseline new test: 1/1 passed. After restoring every mutation and removing the probe, all 10 ExportTests passed with the normal build and warnings-as-errors enabled. See final run. I did not rerun the full integration suite or full solution build. git status --short and git diff --exit-code confirmed the detached checkout was clean. No commit, push, or GitHub comment was made.

Nits

None.

Verdict: REQUEST CHANGES to narrow or substantiate the “every dataset” claim; no product defect found.

[mforce]

Response to Astra round 1 (review of 5ab616c7), fixed in 92649e7157e7606b92700be66a7cb9187782f235. No product code changed.

1. P2: only one dataset was covered. Accepted.

• Added ExportSnapshotSourceTests (Application, Roslyn syntax). It fails when any GetDataset arm reads through a context other than activeDb, or when the arm set differs from DatasetNames.
• Your mutation, with only customers reading requestDb.Customers, is now RED: "customers" reads through [requestDb], not only activeDb. Reverted after the run.
• A Theory runs that mutation and three more (db.Customers, this.requestDb.AuditEvents, a renamed arm label) against the real source in CI.
• Renamed the Postgres test to ConsistentRead_HidesAWriteCommittedInsideTheSnapshot. Its comment now says it covers one dataset and names the tests that cover the rest.

2. Endpoint gap. I adopted your recorder approach as ExportTests.FullBackup_EnumeratesEveryDatasetBetweenSnapshotOpenAndClose. It uses the real facade and the real ExportQueries behind a test-only IExportQueries decorator registered through WithWebHostBuilder + ConfigureTestServices. It asserts begin, then each dataset's rows:/rows-end:, then end.

• Deleting the begin line in WriteZipAsync is RED: the sequence has no begin.
• Disposing the snapshot before the loop is RED: the sequence starts begin, end, rows:flocks, ….
• I reverted both mutations.

Verification:

• dotnet build Cluckwork.sln: 0 warnings.
• Application: 644/644 passed.
• Integration: 1873/1875 passed. The two failures were MultiInstanceRateLimitTests and OtlpSubprocessExporterTests under host load. Both passed on a re-run, alone (12/12) and together with ExportTests (23/23).
• The mutation table and test counts are in the PR body.

[mforce]

Review of record: Codex gpt-6-astra, round 2, at head 92649e71. Posted by the coordinator. Log paths named below are local to the review host.

PR #1025, round 2

Reviewed fix-only changes at 92649e7157e7606b92700be66a7cb9187782f235 and the worker's latest PR comment. All work used the assigned detached checkout. No delegation.

Both round-one mutations now fail for the intended reason. Two further coverage defects remain. Neither finding is a product defect in this head. They are false negatives in the new guards; the product defects described below exist only under the reverted mutations.

Findings

P2 | CONFIRMED | tests/Cluckwork.Application.Tests/Architecture/ExportSnapshotSourceTests.cs:69

The scanner treats the identifier activeDb as proof of query ownership, so local shadowing and helpers can route a dataset to the request context while every new guard passes.

Concrete failure scenario: a refactor changes GetDataset to a block body with this local before the otherwise unchanged switch:

var activeDb = dataset == "customers" ? requestDb : this.activeDb;
return dataset switch { /* unchanged arms */ };

Customers now reads outside the snapshot. A customer change committed after the first CSV can appear alongside older sales-order data. The scanner still sees the text activeDb in each arm and never examines the local initializer or resolves the identifier to its declaration.

Evidence: this mutation compiled and passed all 5 source tests and 11 export tests. A second mutation replaced activeDb.Customers with CustomerQuery(activeDb.Customers), where the helper ignored its argument and returned requestDb.Customers; the same 16 tests passed. A third used a ternary containing activeDb.Customers and a new context field declared with the fully qualified AppDbContext type. That field escaped the scanner's exact type-text comparison at line 43, and the same 16 tests passed again.

Logs: local source, local export, helper source, helper export, qualified-field source, qualified-field export.

Do not extend the identifier blacklist. Either recognize the supported direct-query shape and reject unrecognized method bodies/helpers/context expressions, or narrow the stated guarantee to a direct-reference convention. The current comment that this proves every arm uses the snapshot context remains too strong.

P2 | CONFIRMED | tests/Cluckwork.Api.IntegrationTests/ExportTests.cs:348

The recorder does not check when GetDataset executes, allowing queries bound to the request context before snapshot creation to pass the endpoint guard.

Concrete failure scenario: WriteZipAsync first builds a dictionary of exports.GetDataset(name) results, then opens the snapshot and enumerates those saved datasets. GetDataset has already constructed each IQueryable against the old request context. Opening the snapshot afterward cannot change that captured context, although the recorder emits exactly the expected begin/rows/end sequence.

Evidence: this production mutation passed all 11 export tests and all 5 source tests. I then added a temporary behavioral probe that commits an expense category after the first dataset finishes and checks the downloaded category CSV. The unchanged endpoint excluded the row and passed; the prebuilt-query mutation included it and failed Assert.DoesNotContain. The interleaving is awaited, with no sleeps or timing dependency.

Logs: submitted export tests stay green, source tests stay green, behavioral baseline, behavioral failure.

Record GetDataset calls as well as enumeration and require both to occur after begin and before disposal. Keep the existing enumeration checks: construction alone does not execute the query. This closes the demonstrated gap without a production hook.

Mutation results

Source-only rows below ran the real-tree Fact against the mutated source. Surviving source mutations additionally compiled and ran all export tests and all five source tests, as reported above.

Mutation	Result and reason
Customers uses requestDb.Customers	RED: "customers" reads through [requestDb], not only activeDb
Customers uses this.db.Customers, with an explicit context field	RED: identifies db
Ternary chooses between activeDb and requestDb	RED: identifies both contexts
Request field renamed to fallbackDb, then used by customers	RED: identifies the renamed context
Local shadow routes only customers to request context	GREEN, finding 1
Helper ignores the supplied active-context query	GREEN, finding 1
Ternary uses a fully qualified context field	GREEN, finding 1
Delete endpoint begin call	RED: expected begin event is absent
Begin twice	RED: extra begin/end events
Dispose while the final dataset is enumerating	RED: download fails because the underlying reader is closed
Construct dataset queries before begin	GREEN, finding 2
Replace await using with disposal only on successful completion	GREEN on submitted tests; exception limitation below

The last-dataset mutation disposes the snapshot after the audit-event enumerator yields a row and before it finishes, rather than merely closing it before the loop. Its failure is a real reader-lifetime failure, not compilation or setup failure.

Exact mutations and logs are retained beside this report: source runner, endpoint runner, behavioral probes.

Exception path and determinism

The submitted endpoint test exercises successful completion only. Moving disposal onto the success path leaves all 16 relevant tests green. A temporary probe injected an exception during audit-event enumeration and checked that disposal still occurred. The actual endpoint passed; success-only disposal failed, with the last event rows:audit-events instead of end. This is a measured coverage limit, not an additional blocking finding or an existing cleanup defect. An exception-path test would be a small follow-up if cleanup is part of the promised guarantee.

No flakiness found in the submitted tests. Each endpoint test uses a fresh account, host, and event list. The producer records events sequentially, and the endpoint awaits the producer before completing the response callback. The client fully consumes the response before reading the list. No global recorder, timer, row-order assumption, or cross-test account dependency was introduced. Empty datasets still record enumeration start and completion. The wrapper forwards cancellation and awaits the real begin/disposal operations.

The decorator explicitly constructs ExportQueries, so it does not verify the production IExportQueries registration. The separate facade test still resolves production DI. The confirmed ordering gap is the unrecorded query-construction step in finding 2.

Nits and false positives

P3 | CONFIRMED | tests/Cluckwork.Application.Tests/Architecture/ExportSnapshotSourceTests.cs:64

SequenceEqual rejects a harmless switch-arm reorder even though archive ordering comes from DatasetNames.

Concrete scenario: move the unchanged customers arm immediately before the fallback arm. Every mapping and query stays the same, but the source Fact fails because its arm sequence differs from the registry. Mutation evidence. Compare membership and uniqueness if the intended invariant is complete dataset coverage, rather than requiring incidental source order. This is not a product defect.

A consistent rename of the private activeDb field also fails the explicit name assertion. That is visible name coupling, not a runtime regression; document it if intentionally retained.

Validation and checkout state

The unchanged baseline and final restored-source runs both passed all 5 source tests and 11 export tests. See final source results and final export results. The changed files retain outside-namespace usings and file-scoped namespaces; builds run with the repository's warnings-as-errors and #985 style gate. No style defect found. Full solution and full integration-suite runs were not repeated.

Every mutation and temporary probe was reverted; git status --short and git diff --exit-code confirmed a clean checkout at the reviewed head. No commit, push, GitHub comment, or change to another checkout was made.

Verdict: REQUEST CHANGES for the two confirmed guard bypasses; no product defect found in the reviewed head.

[mforce]

Response to Astra round 2 (review of 92649e71). Fixed in fae40e76372fe6b5e2cbe808d870bef0471a9e49. No product code changed.

1. P2: the source guard treated the identifier activeDb as proof. Accepted and narrowed, not extended, on the owner's decision.

• Renamed the test to EveryDatasetArm_NamesTheSnapshotFieldDirectly.
• The test's comment and the PR body now state only what it checks: each arm names the activeDb field directly, names no other AppDbContext field or parameter, and the arms cover DatasetNames.
• Both also state that indirection passes this check: a shadowing local, a helper that ignores its argument, a ternary. Code review and the behavioural tests in ExportTests cover those cases.
• P3 fixed. Arms are now compared to DatasetNames without regard to order. Moving the customers arm to the end now passes. A renamed or missing arm still fails, through the Theory case.
• The activeDb name coupling stays on purpose, and the comment says so.

2. P2: the recorder did not check when GetDataset runs. Fixed.

• The recorder now logs each GetDataset call (query:<dataset>) as well as each enumeration.
• The renamed test, FullBackup_BuildsAndEnumeratesEveryDatasetInsideTheSnapshot, requires begin first and end last. Every query:, rows: and rows-end: event must fall between them, in that order for each dataset.
• Your prefetch mutation (build all queries into a dictionary before begin, then enumerate them inside the snapshot) is RED: the first event is query:flocks, not begin. I reverted it.
• The round-1 mutations still fail on the new assertions: deleted begin gives query:flocks first, and early disposal gives rows-end:audit-events last.

Verification on the final head:

• dotnet build Cluckwork.sln: 0 warnings.
• Application: 644/644 passed.
• Integration: 1875/1875 passed.
• Updated mutation table and counts are in the PR body.

The review loop was stopped deliberately by the owner after two consecutive rounds that found no product defect (rounds 1 and 2 found only test gaps). No round 3 will be triggered.