Skip to content

fix(preflight): bound DNS lookup by timeout and shutdown - #625

Open
lxcario wants to merge 2 commits into
TestSprite:mainfrom
lxcario:fix/target-url-preflight-dns-timeout
Open

lxcario wants to merge 2 commits into
TestSprite:mainfrom
lxcario:fix/target-url-preflight-dns-timeout

Conversation

@lxcario

@lxcario lxcario commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

What does this PR do?

Target URL preflight currently awaits DNS before arming its timeout or observing shutdown. A stalled resolver can therefore hold a run command indefinitely before the trigger request, and Ctrl-C cannot settle the probe while DNS is pending.

Start critical-operation tracking and one shared 8-second DNS/HTTP deadline before the lookup. Stop waiting promptly on shutdown or timeout, clear the deadline timer on every exit, and remove the DNS abort listener. Late resolver results cannot start HTTP; late rejections remain handled.

DNS timeouts retain the existing advisory policy. Interruption propagates the original reason without dispatching a run. Existing DNS refusal/proxy handling, resolved-address checks, redirect behavior, literal-IP handling, and --skip-preflight remain covered.

Related issue

Closes #502

Season 4 CLI Improvement Bonus

  • I'm entering this PR for the Season 4 CLI Improvement Bonus

Discord username: resquwue

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Documentation only
  • Build / CI / chore

Checklist

  • PR targets the main branch.
  • Commits follow Conventional Commits.
  • npm run lint and npm run format:check pass.
  • npm run typecheck passes.
  • The full unit suite passes under npm run test:coverage, with all four coverage measures above 80%.
  • New behavior is covered by unit tests (mock-based; no network or credentials required).
  • No secrets, API keys, internal endpoints, or personal data are included.
  • The shared deadline and interruption behavior are documented in DOCUMENTATION.md.

Notes for reviewers

  • Added 20 deterministic regression cases covering stalled DNS, interruption and already-aborted signals, late resolver resolution/rejection, the shared DNS/HTTP budget, cleanup, and command dispatch/advisory behavior. Against the original production source, 16 fail and four cleanup checks pass; all pass with the fix.
  • Windows, Node 26.5.1: full unit suite 4,809 passed, 11 skipped, across 123 files. Coverage: 92.15% lines, 89.98% statements, 85.00% branches, 83.67% functions. The changed preflight module has 98.92% line coverage. Lint passes with existing unused-disable warnings; formatting and typecheck pass. Build, --version, and --help pass.
  • An offline check of the built module settled interruption in 5 ms and a never-settling DNS lookup in 8,010 ms, with zero HTTP calls. A late DNS rejection produced no unhandled rejection.
  • npm run test:e2e -- --testTimeout=15000: 101 passed, 10 skipped, across eight passing and two skipped files. With the default 5-second limit, the agent-install test that launches all eight targets times out on this Windows machine; the identical isolated test also times out with the original production source (7.23 s). No repository timeout setting or unrelated test was changed.
  • The underlying system DNS lookup is not cancellable; this bounds the caller's wait rather than claiming to cancel resolver work. No runtime dependency is added.
  • The reachability probe runs on the CLI's machine, while tests run in AWS Lambda. Reachability from the CLI does not prove reachability from the test runner; ambiguous failures remain advisory, and --skip-preflight remains available.

Summary by CodeRabbit

  • Bug Fixes
    • Public target URL checks now share an 8-second time limit across DNS resolution and the HTTP probe. If DNS takes too long, the check reports a warning and the run can continue.
    • Interrupting a run during DNS resolution stops the check promptly without sending probe or trigger requests, allowing the run to exit cleanly.
  • Documentation
    • Clarified that the shared 8-second time limit for public target URL checks covers both DNS resolution and the HTTP probe.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: TestSprite/testsprite-cli/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a329c073-ae1c-475a-bd32-32ab3ab4c877
📥 Commits

Reviewing files that changed from the base of the PR and between 87a4a54 and a674216.

