test: fix flaky ReferenceCountingResourceHolderTest cleaner checks - #20317
FrankChen021 wants to merge 1 commit into
Conversation
The leaked resource counter is global to the JVM and is incremented before the closer runs. With reuseForks=true, the forced GC loop can be ended by a holder leaked by another test class in the same fork, before this test's closer has run, so the exact-count assertion and the released check fail intermittently. Wait for this test's own released flag instead, and only assert a lower bound on the global counter.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to test logic, aligns with the flakiness root cause, and reduces reliance on JVM-global state without affecting production code.
Pull request overview
This PR fixes flakiness in ReferenceCountingResourceHolderTest.testResourceHandlerClearedByJVM caused by relying on a JVM-global leaked-resource counter in a test environment where Maven Surefire reuses forks, allowing unrelated leaked holders from other tests to affect the assertion.
Changes:
- Update
verifyCleanerRunto wait for this test’s own completion signal (releasedflag) rather than waiting for a change in the global leak counter. - Relax the leaked-resource counter assertion from exact
+1to a lower bound (>= initial + 1) to tolerate additional leaked holders being collected in the same JVM.
File summaries
| File | Description |
|---|---|
| processing/src/test/java/org/apache/druid/collections/ReferenceCountingResourceHolderTest.java | Makes the cleaner/GC verification logic deterministic by waiting on the test-specific released flag and loosening the global counter expectation. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
No actionable issues found in this review. The test now waits for its own closer completion signal, while treating the JVM-global leak counter as a lower bound, which matches the cleaner ordering and avoids interference from unrelated leaked holders.
Reviewed 1 of 1 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
Related to #20312 (item 2).
Description
ReferenceCountingResourceHolderTest.testResourceHandlerClearedByJVMfails intermittently on master with:Example: https://github.com/apache/druid/actions/runs/34432737655/job/102731457738.
ReferenceCountingResourceHolder.LEAKED_RESOURCESis a JVM-global counter, and surefire runs withreuseForks=true.verifyCleanerRunloops onSystem.gc()until the counter changes and then asserts an exact+1. Holders leaked by other test classes in the same fork are collected by the same forced GC, so the delta can be larger than 1. Because the counter is incremented before the closer runs, the loop can also exit before this test's own closer has executed, which would make thereleasedassertion fail too.Changes
releasedflag is set, which is the completion signal that belongs to this test.released, and only assert a lower bound (>= initial + 1) on the global counter.Verified locally with
mvn -pl processing test -Dtest=ReferenceCountingResourceHolderTest(3 tests pass).Key changed/added classes in this PR
ReferenceCountingResourceHolderTestThis PR has:
Additional CI evidence
The same global-counter/cleaner race reappeared as a recovered flaky failure in both dependency-update PRs:
expected: <1> but was: <3>.expected: <1> but was: <3>.These occurrences were recorded as flaky failures; they are independent of the dependency changes in those PRs.