From f20d342eaba20783b96237622aca9d2d0ea8172f Mon Sep 17 00:00:00 2001 From: Alexey Karimov Date: Sat, 5 Sep 2026 23:59:31 +0500 Subject: [PATCH] fix(core,browser-utils): let the collector's code win over the status, and bound the browser retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DATA plane classified server answers; the CONTROL plane did not, so a 400 from /v2/issues was retried at every launch — bounded on node by the 7-day retention, and on browser/worker not bounded at all (docs/review/OPEN-FINDINGS.md, "New open items from round 4"). Two halves, deliberately different kinds of thing. WHAT BECAME A VERDICT — the collector's own error code, when it arrives with a status attached. `bugsee-api.ts` threw on `!isOk(status)` BEFORE parsing the body, so a verdict the SDK honours perfectly on an HTTP 200 was discarded the moment the collector also sent a status: `14019 InvalidAppToken` on a 400 was retried forever, and `99099 KillSdk` on a 400 could not switch the SDK off at all. The failed body is now read (`httpFailure` -> `serverErrorCodeOf`) and its code goes on `serverCode`, where the existing drift-tested `classifyServerErrorCode` already sees it. No code was added to the permanent set: the same table now simply reaches a response shape that was being thrown away, which is what Android does on both endpoints (`ReportUploadExecutor.java:468-484`, `CommunicationRequests.obtainSession`). Everything about the parse fails towards `undefined` — a non-JSON body, no error object, a non-numeric code, and `code: 0`, which Android also treats as absent — and `undefined` means retry. WHAT DID NOT — a naked non-2xx. Android's /v2/issues path falls back to `classifyHttpStatus` (any non-401/408/425/429 4xx -> PERMANENT -> the bundle file is deleted) and this SDK deliberately does not follow it there, because Android's OWN /v2/sessions path falls back to `classifyServerErrorCode(0)` -> TRANSIENT for the identical status. Two adjacent calls to one collector cannot both be right, which is the tell that a status is a verdict about the request and not about the bytes; and on the web a bare 4xx is what an intermediary answers (captive portal, MITM proxy, WAF, stale CDN route, service worker), none of which read the payload. Pinned by tests so the decision is falsifiable rather than an omission. The already-decided doubly-403 signed PUT (`renew_failed`) is untouched. WHAT BOUNDS THE REST — `recoverSiblingBundleQueue` now applies `DEFAULT_DURABLE_RETENTION.maxAgeMs`. This is not a new policy: it is the same 7 days the same bytes already meet on the same tier when an instance recovers its OWN queue, and the TTL node's dead-subtree sweep uses. Age only — the count and byte caps evict the oldest SURVIVORS, which on this leg would delete a report the collector was never asked about merely because a burst arrived after it. A frame with no `firstSeenMs` is NEVER expired: reading unknown as the epoch would delete every pending report on the launch after an upgrade, and that rail is load-bearing — injecting `firstSeenMs ?? 0` is caught 23 times over by the pre-existing P2 in harness set H. Evictions are announced via `pipeline.drop('retention_expired', 'issue')`, never silent. Reproduced before fixing, and the reproductions are the harness cases. Set M gains 16 control cases (the same envelopes over a non-2xx, plus 6 naked statuses as a fence) and a new set N drives the browser age bound across four launches over real IndexedDB. Pre-fix: 7 violations in M, including "P7 the client KEPT CAPTURING after the collector switched the SDK off", and "it was still OFFERED 9x past the age bound" in N. Post-fix: 369 cases, 0 violations. The harness oracle was checked before being relied on: the flaw recorded in OPEN-FINDINGS Evidence (keying on `reportId ?? 'anon:' + summary`) is in the ROUND-1 copy under `.session-artifacts/`. The committed harness no longer does that and imports no SDK predicate. Core also exports `deserializeBundleFrame` and `DEFAULT_DURABLE_RETENTION`, so the bound is read from one place rather than restated. Gates: pnpm ci:local green (lint, typecheck 100/100, cycles, per-package coverage) plus --force --e2e (54 tasks, 195 e2e tests). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- .changeset/control-plane-classification.md | 35 ++++ docs/review/OPEN-FINDINGS.md | 39 +++- .../browser-utils/src/coexistence.test.ts | 72 ++++++- .../src/recover-dead-instances.test.ts | 148 +++++++++++++- .../src/recover-dead-instances.ts | 63 +++++- packages/core/src/bugsee-api.test.ts | 116 +++++++++++ packages/core/src/bugsee-api.ts | 62 +++++- packages/core/src/capture-recovery.test.ts | 4 +- .../core/src/durable-upload-pipeline.test.ts | 56 ++++++ packages/core/src/durable-upload-pipeline.ts | 35 +++- packages/core/src/errors.ts | 19 +- packages/core/src/index.ts | 3 + packages/core/src/transport.ts | 5 +- packages/core/src/upload-pipeline.test.ts | 127 +++++++++++- .../instrumentation-tests/harness/README.md | 18 +- .../harness/invariants.mts | 183 ++++++++++++++++++ 16 files changed, 943 insertions(+), 42 deletions(-) create mode 100644 .changeset/control-plane-classification.md diff --git a/.changeset/control-plane-classification.md b/.changeset/control-plane-classification.md new file mode 100644 index 00000000..56db7c15 --- /dev/null +++ b/.changeset/control-plane-classification.md @@ -0,0 +1,35 @@ +--- +'@bugsee/core': minor +'@bugsee/browser-utils': minor +--- + +The control plane now reads the collector's own error code out of a FAILED response, and the browser / +worker dead-sibling queue gets an age bound. + +Two halves of one asymmetry. The DATA plane classified server answers; the CONTROL plane did not, so a +`400` from `/v2/issues` was retried at every launch — bounded on node by the 7-day retention, and on +browser/worker not bounded at all. + +**What is now classified.** `bugsee-api.ts` threw on a non-2xx status *before* parsing the body, so a +verdict the SDK honours perfectly on an HTTP 200 was discarded the moment the collector attached a status +to it: `14019 InvalidAppToken` delivered with a 400 was retried forever, and `99099 KillSdk` with a status +could not switch the SDK off at all. The failed body is now read for `{ ok: false, error: { code } }` and +the code goes on `BugseeError.serverCode`, where the existing (Android-drift-tested) `classifyServerErrorCode` +sees it. This adds no codes to the permanent set — it applies the set that was already there to a response +shape that was being thrown away. Android does the same on both endpoints. + +**What is deliberately NOT classified.** A NAKED non-2xx — a status with no collector code behind it — +stays retryable. Android's own two endpoints disagree about the identical status, which is the tell that a +status is not a verdict about the payload; and on the web a bare 4xx is what an intermediary answers (a +captive portal, a MITM proxy, a WAF, a stale CDN route), none of which read the bytes. Pinned by tests, so +the decision is falsifiable rather than an omission. + +**What bounds the rest.** `recoverSiblingBundleQueue` now applies `DEFAULT_DURABLE_RETENTION.maxAgeMs` +(7 days) — the same bound the same bytes already meet on the same tier when an instance recovers its own +queue, and the same TTL node's dead-subtree sweep uses. Only the age cap is mirrored, not the count/byte +caps: those evict the oldest *survivors*, which here would delete a report the collector was never asked +about. A frame with no staging timestamp (staged by an older SDK) is NEVER expired — reading unknown as the +epoch would delete every pending report on the launch after an upgrade. + +`@bugsee/core` additionally exports `deserializeBundleFrame` and `DEFAULT_DURABLE_RETENTION`, so the bound +is read from one place rather than restated. diff --git a/docs/review/OPEN-FINDINGS.md b/docs/review/OPEN-FINDINGS.md index b8e7609f..488dd9af 100644 --- a/docs/review/OPEN-FINDINGS.md +++ b/docs/review/OPEN-FINDINGS.md @@ -660,12 +660,35 @@ get it wrong. ### New open items from round 4 -- **The control plane never classifies at all** — asymmetry with the data plane. `bugsee-api.ts:82-84` - (`/v2/issues`) and `:107-109` (`/v2/sessions`) throw for every non-2xx and `upload-pipeline.ts:97-125` - defaults `permanent` to false, so a `400` from `/v2/issues` is retried forever. Fails SAFE (data kept), - bounded on node by the 7-day retention, **unbounded on browser/worker** (`recover-dead-instances.ts` - applies none). Deliberately NOT fixed: widening a deletion path unilaterally is what produced a new loss - path in each of the last three rounds. Needs a decision, and should land WITH its harness case. +- **The control plane never classifies at all** — **CLOSED**, on `fix/control-plane-classification`, in two + halves that are deliberately different kinds of thing. + - **What became a verdict:** the collector's OWN error code, when it arrives with a status attached. + `bugsee-api.ts` threw on `!isOk(status)` BEFORE parsing the body, so a code the SDK already honours on + an HTTP 200 was discarded the moment the collector also sent a status — `14019 InvalidAppToken` on a + 400 retried forever, `99099 KillSdk` on a 400 unable to disable the SDK at all. The failed body is now + read (`httpFailure` → `serverErrorCodeOf`) and its code goes on `serverCode`, where the existing + drift-tested `classifyServerErrorCode` sees it. **No code was added to the permanent set** — the same + table now simply reaches a response shape that was being thrown away, which is what Android does on + both endpoints (`ReportUploadExecutor.java:468-484`, `CommunicationRequests.obtainSession`). + - **What did NOT become a verdict:** a naked non-2xx. Android's `/v2/issues` path falls back to + `classifyHttpStatus` (any non-401/408/425/429 4xx → PERMANENT → the bundle file is deleted), and this + SDK deliberately does not follow it, because Android's OWN `/v2/sessions` path falls back to + `classifyServerErrorCode(0)` → TRANSIENT for the identical status. Two adjacent calls to one collector + cannot both be right, and on the web a bare 4xx is what an intermediary answers. Pinned by tests + (`upload-pipeline.test.ts`, "a status is never a verdict here") so the decision is falsifiable. + - **What bounds the rest:** `recoverSiblingBundleQueue` now applies `DEFAULT_DURABLE_RETENTION.maxAgeMs` + (7 days) — the SAME bound these bytes already meet on this tier via the instance's own `recover()`, and + the TTL node's sweep uses. Age only: the count/byte caps evict the oldest SURVIVORS, which on this leg + would delete a report the collector was never asked about. A frame with no `firstSeenMs` is never + expired (the upgrade launch), and that rail is load-bearing — injecting `firstSeenMs ?? 0` is caught 23 + times over by the pre-existing P2 in harness set H. + - **Harness:** set M gained 16 control cases (the same envelopes over a non-2xx, plus 6 naked statuses as + a fence), and a new set N drives the browser age bound across 4 launches over real IndexedDB. 369 cases, + 0 violations. Reproduced first: pre-fix the new M cases give 7 violations (incl. `P7 the client KEPT + CAPTURING after the collector switched the SDK off`) and set N gives `it was still OFFERED 9x past the + age bound`. The §Evidence note that the harness oracle is flawed refers to the ROUND-1 copy in + `.session-artifacts/`; the committed `packages/instrumentation-tests/harness/invariants.mts` no longer + keys on `reportId ?? 'anon:'+summary` and imports no SDK predicate — verified before relying on it. - **A doubly-403 signed PUT** returns `permanent: false` (`upload-pipeline.ts:169-186`, `renew_failed`) where Android calls 403 PERMANENT. Fail-safe, and arguably right since each launch mints a fresh signed URL. Left alone. @@ -912,7 +935,9 @@ The live pipeline treats `permanent` as settled and frees the blob (`recover-instances.ts:89`, `browser-utils/src/recover-dead-instances.ts:39`). So a 4xx-refused bundle is re-uploaded every launch forever, and its marker is never retired either. Bounded at 7 days on node by `sweep-instances.ts:24`; **unbounded on browser/worker** — -`recoverSiblingBundleQueue` applies no retention and no `permanent` check. +`recoverSiblingBundleQueue` applies no retention and no `permanent` check. *(Both halves since fixed: +the `permanent` check by round 4's `isUploadSettled` unification, and the missing retention by the +7-day age bound on `recoverSiblingBundleQueue` — see §New open items from round 4.)* ### R2-4 · `skipReportIds.add` is not throw-safe — SEV3 diff --git a/packages/browser-utils/src/coexistence.test.ts b/packages/browser-utils/src/coexistence.test.ts index 4976fd1e..bcab9f7e 100644 --- a/packages/browser-utils/src/coexistence.test.ts +++ b/packages/browser-utils/src/coexistence.test.ts @@ -76,9 +76,15 @@ const okPipeline = (): UploadPipeline & { enqueue: ReturnType } => }); // Seed a bundle into another instance's prefix within the per-token bundle-queue database. -async function seedSibling(idb: IDBFactory, instanceId: string, bundleId: string, b: Bundle) { +async function seedSibling( + idb: IDBFactory, + instanceId: string, + bundleId: string, + b: Bundle, + firstSeenMs?: number, +) { const shared = createIdbBlobStore({ databaseName: coexistenceDatabaseName(TOK), indexedDB: idb }); - await shared.put(`${instanceId}/${bundleId}`, serializeBundle(b)); + await shared.put(`${instanceId}/${bundleId}`, serializeBundle(b, firstSeenMs)); } // Seed a report marker into a sibling's prefix within the per-token marker database. The bytes are a // REAL serialized ReportMarker — coexistence now hydrates these to reconcile them against the bundle queue. @@ -163,6 +169,68 @@ describe('createCoexistence — pass-through', () => { }); }); +describe('createCoexistence — the dead-sibling age bound, end to end', () => { + // The coordinator wires `recoverSiblingBundleQueue` with its DEFAULTS, so this is what actually + // decides how long a browser or worker keeps re-offering an undeliverable blob. Unit-testing the leg + // alone proves the arithmetic; only driving the real coordinator over real IndexedDB proves it is + // reached at all. On this tier an instance is dead the moment its tab closes, so before the bound a + // blob the collector never settled was replayed on every launch for the lifetime of the installation. + const DAY = 24 * 60 * 60 * 1000; + + const unreachable = () => { + // Never settles: the collector is offline, or answering a bare 4xx this SDK keeps retrying. + const enqueue = vi.fn(() => Promise.resolve({ ok: false })); + const drop = vi.fn(); + return { enqueue, drop, flush: () => Promise.resolve(true) } satisfies UploadPipeline; + }; + + const coexist = (idb: IDBFactory) => + createCoexistence({ + appToken: TOK, + persist: true, + indexedDB: idb, + locks: fakeLocks().manager, + }); + + it('gives up on a blob a dead sibling staged 90 days ago, and keeps yesterday’s', async () => { + const idb = new IDBFactory(); + await seedSibling(idb, 'deadsib', 'ancient', aBundle('ancient'), Date.now() - 90 * DAY); + await seedSibling(idb, 'deadsib', 'recent', aBundle('recent'), Date.now() - DAY); + const pipeline = unreachable(); + + await coexist(idb).recoverDeadSiblings({ uploadPipeline: pipeline }); + + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect((pipeline.enqueue.mock.calls[0]?.[0] as Bundle).request.summary).toBe('recent'); + expect(pipeline.drop).toHaveBeenCalledWith('retention_expired', 'issue'); + const keys = await rawKeys(idb); + expect(keys).not.toContain('deadsib/ancient'); + expect(keys).toContain('deadsib/recent'); // still pending — the next launch tries again + }); + + it('re-offers the SAME recent blob on launch after launch — the bound is age, not attempts', async () => { + const idb = new IDBFactory(); + await seedSibling(idb, 'deadsib', 'b1', aBundle('kept'), Date.now() - DAY); + for (let launch = 0; launch < 3; launch += 1) { + const pipeline = unreachable(); + await coexist(idb).recoverDeadSiblings({ uploadPipeline: pipeline }); + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect(pipeline.drop).not.toHaveBeenCalled(); + } + expect(await rawKeys(idb)).toContain('deadsib/b1'); + }); + + it('never expires a blob staged by an older SDK, which carries no timestamp at all', async () => { + const idb = new IDBFactory(); + await seedSibling(idb, 'deadsib', 'legacy', aBundle('legacy')); // no firstSeenMs + const pipeline = unreachable(); + await coexist(idb).recoverDeadSiblings({ uploadPipeline: pipeline }); + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect(pipeline.drop).not.toHaveBeenCalled(); + expect(await rawKeys(idb)).toContain('deadsib/legacy'); + }); +}); + describe('createCoexistence — bundle queue', () => { it('writes under a per-token database + per-instance prefix and holds the instance lock', async () => { const idb = new IDBFactory(); diff --git a/packages/browser-utils/src/recover-dead-instances.test.ts b/packages/browser-utils/src/recover-dead-instances.test.ts index 7ce532f1..60dca171 100644 --- a/packages/browser-utils/src/recover-dead-instances.test.ts +++ b/packages/browser-utils/src/recover-dead-instances.test.ts @@ -1,4 +1,9 @@ -import { type Bundle, serializeBundle, type UploadPipeline } from '@bugsee/core'; +import { + type Bundle, + DEFAULT_DURABLE_RETENTION, + serializeBundle, + type UploadPipeline, +} from '@bugsee/core'; import { Severity } from '@bugsee/protocol'; import { describe, expect, it, vi } from 'vitest'; import type { AsyncBlobStore } from './idb'; @@ -128,3 +133,144 @@ describe('recoverSiblingBundleQueue', () => { expect(pipeline.enqueue).not.toHaveBeenCalled(); }); }); + +// ══════════════════════════════════════════════════════════════════════════════════════════════════ +// THE AGE BOUND — what stops "retried at every launch" from meaning "forever" on this tier +// ══════════════════════════════════════════════════════════════════════════════════════════════════ +// +// Everything above is about a SETTLED answer. The dangerous case is the one that never settles: a +// collector that is simply unreachable, or a control-plane failure this SDK deliberately keeps retrying +// (a bare 4xx on `/v2/issues` — see `upload-pipeline.test.ts`, "a status is never a verdict here"). +// +// On node such a blob is bounded twice over: `sweep-instances` reaps a dead instance's whole subtree at +// 7 days, and `recover()` applies the durable queue's own retention. This leg has neither — it reads a +// dead instance's prefix DIRECTLY — and on the web an instance is dead the moment its tab closes. So a +// blob that never settles was re-offered on every launch for the lifetime of the installation. +// +// The bound applied here is not a new policy: it is `DEFAULT_DURABLE_RETENTION.maxAgeMs`, the SAME +// 7 days already applied to the SAME bytes on the SAME tier by the instance's own `recover()`, and the +// same TTL node's sweep uses. Only the count and byte caps are deliberately left off — see below. +describe('recoverSiblingBundleQueue — the retention age bound', () => { + const DAY = 24 * 60 * 60 * 1000; + const NOW = 1_800_000_000_000; + const failing = () => { + // The collector is unreachable — never a refusal, so nothing here is ever "settled". + const enqueue = vi.fn(() => Promise.resolve({ ok: false })); + const drop = vi.fn(); + return { enqueue, drop, flush: () => Promise.resolve(true) } satisfies UploadPipeline; + }; + + it('stops re-offering a blob past the age bound, and says so rather than deleting it silently', async () => { + const shared = memBlobWith([ + [ + 'dead/old', + serializeBundle(aBundle('a crash from a tab closed months ago'), NOW - 90 * DAY), + ], + ]); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { now: () => NOW }); + expect(pipeline.enqueue).not.toHaveBeenCalled(); + expect(shared.map.has('dead/old')).toBe(false); + // Announced, exactly as `durable-upload-pipeline` announces its own evictions: a bundle that + // vanishes without an outcome is indistinguishable from one that was delivered. + expect(pipeline.drop).toHaveBeenCalledWith('retention_expired', 'issue'); + }); + + it('keeps replaying a blob INSIDE the bound, launch after launch', async () => { + const shared = memBlobWith([['dead/recent', serializeBundle(aBundle('yesterday'), NOW - DAY)]]); + const pipeline = failing(); + for (let launch = 0; launch < 3; launch += 1) { + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { + now: () => NOW, + }); + } + expect(pipeline.enqueue).toHaveBeenCalledTimes(3); + expect(shared.map.has('dead/recent')).toBe(true); + expect(pipeline.drop).not.toHaveBeenCalled(); + }); + + it('is EXCLUSIVE at the bound — exactly maxAgeMs old is still replayed', async () => { + const shared = memBlobWith([ + ['dead/at', serializeBundle(aBundle('at'), NOW - DEFAULT_DURABLE_RETENTION.maxAgeMs)], + ['dead/past', serializeBundle(aBundle('past'), NOW - DEFAULT_DURABLE_RETENTION.maxAgeMs - 1)], + ]); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { now: () => NOW }); + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect((pipeline.enqueue.mock.calls[0]?.[0] as Bundle).request.summary).toBe('at'); + expect(shared.map.has('dead/at')).toBe(true); + expect(shared.map.has('dead/past')).toBe(false); + }); + + it('NEVER expires a frame that predates `firstSeenMs` — the upgrade launch', async () => { + // `serializeBundle(bundle)` with no timestamp is what every blob staged by an older SDK looks like. + // Reading "unknown" as the epoch would delete every pending crash report on the first launch after + // an upgrade — losing exactly the reports the upgrade was installed to deliver. Same rule as + // `durable-upload-pipeline.recoverPass`. + const shared = memBlobWith([['dead/legacy', serializeBundle(aBundle('legacy'))]]); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { now: () => NOW }); + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect(shared.map.has('dead/legacy')).toBe(true); + expect(pipeline.drop).not.toHaveBeenCalled(); + }); + + it('defaults to the durable queue’s own 7 days, not a second copy of the number', async () => { + const shared = memBlobWith([ + [ + 'dead/old', + serializeBundle(aBundle('old'), Date.now() - DEFAULT_DURABLE_RETENTION.maxAgeMs - 60_000), + ], + ['dead/new', serializeBundle(aBundle('new'), Date.now() - 60_000)], + ]); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline); // no clock, no bound: the defaults + expect(pipeline.enqueue).toHaveBeenCalledTimes(1); + expect(shared.map.has('dead/old')).toBe(false); + expect(shared.map.has('dead/new')).toBe(true); + }); + + it('honours an explicit maxAgeMs override', async () => { + const shared = memBlobWith([['dead/b', serializeBundle(aBundle('b'), NOW - 2 * DAY)]]); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { + now: () => NOW, + maxAgeMs: DAY, + }); + expect(pipeline.enqueue).not.toHaveBeenCalled(); + expect(shared.map.has('dead/b')).toBe(false); + }); + + it('reports a failed eviction to onError instead of throwing out of launch', async () => { + const onError = vi.fn(); + const shared = memBlobWith([['dead/old', serializeBundle(aBundle('old'), NOW - 90 * DAY)]]); + const store: AsyncBlobStore = { + ...shared.store, + remove: () => Promise.reject(new Error('idb gone')), + }; + const pipeline = failing(); + await expect( + recoverSiblingBundleQueue(store, 'dead', pipeline, onError, { now: () => NOW }), + ).resolves.toBeUndefined(); + expect(onError).toHaveBeenCalledTimes(1); + expect(pipeline.enqueue).not.toHaveBeenCalled(); // still withheld — the bound holds either way + }); + + it('bounds by AGE ONLY — a hundred fresh blobs are all still offered', async () => { + // The count and byte caps `recover()` also applies are deliberately NOT mirrored here. They evict + // the OLDEST survivors to make room, which on this leg would mean deleting a crash report the + // collector has never been asked about, purely because a burst arrived after it. Age is the one + // bound where "give up" and "this is worthless now" are the same statement. + const shared = memBlobWith( + Array.from({ length: 100 }, (_, i): [string, Uint8Array] => [ + `dead/b${i}`, + serializeBundle(aBundle(`b${i}`), NOW - DAY), + ]), + ); + const pipeline = failing(); + await recoverSiblingBundleQueue(shared.store, 'dead', pipeline, undefined, { now: () => NOW }); + expect(pipeline.enqueue).toHaveBeenCalledTimes(100); + expect(shared.map.size).toBe(100); + expect(pipeline.drop).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/browser-utils/src/recover-dead-instances.ts b/packages/browser-utils/src/recover-dead-instances.ts index c09d4289..58194724 100644 --- a/packages/browser-utils/src/recover-dead-instances.ts +++ b/packages/browser-utils/src/recover-dead-instances.ts @@ -1,12 +1,45 @@ -import { type Bundle, deserializeBundle, isUploadSettled, type UploadPipeline } from '@bugsee/core'; +import { + DEFAULT_DURABLE_RETENTION, + deserializeBundleFrame, + type IdentifiedBundle, + isUploadSettled, + type UploadPipeline, +} from '@bugsee/core'; import type { AsyncBlobStore } from './idb'; import { createPrefixedBlobStore } from './instance-coexistence'; +/** + * How long a dead sibling's staged bundle may keep being replayed before it is given up on. + * + * This is the ONE bound this tier has, and it exists because "keep retrying" here used to mean literally + * forever. On node a blob that never settles is bounded twice: `sweep-instances` reaps a dead instance's + * whole subtree at 7 days, and any blob replayed through `recover()` meets the durable queue's own + * retention. This leg reads a dead instance's prefix DIRECTLY, so it met neither — and on the web an + * instance is dead the moment its tab closes, so an undeliverable blob was re-offered at every launch for + * the lifetime of the installation. + * + * NOT bounded here, deliberately: the count and byte caps `recover()` also applies. Those evict the + * OLDEST SURVIVORS to make room, which on this leg would delete a crash report the collector has never + * been asked about merely because a burst arrived after it. Age is the one bound where "give up" and + * "this is worthless now" say the same thing. + */ +export interface SiblingQueueRetention { + /** + * Max age of a staged bundle. Default {@link DEFAULT_DURABLE_RETENTION}`.maxAgeMs` (7 days) — taken + * from there rather than restated, because a second copy of a number that decides whether a crash + * report is deleted is a copy that drifts. + */ + maxAgeMs?: number; + /** Wall clock, for the age comparison. Default `Date.now`. */ + now?: () => number; +} + /** * Recover ONE dead instance's durable BUNDLE queue: re-upload each persisted bundle via `pipeline`, dropping * the durable copy once the attempt SETTLES — delivered, or permanently refused. A refused bundle kept here - * would be re-uploaded at every launch for the life of the installation, and nothing in this tier would ever - * bound it (unlike node, browser/worker recovery has no retention sweep at all). + * would be re-uploaded at every launch for the life of the installation. A bundle the collector never + * settles at all is bounded instead — see {@link SiblingQueueRetention}; before that this tier had no + * bound of any kind, where node has both a dead-subtree sweep and the durable queue's own retention. * * AWAITABLE — the coordinator holds the dead instance's Web Lock for the whole duration (so a concurrent peer * skips). Reads straight from the dead instance's prefix of the shared store; unparseable leftovers are purged @@ -17,7 +50,11 @@ export async function recoverSiblingBundleQueue( deadInstanceId: string, pipeline: UploadPipeline, onError: (error: unknown) => void = () => {}, + retention: SiblingQueueRetention = {}, ): Promise { + const maxAgeMs = retention.maxAgeMs ?? DEFAULT_DURABLE_RETENTION.maxAgeMs; + const now = retention.now ?? Date.now; + const at = now(); const view = createPrefixedBlobStore(shared, deadInstanceId); let entries: Array<[string, Uint8Array]>; try { @@ -27,14 +64,30 @@ export async function recoverSiblingBundleQueue( return; } for (const [id, bytes] of entries) { - let bundle: Bundle; + let bundle: IdentifiedBundle; + let firstSeenMs: number | undefined; try { - bundle = deserializeBundle(bytes); + ({ bundle, firstSeenMs } = deserializeBundleFrame(bytes)); } catch (error) { onError(error); view.remove(id).catch(onError); // unparseable leftover — purge it (best-effort) continue; } + // An UNKNOWN staging time is never expired. A frame written before the header carried one reads as + // "we do not know", not as "staged at the epoch" — treating it as the epoch would delete every + // pending bundle on the first launch after an upgrade, losing exactly the crash reports the upgrade + // was installed to deliver. Same rule as `durable-upload-pipeline.recoverPass`. + if (firstSeenMs !== undefined && at - firstSeenMs > maxAgeMs) { + // Announced, not silent: a bundle that vanishes without an outcome is indistinguishable from one + // that was delivered. + pipeline.drop('retention_expired', 'issue'); + try { + await view.remove(id); + } catch (error) { + onError(error); // the bound holds either way — it is withheld from the pipeline regardless + } + continue; + } try { const result = await pipeline.enqueue(bundle); if (isUploadSettled(result)) { diff --git a/packages/core/src/bugsee-api.test.ts b/packages/core/src/bugsee-api.test.ts index 9a794ed2..a9da214f 100644 --- a/packages/core/src/bugsee-api.test.ts +++ b/packages/core/src/bugsee-api.test.ts @@ -280,6 +280,122 @@ describe('createBugseeApi — the v2 response envelope', () => { }); }); +describe('createBugseeApi — a NON-2xx that also carries a collector code', () => { + // THE GAP THIS CLOSES. `unwrap` reads the collector's own `error.code` only out of a 2xx body, + // because a v2 rejection normally arrives with HTTP 200. But the collector does NOT always answer + // 200: a 4xx can carry the same `{ ok: false, error: { code } }` envelope, and the SDK threw on the + // status BEFORE reading it. So `14019 InvalidAppToken` delivered with a 400 produced a BugseeError + // with no `serverCode`, `finalControlPlaneFailure` returned null, and the bundle was retried at every + // launch for the life of the installation — while the byte-identical body on a 200 was classified + // permanent and freed. + // + // Android reads the body on BOTH endpoints' failure paths and lets the collector's code win over the + // status (`ReportUploadExecutor.java:468-484` for /v2/issues, `CommunicationRequests.obtainSession` + // for /v2/sessions, where a non-2xx with `errorCode != 0` becomes `createFailure(errorCode)`). + // + // What does NOT change: the STATUS never becomes a verdict. `code` still carries it, `serverCode` + // carries the collector's code, and a status alone still means retry. + + /** A non-2xx that carries the collector's envelope anyway. */ + const failedWithCode = (status: number, code: number, type = 'InvalidAppTokenError') => ({ + status, + headers: {}, + body: enc({ ok: false, error: { type, message: type, code } }), + }); + + it.each([ + 400, 403, 404, 422, 500, + ])('reads the collector code out of a %i on /v2/issues, with the status still on `code`', async (status) => { + const { transport } = recorder((url) => + url.endsWith('/v2/sessions') ? sessionOk() : failedWithCode(status, 14019), + ); + const a = api(transport); + await a.ensureSession(env); + await expect(a.createIssue(requestJson)).rejects.toMatchObject({ + code: status, // the STATUS namespace — unchanged + serverCode: 14019, // …and the COLLECTOR namespace, which used to be thrown away + }); + }); + + it.each([ + 400, 403, 404, 422, 500, + ])('reads the collector code out of a %i on /v2/sessions too', async (status) => { + const { transport } = recorder(() => failedWithCode(status, 99099, 'KillSdkError')); + await expect(api(transport).ensureSession(env)).rejects.toMatchObject({ + code: status, + serverCode: 99099, + }); + }); + + it('carries the code through renewUpload as well', async () => { + const { transport } = recorder((url) => + url.endsWith('/v2/sessions') ? sessionOk() : failedWithCode(400, 11004), + ); + const a = api(transport); + await a.ensureSession(env); + await expect( + a.renewUpload(requestJson, 'i1' as IssueId, 'r1' as RecordingId), + ).rejects.toMatchObject({ code: 400, serverCode: 11004 }); + }); + + // ── Everything below is the SAFE direction: no code found ⇒ no `serverCode` ⇒ the status alone is + // not a verdict, and the failure stays retryable. Each of these is a body an intermediary can + // produce, and none of them is the collector speaking. + + it('reports NO collector code when a non-2xx body is not JSON at all (an nginx error page)', async () => { + const { transport } = recorder(() => ({ + status: 502, + headers: {}, + body: strToU8('502 Bad Gateway'), + })); + await expect(api(transport).ensureSession(env)).rejects.toMatchObject({ + code: 502, + serverCode: undefined, + }); + }); + + it.each([ + ['an empty object', {}], + ['an error with no code', { ok: false, error: { type: 'Whatever' } }], + ['a JSON scalar', 'nope'], + ['null', null], + ['a JSON array', [1, 2]], + ['a non-numeric code', { ok: false, error: { code: 'oops' } }], + ['a nested error that is a scalar', { ok: false, error: 7 }], + ])('reports NO collector code for %s', async (_label, body) => { + const { transport } = recorder(() => ({ status: 400, headers: {}, body: enc(body) })); + await expect(api(transport).ensureSession(env)).rejects.toMatchObject({ + code: 400, + serverCode: undefined, + }); + }); + + it("treats the collector's `code: 0` as ABSENT, exactly as Android does", async () => { + // `optInt("code", 0)` cannot tell "absent" from "zero", so Android's failure paths both guard on + // `errorCode != 0`. Mirroring that keeps 0 out of the classifier, where it would land on the + // `transient` default anyway — but only by luck, and luck is not the property to rely on when the + // answer decides whether a crash report is deleted. + const { transport } = recorder(() => failedWithCode(400, 0, 'CollectorError')); + await expect(api(transport).ensureSession(env)).rejects.toMatchObject({ + code: 400, + serverCode: undefined, + }); + }); + + it('still throws, and still names the status, when a code IS found', async () => { + const { transport } = recorder(() => failedWithCode(400, 14019)); + await expect(api(transport).ensureSession(env)).rejects.toThrow(/session create failed.*400/); + }); + + it('never caches a session from a non-2xx, whatever the body says', async () => { + const { transport, calls } = recorder(() => failedWithCode(400, 14019)); + const a = api(transport); + await expect(a.ensureSession(env)).rejects.toThrow(); + await expect(a.ensureSession(env)).rejects.toThrow(); + expect(calls).toHaveLength(2); + }); +}); + describe('createBugseeApi — invalidateSession', () => { it('forces the next ensureSession to re-authenticate', async () => { const { transport, calls } = recorder(() => sessionOk()); diff --git a/packages/core/src/bugsee-api.ts b/packages/core/src/bugsee-api.ts index 262186f8..0e55e3e4 100644 --- a/packages/core/src/bugsee-api.ts +++ b/packages/core/src/bugsee-api.ts @@ -2,13 +2,15 @@ import type { EnvironmentEnvelope, RequestJson } from '@bugsee/protocol'; import type { AccessToken, IssueId, RecordingId } from '@bugsee/types'; import { randomId, strFromU8 } from '@bugsee/util'; import { BugseeError } from './errors'; -import type { BugseeApi, HttpTransport, IssueCreateResult } from './transport'; +import type { BugseeApi, HttpResponse, HttpTransport, IssueCreateResult } from './transport'; // The control-plane BugseeApi (design §7.5/§8.1/§8.2) — platform-agnostic logic over an injected // HttpTransport. Lazily creates a session (POST /v2/sessions), memoizes the access token, and // authenticates issue creation / renewal with it. Non-2xx responses throw a BugseeError carrying the -// status as `code` — the UploadPipeline catches the throw, calls invalidateSession() and retries (so -// a stale-token 401 is re-acquired). `app_token` travels in the X-App-Token header (§8.1 v3). The +// status as `code` — plus the collector's own `error.code` on `serverCode` when the failed body carried +// one, which is the only thing the UploadPipeline reads as a verdict about the payload. The +// UploadPipeline catches the throw, calls invalidateSession() and retries (so a stale-token 401 is +// re-acquired). `app_token` travels in the X-App-Token header (§8.1 v3). The // only platform-specific piece is the transport, supplied by the platform. export interface BugseeApiOptions { @@ -58,6 +60,56 @@ function unwrap(body: Uint8Array, what: string): unknown { return envelope.result; } +/** + * The collector's OWN `error.code` inside the body of a FAILED (non-2xx) response, or `undefined`. + * + * A `/v2/*` rejection usually arrives with HTTP 200 and the verdict in the envelope, which is what + * {@link unwrap} reads. But it does not always: the collector can answer a 4xx and put the SAME envelope + * in the body, and this client threw on the status before ever looking. The byte-identical body was + * therefore classified `permanent` on a 200 and retried at every launch forever on a 400 — including + * `99099 KillSdk`, which could not switch the SDK off if it arrived with a status attached. + * + * Android reads the body on both endpoints' failure paths and lets the collector's code win over the + * status: `ReportUploadExecutor.java:468-484` for `/v2/issues` (`serverErrorCode != 0` → + * `classifyServerErrorCode`, and only otherwise the status), and `CommunicationRequests.obtainSession` + * for `/v2/sessions` (a non-2xx with `errorCode != 0` → `createFailure(errorCode)`). + * + * Everything here fails towards `undefined`, and `undefined` means "the status is all we know" — which + * the pipeline retries. A non-JSON body (an nginx/WAF/captive-portal error page), a body with no error + * object, a non-numeric code: none of them is the collector speaking, so none of them may license + * deleting a crash report. `0` is ABSENT rather than a code, matching Android's `!= 0` guards — the + * value `optInt("code", 0)` cannot tell from a missing field. + */ +function serverErrorCodeOf(body: Uint8Array): number | undefined { + let decoded: unknown; + try { + decoded = decode(body); + } catch { + return undefined; // not JSON — an intermediary answered, not the collector + } + if (typeof decoded !== 'object' || decoded === null) { + return undefined; + } + const error = (decoded as V2Envelope).error; + if (typeof error !== 'object' || error === null) { + return undefined; + } + const code = error.code; + return typeof code === 'number' && code !== 0 ? code : undefined; +} + +/** The error for a non-2xx control-plane answer: the STATUS on `code`, and the collector's own code on + * `serverCode` when the body carried one. The two namespaces stay disjoint; only one call now + * populates both, and nothing reads either as the other. */ +function httpFailure(what: string, response: HttpResponse): BugseeError { + const serverCode = serverErrorCodeOf(response.body); + return new BugseeError( + `${what} failed (status ${response.status})`, + response.status, + serverCode !== undefined ? { serverCode } : undefined, + ); +} + export function createBugseeApi(transport: HttpTransport, options: BugseeApiOptions): BugseeApi { const { baseUrl, appToken, sdkVersion } = options; const sessionId = options.sessionId ?? randomId(); @@ -89,7 +141,7 @@ export function createBugseeApi(transport: HttpTransport, options: BugseeApiOpti body: JSON.stringify(body), }); if (!isOk(response.status)) { - throw new BugseeError(`issue create failed (status ${response.status})`, response.status); + throw httpFailure('issue create', response); } // The collector answers snake_case (`issue_id`/`recording_id`); the SDK's own shape is camelCase. const result = unwrap(response.body, 'issue create') as { @@ -117,7 +169,7 @@ export function createBugseeApi(transport: HttpTransport, options: BugseeApiOpti body: JSON.stringify({ app_token: appToken, environment, session_id: sessionId }), }); if (!isOk(response.status)) { - throw new BugseeError(`session create failed (status ${response.status})`, response.status); + throw httpFailure('session create', response); } const token = (unwrap(response.body, 'session create') as { access_token?: string } | null) ?.access_token; diff --git a/packages/core/src/capture-recovery.test.ts b/packages/core/src/capture-recovery.test.ts index d51e0766..fbcdd239 100644 --- a/packages/core/src/capture-recovery.test.ts +++ b/packages/core/src/capture-recovery.test.ts @@ -486,7 +486,7 @@ describe('recoverReports', () => { // A collector refusal is settled, exactly as the live durable pipeline treats it: rebuilding the same // bundle every launch would be a self-DoS, and the marker (plus the generation it pins) would never be - // freed — unbounded on browser/worker, where no retention sweep exists. + // freed — and on browser/worker nothing bounded that until `recoverSiblingBundleQueue` gained an age bound. it('retires a permanently-refused incident instead of rebuilding it forever', async () => { const storage = createInMemoryChunkStorage(); seedGen(storage, 100, [logRecord(1, { m: 'x' })]); @@ -680,7 +680,7 @@ describe('createMarkerAwareBundleReplay', () => { // A collector refusal is SETTLED — the durable pipeline frees the blob for it (a re-upload would just be // refused again, forever). The marker must go with it, or the incident is rebuilt on every later launch - // and its capture generation is pinned for good (unbounded on browser/worker: no retention sweep there). + // and its capture generation is pinned for good (on browser/worker, until the sibling leg's age bound, forever). it('retires the marker when the collector permanently refuses the staged bundle', async () => { const markers = fakeMarkers([marker('inc1', 100)]); const inner = innerPipeline({ ok: false, permanent: true }); diff --git a/packages/core/src/durable-upload-pipeline.test.ts b/packages/core/src/durable-upload-pipeline.test.ts index ae3ac564..ed3f7885 100644 --- a/packages/core/src/durable-upload-pipeline.test.ts +++ b/packages/core/src/durable-upload-pipeline.test.ts @@ -3,7 +3,9 @@ import { describe, expect, it, vi } from 'vitest'; import { type BundleStore, createDurableUploadPipeline, + DEFAULT_DURABLE_RETENTION, deserializeBundle, + deserializeBundleFrame, type IdentifiedBundle, serializeBundle, } from './durable-upload-pipeline'; @@ -54,6 +56,60 @@ function fakePipeline(result: UploadResult = { ok: true }) { return { pipeline: { enqueue, flush, drop } as UploadPipeline, enqueue, flush, drop }; } +describe('deserializeBundleFrame + DEFAULT_DURABLE_RETENTION', () => { + // Both exist for ONE reason: the browser/worker dead-sibling recovery leg reads a dead instance's blobs + // straight out of the shared store, so it never went through `recover()` and never saw the retention + // bounds `recover()` applies. To apply the SAME age bound it has to be able to read the frame's staging + // timestamp, and to use the SAME default rather than a second copy of the number. + + it('returns the staging timestamp alongside the bundle', () => { + const framed = serializeBundle({ ...bundle(), reportId: 'inc-1' }, 1_700_000_000_000); + expect(deserializeBundleFrame(framed)).toEqual({ + bundle: expect.objectContaining({ reportId: 'inc-1', fileName: 'abc.bundle.zip' }), + firstSeenMs: 1_700_000_000_000, + }); + }); + + it('reports firstSeenMs as UNDEFINED for a frame written before it existed', () => { + // The upgrade-launch case. A reader must be able to tell "staged at the epoch" from "we do not + // know", because treating unknown as the epoch expires every pending bundle on the launch after an + // upgrade — losing exactly the crash reports the upgrade was meant to deliver. + expect(deserializeBundleFrame(serializeBundle(bundle())).firstSeenMs).toBeUndefined(); + }); + + it('agrees with deserializeBundle about the bundle itself', () => { + const framed = serializeBundle(bundle({ fileName: 'x.zip' }), 5); + expect(deserializeBundleFrame(framed).bundle).toEqual(deserializeBundle(framed)); + }); + + it('throws on an unparseable frame, exactly as deserializeBundle does', () => { + expect(() => deserializeBundleFrame(new Uint8Array([9, 9, 9, 9, 9]))).toThrow(); + }); + + it('publishes the retention defaults the durable queue actually applies', () => { + expect(DEFAULT_DURABLE_RETENTION).toEqual({ + maxBundles: 32, + maxBytes: 64 * 1024 * 1024, + maxAgeMs: 7 * 24 * 60 * 60 * 1000, + }); + }); + + it('is the SAME object the queue defaults from, not a copy that can drift', async () => { + // Pinned behaviourally: a blob one millisecond past the published maxAgeMs must be evicted by a queue + // given no explicit retention. A second hand-written copy of "7 days" is exactly the kind of + // duplication that drifts, and the cost of drift here is a deleted crash report. + const { store, map } = memStore(); + const now = 10 * DEFAULT_DURABLE_RETENTION.maxAgeMs; + map.set('old', serializeBundle(bundle(), now - DEFAULT_DURABLE_RETENTION.maxAgeMs - 1)); + map.set('fresh', serializeBundle(bundle(), now - DEFAULT_DURABLE_RETENTION.maxAgeMs)); + const { pipeline, drop } = fakePipeline({ ok: false }); + createDurableUploadPipeline({ store, pipeline, now: () => now }).recover(); + await vi.waitFor(() => expect(map.has('old')).toBe(false)); + expect(map.has('fresh')).toBe(true); // exactly AT the bound is kept — the comparison is strict + expect(drop).toHaveBeenCalledWith('retention_expired', 'issue'); + }); +}); + describe('serializeBundle / deserializeBundle', () => { // The report id rides in the frame HEADER (local storage), never in `request` (the wire envelope). It is // what lets recovery reconcile a staged blob against a still-pending report marker for the SAME incident. diff --git a/packages/core/src/durable-upload-pipeline.ts b/packages/core/src/durable-upload-pipeline.ts index fa8981f3..14cc5743 100644 --- a/packages/core/src/durable-upload-pipeline.ts +++ b/packages/core/src/durable-upload-pipeline.ts @@ -156,7 +156,16 @@ export interface DurableQueueRetention { maxAgeMs?: number; } -const DEFAULT_RETENTION: Required = { +/** + * The bounds `recover()` applies when a caller supplies none. + * + * EXPORTED because the durable queue is not the only reader of a staged blob: the browser/worker + * dead-sibling leg (`browser-utils/recover-dead-instances.ts`) replays another instance's blobs straight + * out of the shared store, so it never passes through `recover()` and never saw these bounds at all. It + * applies `maxAgeMs` from HERE rather than from a second hand-written "7 days", because a second copy of + * a number that decides whether a crash report is deleted is a copy that will eventually drift. + */ +export const DEFAULT_DURABLE_RETENTION: Required = { maxBundles: 32, maxBytes: 64 * 1024 * 1024, maxAgeMs: 7 * 24 * 60 * 60 * 1000, @@ -186,11 +195,23 @@ export function serializeBundle(bundle: IdentifiedBundle, firstSeenMs?: number): } export function deserializeBundle(bytes: Uint8Array): IdentifiedBundle { - return readFrame(bytes).bundle; + return deserializeBundleFrame(bytes).bundle; } -/** Parse a durable frame into its bundle plus the staging metadata the retention policy needs. */ -function readFrame(bytes: Uint8Array): { bundle: IdentifiedBundle; firstSeenMs?: number } { +/** + * Parse a durable frame into its bundle plus the staging metadata the retention policy needs. + * + * EXPORTED for the browser/worker dead-sibling leg, which replays another instance's blobs straight out + * of the shared store and so needs the staging time without going through `recover()`. + * + * `firstSeenMs` is `undefined` for a frame written before the header carried it. A reader MUST NOT treat + * that as the epoch: doing so expires every pending bundle on the launch after an upgrade, which loses + * exactly the crash reports the upgrade was installed to deliver. + */ +export function deserializeBundleFrame(bytes: Uint8Array): { + bundle: IdentifiedBundle; + firstSeenMs?: number; +} { const headerLength = new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength).getUint32( 0, true, @@ -242,7 +263,7 @@ export function createDurableUploadPipeline( const now = options.now ?? (() => Date.now()); const stagedWaitMs = options.stagedWaitMs ?? 5_000; const sleep = options.sleep ?? defaultSleep; - const retention = { ...DEFAULT_RETENTION, ...options.retention }; + const retention = { ...DEFAULT_DURABLE_RETENTION, ...options.retention }; let counter = 0; const newId = options.newId ?? @@ -344,7 +365,7 @@ export function createDurableUploadPipeline( if (bytes === undefined) continue; // removed between list() and read() let bundle: Bundle; try { - bundle = readFrame(bytes).bundle; + bundle = deserializeBundleFrame(bytes).bundle; } catch (error) { onError(error); removeSafe(id); // unparseable leftover — purge so it can't wedge the pump forever @@ -385,7 +406,7 @@ export function createDurableUploadPipeline( } let frame: { bundle: IdentifiedBundle; firstSeenMs?: number }; try { - frame = readFrame(bytes); + frame = deserializeBundleFrame(bytes); } catch (error) { onError(error); removeSafe(id); // unparseable leftover — purge so it can't wedge recovery forever diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index de1669dd..b3a6e297 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -3,9 +3,12 @@ // // TWO DISJOINT NUMERIC NAMESPACES, and conflating them is a data-loss bug: `code` is the HTTP STATUS // (`0` when none was reached), while `serverCode` is the COLLECTOR's own code from a `{ ok: false, -// error: { code } }` envelope — the mobile-parity codes such as 12003/12004 (§7.7) and 14019. The -// collector answers HTTP 200 with an error envelope, so the two never arrive together and a collector -// code must never be read as a status. +// error: { code } }` envelope — the mobile-parity codes such as 12003/12004 (§7.7) and 14019. +// +// They CAN arrive together. The usual rejection is an error envelope on an HTTP 200, so `code` is 0 and +// only `serverCode` is set — but the collector can also attach a status to the same envelope, and then +// both are populated (`bugsee-api.ts`, `httpFailure`). What must never happen is either being read as the +// other: only `serverCode` is a verdict about the payload, and a status is never one. export interface BugseeErrorOptions { /** The underlying error this one wraps, surfaced as the standard `Error.cause`. */ @@ -22,11 +25,11 @@ export interface BugseeErrorOptions { /** * The COLLECTOR's own error code, from a `/v2/*` `{ ok: false, error: { code } }` envelope. * - * A SEPARATE numeric namespace from `code`, and it has to be: a v2 rejection arrives with **HTTP - * 200**, so there is no status to report, and the collector's codes overlap HTTP statuses by pure - * accident (a collector code of `403` is not an auth failure). Carrying one in `code` meant the - * upload pipeline read it as a status — retrying Android's permanent codes forever, and disabling the - * SDK on a transient rejection that merely happened to be numbered 401 or 403. + * A SEPARATE numeric namespace from `code`, and it has to be: a v2 rejection usually arrives with + * **HTTP 200**, so there is often no status to report at all, and the collector's codes overlap HTTP + * statuses by pure accident (a collector code of `403` is not an auth failure). Carrying one in `code` + * meant the upload pipeline read it as a status — retrying Android's permanent codes forever, and + * disabling the SDK on a transient rejection that merely happened to be numbered 401 or 403. * * Read it through {@link classifyServerErrorCode}, never by comparing it to a status. */ diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 08947029..f7a6cb6f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -114,9 +114,12 @@ export { type BundleStore, BundleStoreToken, createDurableUploadPipeline, + DEFAULT_DURABLE_RETENTION, + type DurableQueueRetention, type DurableUploadPipeline, type DurableUploadPipelineOptions, deserializeBundle, + deserializeBundleFrame, type IdentifiedBundle, serializeBundle, } from './durable-upload-pipeline'; diff --git a/packages/core/src/transport.ts b/packages/core/src/transport.ts index b29e0949..32937572 100644 --- a/packages/core/src/transport.ts +++ b/packages/core/src/transport.ts @@ -167,7 +167,10 @@ export interface UploadResult { * ONE definition, because three separately-written copies of it drifted: the live durable pipeline treated * `permanent` as settled while both recovery legs gated on `ok` alone, so a 4xx-refused bundle recovered * from a dead instance was re-uploaded on every launch forever — bounded at 7 days on node by the instance - * sweep, unbounded on browser/worker, which has no retention pass at all. + * sweep, and on browser/worker not bounded at all. That second half is now closed separately, by the age + * bound `recoverSiblingBundleQueue` applies from `DEFAULT_DURABLE_RETENTION`: a verdict settles a bundle, + * and a bound gives up on one, and the two must stay different things. Widening what counts as a verdict + * to make up for a missing bound is what produced a new loss path in three consecutive review rounds. * * Android parity: `CommunicationErrorClassifier.java:14-33` + `ReportUploadExecutor.java:258-268`. */ diff --git a/packages/core/src/upload-pipeline.test.ts b/packages/core/src/upload-pipeline.test.ts index 0a25c01d..373a6966 100644 --- a/packages/core/src/upload-pipeline.test.ts +++ b/packages/core/src/upload-pipeline.test.ts @@ -1,7 +1,8 @@ import type { EnvironmentEnvelope, RequestJson } from '@bugsee/protocol'; import type { AccessToken, IssueId, RecordingId } from '@bugsee/types'; -import { createDeferred } from '@bugsee/util'; +import { createDeferred, strToU8 } from '@bugsee/util'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { createBugseeApi } from './bugsee-api'; import { BugseeError } from './errors'; import type { BugseeApi, Bundle, BundleUploader, IssueCreateResult, PutResult } from './transport'; import { @@ -614,3 +615,127 @@ describe('createUploadPipeline — diagnosability', () => { expect(result.error?.cause).toBe(boom); }); }); + +// ══════════════════════════════════════════════════════════════════════════════════════════════════ +// INTEGRATION — the REAL BugseeApi over a fake transport, so the whole control-plane verdict path runs +// ══════════════════════════════════════════════════════════════════════════════════════════════════ +// +// Every test above injects a `BugseeApi` double that throws a ready-made `BugseeError`, so the step that +// actually DECIDES the verdict — reading the collector's `error.code` off the wire — was never covered +// end to end. That is precisely where the gap lived: `bugsee-api.ts` threw on a non-2xx status before +// looking at the body, so a `14019 InvalidAppToken` delivered with an HTTP 400 reached this pipeline with +// no `serverCode`, was classified transient, and was re-uploaded at every launch for the life of the +// installation — while the byte-identical body on an HTTP 200 was classified permanent and freed. +describe('createUploadPipeline + createBugseeApi — the control plane end to end', () => { + const enc = (json: unknown): Uint8Array => strToU8(JSON.stringify(json)); + const sessionOk = { + status: 200, + headers: {}, + body: enc({ ok: true, result: { access_token: 't' } }), + }; + + /** + * Wire the REAL api to a transport that answers `on` with `status` + `body`, and count the calls. + * The signed PUT always succeeds, so anything that fails here failed on the control plane. + */ + const wired = (on: '/v2/sessions' | '/v2/issues', status: number, body: unknown) => { + const calls = { session: 0, issue: 0 }; + const transport = async (url: string) => { + const which = url.endsWith('/v2/sessions') ? 'session' : 'issue'; + calls[which] += 1; + if (url.endsWith(on)) { + return { status, headers: {}, body: enc(body) }; + } + return which === 'session' + ? sessionOk + : { + status: 200, + headers: {}, + body: enc({ + ok: true, + result: { endpoint: 'https://put/1', issue_id: 'i1', recording_id: 'r1' }, + }), + }; + }; + const api = createBugseeApi(transport, { + baseUrl: 'https://api.test', + appToken: 'tok', + sdkVersion: '0', + }); + return { api, calls }; + }; + const envelope = (code: number, type: string) => ({ + ok: false, + error: { type, message: type, code }, + }); + const run = (api: BugseeApi) => + createUploadPipeline(deps({ api, maxRetries: 2 })).enqueue(bundle); + + it.each([ + ['/v2/issues' as const, 400], + ['/v2/issues' as const, 404], + ['/v2/sessions' as const, 400], + ['/v2/sessions' as const, 403], + ])('drops the bundle on a permanent collector code carried by a %s HTTP %i', async (on, status) => { + const { api, calls } = wired(on, status, envelope(14019, 'InvalidAppTokenError')); + const result = await run(api); + expect(result.permanent).toBe(true); // the durable queue frees it — no more launch-forever loop + expect(result.error?.serverCode).toBe(14019); + expect(result.error?.code).toBe(status); // the status is still reported, just not as the verdict + expect(result.error?.fatal).toBe(false); // a bad token drops a payload; it does not stop the SDK + expect(calls[on === '/v2/sessions' ? 'session' : 'issue']).toBe(1); // not retried + }); + + it('switches the SDK off when KILL_SDK (99099) arrives WITH a status, not only inside a 200', async () => { + const { api } = wired('/v2/sessions', 400, envelope(99099, 'KillSdkError')); + const result = await run(api); + expect(result.error?.fatal).toBe(true); + expect(result.permanent).toBe(true); + }); + + it.each([ + [503, 99013, 'ServerTooBusy — the collector is shedding load, not refusing the payload'], + [400, 14002, 'SessionNotFound — the session is stale, the payload is fine'], + [400, 123_456, 'a code this SDK has never heard of'], + ])('KEEPS the bundle for a %i carrying %i (%s)', async (status, code) => { + const { api, calls } = wired('/v2/sessions', status, envelope(code, 'X')); + const result = await run(api); + expect(result.permanent).toBeUndefined(); + expect(calls.session).toBe(3); // initial + 2 retries, then kept for the next launch + }); + + // ── THE DECISION, pinned. ──────────────────────────────────────────────────────────────────────── + // + // Android's issue-create path DOES fall back to the HTTP status when the body carries no code, and + // `classifyHttpStatus` calls any non-401/408/425/429 4xx PERMANENT — which would delete the report. + // This SDK deliberately does NOT follow it there, and this test is the fence: + // + // • Android's OWN session path disagrees with its issue path about the identical status: an + // `/v2/sessions` non-2xx with no body code becomes `UNKNOWN_ERROR` → `classifyServerErrorCode(0)` + // → TRANSIENT → retry (`CommunicationRequests.obtainSession`). Two adjacent calls to the same + // collector cannot both be right, which is the tell that the STATUS is not the verdict — the + // collector's code is. + // • A bare 4xx on the control plane is the answer an intermediary gives: a captive portal, a + // corporate MITM proxy, a WAF, a stale CDN route, a service worker. None of them read the payload. + // • Widening a deletion path on a status is what produced a NEW data-loss path in each of three + // consecutive review rounds here (R3-1: `status >= 500` deleted every 401/408/425/429). + // + // The cost of NOT classifying was "retried forever". That cost is now paid for separately, by BOUNDS + // rather than by verdicts: node sweeps dead subtrees at 7 days, the durable queue's own retention caps + // at 32 bundles / 64 MiB / 7 days, and `recoverSiblingBundleQueue` now applies that same age bound on + // browser and worker, which is where "forever" was literally true. + it.each([ + ['/v2/issues' as const, 400], + ['/v2/issues' as const, 403], + ['/v2/issues' as const, 404], + ['/v2/issues' as const, 422], + ['/v2/sessions' as const, 400], + ['/v2/sessions' as const, 404], + ])('KEEPS the bundle for a NAKED %s HTTP %i — a status is never a verdict here', async (on, status) => { + const { api } = wired(on, status, { message: 'Bad Request' }); + const result = await run(api); + expect(result.permanent).toBeUndefined(); + expect(result.error?.fatal).toBe(false); + expect(result.error?.serverCode).toBeUndefined(); + }); +}); diff --git a/packages/instrumentation-tests/harness/README.md b/packages/instrumentation-tests/harness/README.md index fbe57199..3e813320 100644 --- a/packages/instrumentation-tests/harness/README.md +++ b/packages/instrumentation-tests/harness/README.md @@ -6,11 +6,11 @@ blocking defects fixed in round 6 (R5-1 live-path marker loss, R5-2 kill-state o collector-code namespace collision). Run it from the repo root: `pnpm exec tsx packages/instrumentation-tests/harness/invariants.mts` -(~30 s, 350 cases). It resolves the packages it exercises from its OWN location, so it can be run +(~30 s, 369 cases). It resolves the packages it exercises from its OWN location, so it can be run from a git worktree and will validate THAT tree — but only if the worktree's dependencies are its own. A worktree whose `node_modules` are symlinked to the main checkout resolves `@bugsee/*` back to the main tree, and the harness then certifies code that is not the code under test. -Expected output: `350 cases swept, 0 invariant violations`. +Expected output: `369 cases swept, 0 invariant violations`. ## Why it lives here and not in `test/` @@ -18,7 +18,8 @@ It is not a vitest suite — it is a single long-running sweep with its own orac vitest configs glob `test/**`, so it is deliberately not collected. **It is not yet in CI.** Wiring it in is a known follow-up; until then it must be run by hand before changing anything under `core/src/{client,upload-pipeline,durable-upload-pipeline,capture-recovery,transport}.ts`, -`node/src/recover-instances.ts` or `browser-utils/src/coexistence.ts`. +`node/src/recover-instances.ts`, `core/src/bugsee-api.ts`, `browser-utils/src/coexistence.ts` or +`browser-utils/src/recover-dead-instances.ts`. ## The one rule that must not be broken @@ -39,3 +40,14 @@ bytes the SDK threw away, or the SDK re-asking about bytes finally refused) rath because both sides made the same mistake. Independence of provenance is not enough. It has to be independence of outcome. + +## The one licensed exception to P2 + +P2 says nothing may be deleted that the collector has not first taken or refused. **Retention is the +single exception**, because giving up on a blob is by definition deleting one the collector never +answered about. Set N (the browser/worker age bound) therefore carries its own explicit assertions +instead of being routed through `judge` — weakening P2 for everyone to accommodate it is how a real +loss would hide. Every other artifact in those cases is still judged normally, and the `no timestamp` +control in that set is the safety rail: an unknown staging time must NEVER expire, or the launch after +an SDK upgrade deletes every pending crash report. Injecting `firstSeenMs ?? 0` there is caught by the +pre-existing P2 in set H, 23 times over. diff --git a/packages/instrumentation-tests/harness/invariants.mts b/packages/instrumentation-tests/harness/invariants.mts index e2f1c561..d46996e8 100644 --- a/packages/instrumentation-tests/harness/invariants.mts +++ b/packages/instrumentation-tests/harness/invariants.mts @@ -1536,6 +1536,54 @@ const collectorCase = (on: 'session' | 'issue', code: number, type: string): Con }; }; +/** + * The SAME collector envelope, delivered with a NON-2xx status instead of the usual 200. + * + * The gap this closes. `bugsee-api.ts` threw on the status BEFORE reading the body, so a verdict the SDK + * honours perfectly on an HTTP 200 was thrown away the moment the collector attached a status to it: + * `14019 InvalidAppToken` on a 400 was retried at every launch for the life of the installation, and + * `99099 KillSdk` on a 400 could not switch the SDK off at all. Android reads the body on BOTH endpoints' + * failure paths — `ReportUploadExecutor.java:468-484` for `/v2/issues`, `CommunicationRequests.obtainSession` + * for `/v2/sessions` — and lets the collector's code win over the status. + * + * The expected outcome is DERIVED from the same Android-canonical table as {@link collectorCase}, so this + * adds no second opinion about which codes are permanent: it asserts only that the STATUS does not change + * the answer. + */ +const collectorCaseOverHttp = ( + on: 'session' | 'issue', + status: number, + code: number, + type: string, +): ControlCase => ({ + ...collectorCase(on, code, type), + label: `${on} HTTP ${status} carrying code ${code} ${type}`, + answer: { status, body: { ok: false, error: { type, message: type, code } } }, +}); + +/** + * A NAKED non-2xx on the control plane — a status with no collector code behind it. + * + * These are declared TRANSIENT, and that is the decision this harness exists to keep honest rather than + * an observation about HTTP. A bare 4xx on the control plane is the answer an INTERMEDIARY gives — a + * captive portal, a corporate MITM proxy, a WAF, a stale CDN route, a service worker — none of which read + * the payload, and all of which go away. Android's own two endpoints disagree about the identical status + * (`/v2/issues` falls back to `classifyHttpStatus` → PERMANENT; `/v2/sessions` falls back to + * `classifyServerErrorCode(0)` → TRANSIENT), which is the tell that a status is not a verdict about the + * bytes. So the SDK keeps the report, and these cases assert that it is still there to deliver once the + * interposer is gone — P4 fires if a future change starts deleting on a status. + * + * "Retried forever" is answered by BOUNDS instead: node's 7-day dead-subtree sweep, the durable queue's + * own retention, and — new with these cases — the same age bound on the browser/worker sibling leg (set N). + */ +const bareStatusCase = (on: 'session' | 'issue', status: number): ControlCase => ({ + label: `${on} HTTP ${status} with NO collector code (an intermediary answered)`, + on, + answer: { status, body: { message: 'Bad Request' } }, + intent: 'transient', + staysAlive: true, +}); + const CONTROL_CASES: ControlCase[] = [ // ── R5-2: an HTTP auth status on the control plane. Android treats 401 as session expiry and // retries once (`BugseeCommunicationManager.java:614-635`); the token blacklist fires ONLY on @@ -1590,6 +1638,27 @@ const CONTROL_CASES: ControlCase[] = [ // The same namespace on the ISSUE call. collectorCase('issue', 12003, 'SimilarCrashExistsError'), collectorCase('issue', 99013, 'ServerTooBusyError'), + // ── The same codes with an HTTP STATUS attached. The status must not change the verdict, in + // EITHER direction: a permanent code stays permanent, and ServerTooBusy stays retryable even + // though it arrived on a 503 that a status-first reading would also have called retryable — + // and on a 400 that a status-first reading would have DELETED. + collectorCaseOverHttp('issue', 400, 14019, 'InvalidAppTokenError'), + collectorCaseOverHttp('issue', 403, 11004, 'ApplicationTypeMismatchError'), + collectorCaseOverHttp('issue', 404, 99098, 'UnsupportedSdkError'), + collectorCaseOverHttp('issue', 422, 12004, 'TooManySimilarCrashesError'), + collectorCaseOverHttp('issue', 503, 99013, 'ServerTooBusyError'), + collectorCaseOverHttp('issue', 400, 99013, 'ServerTooBusyError'), + collectorCaseOverHttp('session', 400, 14019, 'InvalidAppTokenError'), + collectorCaseOverHttp('session', 401, 14002, 'SessionNotFoundError'), + collectorCaseOverHttp('session', 400, 99099, 'KillSdkError'), + collectorCaseOverHttp('session', 500, 99013, 'ServerTooBusyError'), + // ── …and the naked statuses, which stay retryable BY DECISION (see bareStatusCase). + bareStatusCase('issue', 400), + bareStatusCase('issue', 403), + bareStatusCase('issue', 404), + bareStatusCase('issue', 422), + bareStatusCase('session', 400), + bareStatusCase('session', 404), ]; for (const control of CONTROL_CASES) { @@ -1705,6 +1774,120 @@ for (const control of CONTROL_CASES) { rmSync(dir, { recursive: true, force: true }); } +// ══ N. THE BROWSER/WORKER AGE BOUND — the other half of "retried at every launch, forever" ═════════ +// +// Set M pins what the SDK does when the collector gives a FINAL answer. This set pins what happens when +// it never gives one at all — the case the control-plane decision deliberately leaves retryable (a bare +// 4xx from an intermediary, an offline collector, a 5xx). On node such a blob is bounded twice over: +// `sweep-instances` reaps a dead instance's whole subtree at 7 days, and `recover()` applies the durable +// queue's retention. The browser/worker dead-sibling leg reads the dead instance's prefix DIRECTLY, so it +// met neither — and on the web an instance is dead the moment its tab closes, so the blob was re-offered +// on every launch for the lifetime of the installation. That is what `recoverSiblingBundleQueue`'s age +// bound closes, using the SAME `DEFAULT_DURABLE_RETENTION.maxAgeMs` the same bytes already meet on the +// same tier when the instance recovers its OWN queue. +// +// WHY THIS SET IS NOT JUDGED BY `judge`. Retention is the one licensed exception to P2 ("nothing is +// deleted that was not first delivered or refused"): giving up on a blob is, by definition, deleting one +// the collector never answered about. Routing it through `judge` would either report a violation the +// design intends or force P2 to be weakened for everyone, and a weakened P2 is how real losses hide. So +// the bound gets its own explicit assertions, and every OTHER blob in the case is still judged normally. +// +// The `firstSeenMs` dimension is the whole safety argument. A frame written before the header carried one +// reads as UNKNOWN, and unknown must NEVER expire — treating it as the epoch would delete every pending +// crash report on the first launch after an SDK upgrade, which is the exact loss the upgrade was +// installed to prevent. The `legacy` case below is that control, and it must be DELIVERED. + +const AGE_DAY = 24 * 60 * 60 * 1000; +const AGE_NOW = Date.now(); + +/** A durable frame carrying an explicit staging time (or none at all, for the legacy control). */ +const agedFrame = (sub: string, key: string, incident: string, firstSeenMs?: number) => + core.serializeBundle( + { + request: { + type: 'crash', + summary: blobSummary(sub, key), + severity: 3, + source: { type: 'crash', mechanism: 'uncaught' }, + created_on: '2026-05-29T00:00:00Z', + environment: env, + }, + body: new Uint8Array([1, 2, 3]), + fileName: 'p.zip', + reportId: incident, + }, + firstSeenMs, + ); + +for (const [label, staleFirstSeenMs, mustGiveUp] of [ + ['90 days old', AGE_NOW - 90 * AGE_DAY, true], + ['one day old', AGE_NOW - AGE_DAY, false], + ['no timestamp at all (staged by an older SDK)', undefined, false], +] as Array<[string, number | undefined, boolean]>) { + cases += 1; + const idb = new fidb.IDBFactory(); + const locks = fakeLocks(); + const sub = 'deadN'; + const bundles = bu.createIdbBlobStore({ + databaseName: bu.coexistenceDatabaseName(TOK), + indexedDB: idb, + }); + // The blob under test, plus a FRESH sibling that must be unaffected by whatever happens to it. + await bundles.put(`${sub}/stale`, agedFrame(sub, 'stale', 'STALE', staleFirstSeenMs)); + await bundles.put(`${sub}/fresh`, agedFrame(sub, 'fresh', 'FRESH', AGE_NOW - AGE_DAY)); + + // The collector is simply unreachable for the first two launches and then comes back — a TRANSIENT + // condition throughout, so nothing here is ever refused. That is the point: the only thing that may + // stop a blob being offered is the age bound. + let launch = 1; + const c = collector(() => (launch <= CLEARS_AFTER ? 503 : 200)); + const perLaunch: Answered[][] = []; + for (; launch <= LAUNCHES; launch += 1) { + const mark = c.puts.length; + await bu + .createCoexistence({ + appToken: TOK, + persist: true, + locks: locks.manager, + indexedDB: idb, + onError: () => {}, + }) + .recoverDeadSiblings({ uploadPipeline: realPipeline(c) }); + await settle(); + perLaunch.push(c.puts.slice(mark)); + } + + const name = `N browser age bound — a stale blob ${label}`; + const offeredStale = c.puts.filter((p) => p.summary === blobSummary(sub, 'stale')); + const left = await browserLeft(idb); + if (mustGiveUp) { + // GIVEN UP: never offered at all, and gone from the store — the self-DoS is over. + if (offeredStale.length > 0) { + failures.push(`${name} :: it was still OFFERED ${offeredStale.length}x past the age bound`); + } + if (left.has(`${sub}/stale`)) { + failures.push(`${name} :: it was withheld from the collector but LEFT ON DISK forever`); + } + } else { + // KEPT: offered, and delivered once the collector came back. A bound that eats these is a loss. + if (!offeredStale.some((p) => p.verdict === 'accept')) { + failures.push(`${name} :: it was NEVER DELIVERED, though the collector came back for it`); + } + if (left.has(`${sub}/stale`)) { + failures.push(`${name} :: it was delivered and then LEFT ON DISK`); + } + } + // …and the fresh sibling is judged exactly as any other blob: the bound must not touch it. + judgeCrossLaunch({ + label: `${name} (its fresh neighbour)`, + staged: [{ slot: `${sub}/fresh`, incident: 'FRESH', ownSummary: blobSummary(sub, 'fresh') }], + incidentOf: (summary: string) => (summary === blobSummary(sub, 'fresh') ? 'FRESH' : undefined), + perLaunch, + left, + mustDeliver: true, + }); +} + // ══════════════════════════════════════════════════════════════════════════════════════════════════ console.log(`\n${cases} cases swept, ${failures.length} invariant violations`);