Review requested:

• @nodejs/http
• @nodejs/net

@jasnell @targos @lpinca Please review my pull request when you have a chance. Thank you!

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.06%. Comparing base (7215d9b) to head (e59f1f3).
Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main   #59203      +/-   ##
==========================================
+ Coverage   90.04%   90.06%   +0.02%     
==========================================
  Files         648      648              
  Lines      191041   191041              
  Branches    37448    37455       +7     
==========================================
+ Hits       172026   172065      +39     
+ Misses      11651    11601      -50     
- Partials     7364     7375      +11     

... and 32 files with indirect coverage changes

• ❄️ 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.

Hey @mcollina ,

Thanks for your feedback on the keep-alive timeout.

I’ve chosen 60 seconds to align with defaults used by major cloud load balancers like AWS ELB/ALB, ensuring broad compatibility and preventing unexpected connection drops. Increasing the timeout to 72 seconds, as seen in Fastify and NGINX, could slightly boost connection reuse, but also raises the risk of file descriptor exhaustion under DoS scenarios.

Overall, 60 seconds provides a safer balance between interoperability and security for most environments. I’m open to discussing further if there’s a strong case for a higher value.

@mcollina , I've resolved this issue and removed the breaking change alert regarding http.Server.keepAliveTimeout. The documentation now reflects the correct status, in line with the release team's guidance. Please review my pull request when you have a chance. Thank you!

I've seen 60 seconds cause problems due to time network delays - I've actually seen this happen. Going over 60 seconds is necessary to account for that.

Okay, sure—that makes sense. Is 72 seconds sufficient to account for those network delays, or do you recommend going even higher? Happy to adjust as needed based on your experience.

I literally picked 72 with no reasoning. We could do 65.

Thanks for clarifying. I’m fine with 65 seconds if that works better—happy to update the default to 65 to provide a bit more buffer over 60. Let me know if you have any concerns or if you think another value might be preferable.

@pras529 there are 4 frailing tests and linting to be fixed, could you address them?

@mcollina Yes, I’ll address the failing tests and linting issues—just need a bit of time to work through them.

Thanks for referencing this and working on a fix!
Looks like a great step forward, and I appreciate you taking the time to address it 🙏

Hello @mcollina,

Thank you for your review. I’ve addressed the changes you requested and updated the commit message to fully comply with our guidelines. I’m still a bit uncertain about the Linux and macOS test results—could you please take a look and let me know if anything else needs adjustment?

Thanks for your help!

I'm on the fence to consider this a breaking change or not. If so, this change will probably go out in v25,
but I think it's pretty harmless to ship in v24 as well.

@pimterry wdyt?

@pras529 can you adjust the first commit message? (You can also squash all commits too)

I'm content with 65 seconds. Appreciate this change greatly.

What's the reason for it be semver-minor and not semver-major? @mcollina

What's the reason for it be semver-minor and not semver-major?

#59203 (comment)

If we think there's a real risk that this could break anything in existing code, or if people would just prefer to be cautious, I'm not opposed to making this major, but personally I'd be quite surprised it caused any issues in practice. For users it should really just be a bug fix & performance improvement.

Do we have any specific likely scenario where this might break things?

It's hard to say its real impact, but by experience, whenever we change a default option on HTTP, it tends to break people. It's also hard to have those actions covered by our test suite.

I feel that we might want to wait for it to land on Node.js 25 and then backport after assessing its impact. I think CITGM wouldn't cover this scenario too.

@RafaelGSSy assumption is that risk in shipping on Node 24 before it goes LTS is close to zero, but very high afterwars.

Hi folks! If you plan to increase keepAliveTimeout to 65 seconds, should you also increase headersTimeout to 66 at least? Please check #27363

A very good question @ex1st! From what I can see though, that issue was resolved by #32329, which changed how headerTimeout was measured entirely, so it shouldn't overlap with keepAliveTimeout as it used to. From my reading of the code there, nowadays the header timer doesn't start until a new request starts, and it's disabled when it finishes, so it's not measuring idle time at all. Does that make sense?

Is this change still something we're looking to land on Node.js 25? It seemed to get a lot of approvals/support for changing the default at the time.

It would be great to get this merged, @BethGriggs mentioned yesterday that it's still causing issues, and it would be a clear improvement for lots of use cases. It looks like there's a pending issue on the commit message and some debate about whether it's semver-major or not.

I'm happy to go for semver-major if there's uncertainty, just in case. Any objections to that? I'll update it now but shout if anybody feels that's an issue. That'll delay releasing this significantly unfortunately but it's better than leaving it stuck.

@pras529 can you update the commit message & force push, so the checks pass? Since this was done the requirements have also changed, so you'll now also need to include a 'Signed-Off-By' footer - see the docs at https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#commit-message-guidelines for details & examples. En route, a rebase would be helpful since this is now a long way back from main, and the tests will need to be passing too, if that doesn't get resolved by the rebase & rerun (logs are gone, so it's unclear what failed there).

@pimterry feel free to open a fresh new PR and add @pras529 on Co-Authored-By section in case of silent communication.

feel free to open a fresh new PR and add @pras529 on Co-Authored-By section in case of silent communication.

Now done - #62782 has these changes, but rebased, with the linting & tests passing. I'll close this and we can continue over there.

Reviews welcome please from anybody here who's still following this 😄