Conversation
Route resumable sessions to long-term storage and publish completed revisions through high-volume tombstones.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #642 +/- ##
==========================================
+ Coverage 91.27% 91.31% +0.04%
==========================================
Files 116 116
Lines 23496 23625 +129
==========================================
+ Hits 21445 21574 +129
Misses 2051 2051
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lcian
changed the title
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 09aadd1. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Route resumable sessions only to long-term storage, where uploads can be resumed. Update the existing tiered resumable tests to use long-term sizes and document the cutoff.
Store the long-term revision and backend token directly in the tiered token. Decode its authenticated payload without redundant key-shape validation.
| struct TieredResumableToken { | ||
| inner: LongTermBackendToken, | ||
| revision: String, | ||
| total_length: u64, |
There was a problem hiding this comment.
We now have the total length at this layer and inside the wrapped long term backend token, at least in the GCS case. In order to fulfill the contract, we will always need the length. Do you see a way to hoist the length up or move it out of the encoded string so that it's readable by every layer?
There was a problem hiding this comment.
Either
- move this one layer up and pass it as argument
- make each token an object and Box it and have some traits to access the length from the inner token
There was a problem hiding this comment.
It's easy to do it with moving it one layer up.
I'll do that in a follow-up as that likely needs to touch many files/call sites.
| return match progress { | ||
| UploadProgress::Incomplete { .. } => Ok(progress), | ||
| UploadProgress::Complete => Ok(UploadProgress::Complete), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Bug: When a non-final chunk upload returns UploadProgress::Complete, the code propagates this status without publishing a tombstone, making the object inaccessible.
Severity: LOW
Suggested Fix
When handling a non-final chunk, if the underlying put_chunk call returns UploadProgress::Complete, do not simply propagate the Complete status. Instead, ensure a tombstone is published for the object to make it accessible, or handle this state as a distinct case rather than a successful completion of the chunk.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: objectstore-service/src/backend/tiered.rs#L479-L483
Potential issue: In the tiered storage backend, when a non-final chunk of a multi-part
upload is processed, the code forwards it to the long-term storage. If the long-term
storage reports that the upload is already `Complete` (e.g., due to a race condition or
a client retry after completion), the `put_chunk` function in `tiered.rs` incorrectly
returns `UploadProgress::Complete`. However, it fails to publish a 'tombstone' record,
which is required to finalize the object's state in the tiered system. This oversight
leads to the object being successfully stored but remaining inaccessible to clients. The
bug is an edge case that requires unusual client behavior or specific network timing.
Also affects:
objectstore-service/src/backend/tiered.rs:535~543
3 participants
This implements Resumable Uploads in
TieredStorage.Notes:
BACKEND_SIZE_THRESHOLD, smaller ones are rejected at creation time. There was an idea to still accept the upload creation request and then force the client to upload the whole payload in a single shot by always reporting offset 0, but that cannot be achieved unless we always keep some state about which uploads exist/are active at a given moment. So, we might as well reject the creation request and then the user is still practically forced to use a regularputand upload in a single shot.If we change the way we do routing, this can trivially be adapted.
Refs FS-508