Skip to content

feat(cli): send telemetry from a detached subprocess to unblock CLI exit - #1779

Draft
sanjanaravikumar-az wants to merge 6 commits into
mainfrom
sanjrkmr/telemetry-subprocess
Draft

feat(cli): send telemetry from a detached subprocess to unblock CLI exit#1779
sanjanaravikumar-az wants to merge 6 commits into
mainfrom
sanjrkmr/telemetry-subprocess

Conversation

@sanjanaravikumar-az

@sanjanaravikumar-az sanjanaravikumar-az commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

No description provided.

Roko AI Agent added 3 commits July 28, 2026 20:06
Roko AI Agent
feat(cli): send telemetry from a detached subprocess
e91f30a 
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
Roko AI Agent
test(cli): cover the detached telemetry sender
0c31c3d 
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).
Roko AI Agent
chore(cli): address telemetry sender review feedback (accurate trace,…
b710b27 
… 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 github-actions Bot added the p2 label Jul 29, 2026
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Dependency Review

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

Scanned Files

None

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

codecov-commenter commented Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.46237% with 98 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.94%. Comparing base (d87457e) to head (94d0305).

Files with missing lines Patch % Lines
packages/aws-cdk/lib/cli/telemetry/sender.ts 88.00% 80 Missing and 3 partials ⚠️
...es/aws-cdk/lib/cli/telemetry/sink/endpoint-sink.ts 91.53% 11 Missing ⚠️
packages/aws-cdk/lib/cli/telemetry/cli-bin-path.ts 91.30% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1779      +/-   ##
==========================================
- Coverage   90.03%   89.94%   -0.09%     
==========================================
  Files          80       81       +1     
  Lines       12117    12976     +859     
  Branches     1697     1814     +117     
==========================================
+ Hits        10909    11671     +762     
- Misses       1179     1272      +93     
- Partials       29       33       +4
Flag Coverage Δ
suite.unit 89.94% <89.46%> (-0.09%) ⬇️

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.

Roko AI Agent
fix(cli): give detached telemetry sender a realistic network timeout …
adfa346 
…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
Roko AI Agent
fix(cli): enforce endpoint TLS identity on the proxied telemetry path…
94d0305 
… (+ 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

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

p2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sanjanaravikumar-az @codecov-commenter