Skip to content

fix(v2): await run log finalization before sync execute responds #8369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lakshya-dhariwal wants to merge 1 commit into
base: staging
Choose a base branch
from
+39 −0
Open

fix(v2): await run log finalization before sync execute responds #8369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions apps/sim/app/api/v2/workflows/[workflowId]/execute/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
executionPreprocessingMock,
executionPreprocessingMockFns,
loggingSessionMock,
loggingSessionMockFns,
resetDbChainMock,
setEnv,
workflowAuthzMockFns,
Expand Down Expand Up @@ -329,6 +330,39 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => {
})
})

it('holds the sync response until post-execution logging finalizes', async () => {
let releaseFinalization!: () => void
loggingSessionMockFns.mockWaitForPostExecution.mockImplementationOnce(
() =>
new Promise<void>((resolve) => {
releaseFinalization = resolve
})
)

let responded = false
const pending = callExecute({ input: { hello: 'world' } }).then((res) => {
responded = true
return res
})

// Wait until execution is actually parked at the finalization gate, then
// prove the gate is what holds the response back.
await vi.waitFor(() =>
expect(loggingSessionMockFns.mockWaitForPostExecution).toHaveBeenCalled()
)
expect(responded).toBe(false)

releaseFinalization()
const res = await pending
expect(res.status).toBe(200)
const body = await res.json()
expect(body.data).toMatchObject({
runId: 'execution-123',
status: 'completed',
output: { result: 'done' },
})
})

it('streams an immediate heartbeat and the same sync result when NDJSON is accepted', async () => {
vi.useFakeTimers()
try {
Expand Down
5 changes: 5 additions & 0 deletions apps/sim/lib/workflows/executor/execute-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,11 @@ export async function executeWorkflowService(
} finally {
abortSignal?.removeEventListener('abort', abortFromRequest)
timeoutController.cleanup()
// The sync response doubles as the run's terminal receipt: callers read
// 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()

@greptile-apps greptile-apps Bot Sep 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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

@lakshya-dhariwal lakshya-dhariwal Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@greptile-apps greptile-apps Bot Sep 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down