From 1185e2201fca9eb86635f7eb8c480053e8d5ee59 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Sun, 6 Sep 2026 18:08:48 +0300 Subject: [PATCH] core, app: a decade of races closed -- log routes, restarts, results, defaults Two routes silently shared /api/log; a pack's "updates available" badge outlived the update; nothing server-side stopped a restart mid-generation, and the fixes for that had their own dead end, a lock scoped to the wrong event loop, an unlocked status fetch, and a check that only ran once instead of at the moment it mattered. A run's result was lost outright if the project it belonged to wasn't the one open when it finished -- on a project switch or a page reload. A brand-new project inherited the previous one's width, then inherited the wrong "default" for it. Found and confirmed via an adversarial pass over 9dcd94c..HEAD, one round per fix. --- app/boot.js | 38 ++++- app/catalogue/log.js | 2 +- app/shell/client.js | 15 +- app/shell/packs.js | 4 + app/shell/projects.js | 23 +++ app/shell/prompt.js | 14 +- app/shell/run.js | 13 +- app/shell/session.js | 46 +++++- app/shell/tests/client.test.js | 54 +++++- app/shell/tests/projects.test.js | 60 +++++++ app/shell/tests/prompt.test.js | 33 ++++ app/shell/tests/run.test.js | 17 ++ app/shell/tests/session.test.js | 47 +++++- app/shell/updates.js | 25 +++ app/tests-layout/packs.spec.js | 31 ++++ app/tests-layout/project_video.spec.js | 35 ++++ app/tests-layout/updates.spec.js | 32 ++++ core/routes.py | 180 ++++++++++++++++---- core/tests/test_routes_update.py | 219 +++++++++++++++++++++++++ 19 files changed, 829 insertions(+), 59 deletions(-) diff --git a/app/boot.js b/app/boot.js index b98b7f99..ffc72f3c 100644 --- a/app/boot.js +++ b/app/boot.js @@ -82,8 +82,13 @@ async function start() { * window nobody had a reason to reopen. */ const syncVideo = () => { + // A project that never touched an input is not "leave it alone" -- it is + // "this input has no opinion here", which means the pipeline's own + // default. Without the fallback, switching FROM a project that set width + // TO one that never did left the box (and so overrides(), and so the run) + // still showing the first project's number. for (const { input, control } of (prompts ? prompts.controlsAt("project.video") : [])) { - if (project.video[input] !== undefined) control.setValue(project.video[input]); + control.setValue(project.video[input] !== undefined ? project.video[input] : control.default); } }; @@ -99,15 +104,22 @@ async function start() { // Which scene a run was started for, read at the moment it starts. A run // takes minutes and the user goes on clicking, so "the selected scene" at the - // end is not the one that was generated. + // end is not the one that was generated. The project id travels with it -- + // nothing stops the user opening a DIFFERENT project while this one is in + // flight, and the result still belongs to the project it was started in. let ranFor = null; + let ranForProject = null; // Before the page, because the properties column is composed with it in. inspector = createInspector({ project, onRename: (name) => project.rename(name) }); const page = build(root, { inspector, - onGenerate: () => { ranFor = project.selectedId; session.generate(); }, + onGenerate: () => { + ranFor = project.selectedId; + ranForProject = project.project ? project.project.id : null; + session.generate(); + }, onCancel: () => run.cancel(), onConstructor: () => page.constructor.open(), // The window asks whether a run is in flight, because the restart that @@ -171,14 +183,30 @@ async function start() { }); const session = wire({ run, page, check, id, queuedFor, finishedFor, slots: () => slots, values: allValues, - inputs: () => (prompts ? prompts.overrides() : {}) }); + inputs: () => (prompts ? prompts.overrides() : {}), + // Read by run.start() at the moment IT queues -- not + // at Generate-click time, which is what onGenerate + // sets ranFor/ranForProject from. + extra: () => (ranForProject && ranFor + ? { funpack_scene_id: ranFor, funpack_project_id: ranForProject } : null), + // The other way a run reaches this page: reattaching + // to one after a reload, where the click that started + // it happened on a page that is gone. Called BEFORE + // the run is adopted -- an already-finished run can go + // straight to DONE inside adopt() itself, and setting + // these from session.ready instead would run one tick + // too late, after that DONE already found nothing to + // attach the result to. + onAdopt: (sceneId, projectId) => { ranFor = sceneId; ranForProject = projectId; } }); // What a run produced, on the scene it was started from -- which is what puts // it on the timeline and what makes it still there after a reload. run.subscribe((state) => { if (state.phase !== DONE || !ranFor || !state.images.length) return; - project.setResult(ranFor, viewUrl(state.images[state.images.length - 1])); + if (ranForProject) project.setResultFor(ranForProject, ranFor, viewUrl(state.images[state.images.length - 1])); + else project.setResult(ranFor, viewUrl(state.images[state.images.length - 1])); ranFor = null; + ranForProject = null; }); // Undo from the keyboard, which is where anyone will reach for it first. diff --git a/app/catalogue/log.js b/app/catalogue/log.js index 63696f3d..cf463ae4 100644 --- a/app/catalogue/log.js +++ b/app/catalogue/log.js @@ -15,7 +15,7 @@ const TONE = { Info: "neutral", Alert: "info", Warning: "warn", Critical: "dange let filter = null; async function load() { - const url = "/funpack/api/log" + (filter ? `?level=${encodeURIComponent(filter)}` : ""); + const url = "/funpack/api/log/funpack" + (filter ? `?level=${encodeURIComponent(filter)}` : ""); const response = await fetch(url); if (!response.ok) throw new Error(`${url} answered ${response.status}`); return response.json(); diff --git a/app/shell/client.js b/app/shell/client.js index da01480d..91420bdf 100644 --- a/app/shell/client.js +++ b/app/shell/client.js @@ -77,7 +77,13 @@ export async function queuedFor(id, { fetch: doFetch = globalThis.fetch, base = for (const item of half || []) { if (!Array.isArray(item) || item.length < 4) continue; const [, promptId, , extra] = item; - if (extra && extra.client_id === id && promptId) return { promptId, running }; + if (extra && extra.client_id === id && promptId) { + // Where the run belongs, if it said so when queued -- carried through + // so a reload mid-generation can still land the result on its scene + // instead of just knowing a run is going. + return { promptId, running, sceneId: extra.funpack_scene_id || null, + projectId: extra.funpack_project_id || null }; + } } } return null; @@ -93,7 +99,8 @@ export async function queuedFor(id, { fetch: doFetch = globalThis.fetch, base = * result without it. A history entry's `prompt` is * [number, prompt_id, prompt, extra_data, outputs]. */ -export async function finishedFor(id, candidates, { fetch: doFetch = globalThis.fetch, base = "" } = {}) { +export async function finishedFor(id, candidates, + { fetch: doFetch = globalThis.fetch, base = "", onFound } = {}) { for (const promptId of [...(candidates || [])].reverse()) { let response; try { @@ -108,6 +115,10 @@ export async function finishedFor(id, candidates, { fetch: doFetch = globalThis. const prompt = entry && entry.prompt; if (Array.isArray(prompt) && prompt.length > 3 && prompt[3] && prompt[3].client_id === id) { + // `onFound` hands back the extra_data this run was queued with, without + // changing what this function returns everywhere else it is called -- + // where the run belongs is the reattach path's business, not this one's. + if (onFound) onFound(prompt[3]); return promptId; } } diff --git a/app/shell/packs.js b/app/shell/packs.js index 4bc49bd0..84a66232 100644 --- a/app/shell/packs.js +++ b/app/shell/packs.js @@ -51,6 +51,10 @@ export function open() { draw(); try { await work(); + // Stale news, not just a stale listing: an update that just succeeded must + // not go on claiming there is one available until "Check for updates" is + // pressed again. + if (checked) delete checked[name]; await load(); } catch (err) { problem = err.message; diff --git a/app/shell/projects.js b/app/shell/projects.js index 1ed4a607..c77238d8 100644 --- a/app/shell/projects.js +++ b/app/shell/projects.js @@ -272,6 +272,29 @@ export function createProject({ onChange, onError, onOpen } = {}) { /** Attach what a run produced to the scene it was started from. */ setResult(id, result) { this.setScene(id, "result", result); }, + /** + * The same, but for a run that may have finished after the user switched + * to a DIFFERENT project. A run takes minutes and nothing stops someone + * from opening another project while one is in flight -- `setResult` + * alone would look the scene id up in whatever is open NOW, find nothing, + * and drop the result with no trace: the GPU time spent, the image gone. + */ + async setResultFor(projectId, sceneId, result) { + if (project && project.id === projectId) { + this.setResult(sceneId, result); + return; + } + try { + const doc = await read(projectId); + const scene = (doc.scenes || []).find((s) => s.id === sceneId); + if (!scene) return; // the scene itself is gone -- nothing to attach this to + scene.result = result; + await put(doc); + } catch (err) { + if (onError) onError(err); + } + }, + /** Step back, or forward. True when something moved. */ undo() { return step(past, future); }, redo() { return step(future, past); }, diff --git a/app/shell/prompt.js b/app/shell/prompt.js index f01784a7..506d0590 100644 --- a/app/shell/prompt.js +++ b/app/shell/prompt.js @@ -62,8 +62,14 @@ export async function createPrompts(slots = [], { describe, onChange } = {}) { const setting = widget && settingFor(widget); if (!setting) continue; + // What this input is when nothing has an opinion on it. The PIPELINE's + // own value, not the raw node's widget-schema default -- a module can + // and does bake its own sensible number in (this app's latent slot + // declares 512, the underlying node's own schema default is 768) and + // the pipeline's is the one Generate would actually use. + const atRest = current === undefined ? setting.default : current; const entry = { slot: slot.id, input: role.input, at: role.at, - value: current === undefined ? setting.default : current }; + value: atRest, default: atRest }; const told = (next) => { entry.value = next; if (onChange) onChange(entry); @@ -84,6 +90,12 @@ export async function createPrompts(slots = [], { describe, onChange } = {}) { /** What a caller holds: the value, and a way to set it that both halves see. */ const handleFor = (entry) => ({ get value() { return entry.value; }, + // What this control shows when nothing has an opinion -- the pipeline's own + // declared default, not whatever a PREVIOUS project last set it to. Needed + // by anyone syncing this control from per-project state (project.video): + // without it, a project that never touched this input has no way to say + // "go back to normal" and the last project's value is left standing. + get default() { return entry.default; }, setValue(next) { entry.value = next; if (entry.control.setValue) entry.control.setValue(next); diff --git a/app/shell/run.js b/app/shell/run.js index bb40eafd..aa04ffe5 100644 --- a/app/shell/run.js +++ b/app/shell/run.js @@ -289,8 +289,14 @@ export function createRun({ socket = null; }, - /** Queue a prompt. Resolves with the prompt id, or throws with the reason. */ - async start(prompt) { + /** + * Queue a prompt. Resolves with the prompt id, or throws with the reason. + * + * `extra`, when given, rides in `extra_data` alongside `client_id` -- + * ComfyUI stores and echoes it back on `/queue` and `/history`, which is + * the only way a reload can learn what a run in flight belongs to. + */ + async start(prompt, { extra } = {}) { listen(); pending = []; cancelWanted = false; @@ -302,7 +308,8 @@ export function createRun({ response = await doFetch(`${base}/prompt`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ prompt, client_id: clientId }), + body: JSON.stringify(extra ? { prompt, client_id: clientId, extra_data: extra } + : { prompt, client_id: clientId }), }); } catch (err) { // The queue could not be reached at all -- no ComfyUI, or the dev diff --git a/app/shell/session.js b/app/shell/session.js index 26007572..2634a09a 100644 --- a/app/shell/session.js +++ b/app/shell/session.js @@ -16,7 +16,7 @@ * was live, so two clicks each posted their own /prompt: two jobs burning GPU * time, with the UI following one of them. */ -export function createGenerator({ run, transport, check, ready, slots, values, inputs }) { +export function createGenerator({ run, transport, check, ready, slots, values, inputs, extra }) { let starting = false; return async function onGenerate() { @@ -68,7 +68,11 @@ export function createGenerator({ run, transport, check, ready, slots, values, i } try { - await run.start(plan.prompt); + // Where this run belongs, so a reload mid-generation can still find it + // -- omitted rather than sent empty, since ComfyUI stores whatever + // extra_data it is given verbatim. + const meta = extra ? extra() : null; + await run.start(plan.prompt, meta ? { extra: meta } : {}); return true; } catch { return false; // run.state carries the reason @@ -101,7 +105,7 @@ export function createGenerator({ run, transport, check, ready, slots, values, i * ordering is the thing under test, so the ordering lives where tests can reach * it. */ -export function wire({ run, page, check, id, queuedFor, finishedFor, slots, values, inputs }) { +export function wire({ run, page, check, id, queuedFor, finishedFor, slots, values, inputs, extra, onAdopt }) { // One subscription draws everything a run affects. It lives here rather than // beside it in boot.js because subscribe() delivers the CURRENT state at // once, and doing that after the button was deliberately disabled handed it @@ -130,30 +134,54 @@ export function wire({ run, page, check, id, queuedFor, finishedFor, slots, valu // one on the strength of "phase is idle" is starting one on a guess. page.transport.hold("Looking for a run already in progress"); - const ready = reattach(run, id, { queuedFor, finishedFor }).then((adopted) => { + const ready = reattach(run, id, { queuedFor, finishedFor, onAdopt }).then((adopted) => { page.transport.release(run.state); return adopted; }); - const generate = createGenerator({ run, transport: page.transport, check, ready, slots, values, inputs }); + const generate = createGenerator({ run, transport: page.transport, check, ready, slots, values, inputs, extra }); return { ready, generate }; } -export async function reattach(run, id, { queuedFor, finishedFor } = {}) { +/** + * Resolves to `{promptId, sceneId, projectId}` for a run this page adopted, or + * null. `sceneId`/`projectId` are whatever the run was queued with (see + * `run.start`'s `extra`) -- null if it predates that, or was queued some other + * way. + * + * `onAdopt(sceneId, projectId)`, when given, fires SYNCHRONOUSLY, before + * `run.adopt()` -- not after this function returns. A run that already + * finished while the page was reloading can go straight to DONE the moment it + * is adopted (whatever the socket buffered gets replayed inside `adopt()` + * itself), and a listener reading "which scene is this for" only after + * `reattach` resolves would read it one tick too late, after that DONE + * already passed the caller's own run.subscribe with nothing to attach it to. + */ +export async function reattach(run, id, { queuedFor, finishedFor, onAdopt } = {}) { try { const queued = await queuedFor(id); if (queued) { if (run.state.phase === "idle") { + const sceneId = queued.sceneId || null; + const projectId = queued.projectId || null; + if (onAdopt) onAdopt(sceneId, projectId); run.adopt(queued.promptId, { running: queued.running }); - return queued.promptId; + return { promptId: queued.promptId, sceneId, projectId }; } return null; } const seen = run.seen(); if (!seen.length || run.state.phase !== "idle") return null; - const finished = await finishedFor(id, seen); - if (finished && run.state.phase === "idle") { run.adopt(finished); return finished; } + let meta = null; + const finished = await finishedFor(id, seen, { onFound: (extra) => { meta = extra; } }); + if (finished && run.state.phase === "idle") { + const sceneId = (meta && meta.funpack_scene_id) || null; + const projectId = (meta && meta.funpack_project_id) || null; + if (onAdopt) onAdopt(sceneId, projectId); + run.adopt(finished); + return { promptId: finished, sceneId, projectId }; + } return null; } catch { return null; // nothing to reattach to diff --git a/app/shell/tests/client.test.js b/app/shell/tests/client.test.js index ffdb6044..3d3ddb90 100644 --- a/app/shell/tests/client.test.js +++ b/app/shell/tests/client.test.js @@ -19,7 +19,7 @@ test("the run this browser queued is found by its client id", async () => { queue_pending: [], }), }); - assert.deepEqual(found, { promptId: "mine", running: true }); + assert.deepEqual(found, { promptId: "mine", running: true, sceneId: null, projectId: null }); }); test("another browser's run is not adopted", async () => { @@ -54,7 +54,29 @@ test("a malformed queue entry is skipped rather than crashing the load", async ( queue_pending: [undefined, {}, [3, null, {}, { client_id: "me" }, []]], }), }); - assert.deepEqual(found, { promptId: "mine", running: true }); + assert.deepEqual(found, { promptId: "mine", running: true, sceneId: null, projectId: null }); +}); + +test("where a run belongs travels with it, when it said so at queue time", async () => { + const found = await queuedFor("me", { + fetch: answering({ + queue_running: [[1, "mine", {}, { client_id: "me", funpack_scene_id: "s1", + funpack_project_id: "p1" }, []]], + queue_pending: [], + }), + }); + assert.deepEqual(found, { promptId: "mine", running: true, sceneId: "s1", projectId: "p1" }); +}); + +test("a run queued before this existed has nowhere it said it belongs, and says so with null", async () => { + const found = await queuedFor("me", { + fetch: answering({ + queue_running: [[1, "mine", {}, { client_id: "me" }, []]], + queue_pending: [], + }), + }); + assert.equal(found.sceneId, null); + assert.equal(found.projectId, null); }); test("a finished run is recognised as this browser's from its history entry", async () => { @@ -74,6 +96,30 @@ test("a finished run is recognised as this browser's from its history entry", as assert.equal(asked[0], "/history/mine"); }); +test("onFound hands back the extra_data of a match, without changing what this returns", async () => { + const found = []; + const promptId = await finishedFor("me", ["mine"], { + fetch: async () => ({ ok: true, status: 200, json: async () => ({ + mine: { prompt: [1, "mine", {}, { client_id: "me", funpack_scene_id: "s1", + funpack_project_id: "p1" }, []] }, + }) }), + onFound: (extra) => found.push(extra), + }); + assert.equal(promptId, "mine"); + assert.deepEqual(found, [{ client_id: "me", funpack_scene_id: "s1", funpack_project_id: "p1" }]); +}); + +test("onFound is never called when nothing matches", async () => { + let called = false; + await finishedFor("me", ["theirs"], { + fetch: async () => ({ ok: true, status: 200, json: async () => ({ + theirs: { prompt: [1, "theirs", {}, { client_id: "them" }, []] }, + }) }), + onFound: () => { called = true; }, + }); + assert.equal(called, false); +}); + test("a finished run belonging to someone else is not adopted", async () => { const promptId = await finishedFor("me", ["theirs"], { fetch: async (url) => ({ ok: true, status: 200, json: async () => ({ @@ -110,7 +156,7 @@ test("a run still waiting its turn is found too", async () => { queue_pending: [[2, "mine", {}, { client_id: "me" }, []]], }), }); - assert.deepEqual(promptId, { promptId: "mine", running: false }, + assert.deepEqual(promptId, { promptId: "mine", running: false, sceneId: null, projectId: null }, "a job waiting its turn was reported as one already under way"); }); @@ -121,7 +167,7 @@ test("a run under way is preferred over one still waiting", async () => { queue_pending: [[2, "waiting", {}, { client_id: "me" }, []]], }), }); - assert.deepEqual(promptId, { promptId: "under-way", running: true }); + assert.deepEqual(promptId, { promptId: "under-way", running: true, sceneId: null, projectId: null }); }); test("somebody else's pending run is not adopted either", async () => { diff --git a/app/shell/tests/projects.test.js b/app/shell/tests/projects.test.js index c9fa440b..f949635e 100644 --- a/app/shell/tests/projects.test.js +++ b/app/shell/tests/projects.test.js @@ -95,6 +95,66 @@ test("a run's result lands on the scene it was started for, not the current one" assert.equal(sent.at(-1).scenes[0].result, "/view?filename=a.png"); }); +test("a run's result still lands on its scene after the user opened a DIFFERENT project", async () => { + // A run takes minutes; nothing stops the user opening another project while + // one is in flight. `setResult` alone looks the scene id up in whatever is + // open NOW -- finds nothing in the other project, and drops the result with + // no trace. `setResultFor` is what boot.js calls instead once it knows which + // project the run belongs to. + const store = { + a: { id: "aaaaaaaaaaaa", name: "A", scenes: [{ id: "scene-a", text: "", result: null }], updated_at: 1 }, + b: { id: "bbbbbbbbbbbb", name: "B", scenes: [{ id: "scene-b", text: "", result: null }], updated_at: 1 }, + }; + const puts = []; + globalThis.fetch = async (path, opts = {}) => { + if (path === "/funpack/api/projects") { + return { ok: true, status: 200, json: async () => ({ projects: [{ id: store.a.id }, { id: store.b.id }] }) }; + } + const id = decodeURIComponent(String(path).split("/").pop()); + const key = id === store.a.id ? "a" : "b"; + if (opts.method === "PUT") { + const body = JSON.parse(opts.body); + store[key] = body; + puts.push({ key, body }); + return { ok: true, status: 200, json: async () => ({ ...body, updated_at: (store[key].updated_at || 1) + 1 }) }; + } + return { ok: true, status: 200, json: async () => store[key] }; + }; + + const p = createProject({}); + await p.start(); // opens whatever `list()` names first -- project A + const startedInProject = p.project.id; + const startedForScene = p.selected.id; + assert.equal(startedInProject, store.a.id); + + await p.open(store.b.id); // the user wanders off to a different project + assert.equal(p.project.id, store.b.id); + + await p.setResultFor(startedInProject, startedForScene, "/view?filename=a.png"); + + // The project on screen (B) must be untouched. + assert.equal(p.scenes[0].result, null); + // Project A, not open, got the result written straight to its saved copy. + const saved = puts.find((x) => x.key === "a"); + assert.ok(saved, "project A was never saved"); + assert.equal(saved.body.scenes[0].result, "/view?filename=a.png"); +}); + +test("a result for a project that is open again goes through the normal live path", async () => { + // The user switched away and back before the run finished -- the project is + // open again by the time the result lands, so it should update on screen + // immediately rather than only reaching the server. + server(); + const p = createProject({}); + await p.start(); + p.addScene(); + const id = p.project.id; + const sceneId = p.selectedId; + + await p.setResultFor(id, sceneId, "/view?filename=b.png"); + assert.equal(p.scenes[0].result, "/view?filename=b.png"); +}); + test("renaming a project moves the name in the File menu too", async () => { // The listing and the open project are the same name seen twice; a menu that // kept the old one until a reload is a menu that lies about what is open. diff --git a/app/shell/tests/prompt.test.js b/app/shell/tests/prompt.test.js index fe986a58..9e89c4fe 100644 --- a/app/shell/tests/prompt.test.js +++ b/app/shell/tests/prompt.test.js @@ -148,6 +148,39 @@ test("setting a control's value is what the run then sends", async () => { assert.deepEqual(p.controlsAt("nowhere.at.all"), []); }); +test("a control's declared default travels with it, separate from its current value", async () => { + // What syncVideo() falls back to when a NEW project has no opinion on this + // input -- so it has to be the PIPELINE's own value, since that is what + // Generate actually uses with no override. Without this, a control that had + // been changed had no way back to "nothing here has an opinion" once + // switched away and back. + host("project.video"); + const p = await make([{ id: "latent", node: "EmptyLatent", + roles: [{ at: "project.video", input: "width", label: "Width" }], + inputs: {} }]); // the pipeline slot has no width of its own + + const [found] = p.controlsAt("project.video"); + assert.equal(found.control.value, 512, "starts at the fallback, since nothing overrides it"); + assert.equal(found.control.default, 512); + + found.control.setValue(900); // a project's own value, applied later + assert.equal(found.control.value, 900); + assert.equal(found.control.default, 512, "changing the value must not move the default with it"); +}); + +test("the default is the PIPELINE's own value, not the raw node's widget default", async () => { + // A module can and does bake its own number in, distinct from whatever the + // underlying node's schema declares -- entry.default silently used the + // node's schema default once, which was a different, WRONG number here. + host("project.video"); + const p = await make([{ id: "latent", node: "EmptyLatent", + roles: [{ at: "project.video", input: "width", label: "Width" }], + inputs: { width: 768 } }]); // the pipeline's own default disagrees with NODES.EmptyLatent's (512) + + const [found] = p.controlsAt("project.video"); + assert.equal(found.control.default, 768, "the pipeline's own value must win over the node's schema default"); +}); + test("a role naming a place nobody offers is not shown", async () => { // The same rule a module's panel lives by: absent, not broken, and not a // warning either -- this shell may simply not have that region yet. diff --git a/app/shell/tests/run.test.js b/app/shell/tests/run.test.js index 44d394b6..89d85f61 100644 --- a/app/shell/tests/run.test.js +++ b/app/shell/tests/run.test.js @@ -36,6 +36,23 @@ test("queueing a prompt reports it and keeps the id the server gave", async () = assert.equal(sent[0].body.client_id, "c1", "the socket would never hear about this run"); }); +test("extra rides in extra_data, which ComfyUI echoes back on /queue and /history", async () => { + // The only way a reload can learn which scene/project a run in flight + // belongs to -- nothing else carries it through the queue. + const { run, sent } = runner(ok({ prompt_id: "p-2", number: 4 })); + await run.start({ "1": {} }, { extra: { funpack_scene_id: "s1", funpack_project_id: "pr1" } }); + + assert.deepEqual(sent[0].body.extra_data, { funpack_scene_id: "s1", funpack_project_id: "pr1" }); + assert.equal(sent[0].body.client_id, "c1", "client_id must still be sent alongside extra_data"); +}); + +test("no extra given is no extra_data sent, not an empty one", async () => { + const { run, sent } = runner(ok({ prompt_id: "p-3", number: 5 })); + await run.start({ "1": {} }); + + assert.equal("extra_data" in sent[0].body, false); +}); + test("a graph the queue refuses fails with ComfyUI's own reason", async () => { const { run } = runner(refuse(400, { error: { type: "prompt_outputs_failed_validation", message: "Prompt outputs failed validation" }, diff --git a/app/shell/tests/session.test.js b/app/shell/tests/session.test.js index 2a9c3b1f..530952a5 100644 --- a/app/shell/tests/session.test.js +++ b/app/shell/tests/session.test.js @@ -182,10 +182,10 @@ test("a failed attempt leaves Generate usable again", async () => { test("a run still in the queue is taken back over", async () => { const run = fakeRun(); const got = await reattach(run, "me", { - queuedFor: async () => ({ promptId: "still-going", running: true }), + queuedFor: async () => ({ promptId: "still-going", running: true, sceneId: null, projectId: null }), finishedFor: async () => { throw new Error("history should not have been asked"); }, }); - assert.equal(got, "still-going"); + assert.equal(got.promptId, "still-going"); assert.equal(run.state.phase, "running"); }); @@ -196,7 +196,48 @@ test("a run that finished during the load is found in history", async () => { queuedFor: async () => null, finishedFor: async (_id, seen) => (seen.includes("ended-just-now") ? "ended-just-now" : null), }); - assert.equal(got, "ended-just-now"); + assert.equal(got.promptId, "ended-just-now"); +}); + +test("which scene/project a run belongs to travels with it, for both halves", async () => { + const run = fakeRun(); + const queued = await reattach(run, "me", { + queuedFor: async () => ({ promptId: "q1", running: true, sceneId: "s1", projectId: "p1" }), + finishedFor: async () => { throw new Error("should not be asked"); }, + }); + assert.deepEqual(queued, { promptId: "q1", sceneId: "s1", projectId: "p1" }); + + const run2 = fakeRun(); + run2.seen = () => ["f1"]; + const finished = await reattach(run2, "me", { + queuedFor: async () => null, + finishedFor: async (_id, _seen, { onFound } = {}) => { + if (onFound) onFound({ client_id: "me", funpack_scene_id: "s2", funpack_project_id: "p2" }); + return "f1"; + }, + }); + assert.deepEqual(finished, { promptId: "f1", sceneId: "s2", projectId: "p2" }); +}); + +test("onAdopt fires BEFORE the run is adopted, not after reattach resolves", async () => { + // An already-finished run can go straight to DONE inside adopt() itself + // (whatever the socket buffered gets replayed there) -- a listener that + // learns which scene this is for only once reattach's own promise settles + // would learn it one tick after that DONE already reached run.subscribe with + // no scene to attach it to. This is the ordering the fix actually depends on. + const order = []; + const run = fakeRun(); + const realAdopt = run.adopt.bind(run); + run.adopt = (id, opts) => { order.push("adopt"); return realAdopt(id, opts); }; + run.subscribe((state) => { if (state.phase === "running") order.push("subscriber-saw-it"); }); + + await reattach(run, "me", { + queuedFor: async () => ({ promptId: "q1", running: true, sceneId: "s1", projectId: "p1" }), + finishedFor: async () => { throw new Error("should not be asked"); }, + onAdopt: (sceneId, projectId) => { order.push(`onAdopt:${sceneId}:${projectId}`); }, + }); + + assert.deepEqual(order, ["onAdopt:s1:p1", "adopt", "subscriber-saw-it"]); }); test("history is not asked about a run this page never saw", async () => { diff --git a/app/shell/updates.js b/app/shell/updates.js index 70c9653d..680c2273 100644 --- a/app/shell/updates.js +++ b/app/shell/updates.js @@ -79,6 +79,11 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) + "requirements changed." })]); try { const result = await run(); + // The button is already disabled while a run is in flight, but that check + // goes stale if the dialog was left open across one starting -- the server + // re-checks and refuses the restart itself; this is what tells the user why + // nothing happened instead of it looking like the click did nothing. + if (result && result.blocked) { load(); return; } // The server only restarts when the checkout actually moved. Pressing // Update while already up to date is a normal thing to do, and waiting for // a restart that is not coming would hang on an overlay for three minutes. @@ -117,6 +122,26 @@ export function open({ onRestart = waitForRestart, running = () => false } = {}) })); } + // The change already landed last time this was tried; only the restart is + // still owed. Nothing else in this window works until it happens -- the + // server refuses any further git action while one is pending. + if (s.restart_pending) { + rows.push(composer.banner.warn({ + text: "An update already landed and is waiting to restart ComfyUI.", + })); + rows.push(composer.settingsRow.default({ + label: "Restart", + hint: busy ? "Waiting for the generation to finish." : "Ready.", + control: composer.button.md({ label: "Restart now", tone: "primary", disabled: busy, + onClick: () => act(() => ask("POST", "/restart")) }), + })); + body.set([composer.group.default({ rows })]); + if (window_) window_.setFooter({ actions: [ + composer.button.md({ label: "Close", tone: "ghost", onClick: () => window_.close("done") }), + ] }); + return; + } + if (s.dirty) { // The one state that blocks both actions, said before either is pressed // rather than as a refusal afterwards. diff --git a/app/tests-layout/packs.spec.js b/app/tests-layout/packs.spec.js index b5d02bcc..96e9ba70 100644 --- a/app/tests-layout/packs.spec.js +++ b/app/tests-layout/packs.spec.js @@ -125,6 +125,37 @@ test("checking for updates shows which packs are behind", async ({ page }) => { .toContainText("up to date"); }); +test("updating a pack clears its stale 'updates available' badge", async ({ page }) => { + // run() reloaded the listing after a successful update but never touched the + // `checked` news from the earlier "Check for updates" -- the badge kept + // claiming updates were available for a pack that had just been brought + // current, until the button was pressed again. + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + let updated = false; + await page.route("**/api/packs", (route) => route.fulfill({ + json: { root: "/x/custom_nodes", nodes: [ + { name: "ComfyUI-Behind", is_funpack: false, git: true, branch: "main", + commit: updated ? "ccc" : "aaa" }] }, + })); + await page.route("**/api/packs/check", (route) => route.fulfill({ + json: { checked: { "ComfyUI-Behind": { checked: true, branch: "main", ahead: 0, behind: 3 } } }, + })); + await page.route("**/api/packs/update", (route) => { + updated = true; + route.fulfill({ json: { ok: true } }); + }); + + await openPacks(page); + await page.getByRole("button", { name: "Check for updates" }).click(); + const row = page.locator(".cx-settings-row", { hasText: "ComfyUI-Behind" }); + await expect(row).toContainText("3 updates available"); + + await row.getByRole("button", { name: "Update" }).click(); + await expect(row).not.toContainText("3 updates available"); + await expect(row).toContainText("main"); +}); + test("a pack that could not be checked says why, not 'up to date'", async ({ page }) => { await page.goto("/funpack/"); await page.waitForFunction(() => window.FunPack !== undefined); diff --git a/app/tests-layout/project_video.spec.js b/app/tests-layout/project_video.spec.js index 243856a9..95a2d65e 100644 --- a/app/tests-layout/project_video.spec.js +++ b/app/tests-layout/project_video.spec.js @@ -157,6 +157,41 @@ test("switching projects switches what the run will use, not just what is stored await expect(widthBox(page)).toHaveValue("640"); }); +test("a project that never touched width is not left showing the last project's", async ({ page }) => { + // syncVideo() only pushed a project's OWN values into the controls -- a + // project whose video dict is still {} took neither branch, so the control + // (and so overrides(), and so the run) kept whatever the PREVIOUS project had + // set. A brand-new project is exactly that state. + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + + await page.evaluate(async (n) => { await window.FunPack.project.newProject(n); }, `A ${Date.now()}`); + await openConstructor(page); + await widthBox(page).fill("900"); + await widthBox(page).blur(); + await page.locator(".cx-modal").getByRole("button", { name: "Done" }).click(); + await page.evaluate(() => window.FunPack.project.flush()); + await expect.poll(() => page.evaluate(() => window.FunPack.prompts().latent.width)).toBe(900); + + // A second project that never touches width at all. + await page.evaluate(async (n) => { await window.FunPack.project.newProject(n); }, `B ${Date.now()}`); + await expect.poll(() => page.evaluate(() => window.FunPack.project.video.width)).toBe(undefined); + + // The pipeline's OWN declared default (modules/system/pipeline: width 512), + // not the raw node's widget-schema default (FunPackEmptyLatent's own is + // 768) -- entry.default silently used the node's schema default once, which + // is a different, wrong number. Not just "not 900": the actual right value. + const fallback = await page.evaluate(async () => { + const r = await fetch("/funpack/api/pipeline"); + const slots = (await r.json()).slots || []; + return (slots.find((s) => s.id === "latent") || {}).inputs?.width; + }); + await expect.poll(() => page.evaluate(() => window.FunPack.prompts().latent.width), + { message: "the run would still use the other project's width, or the wrong default" }).toBe(fallback); + await openConstructor(page); + await expect(widthBox(page)).toHaveValue(String(fallback)); +}); + test("an edit can be taken back from the menu and from the keyboard", async ({ page }) => { await page.goto("/funpack/"); await page.waitForFunction(() => window.FunPack !== undefined); diff --git a/app/tests-layout/updates.spec.js b/app/tests-layout/updates.spec.js index 56dc36c3..fd514a52 100644 --- a/app/tests-layout/updates.spec.js +++ b/app/tests-layout/updates.spec.js @@ -123,6 +123,38 @@ test("an update that changed nothing does not wait for a restart", async ({ page await expect(page.locator(".cx-modal")).toContainText("Up to date"); }); +test("a restart blocked mid-run is not lost -- the window offers a way to finish it", async ({ page }) => { + // The client's own "a run is in flight" check is a courtesy, not the guard -- + // it goes stale if the dialog was already open when a run started. The + // server refuses the restart itself and remembers it owes one; the window + // has to surface that or the update is permanently applied-but-not-live. + let pending = false; + await page.goto("/funpack/"); + await page.waitForFunction(() => window.FunPack !== undefined); + await page.route("**/api/git/status**", (route) => route.fulfill({ + json: { ok: true, version: "5.0", branch: "v5", branches: ["v5", "dev"], dirty: false, + ahead: 0, behind: pending ? 0 : 2, fetch_ok: true, checked_remote: true, repo: "/x", + restart_pending: pending }, + })); + await page.route("**/api/git/update", (route) => { + pending = true; + return route.fulfill({ json: { restarting: false, blocked: "A generation is running.", + updated: true } }); + }); + await page.route("**/api/git/restart", (route) => route.fulfill({ json: { restarting: true } })); + await page.route("**/api/health", (route) => route.abort()); + + await openUpdates(page); + await page.getByRole("button", { name: /Update \(2\)/ }).click(); + + await expect(page.locator(".cx-modal")).toContainText(/waiting to restart/i); + const restartBtn = page.getByRole("button", { name: "Restart now" }); + await expect(restartBtn).toBeVisible(); + + await restartBtn.click(); + await expect(page.locator(".cx-blocking")).toBeVisible(); +}); + test("a generation in flight is said, and the buttons that would kill it are off", async ({ page }) => { // The restart takes the run with it, with the GPU time already spent. await page.goto("/funpack/"); diff --git a/core/routes.py b/core/routes.py index 16d88cf9..fff3391b 100644 --- a/core/routes.py +++ b/core/routes.py @@ -5,6 +5,7 @@ so the real routes can be mounted on a throwaway app in a test. The handlers are thin adapters over pure functions in `serve`. """ +import asyncio import json from . import (backend_log, config, graph as graph_mod, log, nodes_manager, projects, @@ -29,6 +30,59 @@ except Exception: # not running inside ComfyUI PromptServer = None +_pending_restart = False +"""Set when a git change landed but a generation blocked the restart it needs. + +Deliberately module state, not per-request: the change is already on disk and +ComfyUI is already running stale code for it. The one way out is `/api/git/ +restart`, gated on the same running-check -- nothing re-runs `pull`/`checkout`/ +`rollback` to "retry", because by the time this fires HEAD has already moved and +running that action again would no longer mean what it did the first time +(rollback especially: run twice, it undoes itself). +""" + +def _generation_running(): + """Whether ComfyUI is mid-generation right now, if this can even be asked. + + The client already disables the update/checkout/rollback buttons while a run + is in flight, but that check goes stale the moment the dialog is left open + across a run starting -- this is the one that actually stops the restart. + Module-level (rather than nested in `register`) so a test can replace it + without a real PromptServer. + """ + instance = getattr(PromptServer, "instance", None) + if instance is None: + return False + try: + return instance.prompt_queue.get_tasks_remaining() > 0 + except Exception: + return False + + +def _schedule_restart(): + """Restart ComfyUI shortly, unless a generation started in the meantime. + + The 0.7s delay (so the response reaches the client before the process + goes) is itself a check-then-act window: `_generation_running()` was + false when this was scheduled, but nothing stops a generation being + queued in the gap. Checked again right here, at the moment that matters, + instead of trusting the answer from 0.7s ago -- a restart scheduled while + idle and fired while running is exactly the thing this whole mechanism + exists to prevent. + """ + global _pending_restart + + def _fire(): + global _pending_restart + if _generation_running(): + _pending_restart = True + return + from . import restart as restart_mod + restart_mod.restart() + + asyncio.get_event_loop().call_later(0.7, _fire) + + def _respond(served): if served.status != 200: return web.Response(status=served.status, headers=served.headers) @@ -87,6 +141,21 @@ def register(routes, prefix=None): """Attach FunPack's routes to an aiohttp route table.""" P = config.UI_PREFIX if prefix is None else prefix + # Serializes every git-mutating route (including the remote fetch inside + # status). Created fresh here, not at module import: an `asyncio.Lock` + # binds to whichever event loop first acquires it, and register() runs + # once per real server process but once per TEST too, each on its own + # loop -- a module-level lock reused across those raised "bound to a + # different event loop" the moment a second test tried it. + # + # Without it, two requests -- two tabs, or a double-click before the + # client's own disable takes effect -- could both pass the + # `_pending_restart` check before either had set it, and then run two real + # `git` subprocesses against the same working tree at once: index.lock + # contention at best, a checkout's branch switch interleaved with a + # pull's fetch/merge at worst. + _git_lock = asyncio.Lock() + @routes.get(P + "/api/health") async def _health(_req): return web.json_response({"ok": True}) @@ -252,45 +321,94 @@ def register(routes, prefix=None): async def _git(action, **kwargs): """Run one git operation off the event loop, or say why it did not.""" - import asyncio - try: - result = await asyncio.to_thread(action, **kwargs) - except update_mod.GitUpdateError as exc: - # A refusal, not a crash: no git, not a checkout, a dirty tree, a - # branch that exists on neither side. Each names what to do. - return web.json_response({"detail": str(exc)}, status=400) - except Exception as exc: # noqa: BLE001 - log.broke("git", exc, doing=getattr(action, "__name__", "git")) - return web.json_response({"detail": f"{type(exc).__name__}: {exc}"}, status=500) + global _pending_restart + async with _git_lock: + if _pending_restart: + # A change already landed and is only waiting on the restart -- + # running another git action on top of it before that restart + # happens is not "retrying", it is a second change stacked on an + # unapplied one. Send them to finish the first restart instead. + return web.json_response({ + "detail": "An earlier update is applied and waiting on a restart " + "(Settings ▸ Updates). Restart before doing anything else here.", + }, status=409) + try: + result = await asyncio.to_thread(action, **kwargs) + except update_mod.GitUpdateError as exc: + # A refusal, not a crash: no git, not a checkout, a dirty tree, a + # branch that exists on neither side. Each names what to do. + return web.json_response({"detail": str(exc)}, status=400) + except Exception as exc: # noqa: BLE001 + log.broke("git", exc, doing=getattr(action, "__name__", "git")) + return web.json_response({"detail": f"{type(exc).__name__}: {exc}"}, status=500) - result = result or {} - # ONLY when the checkout actually moved. Pressing Update while already up - # to date is a normal thing to do -- there is no way to know until it has - # been asked -- and restarting for it costs a boot and, if a generation - # is running, the generation. - moved = bool(result.get("updated")) or ( - result.get("before") is not None and result.get("before") != result.get("after") - ) or ( - result.get("before_branch") is not None - and result.get("before_branch") != result.get("branch") - ) - if not moved: - return web.json_response({"restarting": False, **result}) + result = result or {} + # ONLY when the checkout actually moved. Pressing Update while already + # up to date is a normal thing to do -- there is no way to know until + # it has been asked -- and restarting for it costs a boot and, if a + # generation is running, the generation. + moved = bool(result.get("updated")) or ( + result.get("before") is not None and result.get("before") != result.get("after") + ) or ( + result.get("before_branch") is not None + and result.get("before_branch") != result.get("branch") + ) + if not moved: + return web.json_response({"restarting": False, **result}) - import asyncio as _asyncio - from . import restart as restart_mod - # After the response is written, not before. - _asyncio.get_event_loop().call_later(0.7, restart_mod.restart) - return web.json_response({"restarting": True, **result}) + if _generation_running(): + _pending_restart = True + return web.json_response({ + "restarting": False, + "blocked": "A generation is running. The update is applied -- " + "restart it from this window once the generation finishes.", + **result, + }) + + _pending_restart = False + # After the response is written, not before. + _schedule_restart() + return web.json_response({"restarting": True, **result}) + + @routes.post(P + "/api/git/restart") + async def _git_restart(_req): + """Finish a restart a running generation deferred. + + Never re-runs the git action: HEAD already moved when the change first + landed, and there is nothing left to do here but the restart itself. + """ + global _pending_restart + async with _git_lock: + if not _pending_restart: + return web.json_response( + {"detail": "Nothing is waiting on a restart."}, status=400) + if _generation_running(): + return web.json_response({ + "restarting": False, + "blocked": "A generation is still running.", + }) + _pending_restart = False + _schedule_restart() + return web.json_response({"restarting": True}) @routes.get(P + "/api/git/status") async def _git_status(req): # `?remote=0` answers from the checkout alone. The fetch is the only part # that touches the network and so the only part that can hang; the app # asks without it first so it has something to draw. - import asyncio + # + # `remote=1` runs its own `git fetch` -- a real mutation of this + # checkout's refs, same as pull/checkout/rollback -- so it shares their + # lock. Without it, opening this window mid-update raced two `git fetch` + # invocations against the same repo, which can interleave writes to + # FETCH_HEAD and produce a "cannot fast-forward to multiple branches" + # failure, or a false "you have local commits at risk" refusal, that had + # nothing to do with either fetch's own branch. remote = req.query.get("remote", "1") not in ("0", "false", "no") - return web.json_response(await asyncio.to_thread(update_mod.status, remote=remote)) + async with _git_lock: + payload = await asyncio.to_thread(update_mod.status, remote=remote) + payload["restart_pending"] = _pending_restart + return web.json_response(payload) @routes.post(P + "/api/git/update") async def _git_pull(req): @@ -466,7 +584,7 @@ def register(routes, prefix=None): return web.json_response({"problems": ["no such project"]}, status=404) return web.json_response({"deleted": True}) - @routes.get(P + "/api/log") + @routes.get(P + "/api/log/funpack") async def _log(req): level = req.query.get("level") or None try: diff --git a/core/tests/test_routes_update.py b/core/tests/test_routes_update.py index 0d4650f3..9cfaa88c 100644 --- a/core/tests/test_routes_update.py +++ b/core/tests/test_routes_update.py @@ -24,6 +24,10 @@ def server(comfyui, monkeypatch): restarts = [] from core import restart as restart_mod monkeypatch.setattr(restart_mod, "restart", lambda: restarts.append(1)) + # `_pending_restart` is module state, not per-request -- a test that blocks a + # restart and never finishes it would otherwise leak "an update is waiting" + # into every test that runs after it in the same process. + monkeypatch.setattr(routes, "_pending_restart", False) app = aioweb.Application() table = aioweb.RouteTableDef() @@ -252,6 +256,211 @@ def test_a_branch_switch_restarts_even_when_the_commit_is_the_same(server, monke assert server["restarts"] == [1] +def test_a_generation_started_during_the_07s_delay_still_stops_the_restart(server, monkeypatch): + """`_generation_running()` was only ever checked once, at schedule time -- + the 0.7s gap before the process actually goes (so the response reaches the + client first) was a window nothing re-checked. A run queued in that window + used to be killed anyway, which is exactly what this whole mechanism + exists to prevent.""" + monkeypatch.setattr(update_mod, "pull", + lambda **_k: {"updated": True, "before": "a1", "after": "b2"}) + running = False + monkeypatch.setattr(routes, "_generation_running", lambda: running) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", {}) + assert status == 200, body + assert body["restarting"] is True, "scheduled optimistically, before the delay" + + # A generation starts in the gap between the response and the actual + # restart -- the moment the real bug happened. + running = True + time.sleep(1.0) + + assert server["restarts"] == [], "restarted anyway despite the generation" + + status, body = _request(server["port"], "GET", "/funpack/api/git/status") + assert body["restart_pending"] is True, "the deferred restart is not visible anywhere" + + +def test_a_moved_checkout_does_not_restart_while_a_generation_is_running(server, monkeypatch): + """The client disables the buttons while a run is in flight, but that check + goes stale if the dialog was left open across one starting -- this is the + guard that actually stops the restart, not just the appearance of one.""" + monkeypatch.setattr(update_mod, "pull", + lambda **_k: {"updated": True, "before": "a1", "after": "b2"}) + monkeypatch.setattr(routes, "_generation_running", lambda: True) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", + {"branch": "dev"}) + assert status == 200, body + assert body["restarting"] is False + assert body["blocked"], "did not say why it refused to restart" + assert body["after"] == "b2", "the git operation itself should still have run" + time.sleep(1.0) + assert server["restarts"] == [], "restarted ComfyUI while a generation was running" + + +def test_a_blocked_restart_is_actually_finishable_once_the_run_ends(server, monkeypatch): + """A blocked update used to be a dead end: HEAD had already moved, so the + same button's next press saw nothing to update and never restarted -- + ComfyUI kept running stale code with no way back short of the terminal.""" + monkeypatch.setattr(update_mod, "pull", + lambda **_k: {"updated": True, "before": "a1", "after": "b2"}) + monkeypatch.setattr(routes, "_generation_running", lambda: True) + + status, body = _request(server["port"], "POST", "/funpack/api/git/update", {}) + assert status == 200, body + assert body["restarting"] is False + + status, body = _request(server["port"], "GET", "/funpack/api/git/status") + assert status == 200, body + assert body["restart_pending"] is True, "the owed restart is not visible anywhere" + + # Nothing else here works while a restart is owed -- running another git + # action on top of an unrestarted change is not a retry, it is a second + # change stacked on the first. + status, body = _request(server["port"], "POST", "/funpack/api/git/update", {}) + assert status == 409, body + + # Still running: the dedicated restart route refuses too, same as the guard. + status, body = _request(server["port"], "POST", "/funpack/api/git/restart") + assert status == 200, body + assert body["restarting"] is False + assert server["restarts"] == [] + + # The run ends. The SAME deferred restart finishes -- `pull` is not called + # again, so if it were, this would fail on Mock's default None return. + monkeypatch.setattr(routes, "_generation_running", lambda: False) + status, body = _request(server["port"], "POST", "/funpack/api/git/restart") + assert status == 200, body + assert body["restarting"] is True + time.sleep(1.0) + assert server["restarts"] == [1] + + status, body = _request(server["port"], "GET", "/funpack/api/git/status") + assert body["restart_pending"] is False, "stayed pending after actually restarting" + + +def test_restarting_with_nothing_pending_is_refused(server): + status, body = _request(server["port"], "POST", "/funpack/api/git/restart") + assert status == 400 + assert "detail" in body + + +def test_a_rollback_pressed_twice_while_blocked_does_not_roll_back_twice(server, monkeypatch): + """The dangerous version of the dead end: HEAD already moved once, so + re-running `rollback` on a second press would target the commit BEFORE the + blocked rollback and silently undo it instead of finishing the restart.""" + calls = [] + + def rollback(): + calls.append(1) + return {"branch": "v5", "before": "b2", "after": "a1"} + monkeypatch.setattr(update_mod, "rollback", rollback) + monkeypatch.setattr(routes, "_generation_running", lambda: True) + + status, body = _request(server["port"], "POST", "/funpack/api/git/rollback") + assert status == 200, body + assert body["restarting"] is False + assert calls == [1] + + # Pressed again while still blocked: refused outright, `rollback` not called + # a second time. + status, body = _request(server["port"], "POST", "/funpack/api/git/rollback") + assert status == 409, body + assert calls == [1], "ran rollback a second time instead of refusing" + + +def test_two_git_actions_at_once_do_not_touch_the_checkout_at_the_same_time(server, monkeypatch): + """Two tabs, or a double-click before the button disables -- both requests + used to be able to pass the pending-restart check and then run two real git + subprocesses against the same working tree at once.""" + events = [] + + def slow_pull(**_k): + events.append(("pull", "enter")) + time.sleep(0.3) + events.append(("pull", "exit")) + return {"updated": False, "before": "a1", "after": "a1"} + + def slow_checkout(**_k): + events.append(("checkout", "enter")) + time.sleep(0.3) + events.append(("checkout", "exit")) + return {"updated": False, "before": "a1", "after": "a1", + "before_branch": "v5", "branch": "v5"} + + monkeypatch.setattr(update_mod, "pull", slow_pull) + monkeypatch.setattr(update_mod, "checkout", slow_checkout) + + results = {} + + def fire(name, path, body): + results[name] = _request(server["port"], "POST", path, body) + + t1 = threading.Thread(target=fire, args=("update", "/funpack/api/git/update", {})) + t2 = threading.Thread(target=fire, args=("checkout", "/funpack/api/git/checkout", {"branch": "v5"})) + t1.start() + time.sleep(0.05) # let the first one actually reach the lock first + t2.start() + t1.join(5) + t2.join(5) + + assert results["update"][0] == 200, results["update"] + assert results["checkout"][0] == 200, results["checkout"] + # Whichever ran first, it must have fully exited before the other entered -- + # never interleaved (enter, enter, exit, exit) and never reordered so an + # exit is missing before the next enter. + assert events in ( + [("pull", "enter"), ("pull", "exit"), ("checkout", "enter"), ("checkout", "exit")], + [("checkout", "enter"), ("checkout", "exit"), ("pull", "enter"), ("pull", "exit")], + ), events + + +def test_status_with_a_remote_fetch_does_not_race_a_git_action_either(server, monkeypatch): + """`GET /api/git/status?remote=1` runs its own `git fetch` -- a real + mutation of the checkout's refs, same as pull/checkout/rollback -- and the + Updates window fires it on every open. Without sharing the lock, opening + that window mid-update raced two fetches against the same repo.""" + events = [] + + def slow_status(*, remote=True): + events.append(("status", "enter")) + time.sleep(0.3) + events.append(("status", "exit")) + return {"ok": True, "branch": "v5", "branches": ["v5"], "dirty": False, + "ahead": 0, "behind": 0, "fetch_ok": True, "repo": "/x"} + + def slow_pull(**_k): + events.append(("pull", "enter")) + time.sleep(0.3) + events.append(("pull", "exit")) + return {"updated": False, "before": "a1", "after": "a1"} + + monkeypatch.setattr(update_mod, "status", slow_status) + monkeypatch.setattr(update_mod, "pull", slow_pull) + + results = {} + + def fire(name, method, path, body=None): + results[name] = _request(server["port"], method, path, body) + + t1 = threading.Thread(target=fire, args=("update", "POST", "/funpack/api/git/update", {})) + t2 = threading.Thread(target=fire, args=("status", "GET", "/funpack/api/git/status")) + t1.start() + time.sleep(0.05) + t2.start() + t1.join(5) + t2.join(5) + + assert results["update"][0] == 200, results["update"] + assert results["status"][0] == 200, results["status"] + assert events in ( + [("pull", "enter"), ("pull", "exit"), ("status", "enter"), ("status", "exit")], + [("status", "enter"), ("status", "exit"), ("pull", "enter"), ("pull", "exit")], + ), events + + def test_a_rollback_restarts_because_the_commit_moved(server, monkeypatch): monkeypatch.setattr(update_mod, "rollback", lambda: {"branch": "v5", "before": "b2", "after": "a1"}) @@ -322,6 +531,16 @@ def test_a_silly_limit_does_not_read_a_whole_session_into_memory(server): assert len(body["lines"]) <= 2000, limit +def test_funpacks_own_log_answers_at_its_own_path_not_the_backend_ones(server): + """Both logs were once registered on the same path, one shadowing the + other -- FunPack's severity/source log never answered, and its consumer + crashed on `data.levels`. They live at different paths now.""" + status, body = _request(server["port"], "GET", "/funpack/api/log/funpack") + assert status == 200, body + assert "levels" in body and "records" in body + assert isinstance(body["levels"], list) + + # --- temp files -------------------------------------------------------------- def test_temp_files_come_back_newest_first_with_where_they_are(server, monkeypatch, tmp_path):