diff --git a/apps/backend/docs/concepts-gc-jobs.md b/apps/backend/docs/concepts-gc-jobs.md new file mode 100644 index 0000000..75237b1 --- /dev/null +++ b/apps/backend/docs/concepts-gc-jobs.md @@ -0,0 +1,249 @@ +# Background GC Jobs Reference + +This document is the operational reference for every scheduled cleanup pass the backend +runs: what each one deletes, how often, how long it waits before deleting it, and how to +retune that via environment variables. For the broader file-lifecycle and push-hygiene +picture these jobs sit inside, see +[`concepts-storage-push-jobs.md`](./concepts-storage-push-jobs.md). + +## Contents + +- [Summary table](#summary-table) +- [Device / key GC — `services/deviceGc.ts`](#device--key-gc--servicesdevicegcts) +- [Envelope GC — `services/envelopeGc.ts`](#envelope-gc--servicesenvelopegcts) +- [File cleanup — `services/fileCleanup.ts`](#file-cleanup--servicesfilecleanupts) +- [Push-subscription backoff re-enable](#push-subscription-backoff-re-enable) +- [Idempotency and retry-safety, in general](#idempotency-and-retry-safety-in-general) +- [Multi-node deployment implications](#multi-node-deployment-implications) + +## Summary table + +| Job | Interval (default) | Interval env var | What it removes | Retention window (default) | Retention env var(s) | +|---|---|---|---|---|---| +| Prekey GC | 1 hour | `DEVICE_GC_INTERVAL_MS` | Consumed or unclaimed one-time prekeys (`device_prekeys`, `key_type = 'one_time'`) | 30d consumed / 90d unconsumed | `PREKEY_CONSUMED_RETENTION_DAYS`, `PREKEY_UNCONSUMED_MAX_AGE_DAYS` | +| MLS key package GC | 1 hour | `DEVICE_GC_INTERVAL_MS` | Consumed or unclaimed MLS `KeyPackage`s (`mls_key_packages`) | 30d consumed / 90d unconsumed | `PREKEY_CONSUMED_RETENTION_DAYS`, `PREKEY_UNCONSUMED_MAX_AGE_DAYS` | +| Device stale-flag | 1 hour | `DEVICE_GC_INTERVAL_MS` | Nothing — flags (`stale_flagged_at`) devices revoked past the window | 180d since revocation | `DEVICE_STALE_AFTER_DAYS` | +| Envelope GC | 30 minutes | `ENVELOPE_GC_INTERVAL_MS` | `message_envelopes` rows (per-recipient-device delivery units) | 7d after delivery / 30d max age regardless | `ENVELOPE_DELIVERED_RETENTION_DAYS`, `ENVELOPE_MAX_AGE_DAYS` | +| File hard-delete | 5 minutes | `FILE_GC_INTERVAL_MS` | S3 objects (+ `files` row marked `hard_deleted_at`) for soft-deleted, fully-unreferenced files | 0ms grace by default, then immediate | `FILE_HARD_DELETE_GRACE_MS` | +| Pending-upload GC | 5 minutes (same tick as file hard-delete) | `FILE_GC_INTERVAL_MS` | S3 objects + `files` rows for uploads never confirmed | 24 hours since creation | `PENDING_UPLOAD_TTL_MS` | +| Push-subscription backoff re-enable | 5 minutes (same tick as file hard-delete) | `FILE_GC_INTERVAL_MS` | Nothing — clears `disabled_at` on `push_subscriptions` whose backoff has expired | n/a — driven by the 5-minute backoff set at send time, not a configurable window | none | + +All six passes are started once at process boot, in [`index.ts`](../src/index.ts), and +run on a plain `setInterval` for the lifetime of the process (`.unref()`'d so they never +keep the process alive on their own). + +--- + +## Device / key GC — `services/deviceGc.ts` + +Three independent passes share one hourly timer (`startDeviceGcJob`): + +### 1. One-time prekey pruning (`runPrekeyGcPass`) + +- **Removes**: rows in `device_prekeys` where `key_type = 'one_time'` and either + - `consumed = true` and `created_at` is older than `PREKEY_CONSUMED_RETENTION_DAYS` + (default **30 days**) — the recipient already claimed it, kept only for audit, or + - `consumed = false` and `created_at` is older than `PREKEY_UNCONSUMED_MAX_AGE_DAYS` + (default **90 days**) — nobody claimed it in time. +- Signed prekeys are never touched by this pass — a device has exactly one live signed + prekey and it's replaced in place on upload, not garbage-collected. + +### 2. MLS KeyPackage pruning (`runMlsKeyPackageGcPass`) + +- Same two-tier policy (consumed vs. unconsumed, same two env vars) applied to + `mls_key_packages` instead of `device_prekeys`. Kept as a separate pass because the two + tables are unrelated, but they're tuned by the same knobs since they represent the same + "unclaimed one-time key material" concept for two different protocols. + +### 3. Stale-device flagging (`runDeviceStaleFlagPass`) + +- **Flags, never deletes.** Sets `devices.stale_flagged_at` on rows where + `revoked_at IS NOT NULL`, `revoked_at` is older than `DEVICE_STALE_AFTER_DAYS` + (default **180 days**), and `stale_flagged_at IS NULL`. +- Revocation history is preserved indefinitely — this pass only marks a device eligible + for whatever downstream archival/audit process consumes the flag; it does not delete + the `devices` row or any of its history. + +**Env vars**: `DEVICE_GC_INTERVAL_MS` (tick interval, default 1h), +`PREKEY_CONSUMED_RETENTION_DAYS` (default 30), `PREKEY_UNCONSUMED_MAX_AGE_DAYS` +(default 90), `DEVICE_STALE_AFTER_DAYS` (default 180). All four are read via a shared +`envDays()`/interval helper that falls back to the default on a missing, non-numeric, or +non-positive value — a malformed env var degrades to "use the default", not a crash. + +--- + +## Envelope GC — `services/envelopeGc.ts` + +`message_envelopes` holds one row per **(message, recipient device)** — the actual +delivery unit. It is the only thing that keeps a delivered message's audit trail (the +delivered/read timestamps) around after the fact, and it grows with every message times +every recipient device, so it needs the tightest retention of any table in the system. + +`runEnvelopeGcPass` deletes a row when either is true: + +- **Delivered and aged out**: `delivered_at IS NOT NULL` and `delivered_at` is older + than `ENVELOPE_DELIVERED_RETENTION_DAYS` (default **7 days**) — the common case, the + recipient device picked it up. +- **Past the max-age ceiling regardless of delivery state**: `created_at` is older than + `ENVELOPE_MAX_AGE_DAYS` (default **30 days**) — a device that never comes back to + collect its envelope does not get to pin storage forever. + +**Schedule**: every 30 minutes by default (`ENVELOPE_GC_INTERVAL_MS`). + +**Env vars**: `ENVELOPE_GC_INTERVAL_MS`, `ENVELOPE_DELIVERED_RETENTION_DAYS`, +`ENVELOPE_MAX_AGE_DAYS`. + +--- + +## File cleanup — `services/fileCleanup.ts` + +Implements the soft-delete → hard-delete lifecycle from issue #231. `softDeleteFile()` +sets `files.deleted_at` synchronously when a message referencing it is retracted (it is +not part of the scheduled job — it's called inline from the message-delete path). The +scheduled job (`startFileCleanupJob`, tick interval `FILE_GC_INTERVAL_MS`, default +**5 minutes**) then does three things per tick, in order: + +### 1. Hard-delete pass (`runHardDeletePass`, first half) + +Candidates: `files` rows where `deleted_at IS NOT NULL`, `hard_deleted_at IS NULL`, and +`deleted_at` is older than `FILE_HARD_DELETE_GRACE_MS` (default **0** — no grace period, +eligible as soon as soft-deleted). + +For each candidate the job: + +1. **Re-checks the reference count** (see below) — if any live message still points at + the file, it's skipped this tick and picked up again on a later one once that last + reference clears. +2. Deletes the S3 object via `getObjectStore().deleteObject(storageKey)`. +3. Only after that delete succeeds, sets `hard_deleted_at = now()`. + +A failure at step 2 or 3 is caught, logged, and the file is simply left in its current +(soft-deleted, not-yet-hard-deleted) state for the next tick to retry — no exception +propagates out of the loop, so one bad object doesn't stop the rest of the batch. + +### 2. Pending-upload GC (`runHardDeletePass`, second half) + +Candidates: `files` rows with `status = 'pending'` and `created_at` older than +`PENDING_UPLOAD_TTL_MS` (default **24 hours**) — an upload slot was requested but the +client never confirmed it. The object is deleted from S3 and the `files` row is deleted +outright (there's no soft-delete step for a pending upload; nothing ever referenced it). + +### 3. Push-subscription backoff re-enable + +See [below](#push-subscription-backoff-re-enable) — runs as the last step of the same +tick, after the hard-delete pass, in `startFileCleanupJob`'s interval callback. + +### Reference-counting check + +Before either the initial candidate query or the hard delete itself, the job protects +against deleting a file another live message still points at: + +```sql +SELECT 1 FROM messages +WHERE file_id = + AND deleted_at IS NULL +LIMIT 1 +``` + +If this returns a row, the file is skipped for this tick. This matters because a file +can be attached to more than one message (e.g. forwarded), so retracting *one* message +that references a file must not delete the underlying object while another, +non-retracted message still needs it. `softDeleteFile()` itself runs the same +`NOT EXISTS (...)` check inline (as part of its `UPDATE ... WHERE`) before it will even +set `deleted_at`, so a file only becomes hard-delete-eligible once every referencing +message has been retracted. + +**Env vars**: `FILE_GC_INTERVAL_MS` (default 5min), `FILE_HARD_DELETE_GRACE_MS` (default +0), `PENDING_UPLOAD_TTL_MS` (default 24h). + +--- + +## Push-subscription backoff re-enable + +Not a standalone timer — it's called (`reenableExpiredBackoffs()`, defined in +`services/pushNotification.ts`) as the second step of every file-cleanup tick, so it +inherits `FILE_GC_INTERVAL_MS` as its effective schedule (default every 5 minutes). + +**What it removes**: nothing is deleted. It's a single `UPDATE`: + +```sql +UPDATE push_subscriptions +SET disabled_at = NULL +WHERE disabled_at IS NOT NULL AND disabled_at <= NOW() +``` + +**Where the backoff comes from**: a transient push-send failure (any error other than a +404/410 "gone" response, which prunes the subscription immediately instead) sets +`disabled_at = now() + 5 minutes` in `sendWebPush()`. This pass is what clears that flag +once the 5-minute window has elapsed, making the subscription eligible for delivery +again. There is no separate env var for the backoff duration itself — it's a fixed +5-minute constant at the call site — only the re-enable pass's cadence is configurable, +and only indirectly via `FILE_GC_INTERVAL_MS`. + +--- + +## Idempotency and retry-safety, in general + +Every pass above is safe to crash mid-run and simply pick back up on the next tick, +by construction: + +- **Prekey / MLS key package GC**: a plain `DELETE ... WHERE ` is naturally + idempotent — a row either still matches the cutoff (gets deleted again, a no-op if it + was already gone) or it doesn't (left alone). There's no two-step state to get half-done. +- **Device stale-flag pass**: only touches rows where `stale_flagged_at IS NULL`, so a + row that already got flagged is excluded from the next run's `WHERE` clause — + re-running against already-flagged devices is a no-op. +- **Envelope GC**: same reasoning as prekey GC — a single-statement `DELETE ... WHERE`. +- **File hard-delete — the one two-step case**: this is the pass where crash-safety + actually has to be designed for, because deleting the S3 object and marking the + database row are two separate operations that can't be wrapped in one transaction (the + object store isn't Postgres). The job orders them deliberately: + + > delete the S3 object first, and only set `hard_deleted_at` **after** that delete + > succeeds. + + If the process crashes (or the DB write fails) between those two steps, the object is + already gone from S3 but `hard_deleted_at` is still `NULL`. On the next tick, the file + is picked up as a candidate again; the re-check query finds no live reference, and the + code calls `deleteObject()` a second time. Object stores generally treat deleting an + already-absent key as a success (or the client's error is caught and logged and the row + is retried again next tick) rather than a hard failure, so the retry converges instead + of raising an inconsistency. The failure mode this ordering rules out is the opposite + one — flagging `hard_deleted_at` and then crashing *before* the S3 delete actually runs, + which would permanently orphan the object with no job left to clean it up. See + `fileCleanup.test.ts`'s `'does not mark hardDeletedAt when S3 delete throws (safe + retry)'` case for the regression test on this ordering. +- **Pending-upload GC**: same object-then-row ordering as hard-delete, same retry + argument — a crash between the S3 delete and the Postgres row delete just means the + row is deleted again (already-gone-in-S3, still-present-in-Postgres) on the next tick. +- **Push backoff re-enable**: a single `UPDATE ... WHERE disabled_at <= NOW()` — the + `WHERE` clause makes re-running it against already-cleared rows a no-op. + +## Multi-node deployment implications + +All six passes are started unconditionally in [`index.ts`](../src/index.ts) — +`startFileCleanupJob()`, `startDeviceGcJob()`, `startEnvelopeGcJob()` — with **no leader +election, no distributed lock, and no "only run on node 0" guard**. That means: + +- **Every gateway node runs every job, on its own timer, independently.** In an N-node + deployment, N processes are issuing the same `DELETE ... WHERE ` / + `UPDATE ... WHERE ` queries against the same Postgres database on overlapping + schedules. +- This is safe *because* every pass above is idempotent (see the previous section): a + row is only affected while it still matches the `WHERE` clause, so two nodes racing to + delete/flag the same row just means one of them does the work and the other's query + matches zero rows. Nothing double-decrements or double-processes. +- The cost is redundant work, not correctness risk: N nodes each executing the same scan + and (mostly no-op) `DELETE`/`UPDATE` every tick is wasted query load that scales + linearly with node count, and N nodes each calling `deleteObject()` for the same S3 key + around the same time is wasted object-store calls (again safe, since a repeat delete of + an already-gone key is not an error). +- **Timers are not synchronized across nodes.** Each node starts its own `setInterval` + from its own boot time, so in practice the N nodes' ticks are staggered, which spreads + the redundant load out somewhat rather than having every node hit the database in the + same instant — but this is incidental, not a designed-in stagger. +- If this redundant load ever becomes a real concern at higher node counts, the fix is to + gate `startDeviceGcJob()` / `startEnvelopeGcJob()` / `startFileCleanupJob()` behind a + single-designated-node check (e.g. lowest pod ordinal, or a Redis lock) rather than + changing the jobs' own logic — the jobs themselves don't need to change since they're + already safe to run from any single node. diff --git a/apps/backend/docs/concepts-logging.md b/apps/backend/docs/concepts-logging.md new file mode 100644 index 0000000..e9d4290 --- /dev/null +++ b/apps/backend/docs/concepts-logging.md @@ -0,0 +1,184 @@ +# Backend Structured Logging Conventions + +This document covers [`lib/logger.ts`](../src/lib/logger.ts) — the structured logger for +the encrypted pipeline (issue #393): its configuration, log levels, standard fields, and, +most importantly, the rule that message content, ciphertext, and key material never +appear in a log line. + +## Contents + +- [Configuration](#configuration) +- [Levels, and how level is set per environment](#levels-and-how-level-is-set-per-environment) +- [The no-content rule](#the-no-content-rule) +- [Standard correlation fields](#standard-correlation-fields) +- [Current adoption: `console.*` is still the norm](#current-adoption-console-is-still-the-norm) + +## Configuration + +`lib/logger.ts` exports a single shared [pino](https://getpino.io/) instance: + +```ts +export const logger = pino({ + level: process.env['LOG_LEVEL'] ?? 'info', + redact: { + paths: [ + 'ciphertext', '*.ciphertext', + 'envelopes', '*.envelopes', + 'payload', '*.payload', + 'plaintext', '*.plaintext', + ], + censor: '[redacted]', + }, + formatters: { + level: (label) => ({ level: label }), + }, + base: { service: 'clicked-backend' }, +}); +``` + +- **`level`** — read from `LOG_LEVEL`, defaulting to `'info'`. See + [below](#levels-and-how-level-is-set-per-environment). +- **`redact`** — a fixed list of field-name paths (`ciphertext`, `envelopes`, `payload`, + `plaintext`, each matched both at the top level and one level deep via the `*.` + wildcard) that pino will replace with the literal string `[redacted]` if they ever + appear in a logged object. The paths line up directly with the shapes actually flowing + through the system: `payload` is the field on `EventEnvelope` + (`lib/eventEnvelope.ts`'s `EventEnvelopeSchema`), `envelopes` is the array of + per-recipient-device ciphertext blobs on a `send_message` call, and + `ciphertext`/`plaintext` are the raw content fields on an individual envelope. This is + a **backstop**, not the primary guarantee — see the [no-content rule](#the-no-content-rule) + below for why you can't rely on it alone. +- **`formatters.level`** — pino's default is to log the numeric level; this overrides it + to log the string label (`"info"`, `"warn"`, etc.) instead, so log lines are readable + without a lookup table. +- **`base: { service: 'clicked-backend' }`** — every line this logger emits carries + `service: "clicked-backend"`, so log lines from this process are identifiable in a + shared/aggregated log stream (e.g. alongside the AI agent or other services) without + extra configuration at every call site. + +There is no per-request or per-socket child-logger factory defined yet — see +[Standard correlation fields](#standard-correlation-fields) for what's available to attach +manually, and [Current adoption](#current-adoption-console-is-still-the-norm) for why this +matters less today than it will once call sites migrate to it. + +## Levels, and how level is set per environment + +pino's standard level ordering applies: `fatal` > `error` > `warn` > `info` > `debug` > +`trace` (plus `silent` to disable entirely) — set `level` to a name and every level at or +above it (i.e. more severe or equal) is emitted; everything below it is a no-op with +near-zero overhead (pino checks the level before serializing). + +**Per-environment control is entirely via the `LOG_LEVEL` env var** — there's no +hardcoded environment-name branch (`if (NODE_ENV === 'production')` etc.) in +`lib/logger.ts` itself: + +- Unset → defaults to **`info`**. This is what local dev and any environment that doesn't + explicitly set `LOG_LEVEL` gets. +- Set `LOG_LEVEL=debug` (or `trace`) locally or in a staging environment to see verbose + output while debugging a specific issue — remember to unset it again rather than + leaving a deployment running at `debug` indefinitely, since `debug`/`trace` volume adds + up quickly on a busy gateway. +- Set `LOG_LEVEL=warn` (or stricter) in an environment where you only want actionable + signal and want to cut `info`-level noise (e.g. a high-throughput environment where + per-connection `info` lines aren't worth the log volume). +- `LOG_LEVEL` is read once at module load (`pino({ level: process.env['LOG_LEVEL'] ?? + 'info'] ... })`), so changing it requires a process restart — it is not something a + running process picks up live. + +## The no-content rule + +**Message content, ciphertext, and key material never appear in a log line — this is a +hard rule, not a style preference.** The whole point of the encrypted pipeline is that the +server never has plaintext to leak in the first place, but ciphertext, key material +(prekeys, signed prekeys, MLS `KeyPackage`s, session keys), and full envelope objects are +exactly as sensitive from a logging standpoint: they should never be the value of a +logged field, truncated preview or not, redacted-and-hope or not. + +**The redact list in `lib/logger.ts` is a backstop, not the mechanism you should rely on.** +It only fires if a call site accidentally passes a *whole object* that happens to contain +one of the redacted field names — it does nothing for a raw string interpolated into a +message (`` logger.info(`sending ${ciphertext}`) `` bypasses `redact` entirely, because +`redact` only inspects the structured fields of a log object, not free-text message +strings) and nothing for a field passed under a name the list doesn't happen to cover. +The actual guarantee has to come from what you choose to log in the first place. + +**The safe way to log about a message: ids and counts, never bodies.** + +```ts +// ❌ Never — even though `redact` will catch some of this, don't rely on it, +// and the string-interpolation case isn't caught at all. +logger.info(`delivering message ${JSON.stringify(envelope)}`); +console.log('sending', { ciphertext: envelope.ciphertext }); + +// ✅ Ids, counts, sizes, durations — never the content itself. +logger.info({ messageId, conversationId, recipientDeviceCount: envelopes.length }, + 'message fanout dispatched'); +logger.debug({ deviceId, eventId, envelopeByteLength: envelope.ciphertext.length }, + 'envelope accepted'); +logger.warn({ subscriptionId, statusCode }, 'push send failed, backing off'); +``` + +Concretely, safe fields to log about a message/envelope/key include: `messageId`, +`conversationId`, `deviceId`, `userId`, `eventId`, counts (`recipientDeviceCount`, +`prunedCount`), sizes/lengths (`envelopeByteLength`, never the bytes themselves), status +codes, durations, and timestamps. Never log: `ciphertext`, `plaintext`, the raw +`payload`/`envelopes` object, key material (`identityPublicKey`, prekey bytes, MLS +`KeyPackage` bytes), auth tokens, or `p256dh`/`auth` push subscription keys. When in +doubt, log the *shape* (a count, a length, a boolean) rather than the *value*. + +## Standard correlation fields + +There is no dedicated request-id/correlation-id middleware in this backend today — HTTP +access logging goes through `morgan('dev')` (`app.ts`), which logs method, path, status, +and response time, with no generated request id attached, and is separate from +`lib/logger.ts` entirely. For tracing a specific request or socket event through +structured log lines, use the identifiers already available at each call site: + +- **`deviceId`** — available on every authenticated socket via `socket.auth.deviceId` + (set by `socketAuthMiddleware`) and on every REST request via the equivalent JWT-derived + auth context. The most useful single field for tracing one device's activity across + connect/disconnect/send/receive. +- **`userId`** — `socket.auth.userId` / the REST auth context; ties multiple devices + belonging to the same account together. +- **`eventId`** — present on every dispatched socket envelope (`EventEnvelopeSchema.eventId`, + `lib/eventEnvelope.ts`), generated client-side or via `createEnvelope()`. This is what + the existing replay-protection debug log already keys on + (`dispatcher.ts`: `{ deviceId, eventId, type }`) — follow that shape for any new + per-event log line. +- **`conversationId`** / **`messageId`** — attach whichever is relevant to the operation; + both are safe to log (they're routing/addressing metadata, not content). +- **`socket.id`** — Socket.IO's own per-connection id; useful for correlating multiple + events from the same physical connection within one session, but don't use it as a + stand-in for `deviceId` across reconnects — a new connection gets a new `socket.id`. + +None of these are enforced by a shared child-logger or middleware yet — attach the +relevant subset by hand at each call site (as the redact-list field names already imply +you should for `payload`/`envelopes`/`ciphertext`/`plaintext`), following the pattern +shown in [the no-content rule](#the-no-content-rule) above. + +## Current adoption: `console.*` is still the norm + +**`lib/logger.ts` has no import sites anywhere else in `apps/backend/src` today.** Every +current log line in the backend — startup/shutdown messages in `index.ts`, the GC job +services (`deviceGc.ts`, `envelopeGc.ts`, `fileCleanup.ts`), the socket dispatcher and +messaging handlers, push notification hygiene, presence, backpressure, rate limiting, and +so on — goes through bare `console.log`/`console.warn`/`console.error`/`console.debug` +instead (roughly 60 call sites across the backend as of this writing). Even +`services/stellarListener.ts`, whose own doc comment says it "logs errors via the standard +backend logger," actually defaults its optional `log` dependency to a small hand-rolled +`consoleLogger` wrapper around `console.*`, not to `lib/logger.ts`. + +Practical implications: + +- **None of the level control, redaction backstop, or `service` base field described + above currently applies to the backend's actual log output** — `console.*` calls bypass + all of it. `LOG_LEVEL` has no effect on anything printed via `console.log`. +- **New code should use `lib/logger.ts`, not add more `console.*` calls.** The existing + ~60 call sites are the migration backlog, not the target state — don't grow that number. + When you're already touching a file that logs via `console.*`, prefer converting the + lines you touch to `logger` rather than leaving new logic logging through the old path. +- When you do add a `logger.*` call, follow the [no-content rule](#the-no-content-rule) + and attach the [correlation fields](#standard-correlation-fields) relevant to that + call site, the same way the codebase's existing `console.*` calls already do at their + best (e.g. `dispatcher.ts`'s `{ deviceId, eventId, type }` shape) — the goal is to bring + that same discipline under the structured logger, not to reinvent it. diff --git a/docs/load-testing.md b/docs/load-testing.md new file mode 100644 index 0000000..e4a1bb7 --- /dev/null +++ b/docs/load-testing.md @@ -0,0 +1,253 @@ +# Load and Soak Testing Guide + +This document covers `scripts/loadtest/` — the harness that seeds a fixture, connects a +fleet of Socket.IO clients across one or more backend nodes, and drives a scripted +fan-out/churn/reconnect workload against them. For how this harness is wired into CI, see +the [`loadtest-nightly`](./ci-cd.md#loadtest-nightly) section of `docs/ci-cd.md` — this +document is about the scripts themselves: what they do, how to run them locally, what the +numbers mean, and how to maintain the regression baseline. + +## Contents + +- [What the harness exercises](#what-the-harness-exercises) +- [`seed.ts`](#seedts) +- [`run.ts`](#runts) +- [Required local topology](#required-local-topology) +- [The three phases](#the-three-phases) +- [Metrics and thresholds](#metrics-and-thresholds) +- [`baseline.json`](#baselinejson) +- [The nightly workflow, and what to do when it fails](#the-nightly-workflow-and-what-to-do-when-it-fails) + +## What the harness exercises + +`seed.ts` writes a fixture (users, devices, JWTs, one shared group conversation) straight +to Postgres via Drizzle. `run.ts` reads that fixture, connects every participant as a real +`socket.io-client`, splits those connections evenly across the node URLs you pass it, and +runs three phases against them — sampling process RSS every second throughout. Splitting +connections across multiple node URLs, all pointed at the same Redis, is the point of the +exercise: it's the only place in the repo's test suite that forces a message to fan out +*across* backend instances via `@socket.io/redis-adapter`, rather than within one +in-process `Server`. A single-node run is still useful (it's a valid, if less interesting, +topology) but doesn't exercise that cross-instance path. + +## `seed.ts` + +```bash +npx tsx scripts/loadtest/seed.ts --devices 200 > fixture.json +``` + +- Creates one `type: 'group'` conversation named `loadtest`. +- For each of `--devices` (default **200**) iterations: inserts a `users` row (random + `loadtest--` username), one `devices` row for that user (`platform: 'web'`, + random identity key), adds the user to the conversation via `conversationMembers`, and + signs a JWT for that `(userId, deviceId)` pair with `signToken()` — the same signer the + real auth path uses, so the fixture's tokens are accepted by `socketAuthMiddleware` + exactly like a real client's. +- Writes the resulting `Fixture` — `{ conversationId, participants: [{ userId, deviceId, + token }, ...] }` — to **stdout** as JSON. Progress/errors go to stderr, so + `> fixture.json` captures only the fixture itself. +- Talks to the database directly (imports `db` and the schema from + `apps/backend/src/...`) — it does not go through the HTTP/socket API to seed data, so it + needs a reachable `DATABASE_URL` but not a running backend. +- The only flag is `--devices `; an unparseable or non-positive value silently falls + back to 200. + +## `run.ts` + +```bash +npx tsx scripts/loadtest/run.ts \ + --fixture fixture.json \ + --nodes http://localhost:3001,http://localhost:3002 \ + [--baseline scripts/loadtest/baseline.json] \ + [--out result.json] +``` + +| Flag | Required | Meaning | +|---|---|---| +| `--fixture` | no (default `fixture.json`) | Path to the JSON file `seed.ts` produced. | +| `--nodes` | no (default `http://localhost:3001`) | Comma-separated backend base URLs. Participant `i` connects to `nodes[i % nodes.length]` — a simple round-robin, so device order in the fixture determines which node each device lands on. | +| `--baseline` | no | Path to a baseline JSON file (see [below](#baselinejson)). If omitted, the run still enforces the fixed `THRESHOLDS` but skips the regression comparison. | +| `--out` | no | If set, also writes the JSON summary to this path (in addition to stdout). | + +Every socket connects with `transports: ['websocket']` and `reconnection: false` — the +harness handles reconnects itself (phases 2 and 3 below), so it doesn't want the client +library racing its own reconnect logic against the script's. + +**Exit code**: `0` if every threshold and (when `--baseline` is given) the regression +check pass; `1` otherwise, or on a fatal error (e.g. a connect timeout during the initial +connect-all-devices step, before any phase runs). The JSON summary is printed to stdout +regardless of pass/fail; failure reasons are printed to stderr as `FAIL: ...` lines. + +## Required local topology + +To reproduce what the nightly workflow does on your own machine: + +1. **Postgres and Redis reachable**, with `DATABASE_URL` / `REDIS_URL` pointed at them. + `REDIS_URL` in particular has to be the *same* Redis for every backend node you start — + that shared Redis is what lets `@socket.io/redis-adapter` fan a message out across + nodes. +2. **Run migrations** (`pnpm --filter backend db:migrate`) before seeding — `seed.ts` + writes through Drizzle against the real schema. +3. **Two (or more) backend instances**, each on its own `PORT`, e.g.: + ```bash + PORT=3001 pnpm --filter backend dev & + PORT=3002 pnpm --filter backend dev & + ``` + pointed at the same `DATABASE_URL`/`REDIS_URL`. Wait for both `/health` endpoints + before proceeding — the nightly workflow polls this with a 30-retry/1s loop. +4. **Seed, then run**, from the repo root (the scripts import backend modules by relative + path, so they expect to run from there): + ```bash + npx tsx scripts/loadtest/seed.ts --devices 200 > fixture.json + npx tsx scripts/loadtest/run.ts --fixture fixture.json \ + --nodes http://localhost:3001,http://localhost:3002 \ + --baseline scripts/loadtest/baseline.json --out loadtest-result.json + ``` + +A single-node run (`--nodes http://localhost:3001` only) works too and is a reasonable +smoke test, but it will not catch a cross-instance fan-out regression — use two nodes to +match what the nightly job actually validates. + +## The three phases + +Run in this order, against the same connected set of sockets: + +1. **Fan-out latency.** One participant (`sockets[0]`) is the sender; every other + connected socket is a recipient. The sender emits `send_message` with one envelope per + recipient device (placeholder `ciphertext: 'loadtest-ciphertext'` — the harness isn't + testing encryption, just delivery). Each recipient's `message_envelope` receipt is + timestamped against the send time, producing one latency sample per recipient. The + phase ends when every recipient has received it or after a 30-second timeout, whichever + comes first; any recipients still missing at that point count as a fan-out error. +2. **Presence churn.** 20% of connected sockets (minimum 1) disconnect, wait 500ms, + reconnect (against the same node-assignment rule), wait another 500ms — repeated for + 3 rounds. A reconnect that throws/times out is recorded as an error but does not abort + the run. +3. **Reconnect storm.** Every socket disconnects at once, a 200ms pause, then every + participant reconnects simultaneously. Failures are counted; all sockets are then + disconnected again to close out the run. + +Peak RSS (`process.memoryUsage().rss`, sampled every second via a background interval +that started before phase 1 and is cleared after phase 3) is the memory figure reported — +it's the load-test **client** process's own memory, not the backend's, so it primarily +tracks the socket.io-client fleet's footprint, not server-side memory pressure. + +## Metrics and thresholds + +The summary JSON: + +```json +{ + "deviceCount": 200, + "nodeCount": 2, + "fanoutDelivered": 199, + "fanoutExpected": 199, + "latencyMs": { "p50": 80, "p95": 400, "p99": 900 }, + "peakRssMb": 350, + "errorCount": 0, + "errorRate": 0, + "errors": [] +} +``` + +- **`latencyMs.p50/p95/p99`** — percentiles over the fan-out phase's per-recipient + latency samples only (churn and reconnect-storm timings aren't included in this + distribution). `percentile()` sorts ascending and indexes at + `floor((p/100) * length)`, i.e. nearest-rank, not interpolated. +- **`peakRssMb`** — the single highest RSS sample seen across the whole run (all three + phases), rounded to the nearest MB. +- **`errorCount`** / **`errors`** — a flat list of string messages accumulated from any + phase: missed fan-out receipts, failed churn reconnects, failed storm reconnects. +- **`errorRate`** — `errorCount / (participants.length * 4)`. The `* 4` denominator is a + rough per-participant "opportunities to fail across all phases" estimate, not an exact + count — treat `errorRate` as a normalized signal, not a precise fraction. + +**Fixed thresholds** (`THRESHOLDS`, always enforced regardless of `--baseline`): + +| Threshold | Value | +|---|---| +| `maxP95LatencyMs` | 1500 | +| `maxP99LatencyMs` | 3000 | +| `maxPeakRssMb` | 1024 | +| `maxErrorRate` | 0.01 | + +Any one breach fails the run (exit code 1, with a `FAIL: ...` line on stderr naming which +threshold and by how much) and continues checking the rest — the printed `FAIL` lines from +one run can name multiple breaches at once. + +## `baseline.json` + +`scripts/loadtest/baseline.json` is a small, **statically committed** file — nothing in +the repo generates or writes it back automatically: + +```json +{ + "deviceCount": 200, + "nodeCount": 2, + "latencyMs": { "p50": 80, "p95": 400, "p99": 900 }, + "peakRssMb": 350 +} +``` + +It represents a known-good run's numbers, captured by hand at some point in the past, for +the same topology (`200` devices, `2` nodes) the nightly workflow uses. It exists to catch +*gradual* regressions the fixed `THRESHOLDS` are too loose to catch — e.g. p95 creeping +from 400ms to 900ms is still well under the fixed 1500ms ceiling but is a 2x-plus +regression against what this exact workload used to cost. + +**Regression tolerance**: when `--baseline` is passed and the file parses, `run.ts` +compares only two fields, each against a fixed `REGRESSION_TOLERANCE = 1.25` (25%): + +- fails if `summary.latencyMs.p95 > baseline.latencyMs.p95 * 1.25` +- fails if `summary.peakRssMb > baseline.peakRssMb * 1.25` + +(`p50`/`p99`/error rate are not compared against the baseline — only p95 latency and peak +memory are.) If the baseline file is missing or fails to parse, the script logs +`no usable baseline at , skipping regression check` and does **not** fail the run on +that account — a missing/corrupt baseline degrades to "threshold-only enforcement," not to +a hard failure. + +**Refreshing the baseline**: because nothing writes this file automatically, a +legitimate, accepted change that raises latency or memory (a heavier feature, a new +per-message check, etc.) will fail the nightly run indefinitely until someone manually +updates it. To refresh it: + +1. Confirm the new numbers are an accepted cost, not an unintended regression — read the + failing run's uploaded `loadtest-result.json` artifact (or run the harness locally) and + satisfy yourself the new latency/memory profile is expected given what changed. +2. Copy the new run's `latencyMs`, `peakRssMb` (and `deviceCount`/`nodeCount`, if the + topology itself changed) into `scripts/loadtest/baseline.json`. +3. Commit that change on its own, with a message naming what caused the shift, so the + baseline's history stays legible to whoever hits the next regression. + +## The nightly workflow, and what to do when it fails + +The full workflow mechanics (services, steps, triggers) are documented in +[`docs/ci-cd.md#loadtest-nightly`](./ci-cd.md#loadtest-nightly) — in short, it's a +`schedule`-triggered (03:00 UTC) and `workflow_dispatch`-triggered job that boots +Postgres + Redis + two backend instances (ports 3001/3002) sharing that Redis, seeds a +200-device fixture, and runs exactly the `run.ts` invocation shown above with +`--baseline scripts/loadtest/baseline.json`. It is **not** wired to `push`/`pull_request`, +so a red nightly run never blocks a merge — it's a signal about the prior day's merges, +not a gate. + +When it fails: + +1. **Download the `loadtest-result` artifact** (`loadtest-result.json`, uploaded via + `if: always()` so it's there even on failure) from the workflow run — it has the exact + numbers and the `errors` array. +2. **Read which check failed** from the job's log `FAIL: ...` lines — a fixed-threshold + breach (real latency/memory/error problem, full stop) reads differently from a + baseline-regression-only failure (numbers are still under the hard ceiling, just worse + than they used to be). +3. **Reproduce locally** using the topology above if you need to bisect — run the harness + before and after a suspected commit with the same `--devices`/`--nodes` to compare. +4. **If it's a real regression**, treat it like any other performance bug: find the commit + (the workflow runs nightly, so the failure window is roughly "yesterday's merges to + main"), fix it, and let the next nightly run confirm. +5. **If it's an accepted cost**, refresh `baseline.json` as described above rather than + leaving the nightly job red — a baseline that no longer matches reality makes every + future run's regression signal meaningless. +6. **Use `workflow_dispatch`** to re-run on demand rather than waiting for the next + scheduled run, especially when validating a fix or a change to the load-test scripts + or gateway fan-out logic themselves. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md new file mode 100644 index 0000000..e0bcd1d --- /dev/null +++ b/docs/troubleshooting.md @@ -0,0 +1,269 @@ +# Troubleshooting and FAQ + +Failures contributors actually hit in this repo, with the symptom, the underlying cause, +and the fix. For workflow-by-workflow CI mechanics, see [`docs/ci-cd.md`](./ci-cd.md); for +first-time environment setup, see [`docs/development-setup.md`](./development-setup.md) +(its own §9 covers a few setup-time issues not repeated here). + +## Contents + +- [`pnpm install` fails on a lockfile/package.json mismatch](#pnpm-install-fails-on-a-lockfilepackagejson-mismatch) +- [My migration didn't run](#my-migration-didnt-run) +- ["Works locally, fails in CI" — floating toolchains](#works-locally-fails-in-ci--floating-toolchains) +- [Tests interfere with each other through shared in-process counters](#tests-interfere-with-each-other-through-shared-in-process-counters) +- [Web app fails to build after a dependency drift](#web-app-fails-to-build-after-a-dependency-drift) + +--- + +## `pnpm install` fails on a lockfile/package.json mismatch + +**Symptom**: `pnpm install --frozen-lockfile` (what every CI workflow runs — see +`backend-ci.yml`, `frontend-ci.yml`, `loadtest-nightly.yml`) fails with something like +`ERR_PNPM_OUTDATED_LOCKFILE` / "Lockfile is not up to date with package.json", even +though `pnpm install` (no flag) works fine on your machine. + +**Cause**: this repo has more committed lockfiles than a single pnpm workspace should +have: + +``` +pnpm-lock.yaml ← the one CI actually installs from (root workspace) +package-lock.json ← an npm lockfile, also committed, at the root +apps/backend/pnpm-lock.yaml ← a second pnpm lockfile, inside a workspace package +apps/web/pnpm-lock.yaml +apps/web/pnpm-workspace.yaml← declares apps/web as its own pnpm workspace root +apps/web/package-lock.json ← an npm lockfile for apps/web, also committed +``` + +The root `pnpm-workspace.yaml` (`packages: ['apps/*', 'contracts']`) is the one CI's +`pnpm install --frozen-lockfile` resolves against, using the root `pnpm-lock.yaml`. But +`apps/web/pnpm-workspace.yaml` means that if you `cd apps/web && pnpm install` directly +(a natural thing to do while working on just the frontend), pnpm treats `apps/web` as an +**independent workspace root** and installs against `apps/web/pnpm-lock.yaml` instead — +a completely separate resolution, with its own `node_modules`, that has no relationship +to the root lockfile CI uses. Editing `apps/web/package.json` and installing from inside +`apps/web` updates the *nested* lockfile; the root `pnpm-lock.yaml` is left stale, and +`--frozen-lockfile` at the root then refuses to proceed because it (correctly) sees a +`package.json` that no longer matches the committed root lockfile. Running `npm install` +anywhere in the tree instead of `pnpm install` has the same effect via the npm lockfiles. + +A real instance of this: commit `46eee04` ("fix: update lockfile for new backend deps, +format fanout.ts") exists specifically because `pnpm-lock.yaml was stale after adding +prom-client/pino to apps/backend/package.json, which would fail --frozen-lockfile in the +new security-ci.yml workflow" — a dependency was added to `apps/backend/package.json` +without the root lockfile being regenerated in the same change. + +**Fix**: + +- Always run `pnpm install` from the **repo root**, never from inside `apps/web` or + `apps/backend` — `corepack enable` first so the pinned `pnpm@10.28.1` is used (see + `docs/development-setup.md` §2). +- After changing any workspace package's `package.json` (adding/bumping a dependency), + regenerate the **root** lockfile before committing: `pnpm install` from the root, then + check `git diff pnpm-lock.yaml` is included in your change. +- Do not commit changes to `package-lock.json`, `apps/web/package-lock.json`, + `apps/web/pnpm-lock.yaml`, or `apps/backend/pnpm-lock.yaml` — they are not what CI + installs from; if your editor/IDE or a stray `npm install` touched one, revert it + (`git checkout -- `) before committing. +- If CI is failing with an outdated-lockfile error, diff your branch's root + `pnpm-lock.yaml` against `main`'s — if it didn't change but `package.json` did, that's + the mismatch. + +--- + +## My migration didn't run + +**Symptom**: you added a new Drizzle migration file under `apps/backend/drizzle/` +(usually via `pnpm db:generate`), it's committed, `pnpm db:migrate` exits `0` with no +error — but the column/table it should have added isn't actually in the database, and +your code that depends on it fails at runtime with a Postgres "column does not exist" +error, not a migration error. + +**Cause**: `drizzle-kit migrate` doesn't scan the `drizzle/` directory for `.sql` files +directly — it reads `apps/backend/drizzle/meta/_journal.json`, which is the ordered list +of migrations it considers to exist: + +```json +{ + "version": "7", + "dialect": "postgresql", + "entries": [ + { "idx": 0, "version": "7", "when": 1787281148373, "tag": "0000_lean_scrambler", "breakpoints": true } + ] +} +``` + +A `.sql` file sitting in `drizzle/` with no corresponding `entries[]` row in +`_journal.json` is invisible to `db:migrate` — it is **silently skipped**, not an error, +because as far as the journal is concerned that migration doesn't exist. This is +different from every other failure in this document: there is no red build, no stack +trace, nothing in CI logs to point at. The only signal is application code breaking +against a schema that looks like it should have the change but doesn't. + +This repo has already been through one event that makes this easy to trigger: commit +`d60b648` ("fix(major): full fix") collapsed seven separate migration files +(`0001_add_system_payload_to_messages.sql`, `0001_audit_logs.sql`, +`0001_device_key_history.sql`, `0001_gc_background_jobs.sql`, +`0001_group_control_events.sql`, `0001_mls_group_state.sql`, `0001_mls_key_packages.sql`, +plus a `0002_*` and a `0003_*`) into the single `0000_lean_scrambler.sql` now in the repo, +rewriting `meta/_journal.json` in the same commit. Two ways this bites a contributor +today: + +- **A merge conflict on `meta/_journal.json`** resolved by taking "theirs" (or a stale + rebase) can drop your branch's newly-generated `entries[]` row while your `.sql` file + itself survives the merge untouched — the file is on disk, but the journal no longer + lists it. +- **Editing schema.ts and hand-writing a migration file** instead of running + `pnpm db:generate` skips the step that appends the journal entry — `drizzle-kit + generate` is what keeps the `.sql` file and the journal row in sync; writing the SQL by + hand does not. + +**Fix**: + +- Always create migrations with `pnpm --filter backend db:generate` (or `pnpm db:generate` + from `apps/backend/`) after changing `src/db/schema.ts` — never hand-write a `.sql` file + under `drizzle/` and expect `db:migrate` to pick it up. +- After generating, confirm `git status` shows **both** the new `drizzle/NNNN_*.sql` file + **and** an updated `drizzle/meta/_journal.json` (with a new `entries[]` row) — a + migration PR that touches the `.sql` file but not `_journal.json` is missing its journal + entry. +- If you suspect a migration silently didn't run, check the database directly: Drizzle's + migrator tracks applied migrations in Postgres (in the `drizzle` schema's own migrations + table) — compare what's recorded there against `meta/_journal.json`'s `entries[]`. If a + tag is in the journal but not in the database's applied-migrations table, `db:migrate` + will still apply it on the next run — it errors loudly if a truly out-of-order state + exists. If a tag is missing from the journal entirely, re-add the correct `entries[]` + row (matching its `tag` to the `.sql` filename, `idx` to its position) and re-run + `pnpm db:migrate`. + +--- + +## "Works locally, fails in CI" — floating toolchains + +**Symptom**: `pnpm lint`, `pnpm test`, or `cargo test`/`cargo build` passes on your +machine but fails in the matching GitHub Actions workflow (or vice versa: green in CI, +red for a teammate), with no relevant diff between what you pushed and what's checked +out. + +**Cause 1 — inconsistent Node pin across workflows.** `backend-ci.yml`, `frontend-ci.yml`, +and `security-ci.yml` all set up **Node 20**, but `pr.yml` ("PR Check") still pins +**Node 18.x**. There is no `.nvmrc`/`.node-version` file and no root `engines` field +pinning a Node version for local dev, so a contributor's local Node (whatever `nvm`/ +`asdf`/system default happens to resolve to) can legitimately differ from both. A +Node-version-sensitive change (a newer built-in, a runtime behavior difference) can pass +under one workflow's Node and fail under another's, or pass locally and fail in +`pr.yml`'s older Node. + +- **Fix**: treat Node **20** as the source of truth for local dev (per + `docs/development-setup.md`) — install it via `corepack`/`nvm`. If a `pr.yml`-specific + failure doesn't reproduce under Node 20, the fix belongs in `pr.yml` (bump it to 20 to + match the rest), not in application code. + +**Cause 2 — `contracts/rust-toolchain.toml` pins `channel = "stable"`, not a version.** +`stable` resolves to whatever the latest stable Rust release is *at the moment `rustup` +installs it* — a fresh CI runner and your local machine (toolchain installed however long +ago) can silently be on different actual compiler versions, which occasionally changes +clippy's lint set or accepts/rejects different code. `contracts-ci.yml` runs on a weekly +schedule (`0 8 * * 1`, every Monday) in addition to push/PR specifically because a `stable` +channel can start failing with **no code change at all** — the toolchain itself moved. + +- **Fix**: if `cargo test`/`cargo clippy` fails in CI but not locally (or the reverse), + run `rustup update` locally to pick up the same `stable` CI is on, rather than assuming + your code is wrong. If a weekly scheduled `contracts-ci` run goes red with no merged + changes since the last green run, that's this — the fix is almost always a small clippy/ + compiler-compat patch, not a revert. + +--- + +## Tests interfere with each other through shared in-process counters + +**Symptom**: a backend test passes in isolation (`vitest run path/to/file.test.ts -t +'that one test'`) but fails when the full file or suite runs — often a rate-limit, +backoff, or violation-count assertion that expects a fresh counter but sees leftover state +from an earlier `it()` block in the same file. + +**Cause**: several backend services intentionally keep their working state in a +module-level `Map`/`Set` rather than Redis, either as a same-process fallback (rate +limiting when Redis is unreachable) or because the state is inherently per-process +(heartbeat timers, socket-scoped violation counts): + +| Module | State | Reset export | +|---|---|---| +| `services/rateLimiter.ts` | `localCounters: Map` (fallback counters used only while Redis is down) | `clearLocalRateLimitCounters()` | +| `services/rateLimit.ts` | `violationCount: Map` | `clearViolations(socketId)` | +| `services/prekeyLowSignal.ts` | `localLatch: Set` | `__resetPrekeyLowLatches()` | +| `services/presence.ts` | `pendingOfflineBroadcasts: Map` | `__resetOfflineBroadcastsForTesting()` | +| `services/heartbeat.ts` | `timers`/`lastSeenAt`/`schedules: Map` | `clearHeartbeatTimer(socketId)` (per-socket, not a full reset) | + +Because Vitest runs every `it()` in a file against the **same imported module instance** +(module isolation is per-file, not per-test), this state persists across test cases +within a file unless a test explicitly clears it. A test that relies on a fresh counter +but forgets the reset call — commonly because a new test was added to an existing +`describe` block without also adding it to that block's `beforeEach` — passes or fails +depending on what ran before it in the same file, which is exactly the "passes alone, +fails in the suite" (or the reverse, and sometimes order-dependent) symptom. +`apps/backend/src/__tests__/rateLimit.test.ts` shows the pattern to follow: + +```ts +describe('checkSocketEventRateLimit', () => { + beforeEach(() => { + clearLocalRateLimitCounters(); + }); + // ... +}); +``` + +**Fix**: + +- When writing a test against a service with module-level state, check whether it exports + a reset/clear function (see table above) and call it in `beforeEach`/`afterEach` for + every `describe` block that exercises that state — not just the first one added. + Copy-pasting an existing `it()` into a new `describe` block without also copying its + `beforeEach` is the most common way this regresses. + - `clearViolations`/`clearHeartbeatTimer` take a socket ID and only clear that entry — + use a fresh, unique socket/device ID per test as the simpler alternative to a global + reset where the module doesn't expose one. +- If a test is flaky specifically when run as part of the full suite but not alone, don't + assume the test itself is broken — grep the module it exercises for a module-level + `Map`/`Set`/`let` (`grep -n "^const .* = new Map\|^let " src/services/*.ts`) before + looking anywhere else. +- Do not add new cross-request/cross-test process state to a service without also adding + a `__resetXForTesting()`-style export — the pattern above only works because every + stateful module has one. + +--- + +## Web app fails to build after a dependency drift + +**Symptom**: `pnpm --filter web build` (or the `frontend-ci.yml` "Build" step) fails after +a `package.json` change to `apps/web` — a version-resolution error, a type error that +wasn't there before, or a build-time error from a package that built fine yesterday — +even though the diff "only" touched a dependency. + +**Cause**: this is the same nested-workspace mechanism described in +[the lockfile section above](#pnpm-install-fails-on-a-lockfilepackagejson-mismatch), +manifesting as a build failure instead of an install failure. `apps/web/pnpm-workspace.yaml` +makes `apps/web` installable as its own standalone pnpm root, with its own `pnpm-lock.yaml` +and its own resolved `node_modules` tree, entirely separate from the root workspace's +hoisted layout. If dependency resolution happens against the nested lockfile locally (via +`cd apps/web && pnpm install`) but the root workspace's `pnpm-lock.yaml` resolved different +versions the last time it was regenerated, `apps/web`'s effective dependency tree can +silently diverge between "what you built against locally" and "what `frontend-ci.yml` +installs from the root and builds against" — a transitive version bump in one lockfile but +not the other is enough to change type-checking output (`react`/`@types/react` version +skew is a common one, given `apps/web/package.json` pins `react`/`react-dom` at exact +`19.2.4` but `@types/react`/`@types/react-dom` only at `^19` — a nested vs. root +resolution can legitimately land on different patch versions of the types package). + +**Fix**: + +- Same root fix as the lockfile section: install and build from the repo root + (`pnpm install`, then `pnpm --filter web build`), never from inside `apps/web` directly. +- If a build failure only reproduces with a fresh `node_modules` (e.g. after `rm -rf + node_modules && pnpm install` at the root), suspect a resolution difference rather than + a real code regression — compare the installed version of the package named in the + error (`pnpm why --filter web`) against what's pinned in + `apps/web/package.json` and what the root `pnpm-lock.yaml` actually resolved. +- Before debugging a build error as an application bug, confirm `apps/web/pnpm-lock.yaml` + and `apps/web/package-lock.json` haven't drifted from the root `pnpm-lock.yaml` — if + they have, they're stale artifacts (see above) and should not be committed alongside + your change; regenerate the root lockfile instead.