fix(v2): await run log finalization before sync execute responds - #8369
lakshya-dhariwal wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
| // the log (status, endedAt) and cost ledger as soon as it lands. The | ||
| // core defers that finalization into the session's post-execution | ||
| // promise, so hold the response until the log is final. | ||
| await loggingSession.waitForPostExecution() |
There was a problem hiding this comment.
Finalization wait has no deadline If a finalization database write stalls, the sync response can remain open indefinitely. This wait runs after the execution timeout is cleaned up and the request-abort listener is removed, so neither can stop it. Consider bounding the wait or stopping it when the request is no longer live.
Knowledge Base Used: Application platform
There was a problem hiding this comment.
Not taking this one. Bounding the wait would reintroduce #8354 through the back door: if the deadline fires first, the response returns completed while the log row is still unfinalized, which is the exact race this PR removes. The same unbounded await already exists on the v1 path (execute-workflow.ts awaits waitForPostExecution() in its own finally), so this matches the established pattern. A genuinely stalled finalization write now surfaces as a slow request instead of a log row stuck on running forever, which is the more debuggable failure mode.
There was a problem hiding this comment.
That’s a fair correction. Given the sync endpoint’s terminal-receipt contract, bounding this await would indeed recreate the original race whenever finalization outlives the deadline. The v1 path already uses the same unbounded waitForPostExecution() in its finally, so matching that behavior is consistent. A slow finalization delaying the response is preferable to acknowledging a run whose durable log is still running; I’m withdrawing this concern.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| expect(responded).toBe(false) | ||
|
|
||
| releaseFinalization() |
There was a problem hiding this comment.
Test can miss its gate The fixed 10 ms delay does not establish that execution reached the finalization gate. If the mocked route takes longer, the assertion passes without testing the gate and
releaseFinalization() then fails because it has not been assigned. Wait for entry to the gate before asserting or releasing it.
There was a problem hiding this comment.
Good catch, fixed in 5d98041. The test now waits on vi.waitFor for waitForPostExecution to be entered before asserting the response is held, so it can no longer pass without reaching the gate.
| }) | ||
|
|
||
| // Let the mocked execution settle up to the finalization gate. | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) |
There was a problem hiding this comment.
Inline delay violates shared-utility rule This test implements a delay with
new Promise and setTimeout, but the repository requires the shared sleep(ms) helper from @sim/utils/helpers instead. If a timed delay remains after the test is synchronized, replace it with that helper before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Resolved by the same commit - the inline setTimeout is gone. The test synchronizes on vi.waitFor against the mock instead of a timed delay.
The v2 sync execute route returned 'completed' while the run log was still being finalized: execution-core defers status/endedAt/cost persistence into the logging session's post-execution promise, and nothing on the service's sync path awaited it. A GET on the run's log right after the response could still show status 'running', a null endedAt, and missing cost items. Hold the sync response until the post-execution promise settles, so the terminal response doubles as a durable receipt. Async (queued) runs are unaffected; they already return before execution starts. fixes simstudioai#8354
dfd7f03 to
5d98041
Compare
Summary
Fixes #8354. The v2 sync execute route could return
completedwhile the run's log row was still being finalized:executeWorkflowCoredefers status/endedAt/cost persistence into the logging session's post-execution promise, and the sync path inexecuteWorkflowServicereturned without awaiting it. AGET /api/v2/logs/{runId}right after the response could still showstatus: "running", a nullendedAt, and missing cost items.The sync response now waits for the post-execution promise to settle before it is returned, so a terminal response doubles as a durable receipt. Async (queued) runs are unaffected: they already return before execution starts.
Type of Change
Testing
holds the sync response until post-execution logging finalizes: gateswaitForPostExecutionon a manually released promise and asserts the response is not sent before release. Verified it fails without the fix and passes with it.vitest run app/api/v2/workflows/[workflowId]/execute/route.test.ts: 39/39 pass.biome checkon the changed files: clean.finallyinrunSynchronousWorkflow(apps/sim/lib/workflows/executor/execute-service.ts): the await covers both the success and error terminal results, and is a no-op whenever the core never set a post-execution promise.Checklist
Screenshots/Videos
N/A - server-side API behavior change, covered by the new test.