📒 Files selected for processing: 1
  • src/lib/target-url-preflight-lifetime.test.ts
🚧 Files skipped from review as they are similar to previous changes: 1
  • src/lib/target-url-preflight-lifetime.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.

Walkthrough

The preflight now places DNS resolution and HTTP probing under one 8-second lifetime. DNS timeout produces an advisory, while shutdown and interruption propagate. Tests cover stalled and late DNS results, shared-deadline behavior, and cleanup. Documentation describes the shared budget.

Changes

Target URL preflight lifetime

Layer / File(s) Summary
Bound and interrupt DNS lookup
src/lib/target-url-preflight.ts, src/lib/target-url-preflight-lifetime.test.ts, src/commands/test.run.spec.ts
DNS lookup now runs within the critical operation and shared deadline. Timeout returns an advisory, while shutdown and interruption propagate. Tests cover stalled DNS, cancellation, and late lookup results.
Apply the shared deadline to HTTP
src/lib/target-url-preflight.ts, src/lib/target-url-preflight-lifetime.test.ts, DOCUMENTATION.md
The HTTP probe uses the remaining shared budget. Tests check the shared timeout and cleanup across outcomes. The documentation states that DNS and the HTTP request share an 8-second budget.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant probeTargetUrl
  participant dnsLookup
  participant fetchImpl
  probeTargetUrl->>dnsLookup: Resolve hostname within shared deadline
  dnsLookup-->>probeTargetUrl: Return DNS result
  probeTargetUrl->>fetchImpl: Start HTTP probe within remaining budget
  fetchImpl-->>probeTargetUrl: Return HTTP response
Loading
Merge Risk: ⚪ Minimal · up to a6742

The preflight bounds DNS and HTTP within one deadline and stops on shutdown. No merge-blocking issue was found.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: adding timeout and shutdown handling for DNS lookup during preflight.
Linked Issues check Passed The PR meets the coding requirements in #502. src/lib/target-url-preflight.ts applies one 8-second budget to DNS and HTTP, observes shutdown during DNS, cleans up the timer and DNS abort listener, a…
Out of Scope Changes check Passed The production change, documentation update, and tests directly support #502. The run-command tests verify the required no-dispatch interruption behavior. No unrelated feature, dependency, retry, or D…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files.
Full details: Linked Issues check

Explanation

The PR meets the coding requirements in #502. src/lib/target-url-preflight.ts applies one 8-second budget to DNS and HTTP, observes shutdown during DNS, cleans up the timer and DNS abort listener, and handles late DNS settlement. DNS timeout keeps the advisory policy. Interruption propagates without HTTP or trigger dispatch. The added tests cover stalled DNS, interruption, already-aborted signals, late resolution and rejection, shared-budget behavior, cleanup, and preserved refusal, advisory, literal-IP, proxy, redirect, and --skip-preflight behavior.

Full details: Out of Scope Changes check

Explanation

The production change, documentation update, and tests directly support #502. The run-command tests verify the required no-dispatch interruption behavior. No unrelated feature, dependency, retry, or DNS64 change is shown.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

🏆 Season 4 CLI Improvement Bonus: entry recorded. Thanks, @lxcario!

To be counted, this PR needs the following by Oct 29, 11:59 PM PT:

  • ✅ A linked issue assigned to you.
  • ✅ Out of draft.
  • ✅ A Discord username in the description.

Rewards go to merged PRs judged a genuine improvement, and up to three bonus-eligible PRs count per person. Untick the box to withdraw the entry. This comment refreshes when you edit the PR. Full rules.

@github-actions github-actions Bot added the s4-cli-bonus Entered for the Season 4 CLI Improvement Bonus label Oct 22, 2026
@lxcario
lxcario marked this pull request as ready for review October 22, 2026 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

s4-cli-bonus Entered for the Season 4 CLI Improvement Bonus

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hackathon] Target URL preflight leaves DNS lookup outside its timeout and abort handling

1 participant