Uh oh!
There was an error while loading. Please reload this page.
Recognise Apple crash frames in the error fingerprint - #550
Merged
Conversation
FRAME_LINE_PATTERN matches stack frames by shape, one alternative per runtime — V8/JVM, Python, Ruby, Firefox/Safari, Go/Rust. It had none for Apple's, so no frame of an iOS crash matched anything, _fpFrames came out empty, and the hash fell through to _msgFallback. That branch redacts hex and long digit runs, which normalises `EXC_BAD_ACCESS at 0x10` and `at 0xdeadbeef` to the same string. The result was one issue per exception type per service, however many distinct bugs were behind it. Measured against a local store: four crashes with different stacks and different messages all produced fingerprint 10540390751701351631, every one with an empty TopFrame. The new alternative keys on frame index, binary name and hex address: ^[0-9]+ +[^ ]+ +0x[0-9a-fA-F]+ There is no source position to key on. An iOS crash arrives unsymbolicated, because the app's symbols live in a dSYM that never leaves the build machine. FRAME_REDACTIONS then erases the address and the (deliberately hex-rendered) offset, so a frame reduces to `index binary +` and grouping keys on the sequence of binaries. Coarse, but stable across releases — raw offsets shift with any code change and would re-split every iOS issue on every build. When dSYM symbolication lands, function names drop into the same slot. Verified end to end against a fresh local store: the same crash site on two builds now shares a fingerprint, a different site separates, and a stack with no frames still falls back to the message. FINGERPRINT_VERSION goes 2 -> 3 in the same change; only iOS hashes rotate, no other runtime's frame matching is touched. Nothing is backfilled, for the reason 0003 and 0016 give — recomputing FingerprintHash would re-bucket every existing issue, and the version bump retires the collapsed iOS ones instead. Migration 0018 recreates both frozen MVs for deployed servers, and the local store gets the matching v7 -> v8 edge.
…nary names Two defects found reviewing the previous commit. The FINGERPRINT_VERSION bump would have archived every error issue in every org, permanently, for every runtime. ErrorsService archives every kind:"error" issue whose fingerprintVersion is below the constant. The premise for that, stated on the column itself, is that a row on an older version can never receive another occurrence — true only when a bump rotates every hash. This change rotates iOS hashes alone, so every Node, Python, Go and browser hash is byte-identical across the two versions. Their issues would be archived by the next retention phase, and nothing would bring them back: the tick's upsert conflicts on (orgId, fingerprintHash), which is unique, and its SET never clears archivedAt. The rows would keep accumulating occurrences while invisible in /errors. So the version stays at 2. The cost is bounded and small: a collapsed iOS issue stops receiving occurrences the moment its hash changes and retires through the ordinary resolved window instead of on sight. Retiring by hash rather than by version is what would let a partial-rotation change bump this safely, and that belongs in the retention model, not here. The frame alternative also required the binary name to be one space-free token, so it never matched an app whose Mach-O image name contains a space. That name is the target's PRODUCT_NAME, and "My App" is an ordinary thing to call an app — those apps stayed collapsed on the message hash, which is the exact failure the alternative exists to fix. The name is matched as `\S.*` now. Fixing it here rather than in the SDK's renderer is deliberate: 0.3.0 is already tagged, and old SDK versions stay in the wild for a long time. A backend fix covers the crashes they are already sending. Verified against a fresh local store: "MyApp" and "My App" each group their own two builds of one crash site together and separate a different site.
The probe asserts the marker the native migration promotes to, and it is pinned to the head identity on purpose: that is what proves the migration walks all the way to head rather than merely running. Its own comment says it has to move in lockstep with LOCAL_SCHEMA_VERSION, and I left it on v7. The chain itself was fine — all eight steps verified in CI, including v7 -> v8 — so the failure was the probe's expectation, not the migration.
Five bespoke `throw new Error` branches and a hand-rolled `isRecord`, copied wholesale from the v5 -> v6 module, replaced by the schema they were approximating. Effect Schema is what the repo uses for this, and `decodeChildMetrics` in apps/cli/src/commands/archive.ts already establishes the exact idiom — `decodeUnknownSync` with `onExcessProperty: "error"`. The signature stays synchronous and throwing. `decodeState` is part of `LocalStoreMigrationModule`, shared with seven sibling modules and called from the plain-TS runner that drives them; making this one return an Effect would mean refactoring all of it, which is not this change. The schema is the part that matters, and `decodeUnknownEffect` is a one-liner away if that runner ever moves. Behaviour is preserved exactly, and now has tests rather than being asserted by reading: unknown fields rejected, another module's state rejected, row counts that are not unsigned decimal strings rejected, a missing or unknown raw table rejected, a non-integer retention floor rejected, absent progress distinguished from invalid progress. `retentionDays` is `optionalKey` rather than `optional`: the journal is JSON, where an absent floor is an absent key, not a present `undefined`. The literal-returning branch in `preflight` keeps it that way, and satisfies no-conditional-empty-object-spread — which is what the original's `!(x === undefined) ? … : undefined` was working around, obscurely.
The sweep the v7 -> v8 module started. All seven remaining edges carried hand-rolled validation — six of them byte-identical copies of the same `isRecord` + `decodeCounts` loop + five `throw new Error` branches, differing only in the strings. Those six now share `journal-codecs.ts`: one `RawRowsSchema` built from RAW_TELEMETRY_TTL_COLUMNS, so a table added there is covered without touching any edge, and one `makeRawRowsState(moduleId)` for the state shape they all round-trip. legacy-to-current carried its own parser combinator library — `record`, `nonEmptyString`, `nullableString`, `uint64String`, `nonNegativeInteger`, `exactKeys` — under four nested decoders. Those are now schemas, with the two genuine invariants left as explicit checks after the decode, because both are about the map *keys* rather than the values: a table key this build does not know, and an inventory whose own `table` field disagrees with the key it is stored under. The second one matters — if those disagree the journal describes a copy of one table under another's cursor. Only one test touched that decoder beforehand, which is far too thin a net for rewriting the validation guarding a resumable copy out of a pre-v1 store, so its accept/reject boundary is characterized first (9 cases, written against the old implementation and passing before anything changed). That immediately paid for itself. `Schema.Record` keyed on a literal union requires *every* key to be present, but `copied` legitimately holds a subset — it starts empty and fills one table at a time as the replay progresses. The strict keying rejected the journal of every partially-progressed migration. The maps are keyed by `Schema.String` with the table names checked after, which is what the hand-rolled loop did. One pre-existing assertion moves from the old decoder's exact wording to the field it fails on; the behaviour it guards is unchanged. 555 lines deleted, 302 added.
…hema The migration journal, store marker, retention config, and archive journal are all untrusted JSON written by some build of this CLI, not necessarily this one. Each was decoded by hand: isRecord guards, typeof chains, an `as string` on every field read, and validation duplicated between the read path and the write path. Declare the shapes instead: - The coordinator journal envelope moves to journal-schema.ts, taking local-store-migrations.ts from 23 casts to one. MigrationPhase, MigrationStepStatus, and MigrationStepJournal now derive from the schema, so the persisted form and the in-memory type cannot drift. The chain invariants stay imperative — they relate phase, currentStepIndex, and per-step status, which is a state machine rather than a struct. - makeStoreMarker constructs through the schema readMarkerState decodes with, so the 16-hex and 64-hex rules are stated once instead of four times. Provenance fields stay lenient on purpose: a store with a garbled `maple` version is still an openable store, and refusing to start over it would be a worse failure than recording "unknown". - parseJsonEachRow was copy-pasted nine times, each ending in `JSON.parse(line) as A` at the point where chDB hands us bytes. One helper now, with the row shapes declared. - The seven versioned edges shed their identical RAW_TABLES, rawRowCounts, and expectedManifest copies. - schema-identity.ts's eight repeated identity blocks and SQL/manifest triples collapse to a derived snapshot table, and the eight copy-pasted checks in the schema gate become a loop. That closes a gap: the copies stopped at v7, so v8 was never verified. A non-vacuity guard keeps the loop honest. - In archives/, only the genuinely duplicated shape rules move — the non-negative safe integer checked at four sites and the sha256 pattern at six. The rest of those parsers are domain invariants with individually tested messages, and a struct declaration would state them worse. Behaviour-preserving throughout: no on-disk format changes, and every schema fingerprint, digest, and manifest digest is byte-identical. Verified with the full CLI suite, the schema gate, and the native probes for migration, retention, checkpoints, and archives.
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FRAME_LINE_PATTERNmatches stack frames by shape, one alternative per runtime — V8/JVM, Python, Ruby, Firefox/Safari, Go/Rust. It had none for Apple's, so no frame of an iOS crash matched anything,_fpFramescame out empty, and the hash fell through to_msgFallback. That branch redacts hex and long digit runs, which normalisesEXC_BAD_ACCESS at 0x10andat 0xdeadbeefto the same string.The result was one issue per exception type per service, however many distinct bugs were behind it. Measured against a local store — four crashes, different stacks, different messages:
The change
A sixth alternative, keying on frame index, binary name and hex address:
There is no source position to key on: an iOS crash arrives unsymbolicated, because the app's symbols live in a dSYM that never leaves the build machine.
FRAME_REDACTIONSthen erases the address and the (deliberately hex-rendered) offset, so a frame reduces toindex binary +and grouping keys on the sequence of binaries.That is coarse — binaries rather than functions. It is also the only option that is stable across releases: raw offsets shift with any code change above them and would re-split every iOS issue on every build. When dSYM symbolication lands, function names drop into the same slot.
The binary name is matched as
\S.*rather than as one space-free token. A Mach-O image name is the target's PRODUCT_NAME, andMy Appis an ordinary thing to call an app; requiring a single token would have silently excluded every such app from frame matching and left it collapsed on the message hash.FINGERPRINT_VERSIONis deliberately NOT bumpedThe rule on that constant says to bump it whenever frame matching rotates hashes for errors that are still occurring, which this does — for iOS. Bumping it anyway would have been a serious regression, so the version stays at 2.
The retirement it drives is version-keyed, not hash-keyed.
ErrorsServicearchives everykind: "error"issue whosefingerprintVersionis below the constant, on the premise — stated on the column itself — that a row on an older version can never receive another occurrence. That holds only when a bump rotates every hash. This change rotates iOS hashes alone, so every Node, Python, Go and browser hash is byte-identical across v2 and v3.Had it bumped: the next retention phase archives every error issue in every org; the next tick re-hits each row through the unique index on
(orgId, fingerprintHash), bumps its version and accumulates occurrences, but never clearsarchivedAt— it is not in the upsert's SET. Every actively firing bug across every runtime ends up permanently archived and invisible in /errors, with no path back.The cost of not bumping is bounded: a collapsed iOS issue stops receiving occurrences the moment its hash changes, and retires through the ordinary resolved window instead of on sight. Retiring by hash rather than by version is what would let a partial-rotation change bump this safely — that belongs in the retention model, and is worth doing separately.
Verification
Fresh local store on schema v8, crashes posted in exactly the format the Swift SDK renders:
Same crash site on two builds → one issue, for both spaced and unspaced binary names. Different site → separate. A stack with no frames still falls back to the message.
Checked for false positives against Node C++ fatals, Android tombstones, Rust backtraces, Go and V8 frames — none match, and there are tests pinning that.
Not backfilled
For the reason 0003 and 0016 give: recomputing
FingerprintHashwould re-bucket every existing issue. iOS crashes already inerror_eventskeep their collapsed hashes for the rest of their TTL.Shape of the diff
fingerprint.ts— the pattern. The MV SQL is generated from it, never hand-edited.0018— DROP + CREATE both frozen MVs for already-deployed servers, byte-identical to the emitter's output and preserving 0016's 4xx guard and 0017'sServiceVersioncolumns.requiredForIngest: false; gating would un-ready every BYO-ClickHouse org for a read-path change the gateway never sees.v7 → v8edge, following thev5 → v6template (the same class of MV-recreate change), plus the regenerated schema artifacts. The v7→v8 schema diff touches only these two MVs.Pairs with maple-swift
0.3.0, which reports crashes as spans carrying an OTelexceptionevent — no new backend pipeline, the existing error contract carries it.