mirror_worker: implement the add-entries submission API - #264
mirror_worker: implement the add-entries submission API#264lukevalenta wants to merge 11 commits into
Conversation
0b8665c to
530274c
Compare
|
/bonk review |
2cc18dd to
628f4c7
Compare
|
/bonk review again |
|
/bonk review |
Bonk routes through a Cloudflare AI Gateway. With response caching enabled on the gateway, re-running `/bonk review` on a PR returns the first run's cached completion, so the review stays stale even after new commits are pushed (observed on #264 after a force-push: three re-runs all returned an identical review of the pre-split code). Inject cf-aig-skip-cache via OpenCode's provider.options.headers so every model request bypasses the gateway cache and reflects the current PR.
628f4c7 to
8463e0f
Compare
|
/bonk review again, hopefully this time without caching |
|
/bonk review |
|
Review Summary – PR #264 (mirror_worker: add‑entries API) Key Issue Fixed
Verification
Conclusion |
Read and verify entry packages against the target pending checkpoint, persist them as tlog-tiles entry bundles and hash tiles (see commit.rs), and advance the persisted-entry frontier. A complete upload cosigns the mirror checkpoint and returns 200; a client-truncated upload persists the verified prefix and returns 202 with the advanced next entry so the client can resume (C2SP/C2SP#253). The whole request body is buffered and all its packages are committed in a single pass. commit.rs persists resumably from the current frontier, so a client that truncated at a package boundary resumes cleanly on its next request. A follow-up commit adds incremental streaming so a large upload does not have to buffer the entire body. Includes body-decoding (transparent gzip), per-origin prefixed R2 storage, and the ticket-sealer for recovering a past pending checkpoint on resume.
Enforce the spec's excess_entries bound in add-entries "Processing": reject with 409 when min(upload_end, next_entry) - upload_start exceeds one package (256). Entries below the persisted frontier are re-verified but not re-saved, so without a lower bound on upload_start a client could set upload_start=0 and force the mirror to re-verify the entire persisted prefix on every request. A legitimate resume sets upload_start to the persisted frontier (or, mid-tile, the frontier rounded down to a 256 boundary), keeping excess_entries at or below the threshold.
An incremental commit that resumes from a mid-tile frontier reloads the current partial entry bundle from object storage and extends it. Those bytes were re-uploaded and re-served without ever being checked: new-leaf hashes are derived only from the freshly-uploaded entries and the authenticated edge tiles, and the recomputed root never reads the reloaded bundle, so a corrupted or tampered partial bundle went undetected. Verify the reloaded bundle decodes to exactly its [subtree_start, persisted_size) leaves with no trailing bytes, and that each decoded entry's record_hash matches the authenticated leaf hash from the edge tiles, before extending it.
cosign_and_serve wrote the served checkpoint object to R2 (an unconditional overwrite of a fixed key) before advancing the durable mirror checkpoint via the DO. Under concurrent add-entries requests a slower request could overwrite R2 with an older checkpoint after a newer request had already advanced it, rewinding the served checkpoint. Move the R2 write into the state DO's /commit, serialized with the durable advance under a commit mutex (as the sequencer serializes its init path). Because a Durable Object has a single live instance per origin, holding the lock across the durable put and the R2 write makes the two atomic with respect to concurrent commits, including across the external R2 write's await, so the served checkpoint can no longer be rewound. Durable storage is advanced before the R2 write so the served object is never ahead of committed; a failed R2 write leaves it lagging and is rewritten by the next commit. The DO resolves the origin bucket from its own name (state.id().name()).
8463e0f to
fc238ed
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary – PR #264 (mirror_worker: add-entries API)
Verification: cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic clean, cargo test all pass (315 passed, 5 ignored, 0 failures), cargo fmt --all --check clean.
The three fixup commits (excess-entries bound, partial-bundle authentication, DO-owned checkpoint write) cleanly address all three review findings from @lbaquerofierro. The commit decomposition is logical and the messages accurately describe the code.
Overall this is a well-structured, carefully documented implementation. The spec references are thorough, the concurrency model is sound, and the test coverage exercises both happy and error paths for the core verification and persistence logic. A few findings below, none blocking.
Findings
1. advance_next_entry not serialized by the commit mutex (low severity, informational)
mirror_state_do.rs:366 – /advance-next-entry reads the snapshot and conditionally advances without holding commit_mux. With concurrent add-entries requests, two interleaved /advance-next-entry calls could both read the same snapshot, both pass the body.size > pending.size check, and race on the durable put. The current code is safe because the compare-and-swap semantics (line 377: body.size <= next_entry.size → no-op) prevent regression, and DO input/output gates serialize within a single request. But if two requests are in flight, the DO processes them sequentially (its single-threaded guarantee), so this is actually not a real race. Just noting that the absence of the mutex here is correct because the DO's execution model already serializes it.
2. read_persisted_leaves trusts the reloaded entry bytes (low severity)
commit.rs:71-108 – read_persisted_leaves (used by first_package_prefix in add_entries.rs:456) reads entries from R2 and returns them without verifying their record_hash against the authenticated hash tiles. These prefix leaves feed into verify_package, where they contribute to the subtree hash that is checked against the consistency proof, so a tampered prefix leaf would cause proof verification to fail, which is the correct behavior. However, the error message the client sees would be "subtree consistency proof failed" rather than something identifying that the stored prefix is corrupt. This is a minor diagnostic concern, not a security issue.
3. base64 moved from dev-dependency to dependency (correct but worth noting)
Cargo.toml diff – base64 was previously a dev-only dependency for dev-config pin tests. It's now a regular dependency because lib.rs:274 uses it in load_ticket_sealer to decode MIRROR_TICKET_KEY. This is correct.
4. CommitRequest.signed_note_bytes carries the cosigned note, not the log's original
add_entries.rs:410-421 – cosign_and_serve constructs the served checkpoint as target.signed_note_bytes + cosig_body and sends that to the DO via CommitRequest.signed_note_bytes. The DO stores this in CommittedCheckpoint.signed_note_bytes (line 336). The doc comment on CommittedCheckpoint.signed_note_bytes (mirror_state_do.rs:79) says "Full signed-note bytes as the log signed them" — but after this PR the value stored there is actually the log's note plus the mirror's cosignature lines. The comment should be updated to match reality.
5. Test helper duplication: entry() function
add_entries.rs:833 and commit.rs:372 both define fn entry(i: u64) -> Vec<u8> identically (format!("entry-{i}").into_bytes()). Not worth refactoring now given the modules are different test scopes, but if a third copy appears, consider extracting it.
No blocking issues. The prior review findings have all been addressed correctly.
|
Review posted. All builds, lint, and tests pass. Five low-severity findings, none blocking:
|
CommittedCheckpoint.signed_note_bytes stores the served checkpoint (the log's signed note with the mirror's cosignature appended), not the log's original note, so describe it that way.
Fixed with 90e5403. None of the other comments require changes. |
read_persisted_leaves returned reloaded entry-bundle bytes without checking them, so a corrupted or stale bundle flowed into package verification and surfaced as a client 422 for a mirror storage fault. Authenticate the decoded leaves against the frontier root via the tlog-tiles tile-hash reader (edge tiles alone cannot cover a resume that lands below the frontier's edge tile, which the excess_entries bound permits). Addresses bonk #264 review on commit.rs read_persisted_leaves.
The spec says the mirror discards any partial bytes after the last successfully authenticated entry package; the only defined 400 is when no package was authenticated at all. The trailing-byte 400 also ran after persist_entries and advance_next_entry had already committed and advanced the frontier, so it told the client nothing was saved when everything was, with no next_entry to resume from. Drop the check. Addresses bonk #264 review on add_entries.rs trailing-data handling.
dispatch_commit discarded the DO response and cosign_and_serve always returned 200 with a cosignature, even when the DO refused to rewind because a concurrent add-entries had already advanced the mirror checkpoint past upload_end. The client read that as "my checkpoint is served" while the mirror was actually at a larger size. Return the DO's CommittedCheckpoint and, when its size is ahead of upload_end, respond 409 with mirror-info per the spec's "upload_end too small" case so the client resyncs. Addresses bonk #264 review on cosign_and_serve.
The spec requires the mirror to accept upload_end equal to the mirror checkpoint's tree size, not just a known pending value; resolve_target_ pending only matched the current pending or a ticket-carried past pending, so a client targeting the already-committed size with no valid ticket got a spurious 409. Accept upload_end == committed.size using the committed checkpoint as the target. cosign_and_serve now skips the /commit dispatch when the upload only reaches the committed size, which also avoids redundantly rewriting R2 and appending a duplicate cosignature line. Addresses bonk #264 review on resolve_target_pending.
Enforce the spec's Content-Type MUST, advertise gzip support in responses, and use a fresh DO snapshot when a concurrent commit forces a final 409 so the mirror-info body is not stale.
2 participants
NOTE(lvalenta): I separated #267 (streaming in the add-entries request body) out from this commit to try to make it slightly easier to review. Integration tests are also deferred to a later PR.
Read and verify entry packages against the target pending checkpoint,
persist them as tlog-tiles entry bundles and hash tiles (see commit.rs),
and advance the persisted-entry frontier. A complete upload cosigns the
mirror checkpoint and returns 200; a client-truncated upload persists the
verified prefix and returns 202 with the advanced next entry so the
client can resume (C2SP/C2SP#253).
The whole request body is buffered and all its packages are committed in
a single pass. commit.rs persists resumably from the current frontier, so
a client that truncated at a package boundary resumes cleanly on its next
request. A follow-up commit adds incremental streaming so a large upload
does not have to buffer the entire body.
Includes body-decoding (transparent gzip), per-origin prefixed R2
storage, and the ticket-sealer for recovering a past pending checkpoint
on resume.