ci: add Integration Smoke workflow#5
Open
warku123 wants to merge 10 commits into
Open
Conversation
* fix(config): remove redundant JSON-RPC config fields and consolidate parameter binding Remove maxBlockFilterNum, maxAddressSize, and maxRequestTimeout from NodeConfig/CommonParameter since they were superseded by the jsonRpcMaxBatchSize/jsonRpcMaxResponseSize parameters. Consolidate the config binding path so all JSON-RPC size limits flow through a single reference.conf entry, and clean up stale test-config fixtures. * fix bug of NodeConfigTest * remove allowShieldedTransactionApi from reference.conf * add testcase of external.ip is null * change comment * fix the bug of --es and 7 failed testcases
Pulls warku123/java-tron-integration-test image and runs --smoke against the FullNode.jar built from the PR. Triggers on PR to develop/release_**. Test reports are extracted from the container and uploaded as artifacts. JAVA_HOME is pinned to JDK 8 inside the container so FullNode runs on the production runtime (a few smoke tests assert java.version starts with "1.8"); JAVA_HOME_17 keeps Gradle on JDK 17 for the test tooling.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an integration-test GitHub Actions workflow; implements a configurable blocking/non-blocking rate-limiter API with refactors across strategies/adapters and runtime callers; gates event plugins by minimum version and adjusts event-config application; and updates tests and small runtime guards. ChangesIntegration Test Workflow
Rate limiter blocking/non-blocking API and refactor
Event plugin gating, Args flow, and tests
Misc test lifecycle and small runtime guards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/integration-test.yml (1)
41-42: ⚡ Quick winConsider validating FullNode.jar existence before Docker mount.
While Docker will fail if the jar doesn't exist, an explicit check would provide clearer error messages during debugging.
✅ Add validation step
- name: Build FullNode.jar run: ./gradlew clean build -x test --no-daemon + +- name: Verify FullNode.jar exists + run: test -f build/libs/FullNode.jar || (echo "FullNode.jar not found" && exit 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/integration-test.yml around lines 41 - 42, Add an explicit validation step immediately after the "Build FullNode.jar" step that checks for the built JAR file produced by the ./gradlew clean build -x test --no-daemon command (e.g., the expected FullNode.jar path under build/libs or the configured output location) and fail the job with a clear error message if the file is missing; update the workflow to reference the same build step name ("Build FullNode.jar") and the exact expected artifact path so the check uses a deterministic -f (or equivalent) test and reports a helpful diagnostic before the Docker mount runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/integration-test.yml:
- Around line 41-42: Add an explicit validation step immediately after the
"Build FullNode.jar" step that checks for the built JAR file produced by the
./gradlew clean build -x test --no-daemon command (e.g., the expected
FullNode.jar path under build/libs or the configured output location) and fail
the job with a clear error message if the file is missing; update the workflow
to reference the same build step name ("Build FullNode.jar") and the exact
expected artifact path so the check uses a deterministic -f (or equivalent) test
and reports a helpful diagnostic before the Docker mount runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9358e348-5ad5-43a8-b4b2-1ed45c18c4fa
📒 Files selected for processing (1)
.github/workflows/integration-test.yml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/integration-test.yml">
<violation number="1" location=".github/workflows/integration-test.yml:58">
P2: Pin the integration test image to an immutable digest (or fixed version tag) instead of `:latest` so smoke runs are reproducible and safer.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| -e JAVA_HOME=/usr/lib/jvm/temurin-8 \ | ||
| -e JAVA_HOME_17=/opt/java/openjdk \ | ||
| -v "${{ github.workspace }}/build/libs/FullNode.jar:/javatron/FullNode.jar:ro" \ | ||
| warku123/java-tron-integration-test:latest \ |
There was a problem hiding this comment.
P2: Pin the integration test image to an immutable digest (or fixed version tag) instead of :latest so smoke runs are reproducible and safer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/integration-test.yml, line 58:
<comment>Pin the integration test image to an immutable digest (or fixed version tag) instead of `:latest` so smoke runs are reproducible and safer.</comment>
<file context>
@@ -0,0 +1,78 @@
+ -e JAVA_HOME=/usr/lib/jvm/temurin-8 \
+ -e JAVA_HOME_17=/opt/java/openjdk \
+ -v "${{ github.workspace }}/build/libs/FullNode.jar:/javatron/FullNode.jar:ro" \
+ warku123/java-tron-integration-test:latest \
+ --smoke --clean
+
</file context>
…onprotocol#6760) Pre-3.0.0(The previous event-plugin public release is 2.2.0) event-plugin builds still link against com.alibaba.fastjson, which was removed from java-tron in tronprotocol#6701. When such a plugin is loaded, the NoClassDefFoundError surfaces inside the plugin's own worker thread and is swallowed by its catch(Throwable) handler. The node keeps running while silently dropping every trigger, leaving operators with no signal that the event subscription is broken. Enforce a minimum Plugin-Version at startup in EventPluginLoader.startPlugin using pf4j's VersionManager (semver). When the descriptor version is below 3.0.0, log a clear upgrade hint and return false; the existing call chain in Manager.startEventSubscribing wraps that into TronError(EVENT_SUBSCRIBE_INIT) and aborts node startup instead of silently degrading.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/integration-test.yml">
<violation number="1" location=".github/workflows/integration-test.yml:59">
P1: The workflow no longer passes the `--smoke` flag to the integration test container, which changes test scope from smoke-only to default behavior. This can make PR checks slower and less reliable than intended for smoke gating.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The smoke target (~74 cases) is a strict subset of the default `test` target (~hundreds of cases). Smoke gives a 5-min PR signal but misses regressions outside the @tag("smoke") classes. Switch to running the full single-node suite (~12-15 min) so PRs catch more issues. Renames job/container/artifact from 'smoke' to 'integration' to reflect the wider scope. Bumps timeout from 30 to 45 minutes.
warku123
force-pushed
the
feat/ci-integration-smoke
branch
from
af0b1ce to
1384b91
Compare
…rs (tronprotocol#6761) - Add `rate.limiter.apiNonBlocking` switch (default false). When off, callers wait for a permit; when on, they reject immediately and shed load. - Wire the switch through `RateLimiterConfig` → `Args` → `CommonParameter`; update `reference.conf` / `config.conf`. - Add blocking `acquire()` paths on `IRateLimiter`, `GlobalRateLimiter`, and `QpsStrategy` / `IPQpsStrategy` / `GlobalPreemptibleStrategy`. Only the semaphore-based strategy is bounded (2s); the QPS-based paths use unbounded Guava `RateLimiter.acquire()`. - Introduce `acquirePermit()` dispatcher (default method on `IRateLimiter`; static on `GlobalRateLimiter`) that picks blocking vs non-blocking based on the switch. - Swap `tryAcquire` → `acquirePermit` at the `RateLimiterServlet` and `RateLimiterInterceptor` call sites; preserve per-endpoint-before-global ordering to avoid consuming global quota on per-endpoint rejection. - Extract `loadIpLimiter()` in `GlobalRateLimiter` and `loadLimiter()` in `IPQpsStrategy` to share cache+exception handling between `tryAcquire` and `acquire`. - Add unit tests covering dispatcher routing (both directions), acquire IP-before-global ordering, and IP-loader-failure fail-closed behaviour.
… and tronprotocol#6761 (rate-limiter blocking mode)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java (1)
56-80: ⚡ Quick winAdd a malformed-version regression case for
checkVersion(...).Please add a case like
"v3"or"3.x"in Line 60-70 expectations to ensure invalid format is rejected without throwing after validation hardening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java` around lines 56 - 80, Add a malformed-version regression case to the test: update EventLoaderTest.testIsPluginVersionSupported by adding one or more assertions calling checkVersion("v3") and/or checkVersion("3.x") (or similar malformed strings) in the block that asserts rejected versions (around the existing false checks for "1.0.0","2.2.0","2.9.9") and assertFalse on those calls; this ensures EventPluginLoader.isPluginVersionSupported (which uses EventPluginLoader.MIN_PLUGIN_VERSION and is exercised via the helper checkVersion) returns false for malformed version formats without throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java`:
- Around line 491-503: The isPluginVersionSupported method currently
dereferences pm.getPlugin(pluginId).getDescriptor().getVersion() without null
checks and calls VERSION_MANAGER.compareVersions which can throw LexerException
on malformed versions; update isPluginVersionSupported to defensively check for
nulls from pm.getPlugin(pluginId) and plugin.getDescriptor() and return false
with a clear logger.warn/error if metadata is missing, then wrap
VERSION_MANAGER.compareVersions(pluginVersion, MIN_PLUGIN_VERSION) in a
try/catch that catches LexerException (and optionally IllegalArgumentException),
logs the malformed version including pluginId and pluginVersion, and returns
false instead of letting the exception propagate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.java`:
- Around line 34-45: The current GlobalRateLimiter.acquire(RuntimeData) uses
unbounded blocking calls (r.acquire() and rateLimiter.acquire()) which can stall
threads; change both acquires to bounded/time-limited waits (e.g., tryAcquire
with a timeout like the 2-second pattern used in GlobalPreemptibleStrategy) and
return false when a timeout occurs; update the per-IP loader path (loadIpLimiter
-> r) and the global rateLimiter usage to use the timed-try-acquire pattern and
ensure both success checks are handled (and mirror the same timeout behavior for
the other acquire overload referenced around lines 47-51).
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/RateLimiterInterceptor.java`:
- Around line 111-114: The interceptor currently calls blocking acquirePermit()
on RateLimiter and GlobalRateLimiter (variables perEndpointAcquired and
acquireResource) before next.startCall(), which can park RPC worker threads;
change this to a non-blocking or deadline-bound acquisition: use a
non-blocking/tryAcquire API (e.g., tryAcquirePermit or an acquirePermit variant
that accepts a timeout) for RateLimiter.acquirePermit(runtimeData) and
GlobalRateLimiter.acquirePermit(runtimeData), or compute the remaining RPC
deadline from Context.current().getDeadline() and pass that as a bounded timeout
to the acquire methods and immediately fail fast if the deadline has elapsed;
ensure next.startCall(...) is never preceded by an unbounded blocking call in
RateLimiterInterceptor.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.java`:
- Around line 29-36: In IPQpsStrategy.acquire, avoid uninterruptible blocking by
replacing limiter.acquire() with a bounded, interruptible attempt: call
limiter.tryAcquire(timeout, TimeUnit) (choose a sensible timeout) and handle
InterruptedException by restoring the thread interrupt status and returning
false; if tryAcquire returns false (timeout) return false as well. Locate this
logic in the acquire(String ip) method (and use loadLimiter(ip) and the limiter
variable) and ensure the method returns true only when tryAcquire succeeds.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.java`:
- Around line 33-36: QpsStrategy.acquire() currently calls rateLimiter.acquire()
which blocks uninterruptibly; change it to use a timeout-based acquisition
similar to GlobalPreemptibleStrategy.acquire(): call
rateLimiter.tryAcquire(timeout, TimeUnit.MILLISECONDS) (choose a sensible
timeout), handle InterruptedException by restoring the thread interrupt status
and returning false, and return the boolean result of tryAcquire so callers can
detect failure instead of blocking forever; update the QpsStrategy.acquire()
implementation and any callers/tests expecting unconditional true.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.java`:
- Around line 56-80: Add a malformed-version regression case to the test: update
EventLoaderTest.testIsPluginVersionSupported by adding one or more assertions
calling checkVersion("v3") and/or checkVersion("3.x") (or similar malformed
strings) in the block that asserts rejected versions (around the existing false
checks for "1.0.0","2.2.0","2.9.9") and assertFalse on those calls; this ensures
EventPluginLoader.isPluginVersionSupported (which uses
EventPluginLoader.MIN_PLUGIN_VERSION and is exercised via the helper
checkVersion) returns false for malformed version formats without throwing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ce1df56-5376-40ce-99ee-188fb656acb0
📒 Files selected for processing (23)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacommon/src/main/java/org/tron/core/config/args/EventConfig.javacommon/src/main/java/org/tron/core/config/args/RateLimiterConfig.javacommon/src/main/resources/reference.confcommon/src/test/java/org/tron/core/config/args/RateLimiterConfigTest.javaframework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/net/peer/PeerConnection.javaframework/src/main/java/org/tron/core/services/http/RateLimiterServlet.javaframework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.javaframework/src/main/java/org/tron/core/services/ratelimiter/RateLimiterInterceptor.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/DefaultBaseQqsAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/GlobalPreemptibleAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/IPQPSRateLimiterAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/IRateLimiter.javaframework/src/main/java/org/tron/core/services/ratelimiter/adapter/QpsRateLimiterAdapter.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/GlobalPreemptibleStrategy.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.javaframework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/logsfilter/EventLoaderTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javaframework/src/test/java/org/tron/core/net/messagehandler/ChainInventoryMsgHandlerTest.java
✅ Files skipped from review due to trivial changes (2)
- common/src/main/java/org/tron/core/config/args/EventConfig.java
- common/src/main/java/org/tron/common/parameter/CommonParameter.java
Comment on lines
+491
to
+503
| static boolean isPluginVersionSupported(PluginManager pm, String pluginId) { | ||
| String pluginVersion = pm.getPlugin(pluginId).getDescriptor().getVersion(); | ||
| if (Strings.isNullOrEmpty(pluginVersion)) { | ||
| return false; | ||
| } | ||
| boolean isSupported = VERSION_MANAGER.compareVersions(pluginVersion, MIN_PLUGIN_VERSION) >= 0; | ||
|
|
||
| if (!isSupported) { | ||
| logger.error( | ||
| "event-plugin '{}' version {} is older than required {}, please upgrade event-plugin", | ||
| pluginId, pluginVersion, MIN_PLUGIN_VERSION); | ||
| } | ||
| return isSupported; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PF4J, does DefaultVersionManager.compareVersions(...) throw on non-semver strings (e.g., "v3", "3.x"), and what exception type is documented?
💡 Result:
Yes—PF4J’s DefaultVersionManager.compareVersions(...) will throw when given non-SemVer-ish version strings such as "v3" or "3.x". PF4J’s default VersionManager implementation is built on the external SemVer parsing library, and failures to tokenize/parse a version/constraint surface as com.github.zafarkhaja.semver.expr.LexerException (thrown from the SemVer expression/version parser) [1]. Exception type documented/observed: com.github.zafarkhaja.semver.expr.LexerException [1].
Citations:
🏁 Script executed:
# First, check the file exists and get its size
wc -l framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.javaRepository: warku123/java-tron
Length of output: 140
🏁 Script executed:
# Read the specific method and surrounding context (lines 485-510)
sed -n '485,510p' framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.javaRepository: warku123/java-tron
Length of output: 802
🏁 Script executed:
# Look for imports to understand what PluginManager, PluginWrapper are
head -50 framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java | grep -E "import|package"Repository: warku123/java-tron
Length of output: 1310
🏁 Script executed:
# Check if getPlugin() is used elsewhere and how null safety is handled
rg "pm\.getPlugin|PluginManager" framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java -B 2 -A 2Repository: warku123/java-tron
Length of output: 981
🏁 Script executed:
# Look for where startPlugin calls isPluginVersionSupported to understand the context
rg "isPluginVersionSupported|startPlugin" framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java -B 2 -A 5Repository: warku123/java-tron
Length of output: 1093
🏁 Script executed:
# Search for other usages of pm.getPlugin() in the codebase to see if null checks are used
rg "getPlugin\(" framework/src/main/java/org/tron/common/logsfilter/ -B 1 -A 1Repository: warku123/java-tron
Length of output: 491
🏁 Script executed:
# Check if there are other null safety patterns for plugin operations
rg "Objects\.isNull|Objects\.nonNull|== null" framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.javaRepository: warku123/java-tron
Length of output: 719
Harden version validation to avoid startup-time exceptions on bad plugin metadata.
Line 492 chains three dereferences (pm.getPlugin(pluginId).getDescriptor().getVersion()) without null checks. Both getPlugin() and getDescriptor() can return null in PF4J, causing a NullPointerException if metadata is missing. Additionally, at line 496, VERSION_MANAGER.compareVersions() throws LexerException when given malformed version strings (e.g., non-SemVer formats), which will abort startup instead of cleanly rejecting the plugin.
Proposed fix
static boolean isPluginVersionSupported(PluginManager pm, String pluginId) {
- String pluginVersion = pm.getPlugin(pluginId).getDescriptor().getVersion();
- if (Strings.isNullOrEmpty(pluginVersion)) {
- return false;
- }
- boolean isSupported = VERSION_MANAGER.compareVersions(pluginVersion, MIN_PLUGIN_VERSION) >= 0;
+ org.pf4j.PluginWrapper wrapper = pm.getPlugin(pluginId);
+ if (wrapper == null || wrapper.getDescriptor() == null) {
+ logger.error("event-plugin '{}' descriptor is missing", pluginId);
+ return false;
+ }
+
+ String pluginVersion = wrapper.getDescriptor().getVersion();
+ if (Strings.isNullOrEmpty(pluginVersion)) {
+ logger.error("event-plugin '{}' version is empty", pluginId);
+ return false;
+ }
+
+ final boolean isSupported;
+ try {
+ isSupported = VERSION_MANAGER.compareVersions(pluginVersion, MIN_PLUGIN_VERSION) >= 0;
+ } catch (RuntimeException e) {
+ logger.error("event-plugin '{}' has invalid version '{}'", pluginId, pluginVersion, e);
+ return false;
+ }
if (!isSupported) {
logger.error(
"event-plugin '{}' version {} is older than required {}, please upgrade event-plugin",
pluginId, pluginVersion, MIN_PLUGIN_VERSION);
}
return isSupported;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/src/main/java/org/tron/common/logsfilter/EventPluginLoader.java`
around lines 491 - 503, The isPluginVersionSupported method currently
dereferences pm.getPlugin(pluginId).getDescriptor().getVersion() without null
checks and calls VERSION_MANAGER.compareVersions which can throw LexerException
on malformed versions; update isPluginVersionSupported to defensively check for
nulls from pm.getPlugin(pluginId) and plugin.getDescriptor() and return false
with a clear logger.warn/error if metadata is missing, then wrap
VERSION_MANAGER.compareVersions(pluginVersion, MIN_PLUGIN_VERSION) in a
try/catch that catches LexerException (and optionally IllegalArgumentException),
logs the malformed version including pluginId and pluginVersion, and returns
false instead of letting the exception propagate.
Comment on lines
+34
to
+45
| public static boolean acquire(RuntimeData runtimeData) { | ||
| String ip = runtimeData.getRemoteAddr(); | ||
| if (!Strings.isNullOrEmpty(ip)) { | ||
| RateLimiter r = loadIpLimiter(ip); | ||
| if (r == null) { | ||
| return false; | ||
| } | ||
| r.acquire(); | ||
| } | ||
| rateLimiter.acquire(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== request-path permit usage ==="
rg -nP '\bacquirePermit\s*\(' --type=java framework/src/main/java/org/tron/core/services/http framework/src/main/java/org/tron/core/services/ratelimiter
echo
echo "=== blocking acquire callsites in ratelimiter package ==="
rg -nP '\.\s*acquire\s*\(' --type=java framework/src/main/java/org/tron/core/services/ratelimiterRepository: warku123/java-tron
Length of output: 2495
🏁 Script executed:
cat -n framework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.javaRepository: warku123/java-tron
Length of output: 2557
🏁 Script executed:
# Check for ACQUIRE_TIMEOUT_MS or similar timeout constants
rg -n 'ACQUIRE_TIMEOUT|TIMEOUT_MS' --type=java framework/src/main/java/org/tron/core/services/ratelimiter
# Check the Args configuration for the default value
rg -n 'isRateLimiterApiNonBlocking|rateLimiterApiNonBlocking' --type=java framework/src/main/javaRepository: warku123/java-tron
Length of output: 814
🏁 Script executed:
# Check default value of isRateLimiterApiNonBlocking
rg -B5 -A5 'rateLimiterApiNonBlocking' --type=java framework/src/main/java/org/tron/core/config/args/Args.java | head -30
# Verify that request paths only use acquirePermit()
rg -n 'GlobalRateLimiter\.acquire\(' --type=java framework/src/main/java/org/tron/core/servicesRepository: warku123/java-tron
Length of output: 802
🏁 Script executed:
# Check the RateLimiter configuration defaults
rg -n 'isApiNonBlocking|class RateLimiter' --type=java framework/src/main/java/org/tron/core/config | head -20
# Check if there's a default config file
fd -i 'config\|application' framework/src/main/resources | head -20Repository: warku123/java-tron
Length of output: 191
🏁 Script executed:
# Look for RateLimiter configuration classes and default values
rg -n 'isApiNonBlocking|ApiNonBlocking' --type=java framework/src/main/java/org/tron/core/config
# Check if there's a default config file in resources
find framework/src/main/resources -name '*.properties' -o -name '*.yml' -o -name '*.yaml' -o -name '*.conf' 2>/dev/null | head -20Repository: warku123/java-tron
Length of output: 232
🏁 Script executed:
# Check the default config file
cat framework/src/main/resources/config.conf | grep -A10 -B2 'api.*[Nn]on[Bb]locking\|[Rr]ate.*[Ll]imiter' | head -50Repository: warku123/java-tron
Length of output: 1558
🏁 Script executed:
# Search for isApiNonBlocking in config.conf
grep -n -i 'apinonblocking\|api.*non' framework/src/main/resources/config.conf
# Also check the RateLimiterConfig bean class to understand the default
rg -n 'class RateLimiterConfig|isApiNonBlocking' --type=java framework/src/main/java/org/tron/core/configRepository: warku123/java-tron
Length of output: 302
Blocking acquire path can indefinitely stall request threads under default configuration.
acquire() waits unbounded on both per-IP and global limiters. With the default configuration (apiNonBlocking = false), request threads can be indefinitely blocked, causing thread pool saturation and long tail latency. Use bounded waits (timeout-based acquire) instead, as already demonstrated in GlobalPreemptibleStrategy which uses a 2-second timeout.
Suggested direction
- r.acquire();
+ if (!r.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT, TimeUnit.SECONDS)) {
+ return false;
+ }
...
- rateLimiter.acquire();
- return true;
+ return rateLimiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT, TimeUnit.SECONDS);Also applies to: 47-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/GlobalRateLimiter.java`
around lines 34 - 45, The current GlobalRateLimiter.acquire(RuntimeData) uses
unbounded blocking calls (r.acquire() and rateLimiter.acquire()) which can stall
threads; change both acquires to bounded/time-limited waits (e.g., tryAcquire
with a timeout like the 2-second pattern used in GlobalPreemptibleStrategy) and
return false when a timeout occurs; update the per-IP loader path (loadIpLimiter
-> r) and the global rateLimiter usage to use the timed-try-acquire pattern and
ensure both success checks are handled (and mirror the same timeout behavior for
the other acquire overload referenced around lines 47-51).
Comment on lines
+29
to
+36
| public boolean acquire(String ip) { | ||
| RateLimiter limiter = loadLimiter(ip); | ||
| if (limiter == null) { | ||
| return false; | ||
| } | ||
| limiter.acquire(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Uninterruptible blocking in acquire() may tie up threads indefinitely.
Similar to QpsStrategy, calling limiter.acquire() on Line 34 blocks without a timeout and cannot be interrupted. Under sustained per-IP overload, request-handling threads may wait indefinitely.
Consider using limiter.tryAcquire(timeout, TimeUnit) with interrupt handling to bound the wait time and allow graceful shutdown.
🔒 Proposed fix to add timeout-based acquisition
+ public static final int DEFAULT_ACQUIRE_TIMEOUT = 2;
+
public boolean acquire(String ip) {
RateLimiter limiter = loadLimiter(ip);
if (limiter == null) {
return false;
}
- limiter.acquire();
- return true;
+ try {
+ return limiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT, java.util.concurrent.TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ logger.warn("acquire permit interrupted for IP {}: {}", ip, e.getMessage());
+ Thread.currentThread().interrupt();
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/IPQpsStrategy.java`
around lines 29 - 36, In IPQpsStrategy.acquire, avoid uninterruptible blocking
by replacing limiter.acquire() with a bounded, interruptible attempt: call
limiter.tryAcquire(timeout, TimeUnit) (choose a sensible timeout) and handle
InterruptedException by restoring the thread interrupt status and returning
false; if tryAcquire returns false (timeout) return false as well. Locate this
logic in the acquire(String ip) method (and use loadLimiter(ip) and the limiter
variable) and ensure the method returns true only when tryAcquire succeeds.
Comment on lines
+33
to
+36
| public boolean acquire() { | ||
| rateLimiter.acquire(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Uninterruptible blocking in acquire() may tie up threads indefinitely.
rateLimiter.acquire() blocks without a timeout and cannot be interrupted. If permits are scarce, Netty I/O or gRPC executor threads may wait indefinitely, risking thread starvation under sustained load.
Consider using tryAcquire(timeout, TimeUnit) with interrupt handling (as in GlobalPreemptibleStrategy.acquire()), or document that QpsStrategy is only suitable when overload shedding is managed at a higher level.
🔒 Proposed fix to add timeout-based acquisition with interrupt handling
+ public static final int DEFAULT_ACQUIRE_TIMEOUT = 2;
+
public boolean acquire() {
- rateLimiter.acquire();
- return true;
+ try {
+ return rateLimiter.tryAcquire(DEFAULT_ACQUIRE_TIMEOUT, java.util.concurrent.TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ logger.error("acquire permit interrupted: {}", e.getMessage());
+ Thread.currentThread().interrupt();
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@framework/src/main/java/org/tron/core/services/ratelimiter/strategy/QpsStrategy.java`
around lines 33 - 36, QpsStrategy.acquire() currently calls
rateLimiter.acquire() which blocks uninterruptibly; change it to use a
timeout-based acquisition similar to GlobalPreemptibleStrategy.acquire(): call
rateLimiter.tryAcquire(timeout, TimeUnit.MILLISECONDS) (choose a sensible
timeout), handle InterruptedException by restoring the thread interrupt status
and returning false, and return the boolean result of tryAcquire so callers can
detect failure instead of blocking forever; update the QpsStrategy.acquire()
implementation and any callers/tests expecting unconditional true.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/integration-test.yml">
<violation number="1" location=".github/workflows/integration-test.yml:51">
P2: This step unconditionally requires secrets in a `pull_request` workflow, which will fail for fork PRs. Guard fork PRs (or skip the private-image path) so external PRs don't hard-fail before tests.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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
4 participants
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
.github/workflows/integration-test.ymltriggered on PR (open/sync/reopen) todevelopandrelease_**.FullNode.jarwith JDK 8, pullswarku123/java-tron-integration-test:latest, runs--smoke --cleanagainst the freshly-built jar.docker cpand uploaded as a build artifact.JAVA_HOMEoverridden inside the container to JDK 8 (production parity forjava.versionassertions);JAVA_HOME_17keeps Gradle on JDK 17.Test plan
integration-smoke-reportartifact containsreports/,test-results/, andnode.logSummary by cubic
Adds a CI workflow that builds
FullNode.jarwith JDK 8 and runs the full single-node tests on PRs. Also adds a blocking/non-blocking switch for API rate limiting, fixes--esstartup ordering, and rejects old event‑plugin versions.New Features
develop/release_**and manual dispatch; cancels duplicates; buildsFullNode.jar; pulls and runstroninfra/troninfra-ci:latestwith--clean(no DockerHub login); uploadsreports/,test-results/,test-output.log,node.log, andtron.logasintegration-test-report; caches Gradle; 45‑min timeout.rate.limiter.apiNonBlocking(default false) to choose blocking vs immediate-reject; introducesacquirePermiton per-endpoint/global limiters; preserves per-endpoint‑before‑global and IP‑before‑global ordering; tests cover routing and failure cases.event-pluginversion >= 3.0.0 at startup.Bug Fixes
--esnow applies after config so event plugin config builds when only the CLI flag enables subscribe.event.subscribe.native.useNativeQueuerestored tofalse.PeerConnection.Written for commit ca60f0f. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Chores
New Features
Bug Fixes / Stability
Tests