Process master entry on ObjectRestore:Retry#2768
Draft
maeldonn wants to merge 1 commit into
Draft
Conversation
The populator's master-skip rule was designed for the normal restore flow. On that flow, the populator receives events on both the master and the version entry, so skipping the master avoids producing two `cold-restore-req` messages for the same object. The retrigger restore flow only touches the master entry though, so the master oplog event is the only event we will ever get. For null-version objects (master with `isNull: true`), the existing skip rule silently dropped that event and no restore request reached DMF. Exempt `s3:ObjectRestore:Retry` from the skip rule. The master event is the only one to come, so we must produce on it. Normal restore (`s3:ObjectRestore:Post`) is unchanged. Issue: BB-794
Contributor
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 3 files with indirect coverage changes
@@ Coverage Diff @@
## development/9.1 #2768 +/- ##
===================================================
- Coverage 75.02% 74.79% -0.23%
===================================================
Files 200 200
Lines 13541 13541
===================================================
- Hits 10159 10128 -31
- Misses 3372 3403 +31
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
maeldonn
changed the title
francoisferrand
requested changes
Jun 24, 2026
| // the non-master entry will be processed. The retrigger restore flow | ||
| // (originOp 's3:ObjectRestore:Retry') only updates the master entry, so | ||
| // the master event is the only one we will ever get — do not skip it. | ||
| if (this._isVersionedObject(value) && isMasterKey(entry.key) && operation !== 's3:ObjectRestore:Retry') { |
Contributor
There was a problem hiding this comment.
Introducing this asymmetry seems very weird : AFAIK we are able to restore both "master-only" null versions (i.e. object created before versioning was enabled) and "version-suspended" versions : so _isVersionedObject() seems to handle its job, and prevent duplicate handling of restore while still processing master document when needed.
Generally, lifecycle (cold transition & restore) work for these objects (tested in zenko I think, to be confirmed) : so we need to understand exactly what Sorbet passes in message, so we can properly "forge" the message (+maybe add missing checks to ensure a "bad" message does not make things worse)
"only updates the master entry"
- if there is only a master, should be covered (already) by
_isVersionedObject? - if there is both a master & a version documents, somehow the code ends up creating a desynchro between master & version : which is a separate but critical issue
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
3 participants
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.
For null-version objects (master entry with versionId and isNull: true, typical of buckets that have had versioning Suspended at any point), a requeueRestore Kafka message without objectVersion updates the master metadata as expected but never produces a cold-restore-req message: the lifecycle queue populator's master-skip rule drops the resulting oplog event, and the retry never reaches DMF.
The skip rule was designed for the normal restore flow (s3:ObjectRestore:Post), where the populator receives events on both the master and the version entry and must dedup. The retrigger restore flow (s3:ObjectRestore:Retry) only updates the master entry, so the master event is the only event the populator will ever get for that retry.
This PR exempts s3:ObjectRestore:Retry from the master-skip rule. The normal s3:ObjectRestore:Post path is unchanged.
The current workaround is to pass the encoded internal VersionId returned by head-object in objectVersion, which routes the update through the version-entry path and avoids the skip rule. That requires the caller to know an internal versionId on objects that are otherwise non-versioned from the API's perspective. With this fix, omitting objectVersion works as expected on null-version objects, matching what the S3 API surface implies.
Issue: BB-794