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`);