From 771d755da4e0fc39979ff0d93835cd3ccfc07f23 Mon Sep 17 00:00:00 2001 From: Lucas Winther Date: Fri, 30 Oct 2026 20:53:50 +0100 Subject: [PATCH] fix(studio): review round 1 for imported-course editing - Media exclusions move from the editable root .gitignore to the repo's info/exclude, and dirtyFiles never counts files listed in media.json, so an author's or remote's .gitignore cannot block publish or Sync. Publish and Sync refuse commits that track a file over referenced media. - Importer jobs (code-repo, pdf-deck, transcribe) and the cover route run under the course lock now shared with the studio (core withCourseLock) and re-check the course inside it; after conversion they fail with "this course is now edited in the studio". Convert answers 409 while an import job for the course is queued or running, and the job re-checks. - Crash-safe conversion: the import folder is hidden (.converting-) before the published worktree exists; cross-filesystem moves copy into a staging dir, fsync, verify size and sha256, then rename; the source is retired as .converted-.. Recovery runs at server start and before every convert; a re-run of a finished conversion reports success. The loader skips dot-folders. - Minor: no raw NUL bytes in source, link fallback only on EXDEV/EPERM/EMLINK, referenced media are not read whole by the editor, missing media are logged, mediaDir JSDoc, docs on per-author copies across filesystems. Co-Authored-By: Claude Opus 5.5 --- docs/operations.md | 40 ++- packages/core/src/index.ts | 1 + packages/core/src/locks.ts | 14 + packages/course-format/src/load.ts | 4 +- packages/course-format/test/load.test.ts | 7 + packages/feature-import/src/context.ts | 13 + packages/feature-import/src/jobs/code-repo.ts | 10 +- packages/feature-import/src/jobs/pdf-deck.ts | 5 +- .../feature-import/src/jobs/transcribe.ts | 7 +- packages/feature-import/src/routes.ts | 21 +- .../feature-import/test/studio-race.test.ts | 56 ++++ packages/feature-studio/src/server/convert.ts | 39 ++- packages/feature-studio/src/server/feature.ts | 22 +- .../feature-studio/src/server/media-refs.ts | 10 +- packages/feature-studio/src/server/store.ts | 270 +++++++++++++++--- .../test/convert-routes.test.ts | 46 +++ packages/feature-studio/test/convert.test.ts | 141 ++++++++- 17 files changed, 618 insertions(+), 88 deletions(-) create mode 100644 packages/core/src/locks.ts create mode 100644 packages/feature-import/test/studio-race.test.ts diff --git a/docs/operations.md b/docs/operations.md index 404b62e..5ea07cf 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -90,24 +90,47 @@ studio** on the course's studio page. That runs a background job (`studio.conver and are kept; 2. keeps **large media out of git**: every video or audio file (`mp4 m4v webm mov mkv avi mp3 m4a wav ogg oga opus flac`) and any other file over 8 MiB. Each is listed with its path, size and SHA-256 in `.stcourses/media.json` - in the repo and excluded by one anchored line in the repo's root `.gitignore` (a `.gitignore` the course already - had is kept above that block). Git LFS is not used: it is not in the image and would need an LFS server; + in the repo. They are kept out of `git status` by the repo's `info/exclude` (shared by every worktree), not by + the course's `.gitignore`, and the studio never counts a listed file as a change. So an author or a remote + rewriting `.gitignore` cannot block publishing or Sync. Git LFS is not used: it is not in the image and would + need an LFS server; 3. **moves the import folder to `/media/`** (decided over keeping it in place: a manifest left in `/courses/` would be found as a second course with the same id, and the importer's "add to course" and transcription jobs would keep changing a folder learners no longer see). `/media` is never scanned for courses and nothing writes to it afterwards. File permissions are left alone, because an import made in `link` mode shares its files with your originals; 4. puts every referenced file into the published worktree, and later into each author's draft worktree, as a - **hard link** to `/media//` (a copy if they are on different filesystems). The manifest paths are - unchanged, so `/api/media//…` serves the same URLs with Range support, and no extra disk space is used. + **hard link** to `/media//`. The manifest paths are unchanged, so `/api/media//…` serves the + same URLs with Range support, and no extra disk space is used. If `/worktrees` and `/media` are on + different filesystems, each worktree gets a **full copy** instead: the published one plus one per author who + opens a draft. Keep the data directory on one filesystem. In the studio, referenced media cannot be replaced or deleted (save, upload and conflict resolution refuse them with 409); upload a new file under another name instead. A studio write never goes through a hard link: a file with more -than one link is replaced, not overwritten. +than one link is replaced, not overwritten. Publish and Sync refuse a commit that tracks a file at a referenced media +path, since it would silently replace the media. -Interrupted conversions are safe to retry, and a job left running by a restart is retried automatically: if no -published worktree was made, the partial repo is removed and the conversion starts again; if the repo exists but the -import folder was not moved yet, only the move is done. +**Importer jobs and conversion.** Both use the same per-course lock. Conversion is refused (409, "an import job for +this course is still running") while an importer job for that course is queued or running. An importer job that +starts after the conversion fails with "this course is now edited in the studio" and writes nothing. + +**Crash safety.** The import folder is first renamed to `/courses/.converting-` (dot-folders are never +scanned), before the published worktree is checked out, so the course is never loaded twice. Until that checkout is +done, a failure undoes everything. After it, the conversion is committed and only the move remains: + +- on one filesystem, one `rename` to `/media/`; +- across filesystems, a copy into `/media/.staging-.`, `fsync`, a size and SHA-256 check against + `media.json`, a `rename` into place, and then the source is renamed to `/courses/.converted-.` and + deleted. + +At server start and before every conversion, STCourses recovers each course deterministically: + +- staging folders are deleted; +- a `.converting-` folder without a finished repo is renamed back (the next attempt starts over); +- a `.converting-` folder with a finished repo has its move finished; +- `.converted-` folders are deleted once `/media/` is complete. + +A job left running by a restart is requeued, and re-running a finished conversion reports success. Things to know: @@ -119,6 +142,7 @@ Things to know: as a course diagnostic ("lesson file not found"), never a crash. - After conversion the importer can no longer add decks, code or transcripts to that course (it only edits `/courses`). Add them in the studio. +- `/media` is fixed under the data directory (there is no separate setting). ## Memory budget diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 80df762..888bba5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -3,3 +3,4 @@ export { createJobQueue } from "./jobs"; export { serveMedia, contentType, contentDisposition, mediaSecurityHeaders, MEDIA_SANDBOX_CSP } from "./media"; export { createApp, canView, crossSiteRefusal, type Routes } from "./app"; export { composeServer, type ComposeOptions, type ComposedServer } from "./compose"; +export { withCourseLock } from "./locks"; diff --git a/packages/core/src/locks.ts b/packages/core/src/locks.ts new file mode 100644 index 0000000..1a1d720 --- /dev/null +++ b/packages/core/src/locks.ts @@ -0,0 +1,14 @@ +const locks = new Map>(); + +/** + * Serialises work on one course across features in this process: the studio (saves, publish, conversion of an import) + * and the importer's jobs that add to an existing course. A failed holder never blocks the next one. + */ +export function withCourseLock(courseId: string, fn: () => Promise): Promise { + const prev = locks.get(courseId) ?? Promise.resolve(); + const next = prev.then(fn, fn); + const tail = next.catch(() => {}); + locks.set(courseId, tail); + tail.then(() => { if (locks.get(courseId) === tail) locks.delete(courseId); }); + return next; +} diff --git a/packages/course-format/src/load.ts b/packages/course-format/src/load.ts index 579d6ee..b3171f9 100644 --- a/packages/course-format/src/load.ts +++ b/packages/course-format/src/load.ts @@ -39,7 +39,7 @@ export function resolveInside(root: string, rel: string): string | null { } } -/** Finds stcourse.yaml at depth 0-2 under each dir. */ +/** Finds stcourse.yaml at depth 0-2 under each dir. Dot-folders are skipped (hidden, or retired by the studio). */ export async function discoverManifests(courseDirs: string[]): Promise { const found: string[] = []; const seen = new Set(); @@ -60,7 +60,7 @@ export async function discoverManifests(courseDirs: string[]): Promise } entries.sort((a, b) => a.name.localeCompare(b.name)); for (const e of entries) { - if (e.isDirectory() && !SKIP_DIRS.has(e.name)) await walk(join(dir, e.name), depth + 1); + if (e.isDirectory() && !SKIP_DIRS.has(e.name) && !e.name.startsWith(".")) await walk(join(dir, e.name), depth + 1); } } for (const d of courseDirs) await walk(resolve(d), 0); diff --git a/packages/course-format/test/load.test.ts b/packages/course-format/test/load.test.ts index 2a8c0d6..a1528a2 100644 --- a/packages/course-format/test/load.test.ts +++ b/packages/course-format/test/load.test.ts @@ -45,6 +45,13 @@ describe("discovery", () => { const found = (await discoverManifests([d])).map((p) => p.slice(d.length + 1)); expect(found.sort()).toEqual(["a/b/stcourse.yaml", "a/stcourse.yaml", "stcourse.yaml"]); }); + + test("skips dot-folders (hidden or retired course folders, e.g. an import being converted)", async () => { + const d = await tmp(); + for (const p of ["a/stcourse.yaml", ".converting-a/stcourse.yaml", "x/.hidden/stcourse.yaml"]) await Bun.write(join(d, p), "x"); + const found = (await discoverManifests([d])).map((p) => p.slice(d.length + 1)); + expect(found).toEqual(["a/stcourse.yaml"]); + }); }); describe("overlays and errors", () => { diff --git a/packages/feature-import/src/context.ts b/packages/feature-import/src/context.ts index d0eff37..307bd90 100644 --- a/packages/feature-import/src/context.ts +++ b/packages/feature-import/src/context.ts @@ -1,6 +1,7 @@ import { mkdir, readdir, rename, stat } from "node:fs/promises"; import { join, relative, resolve } from "node:path"; import type { ServerContext } from "@stc/contracts"; +import { withCourseLock } from "@stc/core"; import { readRawManifest, writeManifest, type RawManifest } from "./util/manifest"; import { slugify, uniqueId } from "./util/text"; import { extractFrame, type Tools } from "./media"; @@ -65,9 +66,21 @@ export async function publishCourse(d: Deps, stage: string, id: string): Promise export interface ManagedCourse { id: string; dir: string; manifest: RawManifest } +export const STUDIO_OWNED = "this course is now edited in the studio; add files there instead"; + +/** + * Runs importer work that changes an existing course under the course lock shared with the studio (core + * `withCourseLock`), after checking, inside the lock, that the course is still an importer folder. A conversion to a + * studio course therefore either waits for the job, or the job fails cleanly with STUDIO_OWNED and writes nothing. + */ +export function withManagedCourse(d: Deps, courseId: string | undefined, fn: (c: ManagedCourse) => Promise): Promise { + return withCourseLock(courseId ?? "", async () => fn(await managedCourse(d, courseId))); +} + /** An existing course the importer may add to. Only folders directly under /courses qualify: overlays and authored repos are never edited. */ export async function managedCourse(d: Deps, courseId: string | undefined): Promise { if (!courseId) throw new Error("options.courseId is required: choose the course to add to"); + if (isPlainId(courseId) && (await exists(studioRepo(d, courseId)))) throw new Error(STUDIO_OWNED); const dir = join(coursesDir(d), courseId); const rel = relative(coursesDir(d), dir); if (!isPlainId(courseId) || rel.startsWith("..") || !(await exists(join(dir, "stcourse.yaml")))) { diff --git a/packages/feature-import/src/jobs/code-repo.ts b/packages/feature-import/src/jobs/code-repo.ts index 29aff79..3566222 100644 --- a/packages/feature-import/src/jobs/code-repo.ts +++ b/packages/feature-import/src/jobs/code-repo.ts @@ -1,7 +1,7 @@ import { cp, lstat, mkdir, readdir } from "node:fs/promises"; import { basename, extname, join } from "node:path"; import type { ImportResult, JobContext } from "@stc/contracts"; -import { finish, managedCourse, saveManifest, stageDir, type Deps } from "../context"; +import { finish, saveManifest, stageDir, withManagedCourse, type Deps, type ManagedCourse } from "../context"; import { attachDeck } from "../deck"; import { putFile } from "../media"; import { run } from "../util/exec"; @@ -48,9 +48,13 @@ async function findPdfs(dir: string, depth = 0, out: string[] = []): Promise { - const { path, options } = parsePayload(payload, "code-repo"); + const { options } = parsePayload(payload, "code-repo"); + return withManagedCourse(d, optStr(options, "courseId"), (course) => importCodeRepoInto(d, course, payload, jc)); +} + +async function importCodeRepoInto(d: Deps, course: ManagedCourse, payload: unknown, jc: JobContext): Promise { + const { path } = parsePayload(payload, "code-repo"); const warnings: string[] = []; - const course = await managedCourse(d, optStr(options, "courseId")); if (!GIT_URL.test(path)) await assertAllowedPath(d, path); const stage = await stageDir(d, "repo"); diff --git a/packages/feature-import/src/jobs/pdf-deck.ts b/packages/feature-import/src/jobs/pdf-deck.ts index 855815a..0b96464 100644 --- a/packages/feature-import/src/jobs/pdf-deck.ts +++ b/packages/feature-import/src/jobs/pdf-deck.ts @@ -1,7 +1,7 @@ import { stat } from "node:fs/promises"; import { join } from "node:path"; import type { ImportResult, JobContext } from "@stc/contracts"; -import { finish, managedCourse, saveManifest, type Deps } from "../context"; +import { finish, saveManifest, withManagedCourse, type Deps } from "../context"; import { assertAllowedPath } from "../roots"; import { attachDeck } from "../deck"; import { optStr, parsePayload } from "./common"; @@ -11,7 +11,7 @@ export async function importPdfDeck(d: Deps, payload: unknown, jc: JobContext): const warnings: string[] = []; await assertAllowedPath(d, path); if (!(await stat(path).catch(() => null))?.isFile() || !/\.pdf$/i.test(path)) throw new Error(`not a PDF file: ${path}`); - const course = await managedCourse(d, optStr(options, "courseId")); + return withManagedCourse(d, optStr(options, "courseId"), async (course) => { const attachTo = optStr(options, "attachTo"); if (!attachTo) throw new Error("options.attachTo is required: the lesson or module id the deck belongs to"); await jc.reportProgress(0.05, "Copying the deck"); @@ -25,4 +25,5 @@ export async function importPdfDeck(d: Deps, payload: unknown, jc: JobContext): await finish(d, course.id, warnings); await jc.reportProgress(1, r.pages ? `Deck attached (${r.pages} slides)` : "Deck attached"); return { courseId: course.id, manifestPath: join(course.dir, "stcourse.yaml"), warnings }; + }); } diff --git a/packages/feature-import/src/jobs/transcribe.ts b/packages/feature-import/src/jobs/transcribe.ts index dfbafb4..38cccaa 100644 --- a/packages/feature-import/src/jobs/transcribe.ts +++ b/packages/feature-import/src/jobs/transcribe.ts @@ -1,7 +1,7 @@ import { mkdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; import type { JobContext } from "@stc/contracts"; -import { finish, managedCourse, saveManifest, stageDir, transcriptCacheDir, type Deps } from "../context"; +import { finish, saveManifest, stageDir, transcriptCacheDir, withManagedCourse, type Deps, type ManagedCourse } from "../context"; import { run } from "../util/exec"; import { optStr, withStage } from "./common"; @@ -36,7 +36,10 @@ export async function transcribeCourse(d: Deps, payload: unknown, jc: JobContext const url = d.transcribeUrl; const courseId = optStr(p, "courseId") ?? ""; if (!url) return { courseId, skipped: true, reason: "STC_TRANSCRIBE_URL is not set", transcribed: 0, warnings: [] }; - const course = await managedCourse(d, courseId); + return withManagedCourse(d, courseId, (course) => transcribeInto(d, course, url, p, jc)); +} + +async function transcribeInto(d: Deps, course: ManagedCourse, url: string, p: Record, jc: JobContext): Promise { const only = Array.isArray(p.lessonIds) ? new Set(p.lessonIds.map(String)) : null; const lessons = (course.manifest.modules as any[]).flatMap((m) => m.lessons as any[]).filter((l) => l.type === "video" && !l.transcript && (!only || only.has(l.id))); const warnings: string[] = []; diff --git a/packages/feature-import/src/routes.ts b/packages/feature-import/src/routes.ts index 01384eb..0e0cae4 100644 --- a/packages/feature-import/src/routes.ts +++ b/packages/feature-import/src/routes.ts @@ -3,7 +3,7 @@ import { join, relative, resolve } from "node:path"; import type { Feature, RouteHandler } from "@stc/contracts"; import { serveMedia } from "@stc/core"; import { assertAllowedPath, importRoots, OUTSIDE } from "./roots"; -import { coverCacheDir, coursesDir, exists, isPlainId, managedCourse, saveManifest, type Deps } from "./context"; +import { coverCacheDir, coursesDir, exists, isPlainId, saveManifest, STUDIO_OWNED, withManagedCourse, type Deps } from "./context"; export const KINDS = ["packt-video", "code-repo", "pdf-deck", "video-folder"] as const; const OPTION_KEYS = ["title", "courseId", "attachTo", "media", "transcribe"] as const; @@ -90,13 +90,18 @@ export function importRoutes(deps: () => Deps): Feature["routes"] { if (!isPlainId(rc.params.courseId!)) return json({ error: "Invalid course id" }, 400); const src = join(coverCacheDir(d, rc.params.courseId!), name); if (!(await exists(src))) return json({ error: "No such candidate" }, 404); - let course; - try { course = await managedCourse(d, rc.params.courseId); } catch (e) { return json({ error: (e as Error).message }, 404); } - await mkdir(join(course.dir, "media"), { recursive: true }); - await copyFile(src, join(course.dir, "media", "cover.jpg")); - course.manifest.cover = "media/cover.jpg"; - await saveManifest(course.dir, course.manifest); - await d.ctx.courses.reload(course.id); + try { + await withManagedCourse(d, rc.params.courseId, async (course) => { + await mkdir(join(course.dir, "media"), { recursive: true }); + await copyFile(src, join(course.dir, "media", "cover.jpg")); + course.manifest.cover = "media/cover.jpg"; + await saveManifest(course.dir, course.manifest); + await d.ctx.courses.reload(course.id); + }); + } catch (e) { + const msg = (e as Error).message; + return json({ error: msg }, msg === STUDIO_OWNED ? 409 : 404); + } return json({ cover: "media/cover.jpg" }); }), }, diff --git a/packages/feature-import/test/studio-race.test.ts b/packages/feature-import/test/studio-race.test.ts new file mode 100644 index 0000000..4c7e6f7 --- /dev/null +++ b/packages/feature-import/test/studio-race.test.ts @@ -0,0 +1,56 @@ +// v1.1 S review fix: importer jobs and "Make editable in the studio" share the course lock. Either order is safe: +// a job that started first finishes and its files are converted with the course; a job that comes second fails +// cleanly and never recreates /courses/. +import { expect, test } from "bun:test"; +import { existsSync } from "node:fs"; +import { join } from "node:path"; +import { $ } from "bun"; +import { loadAll } from "@stc/course-format"; +import { createStudioStore } from "@stc/feature-studio"; +import { STUDIO_OWNED } from "../src/context"; +import { importCodeRepo } from "../src/jobs/code-repo"; +import { importPacktVideo } from "../src/jobs/packt-video"; +import { transcribeCourse } from "../src/jobs/transcribe"; +import { makeEnv } from "./harness"; +import { makePacktZip, makeRepoZip, tmp } from "./fixtures"; + +const owner = { name: "Olive Owner", email: "own@example.org" }; + +async function imported(env: Record = {}) { + const e = await makeEnv(env); + const r = await importPacktVideo(e.deps, { path: await makePacktZip(await tmp()), options: { title: "Race Test" } }, e.jc); + const store = createStudioStore({ dataDir: e.dataDir }); + const count = async () => (await loadAll([join(e.dataDir, "courses"), join(e.dataDir, "worktrees")])).filter((c) => c.id === r.courseId).length; + return { e, id: r.courseId, store, count }; +} + +test("an importer job that started first finishes; the conversion waits and commits its files", async () => { + const { e, id, store, count } = await imported(); + const zip = await makeRepoZip(await tmp()); + const job = importCodeRepo(e.deps, { kind: "code-repo", path: zip, options: { courseId: id } }, e.jc); + const conv = store.convertImported(id, owner); + const [res] = await Promise.all([job, conv]); + expect(res.courseId).toBe(id); + const pub = store.paths(id).published; + const files = (await $`git -c core.quotepath=false ls-files`.cwd(pub).quiet()).stdout.toString(); + expect(files).toContain("code/S02 - Images (Sample)/app/main.py"); + expect(await Bun.file(join(pub, "stcourse.yaml")).text()).toContain("code/S02 - Images (Sample)"); + expect(existsSync(join(e.dataDir, "courses", id))).toBe(false); + expect(await count()).toBe(1); +}); + +test("an importer job that comes after the conversion fails cleanly and writes nothing", async () => { + const { e, id, store, count } = await imported({ STC_TRANSCRIBE_URL: "http://127.0.0.1:9/never" }); + const zip = await makeRepoZip(await tmp()); + const conv = store.convertImported(id, owner); + const job = importCodeRepo(e.deps, { kind: "code-repo", path: zip, options: { courseId: id } }, e.jc); + const tr = transcribeCourse(e.deps, { courseId: id }, e.jc); + const [c, j, t] = await Promise.allSettled([conv, job, tr]); + expect(c.status).toBe("fulfilled"); + expect(j.status === "rejected" && (j.reason as Error).message).toBe(STUDIO_OWNED); + expect(t.status === "rejected" && (t.reason as Error).message).toBe(STUDIO_OWNED); + expect(existsSync(join(e.dataDir, "courses", id))).toBe(false); + expect(await count()).toBe(1); + // and later ones too + await expect(importCodeRepo(e.deps, { kind: "code-repo", path: zip, options: { courseId: id } }, e.jc)).rejects.toThrow(STUDIO_OWNED); +}); diff --git a/packages/feature-studio/src/server/convert.ts b/packages/feature-studio/src/server/convert.ts index 6e384ef..8e07f0b 100644 --- a/packages/feature-studio/src/server/convert.ts +++ b/packages/feature-studio/src/server/convert.ts @@ -1,4 +1,5 @@ -import { copyFile, link, mkdir, readdir, rename, rm, cp, stat } from "node:fs/promises"; +import { constants } from "node:fs"; +import { copyFile, link, mkdir, open, readdir, stat } from "node:fs/promises"; import { dirname, join } from "node:path"; import { $ } from "bun"; import { gitignoreLine, isLargeMedia } from "./media-refs"; @@ -43,22 +44,34 @@ export function unreferencable(large: { path: string }[]): string[] { return bad; } -/** A hard link (no extra space), else a reflink or plain copy (another filesystem). The source is only read. */ -export async function linkOrCopy(src: string, dest: string): Promise { - await mkdir(dirname(dest), { recursive: true }); - try { await link(src, dest); return; } catch { /* cross-device or unsupported */ } - const r = await $`cp --reflink=auto -- ${src} ${dest}`.quiet().nothrow(); - if (r.exitCode !== 0) await copyFile(src, dest); +/** Test-only marker for a simulated crash: cleanup code must not run for it (a real crash would not run it either). */ +export class SimulatedCrash extends Error { + constructor(public step: string) { super(`simulated crash at ${step}`); } } -/** Moves the import folder into the media store; across filesystems it copies (keeping links) and then removes. */ -export async function moveDir(src: string, dest: string): Promise { +/** A hard link (no extra space), else a reflink or plain copy when linking is impossible (another filesystem, no + * permission, too many links). Other errors (the destination exists, the source vanished) are real failures. */ +export async function linkOrCopy(src: string, dest: string): Promise { await mkdir(dirname(dest), { recursive: true }); try { - await rename(src, dest); + await link(src, dest); + return; } catch (e) { - if ((e as NodeJS.ErrnoException).code !== "EXDEV") throw e; - await cp(src, dest, { recursive: true, verbatimSymlinks: true, errorOnExist: true, force: false }); - await rm(src, { recursive: true, force: true }); + if (!["EXDEV", "EPERM", "EMLINK", "ENOTSUP", "EOPNOTSUPP"].includes((e as NodeJS.ErrnoException).code ?? "")) throw e; } + const r = await $`cp --reflink=auto -- ${src} ${dest}`.quiet().nothrow(); + if (r.exitCode !== 0) await copyFile(src, dest, constants.COPYFILE_EXCL); +} + +/** fsyncs every file and directory under `root` (or only `root` itself with `shallow`), so a rename after it is durable. */ +export async function fsyncTree(root: string, o: { shallow?: boolean } = {}): Promise { + const sync = async (p: string) => { const fh = await open(p, "r"); try { await fh.sync(); } finally { await fh.close(); } }; + if (!o.shallow) { + for (const e of await readdir(root, { withFileTypes: true })) { + const p = join(root, e.name); + if (e.isDirectory()) await fsyncTree(p); + else if (e.isFile()) await sync(p); + } + } + await sync(root); } diff --git a/packages/feature-studio/src/server/feature.ts b/packages/feature-studio/src/server/feature.ts index 49a3185..46f430d 100644 --- a/packages/feature-studio/src/server/feature.ts +++ b/packages/feature-studio/src/server/feature.ts @@ -49,6 +49,8 @@ export interface StudioFeature extends Feature { store: StudioStore; watcher: PublishedWatcher; draftWriter: DraftWriter; + /** Resolves once interrupted conversions are recovered (every studio route waits for it). */ + ready: Promise; stop(): void; } @@ -108,10 +110,21 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = { /** v1.1 S: only the owner may turn an imported course into a studio course ("Make editable in the studio"). */ const convertible = (u: AuthUser, courseId: string) => isOwner(u) && store.isConvertibleImport(courseId); + /** Importer jobs (queued or running) that would write into this course's import folder. */ + const pendingImports = async (courseId: string) => + (await ctx.jobs.list()).filter((j) => { + if (!j.kind.startsWith("import.") || (j.status !== "queued" && j.status !== "running")) return false; + const p = (j.payload ?? {}) as { courseId?: unknown; options?: { courseId?: unknown } }; + return p.courseId === courseId || p.options?.courseId === courseId; + }); + const IMPORT_RUNNING = "an import job for this course is still running; make it editable once it has finished"; + /** The conversion job: payload `{ courseId, actor }`, checked again by the store under the course lock. */ const convertJob: JobHandler = async (payload, jc) => { + await ready; const p = (payload ?? {}) as { courseId?: unknown; actor?: { id?: unknown; name?: unknown; email?: unknown } }; const courseId = typeof p.courseId === "string" ? p.courseId : ""; + if ((await pendingImports(courseId)).length) throw new StudioError(IMPORT_RUNNING, 409); const actor = { name: String(p.actor?.name ?? "STCourses"), email: String(p.actor?.email ?? "studio@stcourses.invalid") }; const r = await store.convertImported(courseId, actor, { onProgress: (f, m) => jc.reportProgress(f, m) }); await afterPublishedChange(courseId); @@ -123,11 +136,17 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = { watcher.note(courseId, await store.publishedHead(courseId)); }; + // Crash recovery for conversions interrupted by a stop (v1.1 S); learners never see a course twice meanwhile. + const ready: Promise = store.recoverConversions() + .then(async (ids) => { if (ids.length) await ctx.courses.reload(); }) + .catch((e) => console.error("[studio] recovery", e)); + /** Wraps a handler: signed-in author/owner (or per-course author), error mapping. */ const guard = (h: (req: Request, rc: RequestContext, user: AuthUser) => Promise): RouteHandler => async (req, rc) => { const user = rc.user; if (!user) return fail(401, "sign in to use the studio"); try { + await ready; if (!hasRole(user) && (await authoredCourses(user)).length === 0) return fail(403, "the studio is for authors"); return await h(req, rc, user); } catch (e) { @@ -245,6 +264,7 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = { } const pending = (await ctx.jobs.list(CONVERT_JOB)).find((j) => (j.payload as { courseId?: string } | null)?.courseId === courseId && (j.status === "queued" || j.status === "running")); if (pending) return fail(409, "this course is already being made editable"); + if ((await pendingImports(courseId)).length) return fail(409, IMPORT_RUNNING); const jobId = await ctx.jobs.enqueue(CONVERT_JOB, { courseId, actor: { id: user.id, name: who(user).name, email: who(user).email } }); return json({ jobId }, 202); }), @@ -405,5 +425,5 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = { watcher.start(); } - return { name: "studio", routes, jobs: { [CONVERT_JOB]: convertJob }, multipartRoutes: ["/api/studio/courses/:courseId/upload"], store, watcher, draftWriter, stop: () => watcher.stop() }; + return { name: "studio", routes, jobs: { [CONVERT_JOB]: convertJob }, multipartRoutes: ["/api/studio/courses/:courseId/upload"], store, watcher, draftWriter, ready, stop: () => watcher.stop() }; } diff --git a/packages/feature-studio/src/server/media-refs.ts b/packages/feature-studio/src/server/media-refs.ts index 12ba06f..81c048f 100644 --- a/packages/feature-studio/src/server/media-refs.ts +++ b/packages/feature-studio/src/server/media-refs.ts @@ -3,8 +3,8 @@ import { checkRelPath } from "./relpath"; /** * Large media of a converted import stay out of git (v1.1 S, docs/operations.md "Imported courses in the studio"). - * The repo commits `MEDIA_REFS` (path, size and checksum per file, spec 5) and a `.gitignore` line per path; - * every studio worktree gets the files as hard links to `/media//`. + * The repo commits `MEDIA_REFS` (path, size and checksum per file, spec 5); the repo's `info/exclude` (not the + * author-editable `.gitignore`) gets one line per path, and the studio never counts them as changes. Every studio worktree gets the files as hard links to `/media//`. */ export const MEDIA_REFS = ".stcourses/media.json"; export const LARGE_FILE_BYTES = 8 * 1024 * 1024; @@ -26,12 +26,12 @@ export function gitignoreLine(rel: string): string { return `/${s}`; } -const HEADER = "# Imported media kept outside git: see .stcourses/media.json (STCourses)"; +export const MEDIA_EXCLUDE_HEADER = "# Imported media kept outside git: see .stcourses/media.json (STCourses)"; -/** The repo's root `.gitignore`: the course's own one (if any) first, then one line per referenced file. */ +/** Ignore-file content: `existing` (if any) first, then the header and one line per referenced file. */ export function mediaGitignore(paths: string[], existing: string | null): string { const own = existing ? (existing.endsWith("\n") ? existing : existing + "\n") + "\n" : ""; - return `${own}${HEADER}\n${paths.map(gitignoreLine).join("\n")}\n`; + return `${own}${MEDIA_EXCLUDE_HEADER}\n${paths.map(gitignoreLine).join("\n")}\n`; } export async function sha256File(abs: string): Promise { diff --git a/packages/feature-studio/src/server/store.ts b/packages/feature-studio/src/server/store.ts index 54be0a5..8ad3e65 100644 --- a/packages/feature-studio/src/server/store.ts +++ b/packages/feature-studio/src/server/store.ts @@ -1,10 +1,11 @@ import { constants, existsSync, lstatSync, mkdirSync, readdirSync, readFileSync, realpathSync, rmSync, statSync } from "node:fs"; -import { copyFile, mkdir, open, rm, unlink, writeFile } from "node:fs/promises"; +import { cp, copyFile, mkdir, open, readFile, rename as fsRename, rm, unlink, writeFile } from "node:fs/promises"; import { dirname, join, sep } from "node:path"; import { loadCourse, parseManifest, type Diagnostic } from "@stc/course-format"; -import { linkOrCopy, moveDir, scanImport, unreferencable } from "./convert"; -import { git, gitOk, splitZ, withLock, type Identity } from "./git"; -import { MEDIA_REFS, mediaGitignore, parseMediaRefs, sha256File, stringifyMediaRefs, type MediaRef } from "./media-refs"; +import { withCourseLock } from "@stc/core"; +import { fsyncTree, linkOrCopy, scanImport, SimulatedCrash, unreferencable } from "./convert"; +import { git, gitOk, splitZ, type Identity } from "./git"; +import { gitignoreLine, MEDIA_EXCLUDE_HEADER, MEDIA_REFS, parseMediaRefs, sha256File, stringifyMediaRefs, type MediaRef } from "./media-refs"; import { checkRelPath } from "./relpath"; export { checkRelPath } from "./relpath"; @@ -137,11 +138,16 @@ export interface StudioStoreOptions { importedDir?: string; /** AppConfig.studioWorktreesDir (default `/worktrees`). */ worktreesDir?: string; - /** Media stores of converted imports (default `/media`): `/` is the former import folder. */ + /** + * Media stores of converted imports (default `/media`): `/` is the former import folder. + * Not in AppConfig: production always uses the default; tests may point it elsewhere. + */ mediaDir?: string; publishedBranch?: string; /** Tests only: allow local-path remotes (the HTTP feature never sets this). */ allowLocalRemotes?: boolean; + /** Tests only: simulate a crash at a named conversion step, or a cross-filesystem media move. */ + testHooks?: { crashAt?: string; forceExdev?: boolean }; } export function createStudioStore(opts: StudioStoreOptions) { @@ -178,14 +184,27 @@ export function createStudioStore(opts: StudioStoreOptions) { if (!isManaged(courseId)) throw new StudioError(`course ${courseId} is not managed by the studio`, 404); return paths(courseId); }; - const lock = (courseId: string, fn: () => Promise) => withLock(paths(courseId).bare, fn); + /** The process-wide course lock shared with the importer's jobs (core `withCourseLock`). */ + const lock = (courseId: string, fn: () => Promise) => { paths(courseId); return withCourseLock(courseId, fn); }; + const convertingDir = (courseId: string) => join(importedDir, `.converting-${courseId}`); + const listDir = (dir: string): string[] => { try { return readdirSync(dir); } catch { return []; } }; + const crash = async (step: string) => { if (opts.testHooks?.crashAt === step) throw new SimulatedCrash(step); }; + const rename = async (from: string, to: string) => fsRename(from, to); const rev = async (cwd: string, ref: string) => (await gitOk(cwd, ["rev-parse", ref])).trim(); const count = async (cwd: string, range: string) => Number((await gitOk(cwd, ["rev-list", "--count", range])).trim()); const unmerged = async (cwd: string) => [...new Set(splitZ(await gitOk(cwd, ["diff", "--name-only", "-z", "--diff-filter=U"])))]; const isMerging = async (cwd: string) => (await git(cwd, ["rev-parse", "-q", "--verify", "MERGE_HEAD"])).code === 0; - const dirtyFiles = async (cwd: string) => - splitZ(await gitOk(cwd, ["status", "--porcelain", "-z", "--untracked-files=all"])).map((l) => l.slice(3)).filter(Boolean); + /** + * Uncommitted changes, except untracked referenced media: whatever a `.gitignore` (editable by authors and remotes) + * says, the files listed in MEDIA_REFS never make a worktree dirty. + */ + const dirtyFiles = async (cwd: string) => { + const media = new Set((await mediaRefsIn(cwd)).map((r) => r.path)); + return splitZ(await gitOk(cwd, ["status", "--porcelain", "-z", "--untracked-files=all"])) + .filter((l) => !(l.startsWith("?? ") && media.has(l.slice(3)))) + .map((l) => l.slice(3)).filter(Boolean); + }; const remoteUrl = async (bare: string) => { const r = await git(bare, ["remote", "get-url", "origin"]); return r.code === 0 ? r.stdout.trim() : null; @@ -254,7 +273,7 @@ export function createStudioStore(opts: StudioStoreOptions) { const branch = branchOf(authorId); const exists = (await git(p.bare, ["rev-parse", "-q", "--verify", `refs/heads/${branch}`])).code === 0; await gitOk(p.bare, exists ? ["worktree", "add", dir, branch] : ["worktree", "add", "-b", branch, dir, main]); - await materializeMedia(dir, p.media); + await materializeMedia(dir, p.media, p.bare); return dir; } @@ -269,8 +288,9 @@ export function createStudioStore(opts: StudioStoreOptions) { * Idempotent; existing destinations are left alone. A reference must name a regular file whose real path is inside * `fromRoot`, and the destination path may not contain a symlink. Returns the references it could not satisfy. */ - async function materializeMedia(dir: string, fromRoot: string): Promise<{ linked: number; missing: string[] }> { + async function materializeMedia(dir: string, fromRoot: string, bare: string): Promise<{ linked: number; missing: string[] }> { const refs = await mediaRefsIn(dir); + if (refs.length) await writeExclude(bare, refs.map((r) => r.path)); const missing: string[] = []; let linked = 0; let realFrom: string; @@ -289,9 +309,139 @@ export function createStudioStore(opts: StudioStoreOptions) { missing.push(r.path); } } + if (missing.length) console.warn(`[studio] media missing for ${dir}: ${missing.join(", ")}`); return { linked, missing }; } + /** + * Keeps referenced media out of `git status` through the repo's `info/exclude` (shared by every worktree, not + * reachable by authors or remotes, unlike `.gitignore`). Lines are only ever added. + */ + async function writeExclude(bare: string, paths: string[]): Promise { + const file = join(bare, "info", "exclude"); + const cur = await readFile(file, "utf8").catch(() => ""); + const lines = new Set(cur.split("\n")); + const add: string[] = []; + for (const x of paths) { + let line: string; + try { line = gitignoreLine(x); } catch { continue; } + if (!lines.has(line)) { add.push(line); lines.add(line); } + } + if (!add.length) return; + const head = cur.includes(MEDIA_EXCLUDE_HEADER) ? "" : `${MEDIA_EXCLUDE_HEADER}\n`; + await mkdir(dirname(file), { recursive: true }); + await writeFile(file, `${cur}${cur && !cur.endsWith("\n") ? "\n" : ""}${head}${add.join("\n")}\n`); + } + + /** Every reference present under `root` with its size (and, when asked, its sha256). */ + async function mediaComplete(root: string, refs: MediaRef[], hash: boolean): Promise { + for (const r of refs) { + try { + const st = lstatSync(join(root, r.path)); + if (!st.isFile() || st.size !== r.size) return false; + if (hash && (await sha256File(join(root, r.path))) !== r.sha256) return false; + } catch { + return false; + } + } + return true; + } + + /** Renames the old import folder aside (atomic, never discovered: dot-folder) and deletes it. */ + async function retireSource(courseId: string, from: string): Promise { + const gone = join(importedDir, `.converted-${courseId}.${Date.now()}`); + await rename(from, gone); + await crash("source-retired"); + await rm(gone, { recursive: true, force: true }); + } + + /** + * Makes `from` (the hidden import folder) the media store `/`. Same filesystem: one rename. + * Across filesystems: copy into a staging dir, fsync, verify against MEDIA_REFS, rename into place, then retire the + * source. An existing store that matches MEDIA_REFS means an earlier attempt got that far: only the source is retired. + */ + async function finishMove(courseId: string, from: string): Promise { + const p = paths(courseId); + const refs = await mediaRefsIn(p.published); + if (lexists(p.media)) { + if (!(await mediaComplete(p.media, refs, true))) throw new StudioError(`the media store ${p.media} exists but does not match ${MEDIA_REFS}; move it away and retry`, 409); + await retireSource(courseId, from); + return; + } + await crash("before-move"); + await mkdir(mediaDir, { recursive: true }); + try { + if (opts.testHooks?.forceExdev) throw Object.assign(new Error("EXDEV (test)"), { code: "EXDEV" }); + await rename(from, p.media); + return; + } catch (e) { + if ((e as NodeJS.ErrnoException).code !== "EXDEV") throw e; + } + const staging = join(mediaDir, `.staging-${courseId}.${Bun.randomUUIDv7()}`); + try { + await cp(from, staging, { recursive: true, verbatimSymlinks: true, errorOnExist: true, force: false }); + await crash("staging-copied"); + await fsyncTree(staging); + if (!(await mediaComplete(staging, refs, true))) throw new Error(`the copied media in ${staging} do not match ${MEDIA_REFS}`); + await rename(staging, p.media); + await fsyncTree(mediaDir, { shallow: true }); + } catch (e) { + if (!(e instanceof SimulatedCrash)) await rm(staging, { recursive: true, force: true }); + throw e; + } + await crash("staging-placed"); + await retireSource(courseId, from); + } + + /** + * Deterministic recovery of one course's conversion, under its lock: + * - staging copies are deleted (a move is always redone from the source); + * - a hidden import folder without a finished repo is restored and the partial repo removed (the next attempt + * starts over); with a finished repo, the published worktree is reset to its commit and the move finished; + * - retired sources are deleted once the media store matches MEDIA_REFS. + */ + async function recoverUnlocked(courseId: string): Promise { + const p = paths(courseId); + for (const n of listDir(mediaDir)) if (n.startsWith(`.staging-${courseId}.`)) await rm(join(mediaDir, n), { recursive: true, force: true }); + const conv = convertingDir(courseId); + if (lexists(conv)) { + if (!isManaged(courseId)) { + rmSync(p.bare, { recursive: true, force: true }); + rmSync(p.root, { recursive: true, force: true }); + if (lexists(p.imported)) throw new StudioError(`both ${conv} and ${p.imported} exist; remove one of them by hand`, 409); + await rename(conv, p.imported); + } else { + await gitOk(p.published, ["reset", "-q", "--hard", "HEAD"]); + await finishMove(courseId, conv); + await materializeMedia(p.published, p.media, p.bare); + } + } + const retired = listDir(importedDir).filter((n) => n.startsWith(`.converted-${courseId}.`)); + if (retired.length) { + const refs = isManaged(courseId) ? await mediaRefsIn(p.published) : null; + if (refs && lexists(p.media) && (await mediaComplete(p.media, refs, false))) { + for (const n of retired) await rm(join(importedDir, n), { recursive: true, force: true }); + } else { + console.warn(`[studio] ${retired.join(", ")} kept: the media store of ${courseId} is not complete`); + } + } + } + + /** + * Referenced media paths that `treeish` (a draft branch, the remote's branch) tracks in git. Merging those would + * silently replace the media-store file with the commit's content, so publish and Sync refuse them. + */ + async function mediaClash(bare: string, treeish: string): Promise { + const refs = new Set(); + for (const t of [main, treeish]) { + const text = await showText(bare, `${t}:${MEDIA_REFS}`); + for (const r of (text && parseMediaRefs(text)) || []) refs.add(r.path); + } + if (!refs.size) return []; + const r = await git(bare, ["--literal-pathspecs", "ls-tree", "-r", "-z", "--name-only", treeish, "--", ...refs]); + return r.code === 0 ? splitZ(r.stdout) : []; + } + /** Referenced media and the reference file are not editable in the studio (they are not in git). */ async function refuseMediaWrites(dir: string, paths: string[]): Promise { const locked = new Set([MEDIA_REFS, ...(await mediaRefsIn(dir)).map((r) => r.path)]); @@ -398,35 +548,63 @@ export function createStudioStore(opts: StudioStoreOptions) { /** Re-links referenced media into the published worktree (after an outside checkout removed them, say). */ repairMedia: (courseId: string) => lock(courseId, async () => { const p = requireManaged(courseId); - return materializeMedia(p.published, p.media); + return materializeMedia(p.published, p.media, p.bare); }), + /** + * Finishes or undoes every interrupted conversion (server start; `convertImported` also does it for its course). + * Returns the ids it touched. Never throws: a course it cannot repair is logged and left for the owner. + */ + async recoverConversions(): Promise { + const ids = new Set(); + const scan = (dir: string, re: RegExp) => { for (const n of listDir(dir)) { const m = re.exec(n); if (m && ID_RE.test(m[1]!)) ids.add(m[1]!); } }; + scan(mediaDir, /^\.staging-([^.]+)\./); + scan(importedDir, /^\.converting-([^.]+)$/); + scan(importedDir, /^\.converted-([^.]+)\./); + const done: string[] = []; + for (const id of [...ids].sort()) { + try { await lock(id, () => recoverUnlocked(id)); done.push(id); } catch (e) { console.error(`[studio] could not recover the conversion of ${id}:`, e); } + } + return done; + }, + /** * Turns the imported course `/` into a studio course with the same id (v1.1 S, ruling R9): * small files are committed as they are (manifest unchanged: visibility and source.kind stay), large media are - * referenced from MEDIA_REFS and hard-linked into each worktree, and the import folder moves to `/`. - * Owner-only at the HTTP layer. Restart-safe: leftovers of an attempt that never produced a published worktree are - * removed and the conversion redone; a converted course whose import folder is still there only finishes the move. + * referenced from MEDIA_REFS and hard-linked into each worktree, and the import folder ends up as `/`. + * Owner-only at the HTTP layer; holds the shared course lock, so importer jobs on the course wait or fail cleanly. + * + * Crash safety: the import folder is hidden (`/.converting-`, never discovered) before the + * published worktree appears, so the id is never loaded twice. Until the worktree is checked out, a failure undoes + * everything; after that the conversion is committed and `recoverUnlocked` (run first by every attempt and at + * server start) finishes the move. Across filesystems the media are copied into `/.staging-.`, + * fsynced, verified against MEDIA_REFS (size and sha256) and only then renamed into place. */ convertImported(courseId: string, who: Identity, opts: { onProgress?: ConvertProgress } = {}): Promise { if (!ID_RE.test(courseId)) return Promise.reject(new StudioError(`invalid course id: ${courseId}`, 404)); const progress = opts.onProgress ?? (() => {}); return lock(courseId, async () => { const p = paths(courseId); - const srcManifest = join(p.imported, MANIFEST); - const hasImport = lexists(srcManifest); + await recoverUnlocked(courseId); if (isManaged(courseId)) { - const refs = await readFileIn(p.published, MEDIA_REFS); - if (!hasImport || !refs) throw new StudioError(`course "${courseId}" is already a studio course`, 409); - if (lexists(p.media)) throw new StudioError(`the media store ${p.media} already exists`, 409); - await moveDir(p.imported, p.media); - await materializeMedia(p.published, p.media); - return { commit: await rev(p.published, "HEAD"), media: (parseMediaRefs(refs.content) ?? []).length, files: 0, skipped: [], resumed: true }; + const refsFile = await readFileIn(p.published, MEDIA_REFS); + const media = refsFile ? (parseMediaRefs(refsFile.content) ?? []).length : 0; + const head = async () => rev(p.published, "HEAD"); + // already converted (a job re-run after a restart, say): report success + if (refsFile && lexists(p.media) && !lexists(p.imported)) return { commit: await head(), media, files: 0, skipped: [], resumed: true }; + // a conversion made before the import folder was hidden first: finish its move + if (refsFile && lexists(p.imported)) { + await finishMove(courseId, p.imported); + await materializeMedia(p.published, p.media, p.bare); + return { commit: await head(), media, files: 0, skipped: [], resumed: true }; + } + throw new StudioError(`course "${courseId}" is already a studio course`, 409); } - if (!hasImport) throw new StudioError(`there is no imported course "${courseId}" in ${importedDir}`, 404); + const srcManifest = join(p.imported, MANIFEST); + if (!lexists(srcManifest)) throw new StudioError(`there is no imported course "${courseId}" in ${importedDir}`, 404); const top = lstatSync(p.imported); if (top.isSymbolicLink() || !top.isDirectory() || lstatSync(srcManifest).isSymbolicLink()) throw new StudioError("the import folder and its manifest must not be symlinks", 400); - if (lexists(p.media)) throw new StudioError(`the media store ${p.media} already exists`, 409); + if (lexists(p.media)) throw new StudioError(`the media store ${p.media} already exists; move it away and retry`, 409); const parsed = parseManifest(await Bun.file(srcManifest).text()); if (!parsed.ok) throw new StudioError(`the imported manifest is invalid: ${parsed.errors.join("; ")}`, 400); const m = parsed.manifest; @@ -452,50 +630,58 @@ export function createStudioStore(opts: StudioStoreOptions) { done += f.size; } + const conv = convertingDir(courseId); const work = join(dataDir, "tmp", `convert-${courseId}-${Bun.randomUUIDv7()}`); const stage = join(work, "tree"); + const list = [...new Set([...scan.small, MEDIA_REFS])]; + let hidden = false; try { await progress(0.82, "Committing the course files"); for (const f of scan.small) { await mkdir(dirname(join(stage, f)), { recursive: true }); await copyFile(join(p.imported, f), join(stage, f)); } - const ownIgnore = scan.small.includes(".gitignore") ? await Bun.file(join(stage, ".gitignore")).text() : null; await mkdir(join(stage, dirname(MEDIA_REFS)), { recursive: true }); await writeFile(join(stage, MEDIA_REFS), stringifyMediaRefs(refs)); - await writeFile(join(stage, ".gitignore"), mediaGitignore(refs.map((r) => r.path), ownIgnore)); - const list = [...new Set([...scan.small, ".gitignore", MEDIA_REFS])]; - await writeFile(join(work, "paths"), list.join("") + ""); + await writeFile(join(work, "paths"), list.join("\0") + "\0"); mkdirSync(reposDir, { recursive: true }); await gitOk(dataDir, ["init", "-q", "--bare", "-b", main, p.bare]); + await writeExclude(p.bare, refs.map((r) => r.path)); const gd = ["--literal-pathspecs", "--git-dir", p.bare, "--work-tree", stage]; // -f: a nested .gitignore from the course's own code must not drop course files await gitOk(stage, [...gd, "add", "-f", `--pathspec-from-file=${join(work, "paths")}`, "--pathspec-file-nul"]); - await gitOk(stage, [...gd, "commit", "-q", "-m", `Make ${courseId} editable in the studio + await gitOk(stage, [...gd, "commit", "-q", "-m", `Make ${courseId} editable in the studio\n\nConverted from the import in ${p.imported}. ${refs.length} large media files stay outside git (${MEDIA_REFS}).`], who); + await crash("committed"); -Converted from the import in ${p.imported}. ${refs.length} large media files stay outside git (${MEDIA_REFS}).`], who); + // Hide the import folder before the published worktree exists: the id is never discoverable twice. + await rename(p.imported, conv); + hidden = true; + await crash("hidden"); mkdirSync(p.root, { recursive: true }); await gitOk(p.bare, ["worktree", "add", p.published, main]); - - await progress(0.92, "Linking media into the published course"); - const mat = await materializeMedia(p.published, p.imported); + await progress(0.9, "Linking media into the published course"); + const mat = await materializeMedia(p.published, conv, p.bare); if (mat.missing.length) throw new Error(`could not link media: ${mat.missing.join(", ")}`); const dirty = await dirtyFiles(p.published); if (dirty.length) throw new Error(`the converted course is not clean after checkout: ${dirty.join(", ")}`); - - await progress(0.97, "Moving the import folder to the media store"); - await moveDir(p.imported, p.media); - return { commit: await rev(p.published, "HEAD"), media: refs.length, files: list.length, skipped: scan.skipped, resumed: false }; + await crash("worktree"); } catch (e) { - if (lexists(p.imported)) { + if (!(e instanceof SimulatedCrash)) { rmSync(p.bare, { recursive: true, force: true }); rmSync(p.root, { recursive: true, force: true }); + if (hidden && !lexists(p.imported)) await rename(conv, p.imported); } throw e; } finally { await rm(work, { recursive: true, force: true }); } + + // Committed: from here on a failure leaves a state that the next attempt (or a restart) finishes. + await progress(0.95, "Moving the import folder to the media store"); + await finishMove(courseId, conv); + await materializeMedia(p.published, p.media, p.bare); + return { commit: await rev(p.published, "HEAD"), media: refs.length, files: list.length, skipped: scan.skipped, resumed: false }; }); }, @@ -510,6 +696,8 @@ Converted from the import in ${p.imported}. ${refs.length} large media files sta async readFile(courseId: string, authorId: string, rel: string) { const dir = await api.ensureDraft(courseId, authorId); + // referenced media can be gigabytes and are not text: never read them whole (the raw route streams them) + if ((await mediaRefsIn(dir)).some((r) => r.path === rel)) return null; return readFileIn(dir, rel); }, @@ -597,6 +785,8 @@ Converted from the import in ${p.imported}. ${refs.length} large media files sta const diags = (await api.validateDraftUnlocked(courseId, dir)).filter((d) => d.level === "error"); if (diags.length) return { ok: false, reason: "invalid", diagnostics: diags }; if ((await count(p.bare, `${main}..${branch}`)) === 0) return { ok: true, commit: await rev(p.published, "HEAD"), published: false }; + const clash = await mediaClash(p.bare, branch); + if (clash.length) return { ok: false, reason: "invalid", diagnostics: clash.map((path) => ({ level: "error" as const, courseId, path, message: `${path} is imported media kept outside git; the draft must not commit a file there` })) }; const baseManifest = await showText(p.published, `HEAD:${MANIFEST}`); // Merge without committing, so the merged result is checked before it becomes the published version. const r = await git(p.published, ["merge", "--no-ff", "--no-commit", branch], who); @@ -618,7 +808,7 @@ Converted from the import in ${p.imported}. ${refs.length} large media files sta return { ok: false, reason: "invalid", diagnostics: mergedErrors }; } await gitOk(p.published, ["commit", "-q", "-m", `Publish ${branch}`], who); - await materializeMedia(p.published, p.media); + await materializeMedia(p.published, p.media, p.bare); // Bring the draft level with what was just published, so "behind" stays meaningful. await git(dir, ["merge", "-q", "--ff-only", main]); return { ok: true, commit: await rev(p.published, "HEAD"), published: true }; @@ -742,6 +932,8 @@ Converted from the import in ${p.imported}. ${refs.length} large media files sta const local = await rev(p.bare, main); const remote = await rev(p.bare, remoteRef); if (local !== remote) { + const clash = await mediaClash(p.bare, remoteRef); + if (clash.length) return { ok: false, reason: "forbidden", message: `the remote commits files over imported media kept outside git: ${clash.join(", ")}` }; const localBehind = (await git(p.bare, ["merge-base", "--is-ancestor", local, remote])).code === 0; const remoteBehind = (await git(p.bare, ["merge-base", "--is-ancestor", remote, local])).code === 0; const baseManifest = await showText(p.bare, `${main}:${MANIFEST}`); diff --git a/packages/feature-studio/test/convert-routes.test.ts b/packages/feature-studio/test/convert-routes.test.ts index 2f8f87a..ad958d8 100644 --- a/packages/feature-studio/test/convert-routes.test.ts +++ b/packages/feature-studio/test/convert-routes.test.ts @@ -1,6 +1,9 @@ import { afterEach, describe, expect, test } from "bun:test"; import { existsSync, readFileSync } from "node:fs"; import { join, resolve } from "node:path"; +import { studioFeature } from "../src/server/feature"; +import { SimulatedCrash } from "../src/server/convert"; +import { createStudioStore } from "../src/server/store"; import { REPO, setupApp } from "./app-helpers"; import { makeImportedCourse, VIDEO, videoBytes } from "./import-fixture"; @@ -76,6 +79,49 @@ describe.each(dbs)("convert routes [%s]", (_name, dbUrl) => { }); }); +describe.each(dbs)("conversion and importer jobs [%s]", (_name, dbUrl) => { + test("409 while an import job for the course is queued; the job itself re-checks", async () => { + env = await setupApp([], dbUrl, (d) => makeImportedCourse(d)); + // no importer feature in this app: its jobs stay queued + await env.ctx.jobs.enqueue("import.code-repo", { kind: "code-repo", path: "/x", options: { courseId: "imp" } }); + const r = await env.call("owner", "POST", "/api/studio/courses/imp/convert"); + expect(r.status).toBe(409); + expect(r.json.error).toContain("an import job for this course is still running"); + // a convert job that was queued anyway (before the import was) fails cleanly and changes nothing + const id = await env.ctx.jobs.enqueue("studio.convert-import", { courseId: "imp", actor: { id: "u-own", name: "O", email: "o@x" } }); + const j = await env.waitJob(id); + expect(j.status).toBe("failed"); + expect(j.error).toContain("an import job for this course is still running"); + expect(existsSync(join(env.dataDir, "courses/imp/stcourse.yaml"))).toBe(true); + expect(existsSync(join(env.dataDir, "studio/repos/imp.git"))).toBe(false); + // an import job for another course does not block + await env.db.sql`DELETE FROM jobs WHERE kind = 'import.code-repo'`; + await env.ctx.jobs.enqueue("import.transcribe", { courseId: "other" }); + expect((await env.call("owner", "POST", "/api/studio/courses/imp/convert")).status).toBe(202); + }); + + test("server start recovers a conversion that crashed mid-move; learners see the course once", async () => { + env = await setupApp([], dbUrl, (d) => makeImportedCourse(d)); + const crashing = createStudioStore({ dataDir: env.dataDir, testHooks: { crashAt: "staging-copied", forceExdev: true } }); + await expect(crashing.convertImported("imp", { name: "O", email: "o@x" })).rejects.toBeInstanceOf(SimulatedCrash); + await env.ctx.courses.reload(); + expect(env.ctx.courses.list().filter((c) => c.id === "imp").length).toBeLessThanOrEqual(1); + // a restart: a new studio feature recovers before serving + const restarted = studioFeature(env.ctx, { watch: false }); + await restarted.ready; + restarted.stop(); + const loaded = env.ctx.courses.list().filter((c) => c.id === "imp"); + expect(loaded).toHaveLength(1); + expect(loaded[0]!.root).toBe(join(env.dataDir, "worktrees/imp/published")); + expect(existsSync(join(env.dataDir, "courses/imp"))).toBe(false); + const r = await env.app.fetch(new Request(`http://x/api/media/imp/${VIDEO.split("/").map(encodeURIComponent).join("/")}`, { headers: { "x-user": "owner", range: "bytes=0-9" } })); + expect(r.status).toBe(206); + // the requeued job (or the owner) retrying reports success + expect((await env.call("owner", "POST", "/api/studio/courses/imp/convert")).status).toBe(409); + expect(await env.feature.store.convertImported("imp", { name: "O", email: "o@x" })).toMatchObject({ resumed: true }); + }); +}); + describe("convert route refusals", () => { test("only the owner; cross-site requests are refused; nothing changes", async () => { env = await setupApp([], undefined, (d) => makeImportedCourse(d)); diff --git a/packages/feature-studio/test/convert.test.ts b/packages/feature-studio/test/convert.test.ts index f604b64..a22408a 100644 --- a/packages/feature-studio/test/convert.test.ts +++ b/packages/feature-studio/test/convert.test.ts @@ -1,10 +1,12 @@ import { describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { join } from "node:path"; -import { loadCourse } from "@stc/course-format"; -import { MEDIA_REFS, parseMediaRefs } from "../src/server/media-refs"; +import { loadAll, loadCourse } from "@stc/course-format"; +import { createStudioStore } from "../src/server/store"; +import { SimulatedCrash } from "../src/server/convert"; +import { gitignoreLine, MEDIA_REFS, parseMediaRefs } from "../src/server/media-refs"; import { BIG_PDF, makeImportedCourse, VIDEO, VIDEO2, videoBytes } from "./import-fixture"; -import { ada, extGit, newStore } from "./helpers"; +import { ada, extGit, newStore, tempDir } from "./helpers"; const ino = (p: string) => statSync(p).ino; const tracked = async (dir: string) => (await extGit(dir, "-c", "core.quotepath=false", "ls-files")).split("\n").filter(Boolean).sort(); @@ -24,12 +26,15 @@ describe("convertImported", () => { const pub = store.paths("imp").published; expect(await tracked(pub)).toEqual([ - ".gitignore", MEDIA_REFS, "code/S01 - Basics/.gitignore", "code/S01 - Basics/README.md", "media/cover.jpg", "stcourse.yaml", "transcripts/l1.md", + MEDIA_REFS, "code/S01 - Basics/.gitignore", "code/S01 - Basics/README.md", "media/cover.jpg", "stcourse.yaml", "transcripts/l1.md", ]); // the symlink was skipped, never followed; .git contents never copied expect(existsSync(join(pub, "code/leak.txt"))).toBe(false); expect(existsSync(join(pub, "code/.git"))).toBe(false); expect(await extGit(pub, "status", "--porcelain", "--untracked-files=all")).toBe(""); + // the exclusions live in the repo's info/exclude, which authors and remotes cannot change + const exclude = readFileSync(join(dataDir, "studio/repos/imp.git/info/exclude"), "utf8"); + for (const p of [VIDEO, VIDEO2, BIG_PDF]) expect(exclude).toContain(gitignoreLine(p)); const refs = parseMediaRefs(readFileSync(join(pub, MEDIA_REFS), "utf8"))!; expect(refs.map((x) => x.path).sort()).toEqual([BIG_PDF, VIDEO2, VIDEO].sort()); @@ -166,3 +171,129 @@ describe("convertImported", () => { }); }); }); + +describe("an author's or a remote's .gitignore never blocks the course", () => { + const editManifest = async (store: Awaited>["store"], title: string) => { + const m = (await store.readFile("imp", ada.id, "stcourse.yaml"))!; + const text = m.content.replace(/title: (Intro|Welcome \d+)\n/, `title: ${title}\n`); + expect((await store.saveFiles("imp", ada, [{ path: "stcourse.yaml", content: text, baseHash: m.hash }], title)).ok).toBe(true); + }; + + test("un-ignoring the media in .gitignore: publish keeps working, again and again", async () => { + const { dataDir, store } = await newStore(); + makeImportedCourse(dataDir); + await store.convertImported("imp", ada); + expect((await store.saveFiles("imp", ada, [{ path: ".gitignore", content: "!*.mp4\n!*.pdf\n!/media/**\n" }], "tidy .gitignore")).ok).toBe(true); + expect(await store.publish("imp", ada)).toMatchObject({ ok: true, published: true }); + const pub = store.paths("imp").published; + // git itself now sees the media as untracked; the studio does not count them + expect(await extGit(pub, "-c", "core.quotepath=false", "status", "--porcelain")).toContain("media/"); + expect((await store.status("imp", ada.id)).publishedDirty).toEqual([]); + await editManifest(store, "Welcome 2"); + expect(await store.publish("imp", ada)).toMatchObject({ ok: true, published: true }); + await editManifest(store, "Welcome 3"); + expect(await store.publish("imp", ada)).toMatchObject({ ok: true, published: true }); + expect(readFileSync(join(pub, "stcourse.yaml"), "utf8")).toContain("title: Welcome 3"); + }); + + test("a remote commit that rewrites .gitignore: Sync and publish keep working", async () => { + const { dataDir, store } = await newStore({ allowLocalRemotes: true }); + makeImportedCourse(dataDir); + await store.convertImported("imp", ada); + const remote = join(tempDir(), "remote.git"); + await extGit(tempDir(), "init", "-q", "--bare", "-b", "main", remote); + await store.setRemote("imp", remote); + expect(await store.sync("imp", ada)).toMatchObject({ ok: true, pushed: true }); + const other = join(tempDir(), "other"); + await extGit(tempDir(), "clone", "-q", remote, other); + writeFileSync(join(other, ".gitignore"), "!*\n"); + await extGit(other, "add", ".gitignore"); + await extGit(other, "commit", "-qm", "unignore everything"); + await extGit(other, "push", "-q", "origin", "main"); + expect(await store.sync("imp", ada)).toMatchObject({ ok: true, pulled: true }); + expect(await store.updateDraft("imp", ada)).toMatchObject({ ok: true }); + await editManifest(store, "Welcome 2"); + expect(await store.publish("imp", ada)).toMatchObject({ ok: true, published: true }); + expect(await store.sync("imp", ada)).toMatchObject({ ok: true, pushed: true }); + }); + + test("a remote commit that puts a file over referenced media is refused at Sync; the media stay", async () => { + const { dataDir, store } = await newStore({ allowLocalRemotes: true }); + makeImportedCourse(dataDir); + await store.convertImported("imp", ada); + const remote = join(tempDir(), "remote.git"); + await extGit(tempDir(), "init", "-q", "--bare", "-b", "main", remote); + await store.setRemote("imp", remote); + await store.sync("imp", ada); + const other = join(tempDir(), "other"); + await extGit(tempDir(), "clone", "-q", remote, other); + mkdirSync(join(other, "media"), { recursive: true }); + writeFileSync(join(other, VIDEO), "not the video"); + await extGit(other, "add", "-f", VIDEO); + await extGit(other, "commit", "-qm", "replace media"); + await extGit(other, "push", "-q", "origin", "main"); + expect(await store.sync("imp", ada)).toMatchObject({ ok: false, reason: "forbidden" }); + expect(ino(join(store.paths("imp").published, VIDEO))).toBe(ino(join(dataDir, "media/imp", VIDEO))); + }); + + test("referenced media are not read whole into the editor", async () => { + const { dataDir, store } = await newStore(); + makeImportedCourse(dataDir); + await store.convertImported("imp", ada); + expect(await store.readFile("imp", ada.id, VIDEO)).toBeNull(); + expect(await store.readFile("imp", ada.id, "transcripts/l1.md")).not.toBeNull(); + }); +}); + +describe("crash safety: a retry after any crash point succeeds and the id is never loaded twice", () => { + const loaded = async (dataDir: string) => (await loadAll([join(dataDir, "courses"), join(dataDir, "worktrees")])).filter((c) => c.id === "imp"); + const sameFs = ["committed", "hidden", "worktree", "before-move"]; + const crossFs = [...sameFs, "staging-copied", "staging-placed", "source-retired"]; + const cases: [string, boolean][] = [...sameFs.map((s) => [s, false] as [string, boolean]), ...crossFs.map((s) => [s, true] as [string, boolean])]; + + test.each(cases)("crash at %s (cross-filesystem: %p)", async (step, exdev) => { + const dataDir = tempDir(); + makeImportedCourse(dataDir); + const crashing = createStudioStore({ dataDir, testHooks: { crashAt: step, forceExdev: exdev } }); + await expect(crashing.convertImported("imp", ada)).rejects.toBeInstanceOf(SimulatedCrash); + expect((await loaded(dataDir)).length).toBeLessThanOrEqual(1); + + // the process restarts: recovery first, then the owner (or the requeued job) retries + const fresh = createStudioStore({ dataDir, testHooks: { forceExdev: exdev } }); + await fresh.recoverConversions(); + expect((await loaded(dataDir)).length).toBeLessThanOrEqual(1); + await fresh.convertImported("imp", ada); + // and once more: a re-run of a finished conversion reports success + expect(await fresh.convertImported("imp", ada)).toMatchObject({ resumed: true }); + + expect(fresh.isManaged("imp")).toBe(true); + const all = await loaded(dataDir); + expect(all).toHaveLength(1); + expect(all[0]!.status).toBe("ok"); + expect(all[0]!.root).toBe(fresh.paths("imp").published); + expect(readdirSync(join(dataDir, "courses"))).toEqual([]); + expect(readdirSync(join(dataDir, "media"))).toEqual(["imp"]); + const pub = fresh.paths("imp").published; + for (const p of [VIDEO, VIDEO2, BIG_PDF]) { + expect(statSync(join(pub, p)).size).toBe(statSync(join(dataDir, "media/imp", p)).size); + if (!exdev) expect(ino(join(pub, p))).toBe(ino(join(dataDir, "media/imp", p))); + } + expect(readFileSync(join(dataDir, "media/imp", VIDEO))).toEqual(Buffer.from(videoBytes(4096, 7))); + expect(await extGit(pub, "status", "--porcelain", "--untracked-files=all")).toBe(""); + }); + + test("a failed cross-filesystem copy leaves no staging dir and a retry finishes", async () => { + const dataDir = tempDir(); + makeImportedCourse(dataDir); + const store = createStudioStore({ dataDir, testHooks: { forceExdev: true } }); + // the media store's parent is a file: the copy fails in-process + writeFileSync(join(dataDir, "media"), "not a dir"); + await expect(store.convertImported("imp", ada)).rejects.toThrow(); + expect((await loaded(dataDir)).length).toBeLessThanOrEqual(1); + rmSync(join(dataDir, "media")); + await store.convertImported("imp", ada); + expect(readdirSync(join(dataDir, "media"))).toEqual(["imp"]); + expect(readdirSync(join(dataDir, "courses"))).toEqual([]); + expect(await loaded(dataDir)).toHaveLength(1); + }); +});