Conversation
Attachment metadata written before 4.0 lives in _sync.attachments. When
a document is unmarshalled that metadata is moved onto the document's
global sync data and cleared from the sync data, so a write that
persists the sync xattr without the global xattr destroys it: for a
document attachment migration has not reached yet, that was the only
copy.
Two writes did exactly that, dropping the attachment metadata of any
document still in the pre-4.0 layout:
- ResyncDocument ignored the global xattr returned to it by
MarshalWithXattrs.
- CompactDocChannelHistory marshalled the sync data on its own and wrote
only the sync and _mou xattrs. Reachable through
POST /{keyspace}/_channel_history/{docid}/compact.
Both now write the global xattr alongside the sync xattr.
- TestWritePathsPreserveUnmigratedAttachmentMetadata covers every path
that rewrites a document's sync xattr
Contributor
|
Droid finished @torcolvin's task —— View job Droid review (Phase 2 validation) complete. No candidate inline comments to validate for this PR. Overall, the change looks correct: metadata-only write paths now persist _globalSync alongside _sync, and tests cover the critical unmigrated attachment-metadata rewrite scenarios. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-loss risk for pre-4.0 attachment metadata by ensuring any code path that rewrites a document’s _sync xattr also persists the _globalSync xattr (where attachment metadata is moved during unmarshal).
Changes:
- Ensure
ResyncDocumentwrites_globalSyncalongside_sync/_mouon metadata-only writes. - Ensure
CompactDocChannelHistorywrites_globalSyncalongside_sync/_mou. - Add targeted tests to cover tombstone import behavior and to assert all
_syncrewrite paths preserve unmigrated attachment metadata.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
db/document.go |
Factors global-xattr marshalling into marshalGlobalXattr and centralizes attachment-aware gating. |
db/database.go |
Updates ResyncDocument to include _globalSync in the xattrs written. |
db/crud.go |
Updates channel history compaction to also persist _globalSync. |
db/attachment_test.go |
Adds coverage to assert all _sync rewrite paths preserve unmigrated attachment metadata. |
db/import_test.go |
Adds tests pinning import behavior for SDK tombstones and legacy attachment metadata layouts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
1996
to
2000
| updatedDoc := sgbucket.UpdatedDoc{ | ||
| Doc: nil, // Resync does not require document body update | ||
| Xattrs: map[string][]byte{ | ||
| base.SyncXattrName: rawSyncXattr, | ||
| base.MouXattrName: rawMouXattr, | ||
| }, | ||
| Doc: nil, // Resync does not require document body update | ||
| Xattrs: xattrs, | ||
| Expiry: updatedExpiry, | ||
| Spec: []sgbucket.MacroExpansionSpec{ |
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
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.
Attachment metadata written before 4.0 lives in _sync.attachments. When a document is unmarshalled that metadata is moved onto the document's global sync data and cleared from the sync data, so a write that persists the sync xattr without the global xattr destroys it: for a document attachment migration has not reached yet, that was the only copy.
Two writes did exactly that, dropping the attachment metadata of any document still in the pre-4.0 layout:
Both now write the global xattr alongside the sync xattr.
Pre-review checklist
base.UD(docID),base.MD(dbName))docs/apiIntegration Tests