Skip to content

fix(telemetry): cancel pending sends before CLI exit - #295

Open
XXPermanentXX wants to merge 2 commits into
mainfrom
fix/telemetry-exit-294
Open

XXPermanentXX wants to merge 2 commits into
mainfrom
fix/telemetry-exit-294

Conversation

@XXPermanentXX

Copy link
Copy Markdown
Collaborator

When telemetry stalls, flushTelemetry() currently stops waiting after its budget but leaves DNS/HTTP work alive, so a command can print its complete result and still take tens of seconds to exit. This change cancels that work within the telemetry budget and releases unused response bodies.

Related to #294. This PR is stacked on #293 (output-contract-review); its diff contains only this telemetry fix. After #293 lands, retarget or rebase onto main as needed. Keep #294 open until the published Windows binary has been verified.

Changes

  • Remove the independent DNS preflight. Give each send a private cancellation signal and a 1-second deadline; flush cancels pending sends when its own budget expires.
  • Release unused responses and finish the private send scope on completion, including Bun's native-fetch cleanup. Node uses an owned dispatcher with cancellable direct/proxy TLS connections; business/global dispatchers remain intact.
  • Preserve event fields/encoding, batching, opt-out switches, and existing CLI proxy environment selection/fallback. Do not change business commands, business request timeouts, or runtime exit handlers.
  • Timeout-cancelled events are not retried. Other transport failures can be retried from the in-memory queue on a later send in the same process; there is no cross-process replay.
  • Add 11 unit tests and 15 real-process regressions; retain all existing tests. Extend the telemetry maintenance checklist with cancellation and actual-exit requirements.

Validation

  • Final-tree related regression: 98/98 passed across 10 files, including telemetry, proxy, Ctrl-C, EPIPE, concurrent invocations, and output/error behavior.

  • Final full offline run (Node 26.13.0, Bun 1.3.14, 2 workers): 8,280 passed / 297 gated / 2 failed, 401 files, approximately 692 seconds. This is not an all-green full run. Failures were the existing compiled-binary version check (macOS SIGKILL) and a Sandbox polling-timeout assertion receiving the lower-level request-timeout message.

  • Isolated rerun of both existing suites: 59/61 passed; all 13 Sandbox tests passed, while two compiled-binary startup cases received SIGKILL. On unchanged telemetry baseline 6f0cd3d0, the same two binary SIGKILL failures reproduced; the selected Sandbox recovery cases passed. These paths do not invoke the changed telemetry send (direct HTTP probe / telemetry-disabled runner), and no existing test was altered to hide the failures.

  • Normal pre-commit hooks passed: regenerated skill assets without adding unrelated changes, then ran vp check --fix. git diff --check passed; the committed contents exactly match the tested tree.

  • Separate Node 26.13.0 / Bun 1.3.14 loopback probes: successful HTTPS CONNECT with proxy authentication, NO_PROXY bypass, and stalled CONNECT cancellation all passed (6 cases). Successful delivery retained the exact POST payload; the shared dispatcher stayed intact. Stalled CONNECT sends cancelled at about 1 second and both child processes exited naturally.

  • Same-machine Bun 1.3.14, identical fake business gateway and 6-second stalled telemetry, three runs each: median process exit 6.322 -> 1.312 seconds for stalled headers and 6.330 -> 0.337 seconds for stalled body. Business JSON and exit code were unchanged. A separate 50-second stalled-header reproduction exited in 1.499 seconds after the fix, compared with 50.414 seconds before.

Follow-up for Windows verification

  • Reproduce the original command on Windows 11 / PowerShell with the release binary and npm/Node entry. Compare telemetry enabled, DO_NOT_TRACK=1, and NO_UPDATE_NOTIFIER=1 independently, recording output-complete time and actual process exit.
  • Validate the new telemetry path in a compiled binary, including healthy delivery, stalled headers/body/TLS, proxy/NO_PROXY, and native DNS stalls. Existing compiled HTTP tests are not a substitute for this acceptance.
  • The local reproduction confirms a shared lifecycle defect, not the unique cause of the reporter's Windows delay. Node can also reproduce stalled telemetry; do not conclude that completed idle sockets universally prevent exit or that Bun is the sole cause.
  • Node telemetry's private dispatcher does not inherit custom host-installed global dispatcher CA/lookup/proxy policies. Standard CLI proxy environment variables and a fixture CA via NODE_EXTRA_CA_CERTS were verified.
  • New enabled-telemetry subprocess cases cover normal/error-free completion; SIGINT/EPIPE have existing runtime coverage but have not been directly combined with slow telemetry in the new process fixture.
  • The separate npm registry configuration request in [bug]: bl lingers ~50 s after answering (Windows) #294 is outside this change.

Base automatically changed from output-contract-review to main November 1, 2026 07:33

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants