fix(agents): wait for a terminal status in getResult() - #156
Draft
ling-senpeng13 wants to merge 1 commit into
Draft
fix(agents): wait for a terminal status in getResult()#156ling-senpeng13 wants to merge 1 commit into
ling-senpeng13 wants to merge 1 commit into
Conversation
AgentRuntime.run() could return status "RUNNING" for an execution that had already finished on the server — wrong, not late: the failing calls returned in 2-4s, nowhere near any timeout. run() takes its result from the SSE stream, and getResult() read the status endpoint once. The stream ending does not mean the workflow ended, so that single read can land mid-flight and the non-terminal status is returned verbatim. The code comment already said "poll the server for the real terminal status" — it just never polled. Most visible with guardrails: server-side the workflow reaches FAILED with reasonForIncompletion set, after the expected retry iterations and a TERMINATE task, while the SDK reports RUNNING — a guardrail that worked perfectly looks like it never fired, and callers branching on result.status take the wrong branch. Read until the status is terminal, bounded by RESULT_SETTLE_TIMEOUT_MS (30s) so a long-running execution still returns rather than hanging. Reuses the existing TERMINAL_STATUSES set, so this path now agrees with wait(), liveness, and schedules. Only a *successful but non-terminal* read is retried. An unreachable or erroring endpoint returns immediately with what was seen last — preserving the previous behaviour on that path, so a broken endpoint cannot burn the settle budget. The non-streaming path never had this bug: _pollForCompletion() already loops until isComplete. That asymmetry was the defect. Tests: three regression cases — a RUNNING/RUNNING/FAILED sequence resolving to FAILED (the reported bug), a terminal-first status doing exactly one read, and an erroring endpoint falling back to stream inference in one read. Verified end to end against the 4.0.0-rc4 e2e bundle on a live server, with streaming enabled and no server change: Suite 8 goes from 3 failed / 4 passed to 7 passed / 7. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #155.
Problem
AgentRuntime.run()could returnstatus: "RUNNING"for an execution that had already reached a terminal state on the server. Wrong, not late — the failing calls return in 2–4 seconds, nowhere near any timeout.run()takes its result from the SSE stream, andAgentStream.getResult()read the status endpoint once. The stream ending does not mean the workflow ended, so that single read can land mid-flight and the non-terminal status becomes the returned result. The comment above it already said "Poll the server for the real terminal status" — it just never polled.It shows up most clearly with guardrails. Server-side the workflow reaches
FAILEDwithreasonForIncompletionset, after the expected retry iterations and aTERMINATEtask, andGET /agent/{id}/statusreturnsFAILED:The SDK reported
RUNNING. A guardrail that worked perfectly looks like it never fired, and callers branching onresult.statustake the wrong branch.Change
getResult()now reads until the status is terminal, via a new_fetchTerminalStatus():TERMINAL_STATUSESset, so this path agrees withwait(),liveness, andschedulesrather than inventing its own notion of "done".RESULT_SETTLE_TIMEOUT_MS(30s). On expiry it returns the last status seen rather than throwing — callers get the best available answer, never a hang. This bounds reconciliation only; it is not a run timeout.The non-streaming path never had this bug:
_pollForCompletion()already loops untilisComplete. That asymmetry between the two paths was the defect, and this closes it.Tests
Three regression cases in
src/agents/__tests__/stream.test.ts:RUNNING→RUNNING→FAILEDFAILEDwith the righterror; 3 status reads. ReturnedRUNNINGbefore this change.COMPLETED; exactly 1 read — no added latency for the common casesrc/agents/__tests__/stream.test.tspasses 25/25. The settle test takes ~1s of real time, confirming it genuinely polled rather than short-circuiting.End-to-end verification
Built the SDK, installed it into the released
conductor-ai-e2e-typescript-4.0.0-rc4bundle, and ran against a live server with streaming enabled and no server-side change:Suite 8: agent output secrets blockedstatus=RUNNINGSuite 8: max_retries escalationstatus=RUNNINGSuite 8: tool input raise — SQL injection blockedAnd the complete 196-test suite, run twice with identical results, to check for
regressions:
Nothing that passed on stock fails on this branch.
A note on the third guardrail test
tool input raise — SQL injection blockedpasses when run in isolation but fails in a fullrun, so I initially took it for contention and said in #155 that it was not part of this
bug. That was wrong — this branch fixes it too, across both full runs.
It is the same race with a different hit rate: under full-suite load the workflow runs longer
relative to stream close, so the single status read lands mid-flight more often. Worth knowing
because it means the bug's visibility scales with load, and a green isolated run is not
evidence that a caller is safe.
Scope
Draft, and deliberately narrow — it fixes the reported status-reporting race only.
I checked whether it also helps the four
Suite 16streaming failures on the same setup. It does not — they fail identically before and after, so they are a separate problem and are not addressed here.One pre-existing issue I hit, unrelated to this change:
npm run test:unitcrashes on Node 26 withReferenceError: clearTimeout is not definedinsrc/sdk/clients/worker/Poller.ts:76. It reproduces on a clean checkout ofmain. I ran thestream.test.tssuite directly instead. Happy to file it separately.