mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix: harden install queue/dispatcher lifecycle and repair review-sweep regressions (#395)
Fixes 15 defects found by a max-effort multi-agent review of the last 6 merged PRs (#388, #390, #391, #392, #393, #394), all adversarially verified before fixing. Install queue + dispatcher (the serious cluster): - features.ts: finalize the installer child exactly once. A failed spawn fires both "error" and "close", and the second event released the file lock and active slot that pump() had just handed to the next queued bundle, letting two pip processes write the same venv concurrently. Outcome recording now happens before pump() so the next bundle's first progress frame cannot race the previous install's bookkeeping. - feature-status.ts: keep failed-install errors in a per-bundle map instead of the single progress slot. With the queue auto-starting the next install, the slot was overwritten within seconds and a failed install vanished without ever surfacing to GET /features. - bridge.ts: scope child lifecycle per process (stopped-children set + request generation tags) instead of an instance-wide shuttingDown flag that the next spawn reset. A stale SIGTERMed child's late close event could record a phantom crash (5 of which permanently disable the dispatcher), null out the freshly spawned child, and reject the new child's pending requests. The request-timeout kill path still counts as a real crash. - install_feature.py: the pre-write disk re-check measured ai_dir's filesystem even when budgeting the cross-filesystem copy that lands on the venv's disk; now each budget is checked against the filesystem the bytes actually land on, so ENOSPC cannot strike mid-write and leave site-packages half overwritten. Behavior regressions: - embed-subtitles: preserve pre-existing subtitle tracks (0:s?) and MKV attachments (0:t?) that the -map 0:v:0/0:a? rewrite silently dropped; data streams stay unmapped on purpose (the actual MPEG remux fix). The new subtitle maps first so the language tag hits the right stream. - usage-survey-overlay: fail closed when the settings fetch fails; the fail-open path rendered the blocking survey against an unhealthy API and soft-locked admins, the lock-out class #392 fixed. - features-store: queued bundles poll instead of each holding an SSE connection (Install All could pin 7 EventSources and exhaust the browser's 6-per-origin HTTP/1.1 limit, hanging the whole app); listenToProgress closes any prior stream and stops any poll before subscribing; installAll skips bundles already installing or queued. Contracts, tests, i18n: - openapi.yaml: add "queued" to the features status enum and document downloadBytes/installedBytes (Schemathesis conformance). - feature-lifecycle e2e: queue transcription (~0.5 GB) instead of ocr (~6 GB) and give the test a budget that covers both install drains (the stacked waits exceeded the old 900s timeout). - docker-compose.qa.yml: parameterize the host port (QA_APP_PORT) so QA_PROJECT_NAME concurrent stacks can actually bind. - compare + watermark-image: restore per-input error attribution ("Invalid first/second image", "Invalid watermark image") lost in the shared-handler migration. - ai-features-section: the "{size} on disk" suffix now goes through i18n; key added to all 21 locales. - watermark-image + content-aware-resize: migrate to the shared inputHandlerFor("image") chain like compare/vectorize/compose, fixing drift in the inline copies (no SVG sanitize, no RAW extension hint, no AVIF probe). Verified: typecheck across 9 workspaces, Biome clean on all changed files, 584 targeted unit tests and 249 integration tests green (including real-ffmpeg embed-subtitles runs). One unit test updated to the new poll-while-queued contract with a single-EventSource assertion. Claude-Session: https://claude.ai/code/session_017mR1HiHaf3a1BmUtrHX4j3
This commit is contained in:
SnapOtter
@@ -316,24 +316,33 @@ test.describe("Install lifecycle - face-detection", () => {
|
||||
});
|
||||
|
||||
test("install of different bundle while one is active is queued (202)", async ({ request }) => {
|
||||
test.setTimeout(900_000);
|
||||
// The drain below waits out two full installs, each with the helper's
|
||||
// 600s default budget, so the test timeout must cover both plus slack.
|
||||
test.setTimeout(1_500_000);
|
||||
const status = await getBundleStatus(request, "face-detection");
|
||||
if (status !== "installing") return; // fast install already finished; nothing to assert
|
||||
|
||||
const headers = await authHeaders(request);
|
||||
const res = await request.post(`${API}/api/v1/admin/features/ocr/install`, { headers });
|
||||
// Queue transcription: the smallest bundle (~0.5 GB archive) that no
|
||||
// other test here depends on. There is no cancel API for a queued
|
||||
// install, so the drain below really downloads it; queueing ocr here
|
||||
// used to pull a ~6 GB archive just to assert queued=true.
|
||||
const res = await request.post(`${API}/api/v1/admin/features/transcription/install`, {
|
||||
headers,
|
||||
});
|
||||
expect(res.status()).toBe(202);
|
||||
expect((await res.json()).queued).toBe(true);
|
||||
|
||||
const ocr = await getBundle(request, "ocr");
|
||||
expect(["queued", "installing"]).toContain(ocr.status);
|
||||
const queuedBundle = await getBundle(request, "transcription");
|
||||
expect(["queued", "installing"]).toContain(queuedBundle.status);
|
||||
|
||||
// ocr auto-starts once face-detection finishes. Drain and uninstall it so
|
||||
// the rest of the serial suite runs against a clean lock/state.
|
||||
// transcription auto-starts once face-detection finishes. Drain and
|
||||
// uninstall it so the rest of the serial suite runs against a clean
|
||||
// lock/state.
|
||||
await waitForInstallComplete(request, "face-detection");
|
||||
await waitForInstallComplete(request, "ocr", 900_000);
|
||||
await request.post(`${API}/api/v1/admin/features/ocr/uninstall`, { headers });
|
||||
expect(await getBundleStatus(request, "ocr")).toBe("not_installed");
|
||||
await waitForInstallComplete(request, "transcription");
|
||||
await request.post(`${API}/api/v1/admin/features/transcription/uninstall`, { headers });
|
||||
expect(await getBundleStatus(request, "transcription")).toBe("not_installed");
|
||||
});
|
||||
|
||||
test("after install completes, status is installed with version", async ({ request }) => {
|
||||
|
||||
@@ -112,7 +112,7 @@ describe("Login failures", () => {
|
||||
// hash (scrypt), while "wrong password for a real user" always pays the
|
||||
// scrypt cost. That gap lets an attacker enumerate valid usernames purely
|
||||
// from response timing even though the status code and body are identical.
|
||||
// See getDummyHash() in apps/api/src/plugins/auth.ts -- it equalizes cost
|
||||
// See getDummyHash() in apps/api/src/plugins/auth.ts; it equalizes cost
|
||||
// by running verifyPassword against a dummy hash on the unknown-user path.
|
||||
const SAMPLES = 10;
|
||||
const median = (values: number[]) => {
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
* The installer child process (spawn) and the venv lock (@snapotter/ai) are
|
||||
* mocked so no real Python runs: spawn returns a controllable fake child we can
|
||||
* drive with emit("close"). This lets us assert the route contract
|
||||
* deterministically -- a second concurrent install is queued (202 { queued:
|
||||
* deterministically: a second concurrent install is queued (202 { queued:
|
||||
* true }) instead of rejected, the next queued bundle auto-starts when the
|
||||
* running one finishes, and a bundle queued while an import holds the lock
|
||||
* starts once the import route releases it.
|
||||
|
||||
@@ -135,7 +135,7 @@ describe("extract-zip (pure JS, no skipIf)", () => {
|
||||
// Same binary-patch technique as the "../evil.txt" case above, but with a
|
||||
// multi-segment ".." path to make sure the segment-splitting check
|
||||
// (name.split(/[/\\]/).some(s => s === "..")) isn't fooled by extra
|
||||
// "../" hops -- only the first ".." segment mattering would be a bug.
|
||||
// "../" hops; only the first ".." segment mattering would be a bug.
|
||||
const zip = new AdmZip();
|
||||
const traversalName = "../../../etc/passphrase-lol";
|
||||
const placeholder = "X".repeat(traversalName.length);
|
||||
@@ -157,7 +157,7 @@ describe("extract-zip (pure JS, no skipIf)", () => {
|
||||
}, 30_000);
|
||||
|
||||
it("rejects a zip with an absolute Unix path entry (/etc/passwd) with 400", async () => {
|
||||
// Absolute paths bypass ".." detection entirely -- extract-zip must reject
|
||||
// Absolute paths bypass ".." detection entirely; extract-zip must reject
|
||||
// entry names starting with "/" on their own (preValidate's
|
||||
// name.startsWith("/") branch), independent of the ".." segment check.
|
||||
const zip = new AdmZip();
|
||||
|
||||
@@ -640,7 +640,7 @@ async function main() {
|
||||
format: ext,
|
||||
status: statusCode,
|
||||
outputOk: false,
|
||||
note: `BUG: custom body route returned ${statusCode} -- ${body.slice(0, 300)}`,
|
||||
note: `BUG: custom body route returned ${statusCode}. ${body.slice(0, 300)}`,
|
||||
};
|
||||
results.push(r);
|
||||
bugs.push(r);
|
||||
@@ -673,7 +673,7 @@ async function main() {
|
||||
outputOk: verification.ok,
|
||||
note: verification.ok
|
||||
? `pass: ${verification.detail}`
|
||||
: `BUG: corrupt success -- ${verification.detail}`,
|
||||
: `BUG: corrupt success. ${verification.detail}`,
|
||||
};
|
||||
results.push(r);
|
||||
if (verification.ok) {
|
||||
@@ -691,7 +691,7 @@ async function main() {
|
||||
format: ext,
|
||||
status: "network-error",
|
||||
outputOk: false,
|
||||
note: `BUG: custom body request failed -- ${msg.slice(0, 200)}`,
|
||||
note: `BUG: custom body request failed. ${msg.slice(0, 200)}`,
|
||||
};
|
||||
results.push(r);
|
||||
bugs.push(r);
|
||||
@@ -817,7 +817,7 @@ async function main() {
|
||||
} catch {}
|
||||
const msg = parsed.error || parsed.details || body.slice(0, 200);
|
||||
const fullMsg =
|
||||
[parsed.error, parsed.details].filter(Boolean).join(" -- ") || body.slice(0, 300);
|
||||
[parsed.error, parsed.details].filter(Boolean).join(" | ") || body.slice(0, 300);
|
||||
|
||||
// Check if this format is in the tool's own acceptedInputs
|
||||
const isSelfFormat = tool.acceptedInputs.includes(ext);
|
||||
@@ -993,7 +993,7 @@ async function main() {
|
||||
outputOk: verification.ok,
|
||||
note: verification.ok
|
||||
? `pass: ${verification.detail}`
|
||||
: `BUG: corrupt success -- ${verification.detail}`,
|
||||
: `BUG: corrupt success. ${verification.detail}`,
|
||||
};
|
||||
results.push(r);
|
||||
|
||||
|
||||
@@ -8,8 +8,10 @@
|
||||
# Project/container names default to snapotter-qa. Two sessions on the same host
|
||||
# running this file verbatim at the same time will silently steal each other's
|
||||
# container (last `up` wins, no error) since container_name is fixed rather than
|
||||
# derived from the project name. If you need a second concurrent stack, override:
|
||||
# QA_PROJECT_NAME=snapotter-qa-2 docker compose -f tests/qa/docker-compose.qa.yml up -d
|
||||
# derived from the project name. A second concurrent stack also needs its own
|
||||
# host port (the default stack holds 13499), so override both:
|
||||
# QA_PROJECT_NAME=snapotter-qa-2 QA_APP_PORT=13498 \
|
||||
# docker compose -f tests/qa/docker-compose.qa.yml up -d
|
||||
name: ${QA_PROJECT_NAME:-snapotter-qa}
|
||||
|
||||
services:
|
||||
@@ -17,7 +19,7 @@ services:
|
||||
image: snapotter/snapotter:latest
|
||||
container_name: ${QA_PROJECT_NAME:-snapotter-qa}
|
||||
ports:
|
||||
- "13499:1349"
|
||||
- "${QA_APP_PORT:-13499}:1349"
|
||||
volumes:
|
||||
- qa-data:/data
|
||||
- qa-workspace:/tmp/workspace
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
// Drives the REAL in-app AI bundle install flow (Settings > AI Features > Install
|
||||
// All) against a running Docker container, exactly as a user would click through
|
||||
// it. This is the primary AI-install path per docs/prompts/engineering/QA_PROMPT.md
|
||||
// Phase 2 -- the curl-based /api/v1/admin/features/<bundle>/install route is a
|
||||
// Phase 2: the curl-based /api/v1/admin/features/<bundle>/install route is a
|
||||
// verification/fallback path only, never the primary install.
|
||||
//
|
||||
// This script only KICKS OFF the install and captures pre/mid-install evidence; the
|
||||
@@ -47,11 +47,11 @@ async function main() {
|
||||
console.log(" already authenticated (no login form shown)");
|
||||
}
|
||||
|
||||
// Bail loudly if we land on the forced password-change screen -- the caller
|
||||
// Bail loudly if we land on the forced password-change screen; the caller
|
||||
// should set SKIP_MUST_CHANGE_PASSWORD=true for automated runs.
|
||||
if (page.url().includes("/change-password")) {
|
||||
throw new Error(
|
||||
"Landed on /change-password -- set SKIP_MUST_CHANGE_PASSWORD=true on the container for automated QA runs",
|
||||
"Landed on /change-password; set SKIP_MUST_CHANGE_PASSWORD=true on the container for automated QA runs",
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -1251,7 +1251,7 @@ describe("bridge - initDispatcher", () => {
|
||||
// pipe that teardown can surface as an EPIPE/ERR_STREAM_DESTROYED on the
|
||||
// stdin stream (observed in the container shutdown log as a spurious
|
||||
// "[bridge] Dispatcher crash #1"). The "close" handler alone is guarded,
|
||||
// but the stdin error handler must be too -- otherwise shutdownDispatcher()
|
||||
// but the stdin error handler must be too; otherwise shutdownDispatcher()
|
||||
// on every AI bundle install accrues false crashes toward the disable cap.
|
||||
const mock = createMockProcess();
|
||||
vi.mocked(spawn).mockReturnValue(mock.process);
|
||||
|
||||
@@ -366,7 +366,7 @@ describe("applyCorrections pipeline (CLAHE + normalise + gamma)", () => {
|
||||
const claheDisabledBuf = await claheDisabled.toBuffer();
|
||||
|
||||
// Skipping CLAHE via the size cap must produce byte-identical output to
|
||||
// skipping it via the explicit toggle -- proof the cap actually took effect.
|
||||
// skipping it via the explicit toggle: proof the cap actually took effect.
|
||||
expect(Buffer.compare(overCapBuf, claheDisabledBuf)).toBe(0);
|
||||
});
|
||||
|
||||
@@ -380,7 +380,7 @@ describe("applyCorrections pipeline (CLAHE + normalise + gamma)", () => {
|
||||
denoise: 0,
|
||||
};
|
||||
// Use the real buffer's actual 200x150 dimensions (30,000 px, well under
|
||||
// the 16M cap) -- CLAHE's tile size is derived from imageSize, and Sharp
|
||||
// the 16M cap). CLAHE's tile size is derived from imageSize, and Sharp
|
||||
// rejects a tile window larger than the real underlying image, so a fake
|
||||
// imageSize far bigger than the actual small test buffer isn't valid here
|
||||
// (that's exactly what the "above the cap" test above uses instead,
|
||||
@@ -408,7 +408,7 @@ describe("applyCorrections pipeline (CLAHE + normalise + gamma)", () => {
|
||||
);
|
||||
const claheDisabledBuf = await claheDisabled.toBuffer();
|
||||
|
||||
// Under the cap, CLAHE should still run -- output must differ from the
|
||||
// Under the cap, CLAHE should still run: output must differ from the
|
||||
// contrast-disabled baseline.
|
||||
expect(Buffer.compare(underCapBuf, claheDisabledBuf)).not.toBe(0);
|
||||
});
|
||||
|
||||
@@ -202,7 +202,7 @@ describe("useFeaturesStore (expanded)", () => {
|
||||
|
||||
await useFeaturesStore.getState().installBundle("done-bundle");
|
||||
|
||||
// The client always POSTs now -- the server owns the queue/dedup decision.
|
||||
// The client always POSTs now; the server owns the queue/dedup decision.
|
||||
expect(apiPostMock).toHaveBeenCalledWith("/v1/admin/features/done-bundle/install", {});
|
||||
// A 409 already-installed clears installing + error and refreshes silently.
|
||||
const state = useFeaturesStore.getState();
|
||||
|
||||
@@ -448,6 +448,17 @@ describe("useFeaturesStore", () => {
|
||||
}),
|
||||
);
|
||||
|
||||
// The poll for the queued bundle reads server state from GET /v1/features.
|
||||
let bundleBStatus: FeatureBundleState["status"] = "queued";
|
||||
apiGetMock.mockImplementation(() =>
|
||||
Promise.resolve({
|
||||
bundles: [
|
||||
makeBundleState({ id: "bundle-a", status: "installing" }),
|
||||
makeBundleState({ id: "bundle-b", status: bundleBStatus }),
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
await useFeaturesStore.getState().installAll();
|
||||
|
||||
await vi.waitFor(() => {
|
||||
@@ -456,21 +467,32 @@ describe("useFeaturesStore", () => {
|
||||
});
|
||||
expect(useFeaturesStore.getState().installing["bundle-b"]).toBeUndefined();
|
||||
|
||||
// Finish both; the second transitions queued -> installing on its first
|
||||
// progress frame, then completes.
|
||||
const esB = FakeEventSource.instances.find((es) => es.url.includes("bundle-b"));
|
||||
esB?.onmessage?.({ data: JSON.stringify({ phase: "installing", percent: 10, stage: "Go" }) });
|
||||
await vi.waitFor(() => {
|
||||
expect(useFeaturesStore.getState().queued).not.toContain("bundle-b");
|
||||
});
|
||||
// A queued bundle must NOT hold an SSE connection open (it polls
|
||||
// instead): only the actively-installing bundle has an EventSource.
|
||||
expect(FakeEventSource.instances).toHaveLength(1);
|
||||
expect(FakeEventSource.instances[0].url).toContain("bundle-a");
|
||||
|
||||
for (const es of FakeEventSource.instances) {
|
||||
es.onmessage?.({ data: JSON.stringify({ phase: "complete" }) });
|
||||
}
|
||||
await vi.waitFor(() => {
|
||||
expect(useFeaturesStore.getState().installAllActive).toBe(false);
|
||||
});
|
||||
}, 15000);
|
||||
// The server starts bundle-b; its poll moves it queued -> installing.
|
||||
bundleBStatus = "installing";
|
||||
await vi.waitFor(
|
||||
() => {
|
||||
expect(useFeaturesStore.getState().queued).not.toContain("bundle-b");
|
||||
expect(useFeaturesStore.getState().installing["bundle-b"]).toBeDefined();
|
||||
},
|
||||
{ timeout: 8000 },
|
||||
);
|
||||
|
||||
// Finish both: bundle-a via its SSE stream, bundle-b via its poll
|
||||
// observing the terminal server status.
|
||||
bundleBStatus = "installed";
|
||||
FakeEventSource.instances[0].onmessage?.({ data: JSON.stringify({ phase: "complete" }) });
|
||||
await vi.waitFor(
|
||||
() => {
|
||||
expect(useFeaturesStore.getState().installAllActive).toBe(false);
|
||||
},
|
||||
{ timeout: 8000 },
|
||||
);
|
||||
}, 25000);
|
||||
|
||||
it("retries a bundle once if it fails during Install All, then stops", async () => {
|
||||
const bundles = [makeBundleState({ id: "flaky", status: "not_installed" })];
|
||||
|
||||
Reference in New Issue
Block a user