Rescue partially-aborted sweeps instead of re-executing them destructively - #746
Merged
Conversation
…ively A transient exception thrown in the middle of a garbage collection sweep (realistically: a faulty custom object-registry predicate or an OutOfMemoryError) left the sweep flagged for re-execution while the aborted attempt had already reset an unknown prefix of the surviving (marked) entities back to white. Re-executing the flagged sweep with normal delete semantics then read those entities as garbage and deleted reachable data: dangling references on disk, zombie object ids at the next mark, "No entity found for objectId" after restart. Reachable vectors are the task paths that keep the channel alive after a failed issued GC (chain repair) and the import quiesce loop. The sweep now flags its channel on any mid-run throw, and the next execution of the flagged sweep runs as a keep-all rescue pass: every entity is reset to white, nothing is deleted, and no application predicate is consulted - making the rescue exception-free by construction. The sweep completes consistently through the regular bookkeeping tail, a WARN log documents the rescue, and the following mark cycle re-establishes all marks from the roots and the registry seed; garbage collection is merely deferred by one cycle. Adds the deterministic reproducer from the issue as a regression test: a victim reachable only through persisted binary references (root -> Lazy -> victim) is whitened by the aborted sweep attempt and was deleted by the re-executed sweep before this fix.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a GC corruption hazard where a sweep that aborts mid-run can be re-executed with normal delete semantics after some reachable entities have already been whitened, causing irreversible deletion of reachable data. The change introduces a channel-local “rescue sweep” mode that completes the flagged sweep without deleting anything, and adds an integration regression test reproducing the failure.
Changes:
- Track mid-sweep aborts per channel and trigger a keep-all “rescue sweep” on the next pending sweep execution.
- Refactor sweep completion bookkeeping so both normal and rescue sweeps share the same completion path.
- Add
PartialSweepRetryReproTestto validate that a transient mid-sweep failure does not delete reachable entities on subsequent GC.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageEntityCache.java | Adds sweepRescueNeeded and implements a keep-all rescue sweep to safely complete aborted sweeps. |
| integration-tests/src/test/java/test/eclipse/store/gc/PartialSweepRetryReproTest.java | New regression test reproducing the partial-sweep retry data-loss scenario and asserting integrity after restart. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- PartialSweepRetryReproTest: repair the double-encoded punctuation in the javadoc (em dashes and arrows had turned into mojibake) and normalize to plain ASCII. - rescueSweep javadoc: the "exception-free by construction" claim only holds for the sweep pass itself (straight-line pointer operations); the completion bookkeeping can still throw through the mark monitor's registry interaction. Reworded to state that precisely - in that case the flag stays set and the rescue re-runs on the next attempt, still without deleting anything. No behavioral change; reproducer and control test stay green.
- The rescue flag is set for ANY failure before the sweep flag is cleared - mid-iteration or in the completion bookkeeping - not only for a mid-run abort. The field comment, the catch comment, the rescueSweep javadoc and the operator-facing WARN message now state the actual trigger ("failed before completing") instead of the narrower "aborted mid-run".
- The control test now asserts that the detached orphan was actually collected, so it validates collection behavior instead of only the victim's survival. The check runs against the RUNNING storage, where the entity cache is authoritative - after a restart the startup scanner may resurrect logically-deleted-but-not-yet- compacted entities, which would make a post-restart check nondeterministic.
No behavioral change in production code; reproducer and control test
green 3x, gc + zombie battery 89 green.
zdenek-jonas
approved these changes
Jul 9, 2026
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.
Fixes the partial-sweep re-execution hazard
Problem
The GC sweep whitens surviving (marked) entities as it goes and clears the channel's sweep flag only at the very end. A TRANSIENT exception thrown partway through the sweep - realistically a faulty custom object-registry predicate consulted for white entities, or an OutOfMemoryError - leaves the sweep flagged for re-execution while an unknown prefix of the reachable entities has already been reset to white. Re-executing that flagged sweep with normal delete semantics reads those entities as garbage and deletes reachable data: dangling references on disk, zombie object ids at the next mark, and
No entity found for objectIdafter restart. Permanent once file cleanup compacts the region.Reachable re-execution vectors are the task paths that keep the channel alive after a failed issued GC (the #734 chain repair) and the import quiesce loop (#738); the plain housekeeping path fail-stops via channel disruption and is not affected.
Fix
StorageEntityCache.Default, channel-local, no interface changes:sweep(_longPredicate)flags its channel (sweepRescueNeeded) on any mid-run throw and propagates the failure as before.completeSweep), clearing the wedged monitor state, and logs a WARN describing the rescue.If the rescue's own completion tail throws (e.g. a custom registry failing again in the post-sweep seed), the flag stays set and the rescue simply re-runs on the next attempt - still without deleting anything.
Test
PartialSweepRetryReproTest— the reproducer from the issue, adopted verbatim as a regression test. It stages a victim reachable ONLY through persisted binary references (root →Lazy→ victim; the Java instances are collected and the registry entry reaped, so no application-side safety net can rescue it), injects a one-shot transient failure through the sweep-time registry predicate (which fires strictly after the victim was whitened), and re-executes via a second issued full GC. Pre-fix: the victim is deleted, its oid surfaces as a zombie, restart fails withProblem in channel #0. Post-fix: no zombies, graph intact after restart. A control run without the injected failure pins that the orphan (actual garbage) is still collected normally.RED confirmed against unmodified main; GREEN 3× with the fix; gc + zombie + danglingref integration battery 106 green; storage module build green.