Skip to content

Conversation

@NotVivek12
Copy link

dns: fix Windows SRV ECONNREFUSED regression by correcting c-ares fallback detection

Summary

This PR fixes a regression introduced in Node.js v24.13.0 on Windows where DNS SRV lookups (commonly used by MongoDB connection strings) fail with ECONNREFUSED.

The issue stems from a change in c-ares behavior where the fallback resolver (loopback) is reported with port 53 rather than port 0. The existing Node.js glue layer strictly expected port 0 to identify a fallback scenario. Consequently, Node.js failed to detect the fallback, attempting to query a non-listening local stub resolver at 127.0.0.1:53 or [::1]:53.

Changes

  1. **Updated ChannelWrap::EnsureServers**: Modified logic to treat any single loopback server (IPv4 127.0.0.1 or IPv6 ::1) as a fallback configuration, regardless of the port number reported by c-ares.
  2. Proactive Validation: Added a call to EnsureServers() immediately before dispatching queries in the Query template to ensure the channel is correctly initialized before use.

Regression Details

  • Affected Version: Node.js v24.13.0
  • Platform: Windows 11 / Server
  • Error: Error: querySrv ECONNREFUSED
  • Last Known Good: v24.11.1

Reproduction

This issue is 100% reproducible on Windows with Node v24.13.0 using the following script (mimicking a standard Mongoose connection):

const express = require("express");
const mongoose = require("mongoose");
const app = express();

const connectDB = async () => {
  try {
    // Replace with a valid SRV URI
    const conn = await mongoose.connect("mongodb+srv://<user>:<pass>@cluster0.example.mongodb.net/db");
    console.log(`MongoDB Connected: ${conn.connection.host}`);
  } catch (error) {
    console.error('MongoDB connection failed:', error.message);
    process.exit(1);
  }
};
connectDB();

app.listen(3300, () => {
  console.log("mongodb connected at port 3300");
});

Output on v24.13.0:
Error: querySrv ECONNREFUSED _mongodb._tcp.cluster0.example.mongodb.net

Manual Verification

  1. Built Node.js from source on Windows (VS 2022 Build Tools).
  2. Ran the reproduction script above; connection succeeds.
  3. Verified dns.getServers() no longer returns a single loopback address when the system resolver is active.

Risks

Low.

  • The logic only activates when c-ares discovers a single loopback server and the user has not manually called setServers().
  • If setServers() is called, is_servers_default_ becomes false, skipping this check entirely.
  • The logic is platform-agnostic but primarily resolves the specific behavior observed on Windows.
Workaround for users Users on v24.13.0 can temporarily work around this by forcing DNS servers before connection:
require('dns').setServers(['8.8.8.8', '1.1.1.1']);

Checklist

  • Fixes regression in dns / c-ares
  • Tested on Windows
  • Tests added/verified (Manual verification performed)

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. labels Jan 20, 2026
@Ethan-Arrowood
Copy link
Contributor

Do you think you could add a tests so we don't see this regression again in the future? Thanks!

@NotVivek12
Copy link
Author

Do you think you could add a tests so we don't see this regression again in the future? Thanks!

Absolutely, happy to add a test. Did you have a specific scenario in mind, or should I just cover the case from the original bug report?

@NotVivek12
Copy link
Author

Sure, I've added regression tests for this! Created two test files:

test-dns-resolvesrv.js- basic SRV resolution tests
test-dns-resolvesrv-econnrefused.js- specifically tests the ECONNREFUSED regression with MongoDB Atlas-style SRV lookups
Also added SRV record support to dns.js so we can mock SRV responses.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you @NotVivek12! This largely looks good, just have two questions here

@codecov
Copy link

codecov bot commented Jan 27, 2026

Codecov Report

❌ Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.77%. Comparing base (68d7b6f) to head (8b9e334).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/cares_wrap.cc 11.11% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61453      +/-   ##
==========================================
- Coverage   89.77%   89.77%   -0.01%     
==========================================
  Files         673      673              
  Lines      203820   203825       +5     
  Branches    39175    39180       +5     
==========================================
- Hits       182987   182981       -6     
- Misses      13152    13175      +23     
+ Partials     7681     7669      -12     
Files with missing lines Coverage Δ
src/cares_wrap.cc 61.73% <11.11%> (+1.42%) ⬆️

... and 27 files with indirect coverage changes

🚀 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.

@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 28, 2026
@addaleax
Copy link
Member

@NotVivek12 Can you look at the linter failures? Looks like there's some failing tasks

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 28, 2026
@nodejs-github-bot
Copy link
Collaborator

@NotVivek12
Copy link
Author

@NotVivek12 Can you look at the linter failures? Looks like there's some failing tasks

Hello, ive made the changes trying to resolve these errors. Hope it works.

@NotVivek12 NotVivek12 requested a review from addaleax January 29, 2026 04:01
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this file was unintentionally committed

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 30, 2026
@addaleax
Copy link
Member

Kicked off CI to see how well this does, but we'll probably need to remove package-lock.json again before this can be merged

@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jan 30, 2026
@github-actions
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fix fix Windows SRV ECONNREFUSED regression
   ⚠  - Merge branch 'main' of https://github.com/NotVivek12/node
   ⚠  - Merge branch 'nodejs:main' into main
   ⚠  - add regression tests
   ⚠  - remove unintentional file
   ⚠  - Merge branch 'nodejs:main' into main
   ⚠  - Merge branch 'main' of https://github.com/NotVivek12/node
   ⚠  - fix CI/CD errors
   ⚠  - Merge branch 'nodejs:main' into main
   ⚠  - Merge branch 'main' of https://github.com/NotVivek12/node
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/21514988012

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants