mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-18 21:16:47 +00:00
refactor(scheduling): take a cron lease in one conditional upsert
acquire() made sure a row existed and then raced on a conditional UPDATE, two statements the registrar wrapped in a transaction of its own. lint:tx cannot follow a repository reached through a local variable, so it reported the method. One INSERT ... ON CONFLICT DO UPDATE ... WHERE now takes a free lease, renews the holder's and leaves anyone else's alone, atomically on SQLite and on Postgres, and needs no transaction.
This commit is contained in:
+1
-1
@@ -53,7 +53,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`c
|
||||
- **`src/bootstrap.ts` — `buildApp()`** is the single builder for production and the test harness. Everything on the raw Express instance (global middleware, uploads, discovery metadata, statics, the named `jsonParser`/`urlencodedParser` wrappers) is registered **before `app.init()`**, because Nest throws `NotFoundException` on unmatched routes and nothing after init is reachable. The wrappers exempt `/mcp` so the MCP SDK reads the raw stream — **never `@Body()` a `/mcp` route**. Don't add routes to that pre-init shell; it bypasses every global guard, interceptor and filter.
|
||||
- **Realtime**: `/ws` is a Nest gateway in `src/nest/realtime/`. Inject `RealtimeService`; `src/websocket.ts` is a re-export stub for old call sites. Trip mutations forward `X-Socket-Id` so the originating client doesn't echo its own change.
|
||||
- **MCP**: `src/nest-mcp/` is the decorator/registry layer (`@McpController`/`@Tool`/`@Resource`/`@Prompt`; see its `README.md`); tools live in `<domain>.mcp.ts` files beside each module; the HTTP transport is `src/nest/mcp-transport/`; `src/mcp/` holds process-wide state only (scopes, session manager, boot policy). Every tool invocation writes an audit row through the registry's `onInvoke` seam. MCP tools are adapters over the same services and permission checks as REST — a change on one side lands on the other in the same change.
|
||||
- **Scheduling**: `src/nest/scheduling/` (`CronRegistrarService`) is the one way to schedule a cron. Jobs are `*.job.ts` providers in their owning domain. The registrar refuses to schedule under `NODE_ENV=test` (an integration test pins it). **Never use a `@Cron` decorator**: it bypasses that gate. Every tick first takes the job's row in `scheduler_leases` (`SchedulerLeasesRepository.acquire` in one `UnitOfWork` transaction, where a conditional UPDATE decides the race), renews it while it runs and keeps it for a short settle window afterwards, so two processes on one database never run the same tick; a job needs nothing of its own for this. Claims on individual rows (the reminder jobs' `claimReminder`) stay where they are: the lease decides who runs a tick, the claim what a tick may send.
|
||||
- **Scheduling**: `src/nest/scheduling/` (`CronRegistrarService`) is the one way to schedule a cron. Jobs are `*.job.ts` providers in their owning domain. The registrar refuses to schedule under `NODE_ENV=test` (an integration test pins it). **Never use a `@Cron` decorator**: it bypasses that gate. Every tick first takes the job's row in `scheduler_leases` (`SchedulerLeasesRepository.acquire`, one conditional upsert that decides the race), renews it while it runs and keeps it for a short settle window afterwards, so two processes on one database never run the same tick; a job needs nothing of its own for this. Claims on individual rows (the reminder jobs' `claimReminder`) stay where they are: the lease decides who runs a tick, the claim what a tick may send.
|
||||
- **Plugins** (`src/nest/plugins/`): the child-process runtime (`supervisor/`, `runtime/plugin-host-entry.ts`, `protocol/envelope.ts`) is **intentionally not Nest**; it is the security boundary. `host/rpc-host.ts` registers an RPC method at spawn only if the plugin holds the unlocking permission (registration = authorization). Install-time gates live in `install/`; the manifest's `trek` version range fails closed, and `TREK_PLUGINS_IGNORE_TREK_RANGE` downgrades that to a warning. The protocol tables in `protocol/envelope.ts` (+ `src/plugin-event-sink.ts`) are **generated** into `plugin-sdk/` and `shared/`: run `gen:plugin-facts` after touching them and commit the output. What a plugin gets back is `protocol/output-contract.ts`: every wire method has an entry, and an entity result (`trips.getById`, `todos.list`, `vacay.mine`, ...) is cut down to its published fields in `rpc-host.ts` on the way out, together with every child row it carries whose field is listed in `PLUGIN_ENTITY_NESTED` (a day's `notes_items`, a reservation's `endpoints`). A new column on any whole-row table behind those entities, child tables included, fails `tests/unit/plugins/output-contract.schema.test.ts` until it is listed as published or withheld (the SDK's `PLUGIN_ENTITY_FIELDS` and `PLUGIN_ENTITY_NESTED` are generated from it); a method left as `readModel` must return a value built field by field from named columns. A plugin's own SQL (`host/plugin-data.service.ts`) is held to a row cap, and `query`, `tx` and an `exec` script to a time budget checked between rows and statements (`migrate` is deliberately not timed). `runtime/egress-policy.ts` is a hand-kept mirror guarded by a parity test in `plugin-sdk/`; change both sides together.
|
||||
- **Storage**: `src/nest/storage/` is the upload driver abstraction (local / S3 / mirror behind one interface, with an admin surface and a usage job). Domain services never touch the filesystem or the S3 SDK directly. Where the data lives (`data/`, `uploads/`, backups, logs, key files, the DB file) is resolved in one place, `src/app-config/data-paths.ts`; a provider that reads one injects `DataPathsService`, plain helper modules shared with code outside the container call `resolveDataPaths()` (the list is in `src/app-config/README.md`), and nothing anchors a data path on its own `__dirname` or on `process.cwd()`.
|
||||
- **DB** (`src/db/`): `better-sqlite3`, WAL, FK on. `database.ts` only holds the connection and applies pragmas. It does not build a schema, and importing it opens nothing (the first reader does). The connection's lifecycle belongs to the `DatabaseLifecycle` provider (`nest/database/database-lifecycle.service.ts`): `buildApp()` opens it through `open()` (which binds the ORM to later swaps and runs the schema bootstrap below), the shutdown closes it through `close()`, and a restore or the demo reset swaps the file only through the database backup port, `DATABASE_BACKUP` (`nest/database/database-backup.interface.ts`; the SQLite implementation is `nest/backup/sqlite-database-backup.ts`: `VACUUM INTO` snapshot, close, atomic file swap, `reopen()`). The port works on `DatabaseLifecycle.file`, the one resolved path (`TREK_DB_FILE` or `data/travel.db`), and a snapshot that fails fails the backup rather than archiving the live file. `MaintenanceRepository` is provided only by `MaintenanceModule`'s factory and injected everywhere else; no consumer constructs it. A provider that has to follow every reopen (the demo reset job saves the first demo baseline after a restore's re-bootstrap seeded the example trips) registers with `DatabaseLifecycle.onReopened()`, which runs after the reopen's schema bootstrap; a listener contains its own failures, since one that throws fails the reopen. MikroORM's migrations (`db/migrations/`) are the only schema source, **including for tests**: the hand-kept `schema.ts`/`migrations.ts`/`seeds.ts` builders and the parity test that guarded them are gone. Seeding is the MikroORM seeders in `db/seeders/`, applied by `db/orm.ts` from `buildApp()` before anything reads. An install the retired positional runner migrated (a `schema_version` row, no `mikro_orm_migrations` rows) is baselined by `db/legacy-baseline.ts`: the migrations whose `Legacy migration step N` docstring is ≤ its version are recorded as executed (never replayed) in the same transaction as the migrator run, while the baseline migration itself still runs first, as the legacy runner's `createTables()` did on every boot; anything it cannot map refuses the boot. Before a boot applies pending migrations to a file database that holds anything, `db/pre-migrate-snapshot.ts` writes a `VACUUM INTO` copy next to it (`pre-migrate-<schema>-<time>.db`, `<schema>` being `legacy-<schema_version>` or the last recorded migration's timestamp; the newest `TREK_DB_PRE_MIGRATE_SNAPSHOT_KEEP` stay) and the newer-database refusal names the matching copy; a copy that cannot be written refuses the boot unless `TREK_DB_PRE_MIGRATE_SNAPSHOT=false`. It is off under `NODE_ENV=test` unless a suite sets that variable to true, and a restore's re-bootstrap skips it (the archive is the copy). After the migrator, `db/email-case-index.ts` creates the case-insensitive unique index on `users.email` that its migration had to defer on an install holding two accounts whose emails differ only in case (flag `email_case_index_pending`). `db/orm-driver.ts` binds the ORM to that same connection instead of opening a second one — with `:memory:` under test, a second connection is a second, empty database. Kysely statements (`this.kysely<...>()`) type against `DB` from `db/kysely/db.ts`, narrowed with `Pick<DB, 'trips' | 'days'>`; those files are generated from the migrated schema by `scripts/generate-db-types.ts`, so a migration that adds or changes a column is followed by `npm run gen:db-types`, never by a hand-written table interface. Every read and write goes through a repository in `db/repositories/` (each extends the shared `TrekRepository` base in `db/repositories/_shared/`, which validates the request context before delegating to MikroORM) and, for multi-statement writes, `UnitOfWork.transactional(...)` (`nest/database/unit-of-work.ts`, provided by the global `OrmModule`) — there is no `DatabaseService`/`DatabaseModule` to inject any more. Entrypoints outside an HTTP request — an MCP tool call, a plugin RPC dispatch, a WebSocket message, a cron job, boot-time seeding, a backup restore — open one explicitly with `withRequestContext()` (`nest/database/request-context.ts`), since only the pathless `mikroOrmRequestContext` middleware `buildApp()` mounts (with `@mikro-orm/nestjs`'s own `registerRequestContext` turned off in `app.module.ts`, UPLOADS-P16) forks an `EntityManager` per HTTP request. `NODE_ENV=test` ⇒ isolated `:memory:` DB per vitest worker, opened from a snapshot `tests/global-setup.ts` migrates once per run; `TREK_DB_FILE` ⇒ explicit file DB (Playwright harness).
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import type { SchedulerLeases } from '../entities/SchedulerLeases.entity';
|
||||
import { TrekRepository } from './_shared/trek-repository';
|
||||
import type { DB } from '../kysely/db';
|
||||
|
||||
/**
|
||||
* `scheduler_leases`: which process may run a cron job's tick, and until when
|
||||
@@ -9,22 +10,23 @@ import { TrekRepository } from './_shared/trek-repository';
|
||||
export class SchedulerLeasesRepository extends TrekRepository<SchedulerLeases> {
|
||||
/**
|
||||
* Take (or renew) the lease on `name` for `owner` until `until`, if it is
|
||||
* free at `now`. The decision is one conditional UPDATE, so of two
|
||||
* processes asking at the same moment exactly one gets a changed row; the
|
||||
* insert before it only makes sure there is a row to race on. True when
|
||||
* `owner` holds the lease afterwards. Two statements, so the caller runs it
|
||||
* inside `UnitOfWork.transactional` (CronRegistrarService does).
|
||||
* free at `now`. One statement: the insert takes a lease nobody has held
|
||||
* yet, and on a conflict the update only applies while the lease has run out
|
||||
* or already belongs to `owner`. Of two processes asking at the same moment
|
||||
* exactly one gets a row written. True when `owner` holds the lease afterwards.
|
||||
*/
|
||||
async acquire(name: string, owner: string, now: number, until: number): Promise<boolean> {
|
||||
await this.upsert(
|
||||
{ name, owner, expires_at: 0 },
|
||||
{ onConflictFields: ['name'], onConflictAction: 'ignore' },
|
||||
);
|
||||
const changed = await this.nativeUpdate(
|
||||
{ name, $or: [{ expires_at: { $lte: now } }, { owner }] },
|
||||
{ owner, expires_at: until },
|
||||
);
|
||||
return changed > 0;
|
||||
const result = await this.kysely<Pick<DB, 'scheduler_leases'>>()
|
||||
.insertInto('scheduler_leases')
|
||||
.values({ name, owner, expires_at: until })
|
||||
.onConflict((oc) =>
|
||||
oc
|
||||
.column('name')
|
||||
.doUpdateSet({ owner, expires_at: until })
|
||||
.where((eb) => eb.or([eb('scheduler_leases.expires_at', '<=', now), eb('scheduler_leases.owner', '=', owner)])),
|
||||
)
|
||||
.executeTakeFirst();
|
||||
return (result.numInsertedOrUpdatedRows ?? 0n) > 0n;
|
||||
}
|
||||
|
||||
/** Move the expiry of a lease `owner` still holds; false when somebody else has it now. */
|
||||
|
||||
@@ -7,7 +7,6 @@ import { MikroORM } from '@mikro-orm/core';
|
||||
import { readEnv } from '../../app-config';
|
||||
import { RuntimeEnvService } from '../app-config/runtime-env.service';
|
||||
import { withRequestContext } from '../database/request-context';
|
||||
import { UnitOfWork } from '../database/unit-of-work';
|
||||
import { logError } from '../audit/audit-log.logger';
|
||||
import { traceEntry, wasTraced } from '../audit/entry-trace.logger';
|
||||
import { SchedulerLeases } from '../../db/entities/SchedulerLeases.entity';
|
||||
@@ -162,14 +161,8 @@ export class CronRegistrarService implements OnApplicationShutdown {
|
||||
onTick: () => void | Promise<void>,
|
||||
): Promise<'ran' | 'skipped'> {
|
||||
const leases = orm.em.getRepository(SchedulerLeases);
|
||||
// acquire() is two statements (make sure the row exists, then the
|
||||
// conditional UPDATE that decides the race), so it runs as one unit of
|
||||
// work like every other multi-statement write. A UnitOfWork over the ORM
|
||||
// this tick already has, rather than an injected one: the partial e2e
|
||||
// graphs build this service without the global OrmModule.
|
||||
const acquired = await new UnitOfWork(orm.em).transactional(() =>
|
||||
leases.acquire(name, LEASE_OWNER, Date.now(), Date.now() + LEASE_TTL_MS),
|
||||
);
|
||||
// acquire() is one conditional upsert, so the race needs no transaction.
|
||||
const acquired = await leases.acquire(name, LEASE_OWNER, Date.now(), Date.now() + LEASE_TTL_MS);
|
||||
if (!acquired) return 'skipped';
|
||||
const leaseFailed = (err: unknown) =>
|
||||
logError(`Cron job "${name}": lease update failed: ${err instanceof Error ? err.message : String(err)}`);
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest';
|
||||
import { createSnapshotTestDb } from '../../../helpers/db-mock';
|
||||
import { createTestOrm, type TestOrm } from '../../../helpers/test-orm';
|
||||
import { deleteRows } from '../../../helpers/factories/rows';
|
||||
import { SchedulerLeases } from '../../../../src/db/entities/SchedulerLeases.entity';
|
||||
import type { SchedulerLeasesRepository } from '../../../../src/db/repositories/SchedulerLeases.repository';
|
||||
|
||||
@@ -17,8 +18,8 @@ beforeAll(async () => {
|
||||
t = await createTestOrm(testDb);
|
||||
leases = t.repo(SchedulerLeases);
|
||||
});
|
||||
beforeEach(() => {
|
||||
testDb.prepare('DELETE FROM scheduler_leases').run();
|
||||
beforeEach(async () => {
|
||||
await deleteRows(t, SchedulerLeases);
|
||||
t.clear();
|
||||
});
|
||||
afterAll(async () => {
|
||||
|
||||
Reference in New Issue
Block a user