feat(cli): send telemetry from a detached subprocess to unblock CLI exit by sanjanaravikumar-az · Pull Request #1779 · aws/aws-cdk-cli

sanjanaravikumar-az · 2026-07-29T17:56:19Z

This PR removes the telemetry request from the CLI's critical path.

Today every cdk command waits for telemetry to be uploaded before exiting. Although the API Gateway shows that we spend only ~30ms processing the request, the end-to-end cost is much higher because every invocation pays for DNS resolution + TCP connection setup + the TLS handshake + the request. In total, that's about 250–400 ms of idle time for every command. @rix0rrr verified that this isn't something we can optimize on the service side: removing the request validator, WAF, logging, and switching from an edge endpoint to a regional endpoint made essentially no difference, because the latency comes from establishing the connection rather than serving the request.

So in this approach, instead of sending telemetry synchronously, the CLI now hands the payload to a detached child process and exits immediately. The child is unref()'d so it can continue running independently while the parent terminates. This preserves telemetry delivery without making users wait for a network request whose outcome doesn't affect the command they just ran.

Logic:

bin/cdk checks CDK_TELEMETRY_SENDER=1 before loading the main bundle. In that mode it loads a small sender module and exits immediately, so the startup time goes from ~600ms to ~35ms
the sender implements the small amount of HTTP and proxy logic it needs directly using Node built ins.
the parent can't pass its proxy agent across a process boundary, so it forwards the proxy URL and CA bundle contents. The child recreates either a direct HTTPS connection or an HTTP CONNECT tunnel as necessary
SOCKS and PAC proxies intentionally fail closed for telemetry rather than bypassing a proxy configuration unexpectedly.

I verified this by running time cdk synth, and it no longer spends time waiting for telemetry, even when the endpoint is unreachable. All unit tests pass, and 2 integ tests have been added - it tests that the CLI doesn't block on a dead endpoint, and that a batch is actually delivered through a TLS-terminating proxy.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The telemetry POST at the end of every CLI invocation was awaited before the process could exit, adding ~300ms to every command. Hand the payload to a detached child process instead. bin/cdk re-invokes itself with CDK_TELEMETRY_SENDER=1 and dispatches to a new builtins-only sender module before requiring the CLI bundle (which costs ~600ms to load), so the child stays cheap. The parent writes the batch to the child's stdin, unrefs it, and exits. Because the published package has zero runtime dependencies, the sender can only use Node built-ins. That rules out proxy-agent, so it re-implements the parts we actually support: HTTP CONNECT tunnelling through http:// and https:// proxies, Basic proxy auth, a forwarded CA bundle, and proxy-from-env's NO_PROXY semantics. SOCKS and PAC proxies fail closed (telemetry is skipped rather than bypassing a proxy that is usually mandatory). Refs D488314716

Adds unit coverage for the bin/cdk path resolution, and two integration tests: one asserting the CLI's exit time no longer tracks the telemetry endpoint (the endpoint is a TCP black hole that never responds), and one proving delivery still works for proxy users, reusing the existing TLS-terminating mockttp harness. Also applies eslint --fix (import ordering and brace newlines).

… byte-accurate stdin cap, drop blocking connectivity check) The legacy 'Telemetry Sent Successfully' trace was retained verbatim so the existing integration tests kept passing, but it is now a lie: the parent only hands the batch to a detached sender and never learns whether the POST succeeded. Replace it with a single 'Telemetry dispatched (pid N, M bytes)' line, hoist the stable 'Telemetry dispatched' prefix into a named constant so it is obvious it must not change casually, and update all seven integration tests plus the unit test that matched the old string. The sender's stdin cap was compared against a string's length, which counts UTF-16 code units, so a multi-byte payload could reach three times the intended size. Read stdin as Buffers, measure with byteLength, and decode once at the end -- which also removes the need to reason about multi-byte sequences that straddle a chunk boundary. Extracted as readAll() so the cap is directly testable. Finally, drop the NetworkDetector connectivity gate. Checking reachability before dispatching is itself a network call on the CLI's exit path -- up to a 3s HEAD request on a cold cache -- which is exactly what this sink exists to avoid. Offline machines now spawn a child that fails and exits; it has its own timeouts and swallows every error, so being wrong costs one short-lived process. This leaves the sink's 'agent' prop unused (the child receives proxy configuration as proxyUrl/caCert, not as an Agent), so remove that plumbing too. The notices path still uses NetworkDetector and is untouched. Refs D488314716

