diff --git a/packages/core/src/handlers/account-backup.js b/packages/core/src/handlers/account-backup.js index 68050b1..e07fec3 100644 --- a/packages/core/src/handlers/account-backup.js +++ b/packages/core/src/handlers/account-backup.js @@ -5,9 +5,6 @@ // content-addressed, so a run only uploads what the target does not already // hold. The manifest is written last: a snapshot without one is incomplete and // never counts for restore or blob retention. -// -// Not in the package export map. pds.js builds its route table from fragments -// like this one; nothing outside core imports it. import { BACKUP_CHECKPOINT_MS, @@ -21,9 +18,8 @@ import { import { base64UrlDecode } from '../crypto.js'; /** - * Everything this cluster needs from the server around it. Structural types, so - * an annotation does not have to name whichever module currently owns the - * function being passed in. + * Structural types, so no member has to name whichever module owns the function + * passed in. * * @typedef {Object} BackupContext * @property {import('../ports.js').ActorStoragePort} actorStorage @@ -59,8 +55,8 @@ export function createBackupHandlers(ctx) { accountApi, } = ctx; - // In-process guard against two runs in one isolate. The durable claim is the - // heartbeat on the persisted run, which is what survives a restart. + // Only guards this isolate. The claim that survives a restart is the heartbeat + // on the persisted run. let backupRunning = false; /** diff --git a/packages/core/src/handlers/xrpc-sync.js b/packages/core/src/handlers/xrpc-sync.js index 7f107ba..d7877c6 100644 --- a/packages/core/src/handlers/xrpc-sync.js +++ b/packages/core/src/handlers/xrpc-sync.js @@ -1,15 +1,10 @@ // @pdsjs/core/handlers/xrpc-sync - com.atproto.sync.* repo reads. // -// The relay's view of this server: what repos exist, where each one's head is, -// and the blocks to prove it. Every answer is scoped to the single hosted -// account, so a `did` parameter naming anyone else is a RepoNotFound rather -// than a lookup. +// One account is hosted here, so a `did` parameter naming anyone else is a +// RepoNotFound rather than a lookup. // -// getBlob and listBlobs are also com.atproto.sync.*, but they read blob storage -// rather than the repo and live with the other blob endpoints. -// -// Not in the package export map. pds.js builds its route table from fragments -// like this one; nothing outside core imports it. +// getBlob and listBlobs share the namespace but read blob storage rather than +// the repo, and live with the other blob endpoints. import { collectMstBlocks } from '../mst.js'; import { buildCarFile, cborDecode, cidToString } from '../repo.js'; diff --git a/packages/core/src/pds.js b/packages/core/src/pds.js index 9217df5..8e2d0e6 100644 --- a/packages/core/src/pds.js +++ b/packages/core/src/pds.js @@ -831,10 +831,8 @@ export class PersonalDataServer { isDeactivated: () => this.isDeactivated(), }); - // The snapshot engine, built once per server so its in-process run lock is - // per-instance. It takes the repo CAR builder from the sync handlers, so - // those are constructed first; accountApi binds off the class, where the - // rest of the account API lives. + // Built once per server, so the engine's run lock is per-instance. Depends + // on the sync handlers for the repo CAR builder, hence the order. this._backup = createBackupHandlers({ actorStorage: this.actorStorage, blobs: this.blobs, @@ -7710,18 +7708,12 @@ export class PersonalDataServer { ); } - // ════════════════════════════════════════════════════════════════════════════ - // XRPC Handlers - Sync - // ════════════════════════════════════════════════════════════════════════════ - // - // com.atproto.sync.* repo reads live in handlers/xrpc-sync.js, wired up in - // the constructor. getBlob and listBlobs are in the same namespace but read - // blob storage rather than the repo, so they stay with the blob endpoints - // below. - // ════════════════════════════════════════════════════════════════════════════ // XRPC Handlers - Blobs // ════════════════════════════════════════════════════════════════════════════ + // + // getBlob and listBlobs are com.atproto.sync.*, but they read blob storage + // rather than the repo, so they sit here and not in handlers/xrpc-sync.js. /** * com.atproto.repo.uploadBlob @@ -9259,10 +9251,9 @@ export class PersonalDataServer { // ── Backups ─────────────────────────────────────────────────────────── // - // The snapshot engine and its /account/api/backups routes live in - // handlers/account-backup.js, wired up in the constructor. The scheduler is - // reachable here because all three platform packages call it from their - // hourly tick. + // The engine and its /account/api/backups routes are in + // handlers/account-backup.js. The scheduler is reachable here because all + // three platform packages call it from their hourly tick. /** * Run a backup if one is enabled and due. diff --git a/test/backup.test.js b/test/backup.test.js index 8c7dc78..7d7c6e7 100644 --- a/test/backup.test.js +++ b/test/backup.test.js @@ -460,16 +460,11 @@ describe('availability', () => { await expect(pds._backup.runBackup('manual')).rejects.toThrow(/target/); }); - // The three platform packages call the scheduler through the class, not the - // handler module, so the delegation is part of the contract. it('exposes the scheduler on the server for the platform tick', async () => { const { pds } = await createBackupPds(); expect(await pds.maybeRunScheduledBackup()).toBeNull(); }); - // The routes come from the handler module and are merged into the table in - // the constructor, so nothing else in the suite would notice if that merge - // stopped happening. it('merges its routes into the server table', async () => { const { pds } = await createBackupPds(); for (const [path, method] of [ @@ -484,8 +479,6 @@ describe('availability', () => { } }); - // Space routes are merged from the same table; a regression in the merge - // order would silently drop one set or the other. it('keeps space routes alongside them', async () => { const { actorStorage, blobs, target } = await createBackupPds(); const pds = new PersonalDataServer({ diff --git a/test/xrpc-sync.test.js b/test/xrpc-sync.test.js index a7d6c05..e250046 100644 --- a/test/xrpc-sync.test.js +++ b/test/xrpc-sync.test.js @@ -1,9 +1,4 @@ -// com.atproto.sync.* handler tests - repo reads, CAR shape, deactivated status -// -// These drive handlers/xrpc-sync.js through its context object, with no -// PersonalDataServer anywhere: the cluster needs only actorStorage, getDid and -// isDeactivated, so a plain object is the whole dependency surface. That is what -// makes the error paths reachable here rather than only against a live server. +// Sync handler tests - repo reads, CAR shape, proof paths, deactivated status import { describe, expect, it } from 'vitest'; import { parseCarFile } from '../packages/core/src/car.js'; import { generateKeyPair } from '../packages/core/src/crypto.js'; @@ -117,9 +112,8 @@ function req(path) { async function call(sync, path) { const route = sync.routes[path.split('?')[0]]; const { request, url } = req(path); - // The Route typedef declares `this: PersonalDataServer`, for the handlers that - // are methods on it. These take their dependencies from the factory's context - // and never touch `this`. + // The Route typedef declares `this: PersonalDataServer`; these handlers take + // their dependencies from the factory context and never touch `this`. const handler = /** @type {(request: Request, url: URL, auth: null) => Promise} */ ( route.handler @@ -223,12 +217,9 @@ describe('getRepo', () => { expect(response.status).toBe(404); }); - // Documents a bug rather than endorsing it. buildFullRepoCar throws - // "Missing block" for an unreachable block, but the recursive descent sits - // inside a try whose catch swallows any error, so only a missing *root* - // surfaces. A missing MST node or record is dropped and the endpoint answers - // 200 with a truncated CAR, which is the worst outcome for a relay: it - // ingests an incomplete repo and nothing reports a failure. + // A bug, asserted as it stands: buildFullRepoCar's recursive descent sits + // inside a try whose catch swallows its "Missing block" throw, so a missing + // MST node or record is dropped and a relay ingests an incomplete repo. it('serves a truncated CAR when a descendant block is missing', async () => { const { sync, actorStorage } = await createSync(); const commitCid = actorStorage.commits[0].cid; @@ -323,8 +314,7 @@ describe('buildFullRepoCar', () => { expect(parsed.blocks.size).toBeGreaterThanOrEqual(3); }); - // The root is the one position where the missing-block throw is not swallowed - // by the descent's catch. See the truncated-CAR case above. + // The root is the one position the descent's catch does not swallow. it('throws naming the block when the root is unreachable', async () => { const { sync } = await createSync(); await expect(sync.buildFullRepoCar('bafyreimissingblock')).rejects.toThrow( diff --git a/vitest.config.js b/vitest.config.js index 146b69d..8161a7f 100644 --- a/vitest.config.js +++ b/vitest.config.js @@ -10,16 +10,13 @@ export default defineConfig({ reporter: ['text', 'html'], include: ['packages/*/src/**/*.js'], - // Platform entrypoints the v8 provider cannot see. The e2e suite does - // exercise them, but through a runtime this process does not instrument: - // `wrangler dev` and `deno` are spawned as subprocesses, and the readonly - // CLI is invoked as one. blobs-s3's adapter needs a real S3. Left in, they - // report 0% no matter how many tests exist and there is no threshold that - // means anything on them. + // The e2e suite exercises these, but through a runtime this process cannot + // instrument: `wrangler dev`, `deno` and the readonly CLI are spawned as + // subprocesses, and blobs-s3 needs a real S3. Included, they report 0% no + // matter how many tests cover them. // - // PLATFORM=node is the exception: that server runs in-process, so - // `PLATFORM=node vitest run --coverage` does attribute e2e coverage to - // core, node, blobs-fs and storage-sqlite. + // PLATFORM=node is the exception, running in-process, so + // `PLATFORM=node vitest run --coverage` does attribute e2e coverage. exclude: [ ...coverageConfigDefaults.exclude, 'packages/deno/src/**', @@ -32,16 +29,11 @@ export default defineConfig({ // Without this, a single failing test suppresses the whole report. reportOnFailure: true, - // Floors, not targets, and deliberately per-area rather than global. A - // global number here would average two unrelated populations: modules the - // unit suite covers directly, and request handlers whose coverage only - // arrives with the e2e suite that `npm run ci` does not run. It would also - // reward padding the thinnest files instead of holding the line on the - // ones that matter. - // - // Everything below is unit-covered, so these hold under test:coverage - // with no e2e run. Each handler module under core/src/handlers gets its - // own entry. + // Floors, per-area rather than global. A global number would average two + // unrelated populations: modules the unit suite covers directly, and + // handlers whose coverage only arrives with the e2e suite `npm run ci` + // does not run. Everything named below is unit-covered, so these hold + // without one. thresholds: { 'packages/core/src/{crypto,auth,plc,repo,scope,verify,webauthn,backup}.js': { @@ -49,9 +41,9 @@ export default defineConfig({ branches: 78, functions: 95, }, - // Per file rather than a handlers/*.js glob: an aggregate would let a - // well-covered cluster carry a bare one, which is what the two spaces - // entries below exist to avoid. + // A glob key aggregates every file it matches, letting a well-covered + // module carry a bare one, so each gets its own entry. The two spaces + // entries below are split for the same reason. 'packages/core/src/handlers/account-backup.js': { statements: 70, branches: 52,