diff --git a/.gitignore b/.gitignore index 0d7d5fe..9eb60de 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,12 @@ node_modules/ dist/ dist-electron/ release/ -books/ +# Anchored to the repo root on purpose. An unanchored `books/` matches a +# directory named books at ANY depth, which silently swallowed the committed +# migration fixture libraries under server/migrations/__fixtures__/, since +# each one mirrors a real data directory and therefore contains its own +# books/ folder. +/books/ .env *.log .worktrees/ diff --git a/client/features/reader/ReaderPage.tsx b/client/features/reader/ReaderPage.tsx index a976f97..1eb37fe 100644 --- a/client/features/reader/ReaderPage.tsx +++ b/client/features/reader/ReaderPage.tsx @@ -100,6 +100,11 @@ export function ReaderPage({ book, onBack, onQuizReview, onUpdateProfile }: { bufferBoundaryRef, userHasScrolledRef, scrollRef, + // An auth failure is the one generation error the reader can actually + // do something about, so it opens the dialog that fixes it rather than + // leaving the user staring at a retry button that will fail identically + // until the key is corrected. + onAuthFailure: () => setMissingKeyAlert(true), }) // Poll for external chapter updates (e.g. from Claude Code via MCP) diff --git a/client/features/reader/hooks/useGenerationResume.ts b/client/features/reader/hooks/useGenerationResume.ts index 6df1eca..ad2a563 100644 --- a/client/features/reader/hooks/useGenerationResume.ts +++ b/client/features/reader/hooks/useGenerationResume.ts @@ -21,6 +21,13 @@ export interface UseGenerationResumeOptions { bufferBoundaryRef: MutableRefObject userHasScrolledRef: MutableRefObject scrollRef: RefObject + /** + * Called when a generation failed specifically because the provider + * rejected the credentials. The reader opens its existing missing-key + * dialog, which is actionable, instead of leaving the user with a generic + * error they have no way to resolve from the reader. + */ + onAuthFailure?: () => void } /** @@ -59,6 +66,7 @@ export function useGenerationResume({ bufferBoundaryRef, userHasScrolledRef, scrollRef, + onAuthFailure, }: UseGenerationResumeOptions): void { useEffect(() => { let cancelled = false @@ -72,8 +80,21 @@ export function useGenerationResume({ // Check merged generation status if (data.generation.active) { const gen = data.generation - // If already done/error, just use the metadata we already have - if (gen.stage === 'done' || gen.stage === 'error') return + if (gen.stage === 'done') return + + // A generation that already failed before this reader mounted, + // including one the server marked interrupted when it restarted + // mid-chapter. Previously this returned silently, which left the + // reader sitting in whatever phase it had, showing nothing and + // offering no way forward, even though the retry affordance for + // exactly this case already existed one phase away. + if (gen.stage === 'error') { + setGenerationStage(null) + setGenerationError(gen.error ?? 'Generation was interrupted.') + setPhase('generation-error') + if (gen.errorKind === 'auth-failed') onAuthFailure?.() + return + } // Active generation — set phase immediately and connect to stream setGeneratingChapterNum(gen.chapterNum) @@ -107,6 +128,7 @@ export function useGenerationResume({ setGenerationStage(null) setGenerationError(event.message) setPhase('generation-error') + if (event.kind === 'auth-failed') onAuthFailure?.() } }) } diff --git a/docs/plans/refactor/phase-7.md b/docs/plans/refactor/phase-7.md new file mode 100644 index 0000000..356beeb --- /dev/null +++ b/docs/plans/refactor/phase-7.md @@ -0,0 +1,165 @@ +# Phase 7 — Durability, Migrations, and a Typed AI Error Taxonomy + +> **PARALLEL-EXECUTION ADDENDUM (architect, owner-approved):** this phase develops CONCURRENTLY with Phase 6 (E2E), which runs in its own worktree and MERGES FIRST. This phase owns `server/`, `shared/`, and the client seams named below; Phase 6 owns `e2e/` and its three config lines. Consequences for the task list: S10's E2E assertion and S13's journey-h parameterization are DEFERRED to a final integration step after Phase 6 merges, when this branch rebases onto master, runs the full suite, adds those two items, and only then gates and opens its PR. Until then the protection is the unit, contract, and fixture tests plus Phase 6's walking skeleton. Development risk without the full net is accepted by the owner for the schedule. + +## Objective + +Three owner-sanctioned behavior improvements, landed on top of a hexagonal server (Phase 2) and protected by the committed E2E suite (Phase 6). The behavior-preservation rule is lifted only for what is listed under **Sanctioned behavior changes** below; everything else stays byte-identical. Scope is local-single-user: a journal file, not a queue; a startup pass, not a scheduler. + +## 0. Reconciled during execution + +Facts found in the code that contradict what this plan was written against, plus design calls made and ratified by the architect while it ran. The plan below is corrected in place, so the difference between plan and reality is never silent. + +1. **Test baseline.** `pnpm test` on `master` @ `e8a5dd1` is 118 files and 1110 tests, all green. The gate is measured against that plus this phase's additions, and against Phase 6's count once its suite merges. +2. **Every module rename the plan hedged on had already landed.** `server/services/chapter-generation-stream.ts`, `generate-all-chapters.ts`, and `generate-audiobook.ts` all exist under those names. No alternate name needed carrying. +3. **The `.gitignore` gotcha was real.** `git check-ignore -v` on a fixture path exited 0 and named `.gitignore:5:books/`. Anchoring to `/books/` was verified in both directions, the generated library at the repo root is still ignored and the fixtures are now tracked. +4. **`GenerationStatus` is a discriminated union**, `{ active: false } | { active: true, ... }`, so the new `error` field belongs on its active variant rather than on the union. +5. **`generate-chapter` is not a `TaskType`,** and single-chapter generation never enters `BackgroundTasks` at all, it runs through `ChapterGenerationStream`. The journal therefore cannot learn about it from the `BackgroundTasks` decorator. It gets its own six-member `GenerationJobType`, and `ChapterGenerationStream` records and clears its own job. `TaskType` is left untouched, and a type-level test asserts every `TaskType` is a `GenerationJobType`. +6. **The version constants live in `shared/schema-version.ts`, not `shared/domain.ts`.** That module owns both counters, `readSchemaVersion`, `SchemaTooNewError`, and `assertSchemaVersionSupported`. `shared/domain.ts` gains only the two `schemaVersion` fields, which keeps the pure domain module free of version-guard logic. +7. **`schemaVersion` is `.optional()`, never `.default()`.** `z.infer` returns the output type, where a `.default()` field is required, and `BookMeta` is built as an object literal in 44 places across `server/`. A default would have forced the field into all 44. The field stays optional and `fs-book-repository`'s `saveBook` and `saveProfile` stamp the current version on write, so everything the running app writes is current by construction while those 44 sites stay untouched. +8. **Error kinds are kebab-case wire literals** (`auth-failed`, `rate-limited`, `overloaded`, `timed-out`, `network-failed`, `content-refused`, `unknown`), matching `TaskType` and `GenerationStage` rather than the prose casing used in section C below. +9. **A too-new book is reported, not thrown.** `migrate()` records it with reason `too-new` and leaves the file untouched. One downgraded book must not stop a whole library from booting, and `readYaml`'s per-read `SchemaTooNewError` guard still makes any later read of that book fail loudly. Sanctioned change 2 is therefore per-book rather than library-wide. +10. **Audiobook resume cannot skip already narrated chapters, and does not try.** `fs-artifact-store.recoverFromCrash()` deletes a book's whole `audio/` directory whenever `book.m4b` is absent, and `recover-from-crash.ts` then clears `audioGeneratedChapters`. Since the boot order is migrate, then recover, then resume, recovery has already destroyed the evidence a skip would read. Audiobook resume therefore restarts narration from what disk says, which is the honest reading of disk-is-truth. This removed work rather than adding it, the planned `chapterWavExists` addition to `ArtifactStore` is unnecessary and `generate-audiobook.ts` needs no skip logic. +11. **`startServer` must consume the ports and shared services `buildServer` actually registered.** It built a second `createPorts(overrides)`, and `createPorts` returns fresh adapters per call, which `composition-root.test.ts` pins deliberately. Resuming a job through that second set would register it in a `BackgroundTasks` no route can see and seed a `ChapterGenerationStream` no route reads, leaving the tray empty and the reader without its interrupted-chapter panel, both failing silently. The boot block is now an exported `runStartupTasks(ports, log)`, which also makes the boot-order assertion a real unit test rather than one that has to bind a port. +12. **The `BookRepository` contract's round-trip assertion excludes `schemaVersion`.** The real adapter stamps it on write and the in-memory fake returns exactly what it was given. Stamping is a storage-format concern an in-memory fake genuinely does not have, so the guarantee moved to a dedicated real-adapter test asserting the stamp reaches disk, rather than being asserted in a shared contract where it is not universally true. +13. **The failure-scripting surface is a contract-local type, not a widening of `FakeTextGeneration`.** `text-generation.contract.ts` declares its own `FailureScriptable` interface plus a runtime guard, so the shared fake interface never grows the two methods and Phase 6's `ScriptedTextGeneration extends FakeTextGeneration` object literal keeps typechecking unchanged. +14. **Resume never flips a book's status back to `generating`, so the boot-order rationale this plan gave for that step was wrong.** Section B claimed recovery flips `generating` to `reading` and resume legitimately flips it back. Grepping every `status` assignment under `server/services/` shows only `start-book.ts` and `authoring/create-skeleton.ts` ever write `'generating'`, and neither is on a resume path. The ordering is still correct and still load bearing, for a different and verifiable reason: resume's decisions read the same `BookMeta` and audio state that recovery has just reconciled, and audiobook resume in particular depends on recovery having already removed partial audio. +15. **Checkpoints have to be wired on the live path, not only in the resume pass.** The checkpoint calls first landed in `generate-all-chapters.ts` and `generate-audiobook.ts` while the only caller passing a journal was the resume pass, which builds its own instances inside `runStartupTasks`. The two live routes built the same services without one, so on the path a user actually takes the calls were unreachable. Code that cannot run in production is worse than code never written, because it reads as covered. Both routes now pass `ports.jobJournal`. +16. **Two defects were found and fixed that no plan anticipated,** both outside the sanctioned list and both enumerated in the PR body. Issue #50, where the SSE helper listened for `close` on the request rather than the response, and a `fs-job-journal` write chain that could take the whole process down through an unhandled rejection. Each has a test that fails against the code as it was. + + +--- + +## A. Schema versioning + migrations + +**Version scope: two counters, not one.** `schemaVersion` on `meta.yml` versions the whole book directory (meta, toc, progress, feedback, quizzes, summaries, references); `schemaVersion` on `learning-profile.yml` versions the profile independently, because the profile exists with zero books. Rejected a library-wide `library.yml` counter: a book folder is portable (EPUB import creates one, users copy them between machines), and a per-book version travels with it. `meta.yml` is the right host because `listBooks()` already treats its presence as "this directory is a book". + +**Absent field = version 1.** Current libraries are v1 and need no write to be readable. + +**Ship one real migration, not an empty pipeline.** `CURRENT_BOOK_SCHEMA_VERSION = 2`; migration `001-materialize-defaults` writes out the fields the current Zod schemas silently backfill at read time (`tags`, `audioGeneratedChapters`, `skills` on the profile) and stamps the version. That makes what is on disk self-describing instead of implied by whichever `.default()` happened to be in the code, which is the entire argument for schema versioning, and it gives the fixture test something real to assert. + +**Run point: eager, in `startServer`, before `recoverFromCrash()`.** `buildServer()` must stay mutation-free (a P0/P4 invariant, the routes-doc generator and every inject test depend on it), so migration cannot live there. `startServer` already owns the one boot mutation. Order is load-bearing: `recoverFromCrash` reads and writes `BookMeta` through the *current* Zod schema, so an unmigrated book would be skipped by its `listBooks` try/catch. Rejected lazy-per-read: it turns every GET into a potential write, spreads migration across the adapter, and races the MCP server, which writes the same data dir concurrently. + +**Read-side guard, not a lazy migrator.** `readYaml` gains a version pre-check: a `schemaVersion` *greater* than current throws a typed `SchemaTooNewError` (the user downgraded the app) rather than letting Zod mangle it. A book folder dropped in *while the app runs* is migrated at next boot; until then `listBooks` skips it with its existing warning. Documented limitation, not a bug. + +**Code shape.** Pure steps in `server/migrations/book/001-materialize-defaults.ts` (`(raw: unknown) => unknown`, no `fs`), ordered in `server/migrations/book/index.ts`, chained by `server/migrations/migrate.ts::migrateForward(raw, from, to, steps)`. The I/O half is a new **`LibraryMigrator` port** (`migrate(): Promise`) with `server/adapters/fs-library-migrator.ts` as its only adapter, wired in `composition-root.ts` and overridable so E2E boots past it. A port, not a service, because migration reads *below* `BookRepository`, raw YAML that by definition does not validate, and because `ArtifactStore.recoverFromCrash()` already sets the precedent for a startup-mutation method behind a port. Writes go through the existing `writeYaml` temp-then-rename helper in `adapters/fs-paths.ts`. + +**Recipe (goes in `server/migrations/README.md`):** bump the constant, edit the schema in `shared/domain.ts`, add `NNN-name.ts`, commit a fixture library at the previous version, add the round-trip test. A chain-integrity test (`steps` are contiguous and `steps.length === CURRENT - 1`) makes forgetting a step a test failure. + +--- + +## B. Persisted generation jobs + +**Journal: one file per job at `{dataDir}/jobs/{jobId}.yml`.** Chosen over a single journal file because it reuses `writeYaml`'s atomic write exactly, avoids read-modify-write races between the Electron app and the MCP server on a shared data dir, makes completion an `rm`, and confines corruption to one job. Jobs are throwaway state, so the journal is not migrated: an unparseable record is deleted with a warning. + +**Two ports, composed, not one port made async.** Keep `BackgroundTasks` exactly as it is (synchronous `start()` returning a `TaskHandle`, contract test unchanged). Add `server/ports/job-journal.ts` (`record`, `checkpoint`, `clear`, `listInterrupted`, `flush`) and `server/adapters/fs-job-journal.ts`. Then `server/adapters/journalled-background-tasks.ts` decorates the in-memory adapter with the journal. The existing `describeBackgroundTasksContract` runs against the decorated adapter too, which *is* the proof that persistence is transparent. `flush()` exists for tests and shutdown only. + +**Record shape** (`GenerationJobSchema`, `shared/domain.ts` alongside `AudiobookManifest`): `id`, `type`, `bookId`, `bookTitle`, `total`, `startedAt`, `updatedAt`, `status: 'running' | 'interrupted'`, `checkpoint` (discriminated: `{kind:'none'}` | `{kind:'chapters', through:number}` | `{kind:'narration-complete'}`), and `params`, the request body needed to restart (provider, model, quizModel, quizProvider, quizLength; voiceId, speed). No API key is ever journalled; keys stay in the `KeyVault`. + +**Per-type policy.** Every type is journalled uniformly (one code path); only the recovery handler differs. + +| Type | On boot | Why | +|---|---|---| +| `generate-audiobook` | **auto-resume** | Longest job, local TTS so re-running costs nothing but time, already checkpointed per chapter | +| `generate-all` | **auto-resume** | Explicit unfinished user intent; idempotent at chapter granularity | +| `generate-epub`, `generate-cover`, `install-audiobook` | mark `error: "Interrupted by restart"` | Short or cheap; the user re-clicks the button that already exists | +| `generate-chapter` (journal-only, never a tray task) | seed the generation hub with a terminal `error` state | Surfaces through the reader's existing `generation-error` phase | + +**The idempotency rule that makes resume safe: disk is the truth, the checkpoint is advisory.** `generate-all` resume recomputes `startFrom = meta.generatedUpTo + 1` by reading `meta.yml`, never from the journal. Audiobook resume restarts narration from what disk says, which after crash recovery means from the beginning, for the reason recorded in reconciliation item 10. The checkpoint only seeds the progress label. A stale or wrong checkpoint therefore cannot cause a regeneration, and cannot cause a chapter to be skipped that was not really finished. + +**Boot sequence in `startServer`:** `migrate()` then `recoverFromCrash()` then `resumeInterruptedJobs()`. Recovery flips `generating` to `reading`; resume then legitimately flips it back for the jobs it restarts. `TUTOR_NO_AUTO_RESUME=1` disables auto-resume; every resume logs. + +**Zero new UI, verified against real client seams.** Resumed jobs re-enter `BackgroundTasks`, so `GET /api/tasks/stream` and `client/hooks/useBackgroundTasks.ts` render them as ordinary running tasks. The `generate-chapter` case needs one small client change: `GenerationStatus` gains `error?: string` on its active variant (`shared/responses.ts`), and `client/features/reader/hooks/useGenerationResume.ts`, which today early-returns on `gen.stage === 'error'`, sets the generation error and the `generation-error` phase, reusing the retry affordance already in the reader. + +--- + +## C. Typed AI error taxonomy + +**`TextGenerationError`** in `server/ports/text-generation.ts` (precedent: `NotFoundError` on `book-repository.ts`): `kind`, `reason` (human, safe to display), `retryable`, `retryAfterMs?`, `cause`. `TextGenerationErrorKind` is mirrored as a plain string union in `shared/responses.ts` so the client can switch on it without importing zod. + +**Mapping, in `server/adapters/ai-sdk-text-generation.ts`:** + +- `LoadAPIKeyError`, the adapter's own "No API key configured", `APICallError.statusCode` 401/403 → **AuthFailed** +- 429 → **RateLimited**, reading `retry-after` off `APICallError.responseHeaders` +- 500/502/503/529 → **Overloaded**; 408/504 → **TimedOut** +- `TypeError: fetch failed`, or `cause.code` in `ECONNREFUSED|ENOTFOUND|ECONNRESET|EAI_AGAIN|UND_ERR_*` → **NetworkFailed** +- content-filter finish reason, Anthropic `stop_reason: 'refusal'`, `NoObjectGeneratedError` with `finishReason === 'content-filter'` → **ContentRefused** +- everything else, including schema misses (`NoObjectGeneratedError`, `TypeValidationError`, `JSONParseError`, which `experimental_repairText` already covers) → **Unknown** + +**Cancellation is not an error class.** The adapter composes `AbortSignal.any([caller, AbortSignal.timeout()])`. On abort, check the timeout signal: if it fired, throw `TimedOut`; if the *caller's* signal fired, rethrow the original abort untouched, so the `if (signal.aborted) return` paths in generate-all and audiobook still work. + +**Retry inside the adapter.** `RETRY_POLICY` and a pure `nextDelayMs(kind, attempt, retryAfterMs, rng)` in `server/adapters/retry-policy.ts`, unit-tested with an injected rng and an injected `sleep`. RateLimited: honor `retry-after`, else exponential with full jitter, 4 attempts. Overloaded: 3. NetworkFailed: 3, 200ms base. TimedOut: 1 extra. AuthFailed, ContentRefused, and Unknown never retry. Caps: attempts *and* a total-elapsed ceiling. + +**Two things that will bite if missed.** (1) The AI SDK retries internally (`maxRetries`, default 2). Left on, that multiplies to 12 provider calls. Set `maxRetries: 0` on every SDK call in this adapter and gate it with a grep. (2) `streamText` returns an `AsyncIterable`: retrying after a chunk has been yielded duplicates text on screen. Retry only failures raised **before the first chunk is emitted**; once `emitted === true`, propagate. + +**Surfacing.** HTTP error bodies gain `kind` (`http/error-handler.ts` branch); `StreamErrorEvent` in `shared/events.ts` gains `kind?` (purely additive, the client already reads `message`). `client/api/http.ts`'s `ApiError` already carries the parsed body, so `err.body.kind` needs no change to the fetch primitive. The one payoff worth wiring: an `AuthFailed` kind routes the reader to the missing-API-key dialog that already exists, instead of a generic toast. + +--- + +## Implementer tasks + +TDD order is literal: every `test:` commit precedes or accompanies its implementation commit. + +| # | Task | Acceptance | +|---|---|---| +| **S1** | Anchor `.gitignore` `books/` to `/books/`; commit fixture libraries `server/migrations/__fixtures__/{v1-library, v1-profile-only, v1-corrupt-book}` | `git check-ignore` exits 1 on a fixture path; `git status` shows them tracked | +| **S2** | *Tests first.* Pure migration tests: `migrateForward` chain, contiguity and `CURRENT - 1` invariant, `001` step over each fixture | Red against absent modules | +| **S3** | `shared/domain.ts` `schemaVersion` plus `CURRENT_*_SCHEMA_VERSION`; `server/migrations/**` pure steps; `migrate.ts` | S2 green; `pnpm typecheck` clean | +| **S4** | *Tests first.* `LibraryMigrator` contract test: copy fixture to `mkdtemp`, migrate, assert stamped, idempotent on re-run, `SchemaTooNewError` on v99, corrupt book reported not thrown | Red | +| **S5** | `ports/library-migrator.ts` plus fake plus `adapters/fs-library-migrator.ts`; wire into `createPorts`; call from `startServer` **before** `recoverFromCrash` | S4 green; `buildServer` still performs zero writes (assert with a read-only temp dir) | +| **S6** | *Tests first.* `JobJournal` contract test (fake and real over a temp dir) plus `describeBackgroundTasksContract` re-run against the decorated adapter | Red | +| **S7** | `ports/job-journal.ts`, `adapters/fs-job-journal.ts`, `adapters/journalled-background-tasks.ts`; `GenerationJobSchema`; compose in `createPorts` | S6 green; existing BackgroundTasks contract unchanged | +| **S8** | *Tests first.* `resume-interrupted-jobs.test.ts`: generate-all resumes from `generatedUpTo + 1` never earlier; audiobook skips narrated chapters; epub, cover, and install become `error`; `generate-chapter` seeds hub error; `TUTOR_NO_AUTO_RESUME=1` no-ops | Red | +| **S9** | `services/resume-interrupted-jobs.ts`; checkpoint calls in `generate-all-chapters.ts` and `generate-audiobook.ts`; `startServer` hook | S8 green; manual: kill mid-generate-all, restart, no chapter regenerated | +| **S10** | `GenerationStatus.error` plus `useGenerationResume` error phase | E2E: an interrupted chapter shows the existing retry panel | +| **S11** | *Tests first.* Taxonomy contract block on `text-generation.contract.ts`; `retry-policy.test.ts` (pure, injected rng); mapping tests over synthesized `APICallError`s | Red | +| **S12** | `TextGenerationError` plus `retry-policy.ts` plus adapter mapping and retry plus `maxRetries: 0` plus no-retry-after-first-chunk | S11 green; `rg "maxRetries" server/adapters` shows only the explicit 0 | +| **S13** | Fake `scriptFailure(kind)` and `scriptStreamFailure(kind, {afterChunks})`; surface `kind` in error bodies and `StreamErrorEvent`; `AuthFailed` opens the missing-key dialog | Phase 6 journey h parameterized over AuthFailed, RateLimited, and ContentRefused | +| **S14** | ADR drafts in this document; `server/migrations/README.md`; PR body | Every gate below evidenced | + +**Parallel:** {S1 to S5} alongside {S11 to S13}. **Sequential:** S5 then S6 to S10 (B's boot hook sits beside A's). S14 last. + +--- + +## ADR drafts (become 0007 and 0008 in Phase 4) + +**ADR 0007 — Versioned on-disk library with forward-only migrations.** +*Context:* the library is YAML on the filesystem; its shape is implied by whichever Zod `.default()` is in the running build; an older library meeting a newer schema fails at read with no diagnosis. *Decision:* `schemaVersion` on `meta.yml` (per book) and `learning-profile.yml` (global), absent meaning 1; ordered pure forward-only steps; a `LibraryMigrator` port run eagerly in `startServer` before crash recovery; `buildServer` stays mutation-free; committed fixture libraries at old versions are the test corpus. *Consequences:* migration is one auditable pass with a log line, not scattered read-time coercion; schemas can stop leaning on read-time defaults; there is no backward migration, so a downgrade fails loudly instead of corrupting; a book added while the app runs waits for the next boot. *Revisit when:* the library outgrows the filesystem, or a migration ever needs to be reversible. + +**ADR 0008 — Persisted job journal with disk-truth resume.** +*Context:* background tasks and the chapter-generation hub are in-memory; a restart strands long jobs, and crash recovery can only reset statuses. *Decision:* one YAML file per job under `{dataDir}/jobs/`, written with the same temp-then-rename helper; a `JobJournal` port composed onto `BackgroundTasks` by a decorator adapter rather than making the existing port async; auto-resume only for `generate-all` and `generate-audiobook`, everything else marked cleanly retriable; resume recomputes its start point from disk and treats the checkpoint as advisory. *Consequences:* no mid-stream token resume is possible with any provider API, so the semantics are restart-the-step; already-saved chapters and narrated audio are never regenerated; the existing contract test proves the decorator is transparent; two processes on one data dir cannot corrupt a shared journal. *Revisit when:* a job type appears whose steps are not idempotent from disk, or auto-resume is observed spending money the user did not intend. + +--- + +## Sanctioned behavior changes (PR body) + +1. Startup migrates the library forward and stamps `schemaVersion`; the first boot after upgrade rewrites `meta.yml` and `learning-profile.yml`. +2. Reading a book or profile written by a newer build now fails loudly (`SchemaTooNewError`) instead of silently coercing. The failure is per-file rather than library-wide, so one newer book is reported by the migrator and skipped by `listBooks` while the rest of the library still opens. +3. Interrupted `generate-all` and `generate-audiobook` jobs resume automatically at boot; opt out with `TUTOR_NO_AUTO_RESUME=1`. +4. Interrupted epub, cover, and audiobook-install jobs appear in the existing tray as errored and retriable instead of vanishing. +5. An interrupted chapter generation now surfaces in the reader's existing generation-error panel; `GET /api/books/:id` gains `generation.error`. +6. AI failures carry a `kind` in error bodies and SSE error events; retry is now classed (AuthFailed and ContentRefused never retry), and `AuthFailed` opens the existing missing-key dialog. +7. AI SDK internal retries are disabled (`maxRetries: 0`); the adapter owns retry, so total provider calls per failure drop. + +--- + +## Risks + +1. **Migration correctness on a real library** is the highest. Mitigation: fixtures are copies of real book folders, the migrator is idempotent and re-run in the test, and it never deletes. It also writes a one-time backup of `meta.yml` to `meta.yml.bak-v1` on first migration; that costs bytes and turns a bad migration from data loss into a manual restore. +2. **Resume idempotency** is mitigated by disk-is-truth; the S8 test asserting no chapter below `generatedUpTo` is regenerated is the gate. +3. **Retry runaway** is mitigated by SDK `maxRetries: 0` plus attempt caps, a total-elapsed ceiling, and full jitter; the pure policy test asserts the worst-case total delay. +4. **Stream retry duplicating visible text** is mitigated by retrying only before the first emit; a contract test scripts a failure after chunk one and asserts no duplication. +5. **`.gitignore` swallowing fixtures** is confirmed live; S1 fixes and verifies it before anything depends on it. +6. **Boot-order coupling** between migrate, recover, and resume is handled by one ordered block in `startServer` with a comment stating why, and a test that asserts the order. +7. **Phase 6 overlap.** S10 and S13 touch assertions Phase 6 owns, so both are deferred until Phase 6 merges and this branch rebases. + +## Phase gate + +`pnpm test` green (at least the Phase 6 count plus the new contract, migration, resume, and taxonomy tests), `pnpm typecheck` clean, `pnpm lint` zero warnings, the full Phase 6 E2E suite green including journey h per error class, fixture libraries migrate forward and the migrator is idempotent on re-run, manual kill of the app mid-`generate-all` and mid-audiobook followed by a restart verifies resume with zero regeneration, `buildServer()` performs no writes (read-only-temp-dir assertion), `rg "maxRetries" server/adapters` yields only the explicit `0`, and three Electron modes boot. + +### Critical files for implementation +- server/composition-root.ts +- server/index.ts +- server/adapters/ai-sdk-text-generation.ts +- server/adapters/in-memory-background-tasks.ts +- shared/domain.ts diff --git a/e2e/journeys/adaptive-loop.spec.ts b/e2e/journeys/adaptive-loop.spec.ts index 515a613..4e8986f 100644 --- a/e2e/journeys/adaptive-loop.spec.ts +++ b/e2e/journeys/adaptive-loop.spec.ts @@ -27,9 +27,12 @@ import { bookRepository, seedBook } from '../support/seed.js' * response's, filed as github.com/rsml/tutor/issues/50 and out of scope for * this phase. TEST 1 therefore never asserts on-screen chapter 2 content and * never waits on it, it waits on the model's own recorded request instead. - * TEST 2 is the on-screen assertion, quarantined with test.fixme until issue - * 50 lands, so a reader does not mistake TEST 1's silence on rendering for - * an oversight. + * TEST 2 is the on-screen assertion. It was quarantined with test.fixme + * until issue 50 landed, so a reader would not mistake TEST 1's silence on + * rendering for an oversight. Phase 7 fixed issue 50 and TEST 2 now runs. + * TEST 1 is deliberately left as it is rather than folded into TEST 2, + * because asserting the adaptive loop through the model's recorded request + * is a different and stronger claim than asserting pixels. * * See support/journeys/quiz.ts for why every quiz option below is located by * exact text rather than position. generate-quiz.ts shuffles the options @@ -96,11 +99,12 @@ test('quiz answers and chapter feedback shape the chapter-2 prompt the model act expect(chapterTwo?.prompt).toContain(wronglyAnswered.question) }) -test.fixme('renders chapter 2 in the reader once generation finishes', async ({ page, app }) => { - // Quarantined by github.com/rsml/tutor/issues/50. pipeHubToSse closes the - // SSE reply with zero bytes before the browser ever hears "done", so - // chapter 2 never reaches the screen today. Flip back to test() once that - // lands, nothing else here should need to change. +test('renders chapter 2 in the reader once generation finishes', async ({ page, app }) => { + // Was quarantined by github.com/rsml/tutor/issues/50, where pipeHubToSse + // closed the SSE reply with zero bytes before the browser ever heard + // "done", so chapter 2 never reached the screen. Fixed in Phase 7 by + // watching the response for disconnect instead of the request. Nothing + // else in this test had to change, exactly as predicted. await seedBook(app.dataDir, { generatedUpTo: 1 }) await page.goto('/') diff --git a/e2e/journeys/generation-failure.spec.ts b/e2e/journeys/generation-failure.spec.ts index e552549..8cbfc17 100644 --- a/e2e/journeys/generation-failure.spec.ts +++ b/e2e/journeys/generation-failure.spec.ts @@ -4,6 +4,7 @@ import { type GenerationFailureCase, } from '../support/journeys/generation-failure.js' import { test } from '../support/app.js' +import { TextGenerationError } from '@server/ports/text-generation.js' /** * Journey (h) locks in that a scripted provider failure reaches the reader @@ -16,18 +17,18 @@ import { test } from '../support/app.js' * to CASES below. The navigation and assertions in the support module do * not change. * - * The reader-path cases below are `test.fixme`, not `test`, because issue - * #50 means `POST /api/books/:id/generate-next` never delivers an SSE event - * to a real browser at all, success or failure, so no assertion against - * GenerationPanel can pass yet. The wizard case is unaffected and carries - * journey (h)'s claim today. Flip `test.fixme` back to `test` once #50 - * lands. + * The reader-path cases below were quarantined with `test.fixme` while + * issue #50 meant `POST /api/books/:id/generate-next` never delivered an SSE + * event to a real browser at all, success or failure, so no assertion + * against GenerationPanel could pass. Phase 7 fixed that, the listener now + * watches the response rather than the request, so they run. */ /** - * A stand-in for whatever typed error class Phase 7's AI error taxonomy - * introduces. The point of this case is only that a subclass survives - * `throws` all the way to the screen, not this particular shape. + * A stand-in kept from before Phase 7's taxonomy existed. It still earns its + * place: it proves an arbitrary subclass survives `throws` all the way to + * the screen, which is a weaker and more general claim than the + * TextGenerationError cases below make. */ class ScriptedProviderError extends Error { constructor(message: string) { @@ -36,6 +37,35 @@ class ScriptedProviderError extends Error { } } +/** + * One case per error class Phase 7's taxonomy can produce, built with the + * real `TextGenerationError` rather than a stand-in. `reason` is the + * class's `message`, and the SSE error event carries exactly that, so what + * the user reads is what the adapter decided. + * + * The three chosen are the ones whose handling genuinely differs. + * `auth-failed` never retries and routes the reader to the missing-key + * dialog, `rate-limited` retries up to four times, and `content-refused` + * never retries because retrying a refusal just earns another refusal. + */ +const TAXONOMY_CASES: GenerationFailureCase[] = [ + { + name: 'an auth-failed TextGenerationError', + thrown: new TextGenerationError('auth-failed', 'No API key configured for provider: anthropic', false), + expected: /No API key configured for provider: anthropic/, + }, + { + name: 'a rate-limited TextGenerationError', + thrown: new TextGenerationError('rate-limited', 'The provider is rate limiting this key. Try again shortly.', true), + expected: /rate limiting this key/, + }, + { + name: 'a content-refused TextGenerationError', + thrown: new TextGenerationError('content-refused', 'The provider declined to generate this content.', false), + expected: /declined to generate this content/, + }, +] + const CASES: GenerationFailureCase[] = [ { name: 'a plain error with a distinctive message', @@ -47,12 +77,11 @@ const CASES: GenerationFailureCase[] = [ thrown: new ScriptedProviderError('rate limited after 3 retries, provider returned HTTP 529'), expected: /rate limited after 3 retries, provider returned HTTP 529/, }, + ...TAXONOMY_CASES, ] for (const failureCase of CASES) { - // Quarantined behind issue #50 (pipeHubToSse ends /generate-next's SSE - // reply before any event is delivered). Not run until that lands. - test.fixme(`chapter generation failure reaches the reader intact: ${failureCase.name}`, async ({ page, app, model }) => { + test(`chapter generation failure reaches the reader intact: ${failureCase.name}`, async ({ page, app, model }) => { await runGenerationFailureCase({ page, app, model }, failureCase) }) } diff --git a/server/adapters/ai-sdk-text-generation.error-mapping.test.ts b/server/adapters/ai-sdk-text-generation.error-mapping.test.ts new file mode 100644 index 0000000..90af177 --- /dev/null +++ b/server/adapters/ai-sdk-text-generation.error-mapping.test.ts @@ -0,0 +1,214 @@ +import { describe, expect, it } from 'vitest' +import { APICallError, LoadAPIKeyError, NoObjectGeneratedError } from 'ai' +import { TextGenerationError } from '../ports/text-generation.js' +import { mapProviderError } from './ai-sdk-text-generation.js' + +/** + * Unit tests for mapProviderError against synthesized errors, never a real + * network call. APICallError, LoadAPIKeyError, and NoObjectGeneratedError + * are constructed for real, through their actual constructors from the + * installed `ai` package, rather than as hand-shaped plain objects, so + * these tests also catch a future `ai` upgrade that changes what those + * constructors require. + */ + +const NOT_CANCELLED = { timeoutFired: false, callerAborted: false } + +function apiCallError(opts: { + statusCode?: number + responseHeaders?: Record + message?: string +}): APICallError { + return new APICallError({ + message: opts.message ?? 'API call failed', + url: 'https://example.test/v1/messages', + requestBodyValues: {}, + statusCode: opts.statusCode, + responseHeaders: opts.responseHeaders, + }) +} + +function noObjectGeneratedError(finishReason: 'content-filter' | 'error' | 'other'): NoObjectGeneratedError { + return new NoObjectGeneratedError({ + message: 'No object generated', + response: { id: 'resp-1', timestamp: new Date(), modelId: 'claude-test' }, + usage: { + inputTokens: 1, + inputTokenDetails: { noCacheTokens: 1, cacheReadTokens: 0, cacheWriteTokens: 0 }, + outputTokens: 1, + outputTokenDetails: { textTokens: 1, reasoningTokens: 0 }, + totalTokens: 2, + }, + finishReason, + }) +} + +describe('mapProviderError, authentication', () => { + it('maps LoadAPIKeyError to auth-failed, not retryable', () => { + const err = new LoadAPIKeyError({ message: 'OpenAI API key is missing' }) + + const mapped = mapProviderError(err, NOT_CANCELLED) + + expect(mapped).toBeInstanceOf(TextGenerationError) + expect((mapped as TextGenerationError).kind).toBe('auth-failed') + expect((mapped as TextGenerationError).retryable).toBe(false) + }) + + it('maps a plain Error whose message names a missing configured key to auth-failed', () => { + const err = new Error('No API key configured for provider: anthropic') + + const mapped = mapProviderError(err, NOT_CANCELLED) + + expect(mapped).toBeInstanceOf(TextGenerationError) + expect((mapped as TextGenerationError).kind).toBe('auth-failed') + }) + + it('maps a 401 APICallError to auth-failed', () => { + const mapped = mapProviderError(apiCallError({ statusCode: 401 }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('auth-failed') + }) + + it('maps a 403 APICallError to auth-failed', () => { + const mapped = mapProviderError(apiCallError({ statusCode: 403 }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('auth-failed') + }) + + it('passes an already-mapped TextGenerationError through unchanged rather than re-wrapping it', () => { + const original = new TextGenerationError('auth-failed', 'No API key configured for provider: anthropic') + + const mapped = mapProviderError(original, NOT_CANCELLED) + + expect(mapped).toBe(original) + }) +}) + +describe('mapProviderError, rate limiting', () => { + it('maps a 429 to rate-limited and retryable', () => { + const mapped = mapProviderError(apiCallError({ statusCode: 429 }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('rate-limited') + expect((mapped as TextGenerationError).retryable).toBe(true) + }) + + it('reads retry-after in seconds form as milliseconds', () => { + const mapped = mapProviderError( + apiCallError({ statusCode: 429, responseHeaders: { 'retry-after': '2' } }), + NOT_CANCELLED, + ) + + expect((mapped as TextGenerationError).retryAfterMs).toBe(2000) + }) + + it('reads retry-after in HTTP-date form as milliseconds until that date', () => { + const target = new Date(Date.now() + 2000) + const mapped = mapProviderError( + apiCallError({ statusCode: 429, responseHeaders: { 'retry-after': target.toUTCString() } }), + NOT_CANCELLED, + ) + + const retryAfterMs = (mapped as TextGenerationError).retryAfterMs + expect(retryAfterMs).toBeGreaterThan(1000) + expect(retryAfterMs).toBeLessThan(3000) + }) + + it('leaves retryAfterMs undefined when the header is absent', () => { + const mapped = mapProviderError(apiCallError({ statusCode: 429 }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).retryAfterMs).toBeUndefined() + }) +}) + +describe('mapProviderError, provider overload and timeout status codes', () => { + it.each([500, 502, 503, 529])('maps statusCode %i to overloaded, retryable', (statusCode) => { + const mapped = mapProviderError(apiCallError({ statusCode }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('overloaded') + expect((mapped as TextGenerationError).retryable).toBe(true) + }) + + it.each([408, 504])('maps statusCode %i to timed-out, retryable', (statusCode) => { + const mapped = mapProviderError(apiCallError({ statusCode }), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('timed-out') + expect((mapped as TextGenerationError).retryable).toBe(true) + }) +}) + +describe('mapProviderError, network failures', () => { + it('maps a bare TypeError: fetch failed to network-failed', () => { + const mapped = mapProviderError(new TypeError('fetch failed'), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('network-failed') + }) + + it.each(['ECONNREFUSED', 'ENOTFOUND', 'ECONNRESET', 'EAI_AGAIN', 'UND_ERR_CONNECT_TIMEOUT'])( + 'maps an Error whose cause.code is %s to network-failed', + (code) => { + const err = new Error('fetch failed', { cause: { code } }) + + const mapped = mapProviderError(err, NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('network-failed') + }, + ) +}) + +describe('mapProviderError, content refusal', () => { + it('maps NoObjectGeneratedError with finishReason content-filter to content-refused', () => { + const mapped = mapProviderError(noObjectGeneratedError('content-filter'), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('content-refused') + }) + + it('maps NoObjectGeneratedError with any other finishReason to unknown', () => { + const mapped = mapProviderError(noObjectGeneratedError('error'), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('unknown') + }) + + it('maps an error carrying an Anthropic-style stop_reason of refusal to content-refused', () => { + const err = Object.assign(new Error('The model refused to respond'), { stop_reason: 'refusal' }) + + const mapped = mapProviderError(err, NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('content-refused') + }) +}) + +describe('mapProviderError, fallback', () => { + it('maps a bare Error to unknown, not retryable, preserving its message as reason', () => { + const mapped = mapProviderError(new Error('boom'), NOT_CANCELLED) + + expect((mapped as TextGenerationError).kind).toBe('unknown') + expect((mapped as TextGenerationError).retryable).toBe(false) + expect((mapped as TextGenerationError).reason).toBe('boom') + }) +}) + +describe('mapProviderError, cancellation', () => { + it('maps to timed-out when the timeout signal fired', () => { + const err = Object.assign(new Error('This operation was aborted'), { name: 'AbortError' }) + + const mapped = mapProviderError(err, { timeoutFired: true, callerAborted: false }) + + expect((mapped as TextGenerationError).kind).toBe('timed-out') + }) + + it('returns the original error unchanged, by identity, when the caller aborted', () => { + const err = Object.assign(new Error('This operation was aborted'), { name: 'AbortError' }) + + const mapped = mapProviderError(err, { timeoutFired: false, callerAborted: true }) + + expect(mapped).toBe(err) + }) + + it('prefers timed-out when both the timeout and the caller signal fired', () => { + const err = Object.assign(new Error('This operation was aborted'), { name: 'AbortError' }) + + const mapped = mapProviderError(err, { timeoutFired: true, callerAborted: true }) + + expect((mapped as TextGenerationError).kind).toBe('timed-out') + }) +}) diff --git a/server/adapters/ai-sdk-text-generation.test.ts b/server/adapters/ai-sdk-text-generation.test.ts index a51e0c3..a48ec6d 100644 --- a/server/adapters/ai-sdk-text-generation.test.ts +++ b/server/adapters/ai-sdk-text-generation.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest' import type { LanguageModel } from 'ai' import { AI_GENERATION_TIMEOUT_MS } from '../constants.js' import { createFakeKeyVault } from '../ports/key-vault.fake.js' +import { TextGenerationError } from '../ports/text-generation.js' import { composeAbortSignal, resolveModelClient } from './ai-sdk-text-generation.js' // resolveModelClient's return type, LanguageModel, is a union that also @@ -50,6 +51,21 @@ describe('resolveModelClient', () => { .toThrow('No API key configured for provider: anthropic') }) + it('throws a TextGenerationError with kind auth-failed for a missing key, not a plain Error', () => { + const keyVault = createFakeKeyVault() + let caught: unknown + + try { + resolveModelClient(keyVault, 'anthropic', 'claude-sonnet-4-6') + } catch (err) { + caught = err + } + + expect(caught).toBeInstanceOf(TextGenerationError) + expect((caught as TextGenerationError).kind).toBe('auth-failed') + expect((caught as TextGenerationError).retryable).toBe(false) + }) + it('resolves an anthropic model client once a key is present', () => { const keyVault = createFakeKeyVault({ anthropic: 'sk-test-dummy' }) @@ -80,18 +96,18 @@ describe('resolveModelClient', () => { describe('composeAbortSignal', () => { it('is not aborted when neither the caller signal nor the timeout has fired', () => { - const combined = composeAbortSignal(undefined, AI_GENERATION_TIMEOUT_MS) + const { signal } = composeAbortSignal(undefined, AI_GENERATION_TIMEOUT_MS) - expect(combined.aborted).toBe(false) + expect(signal.aborted).toBe(false) }) it('aborts the composed signal when the caller signal aborts', () => { const controller = new AbortController() - const combined = composeAbortSignal(controller.signal, AI_GENERATION_TIMEOUT_MS) + const { signal } = composeAbortSignal(controller.signal, AI_GENERATION_TIMEOUT_MS) - expect(combined.aborted).toBe(false) + expect(signal.aborted).toBe(false) controller.abort() - expect(combined.aborted).toBe(true) + expect(signal.aborted).toBe(true) }) // AbortSignal.timeout()'s countdown runs on the runtime's own native @@ -105,19 +121,36 @@ describe('composeAbortSignal', () => { // AI_GENERATION_TIMEOUT_MS default. it('aborts the composed signal on its own once the internal timeout elapses', async () => { - const combined = composeAbortSignal(undefined, 10) + const { signal } = composeAbortSignal(undefined, 10) - expect(combined.aborted).toBe(false) + expect(signal.aborted).toBe(false) await new Promise((resolve) => setTimeout(resolve, 50)) - expect(combined.aborted).toBe(true) + expect(signal.aborted).toBe(true) }) it('aborts on its own timeout even when a caller signal is provided and never fires', async () => { const controller = new AbortController() - const combined = composeAbortSignal(controller.signal, 10) + const { signal } = composeAbortSignal(controller.signal, 10) await new Promise((resolve) => setTimeout(resolve, 50)) - expect(combined.aborted).toBe(true) + expect(signal.aborted).toBe(true) + }) + + it('exposes the bare timeout signal separately, so a caller can tell a timeout apart from its own cancellation', async () => { + const { timeoutSignal } = composeAbortSignal(undefined, 10) + + expect(timeoutSignal.aborted).toBe(false) + await new Promise((resolve) => setTimeout(resolve, 50)) + expect(timeoutSignal.aborted).toBe(true) + }) + + it('does not abort the timeout signal when only the caller cancels', () => { + const controller = new AbortController() + const { timeoutSignal } = composeAbortSignal(controller.signal, AI_GENERATION_TIMEOUT_MS) + + controller.abort() + + expect(timeoutSignal.aborted).toBe(false) }) }) diff --git a/server/adapters/ai-sdk-text-generation.ts b/server/adapters/ai-sdk-text-generation.ts index 47b6172..6c86575 100644 --- a/server/adapters/ai-sdk-text-generation.ts +++ b/server/adapters/ai-sdk-text-generation.ts @@ -6,6 +6,7 @@ import type { LanguageModel, RepairTextFunction } from 'ai' import { isProviderId, MODEL_REGEX, type ProviderId } from '@shared/provider.js' import { AI_GENERATION_TIMEOUT_MS } from '../constants.js' import type { KeyVault } from '../ports/key-vault.js' +import { TextGenerationError } from '../ports/text-generation.js' import type { GenerateObjectRequest, ModelRef, @@ -14,6 +15,7 @@ import type { TextChunk, TextGeneration, } from '../ports/text-generation.js' +import { nextDelayMs, RETRY_TOTAL_ELAPSED_CEILING_MS } from './retry-policy.js' /** * The real TextGeneration adapter. Every call site listed in @@ -21,7 +23,7 @@ import type { * the `ai` package directly, which makes this the only module in server/ * allowed to import from the `ai` package. * - * It owns three things that were duplicated across those call sites before + * It owns four things that were duplicated across those call sites before * this port existed. The first is resolving a provider + model into a * callable client, lifted from the pre-port `services/model-client.ts`. * The second is the five-minute generation @@ -29,11 +31,33 @@ import type { * `experimental_repairText` logging hook used when a model's JSON output * fails to parse. Previously that hook only ran on the * `/api/books/suggest` route. Every `generateObject` call made through - * this adapter gets it now. + * this adapter gets it now. The fourth is retrying a failed call, through + * {@link mapProviderError} and the policy in ./retry-policy.ts. Every real + * call site used to either not retry at all or hand-roll its own ad hoc + * retry. Every SDK call below turns its own retry count down to zero, so + * the SDK's retrying can never run underneath this adapter's. */ export interface AiSdkTextGenerationDeps { keyVault: KeyVault + /** + * The delay function used between retries. Defaults to a real + * setTimeout-based sleep. Tests inject a no-op so a retry test does not + * actually wait out real delays. + */ + sleep?: (ms: number) => Promise + /** + * The random source `nextDelayMs` uses for full jitter. Defaults to + * Math.random. Tests inject a deterministic function so a retry test's + * delay assertions are exact. + */ + rng?: () => number + /** + * The clock used to measure elapsed retry time against + * `RETRY_TOTAL_ELAPSED_CEILING_MS`. Defaults to Date.now. Tests inject a + * controllable clock. + */ + now?: () => number } /** @@ -60,7 +84,7 @@ export function resolveModelClient( const apiKey = keyVault.get(provider) if (!apiKey) { - throw new Error(`No API key configured for provider: ${provider}`) + throw new TextGenerationError('auth-failed', `No API key configured for provider: ${provider}`, false) } switch (provider) { @@ -87,10 +111,121 @@ export function resolveModelClient( * the runtime's own timer, not the global `setTimeout` that * `vi.useFakeTimers()` intercepts, so no amount of fake-timer-advancing * makes it fire early. + * + * Returns the combined signal and the bare timeout signal separately, + * rather than only the combined one, because {@link mapProviderError} + * needs to tell a timeout apart from a caller cancellation after the + * fact. Checking `timeoutSignal.aborted` on its own answers whether the + * generation timeout fired, independent of whether the caller also + * cancelled. */ -export function composeAbortSignal(signal?: AbortSignal, timeoutMs: number = AI_GENERATION_TIMEOUT_MS): AbortSignal { +export function composeAbortSignal( + signal?: AbortSignal, + timeoutMs: number = AI_GENERATION_TIMEOUT_MS, +): { signal: AbortSignal; timeoutSignal: AbortSignal } { const timeoutSignal = AbortSignal.timeout(timeoutMs) - return signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal + const combined = signal ? AbortSignal.any([signal, timeoutSignal]) : timeoutSignal + return { signal: combined, timeoutSignal } +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null +} + +/** + * Reads the retry-after response header off a mapped API call error's + * responseHeaders, in either form the HTTP spec allows. A plain integer + * string is delta-seconds, converted to milliseconds. Anything else is + * parsed as an HTTP-date and converted to the milliseconds remaining + * until that date. Returns undefined when the header is absent or + * unparseable. + */ +function parseRetryAfterMs(headers: Record | undefined): number | undefined { + const raw = headers?.['retry-after'] + if (typeof raw !== 'string') return undefined + const seconds = Number(raw) + if (Number.isFinite(seconds)) return seconds * 1000 + const target = new Date(raw).getTime() + return Number.isNaN(target) ? undefined : target - Date.now() +} + +function isNetworkFailure(err: unknown): boolean { + if (err instanceof TypeError && err.message === 'fetch failed') return true + const cause = err instanceof Error ? err.cause : undefined + const code = isRecord(cause) && typeof cause.code === 'string' ? cause.code : undefined + if (code === undefined) return false + return code === 'ECONNREFUSED' || code === 'ENOTFOUND' || code === 'ECONNRESET' || code === 'EAI_AGAIN' || code.startsWith('UND_ERR_') +} + +/** + * Maps any error thrown by the `ai` package, or by resolveModelClient, + * onto the TextGenerationError taxonomy this adapter's callers retry + * against. Detection is structural, reading properties like `statusCode` + * and `responseHeaders` off the error, rather than importing the `ai` + * package's own error classes and checking `instanceof` them. The `ai` + * package has changed its exact error class hierarchy across versions + * before, and a structural check survives that, since the wire shape + * those classes wrap, an HTTP status code and its headers, changes far + * less often than the classes do. + * + * `ctx.timeoutFired` and `ctx.callerAborted` both come from the two + * signals composeAbortSignal returns. Timeout wins when both fired, so a + * caller who cancels a task and then also sees it blow past the + * generation timeout still gets told generation itself was slow, not just + * that something upstream gave up. When only the caller aborted, `err` is + * returned unchanged, by identity, since the `if (signal.aborted) return` + * paths in generate-all and audiobook generation depend on an untouched + * abort propagating. + */ +export function mapProviderError(err: unknown, ctx: { timeoutFired: boolean; callerAborted: boolean }): unknown { + const message = isRecord(err) && typeof err.message === 'string' ? err.message : undefined + const reason = message ?? 'The AI provider returned an unrecognized error.' + + if (ctx.timeoutFired) { + return new TextGenerationError('timed-out', reason, true, { cause: err }) + } + if (ctx.callerAborted) { + return err + } + if (err instanceof TextGenerationError) { + return err + } + + if (message?.includes('No API key configured for provider')) { + return new TextGenerationError('auth-failed', reason, false, { cause: err }) + } + + const name = isRecord(err) && typeof err.name === 'string' ? err.name : undefined + if (name?.includes('LoadAPIKeyError')) { + return new TextGenerationError('auth-failed', reason, false, { cause: err }) + } + + const statusCode = isRecord(err) && typeof err.statusCode === 'number' ? err.statusCode : undefined + if (statusCode === 401 || statusCode === 403) { + return new TextGenerationError('auth-failed', reason, false, { cause: err }) + } + if (statusCode === 429) { + const responseHeaders = isRecord(err) && isRecord(err.responseHeaders) ? err.responseHeaders : undefined + return new TextGenerationError('rate-limited', reason, true, { retryAfterMs: parseRetryAfterMs(responseHeaders), cause: err }) + } + if (statusCode === 500 || statusCode === 502 || statusCode === 503 || statusCode === 529) { + return new TextGenerationError('overloaded', reason, true, { cause: err }) + } + if (statusCode === 408 || statusCode === 504) { + return new TextGenerationError('timed-out', reason, true, { cause: err }) + } + + if (isNetworkFailure(err)) { + return new TextGenerationError('network-failed', reason, true, { cause: err }) + } + + const finishReason = isRecord(err) && typeof err.finishReason === 'string' ? err.finishReason : undefined + const stopReason = isRecord(err) && typeof err.stop_reason === 'string' ? err.stop_reason : undefined + if (finishReason === 'content-filter' || stopReason === 'refusal') { + return new TextGenerationError('content-refused', reason, false, { cause: err }) + } + + return new TextGenerationError('unknown', reason, false, { cause: err }) } const repairText: RepairTextFunction = async ({ text, error }) => { @@ -102,37 +237,101 @@ const repairText: RepairTextFunction = async ({ text, error }) => { return null } +function realSleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + export function createAiSdkTextGeneration(deps: AiSdkTextGenerationDeps): TextGeneration { + const sleep = deps.sleep ?? realSleep + const rng = deps.rng ?? Math.random + const now = deps.now ?? Date.now + function resolveModel(ref: ModelRef): LanguageModel { return resolveModelClient(deps.keyVault, ref.provider, ref.model) } function streamText(req: StreamTextRequest): AsyncIterable { - const model = resolveModel(req.model) - const abortSignal = composeAbortSignal(req.signal) - // The AI SDK's Prompt type is a `{ prompt } | { messages }` union. A - // single object literal can't carry both keys at once and still - // satisfy it, even though at most one of req.prompt/req.messages is - // ever set. Branching keeps each call site's object literal to - // exactly one of the two. - const result = req.messages - ? aiStreamText({ model, system: req.system, messages: req.messages, abortSignal }) - : aiStreamText({ model, system: req.system, prompt: req.prompt ?? '', abortSignal }) - return result.textStream + // Lazy: nothing in generate() below runs until the caller starts + // iterating. That laziness is what makes retrying a pre-first-chunk + // failure possible, and it is safe here because every real call site + // iterates immediately rather than holding onto the AsyncIterable + // unread. Checked with `rg '\.streamText\(' server -g '!*.test.ts'`: + // revise-toc.ts:48, start-book.ts:114, create-book.ts:58, and + // generate-next-chapter.ts:71 all do `for await (const chunk of + // deps.ai.streamText({...` directly. explain-passage.ts:54 is the one + // indirection, it returns the AsyncIterable straight through without + // iterating, but its only caller, routes/chat.ts:18, does `for await + // (const chunk of explainPassage(...))` with nothing in between. This + // also converges the adapter with the fake, which has always been + // lazy the same way. + async function* generate(): AsyncGenerator { + const { signal: abortSignal, timeoutSignal } = composeAbortSignal(req.signal) + const startedAt = now() + let emitted = false + + for (let attempt = 1; ; attempt++) { + try { + const model = resolveModel(req.model) + // The AI SDK's Prompt type is a `{ prompt } | { messages }` + // union. A single object literal can't carry both keys at once + // and still satisfy it, even though at most one of + // req.prompt/req.messages is ever set. Branching keeps each + // call site's object literal to exactly one of the two. + // + // This adapter owns retry, so the SDK's own retry count is + // zeroed out below. The SDK's default of 2 would multiply + // provider calls underneath nextDelayMs's own attempt count. + const result = req.messages + ? aiStreamText({ model, system: req.system, messages: req.messages, abortSignal, maxRetries: 0 }) + : aiStreamText({ model, system: req.system, prompt: req.prompt ?? '', abortSignal, maxRetries: 0 }) + + for await (const chunk of result.textStream) { + emitted = true + yield chunk + } + return + } catch (err) { + const mapped = mapProviderError(err, { timeoutFired: timeoutSignal.aborted, callerAborted: req.signal?.aborted ?? false }) + // Retrying after a chunk has already reached the client would + // duplicate visible text on screen, so once emitted is true the + // mapped error always propagates, no matter what kind it is. + if (emitted || !(mapped instanceof TextGenerationError)) throw mapped + const delay = nextDelayMs(mapped.kind, attempt, { retryAfterMs: mapped.retryAfterMs, rng }) + if (delay === null || now() - startedAt + delay > RETRY_TOTAL_ELAPSED_CEILING_MS) throw mapped + await sleep(delay) + } + } + } + + return generate() } async function generateObject(req: GenerateObjectRequest): Promise { - const result = await aiGenerateObject({ - model: resolveModel(req.model), - schema: req.schema, - prompt: req.prompt, - system: req.system, - schemaName: req.schemaName, - schemaDescription: req.schemaDescription, - abortSignal: composeAbortSignal(req.signal), - experimental_repairText: repairText, - }) - return result.object + const { signal: abortSignal, timeoutSignal } = composeAbortSignal(req.signal) + const startedAt = now() + + for (let attempt = 1; ; attempt++) { + try { + const result = await aiGenerateObject({ + model: resolveModel(req.model), + schema: req.schema, + prompt: req.prompt, + system: req.system, + schemaName: req.schemaName, + schemaDescription: req.schemaDescription, + abortSignal, + experimental_repairText: repairText, + maxRetries: 0, // This adapter owns retry. See streamText's comment on why. + }) + return result.object + } catch (err) { + const mapped = mapProviderError(err, { timeoutFired: timeoutSignal.aborted, callerAborted: req.signal?.aborted ?? false }) + if (!(mapped instanceof TextGenerationError)) throw mapped + const delay = nextDelayMs(mapped.kind, attempt, { retryAfterMs: mapped.retryAfterMs, rng }) + if (delay === null || now() - startedAt + delay > RETRY_TOTAL_ELAPSED_CEILING_MS) throw mapped + await sleep(delay) + } + } } function runToolConversation(req: RunToolConversationRequest): AsyncIterable { @@ -143,24 +342,39 @@ export function createAiSdkTextGeneration(deps: AiSdkTextGenerationDeps): TextGe ]), ) + const { signal: abortSignal, timeoutSignal } = composeAbortSignal(req.signal) + const result = aiStreamText({ model: resolveModel(req.model), system: req.system, messages: req.messages, tools, stopWhen: stepCountIs(req.maxSteps), - abortSignal: composeAbortSignal(req.signal), + abortSignal, + maxRetries: 0, // This adapter owns retry, but see the comment below on why this method never uses it. }) // Kicking off aiStreamText() above happens eagerly, the moment - // runToolConversation() is called, matching how streamText() behaves - // in every real call site today. Only the text-delta filtering below - // is lazy, deferred until the caller iterates the returned value. + // runToolConversation() is called. Unlike streamText() above, this + // method has no retry loop, so there is no laziness to gain from + // deferring the call. A tool's execute() runs as a side effect + // partway through the SDK stepping through the conversation, so by + // the time an error reaches the catch block below, that side effect + // has already happened. Retrying would call execute() again and + // repeat it. Errors are still mapped through mapProviderError, so a + // caller sees the same TextGenerationError taxonomy as the other two + // methods, only the retry loop is missing. Only the text-delta + // filtering below is lazy, deferred until the caller iterates the + // returned value. async function* textDeltas(): AsyncGenerator { - for await (const part of result.fullStream) { - if (part.type === 'text-delta') { - yield { type: 'text', text: part.text } + try { + for await (const part of result.fullStream) { + if (part.type === 'text-delta') { + yield { type: 'text', text: part.text } + } } + } catch (err) { + throw mapProviderError(err, { timeoutFired: timeoutSignal.aborted, callerAborted: req.signal?.aborted ?? false }) } } diff --git a/server/adapters/fs-book-repository.test.ts b/server/adapters/fs-book-repository.test.ts index c5a91e0..c0a593a 100644 --- a/server/adapters/fs-book-repository.test.ts +++ b/server/adapters/fs-book-repository.test.ts @@ -1,7 +1,10 @@ -import { afterEach } from 'vitest' -import { mkdtemp, rm } from 'node:fs/promises' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { mkdtemp, rm, mkdir, readFile, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { parse as parseYaml, stringify as stringifyYaml } from 'yaml' +import type { BookMeta, LearningProfile } from '@shared/domain.js' +import { SchemaTooNewError } from '@shared/schema-version.js' import type { BookRepository } from '../ports/book-repository.js' import { describeBookRepositoryContract } from '../ports/book-repository.contract.js' import { createFsBookRepository } from './fs-book-repository.js' @@ -24,3 +27,126 @@ afterEach(async () => { }) describeBookRepositoryContract('real fs adapter', makeSubject) + +// --- Schema version stamping on write, and guarding on read. Whitebox, +// not part of the shared BookRepository contract, since the fake has +// nothing on disk to stamp or guard. --- + +function makeBookMeta(overrides: Partial = {}): BookMeta { + return { + id: 'book-1', + title: 'Test Book', + prompt: 'Teach me testing', + status: 'reading', + totalChapters: 3, + generatedUpTo: 1, + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', + tags: [], + audioGeneratedChapters: [], + ...overrides, + } +} + +function makeProfile(overrides: Partial = {}): LearningProfile { + return { + style: 'mental models', + identity: 'developer', + preferences: { + explainComplexTermsSimply: true, + codeExamples: true, + realWorldAnalogies: true, + includeRecaps: true, + includeSummaries: true, + visualDescriptions: false, + depthLevel: 3, + pacePreference: 3, + metaphorDensity: 3, + narrativeStyle: 3, + humorLevel: 3, + formalityLevel: 3, + }, + skills: [], + ...overrides, + } +} + +describe('schema version stamping and guarding', () => { + let dataDir: string + let repo: BookRepository + + beforeEach(async () => { + dataDir = await mkdtemp(join(tmpdir(), 'tutor-fs-book-repository-version-test-')) + repo = createFsBookRepository({ dataDir }) + }) + + afterEach(async () => { + await rm(dataDir, { recursive: true, force: true }) + }) + + it('stamps a saved book meta.yml with the current schema version', async () => { + await repo.saveBook(makeBookMeta({ id: 'stamped-book' })) + + const raw = parseYaml(await readFile(join(dataDir, 'books', 'stamped-book', 'meta.yml'), 'utf-8')) as { + schemaVersion?: number + } + expect(raw.schemaVersion).toBe(2) + }) + + it('stamps a saved learning-profile.yml with the current schema version', async () => { + await repo.saveProfile(makeProfile()) + + const raw = parseYaml(await readFile(join(dataDir, 'books', 'learning-profile.yml'), 'utf-8')) as { + schemaVersion?: number + } + expect(raw.schemaVersion).toBe(2) + }) + + it('rejects getBook with SchemaTooNewError when meta.yml is newer than this build supports', async () => { + const dir = join(dataDir, 'books', 'too-new-book') + await mkdir(dir, { recursive: true }) + await writeFile( + join(dir, 'meta.yml'), + stringifyYaml({ ...makeBookMeta({ id: 'too-new-book' }), schemaVersion: 99 }), + 'utf-8', + ) + + let thrown: unknown + try { + await repo.getBook('too-new-book') + } catch (err) { + thrown = err + } + expect(thrown).toBeInstanceOf(SchemaTooNewError) + expect((thrown as SchemaTooNewError).found).toBe(99) + expect((thrown as SchemaTooNewError).supported).toBe(2) + }) + + it('still resolves getBook when meta.yml has no schemaVersion at all, since that is simply the oldest possible file', async () => { + const dir = join(dataDir, 'books', 'unmigrated-book') + await mkdir(dir, { recursive: true }) + await writeFile(join(dir, 'meta.yml'), stringifyYaml(makeBookMeta({ id: 'unmigrated-book' })), 'utf-8') + + await expect(repo.getBook('unmigrated-book')).resolves.toMatchObject({ id: 'unmigrated-book' }) + }) + + it('skips a too-new book in listBooks rather than throwing', async () => { + await repo.saveBook(makeBookMeta({ id: 'good-book' })) + + // listBooks does not pass maxSchemaVersion through to readYaml (see + // fs-book-repository.ts), so what has to catch this book is the same + // pre-existing per-book try/catch that already skips any book it + // cannot parse. The invalid status stands in for a field shape only a + // newer build would write. + const badDir = join(dataDir, 'books', 'too-new-book') + await mkdir(badDir, { recursive: true }) + await writeFile( + join(badDir, 'meta.yml'), + stringifyYaml({ ...makeBookMeta({ id: 'too-new-book' }), schemaVersion: 99, status: 'not-a-real-status' }), + 'utf-8', + ) + + const books = await repo.listBooks() + expect(books.map((b) => b.id)).toEqual(['good-book']) + }) +}) diff --git a/server/adapters/fs-book-repository.ts b/server/adapters/fs-book-repository.ts index b656b69..5552ca5 100644 --- a/server/adapters/fs-book-repository.ts +++ b/server/adapters/fs-book-repository.ts @@ -21,6 +21,7 @@ import { type ReferenceManifest, } from '@shared/domain.js' import type { SkillProgress } from '@shared/responses.js' +import { CURRENT_BOOK_SCHEMA_VERSION, CURRENT_PROFILE_SCHEMA_VERSION } from '@shared/schema-version.js' import type { BookRepository } from '../ports/book-repository.js' import { booksDir, bookDir, padChapter, readYaml, writeYaml } from './fs-paths.js' @@ -95,12 +96,21 @@ export function createFsBookRepository(opts: { dataDir: string }): BookRepositor // --- Learning profile --- async getProfile(): Promise { - return readYaml(join(booksDir(dataDir), 'learning-profile.yml'), LearningProfileSchema) + return readYaml(join(booksDir(dataDir), 'learning-profile.yml'), LearningProfileSchema, { + maxSchemaVersion: CURRENT_PROFILE_SCHEMA_VERSION, + }) }, async saveProfile(profile: LearningProfile): Promise { LearningProfileSchema.parse(profile) - await writeYaml(join(booksDir(dataDir), 'learning-profile.yml'), profile) + // Stamping here is what makes everything the running app writes + // current by construction, which is why the schema field can stay + // optional and why the migrator never has to revisit a freshly + // written file. + await writeYaml(join(booksDir(dataDir), 'learning-profile.yml'), { + ...profile, + schemaVersion: CURRENT_PROFILE_SCHEMA_VERSION, + }) }, async getProfileUpdatedAt(): Promise { @@ -137,7 +147,9 @@ export function createFsBookRepository(opts: { dataDir: string }): BookRepositor }, async getBook(bookId: string): Promise { - return readYaml(join(bookDir(dataDir, bookId), 'meta.yml'), BookMetaSchema) + return readYaml(join(bookDir(dataDir, bookId), 'meta.yml'), BookMetaSchema, { + maxSchemaVersion: CURRENT_BOOK_SCHEMA_VERSION, + }) }, async saveBook(meta: BookMeta): Promise { @@ -146,7 +158,11 @@ export function createFsBookRepository(opts: { dataDir: string }): BookRepositor await mkdir(dir, { recursive: true }) await mkdir(join(dir, 'chapters'), { recursive: true }) await mkdir(join(dir, 'feedback'), { recursive: true }) - await writeYaml(join(dir, 'meta.yml'), meta) + // Stamping here is what makes everything the running app writes + // current by construction, which is why the schema field can stay + // optional and why the migrator never has to revisit a freshly + // written file. + await writeYaml(join(dir, 'meta.yml'), { ...meta, schemaVersion: CURRENT_BOOK_SCHEMA_VERSION }) }, async deleteBook(bookId: string): Promise { diff --git a/server/adapters/fs-job-journal.test.ts b/server/adapters/fs-job-journal.test.ts new file mode 100644 index 0000000..bb6f154 --- /dev/null +++ b/server/adapters/fs-job-journal.test.ts @@ -0,0 +1,143 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { access, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { parse as parseYaml } from 'yaml' +import type { GenerationJob } from '@shared/domain.js' +import type { JobJournal } from '../ports/job-journal.js' +import { describeJobJournalContract } from '../ports/job-journal.contract.js' +import { createFsJobJournal } from './fs-job-journal.js' + +// Runs the shared JobJournal contract against the real filesystem adapter, +// over a fresh temp directory per subject, plus real-adapter-only cases +// about the on-disk layout that no fake needs to reproduce. + +const tempDirs: string[] = [] + +async function freshDataDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), 'tutor-fs-job-journal-test-')) + tempDirs.push(dir) + return dir +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +async function makeSubject(): Promise { + const dataDir = await freshDataDir() + return createFsJobJournal({ dataDir }) +} + +describeJobJournalContract('real fs adapter', makeSubject) + +const JOB: GenerationJob = { + id: 'job-1', + type: 'generate-chapter', + bookId: 'book-1', + bookTitle: 'Test Book', + status: 'running', + checkpoint: { kind: 'none' }, + params: {}, + startedAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', +} + +describe('createFsJobJournal', () => { + it('records land as one file per job at {dataDir}/jobs/{jobId}.yml', async () => { + const dataDir = await freshDataDir() + const journal = createFsJobJournal({ dataDir }) + + journal.record(JOB) + await journal.flush() + + const raw = await readFile(join(dataDir, 'jobs', `${JOB.id}.yml`), 'utf-8') + const parsed = parseYaml(raw) as Record + expect(parsed.id).toBe(JOB.id) + expect(parsed.bookId).toBe(JOB.bookId) + }) + + it('a second journal instance built over the same dataDir sees a job the first recorded, which is the property that makes this survive a restart at all', async () => { + const dataDir = await freshDataDir() + const first = createFsJobJournal({ dataDir }) + first.record(JOB) + await first.flush() + + const second = createFsJobJournal({ dataDir }) + const jobs = await second.listInterrupted() + expect(jobs.some((j) => j.id === JOB.id)).toBe(true) + }) + + it('deletes an unparseable .yml in jobs/ and skips it, rather than throwing, while still returning the valid jobs beside it', async () => { + // Jobs are throwaway state: a record that cannot even be parsed can + // never be resumed, so discarding it is correct here, where discarding + // a corrupt book never would be. + const dataDir = await freshDataDir() + const jobsDir = join(dataDir, 'jobs') + await mkdir(jobsDir, { recursive: true }) + await writeFile(join(jobsDir, 'corrupt.yml'), 'key: [unterminated flow sequence', 'utf-8') + + const journal = createFsJobJournal({ dataDir }) + journal.record(JOB) + await journal.flush() + + const jobs = await journal.listInterrupted() + expect(jobs.map((j) => j.id)).toEqual([JOB.id]) + await expect(access(join(jobsDir, 'corrupt.yml'))).rejects.toThrow() + }) + + it('ignores a non-.yml file in jobs/ and leaves it alone', async () => { + const dataDir = await freshDataDir() + const jobsDir = join(dataDir, 'jobs') + await mkdir(jobsDir, { recursive: true }) + await writeFile(join(jobsDir, 'notes.txt'), 'stray file, not a job', 'utf-8') + + const journal = createFsJobJournal({ dataDir }) + const jobs = await journal.listInterrupted() + + expect(jobs).toEqual([]) + await expect(access(join(jobsDir, 'notes.txt'))).resolves.toBeUndefined() + }) + + it('listInterrupted resolves to an empty array when there is no jobs/ directory', async () => { + const dataDir = await freshDataDir() + const journal = createFsJobJournal({ dataDir }) + + expect(await journal.listInterrupted()).toEqual([]) + }) + + // Journal writes are fire and forget, so nothing is awaiting the chain at + // the moment one runs. A write that rejects therefore has no handler + // attached, and an unhandled rejection is fatal in this process rather + // than merely noisy, because the espeak WASM module the narration adapter + // pulls in installs a process-level handler that rethrows. This was found + // by a server test crashing its vitest worker outright. + it('survives a write that fails, without an unhandled rejection and without blocking later writes', async () => { + const dataDir = await freshDataDir() + const journal = createFsJobJournal({ dataDir }) + + const rejections: unknown[] = [] + const onRejection = (err: unknown): void => { rejections.push(err) } + process.on('unhandledRejection', onRejection) + + try { + // A job id that resolves to an unwritable path makes writeYaml reject + // inside the chain, which is the exact shape of a real failure such as + // a data directory removed while a job was still running. + journal.record({ ...JOB, id: 'nested/missing/../../../../../../etc/definitely-not-writable' }) + journal.record({ ...JOB, id: 'good-job' }) + await journal.flush() + + // The chain recovered rather than staying rejected, so the write + // queued after the failure still landed. + const jobs = await journal.listInterrupted() + expect(jobs.map((j) => j.id)).toContain('good-job') + + // Give any stray rejection a turn of the event loop to surface. + await new Promise((resolve) => setImmediate(resolve)) + expect(rejections).toEqual([]) + } finally { + process.off('unhandledRejection', onRejection) + } + }) +}) diff --git a/server/adapters/fs-job-journal.ts b/server/adapters/fs-job-journal.ts new file mode 100644 index 0000000..2117a4f --- /dev/null +++ b/server/adapters/fs-job-journal.ts @@ -0,0 +1,107 @@ +import { readdir, rm } from 'node:fs/promises' +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { GenerationJobSchema, type GenerationJob } from '@shared/domain.js' +import type { JobJournal } from '../ports/job-journal.js' +import { readYaml, writeYaml } from './fs-paths.js' + +/** + * The real JobJournal adapter, one YAML file per job under + * {dataDir}/jobs/{jobId}.yml. + * + * Keeps an in-memory Map alongside the files, but only as a write-side + * mirror of the jobs this instance itself has recorded, never as a read + * cache. checkpoint() needs every field of a job to rewrite its file, + * since writeYaml always overwrites the whole document, and the Map is + * what lets it merge in the new checkpoint and enqueue that write + * synchronously instead of reading the file back first. listInterrupted() + * never consults the Map, it always re-reads {dataDir}/jobs/ from disk, + * because a job recorded by a different JobJournal instance, such as the + * Electron app and the MCP server pointed at the same data directory, or + * by this same process before an earlier restart, can only be discovered + * that way, this instance's Map was never populated for it. + * + * Every write is appended to a single promise chain so two writes for the + * same job can never land out of order. flush() is exactly that chain, + * and the chain recovers from a failed write rather than staying rejected + * forever, so one bad write cannot silently block every job after it. + * + * The factory itself performs no I/O, matching every other adapter + * server/composition-root.ts constructs eagerly at startup. + */ +export function createFsJobJournal(opts: { dataDir: string }): JobJournal { + const { dataDir } = opts + const jobsDir = join(dataDir, 'jobs') + const pathFor = (jobId: string): string => join(jobsDir, `${jobId}.yml`) + + // This instance's own pending writes only, see the header comment. + const mirror = new Map() + let writeChain: Promise = Promise.resolve() + + const enqueue = (task: () => Promise): void => { + // The trailing catch is load bearing, not defensive decoration. These + // writes are deliberately fire and forget, so nobody is awaiting this + // chain at the moment a write runs, and without the catch a failed + // write would leave writeChain rejected with no handler attached. That + // is an unhandled rejection, which is fatal here rather than merely + // noisy, because the espeak WASM module the narration adapter pulls in + // installs a process-level unhandledRejection handler that rethrows. + // One failed journal write would take the process down with it. + // + // Swallowing is the right response for this port specifically. A job + // record is throwaway state whose worst case on loss is that an + // interrupted job is not offered for resume, which is exactly where the + // user already was. Failing a running generation over it would be a + // strictly worse trade. + writeChain = writeChain + .then(task, task) + .catch((err) => { + console.warn('[fs-job-journal] A journal write failed and was dropped', err) + }) + } + + return { + record(job) { + mirror.set(job.id, job) + enqueue(() => writeYaml(pathFor(job.id), job)) + }, + + checkpoint(jobId, checkpoint) { + const job = mirror.get(jobId) + if (!job) return + const updated: GenerationJob = { ...job, checkpoint } + mirror.set(jobId, updated) + enqueue(() => writeYaml(pathFor(jobId), updated)) + }, + + clear(jobId) { + mirror.delete(jobId) + enqueue(() => rm(pathFor(jobId), { force: true })) + }, + + async listInterrupted(): Promise { + if (!existsSync(jobsDir)) return [] + + const jobs: GenerationJob[] = [] + for (const entry of await readdir(jobsDir, { withFileTypes: true })) { + if (!entry.isFile() || !entry.name.endsWith('.yml')) continue + const path = join(jobsDir, entry.name) + try { + const job = await readYaml(path, GenerationJobSchema) + jobs.push({ ...job, status: 'interrupted' }) + } catch { + // Jobs are throwaway state: a record that is not even valid YAML, + // or not a valid GenerationJob, can never be resumed, so + // discarding it here is correct in a way discarding a corrupt + // book never would be. + await rm(path, { force: true }) + } + } + return jobs + }, + + flush(): Promise { + return writeChain + }, + } +} diff --git a/server/adapters/fs-library-migrator.test.ts b/server/adapters/fs-library-migrator.test.ts new file mode 100644 index 0000000..4aa6697 --- /dev/null +++ b/server/adapters/fs-library-migrator.test.ts @@ -0,0 +1,210 @@ +import { afterEach, describe, expect, it } from 'vitest' +import { mkdtemp, mkdir, readdir, readFile, writeFile, copyFile, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { parse as parseYaml, stringify as stringifyYaml } from 'yaml' +import type { LibraryMigrator } from '../ports/library-migrator.js' +import { describeLibraryMigratorContract } from '../ports/library-migrator.contract.js' +import { createFsLibraryMigrator } from './fs-library-migrator.js' + +// The real LibraryMigrator adapter, proven against committed fixture +// libraries frozen at schema version 1 (see server/migrations/__fixtures__/ +// and its README). Every test copies a fixture into a fresh mkdtemp() +// before migrating it, so the committed fixture is never mutated and every +// assertion runs against real files rather than an in-memory mock of a +// filesystem. + +const FIXTURES_DIR = fileURLToPath(new URL('../migrations/__fixtures__/', import.meta.url)) + +const tempDirs: string[] = [] + +async function freshDataDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), 'tutor-fs-library-migrator-test-')) + tempDirs.push(dir) + return dir +} + +async function copyRecursive(src: string, dest: string): Promise { + await mkdir(dest, { recursive: true }) + for (const entry of await readdir(src, { withFileTypes: true })) { + const from = join(src, entry.name) + const to = join(dest, entry.name) + if (entry.isDirectory()) { + await copyRecursive(from, to) + } else { + await copyFile(from, to) + } + } +} + +/** Copies a named fixture (see __fixtures__/README.md) into a fresh temp data directory and returns its path. */ +async function copyFixture(name: string): Promise { + const dest = await freshDataDir() + await copyRecursive(join(FIXTURES_DIR, name), dest) + return dest +} + +async function readRawYaml(path: string): Promise> { + return parseYaml(await readFile(path, 'utf-8')) as Record +} + +afterEach(async () => { + await Promise.all(tempDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))) +}) + +async function makeSubject(): Promise { + const dataDir = await freshDataDir() + return createFsLibraryMigrator({ dataDir }) +} + +describeLibraryMigratorContract('real fs adapter', makeSubject) + +describe('createFsLibraryMigrator', () => { + it('reports an absent profile and no books for an empty data directory, without throwing', async () => { + const dataDir = await freshDataDir() + const migrator = createFsLibraryMigrator({ dataDir }) + + const report = await migrator.migrate() + + expect(report).toEqual({ profile: { outcome: 'absent' }, books: [] }) + }) + + it('migrates v1-library end to end, materializing defaults while preserving every existing value', async () => { + const dataDir = await copyFixture('v1-library') + const migrator = createFsLibraryMigrator({ dataDir }) + + const report = await migrator.migrate() + + expect(report.profile).toMatchObject({ outcome: 'migrated', from: 1, to: 2 }) + expect(report.books).toHaveLength(2) + expect(report.books).toEqual( + expect.arrayContaining([ + expect.objectContaining({ bookId: 'consensus-protocols', outcome: 'migrated', from: 1, to: 2 }), + expect.objectContaining({ bookId: 'vector-clocks', outcome: 'migrated', from: 1, to: 2 }), + ]), + ) + + const consensus = await readRawYaml(join(dataDir, 'books', 'consensus-protocols', 'meta.yml')) + expect(consensus.schemaVersion).toBe(2) + expect(consensus.tags).toEqual([]) + expect(consensus.audioGeneratedChapters).toEqual([]) + expect(consensus.rating).toBe(4.5) + + const vectorClocks = await readRawYaml(join(dataDir, 'books', 'vector-clocks', 'meta.yml')) + expect(vectorClocks.schemaVersion).toBe(2) + expect(vectorClocks.tags).toEqual([]) + expect(vectorClocks.audioGeneratedChapters).toEqual([]) + expect(vectorClocks.series).toBe('Distributed Systems') + + const profile = await readRawYaml(join(dataDir, 'books', 'learning-profile.yml')) + expect(profile.schemaVersion).toBe(2) + expect(profile.skills).toEqual([]) + }) + + it('writes a one-time .bak-v1 backup beside each migrated file, holding the original unmigrated document', async () => { + const dataDir = await copyFixture('v1-library') + const migrator = createFsLibraryMigrator({ dataDir }) + + await migrator.migrate() + + const bookBackup = await readRawYaml(join(dataDir, 'books', 'consensus-protocols', 'meta.yml.bak-v1')) + expect(bookBackup.schemaVersion).toBeUndefined() + expect(bookBackup.tags).toBeUndefined() + expect(bookBackup.id).toBe('consensus-protocols') + + const profileBackup = await readRawYaml(join(dataDir, 'books', 'learning-profile.yml.bak-v1')) + expect(profileBackup.schemaVersion).toBeUndefined() + expect(profileBackup.skills).toBeUndefined() + }) + + it('is idempotent: a second run reports everything current, and never overwrites an existing backup', async () => { + const dataDir = await copyFixture('v1-library') + const migrator = createFsLibraryMigrator({ dataDir }) + await migrator.migrate() + + const bookBackupPath = join(dataDir, 'books', 'consensus-protocols', 'meta.yml.bak-v1') + const profileBackupPath = join(dataDir, 'books', 'learning-profile.yml.bak-v1') + const bookBackupAfterFirstRun = await readFile(bookBackupPath, 'utf-8') + const profileBackupAfterFirstRun = await readFile(profileBackupPath, 'utf-8') + + const second = await migrator.migrate() + + expect(second.profile.outcome).toBe('current') + expect(second.books).toHaveLength(2) + for (const book of second.books) { + expect(book.outcome).toBe('current') + } + + expect(await readFile(bookBackupPath, 'utf-8')).toBe(bookBackupAfterFirstRun) + expect(await readFile(profileBackupPath, 'utf-8')).toBe(profileBackupAfterFirstRun) + }) + + it('migrates v1-profile-only: the profile migrates and the book list is empty', async () => { + const dataDir = await copyFixture('v1-profile-only') + const migrator = createFsLibraryMigrator({ dataDir }) + + const report = await migrator.migrate() + + expect(report.profile).toMatchObject({ outcome: 'migrated', from: 1, to: 2 }) + expect(report.books).toEqual([]) + }) + + it('migrates v1-corrupt-book: the readable book migrates, the truncated one fails as unreadable, and migrate() itself does not throw', async () => { + const dataDir = await copyFixture('v1-corrupt-book') + const migrator = createFsLibraryMigrator({ dataDir }) + + const report = await migrator.migrate() + + expect(report.profile.outcome).toBe('absent') + + const readable = report.books.find((b) => b.bookId === 'readable-book') + expect(readable).toMatchObject({ outcome: 'migrated', from: 1, to: 2 }) + + const truncated = report.books.find((b) => b.bookId === 'truncated-book') + expect(truncated?.outcome).toBe('failed') + expect(truncated?.reason).toBe('unreadable') + expect(truncated?.detail).toBeTruthy() + }) + + it('reports a too-new book as failed without touching its file, while a sibling v1 book in the same library still migrates', async () => { + const dataDir = await copyFixture('v1-library') + + const tooNewDir = join(dataDir, 'books', 'too-new-book') + await mkdir(tooNewDir, { recursive: true }) + const tooNewContent = stringifyYaml({ + id: 'too-new-book', + title: 'From The Future', + prompt: 'Whatever a much later build writes here.', + status: 'reading', + totalChapters: 1, + generatedUpTo: 1, + createdAt: '2026-06-01T00:00:00.000Z', + updatedAt: '2026-06-01T00:00:00.000Z', + schemaVersion: 99, + }) + await writeFile(join(tooNewDir, 'meta.yml'), tooNewContent, 'utf-8') + + const migrator = createFsLibraryMigrator({ dataDir }) + const report = await migrator.migrate() + + const tooNew = report.books.find((b) => b.bookId === 'too-new-book') + expect(tooNew).toMatchObject({ outcome: 'failed', reason: 'too-new' }) + expect(await readFile(join(tooNewDir, 'meta.yml'), 'utf-8')).toBe(tooNewContent) + + const sibling = report.books.find((b) => b.bookId === 'consensus-protocols') + expect(sibling).toMatchObject({ outcome: 'migrated', from: 1, to: 2 }) + }) + + it('ignores a directory under books/ that has no meta.yml', async () => { + const dataDir = await copyFixture('v1-library') + await mkdir(join(dataDir, 'books', 'not-a-book'), { recursive: true }) + await writeFile(join(dataDir, 'books', 'not-a-book', 'notes.txt'), 'stray file, not a book', 'utf-8') + + const migrator = createFsLibraryMigrator({ dataDir }) + const report = await migrator.migrate() + + expect(report.books.map((b) => b.bookId)).not.toContain('not-a-book') + expect(report.books).toHaveLength(2) + }) +}) diff --git a/server/adapters/fs-library-migrator.ts b/server/adapters/fs-library-migrator.ts new file mode 100644 index 0000000..1f9c4bf --- /dev/null +++ b/server/adapters/fs-library-migrator.ts @@ -0,0 +1,151 @@ +import { readFile, writeFile, readdir } from 'node:fs/promises' +import { existsSync } from 'node:fs' +import { join } from 'node:path' +import { parse as parseYaml } from 'yaml' +import { CURRENT_BOOK_SCHEMA_VERSION, CURRENT_PROFILE_SCHEMA_VERSION, readSchemaVersion } from '@shared/schema-version.js' +import { migrateForward, type MigrationStep } from '../migrations/migrate.js' +import { BOOK_MIGRATIONS } from '../migrations/book/index.js' +import { PROFILE_MIGRATIONS } from '../migrations/profile/index.js' +import type { + LibraryMigrator, + MigrationReport, + BookMigrationOutcome, + ProfileMigrationOutcome, + MigrationFailure, +} from '../ports/library-migrator.js' +import { booksDir, bookDir, writeYaml } from './fs-paths.js' + +/** + * The real LibraryMigrator adapter. Walks {dataDir}/books/learning-profile.yml + * and every {dataDir}/books/{bookId}/meta.yml, reading each as raw, + * unvalidated YAML rather than through BookRepository, since a v1 document + * by definition does not parse under the current Zod schema. See the + * port's own doc comment for why that puts this below BookRepository + * rather than behind it. + * + * A document already at the current version is reported 'current' and left + * untouched. A document behind the current version gets a one-time + * `.bak-v{from}` backup of its exact original bytes, written only when + * that backup does not already exist, then is overwritten in place with + * the migrated document through the same atomic writeYaml every other + * adapter uses. A document ahead of the current version is reported + * 'failed' with reason 'too-new' and is never written to, matching the + * forward-only design in server/migrations/README.md, there is no + * downgrade path. + * + * Every failure handling one document is caught locally and turned into + * that document's own 'failed' outcome. migrate() itself never rejects, so + * one bad book can never stop the rest of the library, or the profile, + * from migrating. + */ + +function messageOf(err: unknown): string { + return err instanceof Error ? err.message : String(err) +} + +interface DocumentOutcome { + outcome: 'current' | 'migrated' | 'failed' + from?: number + to?: number + reason?: MigrationFailure + detail?: string +} + +/** + * Migrates a single YAML document in place. Shared by the profile and + * every book, since both follow the same read, compare, backup, migrate, + * write sequence and differ only in which version constant and migration + * chain they use. + */ +async function migrateDocument( + path: string, + currentVersion: number, + steps: readonly MigrationStep[], +): Promise { + let content: string + let raw: unknown + try { + content = await readFile(path, 'utf-8') + raw = parseYaml(content) + } catch (err) { + return { outcome: 'failed', reason: 'unreadable', detail: messageOf(err) } + } + + const from = readSchemaVersion(raw) + + if (from > currentVersion) { + return { outcome: 'failed', reason: 'too-new', from } + } + if (from === currentVersion) { + return { outcome: 'current', from, to: currentVersion } + } + + try { + const backupPath = `${path}.bak-v${from}` + if (!existsSync(backupPath)) { + // The exact original bytes, not a re-serialized copy, so the backup + // stays a faithful historical record even if a future writer's YAML + // formatting ever drifts from the reader's. + await writeFile(backupPath, content, 'utf-8') + } + const migrated = migrateForward(raw, from, currentVersion, steps) + await writeYaml(path, migrated) + return { outcome: 'migrated', from, to: currentVersion } + } catch (err) { + return { outcome: 'failed', reason: 'unreadable', detail: messageOf(err) } + } +} + +async function migrateProfile(dataDir: string): Promise { + const path = join(booksDir(dataDir), 'learning-profile.yml') + if (!existsSync(path)) { + return { outcome: 'absent' } + } + return migrateDocument(path, CURRENT_PROFILE_SCHEMA_VERSION, PROFILE_MIGRATIONS) +} + +/** + * Every subdirectory of books/ that has a meta.yml, in whatever order + * readdir returns. A subdirectory with no meta.yml is not a book this + * migrator knows how to read, and is silently ignored, the same way + * fs-book-repository.ts's listBooks ignores one. + */ +async function listBookIds(dataDir: string): Promise { + const dir = booksDir(dataDir) + if (!existsSync(dir)) return [] + + const ids: string[] = [] + for (const entry of await readdir(dir, { withFileTypes: true })) { + if (!entry.isDirectory()) continue + if (!existsSync(join(dir, entry.name, 'meta.yml'))) continue + ids.push(entry.name) + } + return ids +} + +async function migrateBook(dataDir: string, bookId: string): Promise { + const path = join(bookDir(dataDir, bookId), 'meta.yml') + const outcome = await migrateDocument(path, CURRENT_BOOK_SCHEMA_VERSION, BOOK_MIGRATIONS) + return { bookId, ...outcome } +} + +/** + * No I/O happens here, only closures over dataDir. That matches every + * other adapter's factory, since createPorts constructs all of them + * eagerly at startup, before anything is known to need migrating. + */ +export function createFsLibraryMigrator(opts: { dataDir: string }): LibraryMigrator { + const { dataDir } = opts + + return { + async migrate(): Promise { + const profile = await migrateProfile(dataDir) + const bookIds = await listBookIds(dataDir) + const books: BookMigrationOutcome[] = [] + for (const bookId of bookIds) { + books.push(await migrateBook(dataDir, bookId)) + } + return { profile, books } + }, + } +} diff --git a/server/adapters/fs-paths.ts b/server/adapters/fs-paths.ts index f817a2b..eed4e6f 100644 --- a/server/adapters/fs-paths.ts +++ b/server/adapters/fs-paths.ts @@ -2,6 +2,7 @@ import { readFile, writeFile, mkdir, rename } from 'node:fs/promises' import { existsSync } from 'node:fs' import { join, dirname } from 'node:path' import { parse as parseYaml, stringify as stringifyYaml } from 'yaml' +import { assertSchemaVersionSupported } from '@shared/schema-version.js' /** * Path and YAML helpers shared by fs-book-repository.ts and @@ -31,9 +32,26 @@ export function padChapter(chapterNum: number): string { return String(chapterNum).padStart(2, '0') } -export async function readYaml(path: string, schema: { parse: (data: unknown) => T }): Promise { +/** + * Reads and parses a YAML file, then validates it against `schema`. When + * `opts.maxSchemaVersion` is given, the raw parsed document is checked with + * assertSchemaVersionSupported before `schema.parse` runs. The guard sits + * above Zod on purpose, because a file from a newer build may hold fields + * this build's schema would silently coerce or drop, and failing loudly + * beats mangling a user's library. Only callers that read a versioned + * document pass the option, every other YAML the app reads has no version + * field. + */ +export async function readYaml( + path: string, + schema: { parse: (data: unknown) => T }, + opts?: { maxSchemaVersion?: number }, +): Promise { const content = await readFile(path, 'utf-8') const data = parseYaml(content) + if (opts?.maxSchemaVersion !== undefined) { + assertSchemaVersionSupported(data, opts.maxSchemaVersion, path) + } return schema.parse(data) } diff --git a/server/adapters/journalled-background-tasks.test.ts b/server/adapters/journalled-background-tasks.test.ts new file mode 100644 index 0000000..7b6c361 --- /dev/null +++ b/server/adapters/journalled-background-tasks.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from 'vitest' +import type { GenerationJobParams } from '@shared/domain.js' +import type { StartTaskSpec } from '../ports/background-tasks.js' +import { describeBackgroundTasksContract } from '../ports/background-tasks.contract.js' +import { createFakeClock } from '../ports/clock.fake.js' +import { createFakeJobJournal } from '../ports/job-journal.fake.js' +import { createInMemoryBackgroundTasks } from './in-memory-background-tasks.js' +import { createJournalledBackgroundTasks } from './journalled-background-tasks.js' + +// Running the UNCHANGED existing BackgroundTasks contract against this +// decorated adapter is precisely the proof that adding persistence changed +// no observable behaviour: every one of those cases was written against +// the plain in-memory adapter, and none of it had to be loosened to also +// pass here. +describeBackgroundTasksContract('journalled over in-memory', () => + createJournalledBackgroundTasks({ + inner: createInMemoryBackgroundTasks(), + journal: createFakeJobJournal(), + clock: createFakeClock(), + }), +) + +const SPEC: StartTaskSpec = { type: 'generate-epub', bookId: 'book-1', bookTitle: 'Test Book', total: 5 } + +describe('createJournalledBackgroundTasks (whitebox)', () => { + it('start writes one journal record whose id equals the returned TaskHandle.id, timestamped by the injected clock, with checkpoint none', async () => { + const journal = createFakeJobJournal() + const clock = createFakeClock('2026-03-01T00:00:00.000Z') + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock }) + + const handle = tasks.start(SPEC) + await journal.flush() + + const jobs = await journal.listInterrupted() + expect(jobs).toHaveLength(1) + expect(jobs[0]).toMatchObject({ + id: handle.id, + startedAt: '2026-03-01T00:00:00.000Z', + updatedAt: '2026-03-01T00:00:00.000Z', + checkpoint: { kind: 'none' }, + }) + }) + + it('start journals a given params verbatim', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + const params: GenerationJobParams = { provider: 'anthropic', targetChapterNum: 4, voiceId: 'onyx' } + + tasks.start({ ...SPEC, params }) + await journal.flush() + + const [job] = await journal.listInterrupted() + expect(job.params).toEqual(params) + }) + + it('start without a params journals an empty object', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + + tasks.start(SPEC) + await journal.flush() + + const [job] = await journal.listInterrupted() + expect(job.params).toEqual({}) + }) + + it('succeed clears the journal record', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + const handle = tasks.start(SPEC) + + tasks.succeed(handle.id) + await journal.flush() + + expect(await journal.listInterrupted()).toEqual([]) + }) + + it('fail clears the journal record', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + const handle = tasks.start(SPEC) + + tasks.fail(handle.id, 'boom') + await journal.flush() + + expect(await journal.listInterrupted()).toEqual([]) + }) + + // A cancelled job must not resurrect itself at the next boot, so cancel + // clears the record exactly like succeed and fail do. + it('cancel clears the journal record, so a cancelled job cannot resurrect itself at the next boot', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + const handle = tasks.start(SPEC) + + tasks.cancel(handle.id) + await journal.flush() + + expect(await journal.listInterrupted()).toEqual([]) + }) + + it('report never writes to the journal, even across many calls, because progress is cosmetic', async () => { + const journal = createFakeJobJournal() + const tasks = createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal, clock: createFakeClock() }) + const handle = tasks.start(SPEC) + await journal.flush() + const afterStart = await journal.listInterrupted() + + for (let i = 0; i < 20; i++) { + tasks.report(handle.id, i, `Step ${i}`) + } + await journal.flush() + + expect(await journal.listInterrupted()).toEqual(afterStart) + }) +}) diff --git a/server/adapters/journalled-background-tasks.ts b/server/adapters/journalled-background-tasks.ts new file mode 100644 index 0000000..73b3328 --- /dev/null +++ b/server/adapters/journalled-background-tasks.ts @@ -0,0 +1,75 @@ +import type { GenerationJob } from '@shared/domain.js' +import type { BackgroundTasks, StartTaskSpec, TaskHandle } from '../ports/background-tasks.js' +import type { JobJournal } from '../ports/job-journal.js' +import type { Clock } from '../ports/clock.js' + +export interface JournalledBackgroundTasksDeps { + inner: BackgroundTasks + journal: JobJournal + clock: Clock +} + +/** + * Decorates a BackgroundTasks with persistence, rather than adding + * persistence to createInMemoryBackgroundTasks directly, so the + * persistence concern stays separable and independently testable. Running + * the existing BackgroundTasks contract unchanged against this decorated + * adapter (see journalled-background-tasks.test.ts) is the proof that + * adding persistence changed no observable behaviour, every case there was + * already written against the plain in-memory adapter, and none of it had + * to be loosened to also pass here. + * + * start() journals a record so a job still running when the process dies + * can be found by JobJournal.listInterrupted() at the next boot. succeed(), + * fail(), and cancel() all clear that record, each is a terminal state a + * resumed job would never need to reach again, cancel() included, a + * cancelled job must not resurrect itself at the next boot. report() never + * touches the journal, progress is cosmetic, and journalling every sentence + * of a narration or every percent of an EPUB export would be thousands of + * writes for information nothing on restart needs. + */ +export function createJournalledBackgroundTasks(deps: JournalledBackgroundTasksDeps): BackgroundTasks { + const { inner, journal, clock } = deps + + return { + ...inner, + + start(spec: StartTaskSpec): TaskHandle { + const handle = inner.start(spec) + const now = clock.nowIso() + const job: GenerationJob = { + id: handle.id, + type: spec.type, + bookId: spec.bookId, + bookTitle: spec.bookTitle, + status: 'running', + checkpoint: { kind: 'none' }, + params: spec.params ?? {}, + startedAt: now, + updatedAt: now, + } + journal.record(job) + return handle + }, + + succeed(taskId, result) { + inner.succeed(taskId, result) + journal.clear(taskId) + }, + + fail(taskId, error) { + inner.fail(taskId, error) + journal.clear(taskId) + }, + + cancel(taskId) { + const cancelled = inner.cancel(taskId) + // Clearing unconditionally, not only when cancelled is true, keeps + // this a plain no-op-safe cleanup call either way, journal.clear() + // on an id it never recorded, or already cleared, does nothing. A + // cancelled job must not resurrect itself at the next boot. + journal.clear(taskId) + return cancelled + }, + } +} diff --git a/server/adapters/kokoro-speech-synthesis.ts b/server/adapters/kokoro-speech-synthesis.ts index 1987b0d..2ddcb05 100644 --- a/server/adapters/kokoro-speech-synthesis.ts +++ b/server/adapters/kokoro-speech-synthesis.ts @@ -19,6 +19,35 @@ import { } from '../services/audiobook-installer.js' /** + * ⚠️ PROCESS-WIDE HAZARD, and it is not this module's doing. + * + * `kokoro-js` pulls in `phonemizer`, which bundles the espeak-ng WASM build, + * and that bundle installs its own process-level handlers at import time: + * + * process.on('unhandledRejection', (err) => { throw err }) + * process.on('uncaughtException', ...) + * + * The consequence reaches far past narration. From the moment this module is + * imported, ANY unhandled promise rejection anywhere in the process becomes + * fatal rather than merely logged, including one raised by code that has + * nothing to do with audio. Because `server/composition-root.ts` constructs + * every adapter eagerly, that is every server process, not only one that is + * narrating something. + * + * This bit once already. A fire-and-forget write in + * `server/adapters/fs-job-journal.ts` left its promise chain rejected with no + * handler attached, which is ordinarily a warning, and instead took the whole + * worker down with no failing assertion to point at. See the trailing catch + * there and its comment. + * + * So the rule for anything that runs in this process: a promise you do not + * await must carry its own `.catch`. Do not rely on the default Node + * behaviour of logging and continuing, because it is not in force here. + * + * Deliberately not removed or wrapped. Overriding another package's process + * handlers is worse than documenting them, and the hazard belongs at the + * import that causes it. + * * kokoro-js backed SpeechSynthesis. Real logic lifted verbatim from * server/services/kokoro-service.ts: the voice catalogue tables, the * in-process synthesis pool, and tts.stream()-based synthesis. See that diff --git a/server/adapters/retry-policy.test.ts b/server/adapters/retry-policy.test.ts new file mode 100644 index 0000000..4bc3134 --- /dev/null +++ b/server/adapters/retry-policy.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it } from 'vitest' +import type { TextGenerationErrorKind } from '../ports/text-generation.js' +import { nextDelayMs, RETRY_MAX_DELAY_MS, RETRY_POLICY, RETRY_TOTAL_ELAPSED_CEILING_MS } from './retry-policy.js' + +/** + * Pure unit tests against the retry policy table and nextDelayMs, run with + * an injected deterministic rng so every asserted number is exact. None of + * these tests wait out a real delay or call Math.random, both of which + * would make the suite slow or flaky. + */ + +const RETRYABLE_KINDS: TextGenerationErrorKind[] = ['rate-limited', 'overloaded', 'network-failed', 'timed-out'] +const NEVER_RETRIED_KINDS: TextGenerationErrorKind[] = ['auth-failed', 'content-refused', 'unknown'] + +describe('RETRY_POLICY', () => { + it('pins the max attempts for every retryable kind', () => { + expect(RETRY_POLICY['rate-limited'].maxAttempts).toBe(4) + expect(RETRY_POLICY['overloaded'].maxAttempts).toBe(3) + expect(RETRY_POLICY['network-failed'].maxAttempts).toBe(3) + expect(RETRY_POLICY['timed-out'].maxAttempts).toBe(2) + }) + + it('pins the network-failed base delay at 200ms', () => { + expect(RETRY_POLICY['network-failed'].baseDelayMs).toBe(200) + }) + + it('pins every never-retried kind to exactly one attempt', () => { + for (const kind of NEVER_RETRIED_KINDS) { + expect(RETRY_POLICY[kind].maxAttempts).toBe(1) + } + }) + + it('pins the caps used throughout the policy', () => { + expect(RETRY_MAX_DELAY_MS).toBe(20_000) + expect(RETRY_TOTAL_ELAPSED_CEILING_MS).toBe(60_000) + }) +}) + +describe('nextDelayMs, kinds that never retry', () => { + it.each(NEVER_RETRIED_KINDS)('%s always returns null, regardless of attempt', (kind) => { + expect(nextDelayMs(kind, 1)).toBeNull() + expect(nextDelayMs(kind, 2)).toBeNull() + expect(nextDelayMs(kind, 99)).toBeNull() + }) +}) + +describe('nextDelayMs, the maxAttempts cutoff', () => { + it('returns null once attempt reaches maxAttempts, for every retryable kind', () => { + for (const kind of RETRYABLE_KINDS) { + const { maxAttempts } = RETRY_POLICY[kind] + expect(nextDelayMs(kind, maxAttempts, { rng: () => 1 })).toBeNull() + expect(nextDelayMs(kind, maxAttempts + 5, { rng: () => 1 })).toBeNull() + } + }) + + it('still returns a number one attempt below maxAttempts', () => { + for (const kind of RETRYABLE_KINDS) { + const { maxAttempts } = RETRY_POLICY[kind] + expect(nextDelayMs(kind, maxAttempts - 1, { rng: () => 1 })).not.toBeNull() + } + }) +}) + +describe('nextDelayMs, full jitter formula', () => { + // rng() * min(baseDelayMs * 2 ** (attempt - 1), RETRY_MAX_DELAY_MS). rng + // is injected, so () => 1 exercises the worst case (no reduction from + // jitter), () => 0 the best case (always zero), and () => 0.5 a value in + // between, pinning the multiplication itself rather than just its range. + + it('overloaded, base 1000ms, attempts 1 and 2', () => { + expect(nextDelayMs('overloaded', 1, { rng: () => 1 })).toBe(1000) + expect(nextDelayMs('overloaded', 1, { rng: () => 0 })).toBe(0) + expect(nextDelayMs('overloaded', 1, { rng: () => 0.5 })).toBe(500) + + expect(nextDelayMs('overloaded', 2, { rng: () => 1 })).toBe(2000) + expect(nextDelayMs('overloaded', 2, { rng: () => 0 })).toBe(0) + expect(nextDelayMs('overloaded', 2, { rng: () => 0.5 })).toBe(1000) + }) + + it('network-failed, base 200ms, attempts 1 and 2', () => { + expect(nextDelayMs('network-failed', 1, { rng: () => 1 })).toBe(200) + expect(nextDelayMs('network-failed', 1, { rng: () => 0.5 })).toBe(100) + + expect(nextDelayMs('network-failed', 2, { rng: () => 1 })).toBe(400) + expect(nextDelayMs('network-failed', 2, { rng: () => 0.5 })).toBe(200) + }) + + it('timed-out, base 1000ms, its only retryable attempt', () => { + expect(nextDelayMs('timed-out', 1, { rng: () => 1 })).toBe(1000) + expect(nextDelayMs('timed-out', 1, { rng: () => 0.5 })).toBe(500) + }) + + it('rate-limited without a retryAfterMs falls back to exponential full jitter', () => { + expect(nextDelayMs('rate-limited', 1, { rng: () => 1 })).toBe(1000) + expect(nextDelayMs('rate-limited', 2, { rng: () => 1 })).toBe(2000) + expect(nextDelayMs('rate-limited', 3, { rng: () => 1 })).toBe(4000) + }) +}) + +describe('nextDelayMs, rate-limited retryAfterMs', () => { + it('returns retryAfterMs exactly when it is under the per-delay cap', () => { + expect(nextDelayMs('rate-limited', 1, { retryAfterMs: 5000 })).toBe(5000) + }) + + it('clamps retryAfterMs to RETRY_MAX_DELAY_MS when the provider asks for longer', () => { + expect(nextDelayMs('rate-limited', 1, { retryAfterMs: 999_999 })).toBe(RETRY_MAX_DELAY_MS) + }) + + it('ignores retryAfterMs once attempt has reached maxAttempts', () => { + expect(nextDelayMs('rate-limited', RETRY_POLICY['rate-limited'].maxAttempts, { retryAfterMs: 1000 })).toBeNull() + }) + + it('is ignored for kinds other than rate-limited, which fall back to exponential full jitter', () => { + expect(nextDelayMs('overloaded', 1, { retryAfterMs: 5000, rng: () => 1 })).toBe(1000) + }) +}) + +describe('retry-runaway guard, the total-elapsed ceiling', () => { + it.each(RETRYABLE_KINDS)('worst-case total delay for %s stays at or below the ceiling', (kind) => { + const { maxAttempts } = RETRY_POLICY[kind] + let total = 0 + for (let attempt = 1; attempt < maxAttempts; attempt++) { + total += nextDelayMs(kind, attempt, { rng: () => 1 }) ?? 0 + } + expect(total).toBeLessThanOrEqual(RETRY_TOTAL_ELAPSED_CEILING_MS) + }) + + it('stays at or below the ceiling even when a provider claims an enormous retry-after on every rate-limited attempt', () => { + const { maxAttempts } = RETRY_POLICY['rate-limited'] + let total = 0 + for (let attempt = 1; attempt < maxAttempts; attempt++) { + total += nextDelayMs('rate-limited', attempt, { retryAfterMs: 999_999_999 }) ?? 0 + } + expect(total).toBeLessThanOrEqual(RETRY_TOTAL_ELAPSED_CEILING_MS) + }) +}) diff --git a/server/adapters/retry-policy.ts b/server/adapters/retry-policy.ts new file mode 100644 index 0000000..7b7f2cb --- /dev/null +++ b/server/adapters/retry-policy.ts @@ -0,0 +1,56 @@ +import type { TextGenerationErrorKind } from '../ports/text-generation.js' + +/** + * This module decides how many times, and how long to wait between, each + * TextGenerationError kind gets retried. It lives next to the adapter + * rather than the port, because retrying is this adapter's own policy, not + * a port-level guarantee. Delays use full jitter, `rng() * min(exponential + * backoff, RETRY_MAX_DELAY_MS)`, rather than plain exponential backoff, so + * that many simultaneous retries spread out instead of retrying in + * lockstep. `RETRY_TOTAL_ELAPSED_CEILING_MS` is a second, independent + * bound on top of each kind's `maxAttempts`, so a future change to the + * per-kind table cannot silently make total retry time unbounded. `rng` is + * injected, defaulting to `Math.random`, so the policy is deterministically + * testable. + */ + +export interface RetryRule { + maxAttempts: number + baseDelayMs: number +} + +export const RETRY_MAX_DELAY_MS = 20_000 +export const RETRY_TOTAL_ELAPSED_CEILING_MS = 60_000 + +export const RETRY_POLICY: Record = { + 'rate-limited': { maxAttempts: 4, baseDelayMs: 1000 }, + overloaded: { maxAttempts: 3, baseDelayMs: 1000 }, + 'network-failed': { maxAttempts: 3, baseDelayMs: 200 }, + 'timed-out': { maxAttempts: 2, baseDelayMs: 1000 }, + 'auth-failed': { maxAttempts: 1, baseDelayMs: 0 }, + 'content-refused': { maxAttempts: 1, baseDelayMs: 0 }, + unknown: { maxAttempts: 1, baseDelayMs: 0 }, +} + +/** + * Computes the delay before the next attempt, given that the 1-based + * `attempt` just failed with `kind`. Returns `null` to mean stop retrying. + * A kind with `maxAttempts: 1` never retries, because its first and only + * attempt always reaches the cap. + */ +export function nextDelayMs( + kind: TextGenerationErrorKind, + attempt: number, + opts?: { retryAfterMs?: number; rng?: () => number }, +): number | null { + const rule = RETRY_POLICY[kind] + if (attempt >= rule.maxAttempts) return null + + if (kind === 'rate-limited' && opts?.retryAfterMs !== undefined) { + return Math.min(opts.retryAfterMs, RETRY_MAX_DELAY_MS) + } + + const rng = opts?.rng ?? Math.random + const backoff = Math.min(rule.baseDelayMs * 2 ** (attempt - 1), RETRY_MAX_DELAY_MS) + return rng() * backoff +} diff --git a/server/composition-root.test.ts b/server/composition-root.test.ts index f6ffdef..62b48c1 100644 --- a/server/composition-root.test.ts +++ b/server/composition-root.test.ts @@ -17,6 +17,7 @@ import { createFakeTextGeneration } from './ports/text-generation.fake.js' const PORT_NAMES: Array = [ 'bookRepository', 'artifactStore', + 'libraryMigrator', 'keyVault', 'textGeneration', 'imageGeneration', @@ -26,6 +27,7 @@ const PORT_NAMES: Array = [ 'epubImport', 'epubExport', 'backgroundTasks', + 'jobJournal', 'clock', 'osFileManager', ] diff --git a/server/composition-root.ts b/server/composition-root.ts index 29a4799..b6b75b4 100644 --- a/server/composition-root.ts +++ b/server/composition-root.ts @@ -1,6 +1,7 @@ import { getDataDir } from '@shared/node/data-dir.js' import { createFsBookRepository } from './adapters/fs-book-repository.js' import { createFsArtifactStore } from './adapters/fs-artifact-store.js' +import { createFsLibraryMigrator } from './adapters/fs-library-migrator.js' import { createFileKeyVault } from './adapters/file-key-vault.js' import { createAiSdkTextGeneration } from './adapters/ai-sdk-text-generation.js' import { createHttpImageGeneration } from './adapters/http-image-generation.js' @@ -10,6 +11,8 @@ import { createKrokiDiagramRenderer } from './adapters/kroki-diagram-renderer.js import { createEpub2Import } from './adapters/epub2-import.js' import { createEpubGenExport } from './adapters/epub-gen-export.js' import { createInMemoryBackgroundTasks } from './adapters/in-memory-background-tasks.js' +import { createJournalledBackgroundTasks } from './adapters/journalled-background-tasks.js' +import { createFsJobJournal } from './adapters/fs-job-journal.js' import { createSystemClock } from './adapters/system-clock.js' import { createOsFileManager } from './adapters/os-file-manager.js' import { createChapterGenerationStream, type ChapterGenerationStream } from './services/chapter-generation-stream.js' @@ -17,6 +20,7 @@ import { createGenerateNextChapter } from './services/generate-next-chapter.js' import type { BookRepository } from './ports/book-repository.js' import type { ArtifactStore } from './ports/artifact-store.js' +import type { LibraryMigrator } from './ports/library-migrator.js' import type { KeyVault } from './ports/key-vault.js' import type { TextGeneration } from './ports/text-generation.js' import type { ImageGeneration } from './ports/image-generation.js' @@ -26,6 +30,7 @@ import type { DiagramRenderer } from './ports/diagram-renderer.js' import type { EpubImport } from './ports/epub-import.js' import type { EpubExport } from './ports/epub-export.js' import type { BackgroundTasks } from './ports/background-tasks.js' +import type { JobJournal } from './ports/job-journal.js' import type { Clock } from './ports/clock.js' import type { OsFileManager } from './ports/os-file-manager.js' @@ -41,7 +46,7 @@ import type { OsFileManager } from './ports/os-file-manager.js' * * Adapters are constructed eagerly when createPorts runs rather than * lazily on first use. That is deliberate. Construction is cheap for all - * thirteen, none of them opens a connection or reads a file in its + * fifteen, none of them opens a connection or reads a file in its * factory, and eager construction means a misconfigured dependency fails * at startup instead of on whichever request happens to reach it first. */ @@ -50,6 +55,7 @@ import type { OsFileManager } from './ports/os-file-manager.js' export interface Ports { bookRepository: BookRepository artifactStore: ArtifactStore + libraryMigrator: LibraryMigrator keyVault: KeyVault textGeneration: TextGeneration imageGeneration: ImageGeneration @@ -59,6 +65,7 @@ export interface Ports { epubImport: EpubImport epubExport: EpubExport backgroundTasks: BackgroundTasks + jobJournal: JobJournal clock: Clock osFileManager: OsFileManager } @@ -81,10 +88,17 @@ export interface Ports { export function createPorts(overrides: Partial = {}): Ports { const dataDir = getDataDir() const keyVault = overrides.keyVault ?? createFileKeyVault({ dataDir }) + // Resolved before the object literal too, exactly like keyVault above, so + // an override of either reaches createJournalledBackgroundTasks below + // instead of being silently bypassed by a second, un-overridden instance + // built inline inside it. + const jobJournal = overrides.jobJournal ?? createFsJobJournal({ dataDir }) + const clock = overrides.clock ?? createSystemClock() return { bookRepository: createFsBookRepository({ dataDir }), artifactStore: createFsArtifactStore({ dataDir }), + libraryMigrator: createFsLibraryMigrator({ dataDir }), keyVault, // These two read API keys through the vault, so they take whichever // vault won above rather than building a second one. Overriding the @@ -96,8 +110,13 @@ export function createPorts(overrides: Partial = {}): Ports { diagramRenderer: createKrokiDiagramRenderer(), epubImport: createEpub2Import(), epubExport: createEpubGenExport(), - backgroundTasks: createInMemoryBackgroundTasks(), - clock: createSystemClock(), + // Decorated with journalling rather than a plain in-memory adapter, so + // a task still running when the process dies can be found and resumed + // at the next boot. Takes the jobJournal and clock resolved above so an + // override of either reaches it. + backgroundTasks: createJournalledBackgroundTasks({ inner: createInMemoryBackgroundTasks(), journal: jobJournal, clock }), + jobJournal, + clock, osFileManager: createOsFileManager(), ...overrides, } @@ -138,6 +157,8 @@ export function createSharedServices(ports: Ports): SharedServices { chapterGenerationStream: createChapterGenerationStream({ books: ports.bookRepository, generateNextChapter, + journal: ports.jobJournal, + clock: ports.clock, }), } } diff --git a/server/http/error-handler.test.ts b/server/http/error-handler.test.ts index 117014f..c4ad935 100644 --- a/server/http/error-handler.test.ts +++ b/server/http/error-handler.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect } from 'vitest' import { toErrorResponse } from './error-handler.js' +import { SchemaTooNewError } from '@shared/schema-version.js' +import { TextGenerationError } from '../ports/text-generation.js' import { RequestValidationError } from './parse.js' /** @@ -73,4 +75,43 @@ describe('toErrorResponse', () => { const err = Object.assign(new Error('nope'), { code: 'ENOENT', statusCode: 500 }) expect(toErrorResponse(err).status).toBe(404) }) + + describe('SchemaTooNewError', () => { + it('answers 409, because the server is fine and the data is simply from the future', () => { + const res = toErrorResponse(new SchemaTooNewError(5, 2, '/Users/someone/Library/Application Support/tutor/books/b/meta.yml')) + expect(res.status).toBe(409) + expect(res.logAsServerError).toBe(false) + }) + + it('never echoes the message, because it embeds the absolute path of the offending file', () => { + const path = '/Users/someone/Library/Application Support/tutor/books/b/meta.yml' + const res = toErrorResponse(new SchemaTooNewError(5, 2, path)) + expect(res.body.error).not.toContain(path) + expect(res.body.error).not.toContain('/Users/') + // Rebuilt from the numbers instead, so it still says something useful. + expect(res.body.error).toContain('5') + expect(res.body.error).toContain('2') + }) + }) + + describe('TextGenerationError', () => { + it('carries the failure class to the client as kind, which is what the reader acts on', () => { + const res = toErrorResponse(new TextGenerationError('auth-failed', 'No API key configured for provider: anthropic', false)) + expect(res.body.kind).toBe('auth-failed') + expect(res.body.error).toBe('No API key configured for provider: anthropic') + }) + + it('answers 401 for auth-failed, so the client can tell an unusable key from a busy provider', () => { + expect(toErrorResponse(new TextGenerationError('auth-failed', 'bad key', false)).status).toBe(401) + }) + + it('answers 503 for a retryable class and 502 for a terminal one', () => { + expect(toErrorResponse(new TextGenerationError('overloaded', 'busy', true)).status).toBe(503) + expect(toErrorResponse(new TextGenerationError('content-refused', 'refused', false)).status).toBe(502) + }) + + it('does not log a provider failure as a server fault, because the server is not at fault', () => { + expect(toErrorResponse(new TextGenerationError('rate-limited', 'slow down', true)).logAsServerError).toBe(false) + }) + }) }) diff --git a/server/http/error-handler.ts b/server/http/error-handler.ts index 3fe044d..7bb4d63 100644 --- a/server/http/error-handler.ts +++ b/server/http/error-handler.ts @@ -1,5 +1,8 @@ import type { FastifyInstance } from 'fastify' import { RequestValidationError } from './parse.js' +import { SchemaTooNewError } from '@shared/schema-version.js' +import { TextGenerationError } from '../ports/text-generation.js' +import type { AiErrorKind } from '@shared/responses.js' /** * The single place an error becomes a response. @@ -16,7 +19,7 @@ import { RequestValidationError } from './parse.js' /** What the client receives, plus whether the server should log it as a fault. */ export interface ErrorResponse { status: number - body: { error: string; details?: unknown } + body: { error: string; details?: unknown; kind?: AiErrorKind } logAsServerError: boolean } @@ -44,6 +47,35 @@ export function toErrorResponse(error: AppError): ErrorResponse { return { status: 404, body: { error: 'Not found' }, logAsServerError: false } } + // A library written by a newer build. 409 rather than 500, because the + // server is fine and the data is simply from the future, and it is not + // retryable. The body is rebuilt from `found` and `supported` instead of + // echoing error.message, for the same reason ENOENT above does not echo + // its own: the message embeds the absolute path of the offending file. + // The path still reaches the log, where it is useful and not exposed. + if (error instanceof SchemaTooNewError) { + return { + status: 409, + body: { + error: `This library was written by a newer version of Tutor (data version ${error.found}, this build supports up to ${error.supported}). Update the app to open it.`, + }, + logAsServerError: false, + } + } + + // An AI provider failure that the adapter already classified. The class + // travels to the client as `kind` so the reader can act on it, most + // usefully by opening the missing-key dialog on auth-failed instead of + // showing a toast the user has no way to resolve. `reason` is written to + // be safe to display, unlike a raw provider message. + if (error instanceof TextGenerationError) { + return { + status: error.kind === 'auth-failed' ? 401 : error.retryable ? 503 : 502, + body: { error: error.reason, kind: error.kind }, + logAsServerError: false, + } + } + const statusCode = error.statusCode ?? 500 // An unexpected failure may carry anything in its message, including diff --git a/server/index.test.ts b/server/index.test.ts new file mode 100644 index 0000000..ae841a8 --- /dev/null +++ b/server/index.test.ts @@ -0,0 +1,155 @@ +import { describe, it, expect } from 'vitest' +import { mkdtempSync, chmodSync, readdirSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import Fastify from 'fastify' +import { buildServer, runStartupTasks } from './index.js' +import { createPorts, createSharedServices } from './composition-root.js' +import { createFakeLibraryMigrator } from './ports/library-migrator.fake.js' +import { createFakeBookRepository } from './ports/book-repository.fake.js' +import { createFakeArtifactStore } from './ports/artifact-store.fake.js' +import { createFakeJobJournal } from './ports/job-journal.fake.js' + +describe('buildServer decorations', () => { + // The decoration exists so a caller acting on the server after it is + // built reaches the same port instances the routes hold. createPorts + // returns fresh adapters per call, deliberately, so a second call hands + // back a BackgroundTasks nothing can observe. This test drives that + // difference end to end rather than merely comparing object identity: a + // task started through the decorated ports has to be visible through the + // route, and it would not be if the two were separate instances. + // + // Startup job resume is the caller this protects. Without it, a resumed + // job would be registered somewhere no route reads, so the tray would + // stay empty and nothing would report an error. + it('exposes the same ports the routes were registered with', async () => { + const fastify = await buildServer() + try { + const handle = fastify.ports.backgroundTasks.start({ + type: 'generate-epub', + bookId: 'decoration-book', + bookTitle: 'Decoration Book', + total: 1, + }) + + const res = await fastify.inject({ method: 'GET', url: '/api/tasks' }) + expect(res.statusCode).toBe(200) + expect(res.json().map((t: { id: string }) => t.id)).toContain(handle.id) + } finally { + await fastify.close() + } + }) + + it('exposes the same shared services the routes were registered with', async () => { + const fastify = await buildServer() + try { + expect(fastify.services.chapterGenerationStream).toBeDefined() + expect(fastify.services.chapterGenerationStream.getStatus('no-such-book')).toEqual({ active: false }) + } finally { + await fastify.close() + } + }) +}) + +describe('buildServer', () => { + // Protects the routes-doc generator and every fastify.inject-based + // characterization test in this repo, all of which call buildServer() + // and expect a fully registered instance with no side effect on the data + // directory. Making the directory read-only before building proves + // buildServer() itself never writes, independent of whether the request + // injected against it would have. + it('never writes to the data directory while registering every route', async () => { + const dir = mkdtempSync(join(tmpdir(), 'tutor-buildserver-mutation-test-')) + const previousDataDir = process.env.TUTOR_DATA_DIR + process.env.TUTOR_DATA_DIR = dir + chmodSync(dir, 0o500) + + try { + const fastify = await buildServer() + const res = await fastify.inject({ method: 'GET', url: '/api/health' }) + expect(res.statusCode).toBe(200) + await fastify.close() + } finally { + chmodSync(dir, 0o700) + if (previousDataDir === undefined) { + delete process.env.TUTOR_DATA_DIR + } else { + process.env.TUTOR_DATA_DIR = previousDataDir + } + } + + expect(readdirSync(dir)).toEqual([]) + rmSync(dir, { recursive: true, force: true }) + }) +}) + +describe('runStartupTasks', () => { + // The three-step boot block used to live inline in startServer, which + // binds a real port and therefore is awkward to drive directly from a + // test. Extracted so this can call it straight, with fakes, and assert + // on ordering without listening on anything. + // + // Migration before crash recovery: crash recovery reads and writes + // BookMeta through the CURRENT Zod schema, so a book at an old schema + // version would be silently skipped by listBooks's own try/catch if + // migration had not already run. + // + // Crash recovery before resume: resume's own decisions (most concretely + // audiobook resume, which restarts narration from the beginning) depend + // on recovery having already reconciled BookMeta and wiped any partial + // audio. Running resume first would race recovery over the same file. + it('runs migration, then crash recovery, then interrupted-job resume, in that order', async () => { + const callOrder: string[] = [] + + const realMigrator = createFakeLibraryMigrator() + const libraryMigrator = { + async migrate() { + callOrder.push('libraryMigrator.migrate') + return realMigrator.migrate() + }, + } + + const realBooks = createFakeBookRepository() + const bookRepository = { + ...realBooks, + async listBooks() { + callOrder.push('bookRepository.listBooks') + return realBooks.listBooks() + }, + } + + const realJournal = createFakeJobJournal() + const jobJournal = { + ...realJournal, + async listInterrupted() { + callOrder.push('jobJournal.listInterrupted') + return realJournal.listInterrupted() + }, + } + + const ports = createPorts({ libraryMigrator, bookRepository, artifactStore: createFakeArtifactStore(), jobJournal }) + const services = createSharedServices(ports) + + // A bare, non-listening Fastify instance exists here only to borrow a + // real FastifyBaseLogger, so runStartupTasks gets the exact type it + // asks for without a full buildServer() and without hand-stubbing + // pino's logger interface. + const silent = Fastify({ logger: false }) + + // Resume's autoResume flag reads this var directly from the process + // environment, so it is saved and restored here rather than trusted to + // whatever the shell running this suite happens to have set, exactly + // like the TUTOR_DATA_DIR guard in the mutation test below. + const previousFlag = process.env.TUTOR_NO_AUTO_RESUME + delete process.env.TUTOR_NO_AUTO_RESUME + try { + await runStartupTasks(ports, services, silent.log) + } finally { + if (previousFlag === undefined) delete process.env.TUTOR_NO_AUTO_RESUME + else process.env.TUTOR_NO_AUTO_RESUME = previousFlag + await silent.close() + } + + expect(callOrder).toEqual(['libraryMigrator.migrate', 'bookRepository.listBooks', 'jobJournal.listInterrupted']) + }) +}) diff --git a/server/index.ts b/server/index.ts index 9303c2e..63a0587 100644 --- a/server/index.ts +++ b/server/index.ts @@ -1,5 +1,5 @@ import Fastify from 'fastify' -import type { FastifyInstance } from 'fastify' +import type { FastifyInstance, FastifyBaseLogger } from 'fastify' import rateLimit from '@fastify/rate-limit' import { chatRoutes } from './routes/chat.js' import { libraryRoutes } from './routes/library.js' @@ -19,9 +19,14 @@ import { modelsRoutes } from './routes/models.js' import { audiobookRoutes } from './routes/audiobook.js' import { getDataDir } from '@shared/node/data-dir.js' import { createRecoverFromCrash } from './services/recover-from-crash.js' +import { createResumeInterruptedJobs } from './services/resume-interrupted-jobs.js' +import { createGenerateNextChapter } from './services/generate-next-chapter.js' +import { createGenerateAllChapters } from './services/generate-all-chapters.js' +import { createGenerateAudiobook } from './services/generate-audiobook.js' import { registerErrorHandler } from './http/error-handler.js' import { STATUS_FORBIDDEN, STATUS_NO_CONTENT } from './http/status.js' -import { createPorts, createSharedServices, type Ports } from './composition-root.js' +import { createPorts, createSharedServices, type Ports, type SharedServices } from './composition-root.js' +import type { MigrationReport } from './ports/library-migrator.js' const ALLOWED_ORIGINS = [ 'http://localhost:5173', @@ -58,6 +63,15 @@ function isAllowedOrigin(origin: string): boolean { * its plugin options, so a route module never reaches for a concrete * adapter and never has to be edited when one is swapped. */ +declare module 'fastify' { + interface FastifyInstance { + /** The exact ports this instance's routes were registered with. See buildServer. */ + ports: Ports + /** The exact shared services this instance's routes were registered with. See buildServer. */ + services: SharedServices + } +} + export async function buildServer(overrides: Partial = {}): Promise { const ports = createPorts(overrides) const services = createSharedServices(ports) @@ -140,26 +154,61 @@ export async function buildServer(overrides: Partial = {}): Promise ({ status: 'ok' })) + // Expose the exact ports and services the routes above were registered + // with, so a caller that has to act on this server's state after it is + // built reaches the same instances rather than constructing a second set. + // createPorts deliberately returns fresh adapters on every call, which + // composition-root.test.ts pins, so a second call would hand back a + // BackgroundTasks no route can see and a ChapterGenerationStream no route + // reads. That is harmless for stateless filesystem bridges and silently + // wrong for anything holding in-memory state, which startup job resume + // does. + fastify.decorate('ports', ports) + fastify.decorate('services', services) + return fastify } +function countMigrationOutcomes(report: MigrationReport): { migrated: number; failed: number } { + let migrated = report.profile.outcome === 'migrated' ? 1 : 0 + let failed = report.profile.outcome === 'failed' ? 1 : 0 + for (const book of report.books) { + if (book.outcome === 'migrated') migrated++ + if (book.outcome === 'failed') failed++ + } + return { migrated, failed } +} + /** - * Builds the server, runs crash recovery, and listens. `overrides` is - * forwarded to {@link buildServer}, which is how Electron hands in its own - * BrowserWindow-backed diagram renderer at startup instead of reaching - * into the built instance and reassigning a decoration afterwards. + * The startup sequence startServer runs before it binds a port, pulled out + * on its own so a test can drive it directly, with fakes, without binding + * a real port or standing up a full listening server. + * + * Order is load bearing in both directions. Migration has to run before + * crash recovery, because crash recovery reads and writes BookMeta through + * the CURRENT Zod schema, via BookRepository, so a book still at an old + * schema version would be silently skipped by listBooks's own try/catch, + * per fs-book-repository.ts, and would never be reached by recovery at + * all. Running the migrator first is what guarantees every book crash + * recovery sees is one BookRepository can actually read. + * + * Resume has to run after crash recovery, for the same kind of reason in + * the other direction. Resume's own decisions read the exact BookMeta and + * audio state recovery just finished reconciling, most concretely: audio + * resume in resume-interrupted-jobs.ts restarts narration from the + * beginning specifically because, by the time resume runs, recovery has + * already deleted any partial audio and cleared audioGeneratedChapters + * (see that module's own header for the full chain). Running resume before + * recovery finished would race the same BookMeta file and could let resume + * act on audio state recovery was about to wipe out from under it. */ -export async function startServer(port = 3147, host = '127.0.0.1', overrides: Partial = {}) { - const fastify = await buildServer(overrides) +export async function runStartupTasks(ports: Ports, services: SharedServices, log: FastifyBaseLogger): Promise { + const migration = await ports.libraryMigrator.migrate() + const outcomes = countMigrationOutcomes(migration) + if (outcomes.migrated > 0 || outcomes.failed > 0) { + log.info(outcomes, 'Library migration completed') + } - // A second createPorts(overrides) call, independent of the one buildServer - // made for the routes it registered. bookRepository and artifactStore are - // both stateless bridges to the same on-disk data either way (or, in a - // test, the exact same override object either time, since createPorts - // always applies overrides last), so this composes identically to how the - // book-store.js shim this replaced built its own adapters from getDataDir() - // rather than reusing buildServer's. - const ports = createPorts(overrides) const recoverFromCrash = createRecoverFromCrash({ bookRepository: ports.bookRepository, artifactStore: ports.artifactStore, @@ -167,11 +216,77 @@ export async function startServer(port = 3147, host = '127.0.0.1', overrides: Pa }) const recovery = await recoverFromCrash() if (recovery.booksReset.length > 0 || recovery.artifactsRemoved.length > 0) { - fastify.log.info( + log.info( { booksReset: recovery.booksReset.length, artifactsRemoved: recovery.artifactsRemoved.length }, 'Crash recovery completed', ) } + + // Built here, once per boot, rather than reused from a route module, + // because resume needs its own generateAllChapters and generateAudiobook + // wired to the job journal (so a job it restarts gets checkpointed) and + // the routes' own instances, in routes/generation.ts and + // routes/audiobook-generation.ts, are not. Both are the same stateless + // factory pattern every other service in this file already uses; a + // second instance built from the same ports and shared services behaves + // identically to the routes' own. + const generateNextChapter = createGenerateNextChapter({ ai: ports.textGeneration, books: ports.bookRepository, clock: ports.clock }) + const generateAllChapters = createGenerateAllChapters({ + backgroundTasks: ports.backgroundTasks, + chapterStream: services.chapterGenerationStream, + generateNextChapter, + journal: ports.jobJournal, + }) + const generateAudiobook = createGenerateAudiobook({ + bookRepository: ports.bookRepository, + artifactStore: ports.artifactStore, + speechSynthesis: ports.speechSynthesis, + audioAssembly: ports.audioAssembly, + backgroundTasks: ports.backgroundTasks, + journal: ports.jobJournal, + }) + const resumeInterruptedJobs = createResumeInterruptedJobs({ + journal: ports.jobJournal, + books: ports.bookRepository, + backgroundTasks: ports.backgroundTasks, + chapterStream: services.chapterGenerationStream, + generateAllChapters, + generateAudiobook, + // A debugging escape hatch, exactly '1' rather than any truthy string, + // so an accidental TUTOR_NO_AUTO_RESUME=0 or a typo does not silently + // disable resume. + autoResume: process.env.TUTOR_NO_AUTO_RESUME !== '1', + log: (msg, ctx) => log.info(ctx ?? {}, msg), + }) + const resume = await resumeInterruptedJobs() + if (resume.resumed.length > 0 || resume.markedErrored.length > 0 || resume.skipped.length > 0) { + log.info( + { resumed: resume.resumed.length, markedErrored: resume.markedErrored.length, skipped: resume.skipped.length }, + 'Interrupted job resume completed', + ) + } +} + +/** + * Builds the server, runs startup migration and crash recovery, and + * listens. `overrides` is forwarded to {@link buildServer}, which is how + * Electron hands in its own BrowserWindow-backed diagram renderer at + * startup instead of reaching into the built instance and reassigning a + * decoration afterwards. + */ +export async function startServer(port = 3147, host = '127.0.0.1', overrides: Partial = {}) { + const fastify = await buildServer(overrides) + + // The ports and shared services the routes were actually registered + // with, read back off the instance rather than built a second time. This + // used to be its own createPorts(overrides) call, which was fine while + // startup only ran migration and crash recovery, since both talk to + // stateless bridges over the same data directory. It stops being fine + // the moment startup touches in-memory state, which resume does through + // services.chapterGenerationStream. See the decoration in buildServer + // for the full reason. + await runStartupTasks(fastify.ports, fastify.services, fastify.log) + await fastify.listen({ port, host }) return fastify } diff --git a/server/migrations/README.md b/server/migrations/README.md new file mode 100644 index 0000000..5763346 --- /dev/null +++ b/server/migrations/README.md @@ -0,0 +1,42 @@ +# server/migrations/ + +Forward-only, pure migrations for the two things Tutor persists at the top +of a data directory: a book's `meta.yml` and the library's +`learning-profile.yml`. Each has its own version counter +(`shared/schema-version.ts` explains why two counters instead of one) and +its own chain of steps, `book/` and `profile/`, walked by the same +`migrateForward` in `migrate.ts`. + +A step is a pure function, `(raw: Record) => Record`, with no `fs`, no zod, no knowledge of anything but the shape it +turns into the next shape. The I/O half, reading a file, calling +`migrateForward`, and writing the result plus a one-time backup, is the +`LibraryMigrator` port, a later phase task, not this directory. + +## Adding a migration + +1. Bump `CURRENT_BOOK_SCHEMA_VERSION` or `CURRENT_PROFILE_SCHEMA_VERSION` in + `shared/schema-version.ts`. +2. Edit the schema in `shared/domain.ts` to match the new shape. +3. Add `NNN-name.ts` under `book/` or `profile/` as a `MigrationStep` whose + `to` is the version you just bumped to, and register it in that + directory's `index.ts`. +4. Commit a fixture library at the PREVIOUS version under `__fixtures__/` + (see `__fixtures__/README.md`). Never edit an existing fixture, a + fixture is a historical record of what some released build actually + wrote, not something to bring up to date. +5. Add the round-trip test: run the new step over the new fixture, assert + the fields it adds and the fields it must leave alone, and assert the + result parses under the current Zod schema. + +`chains.test.ts` fails the moment a version constant is bumped without a +matching step added to that chain's `index.ts`. Skipping step 3 above is +therefore a test failure at the next `pnpm test`, not a runtime surprise +discovered against someone's real library months later. + +Steps are pure and forward-only by design. There is no downgrade path, a +step only ever knows how to turn `to - 1` into `to`, never the reverse. +That is deliberate. A bad forward migration can be fixed by writing another +one, but a migration nobody wrote is a step this app has no way to reverse +safely, so a library newer than a build supports fails loudly +(`SchemaTooNewError`) instead of a downgrade silently corrupting it. diff --git a/server/migrations/__fixtures__/README.md b/server/migrations/__fixtures__/README.md new file mode 100644 index 0000000..ebf7929 --- /dev/null +++ b/server/migrations/__fixtures__/README.md @@ -0,0 +1,29 @@ +# Migration fixtures + +Each directory here is a complete stand-in for a Tutor data directory +(`{dataDir}`) frozen at an old schema version. Tests copy one into a fresh +`mkdtemp()` and run the real `LibraryMigrator` against the copy, so the +committed fixture is never mutated and the migrator is exercised against +real files rather than an in-memory mock of a filesystem. + +`{dataDir}/books/` holds both `learning-profile.yml` and one directory per +book, which is why every fixture has that nesting. + +| Fixture | What it pins | +|---|---| +| `v1-library` | The ordinary case. A profile plus two books at version 1, one finished and one part way through generation. | +| `v1-profile-only` | A fresh install where the user set up a profile and never made a book. Proves the profile counter is independent of the book counter. | +| `v1-corrupt-book` | One readable book beside one whose `meta.yml` is not valid YAML. Proves a single bad book is reported rather than aborting the whole migration. | + +Version 1 is what the app wrote before `schemaVersion` existed, so no fixture +file here carries that field. Version 1 also omits every value the Zod schemas +backfill with `.default()` at read time, `tags` and `audioGeneratedChapters` +on a book and `skills` on the profile, because the app never wrote them to +disk. Materializing exactly those is what migration `001` does. + +**Do not "fix" a fixture.** A fixture is a historical record of what some +released build actually wrote. If it looks wrong or incomplete, that is the +point. Adding a new migration means adding a NEW fixture at the version +before it, and leaving these alone forever. + +Up: [server/migrations](../README.md) diff --git a/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/chapters/01.md b/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/chapters/01.md new file mode 100644 index 0000000..f8dba0a --- /dev/null +++ b/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/chapters/01.md @@ -0,0 +1,5 @@ +# A Book That Reads Fine + +One chapter of ordinary content, present so this fixture book is a complete +book directory rather than a bare `meta.yml`. Its neighbour in this fixture +is deliberately broken. diff --git a/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/meta.yml b/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/meta.yml new file mode 100644 index 0000000..2e1f0ab --- /dev/null +++ b/server/migrations/__fixtures__/v1-corrupt-book/books/readable-book/meta.yml @@ -0,0 +1,8 @@ +id: readable-book +title: A Book That Reads Fine +prompt: Anything at all, this fixture exists to prove the good book still migrates. +status: reading +totalChapters: 1 +generatedUpTo: 1 +createdAt: "2026-01-08T11:00:00.000Z" +updatedAt: "2026-01-08T11:30:00.000Z" diff --git a/server/migrations/__fixtures__/v1-corrupt-book/books/truncated-book/meta.yml b/server/migrations/__fixtures__/v1-corrupt-book/books/truncated-book/meta.yml new file mode 100644 index 0000000..06103de --- /dev/null +++ b/server/migrations/__fixtures__/v1-corrupt-book/books/truncated-book/meta.yml @@ -0,0 +1,5 @@ +id: truncated-book +title: "A Book Whose Write Was Interrupted +prompt: The unterminated quote on the line above is the corruption. Do not fix it. +status: reading +totalChapters: [ diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/01.md b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/01.md new file mode 100644 index 0000000..3c8b0a3 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/01.md @@ -0,0 +1,30 @@ +# The Two Generals Problem + +Two armies camp on opposite hills. Between them sits the city they mean to +take, and the only way a messenger gets from one camp to the other is +straight through it. A messenger who leaves might arrive, or might not, and +the general who sent him has no way to tell which happened. + +Both generals must attack at the same dawn or both will lose. So the first +general sends a rider carrying a time. The rider may be captured, so the +first general cannot act on a message he does not know arrived. The second +general, receiving it, faces the same problem in reverse. His acknowledgment +may be captured too, and the first general cannot attack without it. + +Adding another acknowledgment does not help. Whatever the last message in +the chain is, its sender does not know it arrived, so that sender cannot +safely act. The chain has no end that terminates the reasoning. + +## What the impossibility actually rules out + +The result is often quoted as proof that distributed agreement is +impossible. It is narrower than that. What is impossible is *certain* +agreement over a channel that can drop messages, in a bounded number of +rounds, with no probability left on the table. + +Real systems buy their way out by relaxing exactly one of those. They accept +agreement that is certain only with high probability. They accept unbounded +rounds and keep retrying. Or they change the channel, moving to one where a +message either arrives or is detectably lost. + +Chapter two takes the third route and derives Paxos from it. diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/02.md b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/02.md new file mode 100644 index 0000000..3e1656c --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/chapters/02.md @@ -0,0 +1,36 @@ +# Paxos, Slowly + +Start from what chapter one left us. Messages can be lost and delayed, but +never forged. Any number of participants can crash and later recover. We +want a single value chosen, once, such that no participant ever learns a +different one. + +The naive design has each proposer send its value to every acceptor, and an +acceptor take the first value it hears. That fails immediately. Two +proposers race, the acceptors split, and no value has a majority. + +## Why proposals need numbers + +The fix is to make proposals comparable. Every proposal carries a number, +unique to its proposer and increasing over time. An acceptor promises never +to accept a proposal numbered lower than the highest it has already +promised to. + +That promise is the whole mechanism. It lets a later proposer safely +override an earlier one, and it lets an acceptor tell a stale proposer to go +away without any coordination. + +## The two phases + +In the prepare phase a proposer picks a number and asks a majority of +acceptors to promise. Each acceptor that promises reports back the +highest-numbered value it has already accepted, if any. + +In the accept phase the proposer must propose the value that came back with +the highest number, if one came back at all. Only if every acceptor reported +nothing may the proposer use its own value. + +That single constraint is what makes the algorithm safe. Once a value has +been accepted by a majority, every future proposer is forced to re-propose +it, because any majority it prepares against overlaps that first majority in +at least one acceptor. diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/feedback/01.yml b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/feedback/01.yml new file mode 100644 index 0000000..bf9808b --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/feedback/01.yml @@ -0,0 +1,16 @@ +chapter: 1 +feedback: + liked: The framing of what the impossibility does not rule out. + disliked: Wanted one more concrete failure trace. +quiz: + questions: + - question: What does the Two Generals result actually rule out? + options: + - All distributed agreement + - Certain agreement in bounded rounds over a lossy channel + - Agreement between exactly two parties + - Agreement without a shared clock + correctIndex: 1 + userAnswer: 1 + correct: true + score: 1 diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/meta.yml b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/meta.yml new file mode 100644 index 0000000..6dc4251 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/meta.yml @@ -0,0 +1,12 @@ +id: consensus-protocols +title: Consensus Protocols +subtitle: From Two Generals to Raft +prompt: Teach me how distributed consensus actually works, building up from first principles. +status: complete +totalChapters: 2 +generatedUpTo: 2 +createdAt: "2025-11-02T09:14:03.221Z" +updatedAt: "2025-11-02T10:41:55.807Z" +rating: 4.5 +finalQuizScore: 8 +finalQuizTotal: 10 diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/progress.yml b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/progress.yml new file mode 100644 index 0000000..908487e --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/progress.yml @@ -0,0 +1,9 @@ +chapters: + "1": + scroll: 1 + completed: true + completedAt: "2025-11-02T09:52:10.004Z" + "2": + scroll: 1 + completed: true + completedAt: "2025-11-02T10:41:55.807Z" diff --git a/server/migrations/__fixtures__/v1-library/books/consensus-protocols/toc.yml b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/toc.yml new file mode 100644 index 0000000..467f7b3 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/consensus-protocols/toc.yml @@ -0,0 +1,9 @@ +chapters: + - title: The Two Generals Problem + description: >- + Why agreement over an unreliable channel is impossible in the general + case, and what that impossibility actually rules out. + - title: Paxos, Slowly + description: >- + Proposers, acceptors, and the two phases, derived rather than + presented. diff --git a/server/migrations/__fixtures__/v1-library/books/learning-profile.yml b/server/migrations/__fixtures__/v1-library/books/learning-profile.yml new file mode 100644 index 0000000..0c63299 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/learning-profile.yml @@ -0,0 +1,17 @@ +style: >- + Concrete example first, then the general rule. Short paragraphs. Assume + comfort reading code. +identity: A backend engineer moving into distributed systems work. +preferences: + explainComplexTermsSimply: true + codeExamples: true + realWorldAnalogies: true + includeRecaps: true + includeSummaries: true + visualDescriptions: false + depthLevel: 4 + pacePreference: 3 + metaphorDensity: 2 + narrativeStyle: 3 + humorLevel: 2 + formalityLevel: 3 diff --git a/server/migrations/__fixtures__/v1-library/books/vector-clocks/chapters/01.md b/server/migrations/__fixtures__/v1-library/books/vector-clocks/chapters/01.md new file mode 100644 index 0000000..6fd3c28 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/vector-clocks/chapters/01.md @@ -0,0 +1,26 @@ +# Ordering Without a Clock + +Two servers each write a record and each stamps it with the time its own +clock reads. The stamps say the second write happened first. Nothing is +broken, the clocks simply disagree by a few milliseconds, which is normal +and unavoidable. + +The mistake is not the drift. The mistake is asking wall time a question it +cannot answer. Wall time on two machines is two independent measurements of +the same abstraction, and comparing them tells you about the clocks, not +about the events. + +## Happens-before + +Lamport's replacement is a relation, not a number. Event A happens-before +event B when one of three things is true. They happen on the same machine +and A comes first. A is a send and B is the matching receive. Or there is a +chain of the first two rules connecting them. + +That is the whole definition, and everything else in this book is derived +from it. Notice what it does not say. Two events with no chain between them +are not ordered at all. They are concurrent, and concurrent is a real answer +rather than a missing one. + +Chapter two tries to compress this relation into a single number, and shows +exactly what gets lost. diff --git a/server/migrations/__fixtures__/v1-library/books/vector-clocks/meta.yml b/server/migrations/__fixtures__/v1-library/books/vector-clocks/meta.yml new file mode 100644 index 0000000..73d24f0 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/vector-clocks/meta.yml @@ -0,0 +1,11 @@ +id: vector-clocks +title: Vector Clocks +prompt: Explain causality tracking in distributed systems without hand-waving the ordering. +status: reading +totalChapters: 3 +generatedUpTo: 1 +createdAt: "2025-12-14T18:02:44.913Z" +updatedAt: "2025-12-14T18:19:07.556Z" +series: Distributed Systems +seriesOrder: 2 +sortOrder: 10 diff --git a/server/migrations/__fixtures__/v1-library/books/vector-clocks/toc.yml b/server/migrations/__fixtures__/v1-library/books/vector-clocks/toc.yml new file mode 100644 index 0000000..5237097 --- /dev/null +++ b/server/migrations/__fixtures__/v1-library/books/vector-clocks/toc.yml @@ -0,0 +1,13 @@ +chapters: + - title: Ordering Without a Clock + description: >- + Why wall time cannot order events across machines, and what happens-before + replaces it with. + - title: Lamport Timestamps and Their Blind Spot + description: >- + A single counter gives a total order, and loses the one distinction that + matters. + - title: Vector Clocks + description: >- + One counter per participant, and the partial order that falls out of + comparing them. diff --git a/server/migrations/__fixtures__/v1-profile-only/books/learning-profile.yml b/server/migrations/__fixtures__/v1-profile-only/books/learning-profile.yml new file mode 100644 index 0000000..5c072cc --- /dev/null +++ b/server/migrations/__fixtures__/v1-profile-only/books/learning-profile.yml @@ -0,0 +1,15 @@ +style: Plain language, no jargon until it has been earned. +identity: A product designer learning enough engineering to argue with engineers. +preferences: + explainComplexTermsSimply: true + codeExamples: false + realWorldAnalogies: true + includeRecaps: true + includeSummaries: true + visualDescriptions: true + depthLevel: 2 + pacePreference: 4 + metaphorDensity: 4 + narrativeStyle: 4 + humorLevel: 3 + formalityLevel: 2 diff --git a/server/migrations/book/001-materialize-defaults.test.ts b/server/migrations/book/001-materialize-defaults.test.ts new file mode 100644 index 0000000..0f8ba6f --- /dev/null +++ b/server/migrations/book/001-materialize-defaults.test.ts @@ -0,0 +1,81 @@ +import { readFile } from 'node:fs/promises' +import { fileURLToPath } from 'node:url' +import { parse as parseYaml } from 'yaml' +import { describe, it, expect } from 'vitest' +import { BookMetaSchema } from '@shared/domain.js' +import { materializeBookDefaults } from './001-materialize-defaults.js' + +// Exercised against the real, committed v1 fixtures rather than an inline +// stand-in, so this proves the step against bytes an actual released build +// once wrote, not against a shape a test author guessed at. See +// server/migrations/__fixtures__/README.md. Read raw with the same two +// primitives the fixtures README promises callers will use: node:fs and the +// yaml package, never readYaml, which is the I/O half's job and belongs to +// a later task. + +async function readRawFixture(relativePathUnderFixtures: string): Promise> { + const url = new URL(`../__fixtures__/${relativePathUnderFixtures}`, import.meta.url) + const content = await readFile(fileURLToPath(url), 'utf-8') + return parseYaml(content) +} + +describe('materializeBookDefaults', () => { + it('is a step that produces schema version 2', () => { + expect(materializeBookDefaults.to).toBe(2) + }) + + it('materializes tags and audioGeneratedChapters onto consensus-protocols while preserving its fields', async () => { + const raw = await readRawFixture('v1-library/books/consensus-protocols/meta.yml') + const result = materializeBookDefaults.migrate(raw) + + expect(result.tags).toEqual([]) + expect(result.audioGeneratedChapters).toEqual([]) + + // Fields only this fixture has, spelled out explicitly. + expect(result.rating).toBe(4.5) + expect(result.finalQuizScore).toBe(8) + expect(result.finalQuizTotal).toBe(10) + + // The rest of the fixture, untouched. + expect(result.id).toBe('consensus-protocols') + expect(result.title).toBe('Consensus Protocols') + expect(result.subtitle).toBe('From Two Generals to Raft') + expect(result.prompt).toBe( + 'Teach me how distributed consensus actually works, building up from first principles.', + ) + expect(result.status).toBe('complete') + expect(result.totalChapters).toBe(2) + expect(result.generatedUpTo).toBe(2) + expect(result.createdAt).toBe('2025-11-02T09:14:03.221Z') + expect(result.updatedAt).toBe('2025-11-02T10:41:55.807Z') + }) + + it('materializes tags and audioGeneratedChapters onto vector-clocks while preserving its series fields', async () => { + const raw = await readRawFixture('v1-library/books/vector-clocks/meta.yml') + const result = materializeBookDefaults.migrate(raw) + + expect(result.tags).toEqual([]) + expect(result.audioGeneratedChapters).toEqual([]) + + expect(result.series).toBe('Distributed Systems') + expect(result.seriesOrder).toBe(2) + expect(result.sortOrder).toBe(10) + + expect(result.id).toBe('vector-clocks') + expect(result.status).toBe('reading') + expect(result.generatedUpTo).toBe(1) + }) + + it('never clobbers an already-populated tags value', () => { + const raw = { id: 'has-tags', tags: ['distributed-systems'], audioGeneratedChapters: [1] } + const result = materializeBookDefaults.migrate(raw) + expect(result.tags).toEqual(['distributed-systems']) + expect(result.audioGeneratedChapters).toEqual([1]) + }) + + it('produces output that parses cleanly under BookMetaSchema', async () => { + const raw = await readRawFixture('v1-library/books/consensus-protocols/meta.yml') + const result = materializeBookDefaults.migrate(raw) + expect(() => BookMetaSchema.parse(result)).not.toThrow() + }) +}) diff --git a/server/migrations/book/001-materialize-defaults.ts b/server/migrations/book/001-materialize-defaults.ts new file mode 100644 index 0000000..211c782 --- /dev/null +++ b/server/migrations/book/001-materialize-defaults.ts @@ -0,0 +1,26 @@ +import type { MigrationStep } from '../migrate.js' + +/** + * Migration 001: writes out the fields BookMetaSchema has always backfilled + * at read time with `.default([])`, `tags` and `audioGeneratedChapters`. A + * v1 `meta.yml` never wrote either field, since neither existed yet, so + * both are simply absent on disk. Materializing them makes a migrated file + * self-describing: what is on disk is what the book actually has, instead + * of being whatever a Zod default happens to backfill in the build that + * happens to read it. + * + * Every other field passes through untouched. This step only ever adds the + * two fields it owns, and only when they are missing, it never overwrites + * a value some other write path already set. + */ +export const materializeBookDefaults: MigrationStep = { + to: 2, + name: '001-materialize-defaults', + migrate(raw) { + return { + ...raw, + tags: raw.tags ?? [], + audioGeneratedChapters: raw.audioGeneratedChapters ?? [], + } + }, +} diff --git a/server/migrations/book/index.ts b/server/migrations/book/index.ts new file mode 100644 index 0000000..612a271 --- /dev/null +++ b/server/migrations/book/index.ts @@ -0,0 +1,10 @@ +import type { MigrationStep } from '../migrate.js' +import { materializeBookDefaults } from './001-materialize-defaults.js' + +/** + * The book migration chain, in order. Checked for contiguity against + * CURRENT_BOOK_SCHEMA_VERSION by chains.test.ts, so an entry missing from + * this array fails a test rather than silently shipping. To add a + * migration, see server/migrations/README.md. + */ +export const BOOK_MIGRATIONS: readonly MigrationStep[] = [materializeBookDefaults] diff --git a/server/migrations/chains.test.ts b/server/migrations/chains.test.ts new file mode 100644 index 0000000..a9222e0 --- /dev/null +++ b/server/migrations/chains.test.ts @@ -0,0 +1,26 @@ +import { describe, it, expect } from 'vitest' +import { CURRENT_BOOK_SCHEMA_VERSION, CURRENT_PROFILE_SCHEMA_VERSION } from '@shared/schema-version.js' +import { assertChainIntegrity } from './migrate.js' +import { BOOK_MIGRATIONS } from './book/index.js' +import { PROFILE_MIGRATIONS } from './profile/index.js' + +// This is the test that turns "bumped CURRENT_BOOK_SCHEMA_VERSION (or +// CURRENT_PROFILE_SCHEMA_VERSION) without adding the matching migration +// step" into a failing test instead of a library nobody can read. Every +// time either constant moves, this file either still passes because a step +// was added to close the gap, or fails here, loudly, before the change +// ships. + +describe('BOOK_MIGRATIONS', () => { + it('is a contiguous chain matching CURRENT_BOOK_SCHEMA_VERSION', () => { + expect(() => assertChainIntegrity(BOOK_MIGRATIONS, CURRENT_BOOK_SCHEMA_VERSION, 'book')).not.toThrow() + }) +}) + +describe('PROFILE_MIGRATIONS', () => { + it('is a contiguous chain matching CURRENT_PROFILE_SCHEMA_VERSION', () => { + expect(() => + assertChainIntegrity(PROFILE_MIGRATIONS, CURRENT_PROFILE_SCHEMA_VERSION, 'profile'), + ).not.toThrow() + }) +}) diff --git a/server/migrations/migrate.test.ts b/server/migrations/migrate.test.ts new file mode 100644 index 0000000..418119d --- /dev/null +++ b/server/migrations/migrate.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect } from 'vitest' +import { migrateForward, assertChainIntegrity, type MigrationStep } from './migrate.js' + +// Pure chain mechanics shared by every migration chain (book, profile, and +// whatever is added later). Exercised here with synthetic steps rather than +// the real ones, so a bug in a real migration's field logic can never hide +// a bug in the chain-walking logic underneath it, and vice versa. + +function markerStep(to: number, name: string): MigrationStep { + return { + to, + name, + migrate(raw) { + const applied = Array.isArray(raw.applied) ? raw.applied : [] + return { ...raw, applied: [...applied, name] } + }, + } +} + +const STEPS: readonly MigrationStep[] = [markerStep(2, 'a'), markerStep(3, 'b'), markerStep(4, 'c')] + +describe('migrateForward', () => { + it('applies exactly the steps whose `to` is greater than `from` and at most `to`, in array order', () => { + const result = migrateForward({ x: 1 }, 2, 4, STEPS) + // step "a" (to: 2) is skipped: 2 is not greater than from (2) + expect(result.applied).toEqual(['b', 'c']) + }) + + it('returns a record stamped with schemaVersion: to', () => { + const result = migrateForward({ x: 1 }, 1, 4, STEPS) + expect(result.schemaVersion).toBe(4) + }) + + it('applies no step and still stamps the version when from === to', () => { + const result = migrateForward({ x: 1 }, 3, 3, STEPS) + expect(result.applied).toBeUndefined() + expect(result.schemaVersion).toBe(3) + expect(result.x).toBe(1) + }) + + it('throws rather than migrating backward when from > to', () => { + expect(() => migrateForward({ x: 1 }, 3, 2, STEPS)).toThrow() + }) + + it('throws a clear error naming the problem when raw is not a plain object', () => { + expect(() => migrateForward(null, 1, 2, STEPS)).toThrow(/null/i) + expect(() => migrateForward(['array'], 1, 2, STEPS)).toThrow(/array/i) + expect(() => migrateForward('a string', 1, 2, STEPS)).toThrow(/string/i) + expect(() => migrateForward(42, 1, 2, STEPS)).toThrow(/number/i) + }) + + it('does not mutate the input object', () => { + // This matters because the real migrator holds onto the pre-migration + // value so it can write it out as a one-time backup file. + const raw = { x: 1, nested: { y: 2 } } + const snapshotBefore = structuredClone(raw) + migrateForward(raw, 1, 4, STEPS) + expect(raw).toEqual(snapshotBefore) + }) +}) + +describe('assertChainIntegrity', () => { + it('passes for a contiguous chain whose length is currentVersion - 1 and whose `to` values count up from 2', () => { + expect(() => assertChainIntegrity(STEPS, 4, 'test-chain')).not.toThrow() + }) + + it('throws, naming the label, when a step is missing from the middle', () => { + const gappy = [markerStep(2, 'a'), markerStep(4, 'c')] + expect(() => assertChainIntegrity(gappy, 4, 'test-chain')).toThrow(/test-chain/) + }) + + it('throws, naming the label, when `to` values are out of order', () => { + const outOfOrder = [markerStep(3, 'b'), markerStep(2, 'a')] + expect(() => assertChainIntegrity(outOfOrder, 3, 'test-chain')).toThrow(/test-chain/) + }) + + it('throws, naming the label, when `to` values are duplicated', () => { + const duplicated = [markerStep(2, 'a'), markerStep(2, 'a-again')] + expect(() => assertChainIntegrity(duplicated, 3, 'test-chain')).toThrow(/test-chain/) + }) + + it('throws, naming the label, when the chain length disagrees with currentVersion', () => { + expect(() => assertChainIntegrity(STEPS, 10, 'test-chain')).toThrow(/test-chain/) + }) +}) diff --git a/server/migrations/migrate.ts b/server/migrations/migrate.ts new file mode 100644 index 0000000..0389881 --- /dev/null +++ b/server/migrations/migrate.ts @@ -0,0 +1,87 @@ +/** + * Chain mechanics shared by every migration chain (book, profile, and + * whatever is added later). A MigrationStep is a pure function from one + * schema version's raw shape to the next; this module knows nothing about + * what a book or a profile looks like, only how to walk an ordered list of + * such steps and how to prove that list is complete. + * + * Pure by construction, no fs, no yaml parsing, no knowledge of where the + * raw data came from or where the result goes. The I/O half, reading a + * file, calling migrateForward, and writing the result plus a one-time + * backup, is a later task's LibraryMigrator port. + */ + +export interface MigrationStep { + /** The schema version this step produces. The first step in any chain is 2, since 1 is defined as "no migration has ever run." */ + readonly to: number + /** A short, stable identifier for logging and for naming in error messages, e.g. '001-materialize-defaults'. */ + readonly name: string + /** Pure: takes the raw record at `to - 1` and returns the raw record at `to`. Must not mutate its argument. */ + migrate(raw: Record): Record +} + +function describeType(value: unknown): string { + if (value === null) return 'null' + if (Array.isArray(value)) return 'an array' + return `a ${typeof value}` +} + +/** + * Runs every step whose `to` falls in the (from, to] range, in array order, + * over a shallow copy of `raw`, then stamps the result with the target + * version. Copying `raw` up front, rather than trusting each step not to + * mutate its argument, is what lets migrateForward promise its own input is + * untouched regardless of how a step is written: the real caller keeps the + * pre-migration value around to write out as a `.bak` file, so this + * function can never be the thing that corrupts that backup. + */ +export function migrateForward( + raw: unknown, + from: number, + to: number, + steps: readonly MigrationStep[], +): Record { + if (from > to) { + throw new Error(`Cannot migrate backward from version ${from} to version ${to}.`) + } + if (typeof raw !== 'object' || raw === null || Array.isArray(raw)) { + throw new Error(`Cannot migrate ${describeType(raw)}: expected a plain object.`) + } + + let current: Record = { ...(raw as Record) } + for (const step of steps) { + if (step.to > from && step.to <= to) { + current = step.migrate(current) + } + } + return { ...current, schemaVersion: to } +} + +/** + * Fails the build, not just a boot, when a chain is missing a step, has a + * step out of order or duplicated, or disagrees in length with the current + * version constant. This is what turns "bumped CURRENT_BOOK_SCHEMA_VERSION + * and forgot to write the migration" into a test failure instead of a + * library nobody can read. `label` is threaded into every message so a + * failure names which chain, book, profile, or a future one, is broken. + */ +export function assertChainIntegrity( + steps: readonly MigrationStep[], + currentVersion: number, + label: string, +): void { + const expectedLength = currentVersion - 1 + if (steps.length !== expectedLength) { + throw new Error( + `${label} migration chain has ${steps.length} step(s) but CURRENT version ${currentVersion} requires exactly ${expectedLength}. Did you bump the version constant without adding a step, or add a step without bumping it?`, + ) + } + steps.forEach((step, index) => { + const expectedTo = index + 2 + if (step.to !== expectedTo) { + throw new Error( + `${label} migration chain is broken at index ${index}: step "${step.name}" produces version ${step.to}, but a contiguous chain requires ${expectedTo} at this position.`, + ) + } + }) +} diff --git a/server/migrations/profile/001-materialize-defaults.test.ts b/server/migrations/profile/001-materialize-defaults.test.ts new file mode 100644 index 0000000..1a67c49 --- /dev/null +++ b/server/migrations/profile/001-materialize-defaults.test.ts @@ -0,0 +1,79 @@ +import { readFile } from 'node:fs/promises' +import { fileURLToPath } from 'node:url' +import { parse as parseYaml } from 'yaml' +import { describe, it, expect } from 'vitest' +import { LearningProfileSchema } from '@shared/domain.js' +import { materializeProfileDefaults } from './001-materialize-defaults.js' + +// Exercised against the real, committed v1 fixtures. See +// server/migrations/__fixtures__/README.md. + +async function readRawFixture(relativePathUnderFixtures: string): Promise> { + const url = new URL(`../__fixtures__/${relativePathUnderFixtures}`, import.meta.url) + const content = await readFile(fileURLToPath(url), 'utf-8') + return parseYaml(content) +} + +describe('materializeProfileDefaults', () => { + it('is a step that produces schema version 2', () => { + expect(materializeProfileDefaults.to).toBe(2) + }) + + it('materializes skills and preserves every preference on the library profile', async () => { + const raw = await readRawFixture('v1-library/books/learning-profile.yml') + const result = materializeProfileDefaults.migrate(raw) + + expect(result.skills).toEqual([]) + expect(result.style).toContain('Concrete example first') + expect(result.identity).toBe('A backend engineer moving into distributed systems work.') + + const preferences = result.preferences as Record + expect(preferences.explainComplexTermsSimply).toBe(true) + expect(preferences.codeExamples).toBe(true) + expect(preferences.realWorldAnalogies).toBe(true) + expect(preferences.includeRecaps).toBe(true) + expect(preferences.includeSummaries).toBe(true) + expect(preferences.visualDescriptions).toBe(false) + expect(preferences.depthLevel).toBe(4) + expect(preferences.pacePreference).toBe(3) + expect(preferences.metaphorDensity).toBe(2) + expect(preferences.narrativeStyle).toBe(3) + expect(preferences.humorLevel).toBe(2) + expect(preferences.formalityLevel).toBe(3) + }) + + it('materializes skills and preserves every preference on the profile-only fixture', async () => { + const raw = await readRawFixture('v1-profile-only/books/learning-profile.yml') + const result = materializeProfileDefaults.migrate(raw) + + expect(result.skills).toEqual([]) + expect(result.style).toBe('Plain language, no jargon until it has been earned.') + expect(result.identity).toBe('A product designer learning enough engineering to argue with engineers.') + + const preferences = result.preferences as Record + expect(preferences.explainComplexTermsSimply).toBe(true) + expect(preferences.codeExamples).toBe(false) + expect(preferences.realWorldAnalogies).toBe(true) + expect(preferences.includeRecaps).toBe(true) + expect(preferences.includeSummaries).toBe(true) + expect(preferences.visualDescriptions).toBe(true) + expect(preferences.depthLevel).toBe(2) + expect(preferences.pacePreference).toBe(4) + expect(preferences.metaphorDensity).toBe(4) + expect(preferences.narrativeStyle).toBe(4) + expect(preferences.humorLevel).toBe(3) + expect(preferences.formalityLevel).toBe(2) + }) + + it('never clobbers an already-populated skills value', () => { + const raw = { style: 'x', skills: [{ name: 'Rust', level: 3 }] } + const result = materializeProfileDefaults.migrate(raw) + expect(result.skills).toEqual([{ name: 'Rust', level: 3 }]) + }) + + it('produces output that parses cleanly under LearningProfileSchema', async () => { + const raw = await readRawFixture('v1-library/books/learning-profile.yml') + const result = materializeProfileDefaults.migrate(raw) + expect(() => LearningProfileSchema.parse(result)).not.toThrow() + }) +}) diff --git a/server/migrations/profile/001-materialize-defaults.ts b/server/migrations/profile/001-materialize-defaults.ts new file mode 100644 index 0000000..a3238df --- /dev/null +++ b/server/migrations/profile/001-materialize-defaults.ts @@ -0,0 +1,24 @@ +import type { MigrationStep } from '../migrate.js' + +/** + * Migration 001: writes out `skills`, the one field LearningProfileSchema + * has always backfilled at read time with `.default([])`. A v1 + * `learning-profile.yml` never wrote it, since it did not exist yet, so it + * is simply absent on disk. Materializing it makes a migrated file + * self-describing rather than dependent on whichever Zod default happens + * to backfill it. + * + * `style`, `identity`, and every `preferences` value pass through + * untouched. This step only ever adds `skills`, and only when missing, it + * never overwrites a skills list some other write path already set. + */ +export const materializeProfileDefaults: MigrationStep = { + to: 2, + name: '001-materialize-defaults', + migrate(raw) { + return { + ...raw, + skills: raw.skills ?? [], + } + }, +} diff --git a/server/migrations/profile/index.ts b/server/migrations/profile/index.ts new file mode 100644 index 0000000..70cc0c8 --- /dev/null +++ b/server/migrations/profile/index.ts @@ -0,0 +1,10 @@ +import type { MigrationStep } from '../migrate.js' +import { materializeProfileDefaults } from './001-materialize-defaults.js' + +/** + * The profile migration chain, in order. Checked for contiguity against + * CURRENT_PROFILE_SCHEMA_VERSION by chains.test.ts, so an entry missing + * from this array fails a test rather than silently shipping. To add a + * migration, see server/migrations/README.md. + */ +export const PROFILE_MIGRATIONS: readonly MigrationStep[] = [materializeProfileDefaults] diff --git a/server/ports/ai-error-kind.drift.test.ts b/server/ports/ai-error-kind.drift.test.ts new file mode 100644 index 0000000..abe3983 --- /dev/null +++ b/server/ports/ai-error-kind.drift.test.ts @@ -0,0 +1,23 @@ +import { expect, it } from 'vitest' +import type { AiErrorKind } from '@shared/responses.js' +import type { TextGenerationErrorKind } from './text-generation.js' + +/** + * This file pins `AiErrorKind` in shared/responses.ts and + * `TextGenerationErrorKind` in server/ports/text-generation.ts to the same + * set of members. Neither type imports the other, since the client depends + * on `AiErrorKind` without pulling in anything under server/, so nothing + * else stops the two unions from drifting apart. Editing one union without + * the other fails `pnpm typecheck` on the two assignments below, not this + * file's runtime assertion. That assertion exists only so vitest has a + * test to run in this file. + */ + +const _a: AiErrorKind = null as unknown as TextGenerationErrorKind +const _b: TextGenerationErrorKind = null as unknown as AiErrorKind +void _a +void _b + +it('is a compile-time-only guard, see the file header comment', () => { + expect(true).toBe(true) +}) diff --git a/server/ports/background-tasks.ts b/server/ports/background-tasks.ts index a2fea57..0609322 100644 --- a/server/ports/background-tasks.ts +++ b/server/ports/background-tasks.ts @@ -1,5 +1,6 @@ import type { ClientTask, TaskType } from '@shared/responses.js' import type { TaskEvent } from '@shared/events.js' +import type { GenerationJobParams } from '@shared/domain.js' /** * Tracks long-running background jobs (EPUB export, cover generation, @@ -44,6 +45,14 @@ export interface StartTaskSpec { bookId: string bookTitle: string total: number + /** + * Carried through to the job journal (server/ports/job-journal.ts) so an + * interrupted job can be restarted with the same request parameters. + * Optional because most task types need nothing to restart, cover + * generation and EPUB export just redo their one deterministic step. An + * API key must never be put here, see GenerationJobParamsSchema. + */ + params?: GenerationJobParams } export interface BackgroundTasks { diff --git a/server/ports/book-repository.contract.ts b/server/ports/book-repository.contract.ts index 5b62579..d1f181f 100644 --- a/server/ports/book-repository.contract.ts +++ b/server/ports/book-repository.contract.ts @@ -147,7 +147,19 @@ export function describeBookRepositoryContract( it('round trips a saved book', async () => { await repo.saveBook(makeBookMeta()) const book = await repo.getBook('book-1') - expect(book).toEqual(makeBookMeta()) + // schemaVersion is deliberately excluded from this comparison. A + // real adapter stamps the current version onto every write, per + // fs-book-repository.ts, while this contract's in-memory fake + // stores exactly what it was given, so asserting on that field here + // would make the contract adapter-specific instead of shared. + // + // Do not helpfully add it back. The stamp is not untested, it is + // asserted where it is actually true, against the bytes on disk, in + // server/adapters/fs-book-repository.test.ts, which reads meta.yml + // and learning-profile.yml back as raw YAML and expects + // schemaVersion to be the current version in both. + const { schemaVersion: _schemaVersion, ...rest } = book + expect(rest).toEqual(makeBookMeta()) }) it('reflects a saved book in the listing', async () => { diff --git a/server/ports/job-journal.contract.ts b/server/ports/job-journal.contract.ts new file mode 100644 index 0000000..42e75e0 --- /dev/null +++ b/server/ports/job-journal.contract.ts @@ -0,0 +1,121 @@ +import { describe, expect, it } from 'vitest' +import type { GenerationJob, GenerationJobCheckpoint } from '@shared/domain.js' +import type { JobJournal } from './job-journal.js' + +const JOB: GenerationJob = { + id: 'job-1', + type: 'generate-chapter', + bookId: 'book-1', + bookTitle: 'Test Book', + status: 'running', + checkpoint: { kind: 'none' }, + params: { provider: 'anthropic', model: 'claude-sonnet-4-20250514', targetChapterNum: 3 }, + startedAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:00:00.000Z', +} + +/** + * Behavior every JobJournal implementation must satisfy. Written against + * the JobJournal surface only, so this suite can run against the fake now + * and the real filesystem adapter later. Every case awaits subject.flush() + * before asserting on listInterrupted(), since record/checkpoint/clear are + * synchronous and may queue their write rather than complete it inline. + */ +export function describeJobJournalContract(label: string, makeSubject: () => JobJournal | Promise) { + describe(`JobJournal contract (${label})`, () => { + it('a recorded job appears in listInterrupted with every field preserved, including params exactly', async () => { + const subject = await makeSubject() + subject.record(JOB) + await subject.flush() + + const jobs = await subject.listInterrupted() + const found = jobs.find((j) => j.id === JOB.id) + expect(found).toBeDefined() + expect(found).toMatchObject({ + id: JOB.id, + type: JOB.type, + bookId: JOB.bookId, + bookTitle: JOB.bookTitle, + checkpoint: JOB.checkpoint, + params: JOB.params, + startedAt: JOB.startedAt, + updatedAt: JOB.updatedAt, + }) + }) + + // A record still present on disk can only mean the process died before + // clear() ran, so the reader reports the truth rather than echoing the + // status written at record time. Both values in the union are recorded + // here, and both must read back as 'interrupted', which pins that the + // reader overwrites rather than passes through. + it("a recorded job comes back with status 'interrupted', whatever status it was recorded with", async () => { + const subject = await makeSubject() + subject.record({ ...JOB, id: 'job-a', status: 'running' }) + subject.record({ ...JOB, id: 'job-b', status: 'interrupted' }) + await subject.flush() + + const jobs = await subject.listInterrupted() + expect(jobs.find((j) => j.id === 'job-a')?.status).toBe('interrupted') + expect(jobs.find((j) => j.id === 'job-b')?.status).toBe('interrupted') + }) + + it('clear removes a job so listInterrupted no longer contains it', async () => { + const subject = await makeSubject() + subject.record(JOB) + subject.clear(JOB.id) + await subject.flush() + + const jobs = await subject.listInterrupted() + expect(jobs.some((j) => j.id === JOB.id)).toBe(false) + }) + + it('checkpoint updates the stored checkpoint and leaves every other field intact', async () => { + const subject = await makeSubject() + subject.record(JOB) + + const next: GenerationJobCheckpoint = { kind: 'chapters', through: 3 } + subject.checkpoint(JOB.id, next) + await subject.flush() + + const jobs = await subject.listInterrupted() + const found = jobs.find((j) => j.id === JOB.id) + expect(found?.checkpoint).toEqual(next) + expect(found).toMatchObject({ + id: JOB.id, + type: JOB.type, + bookId: JOB.bookId, + bookTitle: JOB.bookTitle, + params: JOB.params, + startedAt: JOB.startedAt, + }) + }) + + it('checkpoint and clear on an unknown id are no-ops and do not throw', async () => { + const subject = await makeSubject() + expect(() => subject.checkpoint('no-such-job', { kind: 'none' })).not.toThrow() + expect(() => subject.clear('no-such-job')).not.toThrow() + await subject.flush() + + expect(await subject.listInterrupted()).toEqual([]) + }) + + it('two different jobs are independent, clearing one leaves the other', async () => { + const subject = await makeSubject() + const other: GenerationJob = { ...JOB, id: 'job-2', bookId: 'book-2' } + subject.record(JOB) + subject.record(other) + + subject.clear(JOB.id) + await subject.flush() + + const jobs = await subject.listInterrupted() + expect(jobs.some((j) => j.id === JOB.id)).toBe(false) + expect(jobs.some((j) => j.id === other.id)).toBe(true) + }) + + it('flush resolves even with nothing queued', async () => { + const subject = await makeSubject() + await expect(subject.flush()).resolves.toBeUndefined() + }) + }) +} diff --git a/server/ports/job-journal.fake.test.ts b/server/ports/job-journal.fake.test.ts new file mode 100644 index 0000000..c5f98b7 --- /dev/null +++ b/server/ports/job-journal.fake.test.ts @@ -0,0 +1,4 @@ +import { describeJobJournalContract } from './job-journal.contract.js' +import { createFakeJobJournal } from './job-journal.fake.js' + +describeJobJournalContract('fake', () => createFakeJobJournal()) diff --git a/server/ports/job-journal.fake.ts b/server/ports/job-journal.fake.ts new file mode 100644 index 0000000..cc8e490 --- /dev/null +++ b/server/ports/job-journal.fake.ts @@ -0,0 +1,36 @@ +import type { GenerationJob } from '@shared/domain.js' +import type { JobJournal } from './job-journal.js' + +/** + * In-memory JobJournal. Holds records in a Map for the lifetime of the + * fake and never touches disk, so a test gets a clean journal regardless + * of what the real filesystem holds. Every write here already happens + * synchronously, so flush() has nothing to wait for and resolves at once. + */ +export function createFakeJobJournal(): JobJournal { + const jobs = new Map() + + return { + record(job) { + jobs.set(job.id, { ...job }) + }, + + checkpoint(jobId, checkpoint) { + const job = jobs.get(jobId) + if (!job) return + jobs.set(jobId, { ...job, checkpoint }) + }, + + clear(jobId) { + jobs.delete(jobId) + }, + + async listInterrupted() { + return Array.from(jobs.values()).map((job) => ({ ...job, status: 'interrupted' })) + }, + + async flush() { + // Nothing is ever queued, every write above already happened inline. + }, + } +} diff --git a/server/ports/job-journal.ts b/server/ports/job-journal.ts new file mode 100644 index 0000000..1e4747c --- /dev/null +++ b/server/ports/job-journal.ts @@ -0,0 +1,44 @@ +import type { GenerationJob, GenerationJobCheckpoint } from '@shared/domain.js' + +/** + * Persists in-flight background jobs so one that was still running when + * the process died can be found and resumed at the next boot. Backs + * server/adapters/journalled-background-tasks.ts, a decorator over + * BackgroundTasks that calls record() when a job starts and clear() when + * it reaches a terminal state, so a record still present at boot can only + * mean the process ended before that terminal state was ever reached. + * + * record, checkpoint, and clear are synchronous so BackgroundTasks.start(), + * report(), succeed(), fail(), and cancel() can all stay synchronous too, + * exactly as the existing BackgroundTasks contract already requires. An + * adapter queues its write internally and the caller never awaits it. + * flush() is the separate seam that lets a test, or a graceful shutdown + * path, wait for every queued write to actually land, it plays no part in + * steady-state operation. + * + * One file per job, rather than a single journal file holding all of them, + * for four reasons: it reuses the atomic writeYaml helper in fs-paths.ts + * exactly the way every other adapter does, it turns a finished job's + * cleanup into a single delete instead of a rewrite of a shared file, it + * confines a corrupted record to the one job that wrote it rather than + * risking the whole journal, and it avoids a read-modify-write race + * between the Electron app and the MCP server when both point at the same + * data directory. + */ +export interface JobJournal { + /** Persists a job at the start of its run, replacing any prior record for the same id. */ + record(job: GenerationJob): void + /** Updates only the checkpoint of an already-recorded job. A no-op for an unknown id. */ + checkpoint(jobId: string, checkpoint: GenerationJobCheckpoint): void + /** Removes a job's record once it reaches a terminal state. A no-op for an unknown id. */ + clear(jobId: string): void + /** + * Every job whose record is still present. Always reports status + * 'interrupted' regardless of what was recorded, since a record that + * survived to be read this way can only mean the process ended before + * clearing it. + */ + listInterrupted(): Promise + /** Resolves once every write queued so far has landed. For tests and shutdown only. */ + flush(): Promise +} diff --git a/server/ports/library-migrator.contract.ts b/server/ports/library-migrator.contract.ts new file mode 100644 index 0000000..950775c --- /dev/null +++ b/server/ports/library-migrator.contract.ts @@ -0,0 +1,48 @@ +import { describe, it, expect } from 'vitest' +import type { LibraryMigrator, MigrationReport } from './library-migrator.js' + +/** + * The behavioural specification every LibraryMigrator must satisfy, + * whether it is the in-memory fake or the real filesystem adapter. Covers + * only what both can satisfy, that a report is well formed and that a + * second call is safe. + * + * Fixture round trips are deliberately not part of this contract. The fake + * has no filesystem to migrate anything on, so an assertion about what + * changed on disk after migrating a specific fixture library can only ever + * run against the real adapter, and lives in that adapter's own test file + * instead, server/adapters/fs-library-migrator.test.ts. + */ + +const PROFILE_OUTCOMES = ['absent', 'current', 'migrated', 'failed'] +const BOOK_OUTCOMES = ['current', 'migrated', 'failed'] + +function assertWellFormed(report: MigrationReport): void { + expect(PROFILE_OUTCOMES).toContain(report.profile.outcome) + expect(Array.isArray(report.books)).toBe(true) + for (const book of report.books) { + expect(typeof book.bookId).toBe('string') + expect(book.bookId.length).toBeGreaterThan(0) + expect(BOOK_OUTCOMES).toContain(book.outcome) + } +} + +export function describeLibraryMigratorContract( + label: string, + makeSubject: () => LibraryMigrator | Promise, +): void { + describe(`LibraryMigrator contract (${label})`, () => { + it('resolves to a well-formed report', async () => { + const subject = await makeSubject() + const report = await subject.migrate() + assertWellFormed(report) + }) + + it('is safe to call twice in a row, and the second call still resolves to a well-formed report', async () => { + const subject = await makeSubject() + await subject.migrate() + const second = await subject.migrate() + assertWellFormed(second) + }) + }) +} diff --git a/server/ports/library-migrator.fake.test.ts b/server/ports/library-migrator.fake.test.ts new file mode 100644 index 0000000..15bb1d9 --- /dev/null +++ b/server/ports/library-migrator.fake.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' +import type { MigrationReport } from './library-migrator.js' +import { createFakeLibraryMigrator } from './library-migrator.fake.js' +import { describeLibraryMigratorContract } from './library-migrator.contract.js' + +describeLibraryMigratorContract('fake', () => createFakeLibraryMigrator()) + +describe('createFakeLibraryMigrator (whitebox)', () => { + it('defaults to reporting an absent profile and no books', async () => { + const fake = createFakeLibraryMigrator() + expect(await fake.migrate()).toEqual({ profile: { outcome: 'absent' }, books: [] }) + }) + + it('reports exactly the report it was constructed with', async () => { + const scripted: MigrationReport = { + profile: { outcome: 'migrated', from: 1, to: 2 }, + books: [{ bookId: 'book-1', outcome: 'migrated', from: 1, to: 2 }], + } + const fake = createFakeLibraryMigrator(scripted) + expect(await fake.migrate()).toEqual(scripted) + }) + + it('counts calls, so a test can assert on boot ordering', async () => { + const fake = createFakeLibraryMigrator() + expect(fake.calls).toBe(0) + await fake.migrate() + expect(fake.calls).toBe(1) + await fake.migrate() + expect(fake.calls).toBe(2) + }) + + it('never lets a caller mutate the scripted report through a returned copy', async () => { + const scripted: MigrationReport = { profile: { outcome: 'absent' }, books: [] } + const fake = createFakeLibraryMigrator(scripted) + + const first = await fake.migrate() + first.books.push({ bookId: 'injected', outcome: 'current' }) + + const second = await fake.migrate() + expect(second.books).toEqual([]) + }) +}) diff --git a/server/ports/library-migrator.fake.ts b/server/ports/library-migrator.fake.ts new file mode 100644 index 0000000..d133d6e --- /dev/null +++ b/server/ports/library-migrator.fake.ts @@ -0,0 +1,32 @@ +import type { LibraryMigrator, MigrationReport } from './library-migrator.js' + +/** + * A LibraryMigrator that does nothing on disk and reports whatever report + * it was constructed with, every time migrate() is called. Its reason for + * existing is letting a test or the e2e harness boot straight past + * migration instead of standing up a real data directory just to satisfy + * the port every server build now depends on. + */ +export interface FakeLibraryMigrator extends LibraryMigrator { + /** How many times migrate() has been called, for tests that care about boot ordering. */ + readonly calls: number +} + +const DEFAULT_REPORT: MigrationReport = { profile: { outcome: 'absent' }, books: [] } + +export function createFakeLibraryMigrator(report: MigrationReport = DEFAULT_REPORT): FakeLibraryMigrator { + let calls = 0 + + return { + get calls() { + return calls + }, + async migrate(): Promise { + calls++ + // structuredClone so a caller mutating the returned report, or a test + // reusing the same scripted report object across subjects, can never + // reach back into this fake's own copy. + return structuredClone(report) + }, + } +} diff --git a/server/ports/library-migrator.ts b/server/ports/library-migrator.ts new file mode 100644 index 0000000..be9e4ba --- /dev/null +++ b/server/ports/library-migrator.ts @@ -0,0 +1,51 @@ +/** + * The report a library-wide migration pass produces, and the port that + * produces it. Run once at startup, before crash recovery, over every + * meta.yml and the learning-profile.yml a data directory holds. + * + * This is a port rather than a service because migration reads below + * BookRepository, at raw YAML that by definition does not validate under + * the current schema, so it cannot be expressed in terms of that port. + * ArtifactStore's recoverFromCrash() is the precedent for a startup + * mutation method living behind a port rather than a plain function. + * + * migrate() never throws for a single bad book. It reports the book as + * failed instead, because one downgraded or corrupt book must not stop a + * whole library from booting. The per-read SchemaTooNewError guard on + * BookRepository still makes any later direct read of that book fail + * loudly, so nothing here weakens that protection, it only keeps one bad + * book from taking every other book down with it. + */ + +/** Why a book or the profile could not be migrated. */ +export type MigrationFailure = 'unreadable' | 'too-new' + +export interface BookMigrationOutcome { + bookId: string + outcome: 'current' | 'migrated' | 'failed' + /** The version this book was found at. Unset when the document could not even be parsed. */ + from?: number + /** The version this book is at now, whether that took a migration or it was already current. Unset when outcome is 'failed'. */ + to?: number + reason?: MigrationFailure + /** A human-readable detail for a 'failed' outcome, such as a caught error's message. */ + detail?: string +} + +export interface ProfileMigrationOutcome { + /** 'absent' means the data directory has no learning-profile.yml at all, the ordinary shape of a fresh install whose profile was never created. */ + outcome: 'absent' | 'current' | 'migrated' | 'failed' + from?: number + to?: number + reason?: MigrationFailure + detail?: string +} + +export interface MigrationReport { + profile: ProfileMigrationOutcome + books: BookMigrationOutcome[] +} + +export interface LibraryMigrator { + migrate(): Promise +} diff --git a/server/ports/text-generation.contract.ts b/server/ports/text-generation.contract.ts index b5b4426..fc3d371 100644 --- a/server/ports/text-generation.contract.ts +++ b/server/ports/text-generation.contract.ts @@ -16,6 +16,24 @@ import type { FakeTextGeneration } from './text-generation.fake.js' * because this port never gets a real adapter, there is no future subject * that would need the narrower type. */ + +/** + * The extra scripting surface the failure taxonomy block below needs, kept + * as its own local type instead of added to FakeTextGeneration itself. + * FakeTextGeneration is not guaranteed to grow these two methods, so this + * contract detects them structurally at runtime, `'scriptFailure' in + * subject`, and this type only describes what that detection unlocks once + * it succeeds. + */ +interface FailureScriptable { + scriptFailure(error: unknown): void + scriptStreamFailure(error: unknown, opts?: { afterChunks?: string[] }): void +} + +function canScriptFailures(subject: FakeTextGeneration): subject is FakeTextGeneration & FailureScriptable { + return 'scriptFailure' in subject && 'scriptStreamFailure' in subject +} + export function describeTextGenerationContract( label: string, makeSubject: () => FakeTextGeneration | Promise, @@ -212,5 +230,51 @@ export function describeTextGenerationContract( expect(textGen.requests.runToolConversation[0].system).toBe('Interview the reader.') }) }) + + describe('failure taxonomy', () => { + const schema = z.object({ questions: z.array(z.string()) }) + + it('propagates a scripted failure out of generateObject with its kind, reason, and retryable intact', async () => { + const textGen = await makeSubject() + if (!canScriptFailures(textGen)) return // this subject carries no failure injection surface, see FailureScriptable above + + const scripted = { name: 'TextGenerationError', message: 'rate limited', kind: 'rate-limited', reason: 'rate limited', retryable: true } + textGen.scriptFailure(scripted) + + await expect(textGen.generateObject({ model, schema, prompt: 'x' })).rejects.toMatchObject({ + kind: 'rate-limited', + reason: 'rate limited', + retryable: true, + }) + }) + + it('rejects on the first iteration when a scripted stream failure has no chunks before it', async () => { + const textGen = await makeSubject() + if (!canScriptFailures(textGen)) return + + const scripted = { name: 'TextGenerationError', message: 'boom', kind: 'unknown', reason: 'boom', retryable: false } + textGen.scriptStreamFailure(scripted, { afterChunks: [] }) + + const iterator = textGen.streamText({ model, prompt: 'x' })[Symbol.asyncIterator]() + await expect(iterator.next()).rejects.toMatchObject({ kind: 'unknown' }) + }) + + it('yields every chunk scripted before a stream failure, then throws, with no chunk duplicated', async () => { + const textGen = await makeSubject() + if (!canScriptFailures(textGen)) return + + const scripted = { name: 'TextGenerationError', message: 'overloaded', kind: 'overloaded', reason: 'overloaded', retryable: true } + textGen.scriptStreamFailure(scripted, { afterChunks: ['a', 'b'] }) + + const received: string[] = [] + await expect((async () => { + for await (const chunk of textGen.streamText({ model, prompt: 'x' })) { + received.push(chunk) + } + })()).rejects.toMatchObject({ kind: 'overloaded' }) + + expect(received).toEqual(['a', 'b']) + }) + }) }) } diff --git a/server/ports/text-generation.fake.ts b/server/ports/text-generation.fake.ts index 05c1645..3bf04e5 100644 --- a/server/ports/text-generation.fake.ts +++ b/server/ports/text-generation.fake.ts @@ -37,6 +37,22 @@ export interface FakeTextGeneration extends TextGeneration { scriptGenerateObject(value: unknown): void /** Queues the steps the next runToolConversation() call works through, in order. A 'tool-call' step invokes that tool's execute() as a side effect and yields nothing; a 'text' step yields one TextChunk. */ scriptToolConversation(steps: ToolConversationStep[]): void + /** + * Optional. A sibling branch declares its own FakeTextGeneration + * implementation as a full object literal, predating failure scripting, + * so a required member here would break that branch's typecheck at + * merge. createFakeTextGeneration implements both this and + * scriptStreamFailure for real. Queues a rejection consumed by the next + * generateObject() call. + */ + scriptFailure?(error: unknown): void + /** + * Optional for the same reason as scriptFailure. Queues a stream that + * yields opts.afterChunks, defaulting to no chunks, and then throws + * error, so a caller can pin exactly how many chunks reach the client + * before a stream fails. + */ + scriptStreamFailure?(error: unknown, opts?: { afterChunks?: string[] }): void } const DEFAULT_STREAM_CHUNKS = ['[fake streamed text]'] @@ -52,6 +68,8 @@ export function createFakeTextGeneration(): FakeTextGeneration { const streamQueue: string[][] = [] const objectQueue: unknown[] = [] const toolConversationQueue: ToolConversationStep[][] = [] + const failureQueue: unknown[] = [] + const streamFailureQueue: Array<{ error: unknown; afterChunks: string[] }> = [] function scriptStreamText(chunks: string[]): void { streamQueue.push(chunks) @@ -65,6 +83,14 @@ export function createFakeTextGeneration(): FakeTextGeneration { toolConversationQueue.push(steps) } + function scriptFailure(error: unknown): void { + failureQueue.push(error) + } + + function scriptStreamFailure(error: unknown, opts?: { afterChunks?: string[] }): void { + streamFailureQueue.push({ error, afterChunks: opts?.afterChunks ?? [] }) + } + async function* yieldChunks(chunks: string[], signal?: AbortSignal): AsyncGenerator { for (const chunk of chunks) { if (signal?.aborted) throw signal.reason @@ -72,8 +98,21 @@ export function createFakeTextGeneration(): FakeTextGeneration { } } + /** Like yieldChunks, but throws error once every chunk has been yielded, so a caller can pin how many chunks reach the client before a stream fails. */ + async function* yieldChunksThenFail(chunks: string[], error: unknown, signal?: AbortSignal): AsyncGenerator { + for (const chunk of chunks) { + if (signal?.aborted) throw signal.reason + yield chunk + } + throw error + } + function streamText(req: StreamTextRequest): AsyncIterable { requests.streamText.push(req) + const scriptedFailure = streamFailureQueue.shift() + if (scriptedFailure) { + return yieldChunksThenFail(scriptedFailure.afterChunks, scriptedFailure.error, req.signal) + } const chunks = streamQueue.shift() ?? DEFAULT_STREAM_CHUNKS return yieldChunks(chunks, req.signal) } @@ -81,6 +120,9 @@ export function createFakeTextGeneration(): FakeTextGeneration { async function generateObject(req: GenerateObjectRequest): Promise { requests.generateObject.push(req as GenerateObjectRequest) if (req.signal?.aborted) throw req.signal.reason + if (failureQueue.length > 0) { + throw failureQueue.shift() + } if (objectQueue.length === 0) { throw new Error( 'createFakeTextGeneration: no scripted generateObject response queued. Call scriptGenerateObject(value) before the code under test invokes generateObject().', @@ -117,6 +159,8 @@ export function createFakeTextGeneration(): FakeTextGeneration { scriptStreamText, scriptGenerateObject, scriptToolConversation, + scriptFailure, + scriptStreamFailure, streamText, generateObject, runToolConversation, diff --git a/server/ports/text-generation.ts b/server/ports/text-generation.ts index 193795f..36ae1d5 100644 --- a/server/ports/text-generation.ts +++ b/server/ports/text-generation.ts @@ -94,6 +94,41 @@ export interface TextChunk { text: string } +/** The failure classes TextGenerationError can carry. Mirrored by `AiErrorKind` in shared/responses.ts, pinned together by the drift guard in ai-error-kind.drift.test.ts. */ +export type TextGenerationErrorKind = + | 'auth-failed' + | 'rate-limited' + | 'overloaded' + | 'timed-out' + | 'network-failed' + | 'content-refused' + | 'unknown' + +/** + * TextGenerationError is a failure from any TextGeneration method. It + * carries enough structure for a caller to decide whether to retry and + * what to show the user. `reason` is human readable and safe to display to + * a user as is. It is never a raw SDK error message. `kind` is a wire + * value, mirrored by `AiErrorKind` in shared/responses.ts, so the client + * can switch on a failure class without importing zod or anything under + * server/. The precedent for a typed error on a port is `NotFoundError` in + * server/ports/book-repository.ts. + */ +export class TextGenerationError extends Error { + readonly retryAfterMs?: number + + constructor( + readonly kind: TextGenerationErrorKind, + readonly reason: string, + readonly retryable: boolean = false, + options: { retryAfterMs?: number; cause?: unknown } = {}, + ) { + super(reason, { cause: options.cause }) + this.name = 'TextGenerationError' + this.retryAfterMs = options.retryAfterMs + } +} + export interface TextGeneration { /** * Streams plain-text output. Covers 5 call sites, the table-of-contents diff --git a/server/routes/audiobook-generation.ts b/server/routes/audiobook-generation.ts index b9b287f..c29c1b1 100644 --- a/server/routes/audiobook-generation.ts +++ b/server/routes/audiobook-generation.ts @@ -19,6 +19,12 @@ export async function audiobookGenerationRoutes(fastify: FastifyInstance, opts: speechSynthesis: ports.speechSynthesis, audioAssembly: ports.audioAssembly, backgroundTasks: ports.backgroundTasks, + // The live route journals its checkpoints too, not just the resume pass + // that rebuilds this service at boot. Without this the checkpoint calls + // inside the narration loop would be unreachable in production, which + // is worse than not having written them, and an interrupted audiobook + // would resume with no idea how far it had got. + journal: ports.jobJournal, }) // POST /api/books/:id/audiobook — start generation diff --git a/server/routes/generation.sse.test.ts b/server/routes/generation.sse.test.ts new file mode 100644 index 0000000..0b3640d --- /dev/null +++ b/server/routes/generation.sse.test.ts @@ -0,0 +1,85 @@ +import { describe, expect, it } from 'vitest' +import type { FastifyInstance } from 'fastify' +import { buildServer } from '../index.js' +import { createFakeTextGeneration } from '../ports/text-generation.fake.js' +import { seedBook } from '../test/route-harness.js' +import type { Ports } from '../composition-root.js' + +/** + * Real-socket tests for the chapter-generation SSE routes, covering issue + * #50, where `POST /api/books/:id/generate-next` answered 200 with an empty + * body and the reader hung in its generating phase forever. + * + * These tests bind a real port and speak HTTP over a real socket instead of + * using `fastify.inject`, and that is the entire point of the file. The bug + * lived in a `close` listener on the REQUEST stream, and light-my-request, + * which backs `inject`, does not model the socket lifecycle that fires it. + * Every inject-based test in this repo passed straight over the defect. So + * a test that could actually observe it has to pay for a real listener. + * + * Keep these few and fast. Everything that does not specifically depend on + * socket lifecycle belongs in the far cheaper inject-based suites. + */ + +/** Builds a server, binds an ephemeral port, and returns it with its base URL. */ +async function listening(overrides: Partial): Promise<{ app: FastifyInstance; base: string }> { + const app = await buildServer(overrides) + await app.listen({ port: 0, host: '127.0.0.1' }) + const address = app.server.address() + const port = typeof address === 'object' && address !== null ? address.port : 0 + return { app, base: `http://127.0.0.1:${port}` } +} + +function postJson(body: unknown): RequestInit { + return { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(body) } +} + +describe('chapter generation SSE over a real socket', () => { + it('streams chapter events from generate-next rather than closing before the first byte', async () => { + const meta = await seedBook({ totalChapters: 3, generatedUpTo: 1 }) + const textGeneration = createFakeTextGeneration() + textGeneration.scriptStreamText(['alpha ', 'beta']) + // Quiz generation is a non-fatal side effect of generating a chapter. + textGeneration.scriptGenerateObject({ questions: [] }) + + const { app, base } = await listening({ textGeneration }) + try { + const res = await fetch(`${base}/api/books/${meta.id}/generate-next`, postJson({ model: 'claude-sonnet-4-6' })) + expect(res.status).toBe(200) + + const body = await res.text() + // The regression this pins: the body was empty. Assert on real + // content rather than merely on length, so a future change that + // writes a byte but drops the events still fails here. + expect(body).not.toBe('') + expect(body).toContain('alpha') + expect(body).toContain('beta') + } finally { + await app.close() + } + }) + + it('streams chapter events from regenerate too, which only ever worked by accident', async () => { + // Before the fix this route passed while generate-next failed, and the + // difference was not intentional. Its handler is `async` and awaits + // getBook() before reaching the SSE helper, so the request stream's + // premature `close` fired during that await, before the listener was + // attached, and was therefore missed. generate-next's handler is + // synchronous, attaches in the same tick, and caught it. Removing an + // await here would have silently broken this route. This test exists so + // that the route's correctness stops depending on that timing. + const meta = await seedBook({ totalChapters: 3, generatedUpTo: 2 }) + const textGeneration = createFakeTextGeneration() + textGeneration.scriptStreamText(['redone']) + textGeneration.scriptGenerateObject({ questions: [] }) + + const { app, base } = await listening({ textGeneration }) + try { + const res = await fetch(`${base}/api/books/${meta.id}/chapters/1/regenerate`, postJson({ model: 'claude-sonnet-4-6' })) + expect(res.status).toBe(200) + expect(await res.text()).toContain('redone') + } finally { + await app.close() + } + }) +}) diff --git a/server/routes/generation.ts b/server/routes/generation.ts index 9a525ca..df3da16 100644 --- a/server/routes/generation.ts +++ b/server/routes/generation.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto' -import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify' +import type { FastifyInstance, FastifyReply } from 'fastify' import { CreateBookBodySchema, GenerateNextBodySchema, @@ -41,7 +41,7 @@ function tocReviewConflict(book: BookMeta, action: string) { } /** Opens the SSE response and forwards the hub's events for one book until a terminal event, or the client disconnects. */ -function pipeHubToSse(request: FastifyRequest, reply: FastifyReply, hub: ChapterGenerationStream, bookId: string, sendBuffered: boolean): void { +function pipeHubToSse(reply: FastifyReply, hub: ChapterGenerationStream, bookId: string, sendBuffered: boolean): void { reply.raw.writeHead(200, SSE_HEADERS) let ended = false @@ -54,7 +54,20 @@ function pipeHubToSse(request: FastifyRequest, reply: FastifyReply, hub: Chapter } }, sendBuffered) - request.raw.on('close', () => { + // Listen on the RESPONSE, never on the request. See issue #50. From Node + // 16 onward an IncomingMessage emits `close` once its own stream is + // finished, not only when the peer disconnects, and for a POST whose body + // Fastify has already read and parsed that is immediately. Listening + // there unsubscribed and ended the reply before generation produced its + // first chunk, so the client received a 200 with an empty body while the + // chapter generated and saved perfectly well behind it. + // + // A ServerResponse emits `close` when the response finishes or the + // connection is torn down early, which is the question actually being + // asked here. On a normal finish `ended` is already true and unsubscribing + // is simply the cleanup that was always wanted; on a real disconnect the + // subscriber is dropped and the reply closed, exactly as before. + reply.raw.on('close', () => { unsubscribe() if (!ended) { ended = true; reply.raw.end() } }) @@ -67,7 +80,13 @@ export async function generationRoutes(fastify: FastifyInstance, { ports, servic const createBook = createCreateBook({ ai: ports.textGeneration, books: ports.bookRepository, clock: ports.clock }) const reviseToc = createReviseToc({ ai: ports.textGeneration, books: ports.bookRepository, clock: ports.clock }) const startBook = createStartBook({ ai: ports.textGeneration, books: ports.bookRepository, clock: ports.clock }) - const generateAllChapters = createGenerateAllChapters({ backgroundTasks: ports.backgroundTasks, chapterStream, generateNextChapter }) + // journal is passed here, not only in the resume pass that rebuilds this + // service at boot. Without it the checkpoint calls inside the generate-all + // loop would be unreachable in production, which is worse than never + // having written them, and an interrupted run would resume with no record + // of how far it had got. The checkpoint is still advisory, resume always + // recomputes its start point from meta.generatedUpTo on disk. + const generateAllChapters = createGenerateAllChapters({ backgroundTasks: ports.backgroundTasks, chapterStream, generateNextChapter, journal: ports.jobJournal }) // --- Single-chapter generation (next / regenerate), backed by the shared hub --- @@ -80,7 +99,7 @@ export async function generationRoutes(fastify: FastifyInstance, { ports, servic const conflict = singleChapterConflict(chapterStream, ports.backgroundTasks, bookId) if (conflict) return reply.status(409).send(conflict) chapterStream.startGeneration(bookId, body) - pipeHubToSse(request, reply, chapterStream, bookId, false) + pipeHubToSse(reply, chapterStream, bookId, false) }, ) @@ -101,7 +120,7 @@ export async function generationRoutes(fastify: FastifyInstance, { ports, servic const conflict = singleChapterConflict(chapterStream, ports.backgroundTasks, bookId) if (conflict) return reply.status(409).send(conflict) chapterStream.startGeneration(bookId, { ...body, targetChapterNum: chapterNum }) - pipeHubToSse(request, reply, chapterStream, bookId, false) + pipeHubToSse(reply, chapterStream, bookId, false) }, ) @@ -117,7 +136,7 @@ export async function generationRoutes(fastify: FastifyInstance, { ports, servic return reply.status(404).send({ error: 'No active generation for this book' }) } - pipeHubToSse(request, reply, chapterStream, bookId, true) + pipeHubToSse(reply, chapterStream, bookId, true) }) // --- Create book (TOC generation) --- diff --git a/server/routes/tasks.ts b/server/routes/tasks.ts index 445bb65..5ebd8fc 100644 --- a/server/routes/tasks.ts +++ b/server/routes/tasks.ts @@ -28,7 +28,15 @@ export async function taskRoutes(fastify: FastifyInstance, opts: { ports: Ports; reply.raw.write(`data: ${JSON.stringify(event)}\n\n`) }) - request.raw.on('close', () => { + // Listen on the RESPONSE, never on the request, for the reason issue + // #50 documents at length in server/routes/generation.ts. This route + // is a GET whose body Fastify never reads, so its request stream is + // not finished early and the old listener happened not to misfire, + // which is why the task tray worked while chapter generation did not. + // That is a coincidence of the verb, not a property worth relying on, + // and this is the stream resumed jobs are reported through, so it gets + // the same correct listener rather than the same latent trap. + reply.raw.on('close', () => { unsubscribe() if (!ended) { ended = true; reply.raw.end() } }) diff --git a/server/services/chapter-generation-stream.test.ts b/server/services/chapter-generation-stream.test.ts index 94ce9ae..f882da7 100644 --- a/server/services/chapter-generation-stream.test.ts +++ b/server/services/chapter-generation-stream.test.ts @@ -2,6 +2,9 @@ import { describe, expect, it, vi } from 'vitest' import type { BookMeta } from '@shared/domain.js' import type { GenerateChapterEvent } from '@shared/events.js' import { createFakeBookRepository } from '../ports/book-repository.fake.js' +import { createFakeJobJournal } from '../ports/job-journal.fake.js' +import { createFakeClock } from '../ports/clock.fake.js' +import { GENERATION_STREAM_CLEANUP_MS } from '../constants.js' import { createChapterGenerationStream } from './chapter-generation-stream.js' import type { GenerateNextChapterOptions } from './generate-next-chapter.js' @@ -242,4 +245,108 @@ describe('createChapterGenerationStream', () => { { type: 'done', chapterNum: 2 }, ]) }) + + describe('seedInterrupted', () => { + it('makes getStatus report an active, errored generation carrying the given chapter number and message', () => { + const books = createFakeBookRepository() + const stream = createChapterGenerationStream({ books, generateNextChapter: makeStubGenerateNextChapter().fn }) + + stream.seedInterrupted(BOOK.id, 3, 'Interrupted by restart') + + expect(stream.getStatus(BOOK.id)).toEqual({ + active: true, + chapterNum: 3, + stage: 'error', + contentLength: 0, + error: 'Interrupted by restart', + }) + }) + + it('leaves isGenerating false for a seeded state, same as any other errored generation', () => { + const books = createFakeBookRepository() + const stream = createChapterGenerationStream({ books, generateNextChapter: makeStubGenerateNextChapter().fn }) + + stream.seedInterrupted(BOOK.id, 3, 'Interrupted by restart') + + expect(stream.isGenerating(BOOK.id)).toBe(false) + }) + + // The regression that matters: a state seeded at boot has no subscriber + // watching it yet, so unlike every other terminal state it must NOT be + // evicted on a timer. A user who does not open this book's reader for + // hours must still find the error waiting when they do. + it('survives well past GENERATION_STREAM_CLEANUP_MS, unlike a generation that reached a terminal state live', async () => { + vi.useFakeTimers() + try { + const books = createFakeBookRepository() + const stream = createChapterGenerationStream({ books, generateNextChapter: makeStubGenerateNextChapter().fn }) + + stream.seedInterrupted(BOOK.id, 3, 'Interrupted by restart') + await vi.advanceTimersByTimeAsync(GENERATION_STREAM_CLEANUP_MS * 10) + + expect(stream.getStatus(BOOK.id)).toMatchObject({ active: true, stage: 'error' }) + } finally { + vi.useRealTimers() + } + }) + }) + + describe('job journalling', () => { + it('records a generate-chapter job while generation is in flight, and clears it once the generation settles', async () => { + const books = createFakeBookRepository() + await books.saveBook(BOOK) + const stub = makeStubGenerateNextChapter() + const journal = createFakeJobJournal() + const clock = createFakeClock() + const stream = createChapterGenerationStream({ books, generateNextChapter: stub.fn, journal, clock }) + + stream.startGeneration(BOOK.id, { model: 'claude-sonnet-4-6', targetChapterNum: 2 }) + await vi.waitFor(() => expect(stub.fn).toHaveBeenCalled()) + + const inFlight = await journal.listInterrupted() + expect(inFlight).toHaveLength(1) + expect(inFlight[0]).toMatchObject({ + type: 'generate-chapter', + bookId: BOOK.id, + bookTitle: BOOK.title, + checkpoint: { kind: 'none' }, + params: { model: 'claude-sonnet-4-6', targetChapterNum: 2 }, + }) + + stub.resolve('the chapter text') + await vi.waitFor(async () => { + expect(await journal.listInterrupted()).toEqual([]) + }) + }) + + it('clears the journal record on an error settlement too, not only on success', async () => { + const books = createFakeBookRepository() + await books.saveBook(BOOK) + const stub = makeStubGenerateNextChapter() + const journal = createFakeJobJournal() + const clock = createFakeClock() + const stream = createChapterGenerationStream({ books, generateNextChapter: stub.fn, journal, clock }) + + stream.startGeneration(BOOK.id, { model: 'claude-sonnet-4-6' }) + await vi.waitFor(() => expect(stub.fn).toHaveBeenCalled()) + expect(await journal.listInterrupted()).toHaveLength(1) + + stub.reject(new Error('model exploded')) + await vi.waitFor(async () => { + expect(await journal.listInterrupted()).toEqual([]) + }) + }) + + it('never writes to the journal when none was supplied, the same as every pre-existing test in this file', async () => { + const books = createFakeBookRepository() + await books.saveBook(BOOK) + const stub = makeStubGenerateNextChapter() + const stream = createChapterGenerationStream({ books, generateNextChapter: stub.fn }) + + expect(() => stream.startGeneration(BOOK.id, { model: 'claude-sonnet-4-6' })).not.toThrow() + await vi.waitFor(() => expect(stub.fn).toHaveBeenCalled()) + stub.resolve('the chapter text') + await vi.waitFor(() => expect(stream.isGenerating(BOOK.id)).toBe(false)) + }) + }) }) diff --git a/server/services/chapter-generation-stream.ts b/server/services/chapter-generation-stream.ts index e02aa0c..8478fa2 100644 --- a/server/services/chapter-generation-stream.ts +++ b/server/services/chapter-generation-stream.ts @@ -1,6 +1,11 @@ -import type { GenerationStage, GenerationStatus } from '@shared/responses.js' +import type { AiErrorKind, GenerationStage, GenerationStatus } from '@shared/responses.js' import type { GenerateChapterEvent } from '@shared/events.js' +import type { GenerationJobParams } from '@shared/domain.js' +import type { ProviderId } from '@shared/provider.js' import type { BookRepository } from '../ports/book-repository.js' +import type { JobJournal } from '../ports/job-journal.js' +import type { Clock } from '../ports/clock.js' +import { TextGenerationError } from '../ports/text-generation.js' import { GENERATION_STREAM_CLEANUP_MS } from '../constants.js' import type { GenerateNextChapter, GenerateNextChapterOptions } from './generate-next-chapter.js' @@ -31,6 +36,7 @@ interface GenerationState { cleanupTimer?: ReturnType doneData?: { chapterNum: number } error?: string + errorKind?: AiErrorKind } export interface GenerationOptions extends GenerateNextChapterOptions { @@ -44,11 +50,28 @@ export interface ChapterGenerationStream { /** Returns an unsubscribe function. sendBuffered replays already-streamed content as one buffered chapter event on join. */ subscribe(bookId: string, callback: Subscriber, sendBuffered: boolean): () => void startGeneration(bookId: string, options: GenerationOptions): void + /** + * Seeds a terminal error state for a book at boot, for a generate-chapter + * job that was still running when the process died. See the + * implementation below for why this deliberately skips scheduleCleanup. + */ + seedInterrupted(bookId: string, chapterNum: number, message: string): void } export function createChapterGenerationStream(deps: { books: BookRepository generateNextChapter: GenerateNextChapter + /** + * Both optional, and independently so, purely so the many existing tests + * that construct this hub with only {books, generateNextChapter} keep + * compiling unchanged. Journalling is additive observability the hub's + * own generation flow does not need to function: a caller (or test) that + * omits either simply gets a hub that never writes a generate-chapter + * record to the job journal, and therefore never has one for + * resume-interrupted-jobs.ts to find at the next boot. + */ + journal?: JobJournal + clock?: Clock }): ChapterGenerationStream { const generationStates = new Map() @@ -66,11 +89,47 @@ export function createChapterGenerationStream(deps: { } async function runGeneration(bookId: string, state: GenerationState, options: GenerationOptions): Promise { + // Journalled under a fresh id whenever a journal and clock were both + // supplied, so a generation still in flight when the process dies can + // be found by resume-interrupted-jobs.ts at the next boot. There is no + // way to resume a partially streamed chapter with any provider, only to + // surface that it was interrupted, which is exactly what that resume + // pass does via seedInterrupted below. bookTitle needs the book, which + // is not known synchronously in startGeneration, so the record is + // written here, right after the getBook call this function already has + // to make, rather than before runGeneration is even invoked. The record + // and its clearing both live in this one function, in a try/finally, so + // every exit path below (the "already generated" guard, a clean finish, + // or a caught failure) leaves nothing behind once this promise settles. + const jobId = deps.journal && deps.clock ? deps.clock.newId() : undefined + try { const meta = await deps.books.getBook(bookId) const nextNum = options.targetChapterNum ?? meta.generatedUpTo + 1 state.chapterNum = nextNum + if (jobId && deps.journal && deps.clock) { + const now = deps.clock.nowIso() + // Only these three. Never an API key, this journal is written to + // disk unencrypted and a key belongs in KeyVault alone. + const params: GenerationJobParams = { + ...(options.targetChapterNum !== undefined ? { targetChapterNum: options.targetChapterNum } : {}), + ...(options.provider !== undefined ? { provider: options.provider as ProviderId } : {}), + ...(options.model !== undefined ? { model: options.model } : {}), + } + deps.journal.record({ + id: jobId, + type: 'generate-chapter', + bookId, + bookTitle: meta.title, + status: 'running', + checkpoint: { kind: 'none' }, + params, + startedAt: now, + updatedAt: now, + }) + } + if (nextNum > meta.totalChapters) { state.stage = 'error' state.error = 'All chapters already generated' @@ -91,8 +150,17 @@ export function createChapterGenerationStream(deps: { } catch (error) { state.stage = 'error' state.error = error instanceof Error ? error.message : 'Generation failed' - emit(state, { type: 'error', message: state.error }) + // Carry the failure class through to the client when the provider + // gave us one. The reader uses it to tell an unusable API key apart + // from a transient overload, so it can open the missing-key dialog + // instead of a toast the user cannot act on. Anything that is not a + // TextGenerationError, such as a parse failure this app raised + // itself, has no class and simply omits the field. + state.errorKind = error instanceof TextGenerationError ? error.kind : undefined + emit(state, { type: 'error', message: state.error, ...(state.errorKind ? { kind: state.errorKind } : {}) }) scheduleCleanup(bookId, state) + } finally { + if (jobId && deps.journal) deps.journal.clear(jobId) } } @@ -105,7 +173,14 @@ export function createChapterGenerationStream(deps: { getStatus(bookId) { const state = generationStates.get(bookId) if (!state) return { active: false } - return { active: true, chapterNum: state.chapterNum, stage: state.stage, contentLength: state.content.length } + return { + active: true, + chapterNum: state.chapterNum, + stage: state.stage, + contentLength: state.content.length, + ...(state.error !== undefined ? { error: state.error } : {}), + ...(state.errorKind !== undefined ? { errorKind: state.errorKind } : {}), + } }, subscribe(bookId, callback, sendBuffered) { @@ -130,7 +205,7 @@ export function createChapterGenerationStream(deps: { return () => {} } if (state.stage === 'error' && state.error) { - callback({ type: 'error', message: state.error }) + callback({ type: 'error', message: state.error, ...(state.errorKind ? { kind: state.errorKind } : {}) }) scheduleCleanup(bookId, state) return () => {} } @@ -158,5 +233,30 @@ export function createChapterGenerationStream(deps: { generationStates.set(bookId, state) state.promise = runGeneration(bookId, state, options) }, + + seedInterrupted(bookId, chapterNum, message) { + // Deliberately does NOT call scheduleCleanup. Every other terminal + // state above is scheduled for eviction after + // GENERATION_STREAM_CLEANUP_MS (five minutes) because a live client is + // already watching it by the time that state is reached, so a short + // grace period after the last subscriber leaves is plenty. A state + // seeded here at boot has no watcher yet, and the user may not open + // this book's reader for hours, so an eviction timer would silently + // discard the only record that this chapter's generation failed, + // often before anyone ever saw it. Instead it is cleaned up the + // ordinary way: the moment a subscriber does join, subscribe()'s + // existing terminal-state branch above delivers the error event and + // schedules cleanup itself, exactly as it does for a generation that + // failed live. + const state: GenerationState = { + content: '', + stage: 'error', + chapterNum, + subscribers: new Set(), + promise: Promise.resolve(), + error: message, + } + generationStates.set(bookId, state) + }, } } diff --git a/server/services/generate-all-chapters.test.ts b/server/services/generate-all-chapters.test.ts index 9498190..306288f 100644 --- a/server/services/generate-all-chapters.test.ts +++ b/server/services/generate-all-chapters.test.ts @@ -14,6 +14,7 @@ const idleChapterStream: ChapterGenerationStream = { getStatus: () => ({ active: false }), subscribe: () => () => {}, startGeneration: () => {}, + seedInterrupted: () => {}, } /** A stub GenerateNextChapter whose per-call behaviour is scripted per chapter number. */ diff --git a/server/services/generate-all-chapters.ts b/server/services/generate-all-chapters.ts index 9d64adb..8c69a1c 100644 --- a/server/services/generate-all-chapters.ts +++ b/server/services/generate-all-chapters.ts @@ -1,4 +1,7 @@ +import type { GenerationJobParams } from '@shared/domain.js' +import type { ProviderId } from '@shared/provider.js' import type { BackgroundTasks } from '../ports/background-tasks.js' +import type { JobJournal } from '../ports/job-journal.js' import type { ChapterGenerationStream } from './chapter-generation-stream.js' import type { GenerateNextChapter, GenerateNextChapterOptions } from './generate-next-chapter.js' @@ -17,6 +20,16 @@ export function createGenerateAllChapters(deps: { backgroundTasks: BackgroundTasks chapterStream: ChapterGenerationStream generateNextChapter: GenerateNextChapter + /** + * Optional so every existing caller and test that builds this without a + * journal keeps compiling unchanged. When present, each completed + * chapter's number is checkpointed against the tray task's id, purely as + * a progress label a UI could show if this job survives to the next + * boot. resume-interrupted-jobs.ts never reads this checkpoint to decide + * what to redo, it always recomputes the real start point from + * meta.generatedUpTo instead, see that module's own doc for why. + */ + journal?: JobJournal }) { return function generateAllChapters( bookId: string, @@ -25,7 +38,17 @@ export function createGenerateAllChapters(deps: { ): { taskId: string } { const startFrom = meta.generatedUpTo + 1 const total = meta.totalChapters - const task = deps.backgroundTasks.start({ type: 'generate-all', bookId, bookTitle: meta.title, total }) + // Carried through to the journal so an interrupted run can be resumed + // with the same request parameters. See GenerationJobParamsSchema for + // why an API key can never be among these. + const params: GenerationJobParams = { + ...(options.model !== undefined ? { model: options.model } : {}), + ...(options.provider !== undefined ? { provider: options.provider as ProviderId } : {}), + ...(options.quizModel !== undefined ? { quizModel: options.quizModel } : {}), + ...(options.quizProvider !== undefined ? { quizProvider: options.quizProvider as ProviderId } : {}), + ...(options.quizLength !== undefined ? { quizLength: options.quizLength } : {}), + } + const task = deps.backgroundTasks.start({ type: 'generate-all', bookId, bookTitle: meta.title, total, params }) // Fire-and-forget ;(async () => { @@ -43,6 +66,7 @@ export function createGenerateAllChapters(deps: { deps.backgroundTasks.report(task.id, num, `Generating chapter ${num} of ${total}`) await deps.generateNextChapter(bookId, num, options, undefined, task.signal) + deps.journal?.checkpoint(task.id, { kind: 'chapters', through: num }) } deps.backgroundTasks.succeed(task.id) } catch (err) { diff --git a/server/services/generate-audiobook.ts b/server/services/generate-audiobook.ts index a949add..fd46bba 100644 --- a/server/services/generate-audiobook.ts +++ b/server/services/generate-audiobook.ts @@ -6,6 +6,7 @@ import type { ArtifactStore } from '../ports/artifact-store.js' import type { SpeechSynthesis } from '../ports/speech-synthesis.js' import type { AudioAssembly } from '../ports/audio-assembly.js' import type { BackgroundTasks } from '../ports/background-tasks.js' +import type { JobJournal } from '../ports/job-journal.js' import type { AudiobookManifest, AudiobookChapterEntry, BookMeta, LearningProfile } from '@shared/domain.js' import { M4B_BITRATE } from '../constants.js' @@ -38,6 +39,16 @@ export interface GenerateAudiobookDeps { speechSynthesis: Pick audioAssembly: AudioAssembly backgroundTasks: BackgroundTasks + /** + * Optional so every existing caller and test that builds this without a + * journal keeps compiling unchanged. When present, narration progress is + * checkpointed purely as a progress label a UI could show if this job + * survives to the next boot. resume-interrupted-jobs.ts never reads it + * to decide what to redo, it always restarts narration from the + * beginning, because crash recovery has already deleted any partial + * audio by the time resume runs. See that module's own doc for why. + */ + journal?: JobJournal } export interface GenerateAudiobookRequest { @@ -79,7 +90,7 @@ async function tryRm(path: string): Promise { } export function createGenerateAudiobook(deps: GenerateAudiobookDeps) { - const { bookRepository, artifactStore, speechSynthesis, audioAssembly, backgroundTasks } = deps + const { bookRepository, artifactStore, speechSynthesis, audioAssembly, backgroundTasks, journal } = deps async function narrate(meta: BookMeta, voiceId: string, speed: number, taskId: string, signal: AbortSignal): Promise { const bookId = meta.id @@ -155,8 +166,16 @@ export function createGenerateAudiobook(deps: GenerateAudiobookDeps) { await bookRepository.saveBook(meta2) backgroundTasks.report(taskId, n, `Narrated chapter ${n} of ${totalChapters}`) + // Informational only, see the journal dep's own doc above: never + // read back to decide what to redo, only ever shown as a progress + // label if this job survives to the next boot. + journal?.checkpoint(taskId, { kind: 'chapters', through: n }) } + // Every chapter is narrated. Same informational-only checkpoint as + // above, ahead of the stitch phase below. + journal?.checkpoint(taskId, { kind: 'narration-complete' }) + if (signal.aborted) throw new Error('Audiobook generation aborted') // 6. Stitch phase: probe durations, then hand concatenation, chapter @@ -282,11 +301,15 @@ export function createGenerateAudiobook(deps: GenerateAudiobookDeps) { } // total=N chapters; narrate() reports progress per chapter narrated. + // params carries the resolved voice and speed through to the journal, + // so an interrupted run resumes with the same request rather than + // falling back to profile defaults a second time. const handle = backgroundTasks.start({ type: 'generate-audiobook', bookId, bookTitle: meta.title, total: meta.totalChapters, + params: { voiceId, speed }, }) ;(async () => { diff --git a/server/services/resume-interrupted-jobs.test.ts b/server/services/resume-interrupted-jobs.test.ts new file mode 100644 index 0000000..73efddc --- /dev/null +++ b/server/services/resume-interrupted-jobs.test.ts @@ -0,0 +1,339 @@ +import { describe, expect, it, vi } from 'vitest' +import type { BookMeta, GenerationJob, GenerationJobType } from '@shared/domain.js' +import type { JobJournal } from '../ports/job-journal.js' +import type { BackgroundTasks } from '../ports/background-tasks.js' +import type { ChapterGenerationStream } from './chapter-generation-stream.js' +import type { GenerateNextChapterOptions } from './generate-next-chapter.js' +import { createFakeJobJournal } from '../ports/job-journal.fake.js' +import { createFakeBookRepository } from '../ports/book-repository.fake.js' +import { createFakeBackgroundTasks } from '../ports/background-tasks.fake.js' +import { createResumeInterruptedJobs } from './resume-interrupted-jobs.js' + +/** + * A book with 4 of 6 chapters generated, the fixture every "resume acts on + * disk" test needs: generatedUpTo is the fact a stale or wrong checkpoint + * must never override. + */ +const BOOK_META: BookMeta = { + id: 'book-1', + title: 'Distributed Systems', + prompt: 'Learn distributed systems', + status: 'reading', + totalChapters: 6, + generatedUpTo: 4, + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-01T00:00:00.000Z', + tags: [], + audioGeneratedChapters: [], +} + +let jobCounter = 0 +/** A journalled job with sane defaults, so each test only states what it's actually testing. */ +function makeJob(overrides: Partial & { type: GenerationJobType; bookId: string }): GenerationJob { + jobCounter += 1 + return { + id: `job-${jobCounter}`, + bookTitle: BOOK_META.title, + status: 'running', + checkpoint: { kind: 'none' }, + params: {}, + startedAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-01T00:00:00.000Z', + ...overrides, + } +} + +type GenerateAllChaptersFn = ( + bookId: string, + meta: { title: string; generatedUpTo: number; totalChapters: number }, + options: GenerateNextChapterOptions, +) => { taskId: string } + +type GenerateAudiobookFn = (req: { + bookId: string + voiceId?: string + speed?: number + confirmReplace?: boolean +}) => Promise<{ outcome: string; taskId?: string }> + +/** + * A fully-typed vi.fn() stub for generateAllChapters. Explicitly annotating + * impl before handing it to vi.fn() (rather than passing an inline arrow + * straight in) is load bearing, not stylistic: vi.fn() infers its mock's + * call signature from whatever it is given, and an untyped zero-arg arrow + * infers a signature too loose to satisfy createResumeInterruptedJobs's + * deps type, which tsc (not vitest itself) then rejects. + */ +function stubGenerateAllChapters(result: { taskId: string } = { taskId: 'task-x' }) { + const impl: GenerateAllChaptersFn = () => result + return vi.fn(impl) +} + +/** Same reasoning as stubGenerateAllChapters above, for generateAudiobook. */ +function stubGenerateAudiobook(result: { outcome: string; taskId?: string } = { outcome: 'not-complete' }) { + const impl: GenerateAudiobookFn = async () => result + return vi.fn(impl) +} + +type LogFn = (msg: string, ctx?: Record) => void + +/** Same reasoning as stubGenerateAllChapters above, for the log dependency. */ +function stubLog() { + const impl: LogFn = () => {} + return vi.fn(impl) +} + +function makeDeps(overrides: { + journal: JobJournal + books: ReturnType + backgroundTasks?: BackgroundTasks + chapterStream?: Pick + generateAllChapters?: ReturnType + generateAudiobook?: ReturnType + autoResume?: boolean + log?: ReturnType +}) { + return { + journal: overrides.journal, + books: overrides.books, + backgroundTasks: overrides.backgroundTasks ?? createFakeBackgroundTasks(), + chapterStream: overrides.chapterStream ?? { seedInterrupted: vi.fn() }, + generateAllChapters: overrides.generateAllChapters ?? stubGenerateAllChapters(), + generateAudiobook: overrides.generateAudiobook ?? stubGenerateAudiobook(), + autoResume: overrides.autoResume ?? true, + log: overrides.log ?? stubLog(), + } +} + +describe('createResumeInterruptedJobs', () => { + describe('generate-all', () => { + it('resumes from a fresh read of meta.generatedUpTo, never from the journalled checkpoint — the gate on the whole resume design', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) // generatedUpTo: 4, on disk, right now + + // The checkpoint lies: it claims the job only got through chapter 1. + // If resume trusted it, chapters 2-4 (already on disk) would be + // regenerated. Disk is the truth, the checkpoint is advisory only. + journal.record(makeJob({ + id: 'job-resume-all', type: 'generate-all', bookId: BOOK_META.id, + checkpoint: { kind: 'chapters', through: 1 }, + params: { model: 'claude-sonnet-4-6' }, + })) + + const generateAllChapters = stubGenerateAllChapters({ taskId: 'task-all' }) + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, generateAllChapters })) + + const report = await resume() + + expect(generateAllChapters).toHaveBeenCalledTimes(1) + const [bookIdArg, metaArg, optionsArg] = generateAllChapters.mock.calls[0] + expect(bookIdArg).toBe(BOOK_META.id) + // The load-bearing assertion: generatedUpTo came from disk (4), not + // from checkpoint.through (1). generateAllChapters itself computes + // startFrom = meta.generatedUpTo + 1 = 5, so nothing at or below + // chapter 4 is ever regenerated. That computation is proven + // separately in generate-all-chapters.test.ts; what resume alone + // must prove is that it handed over the real number. + expect(metaArg).toMatchObject({ generatedUpTo: 4, totalChapters: 6, title: BOOK_META.title }) + expect(optionsArg).toMatchObject({ model: 'claude-sonnet-4-6' }) + expect(report.resumed).toEqual([{ jobId: 'job-resume-all', type: 'generate-all' }]) + }) + + it('skips a generate-all job whose book is already fully generated, and clears its record', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook({ ...BOOK_META, generatedUpTo: 6, totalChapters: 6 }) + journal.record(makeJob({ id: 'job-complete', type: 'generate-all', bookId: BOOK_META.id })) + + const generateAllChapters = stubGenerateAllChapters() + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, generateAllChapters })) + + const report = await resume() + + expect(generateAllChapters).not.toHaveBeenCalled() + expect(report.skipped).toEqual([{ jobId: 'job-complete', type: 'generate-all', reason: expect.any(String) }]) + expect(await journal.listInterrupted()).toEqual([]) + }) + }) + + describe('generate-audiobook', () => { + it('resumes by calling generateAudiobook with confirmReplace true and the journalled voice and speed', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook({ ...BOOK_META, generatedUpTo: 6, totalChapters: 6 }) + journal.record(makeJob({ + id: 'job-audio', type: 'generate-audiobook', bookId: BOOK_META.id, + params: { voiceId: 'onyx', speed: 1.2 }, + })) + + const generateAudiobook = stubGenerateAudiobook({ outcome: 'started', taskId: 'task-audio' }) + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, generateAudiobook })) + + const report = await resume() + + expect(generateAudiobook).toHaveBeenCalledWith({ + bookId: BOOK_META.id, voiceId: 'onyx', speed: 1.2, confirmReplace: true, + }) + expect(report.resumed).toEqual([{ jobId: 'job-audio', type: 'generate-audiobook' }]) + }) + + it('skips when the resumed call does not actually start, using its outcome as the skip reason', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook({ ...BOOK_META, generatedUpTo: 6, totalChapters: 6 }) + journal.record(makeJob({ id: 'job-audio-skip', type: 'generate-audiobook', bookId: BOOK_META.id })) + + const generateAudiobook = stubGenerateAudiobook({ outcome: 'engine-not-installed' }) + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, generateAudiobook })) + + const report = await resume() + + expect(report.skipped).toEqual([{ jobId: 'job-audio-skip', type: 'generate-audiobook', reason: 'engine-not-installed' }]) + expect(await journal.listInterrupted()).toEqual([]) + }) + }) + + describe.each(['generate-epub', 'generate-cover', 'install-audiobook'])('%s', (type) => { + it('is marked errored and retriable in the tray, and never reaches generateAllChapters or generateAudiobook', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) + journal.record(makeJob({ id: 'job-short', type, bookId: BOOK_META.id })) + + const backgroundTasks = createFakeBackgroundTasks() + const generateAllChapters = stubGenerateAllChapters() + const generateAudiobook = stubGenerateAudiobook() + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, backgroundTasks, generateAllChapters, generateAudiobook })) + + const report = await resume() + + const tasksOfType = backgroundTasks.list().filter((t) => t.type === type) + expect(tasksOfType).toHaveLength(1) + expect(tasksOfType[0]).toMatchObject({ + bookId: BOOK_META.id, + status: 'error', + error: 'Interrupted by restart', + }) + expect(report.markedErrored).toEqual([{ jobId: 'job-short', type }]) + expect(generateAllChapters).not.toHaveBeenCalled() + expect(generateAudiobook).not.toHaveBeenCalled() + }) + }) + + describe('generate-chapter', () => { + it('seeds the hub with the journalled targetChapterNum when one was set (a regeneration)', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) // generatedUpTo: 4 + journal.record(makeJob({ + id: 'job-regen', type: 'generate-chapter', bookId: BOOK_META.id, params: { targetChapterNum: 2 }, + })) + + const chapterStream = { seedInterrupted: vi.fn() } + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, chapterStream })) + + const report = await resume() + + expect(chapterStream.seedInterrupted).toHaveBeenCalledWith(BOOK_META.id, 2, expect.any(String)) + expect(report.markedErrored).toEqual([{ jobId: 'job-regen', type: 'generate-chapter' }]) + }) + + it('falls back to meta.generatedUpTo + 1 when no targetChapterNum was journalled (the just-in-time next chapter)', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) // generatedUpTo: 4 + journal.record(makeJob({ id: 'job-next', type: 'generate-chapter', bookId: BOOK_META.id, params: {} })) + + const chapterStream = { seedInterrupted: vi.fn() } + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, chapterStream })) + + await resume() + + expect(chapterStream.seedInterrupted).toHaveBeenCalledWith(BOOK_META.id, 5, expect.any(String)) + }) + }) + + describe('journal hygiene', () => { + it('clears the original record of every job it handles, whichever of resumed, markedErrored, or skipped it lands in', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) // not complete: 4 of 6 + await books.saveBook({ ...BOOK_META, id: 'book-complete', generatedUpTo: 6, totalChapters: 6 }) + + journal.record(makeJob({ id: 'job-resumed', type: 'generate-all', bookId: BOOK_META.id })) + journal.record(makeJob({ id: 'job-skipped', type: 'generate-all', bookId: 'book-complete' })) + journal.record(makeJob({ id: 'job-errored-tray', type: 'generate-epub', bookId: BOOK_META.id })) + journal.record(makeJob({ id: 'job-errored-hub', type: 'generate-chapter', bookId: BOOK_META.id })) + + const resume = createResumeInterruptedJobs(makeDeps({ journal, books })) + + const report = await resume() + + expect(report.resumed).toContainEqual({ jobId: 'job-resumed', type: 'generate-all' }) + expect(report.skipped).toContainEqual({ jobId: 'job-skipped', type: 'generate-all', reason: expect.any(String) }) + expect(report.markedErrored).toContainEqual({ jobId: 'job-errored-tray', type: 'generate-epub' }) + expect(report.markedErrored).toContainEqual({ jobId: 'job-errored-hub', type: 'generate-chapter' }) + expect(await journal.listInterrupted()).toEqual([]) + }) + + // autoResume is a debugging escape hatch (TUTOR_NO_AUTO_RESUME=1), not a + // way to defer work. If it touched the journal, a later boot with the + // flag removed would find nothing left to resume, silently losing every + // job it was meant to merely delay looking at. + it('autoResume: false is a true no-op, so a later boot with the flag removed still finds every record', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + await books.saveBook(BOOK_META) + journal.record(makeJob({ id: 'job-1', type: 'generate-all', bookId: BOOK_META.id })) + journal.record(makeJob({ id: 'job-2', type: 'generate-epub', bookId: BOOK_META.id })) + journal.record(makeJob({ id: 'job-3', type: 'generate-chapter', bookId: BOOK_META.id })) + + const backgroundTasks = createFakeBackgroundTasks() + const chapterStream = { seedInterrupted: vi.fn() } + const generateAllChapters = stubGenerateAllChapters() + const generateAudiobook = stubGenerateAudiobook() + const resume = createResumeInterruptedJobs(makeDeps({ + journal, books, backgroundTasks, chapterStream, generateAllChapters, generateAudiobook, autoResume: false, + })) + + const report = await resume() + + expect(report).toEqual({ resumed: [], markedErrored: [], skipped: [] }) + expect(generateAllChapters).not.toHaveBeenCalled() + expect(generateAudiobook).not.toHaveBeenCalled() + expect(chapterStream.seedInterrupted).not.toHaveBeenCalled() + expect(backgroundTasks.list()).toEqual([]) + + const stillJournalled = await journal.listInterrupted() + expect(stillJournalled.map((j) => j.id).sort()).toEqual(['job-1', 'job-2', 'job-3']) + }) + }) + + describe('fault isolation', () => { + it('skips a job whose book no longer exists, without throwing out of the whole pass, and still resumes a good job that follows it', async () => { + const journal = createFakeJobJournal() + const books = createFakeBookRepository() + // Only 'book-2' exists. 'book-1' (referenced by the first job) does not, + // e.g. the user deleted the book after the job was journalled. + await books.saveBook({ ...BOOK_META, id: 'book-2', generatedUpTo: 0, totalChapters: 3 }) + + journal.record(makeJob({ id: 'job-missing-book', type: 'generate-all', bookId: 'book-1' })) + journal.record(makeJob({ id: 'job-good', type: 'generate-all', bookId: 'book-2' })) + + const generateAllChapters = stubGenerateAllChapters({ taskId: 'task-good' }) + const resume = createResumeInterruptedJobs(makeDeps({ journal, books, generateAllChapters })) + + // No try/catch here on purpose: if resume let the missing-book + // failure escape instead of catching it per-job, this await would + // reject and fail the test on its own. + const report = await resume() + + expect(report.skipped).toContainEqual({ jobId: 'job-missing-book', type: 'generate-all', reason: expect.any(String) }) + expect(generateAllChapters).toHaveBeenCalledTimes(1) + expect(generateAllChapters.mock.calls[0][0]).toBe('book-2') + expect(report.resumed).toContainEqual({ jobId: 'job-good', type: 'generate-all' }) + expect(await journal.listInterrupted()).toEqual([]) + }) + }) +}) diff --git a/server/services/resume-interrupted-jobs.ts b/server/services/resume-interrupted-jobs.ts new file mode 100644 index 0000000..417c690 --- /dev/null +++ b/server/services/resume-interrupted-jobs.ts @@ -0,0 +1,183 @@ +import type { GenerationJob, GenerationJobType } from '@shared/domain.js' +import type { JobJournal } from '../ports/job-journal.js' +import type { BookRepository } from '../ports/book-repository.js' +import type { BackgroundTasks } from '../ports/background-tasks.js' +import type { ChapterGenerationStream } from './chapter-generation-stream.js' +import type { GenerateNextChapterOptions } from './generate-next-chapter.js' +import { DEFAULT_MODEL } from '../constants.js' + +/** + * Runs once at boot, after crash recovery (see the ordering comment on + * runStartupTasks in server/index.ts), and decides what to do with every + * job whose journal record survived a restart. A surviving record can only + * mean the process ended before that job reached a terminal state, see + * JobJournal's own doc for why. + * + * The idempotency rule that makes this safe: disk is the truth, and the + * journalled checkpoint is advisory only. generate-all never resumes from + * checkpoint.through, it re-reads meta.yml through BookRepository and + * hands the resumed generateAllChapters call the SAME generatedUpTo a + * fresh request would see, so a stale or wrong checkpoint can never cause + * a chapter already on disk to be regenerated, the checkpoint only ever + * seeded a progress label, never a decision. generate-audiobook resumes by + * restarting narration from the beginning rather than trying to skip + * chapters already narrated, not out of caution but because by the time + * this runs, crash recovery has already deleted the book's whole audio/ + * directory whenever book.m4b was absent and cleared + * audioGeneratedChapters, so there is nothing partial left to skip. + * + * Per type policy: + * - generate-all and generate-audiobook: auto-resumed, unless the book + * already satisfies the job (fully generated already, or the resumed + * call itself reports it did not actually start), in which case + * skipped instead, with the reason recorded. + * - generate-epub, generate-cover, install-audiobook: short, cheap + * steps. Marked errored and retriable in the existing tray rather than + * resumed, so the user can re-click the button that already exists. + * - generate-chapter: never a tray task, so instead the single-chapter + * generation hub is seeded with a terminal error via seedInterrupted, + * which the reader's existing generation-error panel surfaces. + * + * Every job this function decides on, whichever of resumed, markedErrored, + * or skipped it lands in, has its ORIGINAL journal record cleared once + * decided, so the next boot does not process it again. The one exception + * is autoResume: false (TUTOR_NO_AUTO_RESUME=1), a debugging escape hatch + * that leaves the journal completely untouched, so removing the flag on a + * later boot finds every job exactly as it was. + * + * One bad job, most commonly one whose book was deleted after the job was + * journalled, is skipped rather than allowed to throw out of the whole + * pass, the same promise the library migrator makes for a single corrupt + * book. Every attempted job logs its outcome. + */ + +export interface ResumeReport { + resumed: Array<{ jobId: string; type: GenerationJobType }> + markedErrored: Array<{ jobId: string; type: GenerationJobType }> + skipped: Array<{ jobId: string; type: GenerationJobType; reason: string }> +} + +/** Shown on every tray task this pass marks errored, and used verbatim as the generate-chapter hub error message too. */ +const INTERRUPTED_MESSAGE = 'Interrupted by restart' + +export function createResumeInterruptedJobs(deps: { + journal: JobJournal + books: Pick + backgroundTasks: BackgroundTasks + chapterStream: Pick + generateAllChapters: ( + bookId: string, + meta: { title: string; generatedUpTo: number; totalChapters: number }, + options: GenerateNextChapterOptions, + ) => { taskId: string } + generateAudiobook: (req: { bookId: string; voiceId?: string; speed?: number; confirmReplace?: boolean }) => Promise<{ outcome: string; taskId?: string }> + autoResume: boolean + log: (msg: string, ctx?: Record) => void +}): () => Promise { + const { journal, books, backgroundTasks, chapterStream, generateAllChapters, generateAudiobook, autoResume, log } = deps + + /** The handful of request parameters a job needs to restart, reconstructed from what was journalled. model always has a value, falling back the same way an on-demand quiz regeneration does. */ + function optionsFrom(params: GenerationJob['params']): GenerateNextChapterOptions { + return { + model: params.model ?? DEFAULT_MODEL, + ...(params.provider !== undefined ? { provider: params.provider } : {}), + ...(params.quizModel !== undefined ? { quizModel: params.quizModel } : {}), + ...(params.quizProvider !== undefined ? { quizProvider: params.quizProvider } : {}), + ...(params.quizLength !== undefined ? { quizLength: params.quizLength } : {}), + } + } + + async function handle(job: GenerationJob, report: ResumeReport): Promise { + // Fetched fresh for every job, never trusted from the journal, this + // one call is what makes "disk is the truth" true rather than aspirational. + const meta = await books.getBook(job.bookId) + + switch (job.type) { + case 'generate-all': { + if (meta.generatedUpTo >= meta.totalChapters) { + report.skipped.push({ jobId: job.id, type: job.type, reason: 'Book is already fully generated' }) + log('resume: skipped generate-all, book already complete', { jobId: job.id, bookId: job.bookId }) + return + } + generateAllChapters(job.bookId, meta, optionsFrom(job.params)) + report.resumed.push({ jobId: job.id, type: job.type }) + log('resume: resumed generate-all', { jobId: job.id, bookId: job.bookId, generatedUpTo: meta.generatedUpTo }) + return + } + + case 'generate-audiobook': { + const result = await generateAudiobook({ + bookId: job.bookId, + confirmReplace: true, + ...(job.params.voiceId !== undefined ? { voiceId: job.params.voiceId } : {}), + ...(job.params.speed !== undefined ? { speed: job.params.speed } : {}), + }) + if (result.outcome === 'started') { + report.resumed.push({ jobId: job.id, type: job.type }) + log('resume: resumed generate-audiobook', { jobId: job.id, bookId: job.bookId }) + } else { + report.skipped.push({ jobId: job.id, type: job.type, reason: result.outcome }) + log('resume: skipped generate-audiobook', { jobId: job.id, bookId: job.bookId, reason: result.outcome }) + } + return + } + + case 'generate-epub': + case 'generate-cover': + case 'install-audiobook': { + const handle = backgroundTasks.start({ type: job.type, bookId: job.bookId, bookTitle: job.bookTitle, total: 1 }) + backgroundTasks.fail(handle.id, INTERRUPTED_MESSAGE) + report.markedErrored.push({ jobId: job.id, type: job.type }) + log('resume: marked errored and retriable in the tray', { jobId: job.id, bookId: job.bookId, type: job.type }) + return + } + + case 'generate-chapter': { + const chapterNum = job.params.targetChapterNum ?? meta.generatedUpTo + 1 + chapterStream.seedInterrupted(job.bookId, chapterNum, INTERRUPTED_MESSAGE) + report.markedErrored.push({ jobId: job.id, type: job.type }) + log('resume: seeded an interrupted chapter generation in the hub', { jobId: job.id, bookId: job.bookId, chapterNum }) + return + } + + default: { + // Exhaustiveness guard: a compile error here means a GenerationJobType + // was added without a case above. + const exhaustive: never = job.type + throw new Error(`resume: no handler for job type "${String(exhaustive)}"`) + } + } + } + + return async function resumeInterruptedJobs(): Promise { + const report: ResumeReport = { resumed: [], markedErrored: [], skipped: [] } + + if (!autoResume) { + // A debugging escape hatch (TUTOR_NO_AUTO_RESUME=1), not a way to + // defer work. The journal is never even read, let alone cleared, + // because touching it here would mean a later boot with the flag + // removed finds nothing left to resume, silently losing every job + // this was only ever meant to skip looking at for now. + log('resume: autoResume disabled (TUTOR_NO_AUTO_RESUME=1), leaving the journal untouched') + return report + } + + const jobs = await journal.listInterrupted() + + for (const job of jobs) { + try { + await handle(job, report) + } catch (err) { + const reason = err instanceof Error ? err.message : 'Failed to resume' + report.skipped.push({ jobId: job.id, type: job.type, reason }) + log('resume: skipped a job after it failed', { jobId: job.id, bookId: job.bookId, type: job.type, reason }) + } finally { + // Cleared regardless of outcome, so a job this pass has already + // decided on is never re-processed at the next boot. + journal.clear(job.id) + } + } + + return report + } +} diff --git a/shared/domain.ts b/shared/domain.ts index 1615ac6..e5008d8 100644 --- a/shared/domain.ts +++ b/shared/domain.ts @@ -1,5 +1,6 @@ import { z } from 'zod' import { BOOK_STATUSES, type BookStatus } from './book-status.js' +import { ProviderSchema, MODEL_REGEX } from './provider.js' /** * Built here rather than in `shared/book-status.ts` so that module can stay @@ -55,6 +56,24 @@ export const SkillSchema = z.object({ export type Preferences = z.infer export const LearningProfileSchema = z.object({ + /** + * Which schema version this profile was last written at. Optional rather + * than defaulted on purpose: a `.default()` here would make the field + * required in LearningProfile's inferred type, forcing every existing + * object literal typed as LearningProfile across the codebase to name a + * version number it has no business knowing. Left optional, an absent + * field parses exactly as version 1 always has, and every one of those + * call sites is untouched. + * + * This schema does not stamp anything current. That happens on the write + * side, in fs-book-repository's saveProfile, which sets this field to + * CURRENT_PROFILE_SCHEMA_VERSION on every write, so anything the running + * app produces is current by construction. The migrator that upgrades an + * old file reads the raw YAML below this schema either way, so this + * optional field can never fool it into treating an unmigrated file as + * already current. + */ + schemaVersion: z.number().int().positive().optional(), style: z.string(), identity: z.string(), preferences: PreferencesSchema, @@ -95,6 +114,24 @@ export type Toc = z.infer export { type BookStatus } export const BookMetaSchema = z.object({ + /** + * Which schema version this book folder was last written at. Optional + * rather than defaulted on purpose: a `.default()` here would make the + * field required in BookMeta's inferred type, and BookMeta is built as + * an object literal at dozens of call sites across the server that have + * no business knowing a version number. Left optional, an absent field + * parses exactly as version 1 always has, and every one of those call + * sites is untouched. + * + * This schema does not stamp anything current. That happens on the write + * side, in fs-book-repository's saveBook, which sets this field to + * CURRENT_BOOK_SCHEMA_VERSION on every write, so anything the running + * app produces is current by construction. The migrator that upgrades an + * old file reads the raw YAML below this schema either way, so this + * optional field can never fool it into treating an unmigrated file as + * already current. + */ + schemaVersion: z.number().int().positive().optional(), id: z.string(), title: z.string(), subtitle: z.string().optional(), @@ -206,3 +243,94 @@ export const AudiobookManifestSchema = z.object({ export type AudiobookChapterEntry = z.infer export type AudiobookManifest = z.infer + +// --- Generation jobs --- + +/** + * The on-disk shape of an in-flight background job, written by + * server/adapters/journalled-background-tasks.ts through + * server/ports/job-journal.ts so a job interrupted by a crash or restart + * can be found and resumed the next time the app starts. It deliberately + * carries only what restarting the job needs, a task type, which book, + * how far it got, and the handful of request parameters that chose its + * provider, model, and voice. An API key is never among that. A key + * proves who is asking, and belongs in KeyVault alone, restarting a job + * needs to know what was asked for, not who paid for it, and this journal + * is written to disk unencrypted, unlike KeyVault's storage. + */ + +/** + * Every TaskType in shared/responses.ts, plus 'generate-chapter' for the + * just-in-time single chapter generation that never went through + * BackgroundTasks before this journal existed. Pinned together with + * TaskType by the compile-time guard in shared/generation-job.test.ts, + * which cannot live in this module without importing shared/responses.ts + * and creating a cycle, responses.ts already imports from here. + */ +export const GENERATION_JOB_TYPES = [ + 'generate-all', + 'generate-epub', + 'generate-cover', + 'install-audiobook', + 'generate-audiobook', + 'generate-chapter', +] as const + +export const GenerationJobTypeSchema = z.enum(GENERATION_JOB_TYPES) +export type GenerationJobType = z.infer + +/** How far a job got before it stopped, enough to resume without redoing finished work. */ +export const GenerationJobCheckpointSchema = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('none') }), + z.object({ kind: z.literal('chapters'), through: z.number().int().min(0) }), + z.object({ kind: z.literal('narration-complete') }), +]) + +export type GenerationJobCheckpoint = z.infer + +/** + * The request parameters a job needs to restart, the same handful a + * client already chooses in shared/contracts.ts's request bodies. + * z.strictObject rather than z.object, so a future field added here + * without matching thought, an API key above all, fails to parse instead + * of silently being written to disk. + */ +export const GenerationJobParamsSchema = z.strictObject({ + provider: ProviderSchema.optional(), + model: z.string().min(1).max(100).regex(MODEL_REGEX).optional(), + quizProvider: ProviderSchema.optional(), + quizModel: z.string().min(1).max(100).regex(MODEL_REGEX).optional(), + quizLength: z.number().int().min(1).max(10).optional(), + voiceId: z.string().min(1).max(100).optional(), + speed: z.number().min(0.5).max(2.0).optional(), + targetChapterNum: z.number().int().positive().optional(), +}) + +export type GenerationJobParams = z.infer + +export const GenerationJobSchema = z.object({ + id: z.string(), + type: GenerationJobTypeSchema, + bookId: z.string(), + bookTitle: z.string(), + /** + * Only ever 'running' on disk, because a record is cleared the moment its + * job settles. JobJournal.listInterrupted() reports every surviving record + * as 'interrupted' regardless, since a record that outlived its process + * can only mean the process died before clearing it. + * + * Both values are still named here rather than collapsing the field to a + * bare string. A two-valued field typed as `string` tells a reader nothing + * and lets a typo through, and this is the domain module, where the shapes + * are supposed to be the documentation. Note this is a literal union, not + * TaskStatus, which lives in shared/responses.ts and cannot be imported + * here without creating a cycle. + */ + status: z.enum(['running', 'interrupted']), + checkpoint: GenerationJobCheckpointSchema, + params: GenerationJobParamsSchema, + startedAt: z.string(), + updatedAt: z.string(), +}) + +export type GenerationJob = z.infer diff --git a/shared/events.ts b/shared/events.ts index 4ce8777..f3b702f 100644 --- a/shared/events.ts +++ b/shared/events.ts @@ -1,4 +1,4 @@ -import type { ClientTask, GenerationStage, TaskProgress, TaskType } from './responses.js' +import type { AiErrorKind, ClientTask, GenerationStage, TaskProgress, TaskType } from './responses.js' /** * The Server-Sent Event unions the streaming routes actually write today, @@ -14,8 +14,18 @@ import type { ClientTask, GenerationStage, TaskProgress, TaskType } from './resp * file is types only — it compiles away entirely. */ -/** The error event sent verbatim by the create-book, revise-toc, start-book, and generate-chapter streams. */ -export type StreamErrorEvent = { type: 'error'; message: string } +/** + * The error event sent verbatim by the create-book, revise-toc, start-book, + * and generate-chapter streams. + * + * `kind` is purely additive and always optional. Every existing client path + * reads `message` and keeps working without it. It carries the failure class + * when the failure came from the AI provider, which is what lets the reader + * route an auth failure to the missing-key dialog instead of showing a + * generic toast that the user cannot act on. A failure with no class, such + * as a parse error the app raised itself, simply omits it. + */ +export type StreamErrorEvent = { type: 'error'; message: string; kind?: AiErrorKind } /** POST /api/books — table-of-contents generation stream. */ export type CreateBookEvent = diff --git a/shared/generation-job.test.ts b/shared/generation-job.test.ts new file mode 100644 index 0000000..3db4102 --- /dev/null +++ b/shared/generation-job.test.ts @@ -0,0 +1,124 @@ +import { describe, expect, it } from 'vitest' +import type { TaskType } from './responses.js' +import { + GENERATION_JOB_TYPES, + GenerationJobTypeSchema, + type GenerationJobType, + GenerationJobCheckpointSchema, + GenerationJobParamsSchema, + GenerationJobSchema, + type GenerationJob, +} from './domain.js' + +/** + * Pins the on-disk shape of an in-flight background job: the type union + * that widens TaskType, the checkpoint a job can resume from, the request + * parameters it can carry, and the record those combine into. These types + * do not exist in shared/domain.ts yet, this file is written against the + * shape server/ports/job-journal.ts is about to persist. + */ + +describe('GENERATION_JOB_TYPES / GenerationJobTypeSchema', () => { + it('has exactly six members, the five TaskTypes plus generate-chapter', () => { + const taskTypes: TaskType[] = [ + 'generate-all', + 'generate-epub', + 'generate-cover', + 'install-audiobook', + 'generate-audiobook', + ] + expect(GENERATION_JOB_TYPES).toHaveLength(6) + expect([...GENERATION_JOB_TYPES].sort()).toEqual([...taskTypes, 'generate-chapter'].sort()) + }) + + it('parses every one of its own members', () => { + for (const type of GENERATION_JOB_TYPES) { + expect(GenerationJobTypeSchema.safeParse(type).success).toBe(true) + } + expect(GenerationJobTypeSchema.safeParse('not-a-real-type').success).toBe(false) + }) + + // Compile-time-only guard: every TaskType (shared/responses.ts) must widen + // to a GenerationJobType (shared/domain.ts), so the background-task tray's + // type union and this journal's job-type union can never silently drift + // apart, a type added to one without the other fails this assignment. It + // lives here, in a test, rather than beside either type, because + // shared/domain.ts cannot import shared/responses.ts without creating a + // cycle, responses.ts already imports from domain.ts. Editing one union + // without the other fails `pnpm typecheck` on the line below, not this + // file's runtime assertion, which exists only so vitest has something to + // run in this block. + const _widens: GenerationJobType = null as unknown as TaskType + void _widens + + it('is a compile-time-only guard pinning TaskType as a subset of GenerationJobType, see the comment above', () => { + expect(true).toBe(true) + }) +}) + +describe('GenerationJobCheckpointSchema', () => { + it('parses none, chapters with a through count, and narration-complete', () => { + expect(GenerationJobCheckpointSchema.safeParse({ kind: 'none' }).success).toBe(true) + expect(GenerationJobCheckpointSchema.safeParse({ kind: 'chapters', through: 3 }).success).toBe(true) + expect(GenerationJobCheckpointSchema.safeParse({ kind: 'narration-complete' }).success).toBe(true) + }) + + it('rejects chapters with no through, and an unknown kind', () => { + expect(GenerationJobCheckpointSchema.safeParse({ kind: 'chapters' }).success).toBe(false) + expect(GenerationJobCheckpointSchema.safeParse({ kind: 'not-a-real-kind' }).success).toBe(false) + }) +}) + +describe('GenerationJobParamsSchema', () => { + it('accepts the real restart parameters, all optional, and an empty object', () => { + expect(GenerationJobParamsSchema.safeParse({}).success).toBe(true) + expect( + GenerationJobParamsSchema.safeParse({ + provider: 'anthropic', + model: 'claude-sonnet-4-20250514', + quizProvider: 'openai', + quizModel: 'gpt-4o', + quizLength: 3, + voiceId: 'onyx', + speed: 1.0, + targetChapterNum: 5, + }).success, + ).toBe(true) + }) + + // The guard that keeps a provider credential out of a file on disk. The + // journal this schema backs is written unencrypted, unlike KeyVault, so + // an API key must never be a field a job's params can carry, no matter + // how convenient it would be for restarting a job without re-prompting. + it('rejects an object containing apiKey, and any other unknown key, so the guard is general rather than a blocklist of one', () => { + expect(GenerationJobParamsSchema.safeParse({ provider: 'anthropic', apiKey: 'sk-should-not-be-here' }).success).toBe(false) + expect(GenerationJobParamsSchema.safeParse({ someUnrelatedField: 'nope' }).success).toBe(false) + }) +}) + +describe('GenerationJobSchema', () => { + const FULL_JOB: GenerationJob = { + id: 'job-1', + type: 'generate-chapter', + bookId: 'book-1', + bookTitle: 'Test Book', + status: 'running', + checkpoint: { kind: 'chapters', through: 2 }, + params: { provider: 'anthropic', targetChapterNum: 3, voiceId: 'onyx' }, + startedAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-01T00:05:00.000Z', + } + + it('round trips a full record', () => { + const result = GenerationJobSchema.safeParse(FULL_JOB) + expect(result.success).toBe(true) + if (result.success) { + expect(result.data).toEqual(FULL_JOB) + } + }) + + it('rejects a record missing checkpoint', () => { + const { checkpoint: _checkpoint, ...withoutCheckpoint } = FULL_JOB + expect(GenerationJobSchema.safeParse(withoutCheckpoint).success).toBe(false) + }) +}) diff --git a/shared/responses.ts b/shared/responses.ts index 5b3fd6a..d04aa08 100644 --- a/shared/responses.ts +++ b/shared/responses.ts @@ -27,10 +27,27 @@ export type LibraryBook = BookMeta & { /** The stage a background chapter generation is in, consumed directly by server/services/chapter-generation-stream.ts, not mirrored from elsewhere. */ export type GenerationStage = 'streaming' | 'saving' | 'quiz' | 'done' | 'error' -/** GET /api/books/:id/generation-status — background chapter generation progress for one book. */ +/** Mirrors TextGenerationErrorKind in server/ports/text-generation.ts, so the client can switch on an AI failure class without importing zod or anything under server/. Pinned together by the drift guard in server/ports/ai-error-kind.drift.test.ts. */ +export type AiErrorKind = + | 'auth-failed' + | 'rate-limited' + | 'overloaded' + | 'timed-out' + | 'network-failed' + | 'content-refused' + | 'unknown' + +/** + * GET /api/books/:id/generation-status — background chapter generation + * progress for one book. `error` is set on the active variant when a + * generation ended in error, including one that was interrupted by a + * server restart and seeded that way at boot rather than having actually + * streamed and failed live. The reader surfaces it through its existing + * generation-error panel either way. + */ export type GenerationStatus = | { active: false } - | { active: true; chapterNum: number; stage: GenerationStage; contentLength: number } + | { active: true; chapterNum: number; stage: GenerationStage; contentLength: number; error?: string; errorKind?: AiErrorKind } /** GET /api/books/:id — book meta plus the current generation status. */ export type BookDetail = BookMeta & { generation: GenerationStatus } diff --git a/shared/schema-version.test.ts b/shared/schema-version.test.ts new file mode 100644 index 0000000..8573748 --- /dev/null +++ b/shared/schema-version.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect } from 'vitest' +import { + CURRENT_BOOK_SCHEMA_VERSION, + CURRENT_PROFILE_SCHEMA_VERSION, + readSchemaVersion, + assertSchemaVersionSupported, + SchemaTooNewError, +} from './schema-version.js' + +// Pure version reading and guarding, no I/O and no zod. Every migration +// chain and every future read-side guard shares this one reading of "how +// old is this file," so it is tested here in isolation from both. + +describe('CURRENT_BOOK_SCHEMA_VERSION and CURRENT_PROFILE_SCHEMA_VERSION', () => { + it('are both 2, matching the one shipped migration per chain', () => { + expect(CURRENT_BOOK_SCHEMA_VERSION).toBe(2) + expect(CURRENT_PROFILE_SCHEMA_VERSION).toBe(2) + }) +}) + +describe('readSchemaVersion', () => { + it('reads an absent field as version 1', () => { + expect(readSchemaVersion({})).toBe(1) + expect(readSchemaVersion({ id: 'book-1', title: 'Untitled' })).toBe(1) + }) + + it('reads a non-object as version 1', () => { + expect(readSchemaVersion(null)).toBe(1) + expect(readSchemaVersion(undefined)).toBe(1) + expect(readSchemaVersion('v1')).toBe(1) + expect(readSchemaVersion(42)).toBe(1) + expect(readSchemaVersion(['not', 'a', 'record'])).toBe(1) + }) + + it('reads a numeric field as itself', () => { + expect(readSchemaVersion({ schemaVersion: 1 })).toBe(1) + expect(readSchemaVersion({ schemaVersion: 2 })).toBe(2) + expect(readSchemaVersion({ schemaVersion: 99 })).toBe(99) + }) + + it('reads a non-numeric field as version 1', () => { + expect(readSchemaVersion({ schemaVersion: '2' })).toBe(1) + expect(readSchemaVersion({ schemaVersion: null })).toBe(1) + expect(readSchemaVersion({ schemaVersion: {} })).toBe(1) + expect(readSchemaVersion({ schemaVersion: [2] })).toBe(1) + }) + + it('reads a negative field as version 1, on the theory that a garbage version is ancient rather than a reason to refuse to boot', () => { + // A negative (or otherwise nonsensical) value is never one this app + // wrote. Reading it as 1 sends it through every migration step, which + // is a repair attempt; the steps are safe to run against a file that + // never needed them. The alternative, refusing to boot, would turn one + // corrupt field into a hard outage for no benefit. + expect(readSchemaVersion({ schemaVersion: -1 })).toBe(1) + expect(readSchemaVersion({ schemaVersion: -99 })).toBe(1) + }) +}) + +describe('assertSchemaVersionSupported', () => { + it('is a no-op at or below the supported version', () => { + expect(() => assertSchemaVersionSupported({}, 2)).not.toThrow() + expect(() => assertSchemaVersionSupported({ schemaVersion: 1 }, 2)).not.toThrow() + expect(() => assertSchemaVersionSupported({ schemaVersion: 2 }, 2)).not.toThrow() + }) + + it('throws SchemaTooNewError when the found version exceeds supported', () => { + expect(() => assertSchemaVersionSupported({ schemaVersion: 3 }, 2)).toThrow(SchemaTooNewError) + }) + + it('carries found, supported, and path on the thrown error', () => { + let thrown: unknown + try { + assertSchemaVersionSupported({ schemaVersion: 5 }, 2, '/data/books/some-book/meta.yml') + } catch (error) { + thrown = error + } + expect(thrown).toBeInstanceOf(SchemaTooNewError) + const error = thrown as SchemaTooNewError + expect(error.found).toBe(5) + expect(error.supported).toBe(2) + expect(error.path).toBe('/data/books/some-book/meta.yml') + }) + + it('leaves path undefined when the caller does not have one', () => { + let thrown: unknown + try { + assertSchemaVersionSupported({ schemaVersion: 5 }, 2) + } catch (error) { + thrown = error + } + expect((thrown as SchemaTooNewError).path).toBeUndefined() + }) +}) diff --git a/shared/schema-version.ts b/shared/schema-version.ts new file mode 100644 index 0000000..2ca2cad --- /dev/null +++ b/shared/schema-version.ts @@ -0,0 +1,86 @@ +/** + * Two independent version counters for the two things Tutor persists at the + * top of a data directory: `meta.yml` per book, and `learning-profile.yml` + * once per library. + * + * A book gets its own counter, on `meta.yml`, because a book folder is + * portable. An EPUB import creates one from scratch, and a user can copy a + * single book directory between machines without the rest of the library. + * Whatever schema shape it is written in has to travel with it, not live in + * a separate library-wide file the folder might arrive without. + * + * The profile gets its own counter, on `learning-profile.yml`, because a + * profile exists before any book does. A fresh install has a profile and + * zero books, so tying its version to a book, or to a library-wide file + * that only makes sense once a book exists, would leave it unversioned at + * exactly the moment versioning matters most, first boot. + * + * Absent field means version 1. Every build before this one wrote `meta.yml` + * and `learning-profile.yml` with no `schemaVersion` key at all, so "the key + * is missing" is not corruption, it is simply the oldest possible file. + */ + +export const CURRENT_BOOK_SCHEMA_VERSION = 2 +export const CURRENT_PROFILE_SCHEMA_VERSION = 2 + +/** + * Reads `schemaVersion` off a raw, not-yet-validated YAML document. Used + * below its matching Zod schema on purpose, migration has to know a file's + * version before that file can be trusted to parse. + * + * A missing field, a non-object, a non-numeric value, and a negative value + * all read as 1. The first two are the documented "nothing has ever + * written a version here" case. The last two are deliberately treated the + * same way rather than rejected: a garbage value cannot be a version this + * app ever wrote, and reading it as 1 routes the file through every + * migration step as a repair attempt. Migration steps are safe to run + * against a file that never needed them, so this favors an attempted + * repair over refusing to boot on a field that was never going to be + * trustworthy anyway. + */ +export function readSchemaVersion(raw: unknown): number { + if (typeof raw !== 'object' || raw === null) return 1 + const value = (raw as { schemaVersion?: unknown }).schemaVersion + if (typeof value !== 'number' || !Number.isFinite(value) || value < 0) return 1 + return value +} + +/** + * Thrown when a file's schema version is newer than this build supports, + * meaning the library was written by a newer release and the app was + * downgraded, or pointed at someone else's newer data directory. `found` + * and `supported` let a caller report the exact gap rather than a generic + * parse failure, and `path` names the offending file when the caller has + * one, so the message points at something fixable instead of just failing. + */ +export class SchemaTooNewError extends Error { + readonly found: number + readonly supported: number + readonly path?: string + + constructor(found: number, supported: number, path?: string) { + const location = path ? ` (${path})` : '' + super( + `Schema version ${found}${location} is newer than this build supports (up to ${supported}). ` + + 'Update the app to read this library.', + ) + this.name = 'SchemaTooNewError' + this.found = found + this.supported = supported + this.path = path + } +} + +/** + * Guards a read: no-op when the raw document's version is at or below what + * this build supports, otherwise throws SchemaTooNewError. Deliberately + * separate from readSchemaVersion so a caller that only wants the number, + * such as a migration step choosing where to start, never has to catch an + * exception to get it. + */ +export function assertSchemaVersionSupported(raw: unknown, supported: number, path?: string): void { + const found = readSchemaVersion(raw) + if (found > supported) { + throw new SchemaTooNewError(found, supported, path) + } +}