src: fix libuv assertion on windows#61999
Conversation
Set the pointer to `nullptr` after calling `uv_close` to avoid assertions. Fixes: nodejs#56645
53bbcc6 to
b721fef
Compare
|
That handle is gonna leak, and I don't think it's gonna fix anything. This looks pretty much a AI-generated solution... that does actually nothing regarding your intention. π |
|
This is not an AI-generated solution, it's an inspiration I got from other code. Lines 411 to 436 in a8eb690 |
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61999 +/- ##
==========================================
- Coverage 89.77% 89.73% -0.05%
==========================================
Files 674 676 +2
Lines 205705 206068 +363
Branches 39449 39514 +65
==========================================
+ Hits 184670 184907 +237
- Misses 13280 13312 +32
- Partials 7755 7849 +94
π New features to boost your workflow:
|
My bad... but sadly still... I don't think this is gonna fix the fundamental problem at all and still leaking. |
| double delay_in_seconds) { | ||
| auto locked = tasks_.Lock(); | ||
|
|
||
| if (flush_tasks_ == nullptr) return; |
There was a problem hiding this comment.
If "just not sending" is indeed a valid solution to this issue (which I have not verified), you could set a boolean flag next to flush_tasks_ instead of making it a heap-allocated variable
There was a problem hiding this comment.
Thanks!
v8 is calling PostDelayedTaskOnWorkerThread here, which looks like a task for wasm caching.
https://github.com/v8/v8/blob/1fb9a7b9671a724ebdcf57db3807d4af56dc6cbb/src/wasm/module-compiler.cc#L4004-L4011
Ignoring it should be safe, because anyway, the delay here is 2 seconds, and the node will have finished long before then.
|
@juanarbol I understand now. The AI ββis telling me that the handle wasn't deleted. AI ββis more familiar with C++ than I am. π€¦ββοΈ |
| timers.push_back(timer); | ||
| for (uv_timer_t* timer : timers) | ||
| scheduler_->TakeTimerTask(timer); | ||
| scheduler_->has_shut_down_ = true; |
There was a problem hiding this comment.
This access doesn't look thread safe. I think adding auto locked = scheduler_->tasks_.Lock(); should do it.
Another, and maybe simpler option, could be moving the check to the Stop() method so we are sure after a StopTask is pushed, no other task will.
void Stop() {
auto locked = tasks_.Lock();
if (has_shut_down_) return;
has_shut_down_ = true;
locked.Push(std::make_unique<StopTask>(this));
uv_async_send(&flush_tasks_);
}
* fix(bin): work around libuv async assertion on Windows exit trueconf-setup crashes at the end of a successful run on Windows + Node 24.x with `assertion failed !(handle->flags & UV_HANDLE_CLOSING)` in src/win/async.c. Root cause is the open Node issue nodejs/node#56645 β Node calls `uv_async_send()` on a closing handle when `process.exit()` short-circuits libuv teardown right after `fetch()`. PR nodejs/node#61999 has been open since Feb 2026 with no backport. Two cooperating workarounds, both documented in the upstream thread: - src/probe.mjs: validateOAuthCredentials now always owns its undici Agent and closes it in finally, so no keep-alive sockets dangle into loop teardown. Cleanup is best-effort β errors are swallowed so the OAuth result classification stays unaffected. - bin/trueconf-setup.mjs: CLI entry sets `process.exitCode` instead of calling `process.exit()`, letting the loop drain naturally after the dispatcher and clack readline release their handles. Shell exit contract is unchanged. Tests: - probe.test.mjs: assert dispatcher.close() runs on the success path and on the fetch-throws path. - bin-cli-subprocess-exits.test.ts: spawn the CLI as a real subprocess and assert it exits within an 8s budget on success (0) and OAuth failure (1) β catches loop-hang regressions on any platform. 970 tests | 2 skipped on macOS, tsc clean, oxlint clean. Windows UAT pending. * fix(bin): watchdog + tighter exit-budget test on review feedback Two review findings on the prior libuv-assertion commit: - bin/trueconf-setup.mjs: a future regression that leaks an event-loop handle (clack stdin in raw mode, sharp libvips worker, an unref-missed timer) would now silently freeze the wizard's terminal forever instead of crashing. Add a 10-second .unref()'d watchdog that prints a clear diagnostic to stderr and forces exit, so a leak is loud, not silent. The timer self-defeats on the happy path because .unref() lets the loop drain ahead of it. - tests/integration/bin-cli-subprocess-exits.test.ts: the prior version had three weaknesses. (1) The CLI entry block does not parse --config from argv, so passing `--config /tmp/x.json` was silently ignored and the success test wrote a fake-server config into the developer's real ~/.openclaw/openclaw.json. Sandbox via `HOME` and `USERPROFILE` env vars instead β that's the only knob the wizard's default `homedir()` lookup reads. (2) The 8s budget was too lax: a partial leak (5s timer ref) would slip under the kill timer unnoticed. Assert elapsedMs < 3000 explicitly so partial regressions fail loud. (3) Hard-coded port `1` for the failure path can SYN-drop on CI firewalls and time out, masking a real loop-hang as a flake. Reserve a closed loopback port via listen(0) β close instead β that reliably ECONNREFUSEs. 970 tests | 2 skipped on macOS, tsc clean, oxlint clean. Windows UAT still pending.
Set the pointer to
nullptrafter callinguv_closeto avoid assertions.Fixes: #56645