History

Cover the fingerprint wiring and pin the hash threshold

Rhys Sullivan committed 2 months ago

Commit f8f033f1 parent 4c3f2229 files · +193 −70Browse files

Unified · Split

9 files
  1. apps/cloud/src/observability/index.ts +29 −11
  2. apps/cloud/src/observability/observability.test.ts +20 −2
  3. apps/cloud/src/server.ts +3 −26
  4. apps/desktop/src/main/crash-fingerprint.test.ts +25 −2
  5. apps/desktop/src/main/crash-fingerprint.ts +13 −0
  6. apps/desktop/src/main/diagnostics.ts +2 −5
  7. packages/app/src/crash-reporting.ts +2 −5
  8. packages/core/sdk/src/sentry-grouping.test.ts +87 −19
  9. packages/core/sdk/src/sentry-grouping.ts +12 −0
apps/cloud/src/observability/index.ts+29−11
@@ -16,7 +16,7 @@ import { Cause, Effect, Layer } from "effect"; · 15 unmodified lines
1616 import type * as Tracer from "effect/Tracer";
1717
1818 import { ErrorCapture } from "@executor-js/api";
19−import { stableGroupingFingerprint, type GroupingEvent } from "@executor-js/sdk/sentry-grouping";
19+import { withStableGroupingFingerprint } from "@executor-js/sdk/sentry-grouping";
2020
2121 // Drizzle/postgres-js include the failing SQL (params + bound values) in
2222 // their error message. For OpenAPI source inserts that's 1MB+ of spec
@@ -148,19 +148,13 @@ export const beforeSendWithOtelCorrelation = ( · 125 unmodified lines
148148 };
149149
150150 /**
151+ * The single `beforeSend` the worker and its Durable Objects install.
152+ *
151153 * The worker ships as content-hashed chunks and its frames are not resolved
152154 * back to source, so Sentry's default grouping keys on names like
153155 * `execution-rate-limit-<hash>` and re-opens every issue on the next deploy.
154− * Pin a fingerprint with the hash normalized out; events with no hashed
155− * grouping input are left on the default algorithm.
156− */
157−export const withStableGroupingFingerprint = <T extends GroupingEvent>(event: T): T => {
158− const fingerprint = stableGroupingFingerprint(event);
159− return fingerprint ? { ...event, fingerprint: [...fingerprint] } : event;
160−};
161−
162−/**
163− * The single `beforeSend` the worker and its Durable Objects install.
156+ * `withStableGroupingFingerprint` pins a key with the hash normalized out;
157+ * events with no hashed grouping input are left on the default algorithm.
164158 *
165159 * The two stages are independent and compose in this order: the capture-owner
166160 * pass decides WHETHER the event is reported at all (a cause the Durable
@@ -176,6 +170,30 @@ export const beforeSendCloudEvent = ( · 9 unmodified lines
176170 return reported === null ? null : withStableGroupingFingerprint(reported);
177171 };
178172
173+/**
174+ * The Sentry options the worker and every Durable Object install. It lives
175+ * beside the `beforeSend` it wires so the composition is covered by
176+ * observability.test.ts; `server.ts` only passes this through.
177+ *
178+ * NOTE: do NOT enable `instrumentPrototypeMethods`. It walks the DO prototype
179+ * and reads every property — including accessors — to find methods to wrap,
180+ * which invokes the `sessionId` getter with `this` bound to the prototype
181+ * (where `ctx` is undefined) and throws during construction, 500ing every
182+ * session create / cold restore. The DO captures its own errors via the
183+ * `captureCause` seam (→ Sentry) instead.
184+ */
185+export const cloudSentryOptions = (env: Env) => ({
186+ dsn: env.SENTRY_DSN,
187+ tracesSampleRate: 0,
188+ enableLogs: true,
189+ sendDefaultPii: true,
190+ skipOpenTelemetrySetup: true,
191+ beforeSend: (event: ErrorEvent) =>
192+ beforeSendCloudEvent(event, {
193+ logPayload: !env.SENTRY_DSN || env.SENTRY_OTEL_LOG_PAYLOAD === "true",
194+ }),
195+});
196+
179197 export const addCurrentOtelCorrelationTags = <
180198 T extends { readonly tags?: Record<string, unknown> },
181199 >(
apps/cloud/src/observability/observability.test.ts+20−2
@@ -8,6 +8,7 @@ import { · 7 unmodified lines
88 addCurrentOtelCorrelationTags,
99 beforeSendCloudEvent,
1010 beforeSendWithOtelCorrelation,
11+ cloudSentryOptions,
1112 DO_CAUSE_OWNER_TAG,
1213 DO_CAUSE_OWNER_VALUE,
1314 OTEL_SPAN_ID_TAG,
@@ -86,8 +87,10 @@ describe("sentryPayloadForCause", () => { · 72 unmodified lines
8687 });
8788
8889 // Grouping keys are decided inside the Sentry SDK and never appear on any
89−// product surface, so the e2e harness cannot observe them; the running
90−// beforeSend itself is covered by e2e/cloud/sentry-otel-correlation.test.ts.
90+// product surface, so the e2e harness cannot observe them. The split is:
91+// e2e/cloud/sentry-otel-correlation.test.ts proves the worker really installs
92+// `cloudSentryOptions.beforeSend` (its correlation payload only exists if that
93+// hook ran), and the tests here prove the hook it installs fingerprints.
9194 describe("Sentry grouping", () => {
9295 // The worker bundle ships as content-hashed chunks, so the only module name
9396 // Sentry ever sees for a given frame changes on every deploy.
@@ -143,6 +146,21 @@ describe("Sentry grouping", () => { · 49 unmodified lines
143146 expect(sent).not.toBeNull();
144147 expect(sent?.fingerprint).toBeUndefined();
145148 });
149+
150+ // The wiring check: this is the exact object handed to `Sentry.withSentry`
151+ // and `instrumentDurableObjectWithSentry` in server.ts. If the normalizer is
152+ // ever dropped from the hook the worker installs, this fails.
153+ it("the options the worker and DOs install carry the fingerprinting hook", () => {
154+ const options = cloudSentryOptions({ SENTRY_DSN: "https://public@example.invalid/1" } as Env);
155+ const sent = options.beforeSend(workerEvent("BAuwphPA"));
156+
157+ expect(sent?.fingerprint).toEqual([
158+ "GateCheckTimeoutError",
159+ "timeoutOrElse@execution-rate-limit",
160+ ]);
161+ // Same source, next deploy, new chunk hash — one issue, not two.
162+ expect(options.beforeSend(workerEvent("DkcPBbWe"))?.fingerprint).toEqual(sent?.fingerprint);
163+ });
146164 });
147165
148166 describe("Sentry OTel correlation", () => {
apps/cloud/src/server.ts+3−26
@@ -1,6 +1,5 @@
11 import { DurableObject } from "cloudflare:workers";
22 import { SpanKind, SpanStatusCode, context, trace, type SpanContext } from "@opentelemetry/api";
3−import type { ErrorEvent } from "@sentry/cloudflare";
43 import {
54 ATTR_HTTP_REQUEST_METHOD,
65 ATTR_HTTP_RESPONSE_STATUS_CODE,
@@ -19,7 +18,7 @@ import { classifyMcpPath, prepareMcpOrgScope } from "./mcp/mount"; · 12 unmodified lines
1918 import { parseTraceparent } from "./mcp/traceparent";
2019 import { McpSessionDOSqlite as McpSessionDOBase } from "./mcp/session-durable-object";
2120 import {
22− beforeSendCloudEvent,
21+ cloudSentryOptions,
2322 captureCause,
2423 otelCorrelationContextFromOpenTelemetrySpan,
2524 SENTRY_EVENT_ID_ATTRIBUTE,
@@ -28,28 +27,6 @@ import { · 2 unmodified lines
2827 import { browserTracesResponse } from "./observability/browser-traces";
2928 import { flushTracerProvider, installTracerProvider } from "./observability/telemetry";
3029
31−// ---------------------------------------------------------------------------
32−// Sentry config
33−// ---------------------------------------------------------------------------
34−
35−const sentryOptions = (env: Env) => ({
36− dsn: env.SENTRY_DSN,
37− tracesSampleRate: 0,
38− enableLogs: true,
39− sendDefaultPii: true,
40− skipOpenTelemetrySetup: true,
41− beforeSend: (event: ErrorEvent) =>
42− beforeSendCloudEvent(event, {
43− logPayload: !env.SENTRY_DSN || env.SENTRY_OTEL_LOG_PAYLOAD === "true",
44− }),
45− // NOTE: do NOT enable `instrumentPrototypeMethods`. It walks the DO prototype
46− // and reads every property — including accessors — to find methods to wrap,
47− // which invokes the `sessionId` getter with `this` bound to the prototype
48− // (where `ctx` is undefined) and throws during construction, 500ing every
49− // session create / cold restore. The DO captures its own errors via the
50− // `captureCause` seam (→ Sentry) instead.
51−});
52−
5330 // ---------------------------------------------------------------------------
5431 // Durable Object — wrapped with Sentry so DO errors land in Sentry (inits the
5532 // client inside the DO isolate, which plain `Sentry.captureException` cannot
@@ -58,7 +35,7 @@ const sentryOptions = (env: Env) => ({ · 2 unmodified lines
5835 // ---------------------------------------------------------------------------
5936
6037 export const McpSessionDOSqlite = Sentry.instrumentDurableObjectWithSentry(
61− sentryOptions,
38+ cloudSentryOptions,
6239 McpSessionDOBase,
6340 );
6441
@@ -458,4 +435,4 @@ const cloudflareHandler: ExportedHandler<Env> = { · 393 unmodified lines
458435 },
459436 };
460437
461−export default Sentry.withSentry(sentryOptions, cloudflareHandler);
438+export default Sentry.withSentry(cloudSentryOptions, cloudflareHandler);
apps/desktop/src/main/crash-fingerprint.test.ts+25−2
@@ -1,11 +1,19 @@
11 import { expect, test } from "@effect/vitest";
22
3−import { crashReportFingerprint, type CrashEvent } from "./crash-fingerprint";
3+import {
4+ crashReportFingerprint,
5+ withCrashReportFingerprint,
6+ type CrashEvent,
7+} from "./crash-fingerprint";
8+
9+/** A crash event as Sentry hands it to `beforeSend` — with the grouping key
10+ * slot the hook is allowed to fill. */
11+type SentCrashEvent = CrashEvent & { readonly fingerprint?: readonly string[] | undefined };
412
513 // Chromium's soft-assert path (`NOTREACHED()`/`DCHECK`) dumps and keeps
614 // running. Sentry titles each one with the faulting load address, so one
715 // condition arrives as a new issue every time the address moves.
8−const softAssertEvent = (address: string): CrashEvent => ({
16+const softAssertEvent = (address: string): SentCrashEvent => ({
917 exception: {
1018 values: [
1119 {
@@ -86,6 +94,21 @@ test("renderer chunk hashes are normalized out of the fingerprint", () => { · 74 unmodified lines
8694 expect(release1).toEqual(release2);
8795 });
8896
97+// `withCrashReportFingerprint` is the function object diagnostics.ts installs
98+// as its `beforeSend`, so these assertions cover the main-process wiring and
99+// not just the classifier behind it.
100+test("the main-process beforeSend pins the key and forwards the event", () => {
101+ const collapsed = withCrashReportFingerprint(softAssertEvent("0x00000001a2b3c4d5"));
102+ expect(collapsed.fingerprint).toEqual(["chromium-dump-without-crashing"]);
103+ // Nothing else about the event is touched — this is grouping only.
104+ expect(collapsed.exception?.values?.[0]?.value).toBe("Simulated Exception / 0x00000001a2b3c4d5");
105+
106+ const untouched: SentCrashEvent = {
107+ exception: { values: [{ type: "EXC_CRASH", stacktrace: { frames: [{ function: "abort" }] } }] },
108+ };
109+ expect(withCrashReportFingerprint(untouched)).toBe(untouched);
110+});
111+
89112 test("events with no volatile grouping input are left alone", () => {
90113 expect(crashReportFingerprint({})).toBeUndefined();
91114 expect(
apps/desktop/src/main/crash-fingerprint.ts+13−0
@@ -16,6 +16,7 @@ · 15 unmodified lines
1616
1717 import {
1818 stableGroupingFingerprint,
19+ withStableGroupingFingerprint,
1920 type GroupingEvent,
2021 type GroupingFrame,
2122 } from "@executor-js/sdk/sentry-grouping";
@@ -37,3 +38,15 @@ export const crashReportFingerprint = (event: CrashEvent): readonly string[] | u · 15 unmodified lines
3738 if (frames.some(isSoftAssertFrame)) return [CHROMIUM_SOFT_ASSERT_FINGERPRINT];
3839 return stableGroupingFingerprint(event);
3940 };
41+
42+/**
43+ * The `beforeSend` the Electron main process installs. Grouping only — an
44+ * event with no volatile grouping input is forwarded unchanged.
45+ */
46+export const withCrashReportFingerprint = <T extends CrashEvent>(event: T): T => {
47+ const frames = (event.exception?.values ?? []).flatMap((value) => value.stacktrace?.frames ?? []);
48+ if (frames.some(isSoftAssertFrame)) {
49+ return { ...event, fingerprint: [CHROMIUM_SOFT_ASSERT_FINGERPRINT] };
50+ }
51+ return withStableGroupingFingerprint(event);
52+};
apps/desktop/src/main/diagnostics.ts+2−5
@@ -23,7 +23,7 @@ import { dirname, join } from "node:path"; · 22 unmodified lines
2323 import { app, crashReporter, dialog, shell } from "electron";
2424 import log from "electron-log/main.js";
2525 import * as Sentry from "@sentry/electron/main";
26−import { crashReportFingerprint } from "./crash-fingerprint";
26+import { withCrashReportFingerprint } from "./crash-fingerprint";
2727 import { getServerSettings } from "./settings";
2828
2929 const sentryDsn = __EXECUTOR_SENTRY_DSN__;
@@ -91,10 +91,7 @@ export const initErrorReporting = () => { · 61 unmodified lines
9191 },
9292 },
9393 // Grouping only — the event is forwarded untouched otherwise.
94− beforeSend: (event) => {
95− const fingerprint = crashReportFingerprint(event);
96− return fingerprint ? { ...event, fingerprint: [...fingerprint] } : event;
97− },
94+ beforeSend: withCrashReportFingerprint,
9895 });
9996 } else {
10097 // No DSN baked in — keep native crash dumps local so a user-reported
packages/app/src/crash-reporting.ts+2−5
@@ -12,7 +12,7 @@ · 11 unmodified lines
1212 * once initialized — no reporter rewiring needed.
1313 */
1414
15−import { stableGroupingFingerprint } from "@executor-js/sdk/sentry-grouping";
15+import { withStableGroupingFingerprint } from "@executor-js/sdk/sentry-grouping";
1616
1717 interface CrashReportingConfig {
1818 readonly dsn: string;
@@ -51,10 +51,7 @@ export const initDesktopCrashReporting = (): void => { · 32 unmodified lines
5151 // `atoms-<hash>` and one bug re-groups on every release. Pin a
5252 // fingerprint with the hash normalized out; the event itself keeps its
5353 // hashed filenames so sourcemap resolution is unaffected.
54− beforeSend: (event) => {
55− const fingerprint = stableGroupingFingerprint(event);
56− return fingerprint ? { ...event, fingerprint: [...fingerprint] } : event;
57− },
54+ beforeSend: withStableGroupingFingerprint,
5855 });
5956 } catch {
6057 // Reporting failures stay silent — there is nowhere left to report them.
packages/core/sdk/src/sentry-grouping.test.ts+87−19
@@ -4,6 +4,7 @@ import { · 3 unmodified lines
44 containsContentHash,
55 stableGroupingFingerprint,
66 stripContentHashes,
7+ withStableGroupingFingerprint,
78 type GroupingEvent,
89 } from "./sentry-grouping";
910
@@ -65,6 +66,30 @@ test("stripContentHashes keeps genuinely different modules distinct", () => { · 55 unmodified lines
6566 );
6667 });
6768
69+// Merging two unrelated bugs into one issue is unrecoverable, so the "is this
70+// a hash?" threshold is pinned from both sides: a segment needs two of the
71+// three hash signals (uppercase, uppercase+digit, digits) before it is eaten.
72+// Loosening it to one signal — the tempting fix for the word-shaped hashes
73+// this deliberately misses — silently collapses real chunk names.
74+test("a name segment one signal short of a hash is left in place", () => {
75+ // One capital, no digits.
76+ expect(stripContentHashes("/assets/connections-Settings.js")).toBe(
77+ "/assets/connections-Settings.js",
78+ );
79+ // One digit, no capitals.
80+ expect(stripContentHashes("/assets/storage-s3client.js")).toBe("/assets/storage-s3client.js");
81+ // ...and neither may be mistaken for the chunk it would collapse onto.
82+ expect(stripContentHashes("/assets/connections-Settings.js")).not.toBe(
83+ stripContentHashes("/assets/connections-BAuwphPA.js"),
84+ );
85+});
86+
87+test("a segment with two hash signals is eaten", () => {
88+ expect(stripContentHashes("/assets/chunk-AbcdefgH.js")).toBe("/assets/chunk.js");
89+ expect(stripContentHashes("/assets/chunk-Abcdefg1.js")).toBe("/assets/chunk.js");
90+ expect(stripContentHashes("/assets/chunk-abcdef12.js")).toBe("/assets/chunk.js");
91+});
92+
6893 test("containsContentHash detects only hashed paths", () => {
6994 expect(containsContentHash("/assets/atoms-Yemn7yhP.js")).toBe(true);
7095 expect(containsContentHash("apps/cloud/src/engine/execution-gate.ts")).toBe(false);
@@ -126,27 +151,41 @@ test("minified single-letter frame functions are left out of the fingerprint", ( · 55 unmodified lines
126151 expect(stableGroupingFingerprint(atoms("U", "Yemn7yhP"))).toEqual(["TypeError", "atoms"]);
127152 });
128153
129−test("two vendor chunks sharing a name stay apart on their callers", () => {
130− // A single build emits many `dist-<hash>.js` chunks from unrelated packages,
131− // so the top frame alone is not a safe key.
132− const vendorEvent = (caller: string, hash: string): GroupingEvent => ({
133− exception: {
134− values: [
135− {
136− type: "TypeError",
137− stacktrace: {
138− frames: [
139− { module: `${caller}-${hash}`, function: caller, in_app: true },
140− { module: `dist-${hash}`, function: "x", in_app: false },
141− ],
142− },
154+// A single build emits many `dist-<hash>.js` chunks from unrelated packages,
155+// so the crashing frame alone is not a safe key — two different bugs would
156+// land in one issue. Frames are oldest-first, so the shared chunk is last.
157+const vendorEvent = (caller: string, hash: string): GroupingEvent => ({
158+ exception: {
159+ values: [
160+ {
161+ type: "TypeError",
162+ stacktrace: {
163+ frames: [
164+ { module: `${caller}-${hash}`, function: caller, in_app: true },
165+ { module: `dist-${hash}`, function: "throwHelper", in_app: true },
166+ ],
143167 },
144− ],
145− },
146− });
168+ },
169+ ],
170+ },
171+});
172+
173+test("two vendor chunks sharing a name stay apart on their callers", () => {
174+ const upload = stableGroupingFingerprint(vendorEvent("uploadArtifact", "BQZkXWT2"));
175+ const render = stableGroupingFingerprint(vendorEvent("renderMarkdown", "BQZkXWT2"));
147176
148− expect(stableGroupingFingerprint(vendorEvent("uploadArtifact", "BQZkXWT2"))).not.toEqual(
149− stableGroupingFingerprint(vendorEvent("renderMarkdown", "BQZkXWT2")),
177+ // The crashing frame is identical in both — only the caller chain separates
178+ // them, so a fingerprint built from the top frame alone would over-merge.
179+ expect(upload?.slice(0, 2)).toEqual(["TypeError", "throwHelper@dist"]);
180+ expect(render?.slice(0, 2)).toEqual(upload?.slice(0, 2));
181+ expect(upload).not.toEqual(render);
182+ expect(upload).toEqual(["TypeError", "throwHelper@dist", "uploadArtifact@uploadArtifact"]);
183+});
184+
185+test("the caller chain is itself deploy-stable", () => {
186+ // Every frame in the chain must lose its hash, not just the crashing one.
187+ expect(stableGroupingFingerprint(vendorEvent("uploadArtifact", "BQZkXWT2"))).toEqual(
188+ stableGroupingFingerprint(vendorEvent("uploadArtifact", "Yemn7yhP")),
150189 );
151190 });
152191
@@ -186,6 +225,35 @@ test("a hashed culprit alone is enough to pin the fingerprint", () => { · 33 unmodified lines
186225 ]);
187226 });
188227
228+// The wrapper every process installs as its `beforeSend`. `fingerprint` and
229+// `tags` stand in for the fields a real Sentry event carries around it.
230+type SentEvent = GroupingEvent & {
231+ readonly fingerprint?: readonly string[] | undefined;
232+ readonly tags?: Record<string, string> | undefined;
233+};
234+
235+test("withStableGroupingFingerprint pins the key and changes nothing else", () => {
236+ const input: SentEvent = { ...workerEvent("BAuwphPA"), tags: { a: "b" } };
237+ const hashed = withStableGroupingFingerprint(input);
238+ expect(hashed.fingerprint).toEqual([
239+ "GateCheckTimeoutError",
240+ "timeoutOrElse@execution-rate-limit",
241+ ]);
242+ // The event still carries its hashed filename, or server-side sourcemap
243+ // resolution would stop finding the release's artifacts.
244+ expect(hashed.exception?.values?.[0]?.stacktrace?.frames?.[0]?.filename).toBe(
245+ "/assets/execution-rate-limit-BAuwphPA.js",
246+ );
247+ expect(hashed.tags).toEqual({ a: "b" });
248+});
249+
250+test("withStableGroupingFingerprint forwards an unhashed event untouched", () => {
251+ const plain: SentEvent = { culprit: "checkExecutionBalance(execution-gate.ts)" };
252+ const out = withStableGroupingFingerprint(plain);
253+ expect(out).toBe(plain);
254+ expect("fingerprint" in out).toBe(false);
255+});
256+
189257 test("the outermost exception drives the fingerprint", () => {
190258 // Sentry orders `values` innermost-first; the last entry is the one whose
191259 // type the issue is titled with.
packages/core/sdk/src/sentry-grouping.ts+12−0
@@ -155,3 +155,15 @@ export const stableGroupingFingerprint = (event: GroupingEvent): readonly string · 154 unmodified lines
155155 if (keys.length > 0) return [type, ...keys];
156156 return culprit ? [type, stripContentHashes(culprit)] : undefined;
157157 };
158+
159+/**
160+ * The whole `beforeSend` contract: pin the deploy-stable fingerprint when the
161+ * event has a volatile grouping input, and forward the event untouched when it
162+ * does not. Every call site (cloud worker/DO, desktop main, desktop renderer)
163+ * installs this one symbol rather than its own copy, so the wiring is covered
164+ * by the tests below instead of being retyped per process.
165+ */
166+export const withStableGroupingFingerprint = <T extends GroupingEvent>(event: T): T => {
167+ const fingerprint = stableGroupingFingerprint(event);
168+ return fingerprint ? { ...event, fingerprint: [...fingerprint] } : event;
169+};