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.
This commit is contained in:
+33
-5
@@ -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.
|
||||
|
||||
@@ -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();
|
||||
|
||||
+13
-2
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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); },
|
||||
|
||||
+13
-1
@@ -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);
|
||||
|
||||
+10
-3
@@ -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
|
||||
|
||||
+37
-9
@@ -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
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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" },
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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/");
|
||||
|
||||
+149
-31
@@ -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:
|
||||
|
||||
@@ -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):
|
||||
|
||||
Reference in New Issue
Block a user