github-actions · 2026-07-29T17:57:06Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

codecov-commenter · 2026-07-29T18:14:25Z

Codecov Report

❌ Patch coverage is 95.11344% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.29%. Comparing base (7ff50e7) to head (c47a1b7).

Files with missing lines	Patch %	Lines
.../aws-cdk/lib/cli/telemetry/sink/subprocess-sink.ts	94.89%	12 Missing ⚠️
packages/aws-cdk/lib/cli/proxy-agent.ts	93.39%	7 Missing ⚠️
packages/aws-cdk/lib/cli/telemetry/sender.ts	94.54%	6 Missing ⚠️
packages/aws-cdk/lib/cli/io-host/cli-io-host.ts	93.33%	2 Missing ⚠️
...ckages/aws-cdk/lib/cli/telemetry/post-telemetry.ts	98.64%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1779      +/-   ##
==========================================
+ Coverage   91.10%   91.29%   +0.19%     
==========================================
  Files          80       82       +2     
  Lines       12205    12576     +371     
  Branches     1742     1773      +31     
==========================================
+ Hits        11119    11481     +362     
- Misses       1050     1059       +9     
  Partials       36       36

Flag	Coverage Δ
suite.unit	`91.29% <95.11%> (+0.19%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…so proxied delivery isn't cut off The proxy integ test failed in CI: the parent logged a successful hand-off but the POST never reached the MITM proxy. Root cause is the 500ms budget the sink forwarded to the child as timeoutMs. That number came from the synchronous implementation, where it existed to stop the POST from delaying the user's prompt. The sender applies it to each step of a send, and a proxied send has three sequential steps: connect + CONNECT, then a TLS handshake against the endpoint, then the response. Proxied users therefore had to complete two TLS handshakes within 500ms each. Reproduced by injecting latency in front of the real mockttp harness: at 300ms the proxy records the request, at 600ms the sender aborts with 'ProxyConnectTimeout: No CONNECT response after 500ms' and the proxy sees nothing -- which is precisely what the test observed. CI reaches that latency because the integ jest config sizes maxWorkers at 15x the core count, so the suite runs ~87 workers on a 16-core runner. Nothing waits on the sender any more, so that budget bought the user nothing and only cost us telemetry -- including for real users on slow links, which the old synchronous code silently dropped too. Decouple it: the sender owns NETWORK_TIMEOUT_MS (3s per step, matching what NetworkDetector already treats as a reasonable background budget), the sink no longer forwards a timeout at all, and HARD_KILL_MS rises to 20s so the worst case (3 x 3s, plus reading stdin) stays comfortably inside the ceiling and the ceiling remains a backstop against a genuinely stuck socket. The parent is untouched: it still only spawns and unref()s, so a larger child budget is invisible to the user. Also makes the test hermetic. It relied on the real production endpoint, so every CI run posted live telemetry and put DNS plus internet egress inside the latency-critical path that this bug was sensitive to. TELEMETRY_ENDPOINT now points at a local https server behind the same proxy, which still exercises CONNECT and CA verification -- the assertion is on the request the proxy decrypted, which is the CLI -> proxy hop under test. Refs D488314716

… (+ review nits) upgradeToTls passed a socket, a servername and a CA to tls.connect but no host. servername drives SNI and is deliberately omitted for IP literals (which may not be sent as SNI), so for an IP-literal endpoint Node had nothing to match the certificate against and fell back to the underlying socket's host -- which on this path is the PROXY. A certificate issued for the proxy's name was therefore accepted for a connection intended for the endpoint. Confirmed before fixing, with a certificate whose SAN is DNS:localhost only, tunnelling to https://127.0.0.1:<port> through a proxy reached as 'localhost': before ACCEPTED (authorized=true) after REJECTED (ERR_TLS_CERT_ALTNAME_INVALID) Passing host: hostname fixes it -- host drives the identity check, servername still drives SNI, so nothing changes for hostname endpoints. Four tests cover this: the IP-literal regression, its mirror image so it is not merely asserting that IP literals never work, and hostname-mismatch rejection on both the direct and proxied paths. Only signer trust was tested before, never identity. Review nits, all in the same area: - openTunnel now replays bytes a proxy delivers in the same chunk as its CONNECT response instead of discarding them. This needs socket.pause() first: removing our data listener does not stop the socket flowing, and unshifting into a flowing stream silently drops the data -- the test caught exactly that. - The oversized-response guard is checked unconditionally rather than only while the terminator is missing, so it also fires when a terminator arrives inside an oversized chunk. - postOverSocket gained a symmetric cap so a server that never terminates its headers cannot grow the buffer without bound inside the timeout. - proxyUrl resolution uses ?? rather than ||. The parent forces its configured value whenever --proxy is set at all, including to an empty string meaning 'no proxy'; the child used to treat that as unset and fall back to environment auto-detection, so the two could disagree about whether a proxy applies. - Corrected the ca doc comment: Node's ca option REPLACES the default trust set rather than adding to it. - Noted that bin/cdk's top-level return relies on the CommonJS module wrapper. - The child's swallowed spawn/stdin error listeners now emit a CDK_TELEMETRY_SENDER_DEBUG-gated trace, so a silent delivery failure is at least debuggable. Written synchronously to fd 2 because these fire after the IoHost may be gone, and wrapped so diagnostics can never break the never-throw discipline. Refs D488314716

github-actions · 2026-08-01T16:30:00Z

Total lines changed 1286 is greater than 1000. Please consider breaking this PR down.

iliapolo · 2026-08-14T18:44:25Z

-    // Check the trace that telemetry was not executed successfully
-    expect(output).not.toContain('Telemetry Sent Successfully');
+    // Check the trace that telemetry was never handed to a sender
+    expect(output).not.toContain('Telemetry dispatched');


But now our tests won't ensure telemetry is actually being sent - asserting on a dispatch is not enough. We need a way for the sender to communicate back to the test that the telemetry endpoint responded with 200.

What we probably need is to stand up an HTTP server, set that as the telemetry endpoint, then assert on what gets sent to that endpoint.

Or in this case, we need to assert that after X seconds, we still didn't get any data POSTed to that endpoint.

And that holds for all tests, it will be a better one than asserting on the log line.

iliapolo · 2026-08-14T18:58:25Z

+      // The detached sender is what hangs on the black hole, not us. The headroom is generous
+      // because CI machines are noisy; what this rules out is the CLI blocking on the request
+      // timeout, which shows up as whole seconds.
+      expect(overhead).toBeLessThan(2000);


Add a note that this must assert on a number lower than our request timeout:

aws-cdk-cli/packages/aws-cdk/lib/cli/telemetry/sender.ts

Line 45 in 5a8df75

const NETWORK_TIMEOUT_MS = 3_000;

iliapolo · 2026-08-14T19:21:33Z

+/**
+ * Resolve the proxy to use for `endpoint` from proxy environment variables.
+ *
+ * Faithfully re-implements `proxy-from-env@1`, which is what `proxy-agent` falls back to in the


This introduces massive amounts of code - how do we know it works?

An alternative would be to create a new CLI entry point that depends on the standard proxy-agent package and configure it for bundling as well.

aws-cdk-cli/.projenrc.ts

Lines 1450 to 1452 in 750d650

entryPoints: [

'lib/index.js',

],

We already have two entrypoints in cdk-assets for example:

aws-cdk-cli/.projenrc.ts

Lines 866 to 869 in 750d650

entryPoints: [

'bin/cdk-assets.js',

'bin/docker-credential-cdk-assets.js',

],

We should be mindful about how much will it balloon the tarball, but lets try it.

iliapolo · 2026-08-14T19:52:33Z

+    if (payloadBytes > MAX_DISPATCH_PAYLOAD_BYTES) {
+      // Writing this much to the child's stdin would block our own exit. Drop the batch; it is
+      // not going to get smaller on a retry.
+      await this.ioHelper.defaults.trace(`Telemetry dropped: payload of ${payloadBytes} bytes exceeds ${MAX_DISPATCH_PAYLOAD_BYTES}`);


Previously when the payload was large we would have seen evidence for that on the server logs (because the request was still being sent) - now we won't even try so we might be blind to a sudden increase in large payloads.

Instead of dropping, rewrite the payload to indicate that the original payload was too large - and send that one. We can then add a metric on the server we can track to see if this happens a lot.

rix0rrr

There is still a lot of extraneous detail in the PR body that you could get rid of.

A reader of this PR cares about what we do to the CLI and why. They don't need to care about all the things we tried to do to the server.

The bottom line is:

Posting to an HTTPS server can take 3-4 RTTs, and with latencies of potentially ~100ms (or more) that becomes significant.

No more explanation necessary than that.

rix0rrr · 2026-08-18T07:45:10Z

-    // Check the trace that telemetry was not executed successfully
-    expect(output).not.toContain('Telemetry Sent Successfully');
+    // Check the trace that telemetry was never handed to a sender
+    expect(output).not.toContain('Telemetry dispatched');


What we probably need is to stand up an HTTP server, set that as the telemetry endpoint, then assert on what gets sent to that endpoint.

Or in this case, we need to assert that after X seconds, we still didn't get any data POSTed to that endpoint.

And that holds for all tests, it will be a better one than asserting on the log line.

rix0rrr · 2026-08-18T07:47:08Z

+    const blackHole = net.createServer((socket) => {
+      // Accept and hold. Never respond, never close.
+      sockets.push(socket);
+    });
+    await new Promise<void>((ok) => blackHole.listen(0, '127.0.0.1', ok));
+    const port = (blackHole.address() as AddressInfo).port;


Put all of this in a helper, not in the test itself. That way, we can reuse a server component for other integ tests.

For fun and games, make it implement I[Async]Disposable and use using (although a try/finally on a "normal" object with a dispose method will do in a pinch)

rix0rrr · 2026-08-18T07:51:49Z

+/**
+ * Telemetry has to keep working for users behind a corporate proxy.
+ *
+ * This matters more than it looks. The POST is made by a detached child process that has no access


This matters more than it looks

???????????

Please get the clanker-speech out of my sight.

rix0rrr · 2026-08-18T07:54:33Z

+    // Stand-in for the telemetry endpoint. Never actually serves a response to the proxy; it only
+    // needs to occupy a port so the CONNECT target is real.
+    const { key, cert } = await mockttp.generateCACertificate();
+    const endpointServer = https.createServer({ key, cert }, (_req, res) => {


Same -- get the details of this server out of the test. Create a disposable class or object to represent the running server.

rix0rrr · 2026-08-18T07:55:36Z

+      });
+
+      // The parent reports the hand-off, not the delivery.
+      expect(output).toContain('Telemetry dispatched');


Do we care about what it logs? I don't.

rix0rrr · 2026-08-18T08:12:43Z

+      child.on('error', (e: Error) => {
+        debugTrace(`failed to spawn sender: ${e.message}`);
+      });


Okay. We logged something. And then what, are we going to return an error from this function?

(Answer: no, we would fall through and return true)

Turn an error into a properly reported error/exception and handle/log it in one place.

rix0rrr · 2026-08-18T08:14:23Z

+ * The CLI itself resolves proxies with `proxy-agent`, which delegates to `proxy-from-env` whenever
+ * the user did not pass `--proxy`. The detached telemetry sender cannot use `proxy-agent` (it has
+ * no dependencies available), so `resolveProxy` re-implements that logic. This test pins the
+ * re-implementation to the original by running both over the same table of environments.


These should be lies. Why would we not have dependencies available?

rix0rrr · 2026-08-18T08:15:31Z

+  readonly appendAfterConnectResponse?: string;
+}
+
+async function startConnectProxy(options: ConnectProxyOptions = {}): Promise<Proxy> {


I'm pretty sure I saw a startProxyServer function somewhere already.

rix0rrr · 2026-08-18T08:16:35Z

+  close(): Promise<void>;
+}
+
+async function startEndpoint(ca: TestCa, options: { statusCode?: number; urlHost?: string } = {}): Promise<Endpoint> {


Hey this could be reused in those other tests where I complained you did inline servers.

rix0rrr · 2026-08-18T08:17:15Z

+
+const BODY = { events: [{ identifiers: { sessionId: 'test-session' } }] };
+
+describe('sender', () => {


What all are we testing here, and why?

Only conflict was in proxy-agent.ts: main added up-front proxy address validation in `create()`, and this branch changed the same signature to return `ResolvedProxyAgent`. Kept both.

…ot the cert The detached sender was written against Node built-ins only, which meant hand-rolling an HTTP CONNECT tunnel, a TLS upgrade, an HTTP/1.1 framer and a copy of proxy-from-env. That constraint was self-imposed: the child is detached and nobody waits on its load time, so it does not need to be small. Make it a proper esbuild entry point instead and let it use the real proxy-agent. SOCKS and PAC proxies work again as a result -- the hand-rolled version had to skip those users rather than risk bypassing a mandatory proxy. Also fixes a bug that silently cost every corporate-proxy user all of their telemetry: the sink forwarded the CA bundle CONTENTS in the payload and measured the whole payload against a 64KB cap. A system CA bundle is around 190KB, so those invocations were over the cap and dropped, every single time. Forward the absolute path instead and let the child read it -- which ProxyAgentProvider already knows how to do. The payload now travels in a temp file whose path is passed in argv rather than down the child's stdin, so there is no reason to cap it at all: stdin was only capped because writing more than a pipe buffer's worth would have blocked the exit this whole change exists to avoid. Other cleanups that fall out of the above: - EndpointTelemetrySink goes back to POSTing to the endpoint, as it does on main; the new SubprocessTelemetrySink owns the spawning. The POST itself is shared between them. - bin/cdk is back to its original three lines. It no longer publishes its own path in CDK_CLI_BIN_PATH or re-executes itself as a sender, so cli-bin-path.ts is gone too -- the sender is resolved from the package root directly. - ToolkitError is imported from its defining module rather than the toolkit-lib barrel. Via the barrel, esbuild pulled the entire toolkit into the sender bundle: 11.5MB for one error class, versus 1.9MB without it.

Handing the batch to a detached child means nothing in this process ever learns whether the POST worked. That was the one genuinely uncomfortable part of the design, so give it a way to be measured: the child writes {ok, statusCode, reason, at} to telemetry-last-send.json under CDK_HOME, and the next invocation reads it and reports counters.previousSendFailed on its first event. Only failures are reported -- a counter present on nearly every event tells you nothing -- and the file is consumed on read, so one failure is reported once rather than forever. It gets its own file rather than joining telemetry-state.json because the child would otherwise be racing the parent for that one. Reporting the reason as well needs a schema field, which is a conversation with the telemetry service team rather than something to sneak in here. Error handling now happens in one place per process: - The sink's dispatch() throws instead of returning a boolean that meant "should the caller keep the batch?", and flush() logs once. The batch is always cleared: delivery is one-shot, the process that would retry has usually exited, and retaining it just regrew the batch and re-logged the same failure every 30 seconds. That also settles the no-sender-path case, which never starts working mid-process. - sendTelemetry() returns the status code and lets real errors propagate rather than converting everything into a result object. Judging a non-2xx and catching failures both happen in the entry point, which is also where the breadcrumb is written. CDK_TELEMETRY_SENDER_DEBUG=1 now passes the child's stderr through instead of spawning with stdio:'ignore', which made the only field-debug tool we have unusable. Also fixes a query string being dropped from the endpoint URL: the POST path was url.pathname, so ?foo=bar was silently discarded.

…t it Nearly every telemetry test asserted on the 'Telemetry dispatched' trace, which the parent emits when it hands the batch over. That proves the hand-off and nothing else -- the POST happens in a child that outlives the CLI, so its output cannot appear in ours. Point TELEMETRY_ENDPOINT at a local HTTPS server instead and wait for the request to turn up there, which covers the whole chain: temp-file hand-off, resolving and spawning the sender, forwarding the CA path, and the request itself. The endpoint is mockttp, reusing what the proxy tests already use. That matters for a specific reason: it mints a leaf certificate for the host we ask for, signed by the CA we give it, so --ca-bundle-path genuinely has to work for delivery to succeed. A bare self-signed certificate would fail hostname verification instead, which is why the existing proxy test could only check the CLI -> proxy hop. Added: - a direct-path test that waits for the batch and checks the payload carries no certificate bytes; - a real negative test for both CDK_DISABLE_CLI_TELEMETRY and the persisted cli-telemetry --disable setting: point at a live endpoint and assert nothing arrives during a quiet period long enough that a successful delivery would have shown up; - a >64KB CA bundle test, which is the regression that started all of this. Built by concatenating certificates until it is bigger than the cap that used to drop them, the way a real system bundle is. The does-not-block test now proves both halves. It used to compare two wall-clock samples, which would also have passed if telemetry were silently broken and nothing was sent at all; it now also requires the black hole to have received a connection. Compares the fastest of two runs rather than one sample each, and states the invariant that the threshold has to stay below the sender's network timeout, or the test cannot fail. Replaced the assertion that the spawn options are shaped a certain way (detached/stdio/windowsHide/cwd handed back to us by our own mock) with one that checks the behaviour those options exist for: a driver process using the real sink exits while the sender it spawned is still running. Also corrected the proxy test's description, which still said the child had only Node built-ins and re-implemented CONNECT itself.

@default

The justification for each decision was written as a paragraph next to the code, which is the wrong place for it: it belongs in the PR, where it can be argued about and then forgotten. Cut the multi-paragraph blocks down to a line or two each and dropped the editorialising. What is left is either a one-line "why", or the @default JSDoc the repo convention requires on exported interface properties. Documented CDK_TELEMETRY_SENDER_DEBUG under Environment, and noted in the cli-telemetry section that delivery happens in the background so a failure will not show up in the CLI's output. Also documented CDK_DISABLE_CLI_TELEMETRY, which turns out never to have been listed there. CDK_TELEMETRY_SENDER is gone, so there is nothing to document.

Cut the previousSendFailed breadcrumb, close two silent failure paths, and tighten the boundaries the detached sender depends on. - Cut the `previousSendFailed` breadcrumb entirely (`last-send.ts` and its test). `counters` is a closed schema, so the key was never readable by the endpoint, and the wiring was lossy in three independent ways: the 30s flush interval let several senders race on one non-atomic file, the outcome was consumed at `begin()` but only attached when an event was emitted, and it was consumed even when the only sink was the local file sink. A replacement observability design is deferred. - Delete the orphaned `EndpointTelemetrySink` and its test. It was the sink `SubprocessTelemetrySink` replaced; wiring it back as a fallback would have reintroduced the blocking network call on the exit path that this work removes. Both hand-off failures already trace, so they now report how much was dropped rather than only why. `funnel.test.ts` was an `EndpointTelemetrySink` suite in disguise (it mocked `https.request`); it now tests the Funnel's own contract against real file sinks. - Pin TLS identity to the destination host unconditionally. `https-proxy-agent` does the TLS upgrade itself without handing that host to `tls.connect`, so an IP-literal endpoint had nothing to match against and skipped the check. This was opt-in via `verifyIdentityAgainst` with exactly one caller passing it. - Consolidate the copy-pasted deep imports of `ToolkitError` into `lib/toolkit-error.ts`, so the path that keeps the toolkit barrel out of the sender bundle is stated once. - Preserve an explicitly empty proxy across the process boundary. `--proxy ''` means "go direct"; unset means "auto-detect from the environment". `Settings.get()` is untyped and can surface unset as an empty array, so normalize at the point the setting enters typed code without collapsing `''` into `undefined`. - Integ: let mockttp pick a free telemetry-endpoint port instead of guessing one out of a range, which collides under parallel suites with no retry to recover. - Integ: budget the block-exit overhead relative to the measured baseline, floored and capped below the sender's own network budget, so a loaded runner does not flake while a real regression is still caught.

Close the spawn-failure hole, prove the cleanup path, and correct a narrative that described a bug which never shipped. Blockers: - Report a refused spawn as a failure. Node does not throw when it refuses a spawn (ENOENT, EACCES, EMFILE); it reports on the child's `error` event, which fires after the hand-off has already returned. So the realistic failures traced a successful dispatch with `pid undefined` and the batch was silently counted as sent, never reaching the drop path. libuv does leave `pid` unset synchronously, so check that and route into the existing handling. The `error` handler stays for the residual case where the spawn is accepted and fails afterwards. - Test that residual path. The handler is now pulled off the child and invoked, proving it removes the payload file -- otherwise every such failure leaks a temp file. Added coverage for the synchronous guard, and relabelled the test that mocked EMFILE as a synchronous throw, which is not a shape Node produces. - Split `cdk-telemetry-disabled-posts-nothing` into one integTest per file; it was the only file in the directory carrying two. - Drop the "64KB cap regression" framing. Verified against origin/main: no payload cap has ever existed there in any commit, and the telemetry POST is made in-process with the CA bundle passed as an `https.Agent`, so payload size is structurally unrelated to CA-bundle size. The cap existed only between two commits on this branch and never shipped. These tests pin an invariant -- the payload carries a CA path, not cert bytes, so batch size is independent of the bundle's -- so they now say that, and assert it. Also removed a stale reference to the in-process sink's 500ms budget, which this PR deletes. Nits: - Trace honesty: nothing connects to an endpoint any more, so `Endpoint Telemetry connected` / `NOT connected` become `Telemetry sink registered` / `Telemetry disabled`. Dropped the integ assertion on that string; `waitForBatch` below it is the real one. - Removed `closeConnection`, whose single caller always passed true, and the unused `diagnostics` parameter on `sendTelemetry`. - Normalize `caBundlePath` at the same boundary as `proxy`, through one shared helper: an empty array is truthy, so it slipped past every guard and reached `path.resolve([])`, whose TypeError the resolver swallowed -- silently discarding the bundle. - Exported `DISPATCHED_TRACE` for the unit test rather than repeating the literal, and replaced a poke at the sink's private `senderPath` with an injectable resolver. - Connect to 127.0.0.1 where a test binds to it and does not care about the hostname: `localhost` resolves to ::1 first on a dual-stack box under Node 18+, which would ECONNREFUSED. Kept the hostname where NO_PROXY and the CONNECT target assert on it. - Documented why `toolkit-error.ts` cannot just re-use `api-private.ts`, sorted the README env-var list, and trimmed the disabled-posts-nothing quiet periods to 5s using the existing `sleep` helper.

Both failures are this PR's own new tests, and both come from an assumption about the environment that holds locally but not on CI. SOCKS unit tests (build, collect): `socks5://`, unlike `socks5h://`, resolves the destination on the client side and puts the resulting address into the SOCKS request. The endpoint was addressed as `localhost`, so what landed there depended on how that resolved: an IPv6-first runner produced an ATYP=0x04 address, which the hand-rolled test SOCKS server does not implement -- it ends the socket, surfacing as `Error: Socket closed`. Address the endpoint by IP in those two tests, which is not resolved at all and so has the same shape everywhere. Reproduced locally with `--dns-result-order=ipv6first` (both tests failed identically) and confirmed fixed under both orders. Preferred over teaching the test server ATYP=0x04, which would have added an untested code path to test scaffolding. The certificate already covers `IP:127.0.0.1`, and `localhost` is left alone where it is load-bearing: the NO_PROXY test and the CONNECT-target assertion. Telemetry integ tests (integ_telemetry): The tests handed the throwaway endpoint CA to the whole CLI via `--ca-bundle-path`, which REPLACES the trust store rather than adding to it. The SDK's own call to a public AWS endpoint then had no issuer for it, so `STS.GetCallerIdentity` failed, the default account never resolved, the fixture app's context lookup threw StackAccountRegionNotSpecified, and `cdk synth` exited 1 -- before any telemetry assertion ran. Telemetry itself was working; the log showed the batch being dispatched. Supply the CA through `NODE_EXTRA_CA_CERTS` instead, which adds to the default store, so public roots keep verifying while the detached sender still trusts the local endpoint. Verified against the real sender binary: with no CA anywhere delivery fails, with only `NODE_EXTRA_CA_CERTS` it succeeds, and a public TLS request still verifies with it set but fails with UNABLE_TO_GET_ISSUER_CERT_LOCALLY when the store is replaced -- the CI error. The negative control matters: the endpoint's certificate is still verified, so a successful delivery still means something. The CA is kept in the two disable tests even though nothing should reach the endpoint: without a trusted CA, "nothing arrived" would also be true of an enabled run whose handshake merely failed, and those tests would pass for the wrong reason. Payload `caBundlePath` forwarding is unchanged and still covered where it can be asserted in isolation: the `reads the CA bundle from the path it was given` sender test (a real child, with a negative control) and the proxy integ test, which is left as-is.

