ci: streamline Rocky dependencies and improve build failure diagnostics - #6961
ci: streamline Rocky dependencies and improve build failure diagnostics#69613for wants to merge 14 commits into
Conversation
- Remove Development Tools and unnecessary packages - Install only JDK 8, git-core and zstd with weak dependencies disabled - Use C.utf8 to avoid installing glibc-langpack-en - Resolve and validate JAVA_HOME so Gradle no longer requires which
- Collect tron-test.log, rotated logs and JUnit XML across five PR build jobs - Run artifact uploads even when preceding steps fail - Use distinct artifact names per job and matrix configuration - Retain artifacts for 7 days and warn when no files are found
Remove the stale integration-test-multinode.yml reference to avoid unnecessary API requests when cancelling workflows for closed PRs.
- Save build, RocksDB test and coverage output with tee and plain console mode - Use Bash pipefail to preserve failures when capturing stdout and stderr - Include console logs and HTML test reports in diagnostic artifacts - Preserve existing test retry and base coverage failure policies
Remove the test-retry plugin and retry configuration shared by test and testWithRocksDb so test failures fail the task without retrying.
- Limit diagnostic artifacts to **/logs/tron-test.log and ci-logs/*.log - Upload logs only when a preceding step fails - Include base test failures tolerated by continue-on-error - Keep the existing 7-day retention period
- Set DNF max_parallel_downloads to 10 for Rocky dependency installation - Restore the test-retry plugin and configuration as comments, keeping retries disabled
- Clear the thread-local VM snapshot after constant calls in TransferToAccountTest - Use OS-assigned ports for Prometheus tests to avoid conflicts between test JVMs - Ensure BackupServer closes channels bound after shutdown begins - Add regression coverage for VM state cleanup and shutdown during binding
- Clear thread-local VM configuration after each RuntimeImplTest case - Stop background block production in ShieldedReceiveTest to preserve synthetic Merkle roots in pending sessions - Keep the producer stopped after the manual block production test
- Avoid fixed ports in event queue tests and let Jetty allocate its port - Verify ZeroMQ delivery with bounded waits and reliable cleanup - Propagate worker exceptions and validate concurrent toString results - Shut down reward test executors before closing stores - Close HTTP clients and responses and propagate request failures - Correct lite-node history query coverage and assertions
- Restore VM configuration and static rate limiter state after tests - Remove global output redirection and URL handler changes from servlet tests - Verify async task results and service startup and shutdown futures - Close network services, channels and application contexts on failure - Replace timing-sensitive assertions with controlled clocks and signals - Isolate temporary files and avoid fixed listening ports - Fix unbounded waiting in the disabled concurrent benchmark - Add regression coverage for setup and cleanup failures
- Skip the intentional failure fixture during automatic test discovery while preserving explicit VM state restoration checks - Split transaction handler tests into deterministic submission, queue capacity, and invalid transaction scenarios - Preserve unexpected exception details and strengthen assertions - Complete mock dependency setup and executor cleanup
- Isolate block message handler tests with explicit rejection and routing checks - Wait for producer completion before finishing concurrent filter consumption - Bound worker waits, propagate failures, and clean up test resources - Preserve original exception causes in test failures and temporary directory setup
- Use independent event plugin loaders and restore the original singleton - Restore VM configuration and ensure context and Args cleanup on failure - Assert block metric deltas and unchanged error counts against test baselines - Preserve the cause of temporary directory creation failures
| private BackupManager backupManager; | ||
|
|
||
| private Channel channel; | ||
| private volatile Channel channel; |
There was a problem hiding this comment.
[Question] What's the motivation for making channel volatile here? Reading the surrounding code, this appears to fix a close-vs-bind race: close() can set shutdown while bind() is still in flight and observe a null channel, and the post-bind guard at L79–82 then closes the late-bound channel. Two questions:
-
Is this race reachable in production, or only in tests? The new
BackupServerLifecycleTest#testCloseDuringBindsuggests the failing scenario came from the test side. If this production change is primarily to make unit tests pass, is that the right trade-off — or would fixing the test lifecycle be cleaner, or should this be split into a separatefix:PR with its own justification? It also isn't mentioned in the PR title/body, which currently only describes CI changes. -
If the cross-thread visibility fix is warranted, should
executor(~L35) be volatile as well? It follows the same pattern: written byinitServer(), read unsynchronized byclose(). A cross-threadclose()can still observe a stale-nullexecutorand skipshutdownAndAwaitTermination, leaking the non-daemon worker thread even though the channel is now closed.
| ScheduledExecutorService executor = Mockito.mock(ScheduledExecutorService.class); | ||
| Mockito.when(executor.submit(Mockito.any(Runnable.class))) | ||
| .thenReturn(CompletableFuture.completedFuture(null)); | ||
| ReflectUtils.setFieldValue(service, "executor", executor); |
There was a problem hiding this comment.
[Should] Replacing the bean's executor with a mock here drops the only reference to the real ScheduledExecutorService created by EffectiveCheckService.init(), and it is never restored or shut down. The real non-daemon pool keeps running after this test, and when the Spring context is destroyed, EffectiveCheckService.close() ends up calling shutdownAndAwaitTermination on the mock instead of the real executor — so the actual pool leaks for the rest of the JVM.
Consider capturing the original executor before overwriting the field and restoring it in @After closeP2p() (or explicitly shutting the original down), so the bean lifecycle still manages the real pool.
3 participants
What does this PR do?
C.utf8, and configureJAVA_HOMEexplicitly.**/logs/tron-test.logandci-logs/*.logonly on failure, with 7-day retention.Why are these changes required?
Unnecessary packages increase CI setup time. Missing console logs make test failures harder to diagnose, while automatic retries can hide intermittent failures. Workflow cancellation should also reflect the workflows that still exist.
This PR has been tested by: