Repair the task chain when a prepended garbage collection task fails - #734
Merged
Conversation
The follow-up task of a prepended GC task (issueGarbageCollection, exportChannels with GC) was chained only in succeed(); on failure the chain was severed, stranding the channel on the dead task's monitor for a full housekeeping interval and hanging any subsequently enqueued task - including shutdown - behind the unreachable follow-up. Repair the linkage in the always-executed per-channel cleanUp() when the task has problems.
A throwing StorageGCZombieOidHandler makes the issued garbage collection fail deterministically on a planted dangling reference; before the repair the subsequent shutdown hung for the full (deliberately huge) housekeeping interval - caught by the test timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a storage task-queue deadlock where a prepended garbage-collection request task could fail without ever linking its follow-up task into the chain, causing subsequently enqueued tasks (including shutdown) to stall behind an unreachable head.
Changes:
- Add a
cleanUp(StorageChannel)override toStorageRequestTaskGarbageCollection.Defaultto repair the task chain when the GC task fails beforesucceed()can link the follow-up task. - Add an integration regression test (
FailedGcTaskChainRepairTest) that forces a failing prepended GC under a large housekeeping interval and asserts shutdown completes promptly afterward.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| storage/storage/src/main/java/org/eclipse/store/storage/types/StorageRequestTaskGarbageCollection.java | Repairs the task linkage in per-channel cleanup when a prepended GC task fails. |
| integration-tests/src/test/java/test/eclipse/store/gc/FailedGcTaskChainRepairTest.java | Adds regression coverage ensuring failed prepended GC no longer strands the task chain and blocks shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…only from Javadoc; use a labeled fully-qualified @link instead.
The test was flaky on CI: housekeeping runs after every processed task until the interval time budget is consumed - the huge housekeeping interval only delays the budget refresh, it does not stop housekeeping right after startup. On a slow runner the incremental housekeeping GC could reach the planted zombie OID after the store, which registers a channel DISRUPTION and disables processing. The subsequent shutdown() then fails at task enqueue with StorageExceptionDisruptingExceptions instead of exercising the repaired-task-chain path, failing the test without testing the fix. Two measures pin the intended path: - a housekeeping time budget of 1 ns, exhausted by the first (pre-test-data, harmless) housekeeping task, so no housekeeping GC ever sees the zombie - the throwing zombie handler is armed only around the issued garbage collection, with an assertion that the failure did not take the channel-disruption path Additionally, both shutdown calls now run through a bounded probe on a daemon thread: pre-fix, a plain shutdown() blocked for the full housekeeping interval (an hour here), stalling the test JVM far beyond the @timeout instead of reporting a clean assertion failure after 30 s. The @AfterEach also must not pre-check isRunning(): the hanging shutdown holds the storage system's state monitor, and isRunning() blocks on that same monitor indefinitely. Verified red/green: against pre-fix code all three surefire runs fail with the clean 30 s assertion (~41 s each, build terminates); with the fix the test passes in ~1 s.
Three review findings on the task-chain repair, all confirmed by new red tests: - Export with GC swallowed the failure: exportChannels(.., true) waits on the EXPORT task, never on the prepended GC task, so after the chain repair the export ran and reported success although the "definite minimum" contract was violated. The repair now propagates the GC task's problems into the follow-up task first - the export's waiter fails loudly with the GC failure as cause. (For an issued GC the follow-up is a Dummy nobody waits on; harmless.) - Multi-channel hang: a channel failing mid-marking never delivers its pending marks; sibling channels waited forever in the issued GC's waitForWork loop (unbounded time budget, bounded 10 ms waits, no exit condition, nothing ever interrupts channel threads). New mark-monitor abort signal, raised in the failing channel's cleanUp, checked in both issued-GC loops; marking state itself is left untouched so the abandoned marks are drained consistently by the failed channel's later housekeeping GC. Cleared once per GC task before processing (race-free behind the completion barrier). - Repair interleaving: the failing channel's cleanUp runs before the completion barrier; chaining there let the follow-up run while siblings still collected. The repair is now gated on isProcessed() and explicitly wakes channels already parked in awaitNext (setNext does not notify - without this the failing channel slept out its full awaitNext timeout and stalled the follow-up's barrier).
zdenek-jonas
approved these changes
Jul 6, 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.
What this fixes
Certain storage operations (an explicitly issued full garbage collection, or a channel export, which prepends a GC run) are executed as a small chain of internal tasks: first the garbage collection, then the actual follow-up work. If the garbage collection task fails - for any reason, e.g. a custom handler that throws - the chain between the two tasks is never completed.
The consequence is worse than a failed operation: every task enqueued afterwards, including a storage shutdown, lines up behind a follow-up task that no channel can ever reach. The channel threads then sit on the dead task and only re-check after the full housekeeping interval has elapsed. With a default interval of one second this hides as a small hiccup; with a large configured interval a simple
storage.shutdown()can block for an hour. From the outside the storage just hangs, with no error pointing at the cause.After this fix, a failed garbage collection task repairs the chain on its way out, the failure is reported to the caller as usual, and everything queued behind it - including shutdown - proceeds immediately.
Details
StorageRequestTaskGarbageCollection.Defaultlinks its follow-up task into the processing chain only insucceed(). On a failure,fail()runs instead, the follow-up (and everything enqueued after it, since the task broker already chained new tasks behind it) becomes unreachable, and each channel'sawaitNext(ms)times out and reverts to the same dead task - effectively polling once per housekeeping interval, forever.cleanUp(StorageChannel)on the GC task repairs the chain when the task has problems and no next task is linked yet (this.hasProblems() && this.actualTask != null && this.next() == null → this.setActualTask()). The repair is synchronized and idempotent, so it is safe when multiple channels run their clean-up concurrently.Test
FailedGcTaskChainRepairTest(integration-tests): configures a huge housekeeping interval (one hour) so that a severed chain would visibly hang, makes the prepended GC task fail via an inline throwing zombie-OID handler over a planted dangling reference, asserts the failure reaches the caller, and asserts the subsequent shutdown completes promptly (@Timeout(60)guards the pre-fix behavior, which only recovers after the full interval).The bug exists on
mainindependently of any other pending work; the fix is store-only and self-contained.