Uh oh!
There was an error while loading. Please reload this page.
fix(observability): attach the real cause at three error-swallowing sites - #6336
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Execution usage ledger failures log Trace secret projection attaches a Reviewed by Cursor Bugbot for commit 390dfd7. Configure here. |
Greptile SummaryThis PR improves diagnostics at several error-swallowing boundaries while preserving fail-closed trace redaction.
Confidence Score: 5/5The PR appears safe to merge with no concrete blocking or non-blocking defects identified. The changed paths preserve existing fallback, retry, and caller-facing error behavior while making underlying causes serializable and redacting demonstrated database parameter values.
|
| Filename | Overview |
|---|---|
| packages/utils/src/errors.ts | Extends describeError to redact Drizzle parameter tails in deepest and cause-chain messages while retaining bounded, cycle-safe cause traversal. |
| apps/sim/lib/logs/execution/trace-secret-projection.ts | Adds structured fallback diagnostics while restricting external failures to their error names and retaining fail-closed behavior. |
| apps/sim/lib/logs/execution/logger.ts | Replaces an unserializable raw ledger error with a redacted structured cause description. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts | Preserves the original upload failure as an Error cause and emits structured diagnostics without changing conflict retry behavior. |
| apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts | Replaces the raw swallowed save error with a serializable, redacted cause description. |
| packages/utils/src/errors.test.ts | Covers parameter redaction for wrapped and unwrapped database errors. |
| apps/sim/lib/logs/execution/trace-secret-projection.test.ts | Verifies invariant diagnostics, external-message withholding, and structural fallback behavior. |
| apps/sim/lib/uploads/contexts/workspace/workspace-file-storage-accounting.test.ts | Verifies that upload wrapping preserves the deepest database SQLSTATE and message. |
| apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts | Verifies structured reporting of the database driver cause behind swallowed workspace-save failures. |
| apps/sim/lib/logs/execution/logger.test.ts | Verifies structured SQLSTATE reporting when usage-ledger writes fail. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
E[Underlying error] --> W[Error wrapper with cause]
W --> D[describeError]
D --> R[Redact bound parameters]
R --> L[Structured diagnostic log]
P[Trace projection failure] --> T{Internal projection error?}
T -->|Yes| I[Log static invariant reason]
T -->|No| N[Log error name only]
I --> F[Fail-closed structural fallback]
N --> F
Reviews (1): Last reviewed commit: "fix(observability): attach the real caus..." | Re-trigger Greptile
…ites
Three log sites discarded the underlying error, which blocked root-cause
analysis in production.
- Trace secret projection swallowed TraceSecretProjectionError in four
catch blocks (per-field omission, whole-tree fallback, post-transform
invariant, structural traversal) across ~30 distinct throw sites, so no
warning said which invariant fired. Each now reports the failure. Only
TraceSecretProjectionError messages are logged — they are fixed literals
describing an invariant. A failure raised outside the module may quote
trace content (a JSON parse error embeds the text it choked on), so those
are reported by name only.
- ExecutionLogger's unbilled-charge error logged `"error":{}` because a
plain Error has non-enumerable message/stack. It now logs describeError.
- WorkspaceFileStorage / FetchExternalUrl logged `saveError:{}` for the
same reason, and the upload wrapper rethrew without a cause, so Drizzle's
`Failed query:` wrapper dropped the Postgres SQLSTATE. The wrapper now
chains the cause and both sites log describeError, which reports the
deepest link's code.
describeError additionally strips the `params:` tail Drizzle appends to its
message, so bound parameter values never reach logs.390dfd7 to
a5e0276CompareUh oh!
There was an error while loading. Please reload this page.
Problem
Three log sites discard the underlying error. All three were hit in production and each independently blocked root-cause analysis.
Sites
1.
lib/logs/execution/trace-secret-projection.ts— four barecatch {}blocks fell back to structural-only spans with no reason attached, across ~30 distinct throw sites (node/depth/array limits, cyclic content, non-plain prototypes, accessor properties, ref-fetch failures, budget exceeded). Now each reports which invariant fired.Security: every
TraceSecretProjectionErrormessage is a compile-time literal (the${label}interpolations are all static constants), so logging it is safe. Any other failure originates outside the module and could quote trace content — aSyntaxErrorfromJSON.parseembeds the text it choked on — so those are reported bynameonly.describeProjectionFailureenforces the split.structuralOnlySpanstill stripserrorMessage, deliberately. It is a block's error text and routinely embeds resolved secret values (a provider error echoing a URL or header). The structural fallback exists precisely because projection/matching failed, so no redaction can be trusted on that path — retaining it would leak plaintext exactly when redaction is known broken.2.
ExecutionLogger—"Failed to record execution usage to usage_log ledger; charge may be unbilled"logged"error":{}.Error.message/stackare non-enumerable, soObject.assign(entry, { error })+JSON.stringifyin the production logger yields{}. Now logsdescribeError(error).3.
WorkspaceFileStorage/FetchExternalUrl— samesaveError:{}problem, plusuploadWorkspaceFilerethrewnew Error(...)without a cause, so Drizzle'sFailed query:wrapper — and with it the Postgres SQLSTATE — was unrecoverable. All 137 prod occurrences were the sameselect ... from "workspace" where id = $1 limit $2 for updateinlib/billing/storage/tracking.ts. The rethrow now chains{ cause: error }and both sites logdescribeError, which reports the deepest link'scode. If the leading hypothesis (SELECT ... FOR UPDATErouted to a read replica) is right, the next occurrence logs SQLSTATE25006and settles it. That hypothesis is not proven here — this change only makes it observable.What is now logged
Error names, messages, and driver codes/SQLSTATE. No user data, no secret plaintext, no query parameter values.
describeErroradditionally strips the\nparams: <values>tail thatDrizzleQueryErrorappends to its message, closing a pre-existing leak for every existing caller.Tests
7 new tests, one per behavior; all verified failing with the source changes stashed.