feat: configurable webhook timeout#365
feat: configurable webhook timeout#365GHkrishna wants to merge 2 commits intofeat/oidc-main-syncfrom
Conversation
Signed-off-by: Krishna Waske <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request adds a new Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
GHkrishna
added
the
enhancement
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/events/WebhookEvent.ts`:
- Around line 5-12: The timeout resolution in sendWebhookEvent trusts
parseInt(process.env.WEBHOOK_TIMEOUT_MS) and can yield negative or NaN values;
validate the resolved timeoutMs inside sendWebhookEvent (or at point of
defaulting) to ensure it's a positive integer and if not, fall back to
DEFAULT_TIMEOUT. Specifically, after obtaining timeoutMs (from the parameter or
parsed env), check that it's a finite number >= 0 (or > 0 per desired semantics)
and replace invalid values with DEFAULT_TIMEOUT before using it for the
request/abort logic so negative env values cannot cause immediate aborts.
- Line 14: Replace the direct console.log call in WebhookEvent (the line using
console.log(`Sending webhook event to ${webhookUrl} with timeout of
${timeoutMs}ms`)) with the injected logger by calling logger.info with the same
message; update any imports or class constructor if needed to ensure the logger
instance used by the WebhookEvent.send/sendEvent (or whichever method contains
that console.log) is the injected logger so linting and structured logging are
used consistently.
🪄 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: 61667e3a-8691-4ce1-9ef1-a537b1bd0fcc
📒 Files selected for processing (2)
.env.demosrc/events/WebhookEvent.ts
| const DEFAULT_TIMEOUT = 5000; | ||
|
|
||
| export const sendWebhookEvent = async ( | ||
| webhookUrl: string, | ||
| body: Record<string, unknown>, | ||
| logger: Logger, | ||
| timeoutMs = 5000, | ||
| timeoutMs: number = parseInt(process.env.WEBHOOK_TIMEOUT_MS || '', 10) || DEFAULT_TIMEOUT, | ||
| ): Promise<void> => { |
There was a problem hiding this comment.
Validate resolved timeout before using it.
Line 11 currently trusts parsed env values too much; a negative WEBHOOK_TIMEOUT_MS will cause near-immediate aborts. Since several event modules call this function without an explicit timeout, this can break webhook delivery globally.
💡 Proposed fix
-const DEFAULT_TIMEOUT = 5000;
+const DEFAULT_TIMEOUT = 5000
export const sendWebhookEvent = async (
webhookUrl: string,
body: Record<string, unknown>,
logger: Logger,
- timeoutMs: number = parseInt(process.env.WEBHOOK_TIMEOUT_MS || '', 10) || DEFAULT_TIMEOUT,
+ timeoutMs?: number,
): Promise<void> => {
-
- console.log(`Sending webhook event to ${webhookUrl} with timeout of ${timeoutMs}ms`)
+ const envTimeout = parseInt(process.env.WEBHOOK_TIMEOUT_MS ?? '', 10)
+ const candidateTimeout = timeoutMs ?? envTimeout
+ const resolvedTimeout = candidateTimeout > 0 ? candidateTimeout : DEFAULT_TIMEOUT
// Abort the webhook send events if the request hangs-in for >5 secs
// This can avoid failure of services due to bad webhook listners
const controller = new AbortController()
- const timeout = setTimeout(() => controller.abort(), timeoutMs)
+ const timeout = setTimeout(() => controller.abort(), resolvedTimeout)Also applies to: 18-18
🧰 Tools
🪛 ESLint
[error] 5-5: Delete ;
(prettier/prettier)
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 11-11: Prefer Number.parseInt over parseInt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/events/WebhookEvent.ts` around lines 5 - 12, The timeout resolution in
sendWebhookEvent trusts parseInt(process.env.WEBHOOK_TIMEOUT_MS) and can yield
negative or NaN values; validate the resolved timeoutMs inside sendWebhookEvent
(or at point of defaulting) to ensure it's a positive integer and if not, fall
back to DEFAULT_TIMEOUT. Specifically, after obtaining timeoutMs (from the
parameter or parsed env), check that it's a finite number >= 0 (or > 0 per
desired semantics) and replace invalid values with DEFAULT_TIMEOUT before using
it for the request/abort logic so negative env values cannot cause immediate
aborts.
| timeoutMs: number = parseInt(process.env.WEBHOOK_TIMEOUT_MS || '', 10) || DEFAULT_TIMEOUT, | ||
| ): Promise<void> => { | ||
|
|
||
| console.log(`Sending webhook event to ${webhookUrl} with timeout of ${timeoutMs}ms`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify which logger levels are used in the repo so replacement matches existing conventions.
rg -n --type=ts -C2 '\blogger\.(trace|debug|info|warn|error)\('
# Inspect local logger-related typings/usages to confirm available methods.
rg -n --type=ts -C2 'type Logger|interface Logger|from .+Logger'Repository: credebl/agent-controller
Length of output: 12863
🏁 Script executed:
cat -n src/events/WebhookEvent.tsRepository: credebl/agent-controller
Length of output: 1597
Replace console.log with logger.info.
Line 14 uses direct console logging which triggers no-console linting and can leak webhook URLs into unmanaged logs. Replace with the injected logger for structured, policy-controlled logging:
Replace with:
logger.info(`Sending webhook event to ${webhookUrl} with timeout of ${timeoutMs}ms`)🧰 Tools
🪛 ESLint
[error] 14-14: Unexpected console statement.
(no-console)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/events/WebhookEvent.ts` at line 14, Replace the direct console.log call
in WebhookEvent (the line using console.log(`Sending webhook event to
${webhookUrl} with timeout of ${timeoutMs}ms`)) with the injected logger by
calling logger.info with the same message; update any imports or class
constructor if needed to ensure the logger instance used by the
WebhookEvent.send/sendEvent (or whichever method contains that console.log) is
the injected logger so linting and structured logging are used consistently.
Signed-off-by: Krishna Waske <[email protected]>
|
1 participant
What
Summary by CodeRabbit