sdk: finish the 0.6 cleanups — v1 defaults, v1 naming, transport errors - #1120
Open
kvinwang wants to merge 4 commits into
Open
sdk: finish the 0.6 cleanups — v1 defaults, v1 naming, transport errors#1120kvinwang wants to merge 4 commits into
kvinwang wants to merge 4 commits into
Conversation
…t errors The leftovers named in #1116's description, from the #1094 prerelease checklist, plus one wire bug found on the way. Go's v1 `IssueCert` defaulted `usage_server_auth` to false where Rust, Python and JS default it to true, so the same argument-free call yielded a servable certificate in three languages and an unservable one in the fourth. v0's false default is kept and commented: that surface mirrors released 0.5.x behaviour rather than the better choice. `IssueCertResponseV1.asUint8Array` is removed rather than renamed. It existed to feed the key into the blockchain adapters, and v1 has no chain-flavoured surface -- `IssueCert` returns TLS material, PEM is what a TLS stack takes, and all four SDKs now return the PEM string and the chain and nothing else. The GPU bundle's accessor becomes `decodeEvidence()`, matching Python's and Rust's `decode_evidence`, since that one decodes wire hex rather than serving a chain flow. The JS transport ignored the HTTP status entirely -- its socket branch never parsed the status line -- so an agent with no `/v1` mount answered with an HTML 404 that reached the caller as "failed to parse response". Its socket branch also declared `Content-Length` as `payload.length`, a UTF-16 code unit count, while writing UTF-8: any request carrying a non-ASCII string was truncated on the wire and left the connection poisoned, which the v1 spec's promise that a `domain` may be any byte string made reachable by design. The Rust clients threw away the other half of the same information, since `error_for_status()` keeps the status and drops the body, and the unix crate's JSON helper insists on deserializing the error body too. Both now raise `HTTP <status>: <what the server said>`, and the two duplicated Rust transport blocks became one `http_post`/`unix_post`/`http_error`. `TlsKeyOptions.path` is gone: it was declared but never sent, so callers who set it were silently ignored. Nothing on the wire or the success path changes except the corrected Content-Length; only failures read differently. Verified with sdk/run-tests.sh: Rust, Go, Python green and JS 144 passed, with the new tests asserting against real simulator responses -- a real HTML 404 from a socket with no v1 mount, and a real prpc 400 from EmitEvent.
An adversarial review found the Content-Length fix in the previous commit was half a fix. The write path counted bytes; the read path still compared that byte-counted `Content-Length` against a JS string's `.length`, which counts UTF-16 code units. For any response carrying a non-ASCII character the units never reach the bytes, so the client never decided the body was complete and waited out the agent's ten-second keep-alive instead. One accented character in an app-compose comment was enough to turn `info()` into a ten-second call and make `isReachable()` report a healthy agent as unreachable: measured at 15006ms against a request the agent answered in 9ms. The same branch also did `data += chunk`, which decodes each Buffer on its own, so a UTF-8 sequence split across two TCP reads became a replacement character on each side -- and the call resolved successfully, handing back quietly corrupted data. And with no `Content-Length` at all it sliced to nothing and reported "(empty response body)" for a body that was on the wire. All three are the same defect: a hand-written HTTP parser that confuses bytes with characters. It is gone. The unix branch now uses node's client over `socketPath`, which frames the request, de-chunks the response, and counts bytes where bytes are meant; both branches assemble responses as Buffers and decode once at the end. `agent: false` keeps the old one-connection-per-call behaviour -- node's default agent pools the socket, and an open socket keeps the process alive, so a script awaiting one `info()` would hang. The six unix tests that mocked `net.createConnection` are replaced by four that run against a real unix socket, including a multi-byte body served over a kept-alive connection and a UTF-8 sequence split across two writes. Neither defect was visible to the old fixtures, which fed the client a whole response as one ASCII string. For the same reason the remaining HTTP mocks now emit Buffers: node never emits strings there, and the string fixtures are what let this survive. Also from the review: the CHANGELOG claimed the success path was unchanged. It is not -- six JS v0 methods that never checked the response body used to resolve on a prpc failure and now reject, and the Rust unix path stops sending a duplicated Content-Type header. Both are stated now. Rust's truncation bound is renamed to say it counts characters, and Go's `WithCertUsageServerAuth` documents the default that the other three SDKs express in their signatures.
This was referenced Aug 25, 2026
Two defects the transport rework left in place. The JS timeout aborted the request before rejecting. `abort()` runs its listener synchronously, so `onAbort`'s `request aborted` always won the `isCompleted` race and `request timed out` was unreachable -- which is exactly the message `isReachable()` needs to tell a hung agent apart from any other failure. Rejecting first fixes it; the abort still runs, to destroy the socket. The test that claimed to cover this stubbed `global.setTimeout` so the callback ran before the abort listener was registered, an ordering that cannot happen at runtime, and passed against the broken transport. It is replaced by one against a real socket that accepts and never answers. `http_post` passed `Content-Type` on top of `json()`, and reqwest's `header()` appends rather than replaces, so the header went out twice. Rocket reads the first value and the agent never noticed, but RFC 9110 lets an intermediary refuse a request carrying the field twice.
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.
Follow-up to #1116, closing the SDK leftovers that PR's description named, from the #1094 prerelease checklist. One wire bug turned up on the way.
Go's v1 default was the odd one out
IssueCertbuilt its options from the zero value, sousage_server_authdefaulted tofalsewhere Rust, Python and JS all default it totrue. The same argument-free call produced a servable certificate in three languages and an unservable one in the fourth. Go's v1 default is nowtrue.v0's
GetTlsKeykeeps itsfalsedefault, now with a comment saying why: that surface mirrors what the released 0.5.x Go SDK sent, not the better choice. Both defaults are pinned by a test asserting the actual request payload.asUint8Arrayis removed from v1, not renamedThe obvious move was
toPkcs8Der(), but the accessor should not be on v1 at all: it exists to feed the key into the blockchain adapters, and v1 has no chain-flavoured surface.IssueCertreturns TLS material, PEM is what a TLS stack takes, and a caller who genuinely wants DER converts it with a standard library. All four SDKs' v1 clients now return the PEM string and the chain and nothing else.The GPU bundle's accessor is a different case — it decodes wire hex rather than serving a chain flow — so that one is renamed
decodeEvidence(), matching Python's and Rust'sdecode_evidence.v0's
GetTlsKeyResponse.asUint8Arrayis untouched: released API, and its truncating behaviour is load-bearing for the chain adapters.Transport failures said nothing useful
The JS transport ignored the HTTP status entirely — its unix-socket branch never even parsed the status line — so an agent with no
/v1mount answered with an HTML 404 that reached the caller asfailed to parse response. The Rust clients threw away the other half of the same information:error_for_status()keeps the status and drops the body, which is exactly where the agent puts the reason it refused, and the unix crate's JSON helper insists on deserializing the error body too, so an HTML 404 degenerated into a parse failure there as well.Both now raise
HTTP <status>: <what the server said>— the prpcerrorfield when the body is one, otherwise the body bounded to a few hundred characters so an HTML page does not become a screen-long exception. Consolidating this also collapsed the two duplicated Rust transport blocks into onehttp_post/unix_post/http_error.The bug: the JS socket branch declared
Content-Length: ${payload.length}— a UTF-16 code-unit count — while writing the body as UTF-8. Any request carrying a non-ASCII string was truncated on the wire, and the surplus bytes poisoned the connection. The v1 spec promisesGetKeyRequest.domainmay be any byte string a proto3stringcan carry, so this was reachable by design rather than by accident;getKey('café', 'ed25519')is enough. Rust, Go and Python all computed the length correctly.Dead option removed
TlsKeyOptions.pathwas declared but never sent — the wire message has no such field — so callers who set it were silently ignored. Python, Rust and Go were checked and have no equivalent.Verification
./sdk/run-tests.shgreen: Rust, Go and Python suites pass and JS reports 144 passed. The new tests assert against real simulator responses rather than mocks — a real HTML 404 (a v1 call against the tappd socket, which has no v1 mount) and a real prpc 400 (EmitEvent) — plus wire-level assertions on both Go defaults and a round-trip of a multi-byte UTF-8 payload over the socket path.Nothing on the wire or the success path changes apart from the corrected
Content-Length; only failures read differently. TheEmitEventremoval message still reaches the caller, now asHTTP 400: EmitEvent was removed in dstack 0.6.0….