Skip to content

feat: tag batch commit so the reconciler reaps abandoned batch payloads - #2518

Merged
taddes merged 2 commits into
masterfrom
feat/tag-batch-commit-txn-STOR-668
Aug 25, 2026
Merged

feat: tag batch commit so the reconciler reaps abandoned batch payloads#2518
taddes merged 2 commits into
masterfrom
feat/tag-batch-commit-txn-STOR-668

Conversation

@taddes

@taddes taddes commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Follow up to STOR-657. The reconciler was skipping the delete for every batch_bsos row removal, which fixed the commit handoff bug but leaked the GCS object whenever a batch was genuinely removed, either by TTL expiry or by deleting a user's storage or a collection.

This tags the batch commit transaction in the Spanner commit_batch path with a batch_commit transaction tag. Spanner records that tag on the change stream for the whole transaction, including the cascade batch_bsos deletes. The reconciler now skips a batch_bsos removal only when it carries that tag, and treats any other batch_bsos removal as a real delete, so TTL expiry and storage or collection deletes now clean up their objects.

The publishers already carry transactionTag on the wire from STOR-657, so no publisher change is needed here. The tag path cannot be exercised on the Spanner emulator, so the Rust change is covered by compile and review and the reconciler logic by unit tests.

Closes STOR-668

@chenba

chenba commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

"To enable tagging on a transaction, transactionTag must be set to the same value for all requests belonging to the same transaction, including sessions.beginTransaction." (from https://docs.cloud.google.com/spanner/docs/reference/rest/v1/RequestOptions) is a little confusing. But it sounds like we need to set it for every single request.

.await?
.params(sqlparams)
.param_types(sqlparam_types)
.transaction_tag(BATCH_COMMIT_TRANSACTION_TAG)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can tag at statement level then this is not the right place since the upsert doesn't delete anything.

@taddes
taddes force-pushed the feat/tag-batch-commit-txn-STOR-668 branch from 1608c10 to 8d9cc8a Compare August 14, 2026 21:40
@taddes
taddes requested a review from chenba August 15, 2026 00:27
@chenba

chenba commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

sounds like we need to set it for every single request

@taddes I don't think the latest commit is doing that with the transaction tag.

@pjenvey pjenvey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good,

I suppose another way of wiring the transaction_tag down to Db would be include it in the lock_for_xxx method's params::LockCollection (those methods already take &mut self).

Alternatively you could also throw something in HttpRequest::extensions_mut to pass the tag over to DbPool, but with_transaction_tag is more explicit anyway.

@taddes
taddes merged commit 6417c0b into master Aug 25, 2026
31 checks passed
@taddes
taddes deleted the feat/tag-batch-commit-txn-STOR-668 branch August 25, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants