Repository navigation
fix(cli): a finished capture exits even when a site's DNS never answers - #5983
Conversation
Capture resolved every page request with the system lookup and gave up after 10 s, but a system lookup cannot be cancelled: the abandoned one kept the process alive until the OS resolver timed out, so a capture that had printed its result could run past the app's 5-minute cap and lose its folder. Resolve through c-ares instead, as hostAnswers does, and cancel it at the same 10 s deadline.
Edit accuracy: accurate 271 (base branch 271), smooth 157 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
Dropping the timer cleanup kept every successful lookup alive 10 s and no test noticed.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed exact b010e03dabd56382d19f7553ed6b387d8d7fe550, requested by Miguel. No blocking regression found.
packages/cli/src/capture/publicNetwork.ts:55–70 replaces the uncancellable system lookup with an independently cancellable resolver per query, cancels at the existing10-second deadline and clears the timer on every settled path. All A answers still pass the public-address policy before HTTP/CONNECT dial the checked numeric peer; TLS hostname validation remains unchanged. No forced process exit is introduced.
Independent exact-source controls used real local UDP DNS and Node children: public answer, private rejection, silent lookup and20 concurrent silent lookups. Queries settled/cancelled as expected and children exited5–7ms after printing. Read implementation, changed tests/fixtures, proxy and asset/browser teardown callers; author suite/browser counts were not independently rerun. No complete capture, macOS or Windows run claimed.
Follow-up: publicNetwork.ts:90 rejects on caller abort without cancelling the resolver. A real silent-DNS control aborting at20ms printed its result promptly but stayed alive about9,992ms until the query deadline. This is bounded existing abort behavior, not the prior unbounded OS lookup; the body’s “Nothing it starts outlives the capture” claim is too broad. Consider wiring the caller signal/proxy teardown to resolver cancellation and adding an abort-during-DNS child-exit control. The new process tests prove exit after resolver settlement, not immediate exit after every capture teardown.
The documented hosts-file/cache/managed-DNS behavior changes remain platform limitations; they were not reproduced here.
Need: yes · Size: focused DNS lifecycle repair · Reuse: same c-ares choice as hostAnswers and shared test DNS server · Simpler: per-query cancellation is sufficient without forced exit or a new cache.
— hf-review
Verdict: APPROVE
Reasoning: The stalled system-lookup lifetime is removed while address pinning and cleanup hold; the remaining caller-abort delay is bounded and identified for follow-up.
…result is out c-ares answers first. When it has no answer, the system lookup (hosts file, OS-only DNS) runs in a child process killed at the 10 s deadline, cached per host, because Node waits for an in-process system lookup even in process.exit. Capture then asks the CLI to exit 100 ms after printing its result, as render does.
…ld on exit The hosts file answers before DNS, as the system lookup does (overrides and 0.0.0.0 blocks keep working). A host whose system answer is cached skips c-ares, a caller that joins a running lookup keeps its own deadline, and a pending lookup child is killed when the CLI exits.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 00d153feb81dfdd9021d8b433ef60a90dbd3dd52, including the full delta from my earlier approval at b010e03d. Hosts-file precedence, cached system answers before c-ares, joined-call deadlines, child timeout/exit cleanup, and the real child-script test are now implemented. The shared render exit helper is extracted without changing render's behavior.
P2 blocker — drain the capture result before forced exit. packages/cli/src/commands/capture.ts:309 now schedules root process.exit 100 ms after console.log. That grace period does not ensure an asynchronous stdout pipe has flushed. With the actual scheduleCliExit/root-exit callback, a 1 MiB JSON result and a reader pausing 200 ms per chunk, the child exits 0 after delivering only 146,176 of 1,048,599 bytes; JSON parsing fails. No artificial keepalive is needed. Capture titles come from unbounded document.title (capture/tokenExtractor.ts:38,517), and the command also prints fonts/warnings. Previously the pending stdout write kept the process alive. Await stdout/stderr write completion or an equivalent drain before terminating; add a real slow-pipe process test asserting complete JSON plus exit status.
Independent validation at this head: 87 tests pass across publicNetwork unit/integration/process, hostAnswers, capture command, and commandResult. A separate adversarial pass found the truncation, and I independently reproduced it. Read the resolver, proxy/asset callers, child lifecycle, command output, root finalizer and render helper extraction. No full website capture, Windows, or encrypted-DNS Mac run claimed.
Minor: capture/publicNetwork.ts:95 retains only the first hosts-file address, so “validate every answer” no longer includes later entries for that hostname. Numeric peer pinning still prevents private dialing; this is a parity gap, not a demonstrated SSRF.
Need: yes · Size: coherent lifecycle fix · Reuse: shared commandResult exit ownership and existing DNS fixture · Simpler: keep the bounded child lifecycle, but make result delivery completion—not elapsed grace—the exit prerequisite.
— hf-review
Verdict: REQUEST CHANGES
Reasoning: The DNS fixes address the earlier limitations, but newly forcing capture exit can report success with a truncated, unusable JSON result.
…le result The root exit now waits for stdout and stderr to hand over what is already written before it calls process.exit, so a large JSON result on a slow pipe arrives complete. A hostname listed several times in the hosts file now has every address checked, not only the first.
The output wait is now a helper that only waits; cli.ts still owns the process.exit. The process-ownership check treats test fixtures like tests, since each one runs as its own process.
A process.exit outside a process root used to pass the local hooks and fail only in CI's Preflight.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-reviewed at 5a6379a64aebbbac84cb3a0b63b5e7aab3ae5eba, including the changes since my request for changes at 00d153fe.
The output-truncation blocker is resolved: packages/cli/src/cli.ts:356–359 waits for ordered stdout/stderr write completion through utils/commandResult.ts:111–115 before forced exit. An independent actual-helper process test with readers pausing 200 ms per chunk received complete 1,048,589-byte JSON stdout and 1,200,000-byte Unicode stderr, then exit 0. The hosts parser now aggregates every matching IPv4 line before public-address validation, resolving the earlier minor finding.
All 89 focused DNS unit/integration/process, hosts, capture and command-result unit/process tests pass; ownership checks and their three tests pass. The fresh code pass rechecked child timeout/exit cleanup, cache/deadline paths and the unchanged numeric-peer pinning contract. The additional fixture classification and pre-commit check preserve root process ownership. No new full browser capture, Windows or encrypted-DNS Mac validation is claimed.
Need: yes · Size: focused on DNS/output lifecycle · Reuse: shared command-result/root-exit boundary and resolver validation · Simpler: ordered write callbacks repair the existing exit path without a second output buffer.
— hf-review
Verdict: APPROVE
Reasoning: The previous slow-pipe failure is repaired and independently reproduced as passing, with no remaining blocker found in the reviewed lifecycle paths.
What changes
hyperframes captureresolves every page request through its own proxy (resolvePublicAddress). It used the system lookup with a 10 s give-up, but a system lookup cannot be cancelled: when a page loads a host whose DNS never answers (a filtered tracker, a dead CDN name), the capture moves on and prints its result, and the process then stays alive until the OS resolver times out. On macOS that is tens of seconds to minutes, so a capture that had already succeeded could run past the desktop app's 5-minute cap, be stopped, and lose its folder.A host is now resolved in this order, giving the answer the system lookup would give:
/etc/hosts, or Windows'drivers\etc\hosts), read once, IPv4 entries; the first line for a name is used and every line for it must be public. Overrides and0.0.0.0blocks keep working.dns.Resolver), cancelled after 5 s, the same choicehostAnswersmakes.process.exit(0)for 30.1 s (Node waits for its lookup threads at exit).Capture also asks the CLI to exit 100 ms after printing its result (
scheduleCliExit, moved out ofrender.tsso both commands share it). The root exit incli.tsfirst waits (outputWritten) until stdout and stderr have handed everything already written to the reader, so a large result on a slow pipe arrives whole; a reader that never reads keeps the process, as it did before. The process-ownership check now treats*.fixture.tslike tests: each fixture runs as its own process. The lefthook pre-commit now also runs that check: aprocess.exitoutside a process root passed the local hooks and failed only in CI's Preflight (it did on this PR).Cost: hosts-file and c-ares answers spawn nothing. A host c-ares cannot answer costs one child, about 41 ms (32-48 ms on a loaded Linux box), once per host per capture. If c-ares times out rather than failing fast, that first request for the host also waits up to 5 s; later requests reuse the answer.
Proof
Same source, same private network namespace on Linux: the page loads one image from a host whose DNS server reads queries and never replies (glibc timeout 60 s).
On main, the handles alive after the result were only pending system lookups (
GetAddrInfoReqWrap). The released CLI behaves the same as main here.Tests
publicNetwork.process.test.ts(new, real child processes; 8 cases, 3 runs in a row): no DNS and a stalled system lookup times out and the process is gone within 1 s of printing; the system answer is used when DNS has none; a DNS answer is used without the system; the hosts file wins over DNS; a0.0.0.0hosts entry is refused, also as a second line after a public one; a CLI that exits mid-lookup leaves no lookup child running; the real child script resolveslocalhost.publicNetwork.test.ts: a system answer is checked like a DNS one and asked once per host; a caller joining a running lookup keeps its own deadline and skips c-ares; DNS answers never start the child.commandResult.process.test.ts(new, 3 runs in a row): a CLI held by a stalled handle prints a 1 MiB JSON result, forced exit runs, and a reader that pauses 100 ms per chunk gets all of it with exit 0. Without the wait it got 146,176 bytes.capture.test.ts: after printing its result, or after a capture that threw, capture asks to exit (0 or 1) within 100 ms.cancel(); no c-ares timer cleanup; exit without waiting for output; hosts file keeping only the first line.Reviews (independent agents)
Not exercised: Windows beyond CI, a real encrypted-DNS Mac.