Fix HTTP startup after pre-ready worker restart - #2129
Open
kriszyp wants to merge 5 commits into
Open
Conversation
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request improves HTTP worker startup resilience and diagnostics by tracking startup phases, implementing a startup timeout diagnostic, and handling exhausted restarts. It also adds integration tests to verify recovery when an initial HTTP worker crashes. The review feedback correctly points out that a worker's threadId is reset to -1 after exit in Node.js, and suggests capturing and tracking the active threadId synchronously during startup to ensure accurate logging when a worker exits or restarts are exhausted.
Contributor
|
Reviewed; no blockers found. |
The suite's crash injection has to end an HTTP worker before it reports ready, which means process.exit() from a component module that is still being evaluated. Under Bun that wedges the whole process: the main thread stops turning its event loop (no timers, no 'exit' event for the dead worker, so no restart) and Bun aborts with SIGABRT ~30s later, failing Integration Tests 3/6 (Bun). Deferring the exit by a macrotask clears the wedge under Bun but lands after readiness on Node, and under uWS aborts in Node's own worker teardown, so no injection point is early enough on every runtime. Skip under Bun instead; Node, Windows and uWS still cover the fix. Also assert the injected exit really was pre-ready, via the startup diagnostic in hdb.log — a post-ready restart satisfies the original assertions while covering nothing. Co-Authored-By: Claude Opus <noreply@anthropic.com>
A Worker's threadId reads back as -1 once the thread has exited, which is precisely when these diagnostics run: the pre-ready exit log said "thread -1, attempt 1, phase loading components". Record each attempt's thread id while the worker is still alive and report that instead. Addresses review feedback on #2129. Co-Authored-By: Claude Opus <noreply@anthropic.com>
A rolling restart marks both the outgoing worker and its still-booting replacement wasShutdown, so a shutdown that lands while a replacement is still loading components exited an unready worker through the "exited before ready" error path — routine operation reported at error severity. Skip the log when the exit was intentional. Also drops the narrating comment on onStarted. Addresses review feedback on #2129. Co-Authored-By: Claude Opus <noreply@anthropic.com>
The poll loop let a fetch rejection escape, so a connection refused while the replacement worker was still binding would fail the test outright instead of retrying until the deadline — the deadline is there precisely to absorb that. Retry and surface the last error in the failure message. Addresses review feedback on #2129. Co-Authored-By: Claude Opus <noreply@anthropic.com>
kriszyp
marked this pull request as ready for review
August 12, 2026 22:15
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
independent-review: claude + gemini + harper-domain; full review completed on dc5bf2c, one diagnostic-state race fixed afterward and revalidated with build, lint, and uWS integration coverage. Remaining review notes are called out below.
Summary
Root cause and scope
The scheduled-run artifact proves stdout/stderr capture and process launch were working. The initial HTTP worker exited before its ready message;
manageThreadslaunched a replacement, but startup was still awaiting the promise owned by the dead Worker object. The replacement created a different promise, so the originalPromise.all()could never settle.The exact trigger for that first worker exit was not retained in the old logs. This PR fixes the confirmed wait-ownership hang and makes a repeat observable by phase; it does not claim to identify the original worker failure.
The 60-second startup diagnostic is intentionally non-destructive. A hard boot cutoff or forced
Worker.terminate()would create a new availability risk for slow deployments and Bun. A worker that wedges without exiting therefore remains an operational follow-up rather than changing production boot policy here.Testing
npm run buildoxlint --deny-warningsHARPER_UWS_HTTP=1 npm run test:integration -- integrationTests/server/initial-http-worker-restart.test.tsnpx mocha unitTests/server/threads/threadServerListenOnPorts.test.jsHARPER_UWS_HTTP=1 npm run test:integration -- integrationTests/components/scheduler-jobs.test.tsThe complete integration run was attempted earlier in this task; this regression passed, while unrelated Northwind concurrent CSV and local-Ollama tests failed in the environment.