Skip to content

fix(producer): normalize error messages to prevent [object Object] in telemetry#1099

Merged
miguel-heygen merged 3 commits into
mainfrom
fix/error-message-serialization
May 28, 2026
Merged

fix(producer): normalize error messages to prevent [object Object] in telemetry#1099
miguel-heygen merged 3 commits into
mainfrom
fix/error-message-serialization

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

Summary

  • When a render fails and the caught value is a plain object (not an Error instance), String(error) produces [object Object], masking the real error in PostHog telemetry (~24 errors/day).
  • Adds normalizeErrorMessage() utility that extracts .message from plain objects, falls back to JSON.stringify, and only uses String() as a last resort for primitives.
  • Applied on the three paths where the pattern matters: the main render failure handler (renderOrchestrator.ts:2099), buildRenderErrorDetails in cleanup.ts, and isRecoverableParallelCaptureError (so timeout classification works even with plain-object errors).

Test plan

  • Unit tests for normalizeErrorMessage covering Error instances, strings, plain objects with .message, objects without .message, null, undefined, numbers, non-string .message, and circular references
  • bun test --filter errorMessage passes (9 tests)
  • Full build passes
  • oxlint + oxfmt clean
  • All pre-commit hooks pass (lint, format, fallow, typecheck, commitlint)

… telemetry

When a render fails and the caught value is a plain object (not an Error
instance), String(error) produces [object Object], masking the real error
in PostHog telemetry (~24 errors/day).

Add normalizeErrorMessage() that tries Error.message, string passthrough,
.message on plain objects, JSON.stringify, and String() as a last resort.
Apply it on the two telemetry-feeding paths: the main render failure
handler (renderOrchestrator.ts:2099) and buildRenderErrorDetails
(cleanup.ts), plus the error classifier isRecoverableParallelCaptureError
so timeout detection works even when the thrown value is a plain object.
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Code Review

CI is all green. Implementation is clean. A few findings before this ships:


Blocker: CLI telemetry path is still broken

The stated goal is fixing [object Object] in PostHog telemetry (~24 errors/day). But the PostHog send for render_error events happens in packages/cli/src/commands/render.ts:1000-1018handleRenderError — which still uses the un-normalized pattern:

// render.ts:1007 — NOT touched by this PR
const message = error instanceof Error ? error.message : String(error);
trackRenderError({ ..., errorMessage: message, ... });

executeRenderJob re-throws error at the end of its catch block (throw error). The CLI catches the original value and runs String(error) on it before PostHog. The normalized job.error and job.errorDetails.message this PR sets are used for the SSE/HTTP response path — not for the CLI's PostHog event.

The studio server has no independent PostHog path; trackRenderError tags source: props.source ?? "cli", so the ~24/day events are CLI-sourced. This PR normalizes the wrong side of the rethrow. handleRenderError needs the same fix.


Important: shouldFallbackToScreenshotAfterCalibrationError not converted

isRecoverableParallelCaptureError was converted — correct, it classifies errors via regex and plain-object errors would produce [object Object] → regex miss → wrong result. Its sibling shouldFallbackToScreenshotAfterCalibrationError in packages/producer/src/services/render/captureCost.ts:419-420 has identical code shape and identical failure mode but wasn't touched:

// captureCost.ts:419-420 — NOT touched by this PR
export function shouldFallbackToScreenshotAfterCalibrationError(error: unknown): boolean {
  const message = error instanceof Error ? error.message : String(error);
  return /HeadlessExperimental\.beginFrame timed out|.../i.test(message);
}

If Chrome throws a plain object on BeginFrame timeout (plausible for protocol-layer errors), this regex never matches → no screenshot fallback → harder failure. Same fix: normalizeErrorMessage(error). Should be in this PR alongside isRecoverableParallelCaptureError.


Nit: circular-reference test asserts the bug output

// errorMessage.test.ts:40-41
// Falls through JSON.stringify failure to String()
expect(normalizeErrorMessage(obj)).toBe("[object Object]");

The function falls back to String() for unstringifiable inputs, which produces the exact string the PR is trying to eliminate from telemetry. Defensible (don't throw on bad inputs), but consider a sentinel like "<circular reference>" or Object.keys(obj).join(",") — something distinguishable. As-is, a circular-reference error would still produce [object Object] in PostHog. Not a blocker, but the test comment "Falls through JSON.stringify failure to String()" reads like an apology for the behavior rather than an assertion of intent.


Not a concern: remaining String(err) sites in renderOrchestrator.ts

Lines 118, 561, 725, 784, 943, 1036, 1330, 2055 all go to log.warn/log.debug, not telemetry. They're inconsistency, not a P0 issue. Fine to address in a follow-up codemod.

Not a concern: stack in errorDetails server response

Pre-existing, not introduced here.

Not a concern: require() in engine ESM packages

Checked — no require() calls in packages/engine/src/. Clean.


The normalizeErrorMessage utility itself is correct and well-tested for the cases it covers (Error instances, strings, plain objects, null/undefined, primitives, non-string .message). The PR's approach is right. The two issues above are about coverage, not design.

— Vai

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Both prior findings verified fixed:

  1. CLI telemetry path — in now imports and uses from instead of the inline pattern. The PostHog path is covered.

  2. **** — converted to in .

The utility itself is solid: correct priority ordering (Error → string → .message → JSON.stringify → String fallback → "unknown error"), handles circular references, and has good test coverage across the relevant edge cases.

LGTM. Ship it.

— Vai

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Both prior findings verified fixed:

  1. CLI telemetry pathhandleRenderError in packages/cli/src/commands/render.ts now imports and uses normalizeErrorMessage from @hyperframes/producer instead of the inline instanceof Error pattern. The PostHog render_error path is covered.

  2. shouldFallbackToScreenshotAfterCalibrationError — converted to normalizeErrorMessage in captureCost.ts.

The normalizeErrorMessage utility itself is solid: correct priority ordering (Error → string → .message → JSON.stringify → String fallback → "unknown error"), handles circular references, and has good test coverage across the relevant edge cases.

LGTM. Ship it.

— Vai

…solution

The Vite test runner can't resolve runtime imports from @hyperframes/producer
since its exports point to dist/. Copy the utility into the CLI package and
import locally instead.
@miguel-heygen miguel-heygen merged commit d83a873 into main May 28, 2026
26 checks passed
@miguel-heygen miguel-heygen deleted the fix/error-message-serialization branch May 28, 2026 00:19
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.

2 participants