fix: handle ECONNRESET errors in Node.js 24.x #7772
Conversation
- Replace wait-port dependency with custom implementation - Add retry logic with ECONNRESET error handling - Add HTTP agent with ECONNRESET handling in proxy - Enable framework host detection for Node 24+ - Update tests to use new wait-port API Fixes Node.js 24.x dev server startup failures
|
@VitaliyR did the changes and fixed the tests, they all passed on my end |
| settings.frameworkHost = ipVersion === 6 ? '::1' : '127.0.0.1' | ||
| const nodeVersion = process.versions.node | ||
| settings.detectFrameworkHost = options.skipWaitPort || semver.gte(nodeVersion, '24.0.0') | ||
| settings.detectFrameworkHost = options.skipWaitPort |
There was a problem hiding this comment.
so this change is not needed anymore?
There was a problem hiding this comment.
correct, it was updated with wait-port.ts
|
hopefully this one fixes the failed tests |
ElijahJunaid
changed the title
src/lib/wait-port.ts
Outdated
|
|
||
| socket.on('error', (error) => { | ||
| isResolved = true | ||
| cleanup() |
There was a problem hiding this comment.
cleanup never called neither here nor in other places, since isResolved set to true everywhere, isn't it?
seems that cleanup is being redundant?
There was a problem hiding this comment.
you are absolutely correct, its been removed
There was a problem hiding this comment.
the tests keep failing but it seems unrelated to my changes. my wait port is only to check if a port is open, but the env var isnt being injected, not something i touched
|
removed the proxy.ts handling added by an earlier version, caused unnecessary 500 errors, original problem still solved |
|
@VitaliyR tests that are failing have nothing to do with my pull request, if you want i can investigate further and try to get a spotless test but itd have more files to be involved |
| host: 'localhost', | ||
| output: 'silent', | ||
| timeout: FRAMEWORK_PORT_TIMEOUT_MS, | ||
| ...(settings.pollingStrategies?.includes('HTTP') && { protocol: 'http' }), |
There was a problem hiding this comment.
I was worried about this removal but I think it works out:
- Technically we allow this
pollingStrategiesconfiguration. It's unclear and undocumented what values are even supported... but every single framework is currently configured to usetcp. - Technically a user could directly configure this in their
netlify.toml... but even more technically it isn't a documented field - There is no default here, so we were effectively never passing a
protocolto thewait-portlib - This isn't really documented, but if you don't pass a
protocoltowait-portit will check via TCP only
So... I think it works out. But we should remove pollingStrategies from @netlify/build-info.
There was a problem hiding this comment.
should be done by a maintainer after the merge as its just info
|
@ElijahJunaid let me run the tests on my end, if they would pass I'm inclined to merge this PR |
|
@VitaliyR youve gotten it to 3 failed tests (lint title does not count) |
|
@VitaliyR i readded the prior retry count from the old wait-port to see if that fixes the last 3 tests as they seemed to timeout and i wondered if giving them more time would fix it - if the tests dont pass now, idk what im doing wrong |
|
@VitaliyR you need to do ur magic work again so we can see whats left in actual problems if any. currently i dont see any problems relating to this pull request, but it may be obstructed because of all of the unrelated errors (github problem) |
|
@ElijahJunaid thanks! |
3 participants
#7747 - Replace wait-port dependency with custom implementation
Fixes Node.js 24.x dev server startup failures