fix(serverless-hono): withWaitUntil must not destroy global state when context has no waitUntil#1214
Conversation
…n context has no waitUntil
Two bugs in withWaitUntil:
1. The else branch unconditionally set ___voltagent_wait_until = undefined
even when the global already held a value from an outer scope. This
silently killed background tasks (observability, logging) in middleware
chains where an inner handler passed {} as context.
2. Errors thrown by context.waitUntil (e.g. DataCloneError in Cloudflare
Workers) propagated to callers. The test suite expected them to be
swallowed; wrapping the call in try/catch aligns implementation with
that contract.
Fix:
- Remove the else branch entirely. When context has no waitUntil the
global is left untouched.
- Wrap the bound waitUntil call in try/catch to swallow errors.
- Simplify the cleanup function to always restore previousWaitUntil
unconditionally (no currentWaitUntil guard needed).
All 11 tests in wait-until-wrapper.spec.ts now pass.
Fixes VoltAgent#1203
🦋 Changeset detectedLatest commit: b91b5d5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/serverless-hono/src/utils/wait-until-wrapper.ts (1)
35-46: LGTM — correctly guards global state and swallows invocation errors.Binding before wrapping avoids
Illegal invocation, and thetry/catchinside the wrapper (rather than around.bind) correctly catches per-call failures likeDataCloneError. Removing theelsebranch preserves any outer-scope global, matching the fix for#1203.One optional consideration: fully silent swallowing can hide real bugs during development. The sibling
flushOnFinishinvolt-agent-observability.tslogs aconsole.warnwhenwaitUntilthrows; you may want a similar debug-level log here for symmetry, though it's not required for correctness.Optional: log swallowed errors for observability parity
globals.___voltagent_wait_until = (promise: Promise<unknown>) => { try { boundWaitUntil(promise); - } catch { - // Swallow errors to avoid breaking the caller + } catch (error) { + // Swallow errors (e.g. DataCloneError) to avoid breaking the caller + // eslint-disable-next-line no-console + console.warn("[voltagent] waitUntil invocation failed", error); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/serverless-hono/src/utils/wait-until-wrapper.ts` around lines 35 - 46, The wrapper for globals.___voltagent_wait_until currently swallows errors silently; update the catch block in wait-until-wrapper.ts (inside the if checking currentWaitUntil and after binding via currentWaitUntil.bind(context)) to emit a debug/console.warn log including the caught error and a short context message (e.g., that waitUntil invocation failed) before swallowing, referencing globals.___voltagent_wait_until, boundWaitUntil and context so logs mirror the observability behavior used by flushOnFinish in volt-agent-observability.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/serverless-hono/src/utils/wait-until-wrapper.ts`:
- Around line 35-46: The wrapper for globals.___voltagent_wait_until currently
swallows errors silently; update the catch block in wait-until-wrapper.ts
(inside the if checking currentWaitUntil and after binding via
currentWaitUntil.bind(context)) to emit a debug/console.warn log including the
caught error and a short context message (e.g., that waitUntil invocation
failed) before swallowing, referencing globals.___voltagent_wait_until,
boundWaitUntil and context so logs mirror the observability behavior used by
flushOnFinish in volt-agent-observability.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 150ec58a-50a5-4ffe-a24a-3a0f9901c2e0
📒 Files selected for processing (1)
packages/serverless-hono/src/utils/wait-until-wrapper.ts
![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="packages/serverless-hono/src/utils/wait-until-wrapper.ts">
<violation number="1" location="packages/serverless-hono/src/utils/wait-until-wrapper.ts:52">
P2: Cleanup unconditionally restores a stale global snapshot even when this call never set the global, allowing interleaved executions to be clobbered.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| globals.___voltagent_wait_until = undefined; | ||
| } | ||
| } | ||
| globals.___voltagent_wait_until = previousWaitUntil; |
There was a problem hiding this comment.
P2: Cleanup unconditionally restores a stale global snapshot even when this call never set the global, allowing interleaved executions to be clobbered.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/serverless-hono/src/utils/wait-until-wrapper.ts, line 52:
<comment>Cleanup unconditionally restores a stale global snapshot even when this call never set the global, allowing interleaved executions to be clobbered.</comment>
<file context>
@@ -34,20 +34,21 @@ export function withWaitUntil(context?: WaitUntilContext | null): () => void {
- globals.___voltagent_wait_until = undefined;
- }
- }
+ globals.___voltagent_wait_until = previousWaitUntil;
};
}
</file context>
3 participants
Fixes #1203
Problem
withWaitUntilin@voltagent/serverless-honohad two bugs:Global state destruction: The
elsebranch unconditionally setglobals.___voltagent_wait_until = undefinedwhenevercontext.waitUntilwas absent. This silently destroyed any value previously set by an outer scope, breaking background tasks (observability, logging) in middleware chains where an inner handler passed{}as context.Error propagation: Errors thrown by
context.waitUntil(e.g.DataCloneErrorin Cloudflare Workers) propagated to callers. The test suite expected these to be swallowed, but the implementation let them through.Additionally, the cleanup function only restored the previous
waitUntilwhencurrentWaitUntilwas truthy, meaning cleanup was a no-op when called with a context that had nowaitUntil— leaving the global state permanently altered.Solution
elsebranch entirely: whencontexthas nowaitUntil, the global is left untouched.waitUntilcall intry/catch: errors from the underlying implementation (likeDataCloneError) are now swallowed so callers are not affected.previousWaitUntilunconditionally, regardless of whethercurrentWaitUntilwas set.Testing
All 11 tests in
wait-until-wrapper.spec.tsnow pass, including the two that previously failed:should handle errors from context.waitUntil gracefullyshould not affect global state when context has no waitUntilSummary by cubic
Fixes global waitUntil handling in
@voltagent/serverless-hono’swithWaitUntil. It no longer clears outer global state when a context lackswaitUntil, swallows underlying errors, and always restores the previous global value.context.waitUntilis missing.waitUntilin try/catch to swallow errors (e.g., Cloudflare Workers’DataCloneError).waitUntil.Written for commit b91b5d5. Summary will update on new commits.
Summary by CodeRabbit