rix0rrr · 2026-08-21T11:25:42Z

It would be helpful to re-review if you could go through our comments and either respond to them if you have questions or disagree, or close the conversation if you feel you've addressed the request. Thanks! 🙏

Roko AI Agent added 3 commits July 28, 2026 20:06

sanjanaravikumar-az had a problem deploying to automation July 29, 2026 17:56 — with GitHub Actions Failure

sanjanaravikumar-az temporarily deployed to automation July 29, 2026 17:56 — with GitHub Actions Inactive

github-actions Bot added the p2 label Jul 29, 2026

sanjanaravikumar-az temporarily deployed to no-approval July 29, 2026 17:57 — with GitHub Actions Inactive

aws-cdk-automation requested a review from a team July 29, 2026 17:57

sanjanaravikumar-az had a problem deploying to run-tests July 29, 2026 18:00 — with GitHub Actions Failure

sanjanaravikumar-az temporarily deployed to run-tests July 29, 2026 18:00 — with GitHub Actions Inactive

sanjanaravikumar-az temporarily deployed to run-tests July 29, 2026 18:06 — with GitHub Actions Inactive

sanjanaravikumar-az temporarily deployed to no-approval July 29, 2026 19:03 — with GitHub Actions Inactive

sanjanaravikumar-az temporarily deployed to run-tests July 29, 2026 19:06 — with GitHub Actions Inactive

