Close the multi-channel GC data-loss windows in the storage garbage collector - #755
Merged
Conversation
…ollector
At channelCount >= 2 on the housekeeping GC path a load could straddle a sweep wave and leave the loaded graph partially collected: the parent X handed out to the application while a cross-channel child (e.g. an unloaded Lazy's target Y) was swept, dangling X's persisted reference ("No entity found for objectId Y", made durable by a re-store of X). This is the multi-channel continuation of the single-channel mid-cycle registration race (store#736, GC.md §10.4). Two distinct windows produce that end state; this change closes both.
Window A ("variant 2") - an in-flight load is invisible to sweep initiation.
The per-channel pendingLoad signal is raised only around a channel's own collect and skipped entirely for a channel whose oid subset of the load is empty (if(!loadOids.isEmpty())). So a load already in flight does not stop a sibling channel from initiating a wave: the empty-subset channel sees pendingLoadCount==0 in isMarkingComplete() and sweeps while another channel is still about to serve the load. Fixed with a task-scoped pending-load gate: StorageEntityMarkMonitor gains signalPendingLoadTask()/clearPendingLoadTask() and pendingLoadTaskCount (joined into isMarkingComplete()); StorageTaskBroker signals it once at load-task enqueue - before any channel is notified - and StorageChannelTask clears it once the task completed on all channels (new onLastCompletion() hook, fired from incrementCompletionProgress(), also on the exceptional path). So no wave can initiate while any load task is in flight anywhere, and the existing load-side marking gray-enqueues (walks references transitively) rather than shallow black-marking. The broker reaches the shared mark monitor lazily via StorageSystem (new entityMarkMonitor(), backed by StorageChannel#markMonitor()/StorageEntityCache#markMonitor()), since the monitor does not exist yet when the broker is constructed. No wave-state changes.
Window B (the §10.4 residual) - a registration lands after sweep initiation.
A wave initiates with no load in flight and sweeps channel by channel; a load reheating X (registering X and its unloaded Lazy L, never Y) in the gap between the wave's seed snapshot and a channel's sweep let the shallow id-only predicate rescue X+L while a sibling channel swept the still-unmarked Y. Fixed with a registrationVersion check at sweep entry: at the start of StorageEntityCache#sweep(_longPredicate) - inside the registry mutex, so no registration can interleave with that channel's sweep - the channel compares the registry's current version (lock-free volatile read) against the wave's seedRegistrationVersion snapshot via the new StorageEntityMarkMonitor#isSeedRegistrationStale(long). On a mismatch it runs a keep-all pass (the existing rescueSweep/keep-all mechanism of store#746, factored into keepAllSweep()) instead of deleting; the wave's post-sweep re-seed then marks the newly-registered graph (X -> L -> Y) transitively and the cold sweep keeps it, deferring collection by one cycle. seedRegistrationVersion is made volatile and the check is deliberately lock-free (the caller holds the registry mutex; taking the monitor lock would invert against the seed path). isSeedRegistrationStale defaults to false, so monitor implementations that do not track the version keep their behavior.
Residual (documented, deliberately out of scope): if the child's channel sweeps before the reheating registration is observed, the child is already physically deleted and no post-hoc action can rescue it - the irreducible §10.4 residual. Closing it would require an atomic multi-channel sweep or refusing the partial resurrection. GC.md §10.4 is updated to describe both fixes (Layer 1's task-scoped gate, Layer 3's sweep-entry check) and this residual.
Tests (integration-tests, test.eclipse.store.gc):
- MultiChannelSweepEntryRaceReproTest - deterministic reproducer for Window B's rescuable subset (barrier parks all channels at sweep entry, registers X+L, releases): RED before the sweep-entry check, GREEN after; plus a control run that collects the orphan graph consistently.
- MultiChannelResidualWindowReproTest - the issue's reproducer of the irreducible residual (child's channel sweeps first); its race method is @disabled as executable documentation, its control run stays enabled.
Full test.eclipse.store.gc battery and the storage module unit tests pass.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the storage garbage collector for multi-channel setups by closing two race windows that could lead to partially collected, persisted graphs (dangling cross-channel references) during housekeeping GC waves.
Changes:
- Introduces a task-scoped pending-load gate (signaled at load-task enqueue, cleared on last channel completion) to prevent sweep initiation while any load task is in flight.
- Adds a sweep-entry registrationVersion staleness check that defers destructive sweep via keep-all when the wave seed is stale (deferring collection by one cycle instead of risking partial deletion).
- Updates GC documentation and adds deterministic multi-channel integration tests (including an executable-doc disabled test for the irreducible residual).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageTaskBroker.java | Signals the task-scoped pending-load gate at load enqueue and passes mark monitor into load tasks. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageSystem.java | Exposes a system-wide entityMarkMonitor() accessor. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskLoadRoots.java | Threads mark monitor into roots-load task construction. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskLoadByTids.java | Threads mark monitor into TID-load task construction. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskLoadByOids.java | Threads mark monitor into OID-load task construction. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskLoad.java | Clears the task-scoped pending-load gate on last completion. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskCreator.java | Extends load-task factory methods to accept mark monitor. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageEntityMarkMonitor.java | Adds task-scoped gate APIs and lock-free seed staleness check; integrates into marking-complete predicate. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageEntityCache.java | Adds sweep-entry stale-seed deferral using keep-all sweep; exposes mark monitor accessor. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageChannelTask.java | Adds onLastCompletion() hook for cross-channel completion actions. |
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageChannel.java | Exposes channel-level markMonitor() accessor. |
| storage/storage/GC.md | Documents Window A fix (task-scoped gate), Window B fix (sweep-entry staleness deferral), and the remaining residual. |
| integration-tests/src/test/java/test/eclipse/store/gc/MultiChannelSweepEntryRaceReproTest.java | New deterministic reproducer/regression test for sweep-entry registration race (Window B). |
| integration-tests/src/test/java/test/eclipse/store/gc/MultiChannelResidualWindowReproTest.java | New reproducer documenting the irreducible residual window (race method disabled) + enabled control run. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
signalPendingLoadTask() was called before enqueueTaskAndNotifyAll(), but the enqueue can throw (e.g. StorageExceptionNotRunning when processing is disabled) before the task is chained. On that path the task is never processed to completion, so its onLastCompletion() clear never fires and pendingLoadTaskCount leaks, which would keep isMarkingComplete() false and block sweep initiation. Factor the signal + enqueue of all three load-task kinds (byOids, byTids, roots) into enqueueLoadTaskAndNotifyAll(task, markMonitor), which signals the gate, then enqueues inside a try/catch that clears the gate and rethrows on failure. The signal still happens before the task becomes visible to any channel, so the Window-A ordering guarantee is preserved; the clear only runs on the enqueue failure path, so a successfully enqueued task still clears exactly once via onLastCompletion().
…entityMarkMonitor() (internal#85 review) The GC data-loss fix added several methods to interfaces that are exposed in the public API package, as required (abstract) methods - a binary-incompatible change for any external implementation. Make them default, following the existing convention in these interfaces (e.g. StorageEntityMarkMonitor#signalGcMarkingAbort, #isSeedRegistrationStale; StorageChannel#validateTrustedReferences): - StorageEntityMarkMonitor#signalPendingLoadTask() / #clearPendingLoadTask() are now default no-ops (paired). Custom monitors keep the pre-existing behavior (no task-scoped pending-load gate) and opt in by overriding both, exactly as with the already-defaulted isSeedRegistrationStale. - StorageChannel#markMonitor(), StorageEntityCache#markMonitor() and StorageSystem#entityMarkMonitor() are accessors with no sensible no-op, so their defaults throw UnsupportedOperationException; the built-in implementations override them. This preserves linkage for external subclasses while the internal wiring keeps working unchanged. Also harden StorageSystem.Default#entityMarkMonitor(): it dereferenced channelKeepers[0] unconditionally, which could NPE if called before startup created the channels or after teardown. It now throws StorageExceptionNotRunning when the channels are absent or the system is not running. Its only caller is the load-task enqueue path, which runs only while the system is running, so no legitimate call is rejected. No behavioral change on the production path; the GC test battery stays green.
…nitor to load tasks from the broker The task-scoped pending-load gate threaded the shared mark monitor into load tasks by adding a StorageEntityMarkMonitor parameter to the three StorageRequestTaskCreator factory methods. That is a breaking binary change for external StorageRequestTaskCreator implementations (AbstractMethodError after upgrade), and a defaulted-overload workaround would still leak the gate with a legacy creator that cannot pass the monitor on to the task. Provide the monitor from the broker instead of the creator, so the creator API is restored unchanged: - Revert createLoadTaskByOids / createRootsLoadTask / createLoadTaskByTids to their original signatures, and revert the load-task constructors accordingly. - Add StorageRequestTaskLoad#registerPendingLoadTaskGate(StorageEntityMarkMonitor), a default method returning false (not armed). StorageRequestTaskLoad.Abstract overrides it to store the monitor (now a volatile field, set before the task is published to any channel) and return true; onLastCompletion() clears the gate through it. - StorageTaskBroker arms the task with the monitor, and only signals the gate when the task reported it armed - so a custom load task that would not clear the gate never has it signaled and cannot make it leak. The gate is still signaled before the task becomes visible to any channel, and still released on the enqueue-failure path. Because the broker (not the creator) supplies the monitor, the gate works with any creator, legacy or not - there is no conditional-on-creator behavior to get wrong. Also fix an invalid Javadoc @link in StorageTaskBroker that referenced StorageRequestTaskLoad#onLastCompletion() (the method is on StorageRequestTaskLoad.Abstract, not the interface), which would fail Javadoc generation. No behavioral change on the production path; the GC test battery stays green.
zdenek-jonas
approved these changes
Jul 14, 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.
Summary
At
channelCount >= 2on the housekeeping GC path a load could straddle a sweep wave and leave the loaded graph partially collected: the parentXwas handed out to the application while a cross-channel child (e.g. an unloadedLazy's targetY) was swept, danglingX's persisted reference (StorageExceptionConsistency: No entity found for objectId Y, made durable by a re-store ofX). This is the multi-channel continuation of the single-channel mid-cycle registration race fixed in #736 (GC.md §10.4). Issue #85 spans two distinct windows that produce that same end state; this PR closes both, leaving one irreducible, documented residual.Window A — "variant 2": an in-flight load is invisible to sweep initiation
The per-channel
pendingLoadsignal is raised only around a channel's own collect and skipped entirely for a channel whose oid subset of the load is empty (if(!loadOids.isEmpty())). So a load already in flight does not stop a sibling channel from initiating a wave: the empty-subset channel seespendingLoadCount == 0inisMarkingComplete()and sweeps while another channel is still about to serve the load — reachable on every wave boundary, re-armed by every store.Closed with a task-scoped pending-load gate.
StorageEntityMarkMonitorgainssignalPendingLoadTask()/clearPendingLoadTask()andpendingLoadTaskCount(joined into theisMarkingComplete()AND-chain).StorageTaskBrokersignals it once at load-task enqueue — before any channel is notified — andStorageChannelTaskclears it once the task has completed on all channels (newonLastCompletion()hook, fired fromincrementCompletionProgress(), also on the exceptional path). So no wave can initiate while any load task is in flight anywhere, and the existing load-side marking takes the gray-enqueue (reference-walking) path rather than the shallow black-mark. The broker reaches the shared mark monitor lazily viaStorageSystem#entityMarkMonitor()(backed byStorageChannel#markMonitor()/StorageEntityCache#markMonitor()), since the monitor does not exist yet when the broker is constructed. No wave-state changes.Window B — the §10.4 residual: a registration lands after sweep initiation
A wave initiates with no load in flight and sweeps channel by channel; a load reheating
X(registeringXand its unloadedLazyL, neverY) in the gap between the wave's seed snapshot and a channel's sweep let the shallow id-only predicate rescueX+Lwhile a sibling channel swept the still-unmarkedY.Closed with a registrationVersion check at sweep entry. At the start of
StorageEntityCache#sweep(_longPredicate)— inside the registry mutex, so no registration can interleave with that channel's sweep — the channel compares the registry's current version (lock-free volatile read) against the wave'sseedRegistrationVersionsnapshot via the newStorageEntityMarkMonitor#isSeedRegistrationStale(long). On a mismatch it runs a keep-all pass (the existingrescueSweep/ keep-all mechanism from #746, factored intokeepAllSweep()) instead of deleting; the wave's post-sweep re-seed then marks the newly-registered graph (X -> L -> Y) transitively and the cold sweep keeps it, deferring collection by one cycle.seedRegistrationVersionis madevolatileand the check is deliberately lock-free (the caller holds the registry mutex; taking the monitor lock would invert against the seed path).isSeedRegistrationStaledefaults tofalse, so monitor implementations that do not track the version keep their behavior.Residual (deliberately out of scope, documented)
If the child's channel sweeps before the reheating registration is observed, the child is already physically deleted and no post-hoc action can rescue it — the irreducible §10.4 residual. Closing it would require an atomic multi-channel sweep or refusing the partial resurrection. GC.md §10.4 is updated to describe both fixes (Layer 1's task-scoped gate, Layer 3's sweep-entry check) and this residual.
Testing
MultiChannelSweepEntryRaceReproTest(new) — deterministic reproducer for Window B's rescuable subset: a barrier parks all channels at sweep entry, registersX+L, then releases. RED before the sweep-entry check, GREEN after (verified by temporarily neutralizing only fix 2 and rebuilding); includes a control run that collects the orphan graph consistently.MultiChannelResidualWindowReproTest— the issue's reproducer of the irreducible residual (child's channel sweeps first); its race method is@Disabledas executable documentation of the §10.4 residual, its control run stays enabled.test.eclipse.store.gcbattery (11 run, 0 failures/errors, 1 skipped) and thestoragemodule unit tests pass.