Skip to content

Fix HTTP startup after pre-ready worker restart - #2129

Open
kriszyp wants to merge 5 commits into
mainfrom
fix/integration-startup-hang-diagnostics
Open

Fix HTTP startup after pre-ready worker restart#2129
kriszyp wants to merge 5 commits into
mainfrom
fix/integration-startup-hang-diagnostics

Conversation

@kriszyp

@kriszyp kriszyp commented Aug 9, 2026

Copy link
Copy Markdown
Member

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

  • Keep HTTP startup readiness with the logical worker slot, so a managed replacement can satisfy the original boot wait.
  • Report each worker startup phase and turn bootstrap rejection into an explicit worker failure.
  • Add an end-to-end uWS regression: crash the initial HTTP worker 0 once, then require the replacement to finish startup and run scheduler work.

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; manageThreads launched a replacement, but startup was still awaiting the promise owned by the dead Worker object. The replacement created a different promise, so the original Promise.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 build
  • scoped oxlint --deny-warnings
  • HARPER_UWS_HTTP=1 npm run test:integration -- integrationTests/server/initial-http-worker-restart.test.ts
  • standard HTTP mode for the same integration test
  • npx mocha unitTests/server/threads/threadServerListenOnPorts.test.js
  • HARPER_UWS_HTTP=1 npm run test:integration -- integrationTests/components/scheduler-jobs.test.ts

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

Co-Authored-By: GPT-5 Codex <noreply@openai.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread server/threads/socketRouter.ts
Comment thread server/threads/socketRouter.ts Outdated
Comment thread server/threads/socketRouter.ts Outdated
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

kriszyp and others added 4 commits August 9, 2026 19:19
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
kriszyp marked this pull request as ready for review August 12, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant