Repository navigation
fix(sdk-js): fail overdue writes as outcome-unknown when a dead server's connection is dropped - #363
Merged
Merged
Conversation
jsam
changed the title
…ction-lost (#351) At a sent call's deadline the connection armed the transport probe before the call's grace timer. Both wait timeoutGraceMs, but each setTimeout reads the clock anew, so the grace timer could land a millisecond later; the unanswered probe then dropped the socket first and the close failed the write as ConnectionLostError (1 in 5 runs of the dead_server fixture). Arm the grace first so the call always fails overdue before the probe's drop. A test forces the clock to move while the probe is sent, which fails every time without the fix. dead_server joins the shared conformance set, with a stall step, per-call timeout_ms and a deadline_grace_ms option in the Python runner, and the JS SDK's unit tests and type check now run on PRs. Closes #351
…ps the connection
jsam
changed the title
jsam
marked this pull request as ready for review
October 5, 2026 17:54
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.
Intent
I want you to really hyper focus on making the symbolic engine, you know, crisp and really truly production ready. ... just ensure that both machines are propelry utilized now (Context: implement #351 exactly as written.)
Issue #351, "JS SDK: dead_server connection fixture flakes (OutcomeUnknownError vs ConnectionLostError)": found by the test-strategy review on main edb8bc3, 1 of 3 re-runs fails. The JS SDK unit test
connection fixtures > dead_serverexpectsOutcomeUnknownError, getsConnectionLostError. The fixture (packages/inputlayer-js/tests/fixtures/connection/dead_server.json, from #237) has the mock server accept one write then stall; the SDK must fail the write as outcome-unknown at its deadline, but sometimes the connection teardown wins and reports connection-lost (timing-dependent). Unreported; the JS unit suite runs only in the full suite, so no PR sees it. Fix the race in the mock or the SDK, add it to the conformance set, and run the JS suite on PRs (T-05).What Changed
connection.ts). When the transport probe gets no answer and the client drops the connection, any call already past its deadline (graced) now fails with the overdue error:OutcomeUnknownErrorfor a call that may write,DeadlineExceededErrorfor a query. It no longer fails withConnectionLostError. Calls that are not yet overdue still fail withConnectionLostError. The probe also now starts only after the call has been marked graced. This removes the timing race behind the flakydead_serverfixture.dead_serverconformance fixture. Addedpackages/conformance/connection/dead_server.json, and the conformance format gains three things: anoptions.deadline_grace_mssetting, a per-calltimeout_ms, and a{"stall": true}server step that stops reading, like a dead server. The Python conformance runner supports all three, and the JS connection tests are updated to match. The js-sdk guide, the conformance README and the source comments describe the new failure behavior.JS SDKjob that runsmake pre-pr-js(unit tests and type check) whenpackages/inputlayer-js/,packages/conformance/or the workflow file changes. CONTRIBUTING now lists the SDK jobs under the fast gate.Risk Assessment
✅ Low: The change is small and bounded to the JS SDK's deadline and probe path. Calls in grace now fail the same way whichever timer fires first, which fixes the race in #351. It adds a regression test that fails without the fix, adds the conformance fixture (the Python SDK handles it deterministically because it has no deadline probe), and adds a path-filtered JS CI job. The one remaining issue is the wording of an error message.
Testing
I ran the JS SDK against a real inputlayer-server frozen with SIGSTOP, using both the fixed and the base SDK. On the fixed SDK, every call past its deadline failed the overdue way: OutcomeUnknownError for a write, DeadlineExceededError for a query. The call still inside its deadline got ConnectionLostError. That held in all 5 rounds. The base SDK misclassified the staggered write and the query as ConnectionLostError in all 5 rounds. Non-live supporting checks: the targeted JS tests fail on base and passed 25/25 on HEAD, the Python dead_server conformance fixture passes, and
make pre-pr-js, which the new CI job runs, passes. Those ran against mock servers or locally, so their scenarios are marked untested. The GitHub Actions job itself can only run on GitHub, so I checked its command locally and parsed its config instead. I deleted the build outputs and Python caches afterward.make pre-pr-js, passes locally) a…Evidence: Live SIGSTOP dead-server results, fixed vs base SDK (summary)
fixed: A OutcomeUnknown | B OutcomeUnknown | R DeadlineExceeded | L ConnectionLost (5/5 rounds) base: A OutcomeUnknown | B ConnectionLost | R ConnectionLost | L ConnectionLost (5/5 rounds)Evidence: Live run raw JSON (fixed SDK)
Evidence: Live run raw JSON (base SDK)
Evidence: Live driver script
Evidence: New tests fail against base SDK (regression reproduced)
Evidence: 25 repeated targeted runs on HEAD
Evidence: Python conformance run incl. dead_server
Evidence: make pre-pr-js (CI job command)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
packages/inputlayer-js/src/connection.ts:696- The fix works only because of timer ordering, and the ordering covers just the call that started the probe.probeServer()returns early while a probe is already pending (connection.ts:762), so a second in-flight call whose deadline passes inside the first call's probe window still races the drop. Concrete case: on a dead server (default maxInFlight 15), the client pipelines two writes with timeoutMs 50, sent 1 ms apart, with timeoutGraceMs 100. Write A expires at t=50: it arms overdue@150 and probe@150. Write B expires at t=51: it arms overdue@151, and its probe is skipped. At t=150, A fails OutcomeUnknown and then the probe fires. B and B's cancel are still outstanding (2 < 15), so the probe callsws.terminate().onClose→failInFlight(connection.ts:1148-1157) then rejects B with ConnectionLostError, even though B's deadline passed and it was waiting out its grace. That is exactly the misclassification issue JS SDK: dead_server connection fixture flakes (OutcomeUnknownError vs ConnectionLostError) #351 describes. The keepalive probe has the same problem (connection.ts:1239): if it is pending when a deadline expires, it fires before that call's overdue timer. Recommended durable fix at the shared boundary: infailInFlight, reject calls withcall.gracedset the same wayoverdue()does (OutcomeUnknownError for writes, DeadlineExceededError for reads), so the result no longer depends on which timer fires first. Optionally add a JS test with two staggered writes on a stalled server.🔧 Fix applied.
✅ Re-checked - no issues remain.
packages/inputlayer-js/src/connection.ts:776- Introduced in fix round 2 (a232240c). When the probe drops the connection, calls already in their grace period now fail withoverdueError(call). That error's message says "No reply ${timeoutGraceMs} ms past the deadline", but these calls may have been waiting much less. Example: with grace 100, write A expires at 50 ms and write B at 60 ms. The probe drops the connection at 150 ms, so B fails only 90 ms past its deadline, yet its message still says 100 ms. The error class (OutcomeUnknownError or DeadlineExceededError) is correct, as the user asked; only the text is wrong. Fix: giveoverdueErroran optional reason, so the probe path can say the connection was dropped because the server stopped answering instead of claiming the full grace elapsed. The only other caller isoverdue()(connection.ts:717), where the text is accurate.✅ **Test** - passed
✅ No issues found.
npm testwere done. The remote CI phase will exercise the new js-sdk job on this PR.npx vitest run tests/connection.test.ts(66 pass)npx vitest run tests/connection.test.ts -t "dead_server|dead server"run 30 times in a row while a cargo release build loaded the CPU: 30/30 passThe same selector with src/connection.ts swapped to base 57ad2e95: both new tests fail with ConnectionLostError (3/3 runs). With the first fix commit fa5e9eb4 only, the two-pipelined-writes test fails.npm testin packages/inputlayer-js (the JS unit suite the new CI job runs): 381 passuv run --extra dev pytest tests/test_connection_conformance.py -k dead_server5 times plus the full conformance file (25 pass)Parsed .github/workflows/ci.yml with PyYAML: the changes job outputsjs, its filter covers packages/inputlayer-js/, packages/conformance/ and ci.yml, and the js-sdk job is gated on it and runsmake pre-pr-jscargo build --release --bin inputlayer-server, then run_live.sh: a throwaway server (temp data dir, port 18391); the SDK is driven through vite-node and the server is SIGSTOPped for each of 15 rounds × 4 scenarios, for both the fixed and the pre-fix connection.ts✅ No issues found.
make pre-pr-js, passes locally) a…npx vitest run tests/connection.test.ts -t "dead_server|dead server|deadline probe"repeated 25× on HEAD (25/25 pass)Same targeted vitest run with src/connection.ts swapped to base ff6641af (dead_server fixture and both new probe tests fail with ConnectionLostError; restored afterward)Live: builtcargo build --release --bin inputlayer-server, ran it on a throwaway data dir/port, bundled the JS SDK (HEAD and base) with esbuild into the evidence dir, and drovenode dead-server-live.cjsfor 5 rounds each: warm-up write, then SIGSTOP the server with writes A (200 ms), B (210 ms), query R (205 ms) and write L (60 s) in flight, timeoutGraceMs 300uv run --extra dev pytest tests/test_connection_conformance.py -v(dead_server conformance fixture passes, 25/25)make pre-pr-js(the command the new CI js-sdk job runs: JS unit tests and tsc typecheck pass)Parsed .github/workflows/ci.yml with PyYAML to confirm the js-sdk job is gated on needs.changes.outputs.js and runsmake pre-pr-js🔧 **Document** - 1 issue found → auto-fixed ✅
docs/content/docs/guides/js-sdk.mdx:80- The guide says calls in flight fail with ConnectionLostError when the connection drops. Since this change, when the liveness probe drops the connection, calls already past their deadline (in grace) fail as at the end of their grace instead (OutcomeUnknownError / DeadlineExceededError), which is consistent with line 78. Left unchanged as an internal edge case; consider adding a short qualifier if this should be user-visible.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
✅ No issues found.