fix(sagemaker): pass abortSignal to the transport, not just to the loop - #1566
Open
murdore wants to merge 1 commit into
Open
fix(sagemaker): pass abortSignal to the transport, not just to the loop#1566murdore wants to merge 1 commit into
murdore wants to merge 1 commit into
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (70)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
The same defect as Bedrock's, in a different shape. streaming.ts polls
options.abortSignal between iterations, which stops the consumer reading
but leaves the HTTP request running; the calls that actually needed the
signal are the two client.send() invocations in sagemaker/client.ts, and
neither took an options argument.
Unlike Bedrock, no signal was in scope to pass. invokeEndpoint takes an
InvokeEndpointParams that had no room for one, so this adds an optional
abortSignal field and threads it from the two real call sites in
language-model.ts: doGenerate and the streaming path. The six detection.ts
calls are capability probes and are left alone.
abortSignal is spelled in camelCase against PascalCase neighbours on
purpose. Every other field mirrors an AWS InvokeEndpointCommandInput
member; this one is a transport option handed to client.send() as
@smithy/types HttpHandlerOptions and never travels in the command payload.
doGenerate and doStream type their options as Record<string, unknown>, so
the value arrives as unknown even though LanguageModelV2CallOptions
declares abortSignal?: AbortSignal. readAbortSignal narrows with
instanceof — a real runtime check rather than an assertion, per rule 14.
executeWithRetry needed no change: an AbortError matches none of
RETRYABLE_ERROR_CONDITIONS' error names or message keywords, so an abort
surfaces instead of re-issuing a request the caller already abandoned.
Demonstrated against local stand-in servers, abort fired at 1000ms, with
both runs asserting the request reached the server before anything was read
into the result:
InvokeEndpoint without: still waiting at the 8002ms cap,
server never saw the socket close
with: AbortError at 1002ms, close at 1002ms
InvokeEndpointWithResponseStream without: same, never closed
with: AbortError at 1002ms, close at 1002ms
This also corrects a note left behind by the Bedrock fix, which warned that
a SageMaker probe would need HTTP/2 or report a false negative. It does
not: SageMaker Runtime speaks HTTP/1.1 and fails on an h2c listener with
"Parse Error: Expected HTTP/, RTSP/ or ICE/". Bedrock is the HTTP/2 one.
The probe ran both protocols and let the reached-the-server guard decide,
which is the habit worth keeping — not a fixed assumption about either.
docs/api is regenerated because the new field is on an exported type, and
the docs-currency gate fails on any difference. 67 files move: one gains
the abortSignal section, and the other 66 shift their "Defined in:" line
numbers by ten, because every type they document also lives in
types/providers.ts below the insertion point. Every changed line outside
the new section is such a shift, checked rather than assumed.
Verified: sagemaker-streaming 5/5, providers-mocked 70/70,
provider-structure 3/3, tsc --noEmit --strict clean against a baseline of
the same tree, eslint and prettier clean, and docs:api now regenerates with
no drift.
murdore
force-pushed
the
fix/sagemaker-transport-abort
branch
from
August 26, 2026 21:06
c9e0362 to
e81bcbd
Compare
Contributor
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
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.
The same defect as Bedrock's (#1565), in a different shape.
streaming.tspollsoptions.abortSignalbetween iterations, which stops the consumer reading but leaves the HTTP request running. The calls that actually needed the signal are the twoclient.send()invocations insagemaker/client.ts— neither took an options argument.Why this wasn't a two-line change
Bedrock's
executeStepalready receives a liveAbortSignal. SageMaker had none in scope:invokeEndpointtakes anInvokeEndpointParamswith no room for one. So this adds an optionalabortSignalfield and threads it from the two real call sites inlanguage-model.ts—doGenerateand the streaming path. The sixdetection.tscalls are capability probes and are left alone.Two details worth review:
abortSignalis camelCase against PascalCase neighbours, deliberately. Every other field mirrors an AWSInvokeEndpointCommandInputmember. This one is a transport option handed toclient.send()as@smithy/typesHttpHandlerOptionsand never travels in the command payload — spelling it differently keeps that boundary visible.readAbortSignalnarrows withinstanceof.doGenerate/doStreamtype their options asRecord<string, unknown>, so the value arrives asunknowneven thoughLanguageModelV2CallOptionsdeclaresabortSignal?: AbortSignal.instanceofis a real runtime check rather than an assertion, per rule 14.executeWithRetryneeded no change: anAbortErrormatches none ofRETRYABLE_ERROR_CONDITIONS' error names or message keywords, so an abort surfaces instead of re-issuing a request the caller has already abandoned.Demonstrated
Local stand-in servers, abort fired at 1000ms. Both runs assert the request reached the server before reading anything into the result.
InvokeEndpointInvokeEndpointAbortErrorat 1002msInvokeEndpointWithResponseStreamInvokeEndpointWithResponseStreamAbortErrorat 1002msThis corrects a note the Bedrock fix left behind
That work recorded a warning that any SageMaker probe would need HTTP/2 or report a false negative. It does not. SageMaker Runtime speaks HTTP/1.1 and fails against an h2c listener with
Parse Error: Expected HTTP/, RTSP/ or ICE/. Bedrock is the HTTP/2 one.The probe ran both protocols and let the reached-the-server guard decide which was real. That habit — rather than any fixed assumption about a client's transport — is what caught it, and is the same guard that stopped three earlier Bedrock probes' false negative from repeating here.
Verification
sagemaker-streaming5/5 ·providers-mocked70 passed · 0 failed (of 70) ·provider-structure3/3 ·tsc --noEmit --strict0 errors against a stashed baseline of the same tree · eslint + prettier clean.No behaviour change when nothing aborts: the field is optional and
readAbortSignalreturnsundefinedwhen the SDK supplies no signal, which is exactly whatclient.send()received before.