improvement(condition): batch condition evaluation into one sandbox call, and stop the transport from undercutting a route's own deadline - #6854
Conversation
…ion budget A `timeout` param bounds the work an internal route was asked to do — the code a sandbox runs, the upstream call a proxy route makes. The fetch around it also pays authentication, body parsing, workspace authorization, worker acquisition, and response serialization, none of which that budget was sized for. Arming the client with the bare number made the caller give up at the same instant the route's own deadline fired, so the route could never win the race and report which part actually ran long — the caller saw an unattributable `Request timed out` instead of `Function execution timed out after 5000ms`. Add 30s of headroom, sized above the isolated-vm worker's own 10s startup budget so a cold worker spawn stays inside the transport deadline rather than aborting it. An execution abort signal, when present, still bounds the call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A condition block spent one `function_execute` round trip per branch, so a four-branch block that fell through to `else` paid four sandbox executions before routing. Build one script that tests each expression in order and returns the index of the first truthy one. Ordering and short-circuiting are unchanged: an expression is only reached once every earlier one returned falsy, so a later expression that throws is still never reached and the run takes the same branch it took before. The script's `catch` reports the index it was on as data rather than rethrowing, which is what lets the handler still name the failing branch in its error. A batch that produces no verdict falls back to one call per branch — the path this handler used before. That is load-bearing rather than redundant: a syntax error anywhere in the list fails the whole script at parse time, while evaluating one at a time only reaches, and so only fails on, the branches the run actually takes. A timed-out or cancelled batch skips the fallback, which would otherwise re-run every branch against the same stall. An unrecognized reply is treated as no verdict rather than as "nothing matched", so a garbled response cannot silently route the run down the else path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Expression wrapping uses a shared For internal tool routes, when a Reviewed by Cursor Bugbot for commit 3c3ad93. Configure here. |
Greptile SummaryThe PR batches ordered condition evaluation into one sandbox request while retaining per-branch fallback for responses without a usable verdict, and adds transport headroom beyond internal routes’ own execution budgets.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported evaluation-context typing issue is fixed at HEAD.
|
| Filename | Overview |
|---|---|
| apps/sim/executor/handlers/condition/condition-handler.ts | Batches condition expressions into one ordered sandbox script, validates structured verdicts, and preserves individual fallback for recoverable failures. |
| apps/sim/executor/handlers/condition/condition-handler.test.ts | Updates existing mocks for batched verdicts and covers ordering, fallback, malformed responses, timeout, cancellation, comments, and error redaction. |
| apps/sim/tools/index.ts | Adds 30 seconds of headroom between an explicit internal-route work timeout and the surrounding fetch deadline. |
| apps/sim/tools/index.test.ts | Verifies that a 5-second route budget leaves the transport active until its new 35-second deadline. |
Sequence Diagram
sequenceDiagram
participant Handler as Condition Handler
participant Tool as executeTool
participant Route as Internal Function Route
participant Sandbox as Sandbox
Handler->>Tool: Batched condition script (5s work budget)
Tool->>Route: Fetch (35s transport deadline)
Route->>Sandbox: Execute ordered expressions
alt Usable verdict
Sandbox-->>Route: matchedIndex / no-match / threwAtIndex
Route-->>Handler: Structured result
else Recoverable no-verdict
Route-->>Handler: Failure or unrecognized result
Handler->>Tool: Evaluate branches individually
else Timeout or cancellation
Route-->>Handler: Timeout/cancellation failure
Handler-->>Handler: Surface failure without fallback fan-out
end
Reviews (2): Last reviewed commit: "fix(executor): wrap condition expression..." | Re-trigger Greptile
The batched script put each expression on its own line inside `Boolean(...)`
so a trailing line comment ended before the closing parenthesis; the per-branch
fallback still inlined `Boolean(${expression})` on one line. That made the
recovery path stricter than the path it recovers — a batch that failed to parse
because of a later branch would fall back and then reject an earlier
comment-bearing branch it should have matched.
Both paths now wrap through `buildBooleanTest`, so they cannot drift again.
Also narrows the evaluation-context boundary from `Record<string, any>` to
`Record<string, unknown>`; the context is only ever serialized, never indexed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3c3ad93. Configure here.
What
Two changes to the same failure, from opposite ends.
apps/sim/executor/handlers/condition/condition-handler.ts— a condition block spent onefunction_executeround trip per branch, so a four-branch block that fell through toelsepaid four sandbox executions before it could route. It now builds one script that tests each expression in order and returns the index of the first truthy one.apps/sim/tools/index.ts— atimeoutparam bounds the work an internal route was asked to do (the code a sandbox runs, the upstream call a proxy makes). The fetch around it also pays authentication, body parsing, workspace authorization, worker acquisition, and response serialization, none of which that budget was sized for. Arming the client with the bare number made the caller give up at the same instant the route's own deadline fired, so the route could never win its own race — you saw an unattributableRequest timed outinstead ofFunction execution timed out after 5000ms. Adds 30s of transport headroom, sized above the isolated-vm worker's own 10s startup budget so a cold worker spawn stays inside the deadline rather than aborting it.Behavior that is deliberately unchanged
catchreports the index it was on as data rather than rethrowing, so the handler still throwsEvaluation error in condition "<title>"naming the branch that actually failed.elsehandling. Anelsewins as soon as it is reached, so only the branches ahead of it are testable — matching the original loop, which returned on the firstelseit walked past rather than assumingelsecomes last. Theelseexpression never reaches the sandbox.The fallback, and why it is load-bearing
A batch that produces no verdict falls back to one call per branch — exactly what this handler did before. That is not a competing second system kept for comfort: a syntax error anywhere in the list fails the whole script at parse time, whereas evaluating one at a time only reaches, and so only fails on, the branches the run actually takes. Without it, a workflow with a broken later branch that always matches an earlier one would newly start failing. There is no way to isolate a parse failure per-branch inside a single script, so the second pass is the only recovery.
Two failures skip the fallback, because retrying would make them worse rather than recover them:
Both surface the batch failure as it stands. The whole list was one call, so no single branch owns that failure; the error names the first, where evaluation started.
An unrecognized reply is treated as no verdict rather than as "nothing matched", so a garbled response cannot silently route the run down the
elsepath.Tests
apps/sim/executor/handlers/condition/condition-handler.test.ts— the existing suite was rewritten onto the batched verdict shape (one call, one verdict) and pins the new behavior: a whole list in a single call, declaration order preserved in the emitted script, theelsebranch never sent, per-branch fallback on a no-verdict batch, no fan-out on a timeout or a cancelled run, an out-of-range index falling back, a garbled reply not taking theelsepath, and no secret leaking into logs on either failure path.apps/sim/tools/index.test.ts— asserts the transport is still waiting at the instant the route's own 5s budget expires, and gives up at 35s.Verification
bun run teston both files: 204 passedbun run type-check(apps/sim): cleanbiome checkon all four files: cleanbun run check:api-validation: passesKnown tradeoffs
CONDITION_TIMEOUT_MSinstead of 5s per branch. Conditions are inlined boolean expressions with no I/O, so this is not a budget any real list approaches.isTimeoutFailureclassifies by message text, the same wayisRetryableFailuredoes in@/tools. A sandbox syntax error whose message happened to contain "timeout" would skip the fallback and fail a run that would otherwise have matched an earlier branch. It needs the user's own condition source to contain that substring and a syntax error in the list.🤖 Generated with Claude Code