feat(cogs): report backend changes to a change stream - #595
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## matth/storage-inventory-tracker-2 #595 +/- ##
=====================================================================
+ Coverage 87.97% 88.39% +0.41%
=====================================================================
Files 104 104
Lines 16776 17118 +342
=====================================================================
+ Hits 14759 15131 +372
+ Misses 2017 1987 -30
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ab70a01 to
959eb00
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
959eb00 to
6d8e991
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
6d8e991 to
28100c2
Compare
| /// | ||
| /// This function does not distinguish between object rows and tombstone rows. It does not | ||
| /// include Bigtable's own overhead. | ||
| fn row_bytes(path: &[u8], mutations: &[v2::Mutation]) -> u64 { |
There was a problem hiding this comment.
nit: Let's use length or size since bytes is commonly used to refer to the actual data.
| // Inline `put_row()` because we need the mutations to compute their size. | ||
| let mutations = object_mutations(metadata.clone(), payload.into_bytes().into())?; | ||
| self.mutate(path.clone(), mutations.clone(), "put").await?; | ||
| self.report_write(id, &path, &mutations, metadata.time_expires); |
There was a problem hiding this comment.
You can make object_mutations instead return the size alongside the mutations, return that from put_row, and then use the returned value to report the write. This yields all data in the correct sequence, removes the inlining, and mutations no longer have to be cloned.
| // The payload arrives as a stream with no declared length, so the stored size is | ||
| // only known once the upload has drained. Safe from double-counting because this | ||
| // request is not retried. | ||
| let (stored_size, stream) = counting_stream(stream); |
There was a problem hiding this comment.
Please double-check, but GCS should be returning a X-Goog-Stored-Content-Length from the upload request which we can use. Even better, this is authoritative.
Just know that this doesn't include metadata, and GCS charges for metadata as bytes, too.
There was a problem hiding this comment.
TODO also count metadata
28100c2 to
836790e
Compare
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.
Reviewed by Cursor Bugbot for commit 836790e. Configure here.
|
|
||
| if deleted { | ||
| self.change_stream.delete(id); | ||
| } |
There was a problem hiding this comment.
Missed delete after retry 404
Medium Severity
delete_object only emits a change-stream delete when the retry closure returns true. If GCS applied the delete but the client saw a timeout or other retryable failure, the retry gets 404, returns false, and the delete is never reported. Inventory then keeps a ghost object and overstates storage COGS until expiry (permanently for manual retention).
Reviewed by Cursor Bugbot for commit 836790e. Configure here.
| self.mutate(path, [delete_row_mutation()], "delete").await?; | ||
| self.change_stream.delete(id); |
There was a problem hiding this comment.
Bug: The Bigtable delete_object function unconditionally reports a delete event to the change stream, even if the object does not exist.
Severity: MEDIUM
Suggested Fix
The delete_object function should only report a delete event if the object actually existed and was deleted. Since Bigtable's mutate with DeleteFromRow doesn't indicate if a row was affected, the implementation should first check for the object's existence before attempting the deletion. The GCS backend implementation can be used as a reference, as it checks for a successful deletion before calling self.change_stream.delete(id).
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/bigtable.rs#L1033-L1034
Potential issue: The `delete_object` function for the Bigtable backend unconditionally
reports a delete event to the change stream, even when the object being deleted does not
exist. This is because the underlying Bigtable `MutateRow` operation with
`DeleteFromRow` is idempotent and succeeds on non-existent rows, causing the subsequent
`self.change_stream.delete(id)` call to always execute. This behavior is inconsistent
with other backends like GCS, which verify that an object was actually deleted before
reporting it. This can lead to spurious delete records in the change stream inventory,
resulting in incorrect cost accounting.
2 participants
rig up GCS and Bigtable backends to emit change stream records. nothing is actually plugging in a change stream yet, that'll be future PRs
didn't do other backends yet because i wanted feedback on the stack so far before writing that much more code haha