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-<id>) 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-<id>.<n>. 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 <noreply@anthropic.com>
This commit is contained in:
Lucas Winther
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
08e62ff683
commit
771d755da4
@@ -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));
|
||||
|
||||
@@ -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<ReturnType<typeof newStore>>["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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user