engine: log rejected node-control messages server-side#259
Conversation
…n-socket The node-control handler's catch sent the rejection as a WS `error` frame but logged nothing, so a rejected `node.register`/`node.heartbeat` (e.g. `node_name_conflict` when two nodes share a name, or a UNIQUE violation) left a node running half-registered with no server-side signal — it took manual instrumentation to see. Log the rejection at warn with workspace, node, provider, message type, and code before sending the error frame. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe engine now logs rejected node-control messages and message-trigger dispatch failures at warn level with structured context. Conformance tests verify both logging paths, while existing socket error responses and non-throwing trigger behavior remain unchanged. ChangesEngine observability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds server-side warning logs for rejected node control messages (such as node.register or node.heartbeat) to ensure these rejections are visible server-side rather than only on the client socket. It also includes a new conformance test and updates the changelog. The reviewer suggested distinguishing between client-side errors and unexpected server-side errors, recommending that server-side faults be logged as errors with stack traces instead of warnings to avoid obscuring critical system health issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const error = err as Error & { code?: string }; | ||
| const code = error.code ?? 'node_control_failed'; | ||
| // The rejection reaches the client only as the error frame below. Log it so | ||
| // it is not invisible server-side: a rejected node.register/node.heartbeat | ||
| // (e.g. node_name_conflict, a UNIQUE violation) otherwise leaves a node | ||
| // running half-registered with no signal to operators. Warn, not error — | ||
| // these are client/protocol rejections, not server faults. | ||
| console.warn('[node.control] rejected message', { | ||
| type: message.type, | ||
| code, | ||
| workspaceId: args.workspaceId, | ||
| nodeId: args.nodeId, | ||
| provider: frameProviderName, | ||
| message: error.message, | ||
| }); |
There was a problem hiding this comment.
The catch block catches all errors thrown during message processing, including unexpected database connection failures, internal runtime errors (e.g., TypeError), or other server-side faults (5xx errors).
Logging actual server faults as client-side warnings (console.warn) with a generic [node.control] rejected message prefix can hide critical system health issues from log aggregators and monitoring tools that alert on console.error.
We should distinguish between client-side errors (status code < 500) and unexpected server-side errors (status code >= 500 or undefined), logging the latter as console.error with the stack trace for better observability.
const error = err as Error & { code?: string; statusCode?: number };
const code = error.code ?? 'node_control_failed';
const isClientError = error.statusCode !== undefined && error.statusCode < 500;
if (isClientError) {
// The rejection reaches the client only as the error frame below. Log it so
// it is not invisible server-side: a rejected node.register/node.heartbeat
// (e.g. node_name_conflict, a UNIQUE violation) otherwise leaves a node
// running half-registered with no signal to operators. Warn, not error —
// these are client/protocol rejections, not server faults.
console.warn('[node.control] rejected message', {
type: message.type,
code,
workspaceId: args.workspaceId,
nodeId: args.nodeId,
provider: frameProviderName,
message: error.message,
});
} else {
console.error('[node.control] internal error processing message', {
type: message.type,
code,
workspaceId: args.workspaceId,
nodeId: args.nodeId,
provider: frameProviderName,
message: error.message,
stack: error.stack,
});
}![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/engine/src/__tests__/conformance/nodeProviders.test.ts">
<violation number="1" location="packages/engine/src/__tests__/conformance/nodeProviders.test.ts:109">
P3: The new log payload includes `provider` (the provider name from the frame or registry binding), but the test assertion for the log call doesn't verify it. Consider adding `provider: expect.any(String)` (or the expected value `'alpha'`) to `expect.objectContaining(...)` so a regression that drops the provider field is caught during test runs.</violation>
</file>
<file name="packages/engine/src/engine/node.ts">
<violation number="1" location="packages/engine/src/engine/node.ts:1541">
P2: This catch block handles all errors uniformly — both expected client/protocol rejections (e.g. `node_name_conflict`) and unexpected server-side faults (e.g. database connection failures, `TypeError`). Logging everything at `console.warn` without a stack trace can hide critical system errors from monitoring tools that alert on `console.error`.
Consider distinguishing between expected rejections (errors with a known `code` or a `statusCode < 500`) and unexpected failures, logging the latter at `console.error` with `error.stack` for better observability.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // (e.g. node_name_conflict, a UNIQUE violation) otherwise leaves a node | ||
| // running half-registered with no signal to operators. Warn, not error — | ||
| // these are client/protocol rejections, not server faults. | ||
| console.warn('[node.control] rejected message', { |
There was a problem hiding this comment.
P2: This catch block handles all errors uniformly — both expected client/protocol rejections (e.g. node_name_conflict) and unexpected server-side faults (e.g. database connection failures, TypeError). Logging everything at console.warn without a stack trace can hide critical system errors from monitoring tools that alert on console.error.
Consider distinguishing between expected rejections (errors with a known code or a statusCode < 500) and unexpected failures, logging the latter at console.error with error.stack for better observability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/engine/node.ts, line 1541:
<comment>This catch block handles all errors uniformly — both expected client/protocol rejections (e.g. `node_name_conflict`) and unexpected server-side faults (e.g. database connection failures, `TypeError`). Logging everything at `console.warn` without a stack trace can hide critical system errors from monitoring tools that alert on `console.error`.
Consider distinguishing between expected rejections (errors with a known `code` or a `statusCode < 500`) and unexpected failures, logging the latter at `console.error` with `error.stack` for better observability.</comment>
<file context>
@@ -1532,12 +1532,26 @@ export async function handleNodeControlMessage(args: HandleNodeControlMessageArg
+ // (e.g. node_name_conflict, a UNIQUE violation) otherwise leaves a node
+ // running half-registered with no signal to operators. Warn, not error —
+ // these are client/protocol rejections, not server faults.
+ console.warn('[node.control] rejected message', {
+ type: message.type,
+ code,
</file context>
| await dup.handle.handleMessage(registerFrame('node_a', 'alpha', { name: 'py', instance_id: 'i2' }, [{ name: 'run-etl', kind: 'action' }])); | ||
|
|
||
| expect(dup.sock.ofType('error').at(-1)).toMatchObject({ code: 'provider_instance_conflict' }); | ||
| expect(warn).toHaveBeenCalledWith('[node.control] rejected message', expect.objectContaining({ |
There was a problem hiding this comment.
P3: The new log payload includes provider (the provider name from the frame or registry binding), but the test assertion for the log call doesn't verify it. Consider adding provider: expect.any(String) (or the expected value 'alpha') to expect.objectContaining(...) so a regression that drops the provider field is caught during test runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/__tests__/conformance/nodeProviders.test.ts, line 109:
<comment>The new log payload includes `provider` (the provider name from the frame or registry binding), but the test assertion for the log call doesn't verify it. Consider adding `provider: expect.any(String)` (or the expected value `'alpha'`) to `expect.objectContaining(...)` so a regression that drops the provider field is caught during test runs.</comment>
<file context>
@@ -91,6 +91,32 @@ describe('node providers', () => {
+ await dup.handle.handleMessage(registerFrame('node_a', 'alpha', { name: 'py', instance_id: 'i2' }, [{ name: 'run-etl', kind: 'action' }]));
+
+ expect(dup.sock.ofType('error').at(-1)).toMatchObject({ code: 'provider_instance_conflict' });
+ expect(warn).toHaveBeenCalledWith('[node.control] rejected message', expect.objectContaining({
+ type: 'node.register',
+ code: 'provider_instance_conflict',
</file context>
fireMessageTriggers dispatched best-effort and swallowed every failure, so a trigger whose action is missing, has no handler, or targets an offline node died silently — the message posts, nothing fires, and there is no server-side signal. Log the failure at warn with trigger id, action name, workspace, and error code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/engine/src/engine/trigger.ts">
<violation number="1" location="packages/engine/src/engine/trigger.ts:206">
P3: The `catch (err)` block uses a raw type assertion (`err as Error & { code?: string }`) instead of the project's `asCodedError()` utility from `httpError.ts`. If a thrown value is not an `Error` instance (e.g. a string or plain object), `error.message` would be `undefined` in the log output. The codebase already has `asCodedError()` to handle this robustly — it wraps non-Error thrown values while preserving their own properties, matching the same pattern the engine uses for route-handler error replies.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // — but swallowing the failure silently hides a broken trigger (its | ||
| // action is missing, has no handler, or the handling node is offline). | ||
| // Log it with trigger/action context so a dead trigger is diagnosable. | ||
| const error = err as Error & { code?: string }; |
There was a problem hiding this comment.
P3: The catch (err) block uses a raw type assertion (err as Error & { code?: string }) instead of the project's asCodedError() utility from httpError.ts. If a thrown value is not an Error instance (e.g. a string or plain object), error.message would be undefined in the log output. The codebase already has asCodedError() to handle this robustly — it wraps non-Error thrown values while preserving their own properties, matching the same pattern the engine uses for route-handler error replies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/engine/src/engine/trigger.ts, line 206:
<comment>The `catch (err)` block uses a raw type assertion (`err as Error & { code?: string }`) instead of the project's `asCodedError()` utility from `httpError.ts`. If a thrown value is not an `Error` instance (e.g. a string or plain object), `error.message` would be `undefined` in the log output. The codebase already has `asCodedError()` to handle this robustly — it wraps non-Error thrown values while preserving their own properties, matching the same pattern the engine uses for route-handler error replies.</comment>
<file context>
@@ -198,8 +198,19 @@ export async function fireMessageTriggers(args: {
+ // — but swallowing the failure silently hides a broken trigger (its
+ // action is missing, has no handler, or the handling node is offline).
+ // Log it with trigger/action context so a dead trigger is diagnosable.
+ const error = err as Error & { code?: string };
+ console.warn('[trigger] action dispatch failed', {
+ triggerId: row.id,
</file context>
1 participant
What this is
The node-control handler logs a rejected
node.register/node.heartbeat(and any other node-control message) at warn level with workspace, node, provider, message type, and error code, before sending the client theerrorframe.Why
The handler's catch previously sent the rejection only as a WebSocket
errorframe and logged nothing. A node whose register/heartbeat is rejected — e.g.node_name_conflictwhen two nodes register under the same name, or the underlyingSQLITE_CONSTRAINT_UNIQUEonnodes.(workspace_id, name)— then runs half-registered (dead heartbeats, absent from the roster) with no server-side signal. Diagnosing the self-host two-node boot failure required adding instrumentation just to see the rejection.Test
nodeProviders.test.tsasserts a rejected registration is logged with its code and node/workspace context, in addition to the on-socket error reply.Refs #250.
🤖 Generated with Claude Code