fix(status): count all live worker threads for get_status cross-thread aggregation

[kriszyp]

What / why

CrossThreadStatusCollector.collect() (components/status/crossThread.ts) sized expectedResponses — how many worker-thread responses to wait for before considering a get_status cross-thread aggregation complete — from getWorkerCount(). That function only reports the calling thread's own same-type pool size, and falls back to a hardcoded 1 when called from the main thread (since main isn't itself part of any worker pool).

In a normal deployment, get_status is served from the main thread while N HTTP worker threads run. So the collector capped expectedResponses at 1 and declared the collection complete after the first worker replied, silently discarding every other worker's status — including any that disagreed.

Verified live (repro fixture booted with threads.count: 8): before this fix, get_status's aggregated componentStatus only ever reflected 1 of 8 worker threads, regardless of how many actually responded or disagreed. This is the concrete mechanism behind the "invisible to get_status" half of #1951.

Fix

Use the already-tracked workers registry from server/threads/manageThreads.js (populated on the thread that spawns children — normally main) to size expectedResponses, excluding job workers: broadcastWithAcknowledgement() (which the underlying ITC broadcast goes through) explicitly skips isJobWorker ports, so counting them would make expectedResponses unreachable and stall every get_status call to its full timeout whenever a job worker is running. Falls back to the old getWorkerCount()-based estimate when workers is empty (a genuinely zero-worker process, or — more commonly — this thread isn't the one that spawns children and has no visibility into siblings).

What this does NOT fix

The underlying cross-component load-order race that causes some worker threads to diverge from others in the first place is unaddressed here — that requires the dependency-declaration mechanism #1931 asks for. This PR only closes the operational-visibility gap: once a divergence exists, get_status now actually aggregates from every live worker thread instead of just one.

Known residual limitation (not a regression)

If collect() is invoked from a non-main thread in a deployment with multiple distinct worker-thread types (e.g. HTTP + job pools), workers is empty there too (only the spawning thread populates it), so it falls back to the pre-existing getWorkerCount()-based same-type-pool estimate — still an undercount in that specific scenario, but no worse than before this fix. Flagged by the cross-model review below; left as a documented follow-up rather than a blocker since fixing it generally requires threading a real total-thread-count through IPC.

Cross-model review

Ran the cross-model-review skill (standard mode: Gemini via agy, plus a direct Codex codex exec pass since the reviewer/codex-reviewer orchestration subagents aren't available in this headless environment — legs run directly and adjudicated inline).

• Gemini leg: flagged a false premise (assumed workers might be a Map/Object — it's a plain array, refuted by reading the source), one already-accepted residual limitation (above), and a valid simplification (workers.length || getWorkerCount() || 1 over the more verbose ternary — applied).
• Codex leg (significant, confirmed by code trace and fixed before this PR): workers.length as originally written counted job workers, but broadcastWithAcknowledgement() (server/threads/manageThreads.js) explicitly skips isJobWorker ports — so any job worker in the process would make expectedResponses permanently unreachable, stalling every get_status call to its 5s timeout. Fixed by filtering workers to exclude isJobWorker entries; added unit test coverage for exactly this scenario (2 HTTP workers + 1 job worker → expectedResponses must be 2, not 3).

Test plan

• npm run build (tsc) — clean
• npx mocha "unitTests/components/status/**/*.js" — 136/136 passing (135 existing + 1 new, covering the job-worker-exclusion path)
• npm run lint:required — clean
• Live repro against the qa-scratch fixture from Component load race fails per-worker (200/500 split-brain at threads>1) and is invisible to get_status #1951 (threads.count: 8, two components with an undeclared load-time dependency): confirmed pre-fix that get_status only reflected 1 of 8 workers even while genuine per-worker request divergence (200/500 mix) was occurring; confirmed post-fix that all 8 workers' status is now correctly aggregated (fast, ~15ms, no timeout stall)
• Not covered: an integration test asserting this exact end-to-end scenario (multi-worker get_status divergence visibility) — the qa-scratch fixture used for verification isn't part of this diff; a proper integration test for the cross-component race itself is more naturally scoped to whatever PR eventually addresses [Core] No supported way for a component to declare it serves REST resources (forces boot-order hacks) #1931/Component load race fails per-worker (200/500 split-brain at threads>1) and is invisible to get_status #1951's root cause

Refs #1951, #1931

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request improves the cross-thread status collection by using the authoritative workers registry (excluding job workers) to calculate expected responses, and adds a corresponding unit test. The review feedback highlights a potential 5-second stall during rolling restarts or in zero-worker configurations, and suggests filtering out shutting-down workers. Additionally, the feedback recommends avoiding timing flakiness in the test by using setImmediate instead of setTimeout, and properly capturing and restoring the shared workers array to prevent cross-test contamination.

[gemini-code-assist]

Issue: Potential 5-second stall during rolling restarts/reloads or in zero-worker configurations

There are two distinct scenarios where the current calculation of expectedResponses can cause get_status calls to stall for the full 5-second timeout:

1. During rolling restarts/reloads: When a worker is being shut down, worker.wasShutdown is set to true in manageThreads.js. However, the worker remains in the workers array until its 'exit' event fires (which can take up to 10–30 seconds). Since these shutting-down workers are still in the workers array, they are included in nonJobWorkerCount but will not respond to the COMPONENT_STATUS_REQUEST ITC event. This causes the collector to wait for responses that will never arrive, stalling the status aggregation for the full 5-second timeout.
2. In zero-worker configurations: On the main thread in a zero-worker setup (or during early startup/late shutdown), workers is empty and getWorkerCount() returns undefined. Due to the || 1 fallback, expectedResponses is set to 1. Since there are no workers to respond, the collector stalls for the full 5-second timeout.

Solution:

• Exclude workers that are shutting down by checking !worker.wasShutdown.
• Change the fallback from || 1 to || 0. Since getWorkerCount() always returns a number >= 1 on worker threads, the || 1 fallback is only reached on the main thread when there are genuinely 0 workers, where we should expect 0 responses and resolve immediately.

Suggested change

	const nonJobWorkerCount = workers.filter((worker) => !worker.isJobWorker).length;
	const expectedResponses = nonJobWorkerCount || getWorkerCount() || 1;
	const nonJobWorkerCount = workers.filter((worker) => !worker.isJobWorker && !worker.wasShutdown).length;
	const expectedResponses = nonJobWorkerCount || getWorkerCount() || 0;

[gemini-code-assist]

Improvement: Avoid fixed sleeps and timing flakiness in tests

Using setTimeout with a fixed 50ms delay to simulate asynchronous responses can introduce flakiness in slow or resource-constrained CI environments.

Replacing setTimeout with setImmediate defers the execution to the next turn of the event loop (guaranteeing that the collector has set up its response map) while executing virtually instantaneously. This makes the test faster and completely immune to timing-related flakiness.

onMessageByTypeStub.callsFake((eventType, handler) => {
	setImmediate(() => {
		handler({
			message: {
				requestId: 1,
				workerIndex: 1,
				isMainThread: false,
				statuses: [['poolComp', { status: 'healthy' }]],
			},
		});
		handler({
			message: {
				requestId: 1,
				workerIndex: 2,
				isMainThread: false,
				statuses: [['poolComp', { status: 'healthy' }]],
			},
		});
	});
});

References

1. Prefer using condition-waits (e.g., a helper like waitFor that polls for a condition) instead of fixed sleeps or real-time delays (like setTimeout) in tests to avoid flakiness caused by coarse clock resolutions or slow environments.

[gemini-code-assist]

Improvement: Capture original state of shared array to prevent cross-test contamination

To prevent side effects on other tests, capture the original state of the shared manageThreadsModule.workers array so it can be fully restored in the finally block.

Suggested change

	manageThreadsModule.workers.push(httpWorkerA, httpWorkerB, jobWorker);
	const originalWorkers = [...manageThreadsModule.workers];
	manageThreadsModule.workers.push(httpWorkerA, httpWorkerB, jobWorker);

[gemini-code-assist]

Improvement: Fully restore original state of shared array

Restore the original elements of the shared manageThreadsModule.workers array captured at the start of the test, rather than just clearing it.

manageThreadsModule.workers.length = 0;
manageThreadsModule.workers.push(...originalWorkers);
getWorkerCountStub.restore();

[claude]

Reviewed; no blockers found.

[claude]

The fix itself checks out — getEligibleBroadcastRecipientCount() filters connectedPorts by !isJobWorker, mirroring exactly the filter broadcastWithAcknowledgement() applies when delivering, so count and delivery stay symmetric by construction, from any calling thread. Two things before merge:

• The Node v24 unit-test job is red on this head (v22/v26 green) — needs triage or a re-run.
• The PR description still describes the earlier workers-array approach, including a "residual limitation" for non-main callers that the shipped connectedPorts code doesn't have. Worth updating so the history isn't misleading — no code changes needed.

Non-blocking:

• If a dead port hasn't been spliced from connectedPorts yet, or a just-spawned worker can't answer, expectedResponses overcounts and collect() waits the full 5s timeout instead of resolving early — latency at thread churn, not correctness.
• The new tests push into the real exported connectedPorts array and truncate it in afterEach — fragile if another suite ever touches it.

Review by @heskew (posted via Claude).

---

Generated by Claude Code