From 8018f10273a3fb8feb74a09edc8c5a64abc51a8a Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 15:38:06 +0200 Subject: [PATCH] feat(server): gate writes outside a transaction with lint:tx The rule that a multi-statement write runs in uow.transactional was written down but nothing held it. scripts/tx-writes.mjs reads every class under src/ with the TypeScript parser and counts, per method, the writes it issues outside a transactional callback: repository calls that build an INSERT, UPDATE or DELETE, calls to methods that write (on the class itself or on a constructor-injected one), and a transaction holding writes as one. A write in a loop counts twice, of two branches only the larger counts, local helpers count where they are called, and a helper whose every call sits inside a transaction is not reported. --explain lists the line of every call that counted. Today's 102 reported methods sit in scripts/tx-writes-baseline.json and may only shrink; most are syncs with network I/O between their writes, plugin lifecycle steps and adapters calling two service writes. A new method fails CI, and an entry above what its method holds fails until --update lowers it, the same ratchet as lint:size and lint:boundaries. Two JSDoc tags mark the deliberate cases with their reason in the tag: @txStandalone for a write that stands on its own whatever the caller does (the audit row, notification sends, the booking price mirror, the assignment journey refresh, the quota retention purge), @txIndependent for writes complete one by one on purpose (the OAuth token retention sweep, the notification claim and its release). Runs in CI after lint:boundaries; documented in server/CLAUDE.md, the root CLAUDE.md and the wiki's dev setup page. --- .github/workflows/test.yml | 6 + CLAUDE.md | 3 +- server/CLAUDE.md | 5 +- server/package.json | 1 + server/scripts/tx-writes-baseline.json | 104 ++++ server/scripts/tx-writes.mjs | 495 ++++++++++++++++++ .../db/repositories/OauthTokens.repository.ts | 3 + .../nest/assignments/assignments.service.ts | 3 + server/src/nest/audit/audit.service.ts | 3 + server/src/nest/budget/budget.service.ts | 4 +- .../nest/google-quota/google-quota.service.ts | 4 + .../notifications/notifications.service.ts | 12 + server/tests/unit/scripts/tx-writes.test.ts | 260 +++++++++ wiki/Development-environment.md | 3 +- 14 files changed, 901 insertions(+), 5 deletions(-) create mode 100644 server/scripts/tx-writes-baseline.json create mode 100644 server/scripts/tx-writes.mjs create mode 100644 server/tests/unit/scripts/tx-writes.test.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 51b139e8f..cf461679b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -260,6 +260,12 @@ jobs: # scripts/import-boundaries-baseline.json and may only shrink. run: cd server && npm run lint:boundaries + - name: Writes outside a transaction + # A method that writes twice does it inside one uow.transactional. + # Today's exceptions sit in scripts/tx-writes-baseline.json and may + # only shrink; @txStandalone/@txIndependent mark the deliberate ones. + run: cd server && npm run lint:tx + - name: Open request schemas may only shrink # Needs the shared build above. Counts open shapes (records of # unknown, passthrough, any) in shared's request schemas. diff --git a/CLAUDE.md b/CLAUDE.md index 5b7f3de80..d817087a6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,6 +45,7 @@ cd client && npm run lint:i18n-keys # every key the client names cd server && npm run lint:boundaries # no new import cycles or cross-domain reach-ins (CI gate) cd server && npm run lint:dialect # SQLite-only SQL spellings may only shrink (CI gate; rules in server/CLAUDE.md) cd server && npm run probe:pg # dialect helpers + the repository statements it reaches against Postgres (CI job; needs TREK_PG_PROBE_URL) +cd server && npm run lint:tx # no new method writing twice outside one transaction (CI gate) cd client && npm run theme:lint # flag styling that bypasses appearance tokens (local only) npm run typecheck --workspace=server # tsc --noEmit (also client/shared; server also has typecheck:tests) ``` @@ -119,7 +120,7 @@ These principles come out of a full-repo audit and shape all new code: ## Quality gates in CI -`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:boundaries`, the client's `lint:format`, `lint:layers`, `lint:offline`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `format:check` on `shared/`. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. +`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:boundaries` and `lint:tx`, the client's `lint:format`, `lint:layers`, `lint:offline`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `format:check` on `shared/`. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. - **`ci-ok` is the required check.** The branch ruleset requires `ci-ok` (plus `lint` and `check-target` from their own workflows), and `ci-ok` needs every job above except the scan. It fails if any of them failed or was cancelled and passes if they passed or were skipped. A new gate job counts only once it is listed in `ci-ok`'s `needs:`. - **What runs is decided by `changes`, not by `on.paths`**: `scripts/ci/changed-areas.mjs` maps the changed files to the `code`, `image` and `deploy` areas (tests: `npm run test:ci-scripts`). A path filter on the workflow would leave `ci-ok` unreported on a docs-only PR and block it. Add a new top-level file the build reads to the right area there. diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 3b7017159..a8f13ba51 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -23,6 +23,7 @@ npm run lint:size # no file in src/ or scripts/ past 1000 lines, longer npm run lint:boundaries # import cycles and cross-domain reach-ins may only shrink (CI gate) npm run lint:dialect # SQLite-only SQL spellings under src/ may only shrink (CI gate) npm run probe:pg # dialect helpers + the repository statements it reaches against Postgres (CI job postgres-probe; needs TREK_PG_PROBE_URL) +npm run lint:tx # methods writing more than once outside one transaction may only shrink (CI gate) npm run contracts:dto-open # open shapes in the createZodDto schemas may only shrink (runs in the unit tests) ``` @@ -30,6 +31,8 @@ npm run contracts:dto-open # open shapes in the createZodDto schemas may only sh **`contracts:dto-open`** (`tests/unit/contracts/dto-open-shapes.test.ts`, wrapped by `scripts/dto-open-shapes.mjs`) finds every `export class X extends createZodDto(...)` under `src/`, imports it and counts the open shapes (loose object, record of unknown, `z.unknown()`/`z.any()`, `z.custom()`) in the schema the Zod pipe validates, with shared's `contracts:open` walk. Each DTO class is held to its entry in `scripts/dto-open-shapes-baseline.json`, and a new one may hold none, whatever its schema is called and wherever it is declared. A `createZodDto` call that is not an exported class, or two DTO classes with one name, fail the check. After narrowing a schema, `npm run contracts:dto-open -- --update` lowers the baseline (it never raises or adds an entry). It needs `shared/` built. +**`lint:tx`** (`scripts/tx-writes.mjs`) reads every class under `src/` and counts, per method, the writes it issues outside a `.transactional(...)` callback: a repository call that builds an INSERT/UPDATE/DELETE, a call to a method that writes (of its own class or of a constructor-injected one), a transaction holding writes as one. A write in a loop counts twice, of two branches only the larger counts, and a helper whose every call sits inside a transaction is not reported. A method at two or more is held to its entry in `scripts/tx-writes-baseline.json`; a new one fails, and so does an entry above what its method holds now, so a fix lands with `npm run lint:tx -- --update` (it never raises or adds an entry). `--explain` lists the line of every call that counted. Two JSDoc tags mark the deliberate cases, each with its reason in the tag text: `@txStandalone` on a method whose write stands on its own whatever its caller does (an audit row, a notification, a non-fatal derived refresh, an idempotent lazy provision) makes it count for nothing at its callers, and `@txIndependent` on a method whose writes are complete one by one on purpose (network I/O between them, a retention sweep in passes, one transaction per item of a batch) keeps it out of the report. + **`lint:size`** (`scripts/size-lint.mjs`) counts a line longer than 120 columns once per 120 columns it spans, so joining lines does not make room. Files that were already longer are held at their length in `scripts/size-baseline.json`; a file that needs to grow past its entry is split by concern instead. An entry above what its file holds now, or for a file that is gone, fails as well, so after a split `npm run lint:size -- --update` lowers the baseline in the same change (it never raises or adds an entry). Single test: `npx vitest run tests/unit/nest/weather.controller.test.ts`, or `npx vitest run -t "returns 401 without cookie"`. @@ -56,7 +59,7 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c - **Thin controller → injectable provider.** Never resurrect a plain function-module service; don't grow god services — split a domain into several modules when it outgrows one class (trips and auth already are). - **Inject, don't reach for module globals.** No imports of the global `db` proxy from `nest/` layers at all any more — the last production importer converted onto a repository, and the export now survives only so ~190 test files' `vi.mock('…/db/database', …)` factories keep typechecking; no new code depending on the `ws` singleton or event-sink globals. There is no `DatabaseService` to inject either — take the repository(ies) you need plus `UnitOfWork` for transactions. Modules must be importable without side effects. -- **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. In `src/db/repositories/` and `src/db/seeders/`, ESLint refuses raw SQL in any spelling: `raw()`, the `sql` tag, and SQL text handed to `execute`/`run`/`all`/`get`. Build the statement with the query builder; `MaintenanceRepository`, `AdminSeeder` and `CategorySeeder` are the named exceptions in `eslint.config.mjs`, and `tests/unit/eslint-rules/restricted-syntax-selectors.test.ts` pins how many SQL strings each still holds, so they can only lose SQL, never gain it. An insert that needs the new id reads it with `.returning('id')`: `InsertResult.insertId` is SQLite's `last_insert_rowid` and undefined on Postgres, and ESLint refuses it in `src/db/repositories/` and `src/db/seeders/`. +- **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). The sequence lives in the service, never in a controller, MCP tool or RPC handler, so every surface calls the one method that holds the transaction (`ReservationsService.createWithCost` is the shape: side effects such as broadcasts come back to the caller and go out after the commit). A position or id taken as `MAX(...) + 1` is read inside the same transaction as the insert that uses it: the transaction holds the connection, so two requests at once cannot read the same MAX. `lint:tx` holds the rest. Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. In `src/db/repositories/` and `src/db/seeders/`, ESLint refuses raw SQL in any spelling: `raw()`, the `sql` tag, and SQL text handed to `execute`/`run`/`all`/`get`. Build the statement with the query builder; `MaintenanceRepository`, `AdminSeeder` and `CategorySeeder` are the named exceptions in `eslint.config.mjs`, and `tests/unit/eslint-rules/restricted-syntax-selectors.test.ts` pins how many SQL strings each still holds, so they can only lose SQL, never gain it. An insert that needs the new id reads it with `.returning('id')`: `InsertResult.insertId` is SQLite's `last_insert_rowid` and undefined on Postgres, and ESLint refuses it in `src/db/repositories/` and `src/db/seeders/`. - **Migrations are append-only.** A migration is identified by its class name in `mikro_orm_migrations`, and files sort by the timestamp in that name. Never rename or delete one that has shipped, and never edit its body; add a new one. No env interpolation in SQL. (The old positional `schema_version` array and the hand-kept `db/migrations.ts` test builder are both gone; the numbered migrations are the only schema history, for tests too.) A new foreign key comes with an index that leads with its column(s), or deleting the parent scans the child table (a partial index counts only when its WHERE clause is `IS NOT NULL` on those columns); `tests/unit/db/foreign-key-indexes.test.ts` fails on one without. - **Every write endpoint validates** through the Zod pipe with a schema from `@trek/shared` — no `Record` + ad-hoc checks. Also verify every referenced id exists and belongs to the same trip. - **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation: no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike, or a literal `timeoutMs` in the options object of `safeFetch`/`safeFetchFollow`; an init spelled `undefined`, `null` or `void 0` counts as none (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull). diff --git a/server/package.json b/server/package.json index 446de7392..1bc13e9e9 100644 --- a/server/package.json +++ b/server/package.json @@ -18,6 +18,7 @@ "lint:size": "node scripts/size-lint.mjs", "lint:boundaries": "node scripts/import-boundaries.mjs", "lint:dialect": "node scripts/dialect-lint.mjs", + "lint:tx": "node scripts/tx-writes.mjs", "contracts:dto-open": "node scripts/dto-open-shapes.mjs", "gen:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts", "check:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts --check", diff --git a/server/scripts/tx-writes-baseline.json b/server/scripts/tx-writes-baseline.json new file mode 100644 index 000000000..cff814139 --- /dev/null +++ b/server/scripts/tx-writes-baseline.json @@ -0,0 +1,104 @@ +{ + "src/db/repositories/DemoRepository.ts#DemoRepository.restoreInstanceApiKeys": 2, + "src/nest/admin/admin.controller.ts#AdminController.updateAddon": 2, + "src/nest/atlas/atlas.service.ts#AtlasService.cacheRegionsInBackground": 2, + "src/nest/atlas/atlas.service.ts#AtlasService.repairStaleRegionCache": 2, + "src/nest/atlas/place-regions-repair.job.ts#PlaceRegionsRepairJob.runOnce": 2, + "src/nest/auth/passkey.service.ts#PasskeyService.passkeyLoginOptions": 2, + "src/nest/auth/passkey.service.ts#PasskeyService.passkeyLoginVerify": 2, + "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterOptions": 2, + "src/nest/auth/passkey.service.ts#PasskeyService.passkeyRegisterVerify": 2, + "src/nest/auth/user-cleanup.service.ts#UserCleanupService.erasePluginUserData": 2, + "src/nest/booking-import/booking-import.service.ts#BookingImportService.confirm": 6, + "src/nest/doc-sync/doc-sync-config.service.ts#DocSyncConfigService.upsertConnection": 3, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.createLink": 4, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.deleteConnection": 2, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.deleteLink": 2, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.listConnections": 2, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.syncNow": 2, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.testConnection": 2, + "src/nest/doc-sync/doc-sync.controller.ts#DocSyncController.upsertConnection": 2, + "src/nest/doc-sync/doc-sync.job.ts#DocSyncJob.tick": 3, + "src/nest/doc-sync/doc-sync.mcp.ts#DocSyncMcp.syncNow": 4, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.applyAction": 2, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.pull": 3, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.push": 2, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.renameRemoteTo": 2, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.resolveConflict": 3, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.runLink": 4, + "src/nest/doc-sync/doc-sync.service.ts#DocSyncService.syncLink": 2, + "src/nest/integrations/airtrail-import.service.ts#AirtrailImportService.importAirtrailFlights": 8, + "src/nest/integrations/airtrail-link.service.ts#AirtrailLinkService.pushReservationToAirtrail": 2, + "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.runAirtrailSync": 2, + "src/nest/integrations/airtrail-sync.service.ts#AirtrailSyncService.syncOwner": 8, + "src/nest/integrations/airtrail.service.ts#AirtrailService.saveSettings": 2, + "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.runSync": 2, + "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncTripWindow": 11, + "src/nest/integrations/dawarich-sync.service.ts#DawarichSyncService.syncUserOnce": 4, + "src/nest/journey/journey.controller.ts#JourneyController.galleryProviderPhotos": 2, + "src/nest/journey/journey.controller.ts#JourneyController.providerPhotos": 2, + "src/nest/journey/journey.controller.ts#JourneyController.uploadEntryPhotos": 4, + "src/nest/journey/journey.mcp.ts#JourneyMcp.addJourneyProviderPhotos": 2, + "src/nest/maps/maps.service.ts#MapsService.getPlaceDetails": 2, + "src/nest/maps/maps.service.ts#MapsService.getPlaceDetailsExpanded": 2, + "src/nest/maps/maps.service.ts#MapsService.getPlacePhoto": 7, + "src/nest/memories/immich.controller.ts#ImmichMemoriesController.putSettings": 2, + "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamCachedThumbnail": 3, + "src/nest/memories/photo-resolver.service.ts#PhotoResolverService.streamPhoto": 2, + "src/nest/memories/synology.service.ts#SynologyService._fetchAllSynologyAlbums": 2, + "src/nest/memories/synology.service.ts#SynologyService._getSynologySession": 2, + "src/nest/memories/synology.service.ts#SynologyService._requestSynologyApi": 3, + "src/nest/memories/synology.service.ts#SynologyService.collectSynologyAlbumSelection": 2, + "src/nest/memories/synology.service.ts#SynologyService.getSynologyAlbumPhotos": 2, + "src/nest/memories/synology.service.ts#SynologyService.listSynologyAlbums": 3, + "src/nest/memories/synology.service.ts#SynologyService.testSynologyConnection": 2, + "src/nest/memories/synology.service.ts#SynologyService.updateSynologySettings": 2, + "src/nest/memories/trek-photo-cache.job.ts#TrekPhotoCacheJob.onApplicationBootstrap": 2, + "src/nest/memories/trek-photo-cache.service.ts#TrekPhotoCacheService.sweepExpired": 2, + "src/nest/memories/unified-memories.service.ts#UnifiedMemoriesService.addTripPhotos": 4, + "src/nest/memories/unified-memories.service.ts#UnifiedMemoriesService.syncImmichAlbum": 2, + "src/nest/memories/unified-memories.service.ts#UnifiedMemoriesService.syncSynologyAlbum": 3, + "src/nest/notifications/push/push.controller.ts#PushController.subscribe": 2, + "src/nest/notifications/reminder-jobs.service.ts#ReminderJobsService.onApplicationBootstrap": 2, + "src/nest/notifications/reminder-jobs.service.ts#ReminderJobsService.todoTick": 4, + "src/nest/notifications/reminder-jobs.service.ts#ReminderJobsService.tripTick": 4, + "src/nest/notifications/transports/web-push.service.ts#WebPushService.deliver": 3, + "src/nest/notifications/transports/web-push.service.ts#WebPushService.deliverTo": 2, + "src/nest/notifications/transports/web-push.service.ts#WebPushService.settle": 2, + "src/nest/oidc/oidc.controller.ts#OidcController.callback": 2, + "src/nest/place-enrichment/place-enrichment.service.ts#PlaceEnrichmentService.collectPhotos": 5, + "src/nest/place-enrichment/place-enrichment.service.ts#PlaceEnrichmentService.enrich": 5, + "src/nest/place-enrichment/place-enrichment.service.ts#PlaceEnrichmentService.readCache": 2, + "src/nest/place-enrichment/place-enrichment.service.ts#PlaceEnrichmentService.storeCandidate": 2, + "src/nest/place-photos/place-photo-cache.job.ts#PlacePhotoCacheJob.onApplicationBootstrap": 2, + "src/nest/place-photos/place-photo-cache.service.ts#PlacePhotoCacheService.sweepOrphans": 2, + "src/nest/places/places.mcp.ts#PlacesMcp.bulkDeletePlaces": 3, + "src/nest/places/places.mcp.ts#PlacesMcp.createAndAssignPlace": 2, + "src/nest/places/places.mcp.ts#PlacesMcp.deletePlace": 2, + "src/nest/places/places.rpc.ts#PlacesRpc.create": 2, + "src/nest/places/places.rpc.ts#PlacesRpc.delete": 2, + "src/nest/places/places.rpc.ts#PlacesRpc.update": 2, + "src/nest/places/places.service.ts#PlacesService.backfillMissingAddresses": 2, + "src/nest/places/places.service.ts#PlacesService.enrichImportedList": 2, + "src/nest/places/places.service.ts#PlacesService.enrichOne": 4, + "src/nest/places/places.service.ts#PlacesService.importGoogleDirections": 2, + "src/nest/places/places.service.ts#PlacesService.importGoogleList": 2, + "src/nest/places/places.service.ts#PlacesService.importNaverList": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.activate": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.deactivateForDisabledAddon": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.deactivateWithDependents": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.link": 3, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.onApplicationBootstrap": 7, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.reload": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.runDrainOnce": 4, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.setOperatorEgressHosts": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.sideload": 2, + "src/nest/plugins/plugin-runtime.service.ts#PluginRuntimeService.update": 3, + "src/nest/plugins/plugins.controller.ts#PluginsController.install": 2, + "src/nest/plugins/plugins.controller.ts#PluginsController.update": 2, + "src/nest/plugins/plugins.controller.ts#PluginsController.updateConfig": 3, + "src/nest/reservations/reservations.controller.ts#ReservationsController.update": 2, + "src/nest/trip-members/trip-members.service.ts#TripMembersService.deleteGuest": 2, + "src/nest/trips/trips.service.ts#TripsService.update": 2, + "src/nest/vacay/vacay.service.ts#VacayService.updatePlan": 2 +} diff --git a/server/scripts/tx-writes.mjs b/server/scripts/tx-writes.mjs new file mode 100644 index 000000000..f6cbad4c7 --- /dev/null +++ b/server/scripts/tx-writes.mjs @@ -0,0 +1,495 @@ +#!/usr/bin/env node +/* + * lint:tx: a method that writes more than once must do it inside one transaction. + * + * A crash or a thrown error between two writes leaves half a change behind (a + * journey without its owner row, a reservation whose budget line was not + * updated). The rule in server/CLAUDE.md is that a multi-statement write runs + * in `await this.uow.transactional(...)`; this script finds the methods that + * do not, as far as a static read of the source can. + * + * HOW IT COUNTS + * Every class under src/ is read with the TypeScript parser. A repository + * method (src/db/repositories/) is a write when it builds an INSERT, UPDATE + * or DELETE (insertInto/updateTable/deleteFrom, nativeInsert/nativeUpdate/ + * nativeDelete, insert/insertMany/upsert/upsertMany, a query builder's + * update/delete, flush, or a raw SQL string that starts with a write verb), + * directly or through another method of its own class. + * + * A method's "write units" are then counted over its body: + * - a call to a write, on a constructor-injected field (`this.trips.x()`) + * or on its own class (`this.x()`), is one unit; + * - a `.transactional(...)` call that holds any write is one unit, however + * many writes it holds; + * - a call to a method of another class that is itself reported counts as + * one unit here (the fault is reported once, where it lives); + * - a write inside a loop, or inside a callback handed to forEach/map/ + * reduce and friends, counts twice (it can run any number of times); + * - of two branches (if/else, a ternary, the cases of a switch, an `if` + * that returns or throws against the rest of its block) only the larger + * one counts, since only one of them runs. + * A method tagged `@txStandalone` in its JSDoc (an audit row, a notification, + * a failure record, an idempotent lazy provision, a non-fatal derived refresh) + * adds no unit to its callers; the tag says why in its text. A method tagged + * `@txIndependent` (writes that are each complete on their own on purpose, + * such as a retention sweep in passes or a sync with network I/O between its + * writes) is not reported itself. + * A method holding two or more units is reported, unless every call to it + * that the script can resolve sits inside a `.transactional(...)` callback or + * in a method that is itself only ever called that way (a helper that runs + * inside its callers' transaction). + * + * WHAT IT CANNOT SEE + * A local helper (`const f = async () => ...` inside the method) is counted + * where it is called, not where it is declared. + * Calls through anything but `this..()`, `this.()` and + * such a local helper (a free function, a value passed in as an argument, a + * dynamic dispatch) are not followed, and a write after network I/O that cannot share a + * transaction with it is still reported: those entries stay in the baseline. + * + * THE RATCHET + * scripts/tx-writes-baseline.json holds today's reported methods with their + * unit count. A method not in it fails, and so does one past its count. An + * entry above what its method holds now, or for a method that is no longer + * reported, fails too until --update lowers or drops it, so a fixed method + * cannot quietly regress. + * + * npm run lint:tx check against the baseline (CI) + * npm run lint:tx -- --update lower the baseline to what the code holds now; + * it never raises or adds an entry + * npm run lint:tx -- --list print every reported method with its count + * npm run lint:tx -- --explain the same, with the line of each call that counted + * + * --dir= points the check at another server root (the unit tests use it). + */ +import { existsSync, readdirSync, readFileSync, realpathSync, statSync, writeFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const require_ = createRequire(import.meta.url); +const ts = require_('typescript'); + +const SOURCE = /\.ts$/; +const SKIP = /\.(?:test|spec|d)\.ts$/; +const REPOSITORY_DIR = 'src/db/repositories/'; + +/** Calls that issue a write wherever a repository makes them. */ +const WRITE_CALLS = new Set([ + 'insertInto', + 'updateTable', + 'deleteFrom', + 'replaceInto', + 'nativeInsert', + 'nativeUpdate', + 'nativeDelete', + 'insert', + 'insertMany', + 'upsert', + 'upsertMany', + 'update', + 'delete', + 'truncate', + 'flush', + 'persistAndFlush', + 'removeAndFlush', +]); + +/** The base-class writes a service may call on a repository field directly. */ +const BASE_WRITES = new Set([ + 'nativeInsert', + 'nativeUpdate', + 'nativeDelete', + 'insert', + 'insertMany', + 'upsert', + 'upsertMany', +]); + +/** + * A method whose JSDoc carries this tag writes something that stands on its own, + * whatever its caller does around it: an audit row, a notification, a failure + * record, an idempotent lazy provision, a non-fatal derived refresh. Its calls + * add no unit to the caller. The method's own body is still counted. + */ +export const STANDALONE_TAG = 'txStandalone'; + +/** + * A method whose JSDoc carries this tag writes in steps that are each complete + * on their own on purpose: a retention sweep that deletes in passes, a sync + * whose network round trips sit between its writes, one transaction per item + * of a batch. It is not reported; the tag's text says why. + */ +export const INDEPENDENT_TAG = 'txIndependent'; + +const RAW_WRITE = /^\s*(?:INSERT|UPDATE|DELETE|REPLACE)\b/i; + +/** Array methods that run their callback once per element. */ +const LOOP_CALLBACKS = new Set(['forEach', 'map', 'flatMap', 'filter', 'reduce', 'some', 'every', 'find', 'findIndex']); + +function walk(dir, files = []) { + for (const name of readdirSync(dir)) { + if (name === 'node_modules') continue; + const path = join(dir, name); + if (statSync(path).isDirectory()) walk(path, files); + else if (SOURCE.test(name) && !SKIP.test(name)) files.push(path); + } + return files; +} + +function nameOf(node) { + return node && node.name && (ts.isIdentifier(node.name) || ts.isPrivateIdentifier(node.name)) ? node.name.text : null; +} + +/** The class a type annotation names: `FooRepository`, `Foo | undefined`, `Readonly` all give `Foo...`. */ +function typeName(type) { + if (!type) return null; + if (ts.isTypeReferenceNode(type)) { + const n = type.typeName; + return ts.isIdentifier(n) ? n.text : n.right.text; + } + if (ts.isUnionTypeNode(type)) { + for (const t of type.types) { + const found = typeName(t); + if (found) return found; + } + } + return null; +} + +/** Every class in the tree, with its methods and the classes its constructor injects. */ +export function collect(serverDir) { + const srcDir = join(serverDir, 'src'); + if (!existsSync(srcDir) || !statSync(srcDir).isDirectory()) { + throw new Error(`src/ does not exist under ${serverDir}: the check would pass without looking at anything`); + } + const classes = new Map(); + for (const path of walk(srcDir)) { + const file = relative(serverDir, path).split('\\').join('/'); + const sf = ts.createSourceFile(path, readFileSync(path, 'utf8'), ts.ScriptTarget.Latest, true); + const visit = (node) => { + if (ts.isClassDeclaration(node) && node.name) { + const cls = { + name: node.name.text, + file, + repository: file.startsWith(REPOSITORY_DIR), + fields: new Map(), + methods: new Map(), + standalone: new Set(), + independent: new Set(), + }; + for (const member of node.members) { + if (ts.isConstructorDeclaration(member)) { + for (const p of member.parameters) { + const field = ts.isIdentifier(p.name) ? p.name.text : null; + const type = typeName(p.type); + if (field && type) cls.fields.set(field, type); + } + } else if (ts.isPropertyDeclaration(member) && member.initializer && ts.isArrowFunction(member.initializer)) { + const method = nameOf(member); + if (method) cls.methods.set(method, member.initializer); + } else if (ts.isPropertyDeclaration(member)) { + const field = nameOf(member); + const type = typeName(member.type); + if (field && type) cls.fields.set(field, type); + } else if (ts.isMethodDeclaration(member) && member.body) { + const method = nameOf(member); + if (method) cls.methods.set(method, member); + const tags = method ? ts.getJSDocTags(member).map((tag) => tag.tagName.text) : []; + if (tags.includes(STANDALONE_TAG)) cls.standalone.add(method); + if (tags.includes(INDEPENDENT_TAG)) cls.independent.add(method); + } + } + // Two classes of one name: keep the first; the second is reached by no field type. + if (!classes.has(cls.name)) classes.set(cls.name, cls); + } + ts.forEachChild(node, visit); + }; + visit(sf); + } + return classes; +} + +/** `this.x` → 'x', `this.#x` → '#x', otherwise null. */ +function thisMember(expr) { + if (ts.isPropertyAccessExpression(expr) && expr.expression.kind === ts.SyntaxKind.ThisKeyword) return expr.name.text; + return null; +} + +function exits(stmt) { + if (!stmt) return false; + if (ts.isReturnStatement(stmt) || ts.isThrowStatement(stmt)) return true; + if (ts.isBlock(stmt)) return stmt.statements.length > 0 && exits(stmt.statements[stmt.statements.length - 1]); + if (ts.isIfStatement(stmt)) return exits(stmt.thenStatement) && exits(stmt.elseStatement); + return false; +} + +export function analyse(classes) { + const units = new Map(); // "Class.method" → number + const inProgress = new Set(); + const callSites = new Map(); // "Class.method" → [{ caller, covered }] + const reading = []; // the "Class.method" whose body is being read + const sites = new Map(); // "Class.method" → the calls that added a unit, for --explain + const frames = []; + const locals = []; // per method read: local `const f = async () => ...` helpers, counted where they are called + const running = new Set(); + + /** Records the call that added a unit to the method being read, and returns that unit. */ + function unit(node, label) { + const sf = node.getSourceFile(); + const line = sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1; + frames[frames.length - 1]?.push(`${line}: ${label}`); + return 1; + } + + /** What `callee` is: { cls, method } on a known class, a base write, or null. */ + function resolve(cls, callee) { + if (!ts.isPropertyAccessExpression(callee)) return null; + const method = callee.name.text; + const own = thisMember(callee); + if (own !== null) return cls.methods.has(own) ? { cls, method: own } : null; + const field = thisMember(callee.expression); + if (field === null) return null; + const target = classes.get(cls.fields.get(field)); + if (!target) return null; + if (target.methods.has(method)) return { cls: target, method }; + if (target.repository && BASE_WRITES.has(method)) return { write: true }; + return null; + } + + function directWrite(cls, call) { + if (!cls.repository) return false; + const callee = call.expression; + if (ts.isPropertyAccessExpression(callee) && WRITE_CALLS.has(callee.name.text)) { + // `this.update(...)` on a repository's own method is resolved as a call instead. + if (callee.expression.kind === ts.SyntaxKind.ThisKeyword && cls.methods.has(callee.name.text)) return false; + return true; + } + const first = call.arguments[0]; + return Boolean(first && (ts.isStringLiteral(first) || ts.isNoSubstitutionTemplateLiteral(first)) && RAW_WRITE.test(first.text)); + } + + /** Write units of `node`, read inside `cls`. `covered` is true inside a transactional callback. */ + function count(cls, node, covered) { + if (!node) return 0; + if (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node)) return 0; + if (ts.isBlock(node) || ts.isSourceFile(node) || ts.isModuleBlock(node)) return statements(cls, node.statements, 0, covered); + if (ts.isIfStatement(node)) { + return count(cls, node.expression, covered) + Math.max(count(cls, node.thenStatement, covered), count(cls, node.elseStatement, covered)); + } + if (ts.isConditionalExpression(node)) { + return count(cls, node.condition, covered) + Math.max(count(cls, node.whenTrue, covered), count(cls, node.whenFalse, covered)); + } + if (ts.isSwitchStatement(node)) { + let max = 0; + for (const clause of node.caseBlock.clauses) max = Math.max(max, statements(cls, clause.statements, 0, covered)); + return count(cls, node.expression, covered) + max; + } + if (ts.isForStatement(node) || ts.isForOfStatement(node) || ts.isForInStatement(node) || ts.isWhileStatement(node) || ts.isDoStatement(node)) { + let head = 0; + if (ts.isForStatement(node)) head = count(cls, node.initializer, covered); + else if (!ts.isDoStatement(node) && !ts.isWhileStatement(node)) head = count(cls, node.expression, covered); + return head + 2 * count(cls, node.statement, covered); + } + if (ts.isCallExpression(node)) return call(cls, node, covered); + if ( + ts.isVariableDeclaration(node) && + ts.isIdentifier(node.name) && + node.initializer && + (ts.isArrowFunction(node.initializer) || ts.isFunctionExpression(node.initializer)) + ) { + locals[locals.length - 1].set(node.name.text, node.initializer.body); + return 0; + } + let total = 0; + ts.forEachChild(node, (child) => { + total += count(cls, child, covered); + }); + return total; + } + + /** A block read statement by statement: an `if` that always leaves competes with the rest of the block. */ + function statements(cls, list, from, covered) { + let total = 0; + for (let i = from; i < list.length; i++) { + const stmt = list[i]; + if (ts.isIfStatement(stmt) && !stmt.elseStatement && exits(stmt.thenStatement)) { + const rest = statements(cls, list, i + 1, covered); + return total + count(cls, stmt.expression, covered) + Math.max(count(cls, stmt.thenStatement, covered), rest); + } + total += count(cls, stmt, covered); + } + return total; + } + + function call(cls, node, covered) { + const callee = node.expression; + const method = ts.isPropertyAccessExpression(callee) ? callee.name.text : null; + const receiver = ts.isPropertyAccessExpression(callee) ? count(cls, callee.expression, covered) : count(cls, callee, covered); + if (method === 'transactional') { + let inner = 0; + frames.push([]); // what runs inside is one unit, not a list of its own + for (const arg of node.arguments) inner += count(cls, arg, true); + frames.pop(); + return receiver + (inner > 0 ? unit(node, 'transactional(...)') : 0); + } + let args = 0; + for (const arg of node.arguments) args += count(cls, arg, covered); + if (method && LOOP_CALLBACKS.has(method)) args *= 2; + const local = ts.isIdentifier(callee) ? callee.text : null; + const helper = local !== null ? locals[locals.length - 1].get(local) : undefined; + if (helper && !running.has(helper)) { + running.add(helper); + const inner = count(cls, helper, covered); + running.delete(helper); + return receiver + args + inner; + } + const text = ts.isPropertyAccessExpression(callee) ? callee.getText().replace(/\s+/g, '') : method; + if (directWrite(cls, node)) return receiver + args + unit(node, text); + const target = resolve(cls, callee); + if (!target) return receiver + args; + if (target.write) return receiver + args + unit(node, text); + if (target.cls.standalone.has(target.method)) return receiver + args; + const key = `${target.cls.name}.${target.method}`; + const list = callSites.get(key) ?? []; + list.push({ caller: reading[reading.length - 1], covered }); + callSites.set(key, list); + return receiver + args + (unitsOf(target.cls, target.method) > 0 ? unit(node, text) : 0); + } + + function unitsOf(cls, method) { + const key = `${cls.name}.${method}`; + if (units.has(key)) return units.get(key); + if (inProgress.has(key)) return 0; // recursion: the outer frame counts the body once + inProgress.add(key); + const fn = cls.methods.get(method); + frames.push([]); + locals.push(new Map()); + reading.push(key); + const n = count(cls, fn.body, false); + reading.pop(); + locals.pop(); + sites.set(key, frames.pop()); + inProgress.delete(key); + units.set(key, n); + return n; + } + + for (const cls of classes.values()) for (const method of cls.methods.keys()) unitsOf(cls, method); + + // Bound to a transaction: called at least once, and every call either sits in a + // transactional callback or comes from a method that is bound itself. Grown from + // nothing to a fixed point, so a cycle of methods calling each other binds none. + const bound = new Set(); + for (let grew = true; grew; ) { + grew = false; + for (const [key, list] of callSites) { + if (bound.has(key) || list.length === 0) continue; + if (list.every((site) => site.covered || bound.has(site.caller))) { + bound.add(key); + grew = true; + } + } + } + + const reported = {}; + const explained = {}; + for (const cls of classes.values()) { + for (const method of cls.methods.keys()) { + const key = `${cls.name}.${method}`; + const n = units.get(key); + if (n < 2) continue; + if (bound.has(key) || cls.independent.has(method)) continue; + reported[`${cls.file}#${key}`] = n; + explained[`${cls.file}#${key}`] = sites.get(key); + } + } + return { found: Object.fromEntries(Object.entries(reported).sort(([a], [b]) => a.localeCompare(b))), sites: explained }; +} + +/** The baseline as committed. Missing or malformed stops the run instead of reading as empty. */ +export function readBaseline(path) { + let parsed; + try { + parsed = JSON.parse(readFileSync(path, 'utf8')); + } catch (err) { + throw new Error(`scripts/tx-writes-baseline.json cannot be read: ${err.message}`); + } + if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) { + throw new Error('scripts/tx-writes-baseline.json must be an object of methods to write counts'); + } + for (const [key, n] of Object.entries(parsed)) { + if (!Number.isInteger(n) || n < 2) { + throw new Error(`scripts/tx-writes-baseline.json: ${key} holds ${JSON.stringify(n)}, expected an integer of at least 2`); + } + } + return parsed; +} + +/** The baseline lowered to what the code holds now. Never raises an entry, never adds one. */ +export function lowerBaseline(baseline, found) { + const lowered = {}; + for (const [key, allowed] of Object.entries(baseline)) { + if (key in found) lowered[key] = Math.min(allowed, found[key]); + } + return Object.fromEntries(Object.entries(lowered).sort(([a], [b]) => a.localeCompare(b))); +} + +export function compare(baseline, found) { + const grown = Object.entries(found).filter(([key, n]) => n > (baseline[key] ?? 1)); + const lowered = lowerBaseline(baseline, found); + const stale = Object.entries(baseline).filter(([key, n]) => lowered[key] !== n); + return { grown, stale }; +} + +function main(argv) { + const dirArg = argv.find((a) => a.startsWith('--dir=')); + const serverDir = dirArg ? resolve(dirArg.slice('--dir='.length)) : fileURLToPath(new URL('..', import.meta.url)); + const baselinePath = join(serverDir, 'scripts', 'tx-writes-baseline.json'); + + const { found, sites } = analyse(collect(serverDir)); + if (argv.includes('--list') || argv.includes('--explain')) { + for (const [key, n] of Object.entries(found)) { + console.log(`${n}\t${key}`); + if (argv.includes('--explain')) for (const site of sites[key]) console.log(`\t\t${site}`); + } + return 0; + } + let baseline = readBaseline(baselinePath); + if (argv.includes('--update')) { + baseline = lowerBaseline(baseline, found); + writeFileSync(baselinePath, JSON.stringify(baseline, null, 2) + '\n'); + } + + const { grown, stale } = compare(baseline, found); + for (const [key, n] of grown) { + const entry = baseline[key]; + console.error( + `FAIL ${key}: ${n} writes outside one transaction` + + (entry ? `, its baseline is ${entry}. ` : '. ') + + 'Wrap the sequence in await this.uow.transactional(...) in the service.', + ); + } + for (const [key, entry] of stale) { + console.error( + `FAIL ${key} is held at ${entry} in scripts/tx-writes-baseline.json, ` + + (key in found ? `but it has ${found[key]} now.` : 'but it is no longer reported.'), + ); + } + if (stale.length) { + console.error('Run npm run lint:tx -- --update to lower the baseline with the change that fixed it.'); + } + console.log(`tx: ${Object.keys(found).length} method(s) with writes outside one transaction, ${Object.keys(baseline).length} held at their baseline`); + return grown.length || stale.length ? 1 : 0; +} + +const isCli = + Boolean(process.argv[1]) && existsSync(process.argv[1]) && realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); +if (isCli) { + try { + process.exitCode = main(process.argv.slice(2)); + } catch (err) { + console.error(`FAIL ${err.message}`); + process.exitCode = 1; + } +} diff --git a/server/src/db/repositories/OauthTokens.repository.ts b/server/src/db/repositories/OauthTokens.repository.ts index 3b22fb48f..5a1086124 100644 --- a/server/src/db/repositories/OauthTokens.repository.ts +++ b/server/src/db/repositories/OauthTokens.repository.ts @@ -352,6 +352,9 @@ export class OauthTokensRepository extends TrekRepository { * live chain is what replay detection walks. Each pass removes the current * leaves, so a whole expired chain goes over a few passes; `maxPasses` * bounds a chain that is still growing. Returns the number deleted. + * + * @txIndependent a retention sweep: every pass leaves a consistent table, and the + * next sweep picks up what a failed one left. */ async deleteExpiredBefore(cutoffIso: string, maxPasses = 50): Promise { let total = 0; diff --git a/server/src/nest/assignments/assignments.service.ts b/server/src/nest/assignments/assignments.service.ts index 048defbb1..64061d4f5 100644 --- a/server/src/nest/assignments/assignments.service.ts +++ b/server/src/nest/assignments/assignments.service.ts @@ -132,6 +132,9 @@ export class AssignmentsService { * Re-mirror the trip's day-assigned places onto every linked journey's skeleton * suggestions. Called after any assignment mutation (create/delete/move/time) so * the journey stays in sync. Non-fatal, like the route's try/catch. + * + * @txStandalone a derived refresh that runs after the assignment write and + * never fails it. */ async reconcile(tripId: string | number, socketId?: string): Promise { try { diff --git a/server/src/nest/audit/audit.service.ts b/server/src/nest/audit/audit.service.ts index ac063c766..d223583d5 100644 --- a/server/src/nest/audit/audit.service.ts +++ b/server/src/nest/audit/audit.service.ts @@ -105,6 +105,9 @@ export class AuditService { * gets a message distinct from every other write failure (below), so an * unwrapped entrypoint stays visible in the log even though it never * throws. + * + * @txStandalone the row records a change that already happened and never + * fails it, so it is not part of the caller's transaction. */ async writeAudit(entry: { userId: number | null; diff --git a/server/src/nest/budget/budget.service.ts b/server/src/nest/budget/budget.service.ts index 625925b85..37f6884b2 100644 --- a/server/src/nest/budget/budget.service.ts +++ b/server/src/nest/budget/budget.service.ts @@ -536,8 +536,7 @@ export class BudgetService { * otherwise a €15 museum on a trip switched to JPY starts reading as ¥15. They * carry no frozen rate, so pinning the currency is all they need. * - * Must run *before* the (synchronous) trip update, while the old currency is - * still in `trips`, and is a no-op when the currency isn't actually changing. + * Runs before the trip update, while the old currency is still in `trips`; a no-op without a change. */ async rebaseTripCurrency( tripId: string | number, @@ -910,6 +909,7 @@ export class BudgetService { * it (#2084): their sum while they share one currency, the first one's total * when they don't (a sum across currencies would be meaningless), and no * price at all once none is linked. One expense gives exactly the old mirror. + * @txStandalone a non-fatal mirror onto the booking, worked out again on every change. */ async resyncReservationPrice(tripId: string | number, reservationId: number, socketId?: string): Promise { const linked = await this.budgetItemsRepo.listLinkedToReservation(tripId, reservationId); diff --git a/server/src/nest/google-quota/google-quota.service.ts b/server/src/nest/google-quota/google-quota.service.ts index 0f60ed2b8..7db0bfae3 100644 --- a/server/src/nest/google-quota/google-quota.service.ts +++ b/server/src/nest/google-quota/google-quota.service.ts @@ -67,6 +67,10 @@ export class GoogleQuotaService { await this.usage.recordCall(todayUtc()); // GQ2 } + /** + * The ceiling and today's count, after dropping days past retention. + * @txStandalone the retention purge stands on its own, whatever the caller wrote. + */ async status(): Promise { await this.usage.purgeExpired(RETENTION_DAYS); // GQ3 const limit = await this.dailyLimit(); diff --git a/server/src/nest/notifications/notifications.service.ts b/server/src/nest/notifications/notifications.service.ts index 7f90d38e4..da8bb7ed3 100644 --- a/server/src/nest/notifications/notifications.service.ts +++ b/server/src/nest/notifications/notifications.service.ts @@ -599,6 +599,11 @@ export class NotificationsService { return await this.notificationsRepo.deleteAllForRecipient(userId); } + /** + * Run a notification's action for the recipient's answer. + * @txIndependent the claim commits on its own before the action runs, so a second + * submit sees it; a failing action releases it again. + */ async respond( notificationId: number, userId: number, @@ -655,6 +660,13 @@ export class NotificationsService { // ── Unified dispatcher (from services/notificationService.ts) ───────────── + /** + * Resolves the recipients of one event and delivers it on every active channel. + * + * @txStandalone a notification reports a change after it is written; email + * and webhook delivery are network I/O, which never runs inside a transaction. + * @txIndependent each recipient's row is written on its own, between deliveries. + */ async send(payload: NotificationPayload): Promise { const { event, actorId, params, scope, targetId, inApp } = payload; const delivery: NotificationDelivery = { attempted: 0, delivered: 0 }; diff --git a/server/tests/unit/scripts/tx-writes.test.ts b/server/tests/unit/scripts/tx-writes.test.ts new file mode 100644 index 000000000..aa9a305c4 --- /dev/null +++ b/server/tests/unit/scripts/tx-writes.test.ts @@ -0,0 +1,260 @@ +/** + * scripts/tx-writes.mjs is the CI gate that keeps a method from issuing two + * writes outside one transaction. A gate nobody tests passes forever once it + * breaks, so this runs the real script as a child process against throwaway + * server roots (--dir): small source trees whose counts are known, plus the + * baseline cases the ratchet has to get right. + */ +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; + +const SCRIPT = path.join(__dirname, '../../../scripts/tx-writes.mjs'); + +interface ExecError { + status: number | null; + stdout: string; + stderr: string; +} + +/** A repository with two writes and a read, the way the real ones spell them. */ +const REPOSITORY = ` +export class FooRepository { + kysely(): any { return null; } + async insertFoo() { return await this.kysely().insertInto('foo').values({}).execute(); } + async deleteFoo() { return await this.nativeDelete({ id: 1 }); } + async findFoo() { return await this.kysely().selectFrom('foo').selectAll().execute(); } +} +`; + +const SERVICE_FILE = 'src/nest/foo/foo.service.ts'; +const KEY = (method: string) => `${SERVICE_FILE}#FooService.${method}`; + +function service(body: string): string { + return ` +export class FooService { + constructor(private readonly foos: FooRepository, private readonly uow: UnitOfWork) {} +${body} +} +`; +} + +const roots: string[] = []; + +function serverRoot(files: Record, baseline: unknown = {}): string { + // null writes no baseline file at all. + const dir = mkdtempSync(path.join(tmpdir(), 'trek-tx-writes-')); + roots.push(dir); + mkdirSync(path.join(dir, 'scripts'), { recursive: true }); + for (const [file, text] of Object.entries({ 'src/db/repositories/Foo.repository.ts': REPOSITORY, ...files })) { + mkdirSync(path.dirname(path.join(dir, file)), { recursive: true }); + writeFileSync(path.join(dir, file), text); + } + if (baseline !== null) { + writeFileSync( + path.join(dir, 'scripts/tx-writes-baseline.json'), + typeof baseline === 'string' ? baseline : JSON.stringify(baseline), + ); + } + return dir; +} + +function run(dir: string, ...args: string[]): { status: number | null; out: string } { + try { + const stdout = execFileSync(process.execPath, [SCRIPT, `--dir=${dir}`, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { status: 0, out: stdout }; + } catch (err) { + const { status, stdout, stderr } = err as ExecError; + return { status, out: `${stdout}${stderr}` }; + } +} + +/** What --list reports for a tree, as { key: units }. */ +function listed(dir: string): Record { + const { status, out } = run(dir, '--list'); + expect(status).toBe(0); + return Object.fromEntries( + out + .split('\n') + .filter((line) => /^\d+\t/.test(line)) + .map((line) => { + const [n, key] = line.split('\t'); + return [key, Number(n)]; + }), + ); +} + +afterEach(() => { + for (const dir of roots.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe('tx-writes.mjs: what counts', () => { + it('TXW-001: two writes in a row are reported, one transaction around them is not', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + async twoWrites() { await this.foos.insertFoo(); await this.foos.deleteFoo(); } + async wrapped() { await this.uow.transactional(async () => { await this.foos.insertFoo(); await this.foos.deleteFoo(); }); } + async oneWriteAndReads() { await this.foos.findFoo(); await this.foos.insertFoo(); await this.foos.findFoo(); }`), + }); + expect(listed(dir)).toEqual({ [KEY('twoWrites')]: 2 }); + }); + + it('TXW-002: a write in a loop or a forEach callback counts twice', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + async loop(ids: number[]) { for (const id of ids) await this.foos.insertFoo(); } + async each(ids: number[]) { ids.forEach(async () => { await this.foos.deleteFoo(); }); }`), + }); + expect(listed(dir)).toEqual({ [KEY('loop')]: 2, [KEY('each')]: 2 }); + }); + + it('TXW-003: of two branches only the larger counts, and an early return competes with the rest', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + async either(flag: boolean) { if (flag) await this.foos.insertFoo(); else await this.foos.deleteFoo(); } + async ternary(flag: boolean) { return flag ? await this.foos.insertFoo() : await this.foos.deleteFoo(); } + async early(flag: boolean) { if (flag) { await this.foos.insertFoo(); return; } await this.foos.deleteFoo(); } + async both(flag: boolean) { if (flag) await this.foos.insertFoo(); await this.foos.deleteFoo(); }`), + }); + expect(listed(dir)).toEqual({ [KEY('both')]: 2 }); + }); + + it('TXW-004: a helper whose every call sits in a transaction is not reported, one called bare is', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + private async pair() { await this.foos.insertFoo(); await this.foos.deleteFoo(); } + private async pairToo() { await this.foos.insertFoo(); await this.foos.deleteFoo(); } + async caller() { await this.uow.transactional(() => this.pair()); } + async inner() { await this.pairToo(); } + async outer() { await this.uow.transactional(() => this.inner()); } + async bare() { await this.pairToo(); }`), + }); + // pairToo has one bare call path (bare), so it is reported; bare itself + // counts it as a single unit, the fault being reported where it lives. + expect(listed(dir)).toEqual({ [KEY('pairToo')]: 2 }); + }); + + it('TXW-005: two calls to methods that are each one transaction are reported at the caller', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + async a() { await this.uow.transactional(async () => { await this.foos.insertFoo(); await this.foos.deleteFoo(); }); } + async b() { await this.foos.insertFoo(); }`), + 'src/nest/foo/foo.controller.ts': ` +export class FooController { + constructor(private readonly foo: FooService) {} + async create() { await this.foo.a(); await this.foo.b(); } + async one() { await this.foo.a(); } +} +`, + }); + expect(listed(dir)).toEqual({ 'src/nest/foo/foo.controller.ts#FooController.create': 2 }); + }); + + it('TXW-006: @txStandalone adds nothing to its callers, @txIndependent is not reported itself', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + /** @txStandalone an audit row. */ + async audit() { await this.foos.insertFoo(); } + async withAudit() { await this.foos.deleteFoo(); await this.audit(); } + /** + * @txIndependent one pass at a time. + */ + async sweep(ids: number[]) { for (const id of ids) await this.foos.deleteFoo(); }`), + }); + expect(listed(dir)).toEqual({}); + }); + + it('TXW-007: a local helper counts where it is called, inside or outside the transaction', () => { + const dir = serverRoot({ + [SERVICE_FILE]: service(` + async inside() { + const both = async () => { await this.foos.insertFoo(); await this.foos.deleteFoo(); }; + await this.uow.transactional(async () => { await both(); }); + } + async outside() { + const both = async () => { await this.foos.insertFoo(); await this.foos.deleteFoo(); }; + await both(); + }`), + }); + expect(listed(dir)).toEqual({ [KEY('outside')]: 2 }); + }); + + it('TXW-008: a repository method with two writes of its own is reported, raw SQL included', () => { + const dir = serverRoot({ + 'src/db/repositories/Bar.repository.ts': ` +export class BarRepository { + async reset() { await this.run('DELETE FROM bar'); await this.run('INSERT INTO bar (id) VALUES (1)'); } + async peek() { await this.run('SELECT * FROM bar'); await this.run('SELECT 1'); } + async run(sql: string) { return sql; } +} +`, + }); + expect(listed(dir)).toEqual({ 'src/db/repositories/Bar.repository.ts#BarRepository.reset': 2 }); + }); +}); + +describe('tx-writes.mjs: the baseline', () => { + const TWO = service(' async twoWrites() { await this.foos.insertFoo(); await this.foos.deleteFoo(); }'); + const LOOP = service(' async twoWrites(ids: number[]) { for (const id of ids) { await this.foos.insertFoo(); await this.foos.deleteFoo(); } }'); + + it('TXW-010: a method not in the baseline fails, naming the fix', () => { + const { status, out } = run(serverRoot({ [SERVICE_FILE]: TWO })); + expect(status).toBe(1); + expect(out).toContain(`FAIL ${KEY('twoWrites')}: 2 writes outside one transaction.`); + expect(out).toContain('await this.uow.transactional'); + }); + + it('TXW-011: a method held at its count passes, one past it fails', () => { + expect(run(serverRoot({ [SERVICE_FILE]: TWO }, { [KEY('twoWrites')]: 2 })).status).toBe(0); + const { status, out } = run(serverRoot({ [SERVICE_FILE]: LOOP }, { [KEY('twoWrites')]: 2 })); + expect(status).toBe(1); + expect(out).toContain('4 writes outside one transaction, its baseline is 2.'); + }); + + it('TXW-012: an entry for a fixed method, or one above its count, fails until --update lowers it', () => { + const fixed = serverRoot( + { [SERVICE_FILE]: service(' async twoWrites() { await this.uow.transactional(async () => { await this.foos.insertFoo(); await this.foos.deleteFoo(); }); }') }, + { [KEY('twoWrites')]: 2 }, + ); + const stale = run(fixed); + expect(stale.status).toBe(1); + expect(stale.out).toContain('but it is no longer reported.'); + + const lower = serverRoot({ [SERVICE_FILE]: TWO }, { [KEY('twoWrites')]: 4 }); + expect(run(lower).out).toContain('but it has 2 now.'); + + expect(run(fixed, '--update').status).toBe(0); + expect(JSON.parse(readFileSync(path.join(fixed, 'scripts/tx-writes-baseline.json'), 'utf8'))).toEqual({}); + expect(run(lower, '--update').status).toBe(0); + expect(JSON.parse(readFileSync(path.join(lower, 'scripts/tx-writes-baseline.json'), 'utf8'))).toEqual({ [KEY('twoWrites')]: 2 }); + }); + + it('TXW-013: --update never adds an entry', () => { + const dir = serverRoot({ [SERVICE_FILE]: TWO }); + expect(run(dir, '--update').status).toBe(1); + expect(JSON.parse(readFileSync(path.join(dir, 'scripts/tx-writes-baseline.json'), 'utf8'))).toEqual({}); + }); + + it('TXW-014: a missing or malformed baseline, or a tree without src/, refuses to run', () => { + expect(run(serverRoot({ [SERVICE_FILE]: TWO }, null)).out).toContain('scripts/tx-writes-baseline.json cannot be read'); + expect(run(serverRoot({}, '[]')).out).toContain('must be an object of methods to write counts'); + expect(run(serverRoot({}, { [KEY('x')]: 1 })).out).toContain('expected an integer of at least 2'); + const empty = mkdtempSync(path.join(tmpdir(), 'trek-tx-writes-')); + roots.push(empty); + const { status, out } = run(empty); + expect(status).toBe(1); + expect(out).toContain('src/ does not exist'); + }); + + it('TXW-015: --explain names the line of every call that counted', () => { + const { out } = run(serverRoot({ [SERVICE_FILE]: TWO }), '--explain'); + expect(out).toContain(`2\t${KEY('twoWrites')}`); + expect(out).toMatch(/\t\t\d+: this\.foos\.insertFoo/); + expect(out).toMatch(/\t\t\d+: this\.foos\.deleteFoo/); + }); +}); diff --git a/wiki/Development-environment.md b/wiki/Development-environment.md index 7202a1045..0516e503e 100644 --- a/wiki/Development-environment.md +++ b/wiki/Development-environment.md @@ -175,6 +175,7 @@ The `@trek/shared` package is the single source of truth for code shared between | `npm run check:plugin-facts` | Verify generated plugin facts (CI gate) | | `npm run lint:size` | No source file grows past its line limit or its baseline entry (CI gate) | | `npm run lint:boundaries` | Import cycles and domain boundaries may not grow past their baseline (CI gate) | +| `npm run lint:tx` | Methods that write more than once outside one transaction may not grow past their baseline (CI gate) | | `npm run format` | Format source | ### Client (`/client`) @@ -235,7 +236,7 @@ Then open a Pull Request from your fork to `liketrek/TREK` targeting the `dev` b - Always branch off from an up-to-date `dev` — run `git fetch upstream && git rebase upstream/dev` before starting new work. - Run tests before pushing: `npm test` at the repo root runs all workspaces. That alone is not the full CI gate. With `shared` built, these are the checks CI runs before any test (`.github/workflows/test.yml` is the source of truth): - in `shared/`: `npm run typecheck && npm run contracts:open` - - in `server/`: `npm run build && npm run typecheck && npm run typecheck:tests && npm run typecheck:scripts && npm run check:entities && npm run check:db-types && npm run lint:check && npm run db:call-graph -- --sync --tx && npm run lint:size && npm run lint:boundaries && npm run lint:dialect && npm run check:plugin-facts` + - in `server/`: `npm run build && npm run typecheck && npm run typecheck:tests && npm run typecheck:scripts && npm run check:entities && npm run check:db-types && npm run lint:check && npm run db:call-graph -- --sync --tx && npm run lint:size && npm run lint:boundaries && npm run lint:tx && npm run lint:dialect && npm run check:plugin-facts` - in `client/`: `npm run typecheck && npm run lint:warnings && npm run lint:pages && npm run lint:rtl && npm run lint:size && npm run lint:format && npm run lint:layers && npm run lint:offline && npm run lint:skips && npm run lint:i18n-keys` - at the root, if you touched translations: `npm run i18n:parity:strict --workspace=shared` - Follow the commit message conventions described in the [[Contributing]] guidelines.