fix(studio): review round 2 - a conversion that fails after its commit recovers without a restart
- The convert job reloads the course on success and on failure, so learners get the intact import (rolled back) or the published studio course, never a course whose folder moved away. - POST /convert on a course with an incomplete conversion (.converting-<id>, staging or retired folders) queues a job that recovers and finishes it instead of answering 409; the studio page shows "Conversion incomplete" with Retry for the owner. - The 409 for pending import jobs names their kinds and ids; the dead withLock in git.ts is gone; the dot-folder discovery rule is documented in docs/operations.md and spec section 5. - Route tests (SQLite and Postgres) inject a failed cross-filesystem copy, a checksum mismatch and a pre-commit failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
771d755da4
commit
f02e30450c
11 files changed
+168
-34
No files matched your search
@@ -6,7 +6,7 @@ import { ManifestSchema, parseManifest, resolveInside, type LoadedCourse } from
|
||||
import { aclRepo } from "@stc/db";
|
||||
import { createDraftWriter, type DraftWriter } from "./draft-writer";
|
||||
import { storeDirs, studioWorktreesDir } from "./paths";
|
||||
import { checkRelPath, createStudioStore, MANIFEST, ownerRequiredFor, StudioError, type FileWrite, type StudioStore } from "./store";
|
||||
import { checkRelPath, createStudioStore, MANIFEST, ownerRequiredFor, StudioError, type FileWrite, type StudioStore, type StudioStoreOptions } from "./store";
|
||||
import { createPublishedWatcher, type PublishedWatcher } from "./watch";
|
||||
|
||||
export const OUTSIDE_MESSAGE = "This course is managed outside STCourses";
|
||||
@@ -58,6 +58,8 @@ export interface StudioFeatureOptions {
|
||||
/** Poll the published worktrees for external commits (default true). */
|
||||
watch?: boolean;
|
||||
watchIntervalMs?: number;
|
||||
/** Tests only: conversion failure injection (see StudioStoreOptions.testHooks). */
|
||||
storeTestHooks?: StudioStoreOptions["testHooks"];
|
||||
}
|
||||
|
||||
const json = (body: unknown, status = 200) => Response.json(body, { status });
|
||||
@@ -76,7 +78,7 @@ function manifestToYaml(m: unknown): string {
|
||||
}
|
||||
|
||||
export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = {}): StudioFeature {
|
||||
const store = createStudioStore(storeDirs(ctx.config));
|
||||
const store = createStudioStore({ ...storeDirs(ctx.config), testHooks: opts.storeTestHooks });
|
||||
const watcher = createPublishedWatcher(store, ctx.courses, { intervalMs: opts.watchIntervalMs });
|
||||
const acl = aclRepo(ctx.db);
|
||||
const wtDir = resolve(ctx.config.studioWorktreesDir ?? studioWorktreesDir(ctx.config.dataDir));
|
||||
@@ -117,18 +119,27 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = {
|
||||
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";
|
||||
/** 409 text naming the blocking importer jobs (id and kind), so the owner can find them on the import page. */
|
||||
const importRunning = (jobs: { id: string; kind: string }[]) =>
|
||||
`an import job for this course is still running (${jobs.map((j) => `${j.kind} ${j.id}`).join(", ")}); 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 blocking = await pendingImports(courseId);
|
||||
if (blocking.length) throw new StudioError(importRunning(blocking), 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);
|
||||
return { courseId, ...r };
|
||||
try {
|
||||
const r = await store.convertImported(courseId, actor, { onProgress: (f, m) => jc.reportProgress(f, m) });
|
||||
return { courseId, ...r };
|
||||
} finally {
|
||||
// Success or failure, learners must get whatever is on disk now: the intact import (failure before the commit,
|
||||
// rolled back) or the published worktree (after it), never a course whose folder has moved away.
|
||||
await ctx.courses.reload(courseId).catch((e) => console.error("[studio] reload after conversion", e));
|
||||
if (store.isManaged(courseId)) await store.publishedHead(courseId).then((h) => watcher.note(courseId, h), () => {});
|
||||
}
|
||||
};
|
||||
|
||||
const afterPublishedChange = async (courseId: string) => {
|
||||
@@ -184,6 +195,8 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = {
|
||||
}
|
||||
return {
|
||||
readOnly: false,
|
||||
conversionIncomplete: store.conversionIncomplete(courseId),
|
||||
canRetryConversion: isOwner(user),
|
||||
course: { id: courseId, title: (manifestRaw as { title?: string } | null)?.title ?? courseId, managed: true },
|
||||
status,
|
||||
manifestText: m?.content ?? "",
|
||||
@@ -257,14 +270,17 @@ export function studioFeature(ctx: ServerContext, opts: StudioFeatureOptions = {
|
||||
POST: guard(async (_req, rc, user) => {
|
||||
const courseId = rc.params.courseId ?? "";
|
||||
if (!isOwner(user)) return fail(403, "only the owner can make an imported course editable in the studio");
|
||||
if (store.isManaged(courseId)) return fail(409, `course "${courseId}" is already a studio course`);
|
||||
if (!store.isConvertibleImport(courseId)) {
|
||||
// An incomplete conversion (failed after its commit) is finished by a new job: it recovers first.
|
||||
const incomplete = store.conversionIncomplete(courseId);
|
||||
if (store.isManaged(courseId) && !incomplete) return fail(409, `course "${courseId}" is already a studio course`);
|
||||
if (!incomplete && !store.isConvertibleImport(courseId)) {
|
||||
const c = ctx.courses.get(courseId);
|
||||
return c ? fail(409, `${readOnlyMessage(c) === IMPORTED_MESSAGE ? "this imported course" : OUTSIDE_MESSAGE}: only imported courses in ${importedRoot} can be made editable`) : fail(404, "no such imported course");
|
||||
}
|
||||
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 blocking = await pendingImports(courseId);
|
||||
if (blocking.length) return fail(409, importRunning(blocking));
|
||||
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);
|
||||
}),
|
||||
|
||||
Reference in new issue
Block a user