CBG-5038 allow metadata id to be nil while syncinfo exists#8059
CBG-5038 allow metadata id to be nil while syncinfo exists#8059
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds commented-out code to reproduce a timing-related test failure associated with CBG-5038. The changes prepare tests to verify fixes for attachment migration race conditions without breaking current test execution.
Changes:
- Added stub code in two test functions to reproduce timing issues related to attachment migration
Fix a gap where setting up a non persistent configuration database and running attachment migrration would cause the next start of the database to require resync. attachment migration will write metadata_version and will preserve metadataID With non persistent config, metadataID is not written, but running attachment migration will write a metadata_version. Turn metadataID into an optional value.
torcolvin
force-pushed
the
CBG-5038-test-repro
branch
from
2979928 to
bdad9d1
Compare
torcolvin
changed the title
| requiresAttachmentMigration, err = CompareMetadataVersion(ctx, syncInfo.MetaDataVersion) | ||
| if err != nil { | ||
| return syncInfo.MetadataID != metadataID, true, err | ||
| return *syncInfo.MetadataID != metadataID, true, err |
There was a problem hiding this comment.
In InitSyncInfo, the error path after CompareMetadataVersion dereferences syncInfo.MetadataID (*syncInfo.MetadataID) even though syncInfo.MetadataID can be nil (e.g., when the document was just added and syncInfo wasn't refreshed, or after a CAS mismatch refetch where the stored syncInfo omits metadataID). This can panic during error handling. Compute requiresResync with a nil check (or use metadataID/newSyncInfo in the add-success path) and avoid dereferencing nil pointers here.
|
|
||
| if exists && syncInfo.MetadataID != defaultMetadataID { | ||
| if exists && (syncInfo.MetadataID != nil && *syncInfo.MetadataID != defaultMetadataID) { | ||
| base.InfofCtx(ctx, base.KeyConfig, "Using metadata ID %q for db %q because db uses the default collection, and _sync:syncInfo in the default collection specifies the non-default metadata ID %q", base.MD(standardMetadataID), base.MD(config.Name), base.MD(syncInfo.MetadataID)) |
There was a problem hiding this comment.
This log argument uses base.MD(syncInfo.MetadataID), but MetadataID is now a *string. base.MD will fall back to fmt.Sprintf for *string and log the pointer address rather than the metadata ID value. Log the dereferenced value (with a nil guard) so the message reports the actual metadata ID.
| base.InfofCtx(ctx, base.KeyConfig, "Using metadata ID %q for db %q because db uses the default collection, and _sync:syncInfo in the default collection specifies the non-default metadata ID %q", base.MD(standardMetadataID), base.MD(config.Name), base.MD(syncInfo.MetadataID)) | |
| base.InfofCtx(ctx, base.KeyConfig, "Using metadata ID %q for db %q because db uses the default collection, and _sync:syncInfo in the default collection specifies the non-default metadata ID %q", base.MD(standardMetadataID), base.MD(config.Name), base.MD(*syncInfo.MetadataID)) |
gregns1
left a comment
gregns1
left a comment
There was a problem hiding this comment.
Generally looks good to me just one minor test nit
Co-authored-by: Gregory Newman-Smith <109068393+gregns1@users.noreply.github.com>
CBG-5038 allow metadata id to be nil while syncinfo exists
Fix a gap where setting up a non persistent configuration database and running attachment migration would cause the next start of the database to require resync.
attachment migration will write metadata_version and will preserve metadataID, but the metadata id might not exist if non persistent config is used.
With non persistent config, metadataID is not written, but running attachment migration will write a metadata_version.
Turn metadataID into an optional value.