Repository navigation
fix(client): retry a lost create only where the server deduplicates - #5363
Conversation
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||
cf8c5cc to
13119a3
Compare
createConversation surfaces a transport error when it loses the response. The caller cannot tell whether the create was applied, and replaying the POST risks a second conversation whose initial message runs twice. Re-send once, but only when the server advertises idempotent_conversation_create_v1 (added server-side in the sibling fix), and only when the caller supplied a conversation_id to deduplicate on. A server that cannot be probed is treated as unsupported, so an older backend keeps surfacing the transport error rather than risking a duplicate. Tests use a real HTTP server and assert the negative directly: against a server without the capability the create is not replayed, and the re-send is deduplicated rather than applied twice. Co-authored-by: openhands <openhands@all-hands.dev>
c0611e2 to
d22e5df
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
I checked out the exact head d22e5df2 and reviewed clients/typescript/src/client/conversation-client.ts and the new src/__tests__/conversation-create-retry.test.ts against the repo review guide and clients/typescript/AGENTS.md.
The implementation is correct and appropriately conservative. The retry is gated on three independent conditions, all of which are necessary:
error instanceof HttpErroris rejected first, so a real HTTP answer (including a 400 validation failure) is never concealed — the transport error fromHttpClient.requestis wrapped in a plainError, so the type check correctly distinguishes the two cases.typeof payload.conversation_id !== 'string'ensures there is a stable dedup key; a create with no caller-supplied id is not replayed (that would double-create).supportsCreateRetry()probes/server_infovia the existing per-HttpClientcache and returnsfalseon any probe failure, so an older or unreachable backend keeps surfacing the original transport error rather than risking a duplicate.
I ran the new suite and the full unit suite in the workspace:
npx vitest run --config vitest.config.ts src/__tests__/conversation-create-retry.test.ts-> 4 passed. The tests drive a realnode:httpserver, so they exercise the actual lost-response path rather than a stubbedfetch. The negative cases (capability absent ->creates === 1; noconversation_id-> not replayed; 400 surfaced) assert the safety properties directly.- Full
npx vitest run --config vitest.config.ts-> 334 passed (21 files), matching the PR's stated result.
Current-head CI is green for the substantive jobs: TypeScript client CI (agent-server-api, build, test, endpoint-audit, public-type-budget), Run tests, and TypeScript client integration tests all pass. Note this change is inert on main until #5362 (still open) advertises idempotent_conversation_create_v1, which is the intended safe behavior.
Blocker
One current-head check fails: Validate PR description (run 36551179202, job 109349459173), and the PR is BLOCKED for merge. The failure is:
##[error]Add a short human-written note between `HUMAN:` and `AGENT:`.
PR description validation failed with 1 error(s).
The HUMAN: section of the PR body still contains only the unfilled template comment. Repository policy (.github/workflows/pr-description-check.yml + .github/scripts/check_pr_description.py, MIN_HUMAN_NOTE_CHARS = 20) requires a human-written note there, and the AGENT section must not edit it. This is the only thing standing between this revision and approval; it needs the human author (@neubig), not an AI agent, to add the note. No code change is required.
Linked issue
#5035 is open, labeled ready-for-dev (carries enhancement, sdk, javascript), and its updated acceptance criteria now describe exactly this remedy (client re-send gated on the capability + a caller-supplied id; HTTP failures preserved). The head satisfies the client-side criteria; the server-side criterion lives in #5362.
No line-level findings to report on the changed files.
🔄 CHANGES REQUESTED
|
The remaining Validate PR description failure requires a short human-authored note between the protected |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
I re-fetched the current state and checked out the exact head d22e5df2 (unchanged). This is a fresh pass, warranted because @neubig commented after my earlier review. Scope is correct: this is the TypeScript client for the Agent Server API, owned by this repo per clients/typescript/AGENTS.md.
The implementation is correct and appropriately conservative. createConversation re-sends only when all three conditions hold:
error instanceof HttpErroris rejected first, so a real HTTP answer (including a 400 validation failure) is never concealed.HttpClient.requestwraps transport faults in a plainErrorand throwsHttpErroronly for a received non-OK response, so the type check correctly separates the two cases (verified inclients/typescript/src/client/http-client.ts).typeof payload.conversation_id !== 'string'ensures a stable dedup key; a create with no caller-supplied id is not replayed.supportsCreateRetry()probes/server_infovia the existing per-HttpClientcache (getCachedAgentServerInfo) and returnsfalseon any probe failure, so an older or unreachable backend keeps surfacing the original transport error instead of risking a duplicate. The capability stringidempotent_conversation_create_v1matches exactly what the server-side PR advertises inServerInfo.capabilities.
I ran the focused suite and the full unit suite against this head, plus the typecheck:
npx vitest run --config vitest.config.ts src/__tests__/conversation-create-retry.test.ts-> 4 passed. The tests drive a realnode:httpserver, so they exercise the actual lost-response path (a stubbedfetchwould not). The negative cases assert the safety properties directly: capability absent ->state.creates === 1; noconversation_id-> not replayed; 400 surfaced asHttpError.- Full
npx vitest run --config vitest.config.ts-> 334 passed (21 files), matching the PR's stated result. npx tsc --noEmit-> clean.
All current-head substantive checks pass (test (22.12), test (24.x), build, agent-server-api, endpoint-audit, public-type-budget, integration-test). The change is inert until #5362 (still open) advertises the capability, which is the intended safe default.
Blocker
One current-head check fails: Validate PR description (run 36551179202, job 109349459173), and the PR's mergeable_state is blocked:
##[error]Add a short human-written note between `HUMAN:` and `AGENT:`.
PR description validation failed with 1 error(s).
The HUMAN: region of the PR body still contains only the unfilled template comment. Repository policy (.github/workflows/pr-description-check.yml + .github/scripts/check_pr_description.py, MIN_HUMAN_NOTE_CHARS = 20) requires a human-written note there, and AI agents must not edit that section. @neubig already acknowledged this in a comment; it needs the human author to add the note. No code change is required — this is the only thing between this revision and approval.
Linked issue
#5035 is open with labels enhancement, sdk, javascript, ready-for-dev (still present). Its updated acceptance criteria describe exactly this remedy (client re-send gated on the server capability plus a caller-supplied id; explicit HTTP failures preserved). This head satisfies the client-side criteria; the server-side criterion lives in #5362.
No line-level findings on the changed files.
🔄 CHANGES REQUESTED
Resolved: transient server-info failures now recoverFixed in A rejected Added an observable regression test on one Validation:
This comment was updated by an AI agent (OpenHands) on behalf of Graham Neubig. |
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
I checked out the exact head 575511fe (a merge of main into the feature branch) and reviewed the four changed files against the repo guidance and clients/typescript/AGENTS.md. Scope is correct: this is the TypeScript client for the Agent Server API, owned by this repository, and it follows the SDK/Agent Server → OpenAPI → clients/typescript flow.
The core retry logic is correct and appropriately conservative. createConversation re-sends only when all three conditions hold:
error instanceof HttpErroris rejected first, so a real HTTP answer (including a 4xx validation failure) is never concealed.HttpClient.requestthrowsHttpErroronly for a received non-OK response and wraps transport faults in a plainError, so the type check correctly separates the two cases.typeof payload.conversation_id !== 'string'ensures a stable dedup key; a create with no caller-supplied id is not replayed.supportsCreateRetry()probes/server_infovia the cachedgetCachedAgentServerInfoand returnsfalseon any probe failure, so an older or unreachable backend keeps surfacing the original transport error rather than risking a duplicate. The capability stringidempotent_conversation_create_v1matches exactly what the server-side PR advertises inServerInfo.capabilities.
The follow-up cache change (agent-server-compatibility.ts) is also correct. Evicting a rejected /server_info promise from the WeakMap under an identity guard (serverInfoCache.get(client) === request) lets a later operation re-probe after a transient failure, while successful and concurrent in-flight probes stay cached. The void request.catch(...) cannot produce an unhandled rejection because the caller of getCachedAgentServerInfo still awaits and handles request; this is a strict improvement over the previous behavior, where a failed probe poisoned the cache until clearAgentServerInfoCache was called manually. The createConversation path remains fail-safe: supportsCreateRetry() still returns false on a failed probe, so no create is replayed without a confirmed capability.
Verification on this head
npx vitest run --config vitest.config.ts src/__tests__/conversation-create-retry.test.ts src/__tests__/api-clients.test.ts-> 91 passed (2 files), including the new transient-/server_info-failure regression, which asserts a secondlistWorkspaces()re-probes and succeeds.- Full
npx vitest run --config vitest.config.ts-> 351 passed. npx tsc --noEmit-> clean.
All current-head checks are green, including Validate PR description (now that a human note is present between the HUMAN: and AGENT: markers), test (22.12), test (24.x), build, agent-server-api, endpoint-audit, public-type-budget, integration-test, pre-commit, and the binary builds. No failing or pending checks remain.
Linked issue
#5035 is open, labeled enhancement, sdk, javascript, ready-for-dev. Its updated acceptance criteria describe exactly this remedy, and this head satisfies the client-side criteria; the server-side criterion lives in the still-open #5362. The change is inert on a server without the capability, which is the intended safe default.
Note: mergeable_state is blocked, which is expected while the PR awaits its required human approval and the #5362 dependency; it is not a code-level blocker on this head.
No line-level findings.
✅ APPROVED
HUMAN:
I've performed agent-driven testing of this.
AGENT:
Why
ConversationClient.createConversationsurfaces whatever the transport throws. When the response is lost — timeout, dropped connection — the caller cannot tell whether the create was applied. The conversation may exist with itsinitial_messagealready executing.#5035 works around this in the caller with a bounded
GET /api/conversations/{id}polling loop, and #5036 patches the client to do the same reconciliation internally. Both are symptom-level: neither makes re-sending safe, so both read to recover a value the server is the only authority on, and both carry a new timeout knob (creationRecoveryTimeout) to bound it.Summary
The actual fix for the underlying race is server-side (#5362, in this same series). With it, a re-sent create is deduplicated under the conversation's lifecycle lock: exactly one create applies, the other gets the existing conversation back, and
initial_messageruns once.So the client-side need is only: re-send, where that is safe.
idempotent_conversation_create_v1(added in fix(server): dedupe concurrent conversation creates and forks #5362).conversation_idto deduplicate on — without one there is no stable key, and re-sending would create a second conversation.An
HttpErroris never retried: a real HTTP response means the server's answer is authoritative, and validation failures must surface, not be papered over.This needs no new option, and no polling:
/server_infois already fetched and cached perHttpClient.How to Test
src/__tests__/conversation-create-retry.test.tsdrives a real HTTP server, as #5036's tests did — the failure mode is a lost response, which a stubbedfetchwould not exercise.duplicates === 0);conversation_id;clients/typescript: 334 passed (21 files).tsc --noEmit,eslint,prettierclean.Notes
This supersedes #5036, which I plan to close. #5036's diagnosis is correct and its test harness is a good one; the difference is the remedy. Its reconciliation loop recovers a value instead of making the operation idempotent, so it works around the server rather than fixing it, and it does not make a retry safe on a server that lacks the fix. With #5362 landed, the simpler retry is both smaller and strictly more correct.
This PR was created by an AI agent (OpenHands) on behalf of @neubig.
Issue Number
Fixes #5035.
Type
Notes
Depends on #5362: the retry is gated on the capability that PR advertises, so
on a server without it this change is inert and behavior is unchanged.
Live main-vs-PR validation (revalidated at current head after the
mainmerge)Exact SHAs
575511fec4bc2e21d29a45db8c7f5cab2f3dd88b. This is the merge ofmain@23fc50cd6d65f65513158dcebf38b26bf74c2edeinto335f5d201f660616a214b79a4cff8d399f9bf425.main@5ffdd2933c51423e302f5ee5e5b66df0ad9cc28a. It is 7 commits ahead of the merge base23fc50cd. None of those commits touchclients/typescript/src(onlyclients/typescript/package-lock.jsonchanged, via chore(deps): bump @openrouter/sdk from 1.2.147 to 1.3.32 in /clients/typescript #5305).4cd079abe4c3df906d2ae968729b6e8b1e268a4bis still open, so it was merged locally (cleanly, with no conflicts) onto each side: base+fix(server): dedupe concurrent conversation creates and forks #5362 =d18987a51187eebde61e9753a539c2f0d8dd3988and head+fix(server): dedupe concurrent conversation creates and forks #5362 =48d95c79bc68b908399f0e4e4a45be3a4eed600c. These merges are local and were not pushed.Setup: there were four real agent-servers and two compiled TypeScript clients. The harness imports
clients/typescript/dist/clients.jsanddist/index.jsfrom each tree, afternpm ci && npm run build. A response-drop proxy sat in front of each server. It forwardsPOST /api/conversationsto the real server and waits for the committed upstream response. It then resets the client's TCP connection (SO_LINGER 0) without sending any bytes. It can also answer the next NGET /server_inforequests with HTTP 503. Every result below was checked against the server directly, not through the proxy:GET /api/conversations/{id},/count, and the userMessageEvents from/events/search.Results (base client =
5ffdd29, head client =575511fe):5ffdd29d18987a5575511fe(no capability)48d95c79conversation_idsuppliedfetch failed(ECONNRESET), even though the server committed it (1 conversation, 1 user message). 1 POSTfetch failed, no replay (1 POST)201, 200, 1 conversation, 1 initial user messageconversation_idfetch failed, 1 POSTfetch failed, 1 POSTfetch failed, 1 POST, 0 probesfetch failed, 1 POST, 0 probesHttpError422, 1 POSTHttpError422, 1 POSTHttpError422, 1 POSTHttpError422, 1 POST, 0 probes/server_info503, then recovery, on oneConversationClient(two creates, both responses dropped)201, 201, 200, 1 user message on each conversation/server_info503, then recovery, on oneWorkspacesClient(3 ×listWorkspaces)/server_inforequest, because the rejected probe stays cached/server_inforequest/server_inforequests/server_inforequestsNegative on base, positive on head:
main5ffdd29, a lost create response surfacesfetch failedeven though the conversation was committed, including against a deduplicating server. A single transient/server_infofailure permanently poisons thatHttpClient's capability/version cache.575511fewith a deduplicating server, the lost create is re-sent once and returns the original conversation with no duplicate conversation and no duplicate initial message. The same client recovers after a transient/server_infofailure.conversation_id, after a real HTTP 422, or on the operation whose own probe failed.Caveat: in this sequential lost-response case, a direct re-send of the same id also returned
200from a server without #5362. The capability gate protects the concurrent create race that #5362 fixes. That race was demonstrated atd22e5df2(base: four201s and four initial-message effects; deduplicating server: one effect) and was not repeated here.Regression suite at head
575511fe(clients/typescript):npx vitest run: 351 passed (24 files).npx tsc --noEmit: clean.npm run format:check: clean.npm run lint: 0 errors. There are 7 warnings, and the same 7 appear on base in files this PR doesn't touch.Verdict: confirmed at exact head
575511fec4bc2e21d29a45db8c7f5cab2f3dd88bagainst exact base5ffdd2933c51423e302f5ee5e5b66df0ad9cc28a.Harness:
drop_proxy.py(response-drop proxy)Harness:
lost_response.mjs(drives the compiled client)Raw harness output (JSON lines, one file per client × server pair)
This validation section was updated by an AI agent (OpenHands) on behalf of Graham Neubig.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:575511f-pythonRun
All tags pushed for this build
About Multi-Architecture Support
575511f-python) is a multi-arch manifest supporting both amd64 and arm64575511f-python-amd64) are also available if needed