fix(file-storage): remediate audit findings across cleanup, multipart, authz, and docs#4218
Open
ffedoroff wants to merge 4 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 55 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFile Storage updates add exclusive upload publication, stricter range and multipart validation, independent multipart session TTLs, safer cleanup and authorization behavior, batched metadata and transactional events, configuration/schema changes, expanded API error documentation, and aligned architecture and operational documentation. ChangesFile Storage behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
ffedoroff
force-pushed
the
fix/file-storage-audit-remediation
branch
from
July 16, 2026 14:44
db0622e to
f1fd616
Compare
Open
4 tasks
ffedoroff
force-pushed
the
fix/file-storage-audit-remediation
branch
2 times, most recently
from
July 16, 2026 16:33
3ed3292 to
00cc7c8
Compare
…, authz, and docs Doc-vs-implementation audit of gears/file-storage surfaced correctness bugs, security gaps, and systemic doc drift. Fixes, by area: Data loss / integrity: - cleanup sweep step 1 now uses the status-guarded delete_pending_version, so a version finalized between list and delete survives the sweep - backend abort_multipart now always fires for expired sessions, even when step 1 already reclaimed the version row (S3 MPU handle leak) - abort and expired-session sweep delete multipart_upload_parts rows - create-exclusive blob publish (local-fs via hard_link, in-memory under one mutex): PUT-token replay against a published object returns 409 instead of silently overwriting finalized content Postgres compatibility: - policies.body / retention_rules.body entities switched from Text to Json, matching the jsonb DDL; writes previously failed on Postgres (suite runs SQLite only); regression tests pin the column type DoS / limits: - MAX_PART_COUNT=10000 enforced uniformly in compute_plan with part-size widening up to 5 GiB before rejection; huge declared_size rejected without allocation; plans that S3 would reject at part 10001 are no longer minted - new multipart_session_ttl_secs config (default 24h) decouples session lifetime from the 15-minute signed-URL TTL Authorization / consistency: - file-scope retention-rule authz prefetches with tenant scope, closing a cross-tenant file-existence oracle; orphaned file-scope rules deletable - transfer_ownership returns the pre-captured row instead of a post-commit re-read under the stale scope - idempotent create replay re-validates against the current policy - GET /policy?scope=user without scope_owner_id returns 400 - report-part validates the 32-byte hash like finalize does API / infra: - file.metadata_updated event emitted transactionally with the mutation - Range parsing per RFC 9110 (start>end ignored, strict digits); 416 carries Accept-Ranges; S3 is_ready probes the bucket (ListObjectsV2) instead of folding NoSuchBucket into ready; sidecar fails fast on malformed numeric env vars; config validate() covers page-size/TTL cross-field rules; GET /files returns real custom_metadata via one batched query; missing 400/409 declarations added to OpenAPI routes Documentation sync: - operations.md, DESIGN.md (token codec/claims, ETag formula, hash pipeline, sidecar MIME), api.md (real sidecar headers with an explicit Planned section, actual contracts and status codes), migration.sql (hash-mode DDL, exact event/operation catalogs), ADR-0003/0005/0006 corrections - unimplemented documented features (X-FS-* headers, ip/tok.* claims, enabled_event_types gating, part-hash trust gap) marked as tracked gaps - stale temp plans DEFERRED_ITEMS_PLAN.txt / IMPLEMENTATION_PLAN_TEMP.txt deleted; the one still-open item (content_id FK) moved to DECOMPOSITION.md Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG Signed-off-by: Roland From <rfedorov@linkentools.com>
ffedoroff
force-pushed
the
fix/file-storage-audit-remediation
branch
from
July 16, 2026 17:42
00cc7c8 to
9d2d7d9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gears/file-storage/file-storage/src/domain/service/write.rs (1)
471-501: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDerive the event revision inside the transaction.
With no
expected_meta_version, concurrent patches can both read revisionNand enqueueN+1, while the second transaction commits revisionN+2. Obtain the updated revision insidepatch_metadata_atomicand build the event payload from that committed value.🤖 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 `@gears/file-storage/file-storage/src/domain/service/write.rs` around lines 471 - 501, The event revision is computed before the transaction, allowing concurrent patches to enqueue stale values. Update patch_metadata_atomic and its caller to derive the post-CAS meta_version inside the transaction, then construct the file.metadata_updated event payload from that committed revision before enqueueing or committing it; remove the pre-transaction file.meta_version + 1 calculation.
🧹 Nitpick comments (1)
gears/file-storage/file-storage/tests/multipart_test.rs (1)
3426-3479: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both sides of each TTL contract.
These one-sided checks still pass for already-expired URLs, excessively long sessions, or resume tokens extending beyond the session. Add bounded tolerance windows and verify
claims.exp <= session.expires_at.Proposed assertions
+ assert!( + plan.expires_at >= before + time::Duration::seconds(60 - 5) + ); assert!( plan.expires_at <= after + time::Duration::seconds(60 + 5) ); assert!( session.expires_at >= before + time::Duration::seconds(3600 - 5) ); + assert!( + session.expires_at <= after + time::Duration::seconds(3600 + 5) + ); assert!( claims.exp > (before + time::Duration::seconds(60)).unix_timestamp() ); + assert!(claims.exp <= session.expires_at.unix_timestamp());🤖 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 `@gears/file-storage/file-storage/tests/multipart_test.rs` around lines 3426 - 3479, Strengthen the TTL assertions in the multipart test around plan.expires_at, session.expires_at, and claims.exp: add upper and lower tolerance bounds so expired or excessively long values fail, while preserving the expected short URL TTL and long session TTL. Also assert that claims.exp does not exceed session.expires_at, using the existing session expiry and token verification flow.
🤖 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
`@gears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.md`:
- Around line 155-156: Update the ADR’s migration-status references so the
PASETO v4.public and kid/key-rotation work has one unambiguous tracking status:
remove the stale “no dedicated ticket exists” Tier-4 statement near the cited
passage, or revise the later “Tier 4 item 4.9” reference to match the actual
status.
In `@gears/file-storage/docs/api.md`:
- Around line 528-530: Update the upload-integrity sentence in the API
documentation to remove the claim that hashing occurs at bind. State that
single-part size/hash verification occurs during finalize, while multipart
completion derives the composite hash from reported parts without a full
assembled-object read-back; preserve the existing signed-URL and
max_size/expected_hash context.
In `@gears/file-storage/docs/DESIGN.md`:
- Line 1227: Update the hash_algorithm row in DESIGN.md to describe SHA-256 as
the single algorithm with both whole-object and multipart-composite hash modes,
replacing the inaccurate “single mode” wording while preserving the existing
migration references.
- Around line 1584-1602: The earlier sidecar responsibility section must stop
claiming enforcement of IP/CIDR and token-claim predicates. Update that section
to list only the implemented checks: signature, expiry, operation, target
binding, and streaming size/hash constraints, matching the behavior described in
the verification checklist.
- Around line 1516-1522: Update the earlier signed-URL responsibility section to
remove its contradictory claim that backend_id and backend_path are resolved
from the version row and excluded from the token. Align that description with
the authoritative Claims section by stating that both fields are carried in the
signed token for stateless sidecar resolution.
- Around line 315-323: Update the remaining configuration documentation and
BackendConfig model descriptions to remove stale “static TOML” and “loaded from
TOML” references. Align them with the gear YAML FileStorageConfig source and
FS_SIDECAR_* environment variables described in the configuration section,
without changing the documented runtime behavior.
- Around line 608-619: Update the preceding section rationale and all related
responsibility references to remove the superseded two-tap content-pipeline
model, including centralized sidecar SHA-256 and in-stream magic-byte detection.
Align the documentation with the shipped split described here: sidecar handlers
hash incrementally, while control-plane finalize/complete performs MIME
validation after storage with the existing 400 rejection behavior; do not
describe or imply an in-stream 415 flow.
- Around line 62-63: Update DESIGN.md to remove all remaining claims that the
sidecar connects to or reads from the metadata DB, including the SC --> DB
deployment-diagram relationship, the technology-table entry, and later SDK-based
DB-read references. Preserve the documented boundary that only the control plane
accesses the metadata DB and the sidecar uses verified signed-token claims.
In `@gears/file-storage/file-storage/src/bin/sidecar.rs`:
- Around line 53-75: Update upload and its PublishOutcome handling so a rejected
exclusive publish never finalizes from the rejected attempt’s size/hash.
Validate size and digest before committing the new blob, or obtain trustworthy
metadata for an existing blob and require an exact match before finalization;
ensure newly published blobs are not finalized when later checks fail. Add a
regression covering an invalid first PUT followed by a valid retry.
In `@gears/file-storage/file-storage/src/domain/multipart_service.rs`:
- Around line 541-551: Update the resume URL expiration logic in
introspect_multipart_upload to cap each minted part URL at the configured
url_ttl_secs, using the earlier of session.expires_at and now plus url_ttl_secs.
Preserve the session expiration as the upper bound while preventing resumed URLs
from lasting beyond the short URL lifetime.
In `@gears/file-storage/file-storage/src/domain/policy_service.rs`:
- Around line 66-78: Update the shared policy scope/owner validation used by
both read and write paths, including Self/admin handling, to accept only Tenant
+ None, User + Some(owner), and the other valid scope shapes. Reject Tenant +
Some(scope_owner_id) and User + None with validation errors, and reuse this
validator from the relevant read and write methods instead of maintaining
separate checks.
- Around line 263-287: Update the retention-rule lookup flow around
authorize_retention_scope so the rule is first fetched with the caller’s tenant
scope instead of allow_all, preventing foreign-tenant rules from being
distinguishable from missing IDs when resource-less WRITE is denied; preserve
the existing deleted File-scope fallback and scope-specific authorization. In
gears/file-storage/file-storage/tests/policy_authz_test.rs lines 604-659, add
coverage for a foreign caller denied resource-less WRITE and verify the response
matches the nonexistent-rule result.
In `@gears/file-storage/file-storage/src/domain/service/create.rs`:
- Around line 174-192: The replay validation in the create flow must also
enforce the current effective maximum size. Update the logic around
get_effective_policy_internal, PolicyResolver::check_allowed_mime, and
PolicyResolver::check_metadata_limits to compare the stored upload URL’s
max_size with the current policy and reject replay when it exceeds that limit,
or re-sign the URL using the current constraint.
In `@gears/file-storage/file-storage/src/domain/service/read_ops.rs`:
- Around line 45-56: Restrict list_metadata_for_files so callers cannot fetch
metadata for arbitrary file IDs without authorization. Prefer replacing it with
an authorization-bound list_files_with_metadata operation that accepts
SecurityContext and owner, or make the raw batch lookup private/internal and
require a validated authorization scope before use; update handlers::list_files
and callers accordingly while preserving batched retrieval.
In `@gears/file-storage/file-storage/src/infra/backend/mod.rs`:
- Around line 164-234: The default implementation of Backend::publish_exclusive
is racy because its exists-then-put sequence can overwrite data under concurrent
calls, especially for S3Backend. Remove this fallback behavior and require each
production backend, including S3Backend, to provide an atomic create-exclusive
implementation or explicitly return an unsupported error; ensure uploads are not
routed through a non-atomic implementation.
---
Outside diff comments:
In `@gears/file-storage/file-storage/src/domain/service/write.rs`:
- Around line 471-501: The event revision is computed before the transaction,
allowing concurrent patches to enqueue stale values. Update
patch_metadata_atomic and its caller to derive the post-CAS meta_version inside
the transaction, then construct the file.metadata_updated event payload from
that committed revision before enqueueing or committing it; remove the
pre-transaction file.meta_version + 1 calculation.
---
Nitpick comments:
In `@gears/file-storage/file-storage/tests/multipart_test.rs`:
- Around line 3426-3479: Strengthen the TTL assertions in the multipart test
around plan.expires_at, session.expires_at, and claims.exp: add upper and lower
tolerance bounds so expired or excessively long values fail, while preserving
the expected short URL TTL and long session TTL. Also assert that claims.exp
does not exceed session.expires_at, using the existing session expiry and token
verification flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9247a286-75b5-4d5e-9a04-8d6eaa672c78
📒 Files selected for processing (49)
docs/api/api.jsongears/file-storage/README.mdgears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.mdgears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.mdgears/file-storage/docs/ADR/0005-cpt-cf-file-storage-adr-s3-client-selection.mdgears/file-storage/docs/DECOMPOSITION.mdgears/file-storage/docs/DEFERRED_ITEMS_PLAN.txtgears/file-storage/docs/DESIGN.mdgears/file-storage/docs/IMPLEMENTATION_PLAN_TEMP.txtgears/file-storage/docs/api.mdgears/file-storage/docs/features/audit-trail.mdgears/file-storage/docs/features/multipart-coordinator.mdgears/file-storage/docs/features/ownership-transfer.mdgears/file-storage/docs/features/policy-engine.mdgears/file-storage/docs/features/retention-cleanup.mdgears/file-storage/docs/migration.sqlgears/file-storage/docs/operations.mdgears/file-storage/file-storage/src/api/rest/handlers.rsgears/file-storage/file-storage/src/api/rest/routes.rsgears/file-storage/file-storage/src/bin/sidecar.rsgears/file-storage/file-storage/src/bin/sidecar_tests.rsgears/file-storage/file-storage/src/config.rsgears/file-storage/file-storage/src/config_tests.rsgears/file-storage/file-storage/src/domain/cleanup.rsgears/file-storage/file-storage/src/domain/multipart.rsgears/file-storage/file-storage/src/domain/multipart_service.rsgears/file-storage/file-storage/src/domain/policy_service.rsgears/file-storage/file-storage/src/domain/service/create.rsgears/file-storage/file-storage/src/domain/service/read_ops.rsgears/file-storage/file-storage/src/domain/service/write.rsgears/file-storage/file-storage/src/gear.rsgears/file-storage/file-storage/src/infra/backend/backend_tests.rsgears/file-storage/file-storage/src/infra/backend/in_memory.rsgears/file-storage/file-storage/src/infra/backend/local_fs.rsgears/file-storage/file-storage/src/infra/backend/mod.rsgears/file-storage/file-storage/src/infra/content/range.rsgears/file-storage/file-storage/src/infra/content/range_tests.rsgears/file-storage/file-storage/src/infra/storage/repo/metadata_repo.rsgears/file-storage/file-storage/src/infra/storage/repo/multipart_repo.rsgears/file-storage/file-storage/src/infra/storage/store/files.rsgears/file-storage/file-storage/src/infra/storage/store/metadata.rsgears/file-storage/file-storage/src/infra/storage/store/multipart.rsgears/file-storage/file-storage/tests/audit_test.rsgears/file-storage/file-storage/tests/cleanup_test.rsgears/file-storage/file-storage/tests/idempotency_authz_test.rsgears/file-storage/file-storage/tests/multipart_test.rsgears/file-storage/file-storage/tests/ownership_test.rsgears/file-storage/file-storage/tests/policy_authz_test.rsgears/file-storage/file-storage/tests/service_test.rs
💤 Files with no reviewable changes (1)
- gears/file-storage/docs/DEFERRED_ITEMS_PLAN.txt
Comment on lines
+164
to
+234
| /// Publish a blob at `path`, but **only if nothing is stored there yet** | ||
| /// (create-exclusive semantics) — unlike [`Self::put_stream`], which is | ||
| /// documented as overwrite-allowed and stays that way for its existing | ||
| /// callers (per-part multipart writes, which are deliberately | ||
| /// overwrite-safe for resume; `migrate_backend`'s write to a fresh | ||
| /// backend). This method exists for exactly one call site: the sidecar's | ||
| /// single-shot upload handler, publishing a version's *final*, canonical | ||
| /// object at `/{file_id}/{version_id}`. | ||
| /// | ||
| /// # Why this must not just overwrite (P2 remediation — replay-`PUT` fix) | ||
| /// A `PUT` token's signature covers `op`/`file_id`/`version_id`/size/hash | ||
| /// *constraints*, never the body bytes (DESIGN.md, ADR-0003), and stays | ||
| /// valid until `exp`. Once a version has been finalized and bound as a | ||
| /// file's live content, a holder of that same still-unexpired token could | ||
| /// otherwise re-`PUT` different bytes to the same backend path and | ||
| /// silently replace the live, already-served content out from under the | ||
| /// recorded size/hash/`ETag`/MIME — a HIGH-severity immutability break, | ||
| /// not the merely-orphans-a-version consequence the design previously | ||
| /// assumed. Making this call create-exclusive closes that: only the | ||
| /// *first* write to a given path ever lands; every subsequent attempt | ||
| /// observes `created: false` and the existing bytes are provably | ||
| /// untouched. | ||
| /// | ||
| /// The default implementation is a non-atomic (TOCTOU) `exists` check | ||
| /// followed by `put` — good enough for a backend with no cheaper atomic | ||
| /// primitive, but a real race window exists between the check and the | ||
| /// write. [`LocalFsBackend`](super::LocalFsBackend) (`std::fs::hard_link`, | ||
| /// which atomically fails with `AlreadyExists` if the target already has | ||
| /// a directory entry) and [`InMemoryBackend`](super::InMemoryBackend) | ||
| /// (a single mutex guards both the check and the insert) both override | ||
| /// this with a truly atomic implementation. `S3Backend` is left on this | ||
| /// default fallback (out of scope for this remediation — see its own | ||
| /// module doc for the ADR-0005 merge gate). | ||
| async fn publish_exclusive( | ||
| &self, | ||
| path: &str, | ||
| stream: futures::stream::BoxStream<'_, std::io::Result<Bytes>>, | ||
| max_size: Option<u64>, | ||
| ) -> Result<PublishOutcome, DomainError> { | ||
| use futures::StreamExt; | ||
|
|
||
| let mut buf = Vec::new(); | ||
| let mut stream = stream; | ||
| while let Some(chunk) = stream.next().await { | ||
| let chunk = chunk.map_err(|e| DomainError::backend(self.id(), e.to_string()))?; | ||
| buf.extend_from_slice(&chunk); | ||
| if max_size.is_some_and(|m| buf.len() as u64 > m) { | ||
| return Err(DomainError::validation("size", "exceeds max_size")); | ||
| } | ||
| } | ||
| let bytes_written = buf.len() as u64; | ||
| let digest = | ||
| crate::infra::content::hash::digest_to_array(crate::infra::content::hash::sha256(&buf)); | ||
|
|
||
| // Non-atomic check-then-act: see this method's doc comment for the | ||
| // race this fallback accepts as the price of a backend-agnostic | ||
| // default. | ||
| if self.exists(path).await? { | ||
| return Ok(PublishOutcome { | ||
| bytes_written, | ||
| digest, | ||
| created: false, | ||
| }); | ||
| } | ||
| self.put(path, Bytes::from(buf)).await?; | ||
| Ok(PublishOutcome { | ||
| bytes_written, | ||
| digest, | ||
| created: true, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Do not expose a racy fallback as create-exclusive.
Two concurrent calls can both observe exists == false and then execute overwrite-capable put; the last write wins. Since Lines 194-196 state that S3 uses this fallback, concurrent token replay still violates the promised immutability guarantee. Require an atomic implementation—or return unsupported—for every production backend before routing uploads through this method.
🤖 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 `@gears/file-storage/file-storage/src/infra/backend/mod.rs` around lines 164 -
234, The default implementation of Backend::publish_exclusive is racy because
its exists-then-put sequence can overwrite data under concurrent calls,
especially for S3Backend. Remove this fallback behavior and require each
production backend, including S3Backend, to provide an atomic create-exclusive
implementation or explicitly return an unsupported error; ensure uploads are not
routed through a non-atomic implementation.
Contributor
Author
There was a problem hiding this comment.
Acknowledged and scoped rather than fixed here: the only backends routed through publish_exclusive in a default topology (local-fs, in-memory) have atomic overrides; S3 — the one backend left on the non-atomic fallback — is opt-in and already release-gated by the ADR-0005 external-dependency security review. s3.rs belongs to the split PR #4219, so on this branch we strengthened the default-impl doc comment (explicit TOCTOU warning + requirement of an atomic If-None-Match: * conditional-PUT override before S3 leaves its gate) and recorded it in ADR-0003's Known-gap paragraph alongside the part-hash trust gap.
There was a problem hiding this comment.
@ffedoroff, understood. The atomic overrides protect the default local-fs and in-memory paths, and the strengthened contract/ADR makes the remaining S3 limitation explicit.
I’ll keep the finding open rather than withdraw it: an opt-in S3 deployment would still permit the race until S3Backend supplies an atomic conditional publish. If #4219 does not implement that If-None-Match: * behavior, would you like me to open a dedicated follow-up issue with the ADR-0005 release-gate acceptance criterion?
✏️ Learnings added
Learnt from: ffedoroff
Repo: constructorfabric/gears-rust PR: 4218
File: gears/file-storage/file-storage/src/infra/backend/mod.rs:164-255
Timestamp: 2026-07-16T20:01:48.675Z
Learning: In `gears/file-storage/file-storage/src/infra/backend/mod.rs`, `LocalFsBackend` and `InMemoryBackend` implement atomic `StorageBackend::publish_exclusive` overrides. `S3Backend` currently relies on the non-atomic default fallback, is opt-in, and must implement an atomic conditional PUT (for example, `If-None-Match: *`) before ADR-0005’s release gate permits general availability. S3 work is split into PR `#4219`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
Author
There was a problem hiding this comment.
Agreed — keeping it open as a tracked gate item is the right call. #4219 does not implement the If-None-Match: * conditional publish for S3; please go ahead and open the dedicated follow-up issue with the ADR-0005 release-gate acceptance criterion (atomic conditional-PUT override on S3Backend::publish_exclusive, 412→already-exists mapping, concurrent-replay test). S3 stays behind the ADR-0005 security-review gate until then, so no production topology routes replay through the racy fallback in the meantime.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="constructorfabric/gears-rust"
body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT
cat >"$body_file" <<'EOF'
## Summary
Implement an atomic, create-exclusive `StorageBackend::publish_exclusive` override for `S3Backend`.
`S3Backend` currently inherits the trait's `exists`-then-`put` fallback. Concurrent uploads using the same still-valid single-shot token can both observe an absent object and then overwrite each other, violating the immutable-blob guarantee.
## Required changes
- Add an S3-specific atomic conditional publish operation in `gears/file-storage/file-storage/src/infra/backend/s3.rs`.
- Use S3 conditional creation (`If-None-Match: *`, or an equivalent atomic S3 primitive) when publishing the canonical single-shot object.
- Map S3 `412 Precondition Failed` to `PublishOutcome { created: false, .. }`; preserve the existing object and report the attempted stream's measured bytes/digest.
- Keep all other S3/backend failures surfaced as backend errors.
- Do not rely on the default non-atomic `StorageBackend::publish_exclusive` fallback for S3 uploads.
## Rationale
The LocalFs and in-memory implementations already provide atomic create-exclusive semantics. S3 is opt-in and remains behind the ADR-0005 security-review/release gate, but it must not leave that gate until this implementation is present.
## Acceptance criteria
- [ ] `S3Backend::publish_exclusive` uses an atomic conditional-PUT/create operation.
- [ ] A pre-existing object produces `created: false` by mapping HTTP `412 Precondition Failed`, without modifying stored bytes.
- [ ] A concurrent-replay regression test proves that racing publishes to the same path cannot overwrite the first successfully published object.
- [ ] Relevant S3 error handling and tests cover successful publication, already-exists behavior, and unexpected backend failures.
- [ ] ADR-0005's release gate is not considered satisfied until the above behavior is implemented and validated.
## Backlinks
- Requested by `@ffedoroff` in PR `#4218`: https://github.com/constructorfabric/gears-rust/pull/4218
- Review discussion: https://github.com/constructorfabric/gears-rust/pull/4218#discussion_r3597756704
- Related work: PR `#4219`
EOF
gh issue create \
--repo "$repo" \
--title "Implement atomic S3 publish_exclusive for immutable uploads" \
--body-file "$body_file" \
--assignee "ffedoroff"Length of output: 12182
@ffedoroff, created and assigned the follow-up issue: #4221 — Implement atomic S3 publish_exclusive for immutable uploads.
It includes the ADR-0005 release-gate requirement, atomic If-None-Match: * conditional publish, 412 → created: false mapping, concurrent-replay regression coverage, and backlinks to this PR discussion.
🧬 Code Graph Analysis Results
Relevant snippets
gears/file-storage/file-storage/src/infra/backend/mod.rs
[Approx. lines 80-170] — PublishOutcome
#[derive(Debug, Clone, Copy)]
pub struct PublishOutcome {
pub bytes_written: u64,
pub digest: [u8; 32],
pub created: bool,
}[Approx. lines 150-270] — StorageBackend::publish_exclusive default (non-atomic TOCTOU fallback; S3 explicitly called out as missing atomic conditional-PUT override)
async fn publish_exclusive(
&self,
path: &str,
stream: futures::stream::BoxStream<'_, std::io::Result<Bytes>>,
max_size: Option<u64>,
) -> Result<PublishOutcome, DomainError> {
use futures::StreamExt;
let mut buf = Vec::new();
let mut stream = stream;
while let Some(chunk) = stream.next().await {
let chunk = chunk.map_err(|e| DomainError::backend(self.id(), e.to_string()))?;
buf.extend_from_slice(&chunk);
if max_size.is_some_and(|m| buf.len() as u64 > m) {
return Err(DomainError::validation("size", "exceeds max_size"));
}
}
let bytes_written = buf.len() as u64;
let digest =
crate::infra::content::hash::digest_to_array(crate::infra::content::hash::sha256(&buf));
// Non-atomic check-then-act: see this method's doc comment for the
// race this fallback accepts as the price of a backend-agnostic
// default.
if self.exists(path).await? {
return Ok(PublishOutcome {
bytes_written,
digest,
created: false,
});
}
self.put(path, Bytes::from(buf)).await?;
Ok(PublishOutcome {
bytes_written,
digest,
created: true,
})
}Doc-comment context (inside this method): the default is explicitly non-atomic (exists then put), and the comments state S3 is the one backend still left on this fallback until it is replaced with an atomic conditional-PUT override on S3Backend::publish_exclusive (e.g. conditional write using If-None-Match: *), including mapping the resulting 412 to created: false to match LocalFsBackend/InMemoryBackend, plus closing the replay/exclusive publish race.
gears/file-storage/file-storage/src/infra/backend/local_fs.rs
[lines 316-360] — LocalFsBackend::publish_exclusive (atomic create-exclusive via hard-link; maps already-exists to created: false)
async fn publish_exclusive(
&self,
path: &str,
stream: BoxStream<'_, std::io::Result<Bytes>>,
max_size: Option<u64>,
) -> Result<PublishOutcome, DomainError> {
let (target, parent) = self.prepare_target(path).await?;
let tmp = Self::tmp_path_for(&target);
let write_result = self.write_stream_to_tmp(&tmp, stream, max_size).await;
let (bytes_written, digest) = match write_result {
Ok(v) => v,
Err(e) => {
drop(tokio::fs::remove_file(&tmp).await);
return Err(e);
}
};
match self
.publish_tmp_exclusive(&tmp, &target, parent.as_deref())
.await
{
Ok(true) => Ok(PublishOutcome {
bytes_written,
digest,
created: true,
}),
Ok(false) => {
// The destination already existed: it was never touched.
// `tmp` still holds this attempt's freshly-written (but
// never linked) bytes and must be cleaned up like any other
// rejected upload.
drop(tokio::fs::remove_file(&tmp).await);
Ok(PublishOutcome {
bytes_written,
digest,
created: false,
})
}
Err(e) => {
drop(tokio::fs::remove_file(&tmp).await);
Err(e)
}
}
}[lines 165-179] — publish_tmp_exclusive helper (AlreadyExists → Ok(false))
async fn publish_tmp_exclusive(
&self,
tmp: &Path,
target: &Path,
parent: Option<&Path>,
) -> Result<bool, DomainError> {
match tokio::fs::hard_link(tmp, target).await {
Ok(()) => {
self.finish_exclusive_publish(tmp, parent).await;
Ok(true)
}
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => Ok(false),
Err(e) => Err(self.io_err(e)),
}
}gears/file-storage/file-storage/src/infra/backend/in_memory.rs
[lines 112-143] — InMemoryBackend::publish_exclusive (atomic under a single lock; already-present → created: false)
async fn publish_exclusive(
&self,
path: &str,
mut stream: BoxStream<'_, std::io::Result<Bytes>>,
max_size: Option<u64>,
) -> Result<PublishOutcome, DomainError> {
let mut buf = Vec::new();
while let Some(chunk) = stream.next().await {
let chunk = chunk.map_err(|e| DomainError::backend(&self.id, e.to_string()))?;
buf.extend_from_slice(&chunk);
if max_size.is_some_and(|m| buf.len() as u64 > m) {
return Err(DomainError::validation("size", "exceeds max_size"));
}
}
let bytes_written = buf.len() as u64;
let digest = hash::digest_to_array(hash::sha256(&buf));
let mut blobs = self.lock_blobs()?;
if blobs.contains_key(path) {
return Ok(PublishOutcome {
bytes_written,
digest,
created: false,
});
}
blobs.insert(path.to_owned(), Bytes::from(buf));
Ok(PublishOutcome {
bytes_written,
digest,
created: true,
})
}gears/file-storage/file-storage/src/infra/backend/s3.rs
[lines 309-317] — S3Backend::put (plain PutObject; no conditional header logic shown in this snippet)
async fn put(&self, path: &str, bytes: Bytes) -> Result<(), DomainError> {
let key = Self::path_to_key(path);
let url = self
.bucket
.put_object(Some(&self.credentials), key)
.sign(SIGN_DURATION);
self.send_and_check(self.http.put(url).body(bytes)).await?;
Ok(())
}[lines 596-613] — S3Backend::exists (implemented via HEAD; returns true/false based on status)
async fn exists(&self, path: &str) -> Result<bool, DomainError> {
let key = Self::path_to_key(path);
let url = self
.bucket
.head_object(Some(&self.credentials), key)
.sign(SIGN_DURATION);
let resp = self
.http
.head(url)
.send()
.await
.map_err(|e| self.transport_err(&e))?;
match resp.status() {
StatusCode::OK => Ok(true),
StatusCode::NOT_FOUND => Ok(false),
other => Err(self.head_error(path, other)),
}
}gears/file-storage/file-storage/src/infra/backend/backend_tests.rs
[lines 329-412] — local_fs_publish_exclusive_rejects_second_write_to_same_path (single-path second call → created: false, immutability verified)
async fn local_fs_publish_exclusive_rejects_second_write_to_same_path() {
let root = unique_root();
let b = LocalFsBackend::new("fs", &root);
let stream1: BoxStream<'_, std::io::Result<Bytes>> =
Box::pin(stream::iter(vec![Ok(Bytes::from_static(b"first"))]));
let outcome1 = b
.publish_exclusive("fid/vid", stream1, None)
.await
.expect("first publish_exclusive call must succeed");
assert!(
outcome1.created,
"first publish_exclusive call to a fresh path must create it"
);
assert_eq!(
b.get("fid/vid").await.unwrap(),
Bytes::from_static(b"first")
);
let stream2: BoxStream<'_, std::io::Result<Bytes>> = Box::pin(stream::iter(vec![Ok(
Bytes::from_static(b"second-different-bytes"),
)]));
let outcome2 = b
.publish_exclusive("fid/vid", stream2, None)
.await
.expect("second publish_exclusive call must not error, just report created=false");
assert!(
!outcome2.created,
"a second publish_exclusive call to an already-published path must not overwrite it"
);
assert_eq!(
b.get("fid/vid").await.unwrap(),
Bytes::from_static(b"first"),
"an already-published blob must never be overwritten by publish_exclusive"
);
drop(tokio::fs::remove_dir_all(&root).await);
}[lines 381-412] — in_memory_publish_exclusive_rejects_second_write_to_same_path (same semantics as local-fs test)
async fn in_memory_publish_exclusive_rejects_second_write_to_same_path() {
let b = InMemoryBackend::new("mem");
let stream1: BoxStream<'_, std::io::Result<Bytes>> =
Box::pin(stream::iter(vec![Ok(Bytes::from_static(b"first"))]));
let outcome1 = b
.publish_exclusive("fid/vid", stream1, None)
.await
.expect("first publish_exclusive call must succeed");
assert!(
outcome1.created,
"first publish_exclusive call to a fresh path must create it"
);
let stream2: BoxStream<'_, std::io::Result<Bytes>> = Box::pin(stream::iter(vec![Ok(
Bytes::from_static(b"second-different-bytes"),
)]));
let outcome2 = b
.publish_exclusive("fid/vid", stream2, None)
.await
.expect("second publish_exclusive call must not error, just report created=false");
assert!(
!outcome2.created,
"a second publish_exclusive call to an already-published path must not overwrite it"
);
assert_eq!(
b.get("fid/vid").await.unwrap(),
Bytes::from_static(b"first"),
"an already-published blob must never be overwritten by publish_exclusive"
);
}Code: - introspect resume URLs capped at min(session expiry, now + url_ttl) so the 24h session TTL no longer produces day-long signed part URLs - shared scope/owner-shape validator: GET /policy now also rejects scope=tenant with scope_owner_id - retention-rule prefetch uses the caller's tenant scope (closes the cross-tenant rule-ID 403-vs-404 oracle; orphaned-rule delete preserved) - idempotent create replay re-mints the upload URL against the current effective max_size instead of replaying the stale stored constraint - list_metadata_for_files narrowed to pub(crate) with an authorization contract note - clarifying comment on the sidecar replay path (finalize re-derives size/hash from stored bytes, reported digest cannot poison metadata) and on the non-atomic publish_exclusive default (S3 override required before the ADR-0005 release gate lifts; noted in ADR-0003 known gap) Docs: aligned remaining stale DESIGN.md sections (sidecar DB edges, TOML refs, in-stream MIME 415, token claim contract, ip/tok.* checklist, "single mode" wording), api.md bind-vs-finalize integrity timing, ADR-0004 stale Tier-4 reference. Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG Signed-off-by: Roland From <rfedorov@linkentools.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gears/file-storage/docs/features/multipart-coordinator.md (1)
221-221: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep shipped-status checkboxes consistent.
Line 221 marks part-row deletion complete while the surrounding abort flow and DoD remain unchecked, despite the acceptance criteria stating that full abort cleanup is shipped. Likewise, lines 476-479 mark resume URLs shipped while the final acceptance item at line 484 remains
[ ]. Align these status markers so the document does not present the same behavior as both planned and complete.Also applies to: 476-479
🤖 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 `@gears/file-storage/docs/features/multipart-coordinator.md` at line 221, Align the shipped-status checkboxes in multipart-coordinator.md: update the part-row deletion item and the resume-URLs items to match the surrounding abort-flow and final acceptance statuses. Ensure each behavior is represented consistently as either shipped or not shipped, without changing the documented acceptance criteria.gears/file-storage/docs/DESIGN.md (1)
1521-1523: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one token serialization contract throughout
DESIGN.md.This section correctly says the shipped token is bespoke and not literal PASETO, but earlier architecture, FR, component, and example text still calls the wire token “PASETO
v4.public.” Align those references; otherwise implementers may emit literal PASETO instead of the shipped compact codec.🤖 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 `@gears/file-storage/docs/DESIGN.md` around lines 1521 - 1523, Update all earlier architecture, functional-requirement, component, and example references in DESIGN.md that describe the wire token as PASETO v4.public. Use the shipped bespoke Ed25519-signed compact serialization and reference infra::signed_url::Issuer::issue and Verifier::verify consistently, while preserving the distinction that it is not literal PASETO.gears/file-storage/file-storage/src/bin/sidecar.rs (1)
168-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon't treat
NotUnicodeas missing
std::env::var(name).ok()also turnsVarError::NotUnicodeintoNone, so a malformedFS_SIDECAR_*value silently falls back to the default instead of failing fast. OnlyNotPresentshould use the default; return an error for any other lookup failure.🤖 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 `@gears/file-storage/file-storage/src/bin/sidecar.rs` around lines 168 - 175, Update parse_env_or_default to distinguish environment lookup errors: use the default only when std::env::var returns VarError::NotPresent, and propagate VarError::NotUnicode as an anyhow error instead of passing None to parse_optional. Preserve the existing parsing and default behavior for valid values and missing variables.
🤖 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 `@gears/file-storage/docs/DESIGN.md`:
- Around line 348-350: Remove hash_policy from the BackendConfig field list in
the design documentation, keeping the existing statement that HashPolicy
configuration is unsupported. Ensure the documented BackendConfig model no
longer exposes an unavailable configuration surface.
- Around line 1885-1888: Update the Phase A multipart completion description to
reflect the shipped composite-manifest behavior, including use of persisted
per-part hashes and offsets rather than re-reading and re-hashing the assembled
object. Remove references claiming completion re-hashes the object or that
offset-manifest mode is only proposed, keeping the Phase B description
consistent.
In `@gears/file-storage/file-storage/src/domain/service/create.rs`:
- Around line 220-238: The replay flow around
PolicyResolver::compute_effective_max_bytes and self.sign_url must perform the
same current storage-quota preflight as fresh creation before re-minting the
upload URL. Reuse the existing quota validation path, return its error when the
quota is exhausted, and only call sign_url after validation succeeds.
---
Outside diff comments:
In `@gears/file-storage/docs/DESIGN.md`:
- Around line 1521-1523: Update all earlier architecture,
functional-requirement, component, and example references in DESIGN.md that
describe the wire token as PASETO v4.public. Use the shipped bespoke
Ed25519-signed compact serialization and reference
infra::signed_url::Issuer::issue and Verifier::verify consistently, while
preserving the distinction that it is not literal PASETO.
In `@gears/file-storage/docs/features/multipart-coordinator.md`:
- Line 221: Align the shipped-status checkboxes in multipart-coordinator.md:
update the part-row deletion item and the resume-URLs items to match the
surrounding abort-flow and final acceptance statuses. Ensure each behavior is
represented consistently as either shipped or not shipped, without changing the
documented acceptance criteria.
In `@gears/file-storage/file-storage/src/bin/sidecar.rs`:
- Around line 168-175: Update parse_env_or_default to distinguish environment
lookup errors: use the default only when std::env::var returns
VarError::NotPresent, and propagate VarError::NotUnicode as an anyhow error
instead of passing None to parse_optional. Preserve the existing parsing and
default behavior for valid values and missing variables.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52f288b5-ffb7-4661-8d06-c959c07a1e41
📒 Files selected for processing (16)
gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.mdgears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.mdgears/file-storage/docs/DESIGN.mdgears/file-storage/docs/api.mdgears/file-storage/docs/features/multipart-coordinator.mdgears/file-storage/file-storage/src/bin/sidecar.rsgears/file-storage/file-storage/src/domain/multipart.rsgears/file-storage/file-storage/src/domain/multipart_service.rsgears/file-storage/file-storage/src/domain/policy_service.rsgears/file-storage/file-storage/src/domain/service/create.rsgears/file-storage/file-storage/src/domain/service/read_ops.rsgears/file-storage/file-storage/src/domain/service/read_ops_tests.rsgears/file-storage/file-storage/src/infra/backend/mod.rsgears/file-storage/file-storage/tests/multipart_test.rsgears/file-storage/file-storage/tests/policy_authz_test.rsgears/file-storage/file-storage/tests/service_test.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- gears/file-storage/docs/ADR/0004-cpt-cf-file-storage-adr-signed-url-transport.md
- gears/file-storage/docs/ADR/0003-cpt-cf-file-storage-adr-sidecar-data-plane.md
- gears/file-storage/file-storage/src/domain/service/read_ops.rs
- gears/file-storage/file-storage/src/infra/backend/mod.rs
- gears/file-storage/file-storage/src/domain/multipart_service.rs
- gears/file-storage/file-storage/src/domain/multipart.rs
- gears/file-storage/docs/api.md
5 tasks
- create_file idempotent replay now runs the quota preflight before re-minting the upload URL (was skipped, unlike the fresh path) - DESIGN.md: drop hash_policy from the BackendConfig field list; update the Phase A multipart example and the multipart_upload_parts row to the shipped composite-manifest behavior (no assembled-object re-read) Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG Signed-off-by: Roland From <rfedorov@linkentools.com>
ffedoroff
force-pushed
the
fix/file-storage-audit-remediation
branch
from
July 16, 2026 21:32
297aa9a to
2d024ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gears/file-storage/docs/DESIGN.md (2)
67-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove the contradictory hash-policy claim.
These lines say the full configurable hash-policy surface is exposed in P1, but line 348 says
HashPolicyis not implemented and SHA-256 is hard-coded. This could cause consumers to depend on an unsupported configuration surface.Proposed wording
- the full configurable hash-policy surface is exposed from P1 with a locked allow-list of ["SHA-256"] + hashing is fixed to SHA-256 in P1/P2; the configurable hash-policy surface is not implemented🤖 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 `@gears/file-storage/docs/DESIGN.md` around lines 67 - 69, Update the hash algorithm description in DESIGN.md to remove the claim that a configurable hash-policy surface is exposed in P1. State only the currently supported behavior: SHA-256 is computed on the sidecar’s streaming upload path and remains hard-coded, consistent with the HashPolicy status described later in the document.
345-350: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse one authoritative token-format description.
The changed sections correctly describe a bespoke Ed25519 compact token that is codec-equivalent to, but not literally, PASETO. Earlier architecture text still calls it a
PASETO v4.publictoken; update that wording to avoid clients implementing the wrong wire format.Also applies to: 460-464
🤖 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 `@gears/file-storage/docs/DESIGN.md` around lines 345 - 350, The architecture document contains earlier references describing SignedUrl as a PASETO v4.public token, conflicting with the authoritative bespoke Ed25519 compact-token definition in the SignedUrl entry. Update those earlier descriptions, including the referenced sections around the token format, to consistently state the custom base64url payload/signature format and clarify it is codec-equivalent to but not literally PASETO.
🤖 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 `@gears/file-storage/docs/DESIGN.md`:
- Around line 1876-1880: The later Phase D finalization description must match
the composite-manifest completion flow: replace references to the assembled
object’s “real size/hash” with the reported total size and composite manifest
root. Retain the existing bounded MIME-sniff read, and update only the relevant
Phase D wording.
---
Outside diff comments:
In `@gears/file-storage/docs/DESIGN.md`:
- Around line 67-69: Update the hash algorithm description in DESIGN.md to
remove the claim that a configurable hash-policy surface is exposed in P1. State
only the currently supported behavior: SHA-256 is computed on the sidecar’s
streaming upload path and remains hard-coded, consistent with the HashPolicy
status described later in the document.
- Around line 345-350: The architecture document contains earlier references
describing SignedUrl as a PASETO v4.public token, conflicting with the
authoritative bespoke Ed25519 compact-token definition in the SignedUrl entry.
Update those earlier descriptions, including the referenced sections around the
token format, to consistently state the custom base64url payload/signature
format and clarify it is codec-equivalent to but not literally PASETO.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55d33589-eebc-4dfe-8210-a9215cd78d42
📒 Files selected for processing (2)
gears/file-storage/docs/DESIGN.mdgears/file-storage/file-storage/src/domain/service/create.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- gears/file-storage/file-storage/src/domain/service/create.rs
…ped behavior Third CodeRabbit pass: the Phase D walkthrough still described completion as finalizing from the assembled object's real size/hash and returning 204 with no body. Corrected to the shipped contract — finalize from the reported total part size and composite manifest root (no re-read beyond the bounded MIME sniff), optional If-Match, 200 with the MultipartCompleteDto JSON body. Claude-Session: https://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG Signed-off-by: Roland From <rfedorov@linkentools.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A doc-vs-implementation audit of
gears/file-storage(all ofdocs/cross-checked against the code) surfaced correctness bugs, security gaps, and systemic documentation drift. This PR remediates them in staged batches, each verified with the full test suite.Data loss / integrity
delete_pending_version: a version finalized between the sweep's list and delete no longer gets destroyed along with its blob.backend.abort_multipart, even when sweep step 1 already reclaimed the version row (previously leaked S3 MPU handles).multipart_upload_partsrows (previously accumulated forever, contradicting the feature doc's checked acceptance criterion).local-fsviahard_link,in-memorycheck-and-insert under one mutex): replaying a still-valid PUT token against a published object returns 409 instead of silently overwriting finalized content — restoring the immutable-blob invariant ADR-0003 promises.DoS / limits
MAX_PART_COUNT = 10_000enforced uniformly incompute_planwith part-size widening (up to 5 GiB) before rejection;declared_size = u64::MAXis rejected without allocation (previously ~84 TBVecreservation → process abort); plans S3 would reject at part 10 001 are no longer minted.multipart_session_ttl_secsconfig (default 24 h) decouples multipart session lifetime from the 15-minute signed-URL TTL, making introspect/resume actually usable for large uploads.Authorization / consistency
allow_all— closes a cross-tenant file-existence oracle (201-vs-404).transfer_ownershipreturns the pre-captured row instead of a post-commit re-read under the stale scope (could 404 after a committed swap).create_filereplay re-validates against the current effective policy.GET /policy?scope=userwithoutscope_owner_id→ 400 (was silent 204).report-partvalidates the 32-byte hash exactly likefinalizedoes.API / infra
file.metadata_updatedevent emitted transactionally with the mutation (was documented, never emitted).start>endignored → 200, strict digits, no+); 416 responses carryAccept-Ranges.validate()covers page-size/TTL cross-field rules;GET /filesreturns realcustom_metadatavia one batched query; missing 400/409 declarations added to OpenAPI routes (docs/api/api.json regenerated).Documentation sync
operations.md,DESIGN.md(token codec/claims, ETag formula, hash pipeline, sidecar MIME),api.md(real sidecar headers with an explicit Planned section, actual contracts and status codes),migration.sql(hash-mode DDL, exact event/operation catalogs), ADR-0003/0005 corrections.X-FS-*headers,ip/tok.*claims,enabled_event_typesgating, multipart part-hash trust gap) explicitly marked as tracked gaps instead of shipped behavior.DEFERRED_ITEMS_PLAN.txt/IMPLEMENTATION_PLAN_TEMP.txtdeleted; the one still-open item (content_idFK deferral) moved toDECOMPOSITION.md.Test plan
cargo test -p cf-gears-file-storage -p cf-gears-file-storage-sdkon this branch alone (without fix(file-storage): Postgres jsonb binding, S3 readiness probe, standalone doc corrections #4219) — 407 passed, 0 failedcargo clippy --all-targetsclean,cargo fmt --checkclean,cfs validate— 0 errorscargo build --bin sidecarcleanhttps://claude.ai/code/session_01WZUBxb7myjUZHMYmbRtJqG
Summary by CodeRabbit
New Features
Bug Fixes
409.416responses now includeAccept-Ranges: bytes.Documentation