Batch buffered account-export artefact blob loads (#1355)

[Chris0Jeky]

Closes #1355

What & why

The buffered /api/account/export path (DataExportService.ExportUserDataAsync) loaded artefact blobs one round-trip per artefact via ISourceArtefactRepository.GetContentForUserAsync — up to ~10,000 SELECTs for a heavy export (LOW-2 from the #1341 GEN-01 security review). This batches the blob loads while preserving the export contract byte-for-byte.

Design

• New repository method GetContentsForUserAsync(IReadOnlyCollection<Guid> ids, Guid userId, ct) → IReadOnlyDictionary<Guid, byte[]>: a single keyed IN-query using the same user-scoped artefact→blob join as GetContentForUserAsync (so a foreign artefact id can never surface content). Empty id set short-circuits with no query. Mirrors the existing ChatMessageRepository.CountBySessionIdsAsync batching precedent.
• DataExportService now pages the artefact metadata (already Id-ordered) in bounded chunks of StreamPageSize (500), batch-loading each chunk's blobs and mapping in metadata order.

Chunk size: 500. With the 500-id IN (...) plus the userId parameter that is 501 bound parameters, comfortably under SQLITE_MAX_VARIABLE_NUMBER = 999. Matches the StreamPageSize constant already used by the streaming export's chat-session batching.

Memory posture: deliberately not one mega-query. Each chunk's raw blob dictionary is processed then released before the next chunk loads, so peak raw-bytes held is one chunk's worth. Total buffered content remains bounded by the pre-existing MaxBufferedArtefactBytes (10 MB) and 10,000-row guards, which run before any blob load. Round-trips drop from N to ceil(N/500).

Contract preserved: artefacts are emitted in the exact former (Id) order; a missing blob still throws InvalidOperationException (→ UnexpectedError); user-scoping and the pre-load size guards are unchanged. GetContentForUserAsync is retained on the interface (symmetry with CopyContentForUserAsync; regression tests assert the buffered path no longer calls it).

Tests

Service-level (DataExportServiceTests, Moq):

• 0 artefacts → no blob query at all (batch and per-item both Times.Never).
• 1 chunk (3 artefacts) → exactly one batch call, per-item never called, order + decoded content match.
• 501 artefacts → exactly two batch calls (ids.Count == 500 once, ids.Count == 1 once — the chunk-size and chunk-size+1 boundary), order preserved across chunks, all content correct, per-item never called.
• user-scoping → batch always invoked with the requesting user id, never another.
• missing-blob → export fails (UnexpectedError), contract preserved.

Repository integration (SourceArtefactRepositoryIntegrationTests, real SQLite + DbCommandInterceptor):

• empty id set → empty result, zero ArtefactBlobs SELECTs (empty-set validity).
• 25 artefacts → all resolved in exactly one SELECT (round-trip reduction proven at the SQL level).
• another user's artefact id requested while scoped to the owner → excluded.
• unknown id → omitted without error.

Verification

• dotnet restore + dotnet build backend/Taskdeck.sln -c Release -m:1 → 0 errors (pre-existing warnings only).
• DataExportService* + GdprDataExportRoundTrip* (Application.Tests) → 37 passed.
• SourceArtefactRepositoryIntegrationTests (Api.Tests) → 4 passed.
• DataPortabilityApiTests (end-to-end export through the real repository) → 12 passed.
• AccountDeletionServiceTests + ArtefactServiceTests + ArtefactExtractionServiceTests → 43 passed.
• dotnet ef migrations has-pending-model-changes → "No changes" (no schema impact).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[gemini-code-assist]

Code Review

This pull request introduces batch-loading of blob content for requested artefacts in SourceArtefactRepository and integrates it into DataExportService to avoid database round-trips, supported by comprehensive integration and unit tests. Feedback suggests addressing an N+1 query issue with GetAllExtractionHistoryAsync inside the loop, simplifying the chunking logic using LINQ's .Chunk() method, and adding a guard clause in the repository to prevent SQLite parameter limit violations if called with more than 999 IDs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Chris0Jeky]

Adversarial Code Review

Reviewed the full diff (production + tests) against origin/main, with focus on ordering stability across chunks, SQLite parameter-limit edges, empty-set validity, user-scoping, byte-for-byte export parity, and memory posture.

CRITICAL

• None.

HIGH

• None. User-scoping is preserved (where artefact.UserId == userId identical to the per-item GetContentForUserAsync); the missing-blob InvalidOperationException contract is preserved; the pre-load 10 MB / 10,000-row guards are untouched and still run before any blob load; ordering is driven by the Id-ordered metadata iteration, not the (unordered) dictionary.

MEDIUM

• M1 (simplification / minor inefficiency) — DataExportService: the chunk loop materializes an intermediate List<SourceArtefact> chunk and then chunk.Select(a => a.Id).ToList(), i.e. two allocations per chunk where one id-list plus index iteration over artefactMetadata suffices. Simplify to index-based slicing.
• M2 (test gap) — no explicit assertion that an artefact count of exactly StreamPageSize (500) issues exactly ONE batch call. The loop is correct (no spurious trailing empty query), but the "exactly chunk-size" boundary — a classic off-by-one — should be locked down by a test alongside the existing 501 (chunk-size+1) case.

LOW

• L1 (style) — SourceArtefactRepository: the EmptyContentMap static field is declared between two methods. Relocate to the top of the class for conventional member ordering.

Bot Comments Addressed

• None yet (PR just opened; Gitleaks passed, CodeQL/CI pending). Will re-check after fixes push.

Summary

0 CRITICAL, 0 HIGH, 2 MEDIUM, 1 LOW. Not merge-blocking on correctness; fixing all three (M1, M2, L1) now per zero-skip policy. Fix evidence to follow.

[Chris0Jeky]

Adversarial Review — Fixes Applied

All self-review and bot findings addressed. Verified via targeted suites (38 export-service tests in Taskdeck.Application.Tests; 17 tests across SourceArtefactRepositoryIntegrationTests + DataPortabilityApiTests in Taskdeck.Api.Tests); dotnet build -c Release -m:1 = 0 errors.

Self-review findings

Finding	Severity	Fix Commit	Verified
M1 buffered loop had redundant chunk/select allocations	MEDIUM	86ef2330 (superseded by e8b887dc)	build + export tests pass
M2 no explicit exactly-chunk-size single-batch test	MEDIUM	f8c68459	..._WithExactlyChunkSizeArtefacts_IssuesSingleBatch passes
L1 EmptyContentMap field declared mid-class	LOW	fbc4796c	build passes

Bot findings (gemini-code-assist)

Finding	Severity	Resolution	Verified
N+1 on GetAllExtractionHistoryAsync per artefact	HIGH	Out of scope for #1355 (blob-only) and pre-existing (lived in the former per-item loop). Tracked as #1387 per the review policy — never silently dropped.	n/a
Prefer idiomatic .Chunk() over manual offset/count	MEDIUM	e8b887dc — adopted artefactMetadata.Chunk(StreamPageSize) (supersedes M1's index slicing; readability wins, allocation delta negligible)	export tests pass
Add repository-side parameter-limit guard	MEDIUM	eaf14ca8 (+ test abe58fce) — added MaxBatchIdCount = 900 guard throwing ArgumentException. Note: EF Core 8 parameterises Contains via json_each (a single SQLite parameter), so the raw "too many SQL variables" crash does not actually occur on this stack — the guard is defense-in-depth against a future translation/provider change, keeping the worst case well under 999.	..._WithTooManyIds_ThrowsBeforeTouchingDatabase passes

Notes

• ISourceArtefactRepository.GetContentForUserAsync (per-item) is retained deliberately (symmetry with CopyContentForUserAsync; regression tests assert the buffered path no longer calls it).
• No schema changes: dotnet ef migrations has-pending-model-changes reports "No changes".

CI status: pending at time of writing; it will be re-verified green before any merge. Merge is the maintainer's call — this PR never self-merges.

[Chris0Jeky]

Consolidated Adversarial Review — Two Independent Lenses

Coordinator adjudication of two independent reviews of this PR (query/GDPR-scoping lens; test-discrimination lens).

Lens 1 — Query & GDPR scoping: FULL REFUTATION (no findings)

Every attack angle was probed and refuted:

• Scoping equivalence — GetContentsForUserAsync uses the identical user-scoped artefact→blob join as the per-item GetContentForUserAsync; a foreign artefact id cannot surface content. REFUTED as an attack.
• Dictionary semantics — duplicate ids are Distinct()-ed before the query; last-write-wins on the map is unreachable (Id is the PK); lookups are driven by the Id-ordered metadata, not dict iteration order. REFUTED.
• Ordering — export order is the metadata (Id) order in both the old and new paths; the dictionary never dictates order. REFUTED.
• Parameter limits — EF Core 8 SQLite translates the parameterised Contains via json_each (single parameter); the 900-id guard is defense-in-depth headroom under SQLITE_MAX_VARIABLE_NUMBER=999. REFUTED.
• Transactionality — the buffered export was never transactional per-artefact before (N separate SELECTs); one SELECT per 500-chunk is strictly no worse for consistency and better for latency. REFUTED.
• Guard reachability — the export path chunks at 500, so the 900 guard is unreachable from production callers; it exists for future callers. REFUTED.

Lens 2 — Test discrimination: all claims confirmed; 4 LOW findings

• LOW-1 — the 900-id guard has only the throw-side test (901 ids). Missing the pass-side boundary: exactly 900 ids must NOT throw. Pins > vs >=.
• LOW-2 — GdprDataExportRoundTripTests stubs zero artefacts, so no serialize→parse→deserialize round-trip ever carries batched blob content through the buffered path. Add a round-trip case with ≥2 distinguishable artefacts through the batched path asserting ordered base64 content.
• LOW-3 — the missing-blob test pins only the public UnexpectedError; it should also pin the internal contract: InvalidOperationException with the same "...is missing its blob." message shape as the per-item path (observable via the logged exception at the service seam).
• LOW-4 (comment accuracy) — the DataExportService chunk-loop comment says "peak memory holds at most one chunk of raw blob bytes at a time" — true only for the raw dictionary; the mapped DTOs' base64 strings accumulate across chunks (bounded by MaxBufferedArtefactBytes). Reword to state both halves.
• Note (kept, annotated) — ExportUserDataAsync_BatchBlobLoad_ScopedToRequestingUserOnly is near-tautological (the service forwards its own parameter). Retained as a wiring check, with a comment pointing at the repository integration test as the real scoping enforcement proof.

Summary

0 CRITICAL / 0 HIGH / 0 MEDIUM / 4 LOW + 1 annotation. Not merge-blocking; all four LOWs and the annotation are being fixed now per zero-skip policy. Fix evidence to follow.

[Chris0Jeky]

Consolidated Review — Fixes Applied

All four LOW findings and the annotation from the two-lens consolidated review are fixed, pushed as 9e3aab98 (single push, four per-file commits).

Finding	Fix Commit	Verified
LOW-1 pass-side guard boundary: exactly 900 ids must NOT throw (pins > vs >=)	6202a727	GetContentsForUserAsync_WithExactlyMaxBatchIds_DoesNotThrow — empty map, no throw, against real migrated SQLite
LOW-2 GDPR round-trip suite never carried batched blob content	2c62a55e	ExportUserData_WithBatchedArtefacts_RoundTripsOrderedBase64Content — 2 distinguishable artefacts through the batched path; serialize→parse asserts ordered base64 content; deserialize asserts the typed DTO round-trip
LOW-3 missing-blob test pinned only UnexpectedError	9e3aab98	test now also verifies the logged exception is InvalidOperationException with message exactly "Artefact {id} is missing its blob." (same shape as the per-item path), via the logger seam
LOW-4 memory comment overstated ("at most one chunk")	5b6a783b	comment now states both halves: raw byte[] dict is per-chunk; DTO base64 strings accumulate across chunks; both bounded by the pre-load MaxBufferedArtefactBytes guard
Annotation: near-tautological scoping test	9e3aab98	BatchBlobLoad_ScopedToRequestingUserOnly now carries a NOTE that it is a wiring check only and the real enforcement proof is SourceArtefactRepositoryIntegrationTests.GetContentsForUserAsync_NeverReturnsAnotherUsersBlob

Verification (exact counts)

• dotnet build backend/Taskdeck.sln -c Release -m:1 → 0 errors
• Taskdeck.Application.Tests filter DataExportService|GdprDataExportRoundTrip → 39 passed, 0 failed (was 38; +1 round-trip test, missing-blob test strengthened in place)
• Taskdeck.Api.Tests filter SourceArtefactRepositoryIntegrationTests → 6 passed, 0 failed (was 5; +1 pass-side boundary test)

Lens 1 (query/GDPR scoping)

No findings — full refutation across scoping equivalence, dictionary semantics, ordering, parameter limits, transactionality, and guard reachability (see the consolidated review comment above for the list). Nothing to fix on that lens.

Inline bot threads (gemini-code-assist) are being replied to and resolved with commit references next.