Skip to content

[WIP] Fix DNS query logging issue in mitmproxy package#582

Merged
cute-omega merged 1 commit intomasterfrom
copilot/fix-dns-query-logging-issue
Mar 26, 2026
Merged

[WIP] Fix DNS query logging issue in mitmproxy package#582
cute-omega merged 1 commit intomasterfrom
copilot/fix-dns-query-logging-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 26, 2026

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.

Original prompt

Repo: docmirror/dev-sidecar
Failing GitHub Actions job: https://github.com/docmirror/dev-sidecar/actions/runs/23532778425/job/68500401240#step:13:163
Failing commit ref: 56b3f18

Observed failure (from job logs):

  • During test packages/mitmproxy, Node.js v22.22.1
  • Crash at packages/mitmproxy/src/lib/dns/base.js:215 while constructing an error log message.
  • Root cause: _doDnsQuery() references variable type in log template strings, but type is not defined in that function scope. This throws when DNS query fails and the .catch() branch logs.

Fix:

  1. Update packages/mitmproxy/src/lib/dns/base.js to define type within _doDnsQuery() before it is used in log messages. Compute it from options.family consistently with _lookup() logic: const type = options.family === 6 ? 'AAAA' : 'A'.
  2. Run/adjust tests as needed; ensure lint/tests pass.

Acceptance criteria:

  • CI job test packages/mitmproxy no longer crashes with ReferenceError/undefined type.
  • Error logging continues to include the DNS record type.
  • Minimal change; no behavior change besides preventing crash.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Please find a solution for this failing job. Use the logs, job definition, and any referenced files where the failure occurred. Keep your response focused on the solution and include code suggestions when appropriate.

Assistant: The job is failing during test packages/mitmproxy on Node.js v22.22.1 with a runtime exception coming from:

  • packages/mitmproxy/src/lib/dns/base.js around line 215
  • Error happens while logging an error message inside _doDnsQuery().catch(...)

In the logs you can see the crash point:

  • .../packages/mitmproxy/src/lib/dns/base.js:215
  • Node.js v22.22.1
  • then ELIFECYCLE Test failed

Root cause

In packages/mitmproxy/src/lib/dns/base.js, inside _doDnsQuery() the code logs with a template string that references type, but type is not defined in that scope.

From the file:

  • _lookup() defines const type = options.family === 6 ? 'AAAA' : 'A' (line ~161)
  • _doDnsQuery() uses ${type} in its error logs (lines ~213–216) but does not define it

That becomes a ReferenceError when a DNS query fails (which is common/flaky in CI), and that unhandled error fails the test run.

This explains why it only blows up when there is a DNS error path—normal successful DNS responses won’t hit that code path.

Fix (code change)

Define type inside _doDnsQuery() (or compute it from options.family) before using it in the log statements.

Suggested patch for packages/mitmproxy/src/lib/dns/base.js (around line 186+):

_doDnsQuery (hostname, options = {}, start = null) {
  if (start == null) {
    start = Date.now()
  }

  // FIX: define type in this scope (used in logs below)
  const type = options.family === 6 ? 'AAAA' : 'A'

  return new Promise((resolve, reject) => {
    // ...
    this._dnsQueryPromise(hostname, options)
      .then((response) => {
        // ...
      })
      .catch((e) => {
        // existing logging that references `type` is now safe
        // ...
        reject(e)
      })
  })
}

Alternative (slightly cleaner): pass type into _doDnsQuery() from _lookup() and avoid recomputing it, but the above minimal fix is enough.

Recommended CI stabilization (workflow tweak)

Even after the crash is fixed, DNS integration tests can still be flaky on GitHub-hosted runners. If the packages/mitmproxy tests intentionally hit real upstream DNS-over-UDP/TCP/HTTPS endpoints, consider either:

  • mocking network calls in unit tests, or
  • separating “network/integration” tests and skipping them in CI unless explicitly enabled.

But the immediate job failure is the undefined type reference; fixing that should stop the hard crash and allow tests to properly report DNS failures rather than throwing.

User: open a pull request for your fix

Assistant: I’ve queued up the pull request creation for the fix, and I’m now waiting for your confirmation/approval in the prompt so I can proceed and open it.

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@cute-omega cute-omega marked this pull request as ready for review March 26, 2026 09:36
Copilot AI review requested due to automatic review settings March 26, 2026 09:36
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cute-omega cute-omega merged commit 315a64e into master Mar 26, 2026
1 check failed
Copilot AI requested a review from cute-omega March 26, 2026 09:37
Copilot stopped work on behalf of cute-omega due to an error March 26, 2026 09:37
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.

3 participants