fix: chunk the registration to the registry per-entry content-CID cap - #946
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
WalkthroughThe registry API now returns ChangesRegistry batch contract and validation
Engine registration flow
Publish refusal handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PublishPipeline
participant RegisterHelper
participant RegistryAPI
participant SyncDrain
PublishPipeline->>RegisterHelper: registration entries
RegisterHelper->>RegisterHelper: split oversized entries and batches
RegisterHelper->>RegistryAPI: bounded registration requests
RegistryAPI-->>RegisterHelper: success or registry refusal
RegisterHelper-->>PublishPipeline: registration result
PublishPipeline->>SyncDrain: register failure
SyncDrain-->>PublishPipeline: PayloadRefused or retry classification
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge-order constraint with #944Verified by building the pairwise merge: this PR and #944 are each green against This PR deletes There is also a test-fake collision: both branches add Land this PR first, then rebase #944 — that keeps the constant move owned end-to-end here, and limits #944's rebase to its own new code (one import, two usages, one test-helper rename). After a correct hand-resolution the pair is fully green: Do not merge the two back-to-back on green checkmarks alone. |
|
@coderabbitai full review please |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/registry/registry-error-codes.ts (1)
1-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the new comments.
Keep the permanent-refusal rationale and the blueprint reference. Remove the Nest implementation detail and issue-history narrative.
As per coding guidelines, keep comments short and explain why rather than what.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/registry/registry-error-codes.ts` around lines 1 - 16, Shorten the comments above REGISTRY_BATCH_REFUSED and batchRefusedBody: retain the permanent-refusal rationale, the stable client-facing code purpose, and the blueprint reference, while removing the Nest exception-object implementation details and issue-history narrative.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/openapi.json`:
- Line 455: Define a reusable JSON refusal-error schema with required
statusCode, message, error, and code fields, constraining code to
REGISTRY_BATCH_REFUSED. Apply it to both 400 responses in apps/api/openapi.json
at lines 455-455 and 509-509, and update the register and retirement response
decorators in apps/api/src/registry/registry.controller.ts at lines 55-58 and
83-86 so generated OpenAPI includes the same schema.
In `@apps/api/src/registry/registry.http.integration.test.ts`:
- Around line 153-169: Extend the over-cap batch test around the existing
namesFor assertion to verify that no PinnedCid records are created for the
refused request and that response.body does not contain any submitted content
CID, such as a representative value from contentCids. Keep the existing
refusal-code, constraint-message, IPNS-name, and empty NameInventory assertions
unchanged.
In `@apps/api/src/registry/registry.pipes.ts`:
- Line 63: Update the ParseArrayPipe configuration in the registry pipe to
preserve the original DTO ValidationError objects when invoking refuse, rather
than passing its collected string messages. Ensure malformed batches
consistently receive REGISTRY_BATCH_REFUSED with headCid null, and update the
corresponding null-head test to assert response.body.code equals
REGISTRY_BATCH_REFUSED.
---
Nitpick comments:
In `@apps/api/src/registry/registry-error-codes.ts`:
- Around line 1-16: Shorten the comments above REGISTRY_BATCH_REFUSED and
batchRefusedBody: retain the permanent-refusal rationale, the stable
client-facing code purpose, and the blueprint reference, while removing the Nest
exception-object implementation details and issue-history narrative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40f452bd-cb16-4d0b-8f51-16566d8fbadd
📒 Files selected for processing (17)
apps/api/openapi.jsonapps/api/src/registry/dto/registry.dto.tsapps/api/src/registry/registry-error-codes.tsapps/api/src/registry/registry.controller.tsapps/api/src/registry/registry.http.integration.test.tsapps/api/src/registry/registry.pipes.tsblueprint/api.mdcrates/contract/tests/contract.rscrates/engine/src/api/error.rscrates/engine/src/api/mod.rscrates/engine/src/net/mod.rscrates/engine/src/net/publish.rscrates/engine/src/net/register.rscrates/engine/src/net/retire.rscrates/engine/src/sync/drain.rscrates/engine/src/sync/rebase.rscrates/engine/tests/write_plane.rs
…-CID cap The registry caps contentCids at 1000 per register entry and refuses a larger array fail-closed. PublishRequest built a single entry carrying the version root plus every leaf, so at the production framing any file past ~1 GiB was refused - and a register 400 classified as Halt::Unclassified, which is charged nothing and retried every tick, so the op held the strict-FIFO queue head forever while every pass re-uploaded and re-registered. The registration now splits at the cap into several entries under one ipnsName, the head riding the first so the name and its pointer land ahead of any content-only entry; the server collapses them to one name row and a bare re-register leaves the stored head untouched. Register-first still holds - every chunk lands before the record PUT. A register 400 is now classified permanent, so a registration no retry can satisfy dead-letters instead of looping at the queue head. The retire chunker's cap constant moves to net::REGISTRY_BATCH_MAX, shared by both chunkers rather than copied. Closes #920
The review gates found that treating any 400 as the registry's verdict breaks the positive-evidence rule classify_upload enforces for a 413: a proxy or a version-skewed deploy answering 400 would dead-letter every queued write on sight, retiring its rows and releasing its staged blocks. The registry's batch gate now stamps code REGISTRY_BATCH_REFUSED on its own refusals, published in the OpenAPI document, and the valve branches on that code. A 400 the gate did not stamp is charged like any other pre-PUT refusal and abandons only once the attempt budget runs out. The chunker also moves out of PublishRequest into net::register, the sibling of net::retire, so both of the registry's bounds are enforced on one bounded path no future caller of the raw client can bypass.
… entry The crypto/privacy gate found that routing ParseArrayPipe's exceptionFactory straight into the error body replaced Nest's flattened constraint strings with the raw ValidationError objects - which carry `target` and `value`, so an over-cap register 400 echoed the caller's ipnsName and every contentCid back, twice, into a body that request logging and intermediaries capture. The factory now flattens to constraint strings, restoring the uniform envelope the rest of the surface emits. An explicit `headCid: null` is also refused rather than clearing the stored head: continuation entries of a chunked registration omit the field, and the invariant blueprint/api.md now states has no way to express the opposite.
…tract The 400 responses promised a stable `code` in prose but carried no schema, so a client had no documented shape to classify on. Both register and retire now answer `BatchRefusedDto`, and every refusal normalizes its message to a string list so the documented shape is the only one emitted. Cover the code on the DTO-validation and target-length paths, and assert a refused batch leaves no pin rows and echoes no submitted content CID.
6842706 to
2d27cbd
Compare
|
Rebased onto `origin/main` (76c9e52, picking up #941/#943/#945) and resolved the CodeRabbit review. Branch is now 2d27cbd. Rebase. One conflict, in `crates/engine/tests/write_plane.rs` — an import-line collision only: main added `AuthoredHead` to the `net::author` import while this branch added `REGISTRY_BATCH_MAX` to the `net` import. Resolved as a union of both. Test count went 52 (main) + 3 (this branch) = 55, and `cargo test -p cipherbox-engine` reports `55 passed` for the suite, so nothing was dropped from either side. This branch moves `RETIRE_BATCH_MAX` out of `net/retire.rs` and reintroduces it as `REGISTRY_BATCH_MAX` in `net/mod.rs`, which auto-merge would silently break if main had gained a consumer of the old name. Swept the tree post-rebase: zero occurrences of `RETIRE_BATCH_MAX` remain, and all nine `REGISTRY_BATCH_MAX` references resolve. Clean. Review body nitpick (no thread to reply on): the comments in `registry-error-codes.ts` were trimmed as suggested. The permanent-refusal rationale, the stable client-facing purpose of the code, and the blueprint reference are kept; the issue-history narrative is gone. The Nest note was kept but cut to one line — it is the non-obvious reason the envelope is hand-built rather than left to Nest, and the file now also carries `BatchRefusedDto`, so that rationale explains why the two must stay in lockstep. Verification — all green on the rebased tree, with a per-worktree `CARGO_TARGET_DIR`:
|
|
Follow-up filed as #965 — shorten the API test-tier filename suffixes. The |
1 participant
Problem
POST /registry/registercapscontentCidsat 1000 per entry and refuses a larger array fail-closed. The engine's register caller was unbounded:PublishRequest::registration()built a single entry carrying the version root plus every leaf. At the production framing any file past ~1 GiB frames to more than 1000 leaves, so its registration was refused — and the refusal did not dead-letter. A register400becameHalt::Unclassified, which is charged nothing and retried every tick, so the op held the strict-FIFO queue head forever while every pass re-uploaded and re-registered.Change
Engine. A new
crates/engine/src/net/register.rsis the bounded path to the endpoint, the sibling ofnet::retire. It owns both of the registry's bounds: an entry past the per-entrycontentCidscap splits into several entries under the sameipnsName(the head rides the first, continuations omitheadCidso the stored head survives), and the batch itself chunks toREGISTRY_BATCH_MAX.publishcalls it, so register-first still holds — every chunk lands before the record PUT. The cap constant moved out ofretire.rsintonet::REGISTRY_BATCH_MAX, shared by both chunkers rather than copied, and is now what the tests and the contract suite assert against instead of literal1000s.Failure valve.
classify_registerinsync/drain.rsdead-letters a refused registration instead of looping at the queue head — but on positive evidence only, the disciplineclassify_uploadalready applies to a 413 (#848). The registry's batch gate now stampscode: REGISTRY_BATCH_REFUSEDon its own 400s, published in the OpenAPI document; a 400 the gate did not stamp (a proxy, a body cap) isHalt::UploadAttemptand abandons only once the attempt budget runs out. Without that split, any 400 would destroy every queued write on sight — retiring its rows and releasing its staged ciphertext.API.
registry-error-codes.tsmirrors the existingupload-error-codes.ts. The pipeexceptionFactoryflattens to constraint strings, so the refusal keeps the uniform error envelope instead of echoing the caller'sipnsNameand everycontentCidback in the body. An explicitheadCid: nullis now refused rather than clearing the stored head, since a chunked registration's continuations omit the field and the invariant has no way to express the opposite.blueprint/api.md"Batch bounds" documents the split shape and the new code.Tests
Reverting the fix turns each of these red — verified:
crates/engine/tests/write_plane.rs::a_version_past_the_registration_cap_registers_in_chunks_and_publishes— 1001 leaves underContentProfile::CI. TheBlocksfake now enforces the server's bounds, so an unchunked registration really is refused: revert the split innet::registerand the op dead-letters instead of publishing....::a_registration_the_registry_refuses_dead_letters_instead_of_holding_the_queue_head— a stamped 400 dead-letters on pass 1 withPayloadRefusedand retires what its chunks charged. Red withoutclassify_register....::a_registration_400_from_an_intermediary_is_charged_not_permanent— an unstamped 400 survives toAttemptsExhausted. Red if the classifier branches on the status alone.crates/engine/src/net/register.rsunit tests — empty batch, in-bounds passthrough, over-cap entry split, bare name+head entry, over-cap batch split.crates/contract/tests/contract.rs::an_oversize_register_entry_is_refused_fail_closed— the live leg mirroringan_oversize_retire_batch_is_refused_fail_closed, asserting thecodethe valve branches on and that the engine's exact chunk shape is accepted.apps/api/src/registry/registry.http.integration.test.ts— three legs against real Postgres: the over-cap refusal stamps the code and does not echo the entry, the two-entry chunk shape keeps the head on one name row, and a nullheadCidis refused.Verification
All exit 0 in the worktree:
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo check --workspace --all-targetscargo check -p cipherbox-wasm --target wasm32-unknown-unknown --all-targetscargo test --workspace— no failures;write_plane48 passedpnpm --filter @cipherbox/api run typecheck,test177 passed,test:integration136 passed against the local Postgrespnpm -r --if-present run typecheck,eslint .,markdownlint-cli2 blueprint/api.md,prettier --checkpnpm --filter @cipherbox/api openapi:generate— the regeneratedopenapi.jsonis committed, so the CI staleness gate stays greenGates:
/security-reviewand/crypto-privacy-reviewfindings are folded into commits 2 and 3 (thecodediscriminator, and the error-body flattening plus the null-head refusal)./simplifymoved the chunker tonet::registerand deduplicated the test helpers.Parallel work
#921 is being implemented concurrently in the same registration/retire area of the engine and also extends #916. This PR moves
RETIRE_BATCH_MAXout ofnet/retire.rstonet::REGISTRY_BATCH_MAXand touchesclassify_publish,Halt::UploadAttempt's doc,upload_failure, andDeadLetterReason::PayloadRefused's doc insync/drain.rs/sync/rebase.rs. Whichever merges second should re-check those.Closes #920
Summary by CodeRabbit
New Features
REGISTRY_BATCH_REFUSEDcode.Bug Fixes
Documentation