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); + }); +});