Fix CompletionStageSubscriber race condition, add jcstress tests and CI coverage gate#4296
Merged
andimarek merged 10 commits intoMar 6, 2026
Merged
Conversation
The spec317_mustSupportACumulativePendingElementCountUpToLongMaxValue TCK test intermittently hangs on CI (seen on Java 21). Root cause: a race between onComplete() and finallyAfterEachPromiseFinishes() where the queue-emptiness check and the onCompleteRun read/write are not atomic — the last CF can complete between the check and the set, leaving the runnable stranded forever. Fix by extending the existing ReentrantLock scope to also cover onCompleteRun, making the emptiness check and runnable set/claim a single atomic operation on both sides. Runnables are executed outside the lock to avoid deadlocks with user code. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (59 classes)
|
Add jcstress tests for CompletionStageSubscriber race condition verification and integrate them into both PR and master CI workflows. Since jcstress doesn't produce JUnit XML, parse its console output to extract test counts for the unified test report. Co-Authored-By: Claude Opus 4.6 <[email protected]>
tee opens the output file immediately at startup. Since build/ doesn't exist yet on a fresh runner, tee fails to open the file and exits with code 1 (while still copying stdin to stdout). pipefail then picks up that non-zero exit code, failing the step despite gradle succeeding. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The test subscribes 3 subscribers to the same publisher and asserts all receive elements in the same order. With a new single-thread executor per mapping call, each CF ran on its own thread, making delivery order depend on thread scheduling. Sometimes elements arrived in order (test passed), sometimes not (assertion failed, caught by TCK, converted to skip). Use a shared single-thread executor per publisher (matching the pattern already used by the Ordered publisher variants) so CFs complete sequentially in submission order. The test now deterministically passes. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The Reactive Streams TCK silently converts optional test failures to skips. Add a post-run verification step that asserts exactly 49 tests are skipped, catching any newly-silenced failure or newly-passing test immediately. Co-Authored-By: Claude Opus 4.6 <[email protected]>
The previous gate only checked overall line/branch/method coverage, which allowed per-class regressions to pass when offset by improvements elsewhere. Now fails on any class regressing any metric by >0.05%. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Skip classes with 0 total lines (interfaces, annotations, abstract classes) in the per-class coverage gate and changed-class display, fixing 113 false regressions caused by JaCoCo non-determinism for non-concrete classes. Also exclude these from future baseline generation. Add tests for GraphQLList.equals()/hashCode() and for CompletionStageOrderedSubscriber's cfExceptionUnwrap error path (catch block in emptyInFlightQueueIfWeCan). Co-Authored-By: Claude Opus 4.6 <[email protected]>
andimarek
changed the title
Co-Authored-By: Claude Opus 4.6 <[email protected]>
…notation These methods delegate to super and their coverage is non-deterministic (depends on hash collisions), causing false regressions in the per-class coverage gate. A Gradle task uses ASM to inject an invisible @generated annotation post-compilation, which JaCoCo's AnnotationGeneratedFilter recognizes and excludes from reports. Co-Authored-By: Claude Opus 4.6 <[email protected]>
CI installs JDK 25 and labels the default test run as "java25", but the Gradle toolchain was pinned to 21 causing Gradle to auto-provision JDK 21 instead of using the CI-installed JDK 25. Co-Authored-By: Claude Opus 4.6 <[email protected]>
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
1 participant
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
CompletionStageSubscriber.onComplete()where the completion signal could be lost if an in-flightCompletionStageresolved concurrently, leaving downstream subscribers hanging indefinitelyoptional_spec111TCK multicast test by using a shared single-thread executor per publisherGraphQLList.equals()/hashCode()andCompletionStageOrderedSubscriber.cfExceptionUnwrap()error pathDetails
Race condition fix
The race occurred in
CompletionStageSubscriberwhenonComplete()checkedinFlightDataQ.isEmpty()and decided to signal downstream, but between that check and settingonCompleteRun, another thread could drain the last CF and miss the runnable — leaving the subscriber hanging forever. Fix: extend the existingReentrantLockscope to make the emptiness check andonCompleteRunset/claim a single atomic operation on both sides.jcstress integration
jcstress doesn't produce JUnit XML, so console output is parsed to extract test counts (
planned,passed,failed,soft errs,hard errs) for the unified test report.Coverage gate
The per-class gate compares each class's line/branch/method coverage against the master baseline. Classes with 0 total lines (interfaces, annotations) are skipped to avoid false positives from JaCoCo non-deterministic attribution on non-concrete classes. The master workflow also excludes these from future baselines.
Missing test coverage
GraphQLList.equals()/hashCode(): explicit identity-semantics test (previously only covered non-deterministically viaHashSetcollisions)CompletionStageOrderedSubscriber.cfExceptionUnwrap(): tests the catch block inemptyInFlightQueueIfWeCan()using aSilentlyFailedFuturethat simulates the race where a CF has failed but itswhenCompletecallback hasn't fired yetTest plan
GraphQLListandCompletionStageOrderedSubscribertests pass🤖 Generated with Claude Code