Roko AI Agent and others added 2 commits July 29, 2026 19:47

Merge branch 'main' into sanjrkmr/telemetry-subprocess

0ebb2c4

sanjanaravikumar-az added 4 commits August 3, 2026 16:13

Merge branch 'main' into sanjrkmr/telemetry-subprocess

147ad84

Merge branch 'main' into sanjrkmr/telemetry-subprocess

6977aaf

Merge branch 'main' into sanjrkmr/telemetry-subprocess

f76604c

Merge branch 'main' into sanjrkmr/telemetry-subprocess

5a8df75

iliapolo requested changes Aug 14, 2026

View reviewed changes

rix0rrr requested changes Aug 18, 2026

View reviewed changes

sanjanaravikumar-az added 9 commits August 18, 2026 23:53

Merge branch 'main' into sanjrkmr/telemetry-subprocess

ccd9f1e

Only conflict was in proxy-agent.ts: main added up-front proxy address validation in `create()`, and this branch changed the same signature to return `ResolvedProxyAgent`. Kept both.

Merge branch 'main' into sanjrkmr/telemetry-subprocess

cca1889

	entryPoints: [
	'bin/cdk-assets.js',
	'bin/docker-credential-cdk-assets.js',
	],


		const BODY = { events: [{ identifiers: { sessionId: 'test-session' } }] };

		describe('sender', () => {

Uh oh!

Conversation

sanjanaravikumar-az commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Logic:

Uh oh!

github-actions Bot commented Jul 29, 2026

Dependency Review

Scanned Files

Uh oh!

codecov-commenter commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Aug 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliapolo Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliapolo Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliapolo Aug 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rix0rrr commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sanjanaravikumar-az commented Jul 29, 2026 •

edited

Loading

codecov-commenter commented Jul 29, 2026 •

edited

Loading

iliapolo Aug 14, 2026 •

edited

Loading

iliapolo Aug 14, 2026 •

edited

Loading

iliapolo Aug 14, 2026 •

edited

Loading