records(export): stream installation exports without buffering them - #69
TusanHomichi wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c1eae2fe25
ℹ️ 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".
| // starts delivering. The read transaction stays open around it, so | ||
| // the manifest, the payloads, and the audited instant still come | ||
| // from one snapshot. | ||
| audit_export(pool, actor_user_id, &audited).await?; |
There was a problem hiding this comment.
With five concurrent valid exports, each task can hold its read transaction connection here and then wait for a sixth connection to write the audit event; the production pool is capped at five connections in storage.rs:75-77. All five exports then time out or fail together while unrelated database operations are also starved, and an authorized user can repeat this with parallel requests. Restructure the preflight so it cannot acquire another pooled connection while every export already owns one.
Useful? React with 👍 / 👎.
| if send_before(sender, StreamItem::Failed(detail), deadline).is_err() { | ||
| tracing::error!("the export's failure could not reach the response body"); |
There was a problem hiding this comment.
If a client stops reading for longer than both 10-second send windows and then resumes, the failed data send is followed by a failure send into the same still-full queue; this branch discards that second failure after only logging it. Once the queued chunks drain, all senders have been dropped, so ExportBody observes None and reports a clean end, allowing HTTP/1 to emit its terminal chunk and the browser's response.blob() to resolve with a truncated ZIP. Keep failure/completion state outside the bounded data queue, or otherwise guarantee that channel closure after an unsuccessful producer yields a body error.
AGENTS.md reference: AGENTS.md:L38-L40
Useful? React with 👍 / 👎.
TusanHomichi
left a comment
TusanHomichi
left a comment
There was a problem hiding this comment.
Merge review at c1eae2f: hold this PR. I inspected the export transaction/audit lifecycle, EntryBuffer, response producer/body, and failure tests, and independently agree with both existing unresolved Codex findings. pr-gate run 34661990490 is green; these are uncovered boundary cases, not a claim that CI failed.
-
Pool ownership:
export_to()holds a pooled read transaction whileaudit_export(pool, ...)waits for another pooled connection. The production pool has five connections. Coordinated exports can occupy all five and then each wait for the audit connection, starving exports and unrelated requests until acquisition timeouts. Repair the reservation/admission/connection lifecycle rather than merely increasing the pool or moving an audit write into a transaction held for the full download. Add a deterministic concurrency regression that holds all competing requests at the relevant boundary; prove unrelated ordinary work still progresses, and exercise a one-connection pool or explicitly prove the supported admission contract. Preserve one snapshot for manifest and payload, the intended authorization boundary, and honest audit semantics. -
Terminal outcome: failure is sent through the same bounded queue that just stalled. If both send windows expire,
signal_failure()only logs, the producer disarms its guard and drops all senders, andExportBodytreats channel closure as successful EOF. A late reader can therefore receive a truncated prefix with a clean HTTP completion. Give success/failure an explicit reliable terminal state independent of queue capacity; only an explicitly successful, finalized producer may permit clean EOF. A channel closing without that success must fail closed. Cover ordinary producer error, panic, full queue, disconnect, and finalization failure.
The decisive new stall regression should leave the data queue full beyond BOTH send deadlines, then resume reading and assert a body/transport error rather than clean EOF. Existing tests that inject a failure while there is queue space, or only test one timed-out send, cannot establish that guarantee. Keep short injectable test deadlines and add a real transport-level check as appropriate; do not weaken timeouts or retry until green.
Keep the existing byte-identity/CRC/header, verifier, snapshot, cancellation, and writer-progress proof. State memory cost precisely: streaming removes whole-corpus payload buffering, but unit metadata, the serialized manifest, and ZIP central-directory state still scale with unit count; browser Blob buffering also remains a documented separate limitation. Do not claim constant memory or end-to-end browser streaming.
Revalidate against current main (now containing #68) and rerun the full gates. No merge or deployment was performed for this PR. This review is source inspection plus GitHub CI/review verification, not an independently executed Rust or browser reproduction.
Export read every finalized version with `fetch_all` and assembled the whole ZIP in memory before the handler answered, so an installation export cost memory in proportion to its history (#47). `record_export::export_to` now writes the container into any seekable sink as it produces it: one read transaction, a metadata-only pass for the manifest (which must be the container's first entry), then a pass that streams each unit's stored bytes and unit manifest. `export_stream::EntryBuffer` holds one entry and applies the `zip` writer's patches to it, so the destination sees an append-only stream and the bytes stay identical to the buffered archive: no data descriptors, real CRC-32 and sizes in the local header, the same central directory. A patch is classified by position — inside the held entry — so the CRC and size patch that seeks backwards and the ZIP64 extra-field update that seeks forwards both land correctly. The export holds one pooled connection at a time, which is what the review's first blocker needed. Its audit event is written in its own short write transaction, committed before the read snapshot that both content passes share, and that snapshot is a reader only (ADR 0019): an export never reserves the writer while it streams, and exports sharing a pool never hold one connection while waiting for another. A scope that holds nothing to export is refused before anything is recorded, so the audit still attests exactly what it says it does. Delivery ends only on an explicit success. Whether a transfer was complete is a flag the producer sets when the archive was produced *and* its tail flushed — not the channel closing, which a client that outlasted the failure signal would otherwise read as a complete download. Every other ending fails the body: a production failure, a producer that panicked (a drop guard covers it), a stalled client, and a client that stayed away past both send windows. Proof: streamed and buffered bytes compared across entry sizes including the patch boundary, and the bytes a live socket delivers compared against the buffered export at the instant the delivered manifest states; the installation's peak memory measured in a fresh process against pre-seeded databases (500 units / 8.7 MB archive at 5.1 MiB, the same units at four times the payload at 7.4 MiB, 4,000 units / 69.7 MB at 14.1 MiB), so the payload volume does not drive it while the unit count does; a whole export on a one-connection pool; two exports overlapping on a two-connection pool with ordinary work still progressing; an export observed mid-stream while the installation's own writer commits; a client that outlasts both stall windows and still cannot receive a complete transfer; and unit tests for each terminal case (success, closed channel without success, failure item, guard on drop, stall, departed client). Each of these was falsified against the design it replaces before it was kept. ADR 0014 and the format document state the connection ownership, the memory terms, and what an interrupted transfer means.
c1eae2f to
4755731
Compare
Repairs at
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4755731cb3
ℹ️ 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".
| // instant. It is a reader from beginning to end — the audit is already | ||
| // committed — so a download, however slow, reserves no writer, and the | ||
| // export holds exactly one connection throughout. | ||
| let mut tx = pool.begin().await.context("beginning export read")?; |
There was a problem hiding this comment.
When a client frees one queue slot before each 10-second send deadline, this transaction remains open for the entire arbitrarily slow download. Because the installation uses SQLite in WAL mode, that long-lived reader pins its WAL end mark while ordinary writes continue, preventing checkpoints from advancing and allowing the WAL to grow until the export ends; a sufficiently large or deliberately paced installation export can therefore exhaust the single data directory. Decouple the database snapshot from socket pacing or impose a total bound on its lifetime.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
| Ok(Err(_)) | Err(_) => { | ||
| // A `spawn_blocking` task cannot be cancelled once it runs; the | ||
| // producer stops on its own at its first send, because the | ||
| // response body's receiver is dropped with this return. |
There was a problem hiding this comment.
If an authenticated export request exceeds the 30-second preflight limit or disconnects before preflight completes, dropping the receivers does not stop this detached spawn_blocking task. Its first channel send occurs only after authorization, the audit write, the complete metadata scan, manifest serialization, and buffering of the manifest entry, so many abandoned requests can continue consuming blocking workers, pooled connections, memory, and audit rows long after their responses are gone. Propagate cancellation and check it before the audit and expensive metadata work rather than relying on the eventual first send.
Useful? React with 👍 / 👎.
| fn write(&mut self, buf: &[u8]) -> std::io::Result<usize> { | ||
| self.send_bytes(buf.to_vec())?; |
There was a problem hiding this comment.
For exports containing large finalized records, each completed ZIP entry is cloned into one Bytes item here, while the bounded channel limits only the number of items. The persisted canonical_bytes constraint has no size ceiling, so a stalled transport can retain several full record entries plus the producer's current copies, effectively buffering a substantial installation and risking an OOM despite the streaming change. Split released entries into fixed-size chunks or enforce a byte budget across the queue.
Useful? React with 👍 / 👎.
1 participant
Primary Issue
Closes #47
Problem And Outcome
An installation export read every finalized version with
fetch_alland assembled the whole ZIP in memory before the handler answered, so its memory cost was proportional to the installation's history. It now streams: the same documented bytes, bounded memory, and an incomplete transfer that can never be mistaken for a complete export.Changes
crates/consolebook-server/src/export_stream.rs—EntryBuffer, aWrite + Seekview of an append-only destination that holds one entry and applies thezipwriter's patches to it. A patch is classified by position (inside the held entry), so the CRC-32/size patch that seeks backwards and the ZIP64 extra-field update that seeks forwards both land correctly. This is what keeps a non-seekable destination byte-identical: no data descriptors, real CRC-32 and sizes in the local header, unchanged central directory.crates/consolebook-server/src/record_export.rs—export_towrites the container into any seekable sink: one read transaction, a metadata-only pass for the manifest (which must be the container's first entry), then a pass that streams each unit's stored bytes and unit manifest. The audit event is its own committed statement outside the read transaction, so an export reserves no writer while it streams (ADR 0019) and a scope with nothing to export is never audited.export_atkeeps the buffered API the packet and the tests use.crates/consolebook-server/src/exports_http.rs— the handler produces the archive on a blocking thread into a bounded-channel body: a slow client bounds the producer, a disconnect ends it and releases its connection, and a stall or any production failure after the response starts ends the transfer without its final chunk. The failure sentinel is actually sent (a droppedsendfuture was a silent no-op) and aDropguard covers a producer panic.crates/consolebook-server/tests/export_stream_bytes.rs— byte identity, a real socket, memory, backpressure, stall, and the write-reservation proof; unit tests inexports_http.rsfor the failure signal.docs/decisions/0014-record-export-format.md,docs/formats/record-export.md,docs/development.md,web/src/lib/api.ts.futures-util(stream rows one at a time) andhttp-body(the incremental response body).Cargo.lockchanges only the dependency list.Scope
docs/formats/record-export.mdis unchanged in its normative claims); the browser download helper, which still reads the whole response into a Blob and is documented as such.Verification
Byte identity: streamed bytes equal an independent
ZipWriter<Cursor<Vec<u8>>>reference across eight entry-size shapes (including 0/1/13/14/15/1024/65536/300000), a byte-level walk checks the descriptor flag, the patched CRC-32, the patched sizes, and every local header against the reference, and a live socket response is re-exported at theexported_atits own manifest states and compared withassert_eq!.Memory, peak growth measured in a fresh process against a pre-seeded database with the sink discarding every byte: 500 units / 8.7 MB archive → 5.2 MiB; the same 500 units at 4x payload / 33.3 MB archive → 7.5 MiB; 4,000 units / 69.7 MB archive → 13.6 MiB. The corpus's bytes do not drive the peak; the remaining linear term is the unit metadata and its JSON manifest, which ADR 0014 states.
Reservation: an export observed mid-stream while the installation's own writer commits in well under a second — this fails against the audit-inside-the-transaction design. Failure: a stalled client's transfer carries no terminal chunk and is shorter than the complete archive; a destination that stops accepting ends the export.
export verify, API byte delivery, determinism, packet bytes, and packet pin-history verification are unchanged and green.Review And Merge Notes
EntryBuffer's patch invariant, the audit's new commit point, and the failure path after the response has started.export verifywould reject.