a missing manifest or unreadable chunk on the upload walk is a loss, not a refusal - #20
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: LucaCappelletti94/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @LucaCappelletti94, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 8 hours and 45 minutes by commenting @sourcery-ai review. Upgrade to get a review now.
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. |
Reviewer's GuideThe shared upload and worker paths now distinguish retryable conditions, server refusals, and device-side data loss: unavailable storage remains deferred, refusals stay marked for later retry, and missing manifests or definitively unreadable chunks are retired and reported as lost, with tests confirming consistent behavior across upload and boot integrity paths. Sequence diagram for unreadable chunk handlingsequenceDiagram
participant Walk as UploadWalk
participant Store as ChunkStore
participant Error as ContentError
participant DB as OutboxDB
participant App as Application
Walk->>Store: read_chunk(hash)
alt store unavailable
Store-->>Walk: ambiguous error
Walk->>Error: outcome()
Error-->>Walk: Retry
Walk->>App: UploadDeferred
else chunk definitively unreadable
Store-->>Walk: permanent read error
Walk->>Error: outcome()
Error-->>Walk: Lost
Walk->>DB: retire(file_id)
Walk->>App: BytesLost
end
Flow diagram for upload failure classificationflowchart TD
A[Upload attempt fails] --> B{"ContentError.outcome()"}
B -->|Retry| C[Keep outbox entry]
C --> D[Emit UploadDeferred]
B -->|Refused| E[Mark entry refused]
E --> F[Emit UploadRefused]
B -->|Lost| G{"unreadable_chunks(file_id)"}
G -->|None| C
G -->|Some unreadable data| H[Retire outbox entry]
H --> I[Emit BytesLost]
Flow diagram for unified loss handling across upload and boot passesflowchart LR
A[Missing manifest] --> B[AttemptOutcome::Lost]
B --> C[Upload walk]
B --> D[Boot integrity scan]
C --> E["retire(file_id)"]
D --> E
E --> F[Bytes are gone]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
![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: 748578d127
ℹ️ 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".
| AttemptOutcome::Lost => { | ||
| retire(connection.conn(), file_id)?; |
There was a problem hiding this comment.
When ContentUpload::transfer returns LostChunk in the browser relay, finish_content_upload may already have serviced a concurrent HubEvent::Import and restored that chunk before this bookkeeping runs. This branch nevertheless dequeues the file and records it as retired without the fresh store read performed by ContentClient::flush_outbox and the integrity scan, permanently preventing a now-valid outbox entry from uploading. Recheck the current manifest and chunks before retiring this outcome.
Useful? React with 👍 / 👎.
1 participant
A permanent upload failure used to have a single fate. Whatever the cause, the outbox entry was marked refused and left in place, so a file whose bytes this device no longer holds would sit unsent forever, retried on request only to fail the same way. The boot integrity pass already knew better and retired a file whose manifest had gone, so the two passes disagreed about the very same fact.
This gives the upload walk the rule we settled. A permanent outcome is now one of two events, chosen by where the fact came from. A refusal is an answer from the server, an over-budget or malformed request or a reply this client cannot read, and it keeps the entry marked so a later budget or quota change can still let the same upload through, and the application is told the upload was refused. A loss is a fact about this device, a manifest the outbox no longer holds or a chunk the store cannot read, and it retires the entry the way the boot pass does and tells the application the bytes are gone. A store that is merely unavailable stays ambiguous and keeps the entry, so being offline is never mistaken for a loss.
The change lives entirely in shared code, so the native client and the browser worker inherit the one fix, and the event an application receives for a given fact is the same whichever pass reaches it first. New tests cover the classification itself, the agreement between the two passes on a missing manifest, an unreadable chunk met on the walk, and that a genuine server refusal still keeps its entry in the outbox.
Summary by Sourcery
Distinguish upload refusals from local data loss so the outbox preserves recoverable failures and retires bytes that are gone.
Bug Fixes:
Enhancements:
Tests: