fix(java): harden outcome dispatch and webhook delivery#361
Merged
Conversation
Zero-delay retries burst a down endpoint; now 500ms doubling to 30s cap, matching the Node SDK, with a warning on exhaustion.
store.load() did a blocking native read + JSON parse per job outcome on the single drain thread, even with zero webhooks.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds exponential-backoff retry with URI redaction to webhook delivery, introduces a TTL-based cache for active webhooks in WebhookManager with per-hook delivery error isolation, and wraps middleware dispatch calls in WorkerDispatchBridge to prevent one failing middleware from blocking others, plus a corresponding test. ChangesDelivery reliability and fault isolation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookManager
participant Deliverer
participant TargetServer
WebhookManager->>Deliverer: deliver(request)
Deliverer->>TargetServer: sendWithRetry(attempt=0)
TargetServer-->>Deliverer: 5xx error / transport failure
Deliverer->>Deliverer: schedule delayed retry (backoff)
Deliverer->>TargetServer: sendWithRetry(attempt=1)
TargetServer-->>Deliverer: failure again
Deliverer->>Deliverer: retries exhausted, log warning with redact(URI)
sequenceDiagram
participant Worker
participant WorkerDispatchBridge
participant MiddlewareA
participant MiddlewareB
Worker->>WorkerDispatchBridge: onOutcome(event)
WorkerDispatchBridge->>MiddlewareA: dispatch(event)
MiddlewareA-->>WorkerDispatchBridge: throws RuntimeException
WorkerDispatchBridge->>WorkerDispatchBridge: log warning
WorkerDispatchBridge->>MiddlewareB: dispatch(event)
MiddlewareB-->>WorkerDispatchBridge: onCompleted (countDown)
Related issues: None referenced in the provided context. Related PRs: None referenced in the provided context. Suggested labels: reliability, webhooks, worker Suggested reviewers: None specified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
sdks/java/src/main/java/org/byteveda/taskito/webhooks/WebhookManager.java (1)
125-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse monotonic time for cache TTL checks.
Line 127 uses wall-clock time for elapsed TTL; a backward clock adjustment can keep stale webhook settings past the intended 30s bound. Prefer
System.nanoTime()for elapsed-duration checks.Proposed fix
private List<Webhook> activeHooks() { CachedHooks snapshot = cached; - long now = System.currentTimeMillis(); - if (snapshot == null || now - snapshot.loadedAt() > CACHE_TTL_MS) { + long now = System.nanoTime(); + if (snapshot == null || now - snapshot.loadedAt() > TimeUnit.MILLISECONDS.toNanos(CACHE_TTL_MS)) { snapshot = new CachedHooks(List.copyOf(store.load()), now); cached = snapshot; }+import java.util.concurrent.TimeUnit;🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/webhooks/WebhookManager.java` around lines 125 - 133, The cache expiration check in activeHooks() is using wall-clock time, which can be affected by system clock changes. Update the CachedHooks timestamping and TTL comparison logic to use a monotonic source like System.nanoTime() for elapsed-duration checks, while keeping the rest of WebhookManager.activeHooks() behavior unchanged. Make sure the cached loaded-at value and the now comparison are both based on the same monotonic time source so stale webhook settings cannot persist past CACHE_TTL_MS.
🤖 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 `@sdks/java/src/main/java/org/byteveda/taskito/webhooks/Deliverer.java`:
- Around line 57-64: The redact(URI) helper in Deliverer still logs the URI
path, which can expose secrets embedded in path segments. Update redact(URI) to
strip path components as well as credentials and query string, so the failure
warning only logs a non-sensitive URI form. Keep the fix localized to the redact
method used by Deliverer’s webhook logging.
- Around line 51-53: The backoff calculation in Deliverer.sendWithRetry should
clamp before shifting, because MAX_BACKOFF_MS is being applied after
`BASE_BACKOFF_MS << attempt` can overflow and produce negative or zero delays.
Update the delay computation to bound the effective shift/exponential growth
before it reaches overflow, while keeping the retry scheduling in
`CompletableFuture.delayedExecutor(...).execute(...)` capped at MAX_BACKOFF_MS.
In `@sdks/java/src/main/java/org/byteveda/taskito/webhooks/WebhookManager.java`:
- Line 119: The warning in Deliverer.deliver should not log the raw exception
object because URI.create(hook.url) can include the original URL in its
IllegalArgumentException message and leak tokens. Update the webhook failure
handling in WebhookManager so the LOG.warn call logs only a sanitized message
(or a redacted exception message) instead of e, while keeping the context around
hook.id and the delivery failure.
---
Nitpick comments:
In `@sdks/java/src/main/java/org/byteveda/taskito/webhooks/WebhookManager.java`:
- Around line 125-133: The cache expiration check in activeHooks() is using
wall-clock time, which can be affected by system clock changes. Update the
CachedHooks timestamping and TTL comparison logic to use a monotonic source like
System.nanoTime() for elapsed-duration checks, while keeping the rest of
WebhookManager.activeHooks() behavior unchanged. Make sure the cached loaded-at
value and the now comparison are both based on the same monotonic time source so
stale webhook settings cannot persist past CACHE_TTL_MS.
🪄 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: 160c3fe4-7719-46c1-8c95-edcce8f12a16
📒 Files selected for processing (4)
sdks/java/src/main/java/org/byteveda/taskito/webhooks/Deliverer.javasdks/java/src/main/java/org/byteveda/taskito/webhooks/WebhookManager.javasdks/java/src/main/java/org/byteveda/taskito/worker/WorkerDispatchBridge.javasdks/java/src/test/java/org/byteveda/taskito/WorkerTest.java
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.
Fixes the four medium-severity findings from the Java SDK audit follow-up list.
URI.createthrows) no longer kills delivery to later hooks; each delivery failure is logged and the loop continues../gradlew buildgreen (tests, checkstyle, spotless).Summary by CodeRabbit