ARSN-620 Fix lifecycle listings stuck on PHD master keys - #2685
ARSN-620 Fix lifecycle listings stuck on PHD master keys#2685nicolas2bert wants to merge 1 commit into
Conversation
Hello nicolas2bert,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
| * @return {number} - filter return value | ||
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| handlePHDMaster(key, versionId, value) { |
There was a problem hiding this comment.
I don't understand the reason to pass versionId, I believe PHDs always has an undefined version ID here as their key doesn't have a version ID embedded (there's one within the metadata for internal purposes but it's not relevant for listing purposes).
Also, for a similar reason I think we don't need to pass value which is only internal to metadata, so we should be good with passing just the key.
| } else if (!this.nextKeyMarker) { | ||
| // No previous PHD, and no marker yet. Skip this key's versions | ||
| // rather than leave the listing unable to move forward. | ||
| this.nextKeyMarker = key; | ||
| this.nextVersionIdMarker = undefined; | ||
| } |
There was a problem hiding this comment.
I'm not sure we should keep this second condition block, which seems to add the possibility to skip legit noncurrent versions (i.e. keeps a buggy behavior). My reasoning is, if we're there it means:
addVersionwasn't called yet since it would have setnextKeyMarker- hence either it's the first PHD
- then no risk of having a stuck listing at this point
- or all we have seen so far are PHDs
- then
prevPHDKeyhas already been set earlier, and likelynextKeyMarkeras well to the previous PHD key, starting from the 2nd PHD seen, so no risk of stuck listing either
- then
So removing this block may remove the remaining buggy situation as nextKeyMarker will correctly be set already to the previous PHD.
Not 100% sure, please double check my reasoning.
| * a key-marker with no version-id-marker. S3 reads that as "start after | ||
| * every version of this key". The listing would then skip the versions of a |
There was a problem hiding this comment.
This comment looks suspicious to me: "S3 reads that as ...":
- on one hand, "S3" is too generic and doesn't help the understanding, we could say "the listing algorithm" for example. If S3 refers to Cloudserver, I believe it just passes the received marker values as is to the next call, without further interpretation.
- on the other hand, I believe (and hope) it's not true:
DelimiterVersionswill scan all versions starting atNextKeyMarkerifNextVersionIdMarkeris not present (but it will skip the master).
I believe a more correct comment should say that the listing would skip the "master version of" and leave it unable to recognize that the upcoming version is the new current version. But it should still see it.
| * page 2: phd-3, phd-4, phd-5 -> truncated, NextKeyMarker=phd-4 | ||
| * page 3: phd-5, phd-6 -> done | ||
| * | ||
| * THE FALLBACK: on the first PHD of a listing there is no previous PHD key, |
There was a problem hiding this comment.
cf my comment, I believe the fallback should be removed.
| * apple\0v1 -> first version seen for apple -> current, protected | ||
| * apple\0v2 -> noncurrent -> expirable, staleDate = v1's date | ||
| * | ||
| * apple\0v1 is not deduplicated as the master copy: a PHD gets its |
There was a problem hiding this comment.
| * apple\0v1 is not deduplicated as the master copy: a PHD gets its | |
| * apple\0v1 is not duplicated as the master copy: a PHD gets its |
| if (key !== this.keyName) { | ||
| if (this.value) { | ||
| this._addOrphan(); | ||
| } | ||
| this.prevKeyName = this.keyName; | ||
| this.keyName = key; | ||
| this.value = null; | ||
| } | ||
| return FILTER_ACCEPT; |
There was a problem hiding this comment.
maybe simpler:
| if (key !== this.keyName) { | |
| if (this.value) { | |
| this._addOrphan(); | |
| } | |
| this.prevKeyName = this.keyName; | |
| this.keyName = key; | |
| this.value = null; | |
| } | |
| return FILTER_ACCEPT; | |
| this.addVersion(key, null, null); | |
| return FILTER_ACCEPT; |
Should be equivalent since PHDs always have a different key than the previous entry.
3 participants
Problem
Lifecycle listings can get stuck forever on PHD master keys.
A PHD (placeholder) master is written when the current version of a key is deleted. A repair job later promotes the newest surviving version back to master. Until the repair runs, the master key holds a PHD value. If the repair never runs, or if no version survives, the PHD stays.
DelimiterVersions accepted a PHD master and skipped it without touching the resume marker. That is fine for a plain version listing, which has no scan budget. It is not fine for the two lifecycle listings, DelimiterNonCurrent and DelimiterOrphanDeleteMarker. Both count every scanned entry against maxScannedLifecycleListingEntries and truncate when the budget runs out.
So a run of PHD masters longer than the scan limit burns the whole budget without moving the marker. The listing returns IsTruncated: true with no NextKeyMarker. The next listing starts at the same place and returns the same thing. Lifecycle never gets past that point, and nothing beyond it is ever expired.
Example
Keyspace (v0), scan limit 3:
phd-1 .. phd-6 dangling PHD masters
zebra\0v1 current
zebra\0v2 noncurrent -> must be expired
Before:
page 1: scans phd-1, phd-2, phd-3 -> truncated, no NextKeyMarker
page 2: identical request -> identical result
...forever. zebra\0v2 is never expired.
After:
page 1: phd-1, phd-2, phd-3 -> truncated, NextKeyMarker=phd-2
page 2: phd-3, phd-4, phd-5 -> truncated, NextKeyMarker=phd-4
page 3: phd-5, phd-6, zebra\0v1, zebra\0v2 -> zebra\0v2 listed
What changed
DelimiterVersions — new handlePHDMaster hook
Both PHD branches (keyHandler_NotSkippingV0 and keyHandler_NotSkippingV1) now call handlePHDMaster(key, versionId, value) instead of returning FILTER_ACCEPT directly.
The default implementation returns FILTER_ACCEPT and changes no state. Plain version listings behave exactly as before.
DelimiterOrphanDeleteMarker — treat a PHD as a key transition
The override runs the same new-key branch as addVersion():
The second point matters. A delete marker is held in memory until an entry of another key proves it is an orphan. A PHD master is such an entry. If the marker moved past the candidate without emitting it, the candidate would never be scanned again, and the orphan delete marker would never expire.
scan limit 3, keyspace: banana\0v1 (delete marker), phd-1, phd-2, ...
banana\0v1 -> held as candidate
phd-1 -> new key: banana is proven orphan -> emitted, then marker moves
DelimiterNonCurrent — keep the marker one PHD key behind
The override sets nextKeyMarker to the previous PHD key, never to the key being scanned, and clears nextVersionIdMarker. The marker only moves forward.
A marker on the key being scanned would give the next listing a key-marker with no version-id-marker. S3 reads that as "start after every version of this key". The versions of a PHD master that still has some would be skipped, and they would never be expired. One key behind costs one re-scanned entry per truncation, and skips nothing.
The override leaves prevKey and staleDate untouched. This is deliberate. The next version key scanned is the newest surviving version under the PHD — the one the repair promotes back to master. It must stay classified as current:
apple (PHD) -> marker left behind apple, prevKey untouched
apple\0v1 -> first version seen for apple -> current, protected
apple\0v2 -> noncurrent -> expirable, staleDate = v1's date
Setting prevKey = 'apple' would make apple\0v1 look noncurrent. Noncurrent version expiration would then delete the very version the repair needs to promote. That is data loss.
One fallback remains: if the scan limit lands on the first PHD of the listing, there is no previous PHD and no marker yet. The marker then points at that key, and that one key loses its versions for this pass. Progress matters more than one missed pass, and the alternative is the infinite loop described above.
Tests
All cases run against both v0 and v1 bucket formats.