Skip to content

fix(itc): direct-route component status responses; drop wasteful broadcast - #609

Merged
kriszyp merged 4 commits into
mainfrom
fix/itc-component-status-direct-response-574
May 19, 2026
Merged

fix(itc): direct-route component status responses; drop wasteful broadcast#609
kriszyp merged 4 commits into
mainfrom
fix/itc-component-status-direct-response-574

Conversation

@kriszyp

@kriszyp kriszyp commented May 19, 2026

Copy link
Copy Markdown
Member

Summary

componentStatusRequestHandler fell back to broadcasting the status response to every thread whenever the originator port wasn't in connectedPorts. That fired on every main-thread-originated request and on every race where the originator exited mid-request — wasteful and noisy.

This PR makes direct routing actually work for main-thread originators and drops the broadcast fallback entirely.

Changes

  • server/threads/itc.jssendItcEvent now always stamps event.message.originator = threadId. Removed the !isMainThread guard. Main thread's threadId is 0 by worker_threads convention.
  • server/threads/manageThreads.js:
    • Workers set parentPort.threadId = 0 so connectedPorts.sendToThread(0, ...) routes back to main.
    • sendToThread wraps postMessage in try/catch — a port closing between find() and postMessage() is now treated as unreachable instead of throwing.
  • server/itc/serverHandlers.js — when sendToThread returns false, log at trace and drop. The CrossThreadStatusCollector's own 5s timeout already handles missing replies, so a broadcast just costs every other thread a Map lookup for a response it didn't ask for.
  • components/status/crossThread.ts — the 30s safety-net cleanup also clears responseCheckers (it previously only cleared awaitingResponses).
  • unitTests/server/itc/serverHandlers.test.js — new tests for both the direct-send and silent-drop paths.

Purpose

Closes #574 (CORE-3049). Acceptance criteria from the issue:

  • Race condition: drop silently (trace) when originator is gone; no broadcast.
  • Stamp originator for main-thread events.
  • Broadcast no longer needed — removed entirely; non-originator threads no longer see status responses.
  • awaitingResponses cleanup safety net audited; extended to also reap stale responseCheckers.

Where to look

  • parentPort.threadId = 0 (manageThreads.js) — please double-check this doesn't collide with any Node behavior I haven't considered. The Node convention is that the main thread's threadId is 0; parentPort is a MessagePort, not a Worker, so we're free to attach a threadId property. All existing call sites that compare port.threadId === someId will find it.
  • Removing the broadcast fallback — non-originator threads previously ignored the broadcast cheaply (Map miss in awaitingResponses), so the consequences of broadcasting were small; but the volume could be high in fabric setups. After this change, a stuck/crashed originator just times out via the collector's 5s timeout instead of getting a useless reply blast.
  • sendToThread try/catch around postMessage — silently swallows post failures and returns false. This is correct for the race the issue describes, but worth a sanity check that we don't want to surface a different error class here.

Test plan

  • Existing unit tests (unitTests/server/itc, unitTests/components/status) — all 156 tests pass.
  • New unit tests covering direct-send and silent-drop paths in serverHandlers.test.js.
  • Manual: bring up a multi-worker fabric instance and verify the previous [debug]: Failed to send direct response to thread N, falling back to broadcast log is gone.

Notes

  • Cross-model review: ran the diff through Gemini 0.42. Verdict was clean — no regressions or risks identified. The only suggestion (include requestId in the drop trace) is applied.
  • Generated by Claude Opus 4.7 (1M context). I followed the harper-engineering-guidelines lifecycle.

🤖 Generated with Claude Code

…dcast

componentStatusRequestHandler fell back to broadcasting status responses to
every thread whenever the originator port was missing from connectedPorts —
which happened on every main-thread-originated request (since sendItcEvent
only stamped originator on workers) and on every race where the originator
exited mid-request.

- sendItcEvent now always stamps originator = threadId. Main thread's id is
  0 by worker_threads convention.
- parentPort.threadId = 0 is set in workers, so sendToThread(0, ...) routes
  back to main.
- componentStatusRequestHandler drops silently at trace when sendToThread
  returns false; the collector's own 5s timeout handles missing replies.
- connectedPorts.sendToThread wraps postMessage in try/catch so a port that
  closes between find() and postMessage() is treated as unreachable instead
  of throwing.
- CrossThreadStatusCollector's 30s safety-net cleanup also reaps stale
  responseCheckers entries.

Closes #574

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

@kriszyp
kriszyp marked this pull request as ready for review May 19, 2026 14:57
@kriszyp
kriszyp requested a review from a team as a code owner May 19, 2026 14:57
@kriszyp

kriszyp commented May 19, 2026

Copy link
Copy Markdown
Member Author

CI status: 33/36 checks green. The 2 failing checks are pre-existing Windows-only flakes that also fail on main itself (4 of the last 4 main integration runs hit Integration API Tests (Windows, Node.js v24)):

  • Integration API Tests (Windows, Node.js v24)ECONNREFUSED 127.0.0.1:9925 in 28_transactionLogs.mjs after the http_workers restart in 27_headerTests.mjs. The fixed 15s wait in restartServiceHttpWorkersWithTimeout is not always enough on Windows; headers.test.mjs already uses the readiness-poll variant from lifecycle.mjs (added in Add pre-merge cherry-pick flow for patch-labeled PRs #579) but the older tests/*.mjs suite still uses the timed-wait shim.
  • Integration Tests 4/4 (Windows, Node.js v24)npm install --ignore-scripts in the "Relink bin scripts" step exits 1 with no useful output (Windows runner infra flake).

All Linux Unit Tests, Linux Integration Tests, Build, Lint, Format, and Socket Security checks pass.

🤖 — Claude

port.postMessage(message);
return true;
} catch {
// Port may have closed between find() and postMessage() — treat as unreachable.

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.

💡 Could we catch more specific errors here for the exceptions we expect?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Narrowed the catch in 871427dsendToThread now only swallows err.code === 'ERR_CLOSED_MESSAGE_PORT' (the documented closed-port race) and rethrows everything else. That keeps DataCloneError serialization bugs and other unexpected failures visible instead of silently turning into false. See server/threads/manageThreads.js:55-69.

🤖 — Claude

expect(log_trace_stub).to.have.been.called;
});

// Tests direct-response path: when sendToThread succeeds, log a trace and do not broadcast

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.

The comments before the unit tests don't seem particularly useful

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good call — removed the leading comments above the two new componentStatusRequestHandler test cases in 871427d (unitTests/server/itc/serverHandlers.test.js). The it(...) names already describe what each one covers.

🤖 — Claude

kriszyp and others added 2 commits May 19, 2026 17:00
…ilerplate

Address PR #609 review feedback:
- sendToThread now only swallows ERR_CLOSED_MESSAGE_PORT (the documented
  port-closed-between-find-and-postMessage race) and rethrows everything
  else, so DataCloneError serialization bugs and other unexpected errors
  remain visible.
- Remove redundant leading comments above the two new componentStatusRequestHandler
  tests; the test names already describe what they cover.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tus-direct-response-574

# Conflicts:
#	server/threads/itc.js
@kriszyp
kriszyp merged commit e64eabe into main May 19, 2026
37 checks passed
@kriszyp
kriszyp deleted the fix/itc-component-status-direct-response-574 branch May 19, 2026 23:55
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.

ITC component status: direct response fails and broadcasts unnecessarily

2 participants