Repository navigation
fix(cli): deploy hangs forever when an AWS call gets no response - #2105
abhiramshaji-cyber wants to merge 8 commits into
Conversation
Every SDK client gets a requestTimeout of 5 minutes, but without throwOnRequestTimeout Smithy only logs a warning when it passes. The request is never failed, so the retry strategy never sees it, and a connection that dies silently keeps the CLI waiting until something outside kills it. Add a 60 second socketTimeout instead. It fires only when no data has moved in either direction for that long, so slow but healthy calls such as large asset uploads keep working, which a total request timeout would cut off. The resulting TimeoutError is classified as transient, so the existing retry strategy retries it on a new connection and fails the command once retries run out. 60 seconds matches the AWS CLI read timeout default. Both the CLI and the toolkit library build their request handler through sdkRequestHandler, with or without a proxy agent, so this covers both. The field is added to RequestHandlerSettings so custom base credential providers receive it too. Closes aws#2097
A 60 second idle timeout on every client would cut off a hotswap of a BucketDeployment. Hotswap invokes its custom resource function synchronously, and Lambda sends nothing on the connection until the function returns, which can take up to 15 minutes. The retry strategy would then start up to 7 overlapping runs of the same sync before failing a command that works today. The shared idle timeout is now 300 seconds, the same value requestTimeout already had. Older Smithy versions applied requestTimeout as an idle timeout, so this restores the behavior the CLI ran with for years while still turning an endless hang into a bounded failure. The Lambda client gets 905 seconds, just above the Lambda maximum, so a long running invoke is never dropped and retried while it is still working.
# Conflicts: # packages/@aws-cdk/toolkit-lib/test/api/aws-auth/sdk.test.ts
Head branch was pushed to by a user without write access
|
Hi, hope you're doing well! I merged |
Raise the Lambda client socket timeout from 905s to 960s so a function that runs to the 15 minute maximum after a cold start is not dropped and invoked again. Widen the margin in the slow response test so timer delay on busy CI runners cannot flake it, while the full response still takes longer than the timeout.
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR fixes The change is well-scoped and well-tested: the floor logic is covered by a matrix over disabled/longer/shorter/unset values, the lazy second client is pinned to first-invoke only, and the doc comment matches the implemented semantics. One optional note remains on the test suite's use of a live TCP listener. No other issues found on this change. 🔴 0 blocking · 🟡 0 recommended · ⚪ 1 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
|
|
||
| // Also add notes to the PR about why you imported everything individually and used 'type' so reviewers don't have to ask. | ||
|
|
||
| // a synchronous Invoke sends no data until the function returns: up to the 15 minute Lambda maximum plus cold start init |
There was a problem hiding this comment.
🟡 Recommended — The PR description describes the Lambda idle timeout as 905000 ms ("just above" the 900-second / 15-minute maximum), but the constant here, its use in SDK.lambda(), and the sdk.test.ts assertion all use 960_000 ms. The code is internally consistent and the adjacent comment justifies 960 s as the 15-minute maximum plus cold-start init, so this reads as stale description text rather than a code defect. The mismatch can mislead reviewers and future readers about the intended value.
Suggested change: Update the PR body (and any diagram) to state 960000 ms, or confirm 960_000 is intended and align the prose.
| // Also add notes to the PR about why you imported everything individually and used 'type' so reviewers don't have to ask. | ||
|
|
There was a problem hiding this comment.
⚪ Optional — The new LAMBDA_SOCKET_TIMEOUT constant is inserted directly beneath a pre-existing scratch comment ("Also add notes to the PR about why you imported everything individually..."). That comment predates this change and is not a defect introduced here, but since the diff now edits this exact region, it is a low-cost chance to drop the stale line.
Suggested change: While editing this region, consider removing the stale "add notes to the PR" scratch comment above the new constant.
| httpsAgent: agent, | ||
| }); | ||
| }); | ||
|
|
||
| describe('with a short socket timeout', () => { | ||
| let server: net.Server; | ||
| let sockets: net.Socket[]; | ||
| let respond: (socket: net.Socket) => void; | ||
|
|
||
| beforeEach(async () => { | ||
| sockets = []; | ||
| server = net.createServer((socket) => { | ||
| sockets.push(socket); | ||
| socket.on('error', () => undefined); | ||
| respond(socket); | ||
| }); | ||
| await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve)); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| sockets.forEach((socket) => socket.destroy()); | ||
| await new Promise((resolve) => server.close(resolve)); | ||
| }); | ||
|
|
||
| function client() { | ||
| return new STSClient({ | ||
| region: 'us-east-1', | ||
| endpoint: `http://127.0.0.1:${(server.address() as net.AddressInfo).port}`, | ||
| credentials: { accessKeyId: 'AKIAEXAMPLE', secretAccessKey: 'dummy' }, | ||
| requestHandler: { ...sdkRequestHandler(), socketTimeout: 1_000 }, | ||
| retryStrategy: new ConfiguredRetryStrategy(2, () => 0), | ||
| }); | ||
| } | ||
|
|
||
| test('retries a request that gets no response, then fails', async () => { | ||
| respond = (socket) => socket.resume(); | ||
|
|
||
| await expect(client().send(new GetCallerIdentityCommand({}))).rejects.toMatchObject({ name: 'TimeoutError' }); | ||
| expect(sockets).toHaveLength(2); | ||
| }); | ||
|
|
||
| test('keeps a slow response that is still sending data', async () => { | ||
| const body = '<GetCallerIdentityResponse><GetCallerIdentityResult><Account>123456789012</Account></GetCallerIdentityResult></GetCallerIdentityResponse>'; | ||
| const response = `HTTP/1.1 200 OK\r\nContent-Type: text/xml\r\nContent-Length: ${body.length}\r\nConnection: close\r\n\r\n${body}`; | ||
| respond = (socket) => { | ||
| socket.resume(); | ||
| const chunks = response.match(/.{1,5}/gs)!; | ||
| const timer = setInterval(() => { | ||
| socket.write(chunks.shift()!); | ||
| if (chunks.length === 0) { | ||
| clearInterval(timer); | ||
| socket.end(); | ||
| } | ||
| }, 50); | ||
| }; | ||
|
|
||
| await expect(client().send(new GetCallerIdentityCommand({}))).resolves.toMatchObject({ Account: '123456789012' }); | ||
| expect(sockets).toHaveLength(1); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
⚪ Optional — The two STSClient-against-net.Server tests in the "with a short socket timeout" block set an explicit socketTimeout: 1_000 on a locally constructed client, so they validate Smithy's general handling of socketTimeout rather than this PR's change to sdkRequestHandler and SDK.lambda(), and pass on main as well. Coverage of the actual change is already provided by the settings-shape test and the sdk.test.ts assertions, so these are optional. The fixtures are cleaned up in afterEach, so no resources leak.
Suggested change: Consider whether a real TCP listener belongs in the unit suite, or drop these two in favor of the settings assertions that fail without the change.
Only a synchronous Invoke stays silent for the whole function run. Other Lambda calls, such as UpdateFunctionCode and the hotswap waiters, now keep the shared 300s idle timeout instead of waiting 16 minutes per attempt.
|
A small follow up after a second look at my own change, sorry for the extra noise! In 0f50cb8 and c8300d5 I raised the Lambda |
Also drop a stale scratch comment above LAMBDA_SOCKET_TIMEOUT.
|
Sorry for the noise, the build caught a type error in my own test. The |
… Invoke Invoke only raises the socket timeout to LAMBDA_SOCKET_TIMEOUT when the caller's value is shorter or unset, and its client is created on the first invoke.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2105 +/- ##
==========================================
+ Coverage 92.05% 92.07% +0.02%
==========================================
Files 82 82
Lines 13195 13206 +11
Branches 1894 1900 +6
==========================================
+ Hits 12146 12160 +14
+ Misses 1011 1008 -3
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A deploy that got no answer from AWS used to wait until the job was killed, and now each silent call is dropped after 5 minutes and retried.
What this code is for
sdkRequestHandlerinpackages/@aws-cdk/toolkit-lib/lib/api/aws-auth/awscli-compatible.tsbuilds the HTTP settings that every AWS SDK client in the CLI and in the toolkit library uses. Bothpackages/aws-cdk/lib/cli/cli.ts(with the proxy agent, if any) andToolkit.sdkProviderintoolkit.ts(withsdkConfig.httpOptions.agent, if any) call it, and the result is passed to every client next to aConfiguredRetryStrategythat retries timeouts and other network errors. The one exception after this PR isinvokeCommandinSDK.lambda()insdk.ts, which uses a longer idle timeout.Before this PR
A GitHub Actions job runs
cdk deploy, and the connection under oneDescribeStackscall goes silent after the request was sent, as described in #2097.DescribeStacksthroughNodeHttpHandlerwithconnectionTimeout: 10000andrequestTimeout: 300000.connectionTimeoutnever applies.requestTimeoutpasses.throwOnRequestTimeoutis not set, so Smithy only logs[WARN] a request has exceeded the configured 300000 ms requestTimeout.ConfiguredRetryStrategynever sees an error and never retries.main: one connection, the warning at 300 s, and still waiting at 330 s.After this PR
Same job, same silent
DescribeStackscall.socketTimeout: 300000.TimeoutError.ConfiguredRetryStrategytreatsTimeoutErroras a temporary error and sends the call again on a new connection.TimeoutErrorinstead of waiting. There are 7 attempts in total, so that is about 35 minutes plus the short retry delays. Against the same silent local server with a 60 s value, I saw the call retried on connections 2 to 7 and then failed at about 467 s, which is the same path with a shorter timer.BucketDeploymentinvokes its Lambda function synchronously, and Lambda sends nothing back until the function returns, which can take up to 15 minutes.Invoketherefore getssocketTimeout: 960000, which covers that maximum plus cold start init, so a longs3 syncis never dropped and started again while it is still running.The diagram
flowchart LR classDef box fill:#eef1ff,stroke:#4c5bd4,color:#111827 classDef bad fill:#ffd6d6,stroke:#cc0000,color:#111827,stroke-width:3px classDef good fill:#c8f0c8,stroke:#008800,color:#111827,stroke-width:3px subgraph BEFORE["BEFORE: waits until the job is killed, 1 connection"] direction TB B1["cdk deploy calls DescribeStacks"] --> B2["NodeHttpHandler sends the request"] B2 --> B3["connection goes silent"] B3 --> B4{"requestTimeout 300000 passes"} B4 -->|"throwOnRequestTimeout not set"| B5["logs WARN and keeps waiting forever"] B4 -->|"response arrives"| B6["deploy continues"] end subgraph AFTER["AFTER: retried every 300 s, then fails with TimeoutError"] direction TB A1["cdk deploy calls DescribeStacks"] --> A2["NodeHttpHandler sends the request"] A2 --> A3["connection goes silent"] A3 --> A4{"THE CHANGE: socketTimeout 300000"} A4 -->|"no data for 300 s"| A5["TimeoutError, retried on a new connection"] A4 -->|"data still moving"| A6["slow upload keeps going"] end B1 ~~~ A1 class B1,B2,B3,B6 box class A1,A2,A3,A6 box class B4,B5 bad class A4,A5 good linkStyle 3 stroke:#cc0000,stroke-width:5px linkStyle 8 stroke:#008800,stroke-width:5px style BEFORE fill:#fff5f5,stroke:#cc0000,color:#111827 style AFTER fill:#f3fff3,stroke:#008800,color:#111827The change
sdkRequestHandlernow returnssocketTimeout: 300000next to the existingconnectionTimeoutandrequestTimeout. This is the fix. I reused the 300 seconds thatrequestTimeoutalready has, because the Smithy docs forthrowOnRequestTimeoutsayrequestTimeout"was for a long time incorrectly being set as a socket idle timeout". So the CLI already ran with a 5 minute idle timeout that threw for years, and this brings that behavior back on purpose.SDK.lambda()insdk.tssendsInvokethrough a second client, created on the first invoke, withsocketTimeout: 960000and every other setting unchanged. If the caller already setsocketTimeoutto 0 (no timeout) or to something longer,Invokekeeps that value. The other Lambda calls, such asUpdateFunctionCodeand the hotswap waiters, keep the shared 300 s value, so a silent one of those is not held for 16 minutes per attempt. The hotswap of aBucketDeploymentcallsInvokesynchronously and the function may run for the full 15 minutes, so a shorter idle timeout would drop it and the retries would start overlappings3 sync --deleteruns.RequestHandlerSettingsinbase-credentials.tsgets an optionalsocketTimeoutfield, so the value type checks and a customIBaseCredentialsProviderreceives it inclientConfig.requestHandlertoo.requestTimeoutand its warning are left as they are. I did not turn onthrowOnRequestTimeout, because that timer runs from the start of the request until the response headers arrive, so it would fail a healthy upload that takes longer than 5 minutes. @JimNero009 pointed this out in the issue, and their suggestion of asocketTimeoutis what this PR follows. Thank you for the very clear report and reproduction.awscli-compatible.test.ts: one checks the settingssdkRequestHandlerreturns, including the agent on bothhttpAgentandhttpsAgent, and fails without this change. Two more run a realSTSClientagainst a local TCP server with a 1 ssocketTimeout. To be honest about what these two prove: they pass onmaintoo, because they check how Smithy handlessocketTimeoutrather than this change. I kept them as a guard in case a future Smithy version changes that behavior, but I am very happy to drop them if you would rather not have a TCP server in a unit suite: a server that never answers gets a second connection and the call rejects withTimeoutError; a server that sends its whole response, headers included, in small pieces every 50 ms still succeeds on one connection. That second test fails if the settings userequestTimeoutwiththrowOnRequestTimeoutinstead, which is why I went withsocketTimeout.sdk.test.ts: one checks thatinvokeCommandis sent through a client withsocketTimeout: 960000and the other settings unchanged, and fails without this change; four check that a disabled or longer caller value is kept and a shorter or unset one is raised to 960000; one checks theInvokeclient is created only on the first invoke; two check that other Lambda calls and the CloudFormation client still get the shared settings.I would really value your view on a few choices, and I am happy to change any of them:
cdk-assetsCLI builds its clients incdk-assets-lib/lib/aws.tswithout any of these timeouts. Would you like the same change there as a separate PR?What changes for a user
TimeoutErrorafter the existing retries, about 35 minutes, instead of running until the CI job is killed.cdk deploy --hotswapandcdk watchwith aBucketDeploymentthat takes many minutes keep working, becauseInvokewaits up to 16 minutes.ExecuteChangeSetorInvoke, is now retried after a silent timeout instead of hanging. If AWS did process the first attempt, the retry can fail with a clearer error such as an existing change set. This is the same tradeoff the SDK already makes for a reset connection.sdkRequestHandler.requestTimeoutwarning after 5 minutes still appears for very long uploads, as it does today.Fixes #2097
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license