diff --git a/.comfyignore b/.comfyignore index d943fef..36b573d 100644 --- a/.comfyignore +++ b/.comfyignore @@ -4,6 +4,7 @@ pages/ promo/ .native-test/ docs/archive/ +docs/audits/ __pycache__/ *.pyc AGENTS.local.md diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 0c27197..cc0f521 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -16,6 +16,7 @@ jobs: if: github.repository_owner == 'doubletwisted' steps: - uses: actions/checkout@v4 - - uses: Comfy-Org/publish-node-action@main + # Reviewed upstream main on 2026-11-07; re-review before updating. + - uses: Comfy-Org/publish-node-action@d2366e7abb6ab16f3bb03e3520ae25c8cf749bc9 with: personal_access_token: ${{ secrets.REGISTRY_ACCESS_TOKEN }} diff --git a/CHANGELOG.md b/CHANGELOG.md index c0bb67e..08e60c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,154 @@ All notable changes to Spool are recorded here. Versions follow [semantic versioning](https://semver.org/). +## Unreleased + +### Tests + +- Native browser checks cover permanent take deletion with pending and failed + saves, plus completed OTIO downloads and export failures. They verify project + revisions, retained file bytes, downloaded timelines and blob URL replacement. + +### Fixes + +- OTIO export retries transient Windows sharing denials and removes only its own + unfinished file if writing or publication fails. +- Comparison sheets use temporary frames that are cleaned after success or + failure. Saved sheets and explicitly exported frames remain available. +- FFmpeg errors retain Unicode filenames on Windows instead of failing while + decoding diagnostics during audio preparation, export or cut detection. +- Cancellation preflight reads avoid writer locks and checkpoint rewrites; + cancellation plans and delivery acknowledgements keep their transactions. +- Footage with many cuts imports without Windows command or FFmpeg expression + limits, while keeping exact shot frames and short keyframe intervals for seeking. + The existing FFmpeg/ffprobe 5.1 minimum is documented. +- Save and capture validate clip, take and workflow containers before staging + input files, returning request errors for invalid types. Demo save and take + validation replies include matching error codes. +- Unresolved-save messages also apply to pending actions such as take deletion. +- Transition buttons escape project track values, preventing a crafted track + from executing script when the timeline opens. +- Failed imports remove only their own new working cache, preserving existing or + redirected caches and the original import error when cleanup is denied. Cache + path setup failures also remove the import's empty destination. +- Storage reports include project backups from their managed cache directory. +- Slow launcher settings, startup-file scans and process creation run in workers, + keeping the host responsive. Delayed state updates preserve newer startup + reservations, and cancellation waits for an in-flight launch to finish. +- Local, Deadline and Cloud collection own their temporary files, so concurrent + collectors cannot overwrite or remove each other's unfinished copies. +- Finding an existing Deadline job reports an unavailable service with a retry + action when its recovery lookup fails. +- Missing required revision/request fields return validation errors before the + operation starts, including the save and take actions simulated by the demo. +- Manual reconciliation and background collection serialize work for each + project, preventing duplicate artifact copies. Shutdown waits for collection + workers to finish before releasing ownership. +- Deadline submission grace no longer includes time waiting in the queue or + preparing inputs, so old queued takes can still be submitted. +- Maintenance dry runs scan a private snapshot, so a long report does not block + another editor's save on Windows. +- Invalid or nonfinite probed frame rates produce a clear validation error. +- Cloud observation batches settle before reporting failures; cancellation + preserves other errors and always releases executor ownership on shutdown. + +- The local demo server rejects inverted or out-of-bounds byte ranges with a + complete 416 response instead of a negative content length. + +- Cloud key status follows the effective saved or environment credential, + including empty or whitespace-only values. +- Registry publishing pins its external action to a reviewed commit. + +- Failed input staging and export muxing remove their own partial files while + preserving published inputs, successful exports and the original error. + +- Linked Comfy image-loader choices refresh when a workflow opens or reloads, + keeping valid staged images selectable without changing the saved input. +- Newly described image/video dimensions follow EXIF and display-matrix + orientation. Existing media records and originals are unchanged. +- Path-imported video copies keep project-relative original references, so + moving a project also preserves its shot and audio source references. +- Joined extension audio follows the measured frame shift and sequence frame + rate, keeping its duration aligned with the joined video. +- Snapshot publications carry an identity inside the SQLite snapshot, so + same-size saves with coarse timestamps do not hide newer revisions. +- Demo take deletion follows host media-pool and generation-input protection + rules and removes aliases of a deleted original. + +- New clip output folders stay distinct when names differ only by case, and + Windows device names and truncated trailing spaces/dots are made portable. + Existing assigned output folders are preserved. +- Queue preparation validates every item and source before freezing inputs, + so an invalid later item does not leave copies from earlier items. + +- Authored JSON that impersonates a project evidence blob is rejected before + saving, so an imported value cannot leave the project unreadable. + +- Comparing takes and detecting cuts in a take whose original lies outside the + project and Comfy folders now answer only this computer, as downloading that + original already did. Another machine could otherwise read frames of any image + or video on the Spool computer. +- `python -m spool` agent commands reach a Spool on this computer directly when + `http_proxy` is set for other traffic, instead of failing through the proxy. +- **Find existing Deadline job** works again. It failed on every call, so a + Deadline render whose submission was uncertain could not be resolved. +- A Deadline receipt whose artifact or execution ID is not the ID Spool's worker + writes is rejected. A forged ID could make collection copy a file outside the + project. +- A Deadline receipt that is valid JSON of the wrong shape is recorded as invalid + instead of stopping collection of every later receipt and project. +- Reopening a project after Spool stopped in the middle of a save restores the + last published snapshot. The interrupted save could be rolled into the fresh + copy and, on the next save, undo work published from another computer. +- Applying a workflow or using a take no longer stores another copy of the + project's workflows each time. Repeated without a save in between, it grew the + project until it could not be opened. +- `python -m spool prune --confirm` no longer removes previews the edit uses when + their file names contain spaces or accented letters. +- Opening a project no longer runs script hidden in its clip, take or media IDs, + take numbers or track numbers. The studio now escapes them like names, so an + imported project or shared project folder cannot act as the studio. +- `run_spool.bat` finds Spool with the Windows portable build's Python, whose + `._pth` file leaves the Spool folder off Python's module path. +- Importing footage again after an interrupted import works. A review copy, poster + or waveform cut short by a timeout or a stopped Spool was kept and reused, so + that file could never be imported again. +- MP4 export works for a Windows account whose name contains an apostrophe. +- **Delete from disk** saves your edits first. An edit not yet saved was replaced + by the reply, and one still saving made the delete report a conflict. +- A Deadline task that the farm retried no longer adds its second render to the + clip as an extra take; the accepted result stays the only take. +- A project folder Spool cannot open, such as one a newer Spool upgraded, is + listed as unavailable instead of stopping the project list and every other + project's renders. A folder whose upgrade fails is retried less and less often, + not every few seconds with a new backup each time. +- `python -m spool prune --confirm` and `reclaim --confirm` keep their working copy + in a private temporary folder and remove it afterwards. Another user of the + computer could plant a database at the shared path they used, which a run then + published into the project. +- Projects created, opened or imported at the same moment all stay in the project + list. One could drop another from the list, leaving its folder unreachable. +- Files whose names contain a percent sign followed by two hex digits, such as + `a%41.mp4`, play, export and collect. Their names were decoded a second time + and pointed at a different file. +- The studio shows footage whose file name contains `#`, `?` or `%`, such as an + imported `Take #3.mp4`. Its address was cut at that character. +- A Comfy Cloud workflow whose result is an image is refused before it renders. + Cloud results are collected as video, so the paid render could never arrive. +- Importing a file that is not readable media says so and keeps nothing, instead + of a generic server error with the file left in the project. +- A project folder at the root of a drive or network share (`E:\`, + `\\nas\projects\`) can use its media. Every path in it was refused as outside + the project. +- `python -m spool command` reports a missing or invalid `--file` as a JSON error + (`invalid_request`) instead of a Python traceback. +- A waveform is drawn again when its audio file is replaced at the same path, + instead of showing the old sound. +- Standalone Spool starts, and its Comfy connection status loads, from a checkout + close to the drive root such as `C:\Spool` or `/opt/spool`. +- On macOS and Linux the saved Comfy Cloud key file is private from the moment it + is written; it was briefly readable by other users of the computer. + ## 0.1.1 (2026-11-03) ### Directing tools diff --git a/README.md b/README.md index b7c2bec..4a76826 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ review takes and change the edit there; generating takes needs Spool with ComfyU - ComfyUI 0.39.0 or newer (tested with 0.43.1 / frontend 1.57.4 on Windows, and 0.43.0 / frontend 1.57.4 on Apple Silicon) - Python 3.11 or newer -- `ffmpeg` and `ffprobe` on the PATH of the process running ComfyUI (on macOS, +- `ffmpeg` and `ffprobe` 5.1 or newer on the PATH of the process running ComfyUI (on macOS, Spool also finds a Homebrew or MacPorts install) - For Comfy Cloud: a Comfy API key diff --git a/docs/audits/2026-11-04-full-audit.md b/docs/audits/2026-11-04-full-audit.md new file mode 100644 index 0000000..9eb4c77 --- /dev/null +++ b/docs/audits/2026-11-04-full-audit.md @@ -0,0 +1,402 @@ +# Full audit, 2026-11-04 + +## Summary + +A read of the whole repository (backend, studio, Comfy page extension, demo, +launchers, CI and docs) found 44 problems that could be shown to be real. 29 are +fixed on branch `claude/full-audit-2026-11-04-hta0pw`, each in its own commit and +with a regression test where one can run in the gate. 11 need an owner decision, +because the fix would change documented behaviour or a stored format, or because +it is a trade-off. 4 are plausible but could not be confirmed here. + +| Severity | Fixed | Needs decision | Unconfirmed | Total | +| --- | --- | --- | --- | --- | +| Critical | 0 | 0 | 0 | 0 | +| High | 6 | 1 | 0 | 7 | +| Medium | 12 | 4 | 0 | 16 | +| Low | 11 | 6 | 4 | 21 | + +The most serious problems were: + +- **Host files reachable from another machine.** Routes that are not local-only + could read any image or video on the Spool computer: one was fixed (F-01), the + larger one needs a decision (N-01). +- **A file write outside the project** from a forged Deadline receipt (F-02). +- **A crashed save rolling back another computer's work** (F-04). +- **Stored XSS** through project IDs and take numbers (F-05). +- **Find existing Deadline job** failing on every call (F-03). + +## Methodology + +1. Read `README.md`, `AGENTS.md`, `CLAUDE.md`, `CONTRIBUTING.md`, `docs/` and + `tests/`, then installed the gate's tools as CI does + (`python -m pip install aiohttp pillow -r requirements.txt -r requirements-dev.txt` + and `npm ci`) and ran every gate command separately to get a full baseline + instead of stopping at the first failure. +2. Reviewed the HTTP surface (`spool/routes.py`, local-control guard, entry points, + CLI) directly, and ran four parallel read-only reviews of persistence, execution + targets, media/host code and browser JavaScript. Every reported finding was + re-run or re-read before it was accepted; two candidates were dropped as false + (a "missing" public route that is documented, and an `executionId` traversal + that the receipt namespace check already stopped). +3. For each confirmed defect: a failing test or repro first, the smallest fix, the + test run with and without the fix, then the full gate before the commit. +4. Kept public behaviour and file formats unchanged. Where a fix would change + either, it is listed under _Needs decision_ instead. + +### What could and could not run in this sandbox (Linux, no GPU) + +- **Ran:** the full gate (ruff, Prettier, ESLint, knip, `tsc`, every + `tests/*_checks.mjs` and `tests/test_*.py`), ffmpeg/ffprobe media repros, and a + standalone `python -m spool` host driven by headless Chromium for the studio XSS + and media-address checks (live studio evidence, standalone host). +- **Node 22.23** here; the repository and CI use Node 24. All Node checks passed. +- **Not run:** browser checks and live checks against ComfyUI (no ComfyUI install + or GPU), Deadline and Comfy Cloud (no farm or key; paid checks are opt-in), and + Windows or macOS. Deadline and Cloud findings use the repository's own fixtures + with `deadlinecommand` and the Cloud SDK patched: they are mocked results, not + live evidence. Windows-only findings are marked _review-only_; where the cause + can be shown on Linux (F-09 with `python -I`, F-21 with `/`) it was. + +## Findings + +Lines refer to the code before the fix (`main` at `7caf5c9`). + +| ID | Severity | Area | Location | Finding | Status | +| --- | --- | --- | --- | --- | --- | +| F-01 | High | Security | `spool/routes.py:418`, `:938` | `/comparison` and `/detect-cuts` read takes' originals outside the project for non-local callers | fixed | +| F-02 | High | Security / Deadline | `spool/farm_protocol.py:177`, `spool/farm.py:156` | A receipt's `artifactId` names the ingestion copy's path: file write outside the project | fixed | +| F-03 | High | Deadline | `spool/commands.py:525` | `recover_submission` (**Find existing Deadline job**) raises `UnboundLocalError` on every call | fixed | +| F-04 | High | Data loss | `spool/projects.py:115` | A crashed save's `-journal` is rolled into the fresh working copy, undoing other computers' work | fixed | +| F-05 | High | Security / studio | `ui/timeline-renderer.js:299` and 27 more sinks | Stored XSS through clip/take/media IDs, take and track numbers | fixed | +| F-06 | Medium | Data loss | `spool/store.py:657` | `prune --confirm` deletes previews the edit uses when names hold spaces or non-ASCII | fixed | +| F-07 | Medium | Deadline | `spool/farm.py:130`, `spool/collection.py:76` | A receipt of the wrong JSON shape aborts every scan | fixed | +| F-08 | Medium | Persistence | `spool/store.py:1232` | Each capture re-wraps the packed document until reads hit `RecursionError` | fixed | +| F-09 | High | Launcher (Windows, review-only) | `run_spool.bat:17` | The portable build's `._pth` Python cannot find `spool` with `-m` | fixed | +| F-10 | Medium | Media | `spool/media.py:384`, `:444`, `:459` | An interrupted proxy/poster/waveform encode is reused forever | fixed | +| F-11 | Medium | Export | `spool/export.py:85` | MP4 export fails when the temp path holds an apostrophe | fixed | +| F-12 | Medium | Studio | `ui/take-actions.js:197` | **Delete from disk** replaces an unsaved draft or reports a false conflict | fixed | +| F-13 | Medium | Deadline | `spool/store.py:1158` | A retried farm task's copy becomes a second take | fixed | +| F-14 | Medium | Persistence | `spool/projects.py:342`, `:362` | One unopenable folder stops listing and rendering for every project; failing upgrades back up every tick | fixed | +| F-15 | Medium | Security | `spool/projects.py:54` | Confirmed maintenance trusts a working copy at a predictable shared temp path | fixed | +| F-16 | Low | Concurrency | `spool/projects.py:323`, `:392` | Concurrent create/open/import drop each other's registration | fixed | +| F-17 | Medium | Media | `spool/media.py:139` | Paths are URL-decoded twice (`a%41.mp4` resolves to `aA.mp4`) | fixed | +| F-18 | Medium | Studio | `ui/editor.js:232` | Media addresses are not encoded; a `#` in a file name breaks playback | fixed | +| F-19 | Medium | Cloud | `spool/requests.py:207`, `spool/cloud.py:189` | Cloud renders with an image result are paid for and never collected | fixed | +| F-20 | Low | Media / routes | `spool/media.py:34`, `spool/routes.py:878` | Unreadable media answers 500 and the upload is kept | fixed | +| F-21 | Low | Paths (Windows, review-only) | `spool/media.py:140`, `spool/routes.py:101` | A project at a drive or share root refuses every media path | fixed | +| F-22 | Low | CLI | `spool/cli.py:51` | A missing or invalid `--file` prints a traceback, not the documented JSON error | fixed | +| F-23 | Low | Studio | `ui/exports.js:277` | Timeline export leaks a blob URL per export | fixed | +| F-24 | Low | Media | `spool/media.py:453` | Waveform cache ignores size and mtime: replaced audio keeps its old waveform | fixed | +| F-25 | Low | Startup | `spool/launcher.py:152`, `spool/__main__.py:19`, `spool/maintenance.py:11` | A shallow checkout raises `IndexError` | fixed | +| F-26 | Low | Security | `spool/credentials.py:37` | The Cloud key file is briefly created with umask permissions | fixed | +| F-27 | Low | CLI | `spool/cli.py:58` | `http_proxy` captures CLI requests to 127.0.0.1 | fixed | +| F-28 | Low | Tests | `tests/test_take_files.py:339`, `:360` | Clip-folder tests depend on `xdg-open`; one fails headless, one passes for the wrong reason | fixed | +| F-29 | Low | Docs | `docs/guide/deadline.md:54`, `docs/dev/durable-names.md:27`, `docs/dev/architecture.md:89` | Three docs contradict the code | fixed | +| N-01 | High | Security | `spool/requests.py:292-362`, `spool/routes.py:827` | Generation preview and queue bake host files for non-local callers; the copy is then served | needs-decision | +| N-02 | Medium | Security | `spool/routes.py:183` | DNS rebinding reaches every route that is not local-only | needs-decision | +| N-03 | Medium | Persistence | `spool/projects.py:96` | Snapshot change detection is `(mtime_ns, size)`: stale reads and lost updates on coarse timestamps | needs-decision | +| N-04 | Medium | Media / docs | `spool/routes.py:896`, `spool/media.py:567`, `docs/guide/editing.md:153` | Import by path copies the video, contradicting the guide, and records an absolute path to the copy | needs-decision | +| N-05 | Medium | Media | `spool/media.py:333`, `:371` | Rotated video and EXIF-rotated images report coded, not display, size | needs-decision | +| N-06 | Low | Media | `spool/joins.py:332` | An extension's joined copy keeps the render's audio without retiming it | needs-decision | +| N-07 | Low | Paths | `spool/projects.py:27`, `:258` | Clip output folders collide case-insensitively; a name can end in a space | needs-decision | +| N-08 | Low | Persistence | `spool/store.py:457` | A stored value shaped like `{"__spool_blob__": …}` makes the project unreadable | needs-decision | +| N-09 | Low | CI / supply chain | `.github/workflows/publish.yml:20` | `Comfy-Org/publish-node-action@main` runs unpinned with the registry token | needs-decision | +| N-10 | Low | Demo | `pages/demo-api.js:85` | The demo's delete-takes ignores media-pool and generation-input rules | needs-decision | +| N-11 | Low | Execution | `spool/requests.py:425` | A rejected queue request leaves its frozen input copies behind | needs-decision | +| U-01 | Low | Deadline (review-only) | `spool/farm.py:86` | A submission is timed from attempt creation and may be marked uncertain while still submitting | unconfirmed | +| U-02 | Low | Windows (review-only) | `spool/store.py:771` | A dry-run `prune`/`reclaim` holding `project.spool` open may make a live publish fail | unconfirmed | +| U-03 | Low | Comfy page | `web/template-browser.js:13` | Any loopback origin, on any port, may frame the template chooser and receive the capture | unconfirmed | +| U-04 | Low | Export | `spool/export.py:13` | A fixed 180 s ffmpeg timeout surfaces as a 500 for long exports | unconfirmed | + +## Details of fixed findings + +Each fix commit names the finding in its message. "Test" names the regression +test; it fails without the fix and passes with it. + +**F-01 Comparison and cut detection read outside footage for remote callers.** +`/frame` and `/take-file` refuse footage outside the project and Comfy folders to +non-local callers, as the security guide promises, but `/comparison` and +`/detect-cuts` read the same originals. A client on another machine (or a +DNS-rebound page) could create a project whose take points at +`/home/user/private.png`, request a comparison and download the PNG sheet +(reproduced: 403 from `/frame`, 200 and the image from `/comparison`). Both routes +now apply the same rule. Test: `test_security.RouteGuardTests.test_frames_of_footage_outside_the_project_stay_on_this_computer`. + +**F-02 Forged receipt IDs steer the ingestion copy.** `farm_protocol.verify` +checked only that artifact IDs were unique; `Media.result_path` joins them into +the destination. A receipt with `artifactId: "../../../../ESCAPED"` wrote +`ESCAPED.png` outside the store and the attempt completed. Anyone who can write +the farm share could choose the content and the place. Both IDs must now be the +32-hex `uuid4` the worker writes. Test: +`test_farm_protocol…test_receipt_identities_cannot_name_files_outside_the_attempt`. + +**F-03 Find existing Deadline job.** `apply()` assigns `attempt` and `submission` +in its cancel branch, which makes both local to the whole function, so the +recover branch read unbound names (`UnboundLocalError` with and without a matching +job). An uncertain Deadline submission could not be resolved. Test: +`test_farm_protocol…test_find_existing_deadline_job_adopts_it_or_reports_still_uncertain`. + +**F-04 Stale rollback journal.** The working copy refresh deleted the database and +its `-wal`/`-shm` but not `-journal`. After a writer died mid-transaction, the +journal was rolled into the newly copied snapshot and the next save published a +rolled-back project over another computer's revisions (reproduced with a real +crashed process). Test: `test_project_folders.CrashRecoveryTests`. + +**F-05 Stored XSS.** IDs, workflow keys, take numbers and track numbers were joined +into `innerHTML` raw, next to escaped names. Imported projects and shared folders +keep them as written. Live check on a standalone host with headless Chromium: the +unpatched studio ran injected handlers from a clip ID and a take number; patched, +none ran. Inside ComfyUI the studio shares Comfy's origin. Test: +`tests/html_escape_checks.mjs`, a gate check that fails when one of these fields +reaches markup without `esc()` (it reported all 28 sites before the fix). + +**F-06 Prune deletes referenced previews.** References were collected by an ASCII +token regex over stored JSON, so `Desert walk-….mp4` and `café-….mp4` (stored as +`café`) never matched and were evicted. Whole decoded JSON strings are now +matched as well. Test: `test_native…test_prune_keeps_referenced_previews_named_with_spaces_or_accents`. + +**F-07 Wrong-shape receipts.** `{"artifacts": ["x"]}` raised `TypeError` past the +handlers that catch only `ValueError`/`KeyError`/`OSError`, aborting the scan for +every later receipt and project; the guide promises an invalid receipt cannot hide +a later valid one. Test: `test_farm_protocol…test_receipt_of_the_wrong_shape_does_not_block_a_later_valid_execution`. + +**F-08 Capture re-wrapping.** `Store.capture` loaded the packed document with +`json.loads` and saved it, adding one blob layer per capture; after about 990 +captures without an ordinary save, reads raised `RecursionError`. Test: +`test_store_interface.RepeatedCaptureTests`. + +**F-09 `run_spool.bat` on the portable build (review-only).** A `._pth` file +makes CPython leave the current folder off `sys.path`, so `python -m spool` failed +with "No module named spool". The script now adds the folder and runs the package +with `runpy`. Shown on Linux with an embeddable-style interpreter and with +`python -I`, which has the same effect. Not run on Windows. Test: +`test_launcher.StandaloneScriptTests`. + +**F-10 Interrupted encodes.** `describe()` encoded straight to the cache name and +reuses any file there; a timeout left a 4.9 MB truncated MP4 and every later +describe failed. Encodes now go to a unique pending name and are renamed into +place. Test: `test_media.InterruptedEncodeTests`. + +**F-11 Apostrophe in the concat list.** `file ''` was written unescaped; with +a temp folder under `O'Brien`, ffmpeg opened a different path. Test: +`test_media.ExportPathTests`. + +**F-12 Delete from disk without flushing.** `projectEdit.change()` accepts the +reply when the draft did not change during the request, which replaced an edit +left unsaved after a failed save (mocked transport repro: the retried save wrote +the old name). The action now flushes first, as the other authored actions do; +`flush()` refuses while the draft is unsaved. No gate test: the take actions run +only in browser checks. + +**F-13 Retried farm task as a second take.** Collection copies every execution's +primary into the clip folder, but the folder inventory knew only the winning +attempt's artifacts. Every recorded execution's artifacts are now known. Test: +`test_project_folders…test_a_retried_farm_task_does_not_become_a_second_take`. + +**F-14 One unopenable folder.** `list`, `project_stores` and `lookup` raised on a +folder in a newer format, stopping the collector, dispatcher and Cloud loop for +all projects; a failing upgrade wrote a full backup on every 2 s scan (10 in +20 s). Such a folder is now listed as unavailable and skipped, with retries backing +off from 5 s to an hour unless its files change. Tests: +`test_project_folders.UnavailableFolderTests`. + +**F-15 Shared maintenance cache.** Without a workspace, `prune`/`reclaim +--confirm` used `/spool-maintenance/` and trusted its +working copy; a planted database was published into the project. Writable runs now +use a private `mkdtemp` folder, removed afterwards. Test: +`test_project_folders.MaintenanceCacheTests`. + +**F-16 Registration race.** `project_folders` was read and written back separately +from worker threads; 8 concurrent imports left 4–6 listed. It is now one +`BEGIN IMMEDIATE` transaction. Test: +`test_project_folders…test_projects_imported_at_once_all_stay_listed` (the race +fails it in most unfixed runs; fixed, it passes every run). Two existing import +tests used `set_setting` as their seam and were pointed at `register_folder`. + +**F-17 Double URL decoding.** aiohttp decodes route paths once (including `%2F`); +`contained()` decoded again. `a%41.png` resolved to `aA.png`, and an upload named +`..%252F..%252Foutput%252Fclip%252F…` landed in a clip's output folder. Tests: +`test_uploads…test_upload_names_are_decoded_once`, +`…test_stored_names_holding_a_percent_sign_resolve_to_that_file`. + +**F-18 Unencoded media addresses.** `url()` appended project paths unencoded; +an imported `Take #3.png` requested `…/Take ` (400, broken image). Each segment is +now encoded. Live check on a standalone host: 400 before, 200 after. No gate test. + +**F-19 Cloud image results.** Freezing accepted SaveImage/PreviewImage for Cloud, +but Cloud collection accepts only video files, so the job rendered (paid) and then +polled as `collection_pending` forever. It is now refused when queued, and the +guide says so. Test: `test_queue_operations…test_cloud_refuses_an_image_result_it_could_never_collect`. + +**F-20 to F-29, low.** + +- F-20: ffprobe failures are a 400 (still a `SubprocessError` for existing + handlers), and the upload is deleted. The message no longer carries ffprobe's + absolute path. +- F-21: containment strips a root's trailing separator. +- F-22: the CLI reports `invalid_request` for an unreadable or invalid + `--file` (guide and skill updated). +- F-23: the previous timeline blob URL is revoked when an export replaces it. +- F-24: the waveform key includes size and mtime. +- F-25: missing parents mean "not installed under ComfyUI". +- F-26: the key file is created `0600` with `os.open`. +- F-27: the CLI bypasses proxies for loopback hosts. +- F-28: both clip-folder tests stub the file manager. +- F-29: docs corrected. + +Tests for F-20 to F-28 are in `test_uploads`, `test_unc_paths.RootFolderTests`, +`test_cli`, `test_media.WaveformCacheTests`, `test_launcher.ShallowCheckoutTests`, +`test_credentials` and `test_take_files`. F-23 has no gate test. + +## Needs owner decision + +**N-01 (High) Generation preview and queue copy host files for remote callers.** +Repro (`/generation-preview`, open route): create a project whose original take +is any image on the host, capture a one-loader workflow with a shot input, and +request a preview. Spool bakes the file into Comfy's input folder and returns its +name, and `GET /spool/api/input/` (or ComfyUI's own `/view`) serves the +pixels to that caller. The same happens through `/queue` and the command API. A +fix means refusing non-local callers a request that reads an absolute original +outside the project and Comfy folders, which would stop remote collaborators on a +`--listen` host from generating from footage imported by path. Recommendation: +apply the take-download rule (`outside_footage` in `routes.py`) to request +preparation, and document it as a local-only case. + +**N-02 (Medium) DNS rebinding.** The host-name check protects only local-only +routes. A page whose name re-resolves to 127.0.0.1 sends matching `Host` and +`Origin`, so it can read and edit projects, queue renders (spending Cloud credits) +and delete takes (reproduced: 200 on list and save, 403 on a local-only route). +ComfyUI's own routes have the same exposure. Refusing a loopback peer that names a +non-loopback host would stop it, but it breaks a same-machine reverse proxy that +preserves `Host`, a setup the security guide recommends. + +**N-03 (Medium) Snapshot signatures.** Changes are detected by `(mtime_ns, size)`, +and SQLite snapshots rarely change size. On storage with 1–2 s timestamps (FAT32, +HFS+, some SMB servers) a reader kept revision 300 after 302 was published, and a +second host saved over newer work (simulated; 0 of 300 on ext4). A per-publication +token in `project.json` would fix it, but changes the manifest. + +**N-04 (Medium) Import by path.** `docs/guide/editing.md` says this "reads a video +where it is, without copying it", but `keep_input` copies it into +`input/video/`, and the shots record an absolute `originalFile` for that copy, so +the project breaks when the folder is opened from another mount. Decide whether +to reference in place (and fix the copy) or keep the copy, then store a +project-relative path and correct the guide. + +**N-05 (Medium) Rotation.** A 64×48 video with a 90° display matrix is described +as 64×48 while every render is 48×64; an EXIF-rotated JPEG likewise. The directing +tools take generation size from these fields. Fixing it changes recorded +`width`/`height` for newly described media. + +**N-06 (Low) Joined copy audio.** A 25 fps render in a 24 fps sequence: the plain +take's audio is retimed (5.201 s) but the joined copy copies the stream (5.0 s), so +it drifts up to 0.2 s and ignores the measured shift. + +**N-07 (Low) Folder names.** `Intro` and `intro` get separate folders that are one +directory on Windows and macOS, so a dropped file joins both clips. Truncation runs +after stripping, so a name can end in a space; `CONIN$` is not reserved. A fix +changes the folder names of new clips. + +**N-08 (Low) Reserved markers.** An authored value equal to +`{"__spool_blob__": ""}` is read back as a storage reference; the save +commits and then every `get` fails. Escaping or rejecting reserved keys on write +changes what imports accept. + +**N-09 (Low) Unpinned publish action.** `publish.yml` runs +`Comfy-Org/publish-node-action@main` with `REGISTRY_ACCESS_TOKEN`. Pin it to a +commit SHA. This was not changed here, because the workflow holds a secret and +publishes releases. + +**N-10 (Low) Demo delete-takes.** The host also removes media-pool records that +share the deleted file, and refuses to delete generation inputs; the demo does +neither (demo `["media-b"]`, host `["media-b", "pool-1"]`). Mirror the rules in +`pages/demo-api.js` and `tests/demo_api_checks.mjs`, or document the simplification. + +**N-11 (Low) Frozen copies of a rejected request.** Inputs are frozen before later +items are validated; three rejected requests left three copies in +`input/spool-frozen/`. Validating every item before freezing would change the +order in which errors are reported. + +## Unconfirmed + +- **U-01:** the 30 s uncertainty timer starts at attempt creation, not at + submission (review-only). +- **U-02:** on Windows, SQLite opens files without delete sharing, so a long + dry-run report may block a live publish beyond its retry window. Not testable + here. +- **U-03:** `web/template-browser.js` accepts any loopback origin as its parent. + It needs ComfyUI to test. +- **U-04:** the exception type is certain; whether real exports exceed 180 s was + not measured. + +## Improvements not done + +These are not demonstrated defects: + +- **Request fields.** Missing request fields (`data["revision"]`, + `X-Spool-Revision`) answer 500 `internal_error` instead of 400. +- **Cloud and collection.** + - The Cloud loop's `gather` has no `return_exceptions`, so one failed observation + lets the next pass overlap the others. + - `POST /reconcile` scans alongside the loop without a lock. + - `DeadlineAdapter.recover`'s `RuntimeError` reaches the route as a 500. +- **Cloud settings.** + - `COMFY_BASE_URL` accepts `http://` for remote hosts. + - `key_status` reports `saved` for an empty key file. +- **Leftover files.** + - `stage_input` leaves its `.partial` file on failure. + - `collect` uses a fixed `.partial` name. + - A failed export mux leaves a partial MP4. + - Comparison PNGs accumulate in `exports/`. + - `interchange.py` writes with a plain `replace`. + - Atomic renames never fsync the directory. +- **Media tools.** + - Windows text-mode subprocesses decode ffmpeg output with the locale code page. + - `-force_key_frames` for about 1,800 cuts exceeds Windows' command-line limit. + - `-fps_mode` needs ffmpeg 5.1 or newer, which is not documented. + - `media._rate` would divide by zero on `0/0`; `timing.rate` is safer. +- **Event loop.** The launcher runs SQLite and file globbing on the event loop. +- **Storage housekeeping.** + - `ProjectStore.reading()` holds the per-project read lock for the whole read. + - `blobs` is never garbage-collected. + - `cancellation_done` opens a writer connection just to read. + - `derived_storage` reports backups at the root, not `.spool/backups`. + - `reclaim` can report a negative `reclaimed` value. +- **Import.** + - `import_project` creates `0700` folders with generic names. + - A failed import leaves its cache folder behind. +- **Path containment.** `contained()` is lexical: a symlink inside a project folder + can point outside it. +- **Studio.** + - The escaping check covers ID, number and track fields; turning the + reviewer's general concatenation scan into a gate check would cover more. + - The studio has three copies of `esc`. + - The flush error shown in the delete dialog says "before queueing or capturing". +- **Demo.** `pages/serve.py` answers an inverted byte range with a negative + `Content-Length` (local dev tool only). +- **Repository policy.** AGENTS.md keeps outstanding work in GitHub issues. The + _Needs decision_ items could be filed as issues if this report should not stay + in the repository. + +## Final test run + +Baseline on `main` (`7caf5c9`): 80 of 81 gate commands passed. `test_take_files` +failed `test_open_clip_folder_uses_stable_identity_and_supports_empty_shots` +because this sandbox has no `xdg-open` (F-28). + +Final run on the branch, after the last commit: + +```text +$ python3 tests/run_architecture_checks.py +… 82 commands run (ruff format/check, Prettier, ESLint, knip, tsc, + editor_contract_checks.py, 35 *_checks.mjs, 40 test_*.py files) +exit 0 — 454 Python tests OK, 8 skipped: + 5 need ComfyUI-Deadline-Plugin checked out beside the Spool checkout + 2 UNC paths are a Windows path form + 1 Windows streaming workaround +real 1m35.7s +``` + +Environment: Linux, Python 3.13.18, Node 22.23.4, ffmpeg from the distribution, +aiohttp 3.14.5, comfy-sdk 0.5.0. diff --git a/docs/dev/architecture.md b/docs/dev/architecture.md index 03c9c64..181f5fa 100644 --- a/docs/dev/architecture.md +++ b/docs/dev/architecture.md @@ -73,6 +73,17 @@ Storage and project-folder work runs off ComfyUI's event loop through the published snapshot, import proxies and other referenced media. Playback cache keys include source path/size/mtime, project size/FPS and the cache format version. The preview route enqueues work and returns preparing until its local file is ready. +Launcher probes, status and startup use cancellation-draining workers for their +SQLite and filesystem work, including process creation. Startup reservation and +spawn stay in one worker. Status/error writers replace only the launch state +they observed, so a delayed worker cannot overwrite a newer reservation. + +Cancellation preflight reads the acknowledgement through the immutable read +generation. Planning and recording delivery still use their writer transactions. +FFmpeg text diagnostics use UTF-8 with replacement for invalid bytes, independent +of the host locale; subprocess failure types and timeouts remain unchanged. +Comparison sheets extract frames in a private temporary directory and retain +only the returned sheet. Explicit frame exports remain durable user outputs. ## Project format @@ -86,7 +97,9 @@ imports reject unknown export versions before creating a destination. `store.py` owns ordered, numbered migration steps. Each commits its data and version together, version last; a failed step rolls back and can run again. -Completed earlier steps remain committed. Released steps are frozen: changes +Completed earlier steps remain committed in that database; a project folder +publishes its upgraded working copy only after every step succeeds, so a failure +leaves the published snapshot at its old version. Released steps are frozen: changes require a new number. Before upgrading a folder, Spool copies its last published snapshot to `.spool/backups/project-schema-.spool`. Schema 6 packs existing evidence, then compacts through the existing local-cache path @@ -142,6 +155,23 @@ lock. Only a changed published signature refreshes it under the share lock; writers serialize working-copy refresh/replacement with a different local lock. A writer can publish while an existing reader finishes its older generation. +Each newly published snapshot contains an additive `spool_publication` table +with a fresh publication UUID. The marker is written into the copied database +before it is fsynced and atomically replaces `project.spool`; it travels with the +snapshot even if manifest publication is interrupted. Change detection combines +the marker with timestamp, size and file identity. Unchanged high-resolution +file identities cache the marker; coarse timestamps re-read it. Legacy snapshots +without the table remain readable and gain it on their next new-version save. +Marker reads hold the project lock until their SQLite handle closes, so they +cannot obstruct a concurrent snapshot replacement on Windows. An unreadable +identity fails the operation instead of authorizing reuse of an older runtime. +The manifest, document, API and SQLite schema-version number are unchanged. +Older writers preserve the marker but do not refresh it; mixed-version detection +then relies on the replacement file's identity. +Failed project imports clean their exclusively created destination and new local +working cache. Cleanup preserves pre-existing caches and refuses redirected or +uninspectable paths; a denied cleanup retains the original import failure. + Receipt scans visit pending attempts every two seconds and settled history every five minutes (also on collector startup). Folder scans cache projection/inventory at a revision/event cursor and recheck both inside the save transaction. Collected @@ -154,6 +184,24 @@ and cross-directory moves become new takes. Changed bytes at an indexed path create a new take; the old record points to a missing source instead of silently playing the replacement. Renames/replacements discard old file-owned evidence. +Manual reconciliation, local result observation and Cloud receipt ingestion use +the same collector's per-project thread lock as periodic scans. File collection +for one project cannot reuse another worker's partial files; other projects have +independent locks. Cancellation drains these storage workers before their +executor lock or lifecycle ownership is released, including repeated cancellation. +Local, Deadline and Cloud artifact copies use unique sibling temporary files, +including when independent workspaces collect into the same project. Publication +uses the common replacement retry; cleanup removes only the operation's own +temporary file and retains an original failure if cleanup is denied. This file +ownership does not serialize separate collectors' receipt or ledger decisions. + +Deadline's 30-second missing-acknowledgment grace starts when collection first +observes the durable `submitting.json` marker written immediately before the +external call. Queued time, validation and staging do not consume it. Restarting +the collector grants fresh grace. An observed marker remains evidence if share +files disappear; a never-observed missing marker stays pending conservatively. +Recovery does not automatically submit another job. + Once daily, collection expires events and operation/result payloads older than 30 days, retaining each project's latest event cursor and cancellation effects whose delivery is unconfirmed or whose attempt is still active. Operation ID/project/digest tombstones live forever: expired retries @@ -161,12 +209,19 @@ return an operation conflict and never execute again. Attempt ownership rows expire with old terminal attempts. Attempts, dispatches and execution receipts remain evidence for recovery and deduplication; they are not diagnostic logs. No Python/JavaScript authored document fields or wire versions change. -Maintenance reports open immutable read-only snapshots: no migrations, directory -creation, cache refresh or take-file recovery. +Maintenance reports scan immutable read-only snapshots in private temporary +directories, removed after the report. They do not migrate, create project +directories, refresh the workspace cache or recover take files. Copying holds an +existing project lock until the source handle closes; SQLite then opens only the +private copy, letting writers publish during long scans on Windows. Read-only +inspection never creates a lock. A legacy folder without one can still briefly +obstruct Windows replacement while the source is being copied. `durable.py` owns unique temporary files, flush/fsync, atomic replacement with SMB retry, and failure cleanup for JSON receipts, take sidecars, farm JSON and manifests. `ownership.atomic_replace` reexports the same replacement primitive. +OTIO exports use the same replacement retry and exclusively created pending +files, cleaning only their own unfinished file after a failure. Deadline submissions copy the stdlib-only helper beside their worker protocol. Local `receipt.json` writes use schema 2 and `primaryArtifactId`; collection still verifies schema 1 from older executions. Farm protocol 2 already uses that diff --git a/docs/dev/demo.md b/docs/dev/demo.md index b1ac9d5..74d045e 100644 --- a/docs/dev/demo.md +++ b/docs/dev/demo.md @@ -40,6 +40,12 @@ saved under both the escaped and the unescaped path. The fake mirrors five host replies: the project, the unchanged-project poll, the save, take deletion and Use take. If one of those routes changes shape, change `pages/demo-api.js` and `tests/demo_api_checks.mjs` in the same commit. +The simulated mutations also match the host's required-field validation: missing +fields are `400 invalid_request`; a supplied stale revision remains a conflict. +Supported saves also mirror the host's structural checks for document, clip, +take and capture containers, including input-recovery lists. Executable graph +meaning and input preparation remain host responsibilities. Demo take-value +validation errors carry the same `invalid_request` code as the host. ## Recording the project diff --git a/docs/dev/development.md b/docs/dev/development.md index f941c06..b2e415f 100644 --- a/docs/dev/development.md +++ b/docs/dev/development.md @@ -27,7 +27,7 @@ python -m pip install -r ComfyUI/custom_nodes/Spool/requirements.txt ``` The checks need Python 3.11+ (3.12 is used in development) with aiohttp and -Pillow, Node 24, and `ffmpeg` and `ffprobe` on PATH. Browser checks also need +Pillow, Node 24, and `ffmpeg` and `ffprobe` 5.1 or newer on PATH. Browser checks also need Chrome or Edge. Install the formatter and linters once per checkout or worktree: ```powershell @@ -101,6 +101,15 @@ fails on any page exception. Without names it runs every check except the opt-in profiles (`workflow-project-profile`, `workflow-capture-bundle`); check names are the keys of `CHECKS` in `tests/run_browser_checks.cjs`. +`native-take-delete-flush` checks permanent deletion while a save is pending, +after an injected save failure, and after retry. Successful saves and deletion +reach the test host; the runner also checks media/workflow files on disk. +`native-take-export-download` clicks the real OTIO download link, waits for +Chrome's completed-download event, and compares parsed bytes with the host's +export. Its failure cases distinguish injected network failures from an HTTP +400 produced by sending an empty export plan. Evidence includes screenshots, +download events, completed files and the observed blob URL lifecycle. + `embedded-templates-live` uses the installed native Comfy chooser. Wait for its search indicator to settle before clicking a result: frontend 1.57 debounces search and cancels pending template preparation when the results change, even diff --git a/docs/dev/durable-names.md b/docs/dev/durable-names.md index 603c35d..e094698 100644 --- a/docs/dev/durable-names.md +++ b/docs/dev/durable-names.md @@ -17,6 +17,7 @@ against `spool.__version__`; protocol fixtures pin API and bridge versions. | Template modes `embedded`, `prepare`; messages `spool-template-request`, `spool-template-state`, `spool-prepare-template`, `spool-template-preparation` | `ui/bridge.js` | | Browser bridge identity `spool-comfy-bridge` | `ui/bridge.js` (session storage) | | `project.spool`, `.spool/project.lock`, `app.sqlite3` | `spool/projects.py`, `spool/store.py` | +| Additive snapshot table `spool_publication` | `spool/projects.py` | | Project `input/images`, `input/audio`, `input/video`, `output` (per-clip output folders), `.spool/previews`, `.spool/receipts` | `spool/projects.py` | | `receipt.json` | `spool/receipts.py`, `spool/farm_protocol.py` | | Media-stem `.json` sidecars | `spool/sidecars.py` | @@ -24,8 +25,8 @@ against `spool.__version__`; protocol fixtures pin API and bridge versions. | Environment `SPOOL_STORAGE`, `SPOOL_CREDENTIALS`, `COMFY_API_KEY`, `COMFY_BASE_URL` | `spool/__main__.py`, root `__init__.py`, `spool/credentials.py`, `spool/cloud.py` | | Pending envelopes `spool-native-:queue-request`, `spool-native-:experiment-request` | `ui/editor.js`, `ui/generation.js` (session storage) | -The parallel project-storage lane plans a `project.json` manifest inside project -folders; treat that name as durable when the format lands. Do not rename pending +Project folders hold a `project.json` manifest (`spool/projects.py`); treat its +name and fields as durable. Do not rename pending envelope keys casually: losing an operation ID can cause duplicate submissions after an uncertain reply. The same compatibility rule covers error codes, command action/capability names and the exact-integer tag `__spool_int__`. diff --git a/docs/guide/automation.md b/docs/guide/automation.md index 760ffc2..a820bf9 100644 --- a/docs/guide/automation.md +++ b/docs/guide/automation.md @@ -163,6 +163,10 @@ Read `code`, not message text: `not_found` (404), `invalid_request` (400), changed: a sidecar write can fail after a commit. Inspect state and reuse the identical operation envelope when retrying uncertain operations. +Deadline recovery lookup failures use `unavailable` (503). Retry the same +`recover_submission` envelope when Deadline is reachable again; successful +lookup still verifies the existing job's submission and attempt identity. + The studio sends `X-Spool-Api-Version` on every HTTP request. A mismatching version blocks writes and asks for a reload while keeping the tab's draft. Versionless requests from the CLI, agents and curl remain supported. A bridge @@ -170,7 +174,8 @@ version mismatch asks you to reload the ComfyUI page. These rules apply both inside ComfyUI and standalone, independently of local, Cloud or Deadline targets. The CLI writes JSON errors to stderr and returns 1, including `network_error`, -`http_error` for non-JSON HTTP failures, and `invalid_response` for invalid JSON. +`http_error` for non-JSON HTTP failures, `invalid_response` for invalid JSON, and +`invalid_request` for a command file it cannot read or parse. ### Public routes diff --git a/docs/guide/deadline.md b/docs/guide/deadline.md index 442bc51..1f57a26 100644 --- a/docs/guide/deadline.md +++ b/docs/guide/deadline.md @@ -51,8 +51,10 @@ On the worker, that script: Job scripts follow Deadline's [pre- and post-job/task script](https://docs.thinkboxsoftware.com/products/deadline/10.4/1_User%20Manual/manual/job-scripts.html) contract. Submission files live under -`/spool-submissions//` and receipts under -`spool-receipts/`. +`/spool-submissions//` and receipts under +`/spool-receipts/`. For a project folder the result root is the +project's `.spool/deadline` folder; otherwise it is the submit node's output +directory. ## Collecting results diff --git a/docs/guide/editing.md b/docs/guide/editing.md index 18e8740..5910ab8 100644 --- a/docs/guide/editing.md +++ b/docs/guide/editing.md @@ -150,8 +150,10 @@ The **Media** tab lists the project's footage, images and audio, with filters, search and grid or list views. - **Import footage…** adds files. Drop files onto the sidebar or the timeline. -- **Project → Import video from path…** reads a video where it is, without copying - it, and splits it at detected hard cuts. Each shot becomes a clip whose first +- **Project → Import video from path…** copies a video into the project's + `input/video/` folder and splits it at detected hard cuts. Its original-file + references are project-relative, so they remain valid when the project moves. + Each shot becomes a clip whose first take is **Original clip**; the source audio goes once on A2. - **Detect cuts in selected clip…** (in **Project**, or a clip's right-click menu) finds hard cuts inside the clip, shows the count and splits on request. @@ -171,6 +173,10 @@ seconds of 25 fps footage (125 frames) plays in 4.17 seconds on a 30 fps timelin Linked audio follows the speed change with its pitch preserved. Original files are never changed. +Joined extension copies also retime their own audio and apply the measured +frame alignment. Newly imported media reports its displayed orientation, including +rotated videos and EXIF-oriented images; saved records are not re-described. + ## Export **Export** writes an MP4 of the sequence with its audio mix. diff --git a/docs/guide/generation.md b/docs/guide/generation.md index fc3ea2f..6093c3b 100644 --- a/docs/guide/generation.md +++ b/docs/guide/generation.md @@ -166,6 +166,11 @@ before sending it. Results are copied into the clip's folder and checked against that record before they become takes. New takes never replace the take in the edit. +Spool checks all batch items and their input sources before creating frozen +copies. Fix any reported input or clip error, then submit the batch again. +New clip output folder names are distinct even on disks that ignore case; +previously assigned folders are kept when you rename a clip. + Queue requests use the same defaults in embedded and standalone Spool: an omitted count means one take. An explicit empty item list is rejected rather than replaced with another selection. Scoped experiments use the same item form. @@ -200,6 +205,9 @@ path and execution record. - **Retry submission** appears only when Spool could not confirm a submission. **Find existing Deadline job** looks for a farm job that was accepted before the connection was lost. Neither starts a second render. + If the lookup is temporarily unavailable, retry it when Deadline is reachable. + Deadline's confirmation grace excludes time waiting in the queue or preparing + inputs. A slow preparation does not by itself make submission uncertain. - **Retry collection** appears when a finished render could not be collected. - **Clear from queue** and **Clear finished** tidy the list. Takes, files and records stay. Dismissing a failed take on the take rail clears it here too. @@ -251,7 +259,8 @@ render, and agents act on them only when asked. and VHS_LoadVideo. Other loaders with file inputs are refused. Models and custom-node dependencies must already be installed where the render runs. - **Results.** Spool collects SaveImage, PreviewImage, SaveVideo and - VHS_VideoCombine video outputs. VHS `meta_batch` is not supported. + VHS_VideoCombine video outputs. VHS `meta_batch` is not supported. Comfy Cloud + renders return video only, so a Cloud workflow needs a SaveVideo result. - **Old takes.** Opening the workflow of an imported or historical take does not map its saved values back into widgets; its exact recorded workflow stays available. diff --git a/docs/guide/projects.md b/docs/guide/projects.md index f8bce29..65c5d74 100644 --- a/docs/guide/projects.md +++ b/docs/guide/projects.md @@ -81,7 +81,7 @@ ComfyUI as ComfyUI Desktop makes, and open the studio at python -m spool --open ``` -Either way you need Python 3.11+ with aiohttp and Pillow, and ffmpeg on PATH. Inside `ComfyUI/custom_nodes`, it uses the same settings +Either way you need Python 3.11+ with aiohttp and Pillow, and ffmpeg/ffprobe 5.1 or newer on PATH. Inside `ComfyUI/custom_nodes`, it uses the same settings folder as the ComfyUI extension (`ComfyUI/user/spool`). Options: `--open`, `--port`, `--storage` (or `SPOOL_STORAGE`) for the settings folder, and `--host`. See [Security and network access](security.md) before changing the host. @@ -115,6 +115,9 @@ no clip uses, including obsolete playback copies from older versions in window still apply. `reclaim` also compacts the project database and publishes a new snapshot. +Dry runs read a temporary local copy, so a long report can finish while another +editor saves. They leave the project files unchanged. + ## Diagnostics The status in the top bar shows the current operation and how long it has taken. diff --git a/docs/guide/security.md b/docs/guide/security.md index 64fc5a4..3591313 100644 --- a/docs/guide/security.md +++ b/docs/guide/security.md @@ -1,5 +1,9 @@ # Security and network access +Imported or authored JSON cannot contain a dictionary whose only key is +`__spool_blob__`. That shape is reserved for internal evidence references. +Spool rejects it before saving and retains the previous project revision. + Spool has no user accounts. Who can use it depends on who can reach the port of the ComfyUI it runs in, or of `python -m spool`. @@ -19,7 +23,8 @@ does not apply. - revealing a take or clip folder in the file manager - Comfy connection settings and the Cloud API key - starting ComfyUI -- serving original footage, or any footage stored outside the project and Comfy folders +- serving original footage, or any footage stored outside the project and Comfy folders, + and comparing takes or detecting cuts in that footage - preparing playback and exporting frames A request to them must come from a loopback address. It must also use a loopback diff --git a/pages/demo-api.js b/pages/demo-api.js index a9d2589..36fe290 100644 --- a/pages/demo-api.js +++ b/pages/demo-api.js @@ -52,6 +52,97 @@ } const conflict = () => reply(409, { error: 'This project changed. Reload to see it.', code: 'revision_conflict', current: copy(project) }); + const invalid = error => reply(400, { error, code: 'invalid_request' }); + const object = value => value !== null && typeof value === 'object' && !Array.isArray(value); + function objectRows(value, field, ...keys) { + if (!Array.isArray(value) || value.some(row => !object(row))) return field + ' must be a list of JSON objects'; + if (value.some(row => keys.some(key => typeof row[key] !== 'string'))) + return field + ' entries need string fields: ' + keys.join(', '); + return null; + } + // Match the host's save-container boundary; preparation and graph meaning stay on the host. + function saveShape(document, recipes) { + if (!object(document) || !Array.isArray(document.clips)) + return 'Project document must be an object with a clips list'; + for (const [field, keys] of [ + ['clips', ['id']], + ['media', ['id']], + ['audioClips', []], + ['importedWorkflows', []], + ]) { + const error = objectRows(Object.hasOwn(document, field) ? document[field] : [], 'Document ' + field, ...keys); + if (error) return error; + } + for (const clip of document.clips) { + for (const [field, keys] of [ + ['takes', ['id']], + ['bindings', ['key']], + ]) { + const error = objectRows(Object.hasOwn(clip, field) ? clip[field] : [], 'Clip ' + field, ...keys); + if (error) return error; + } + for (const take of clip.takes || []) + if (!object(take.media) || typeof take.media.id !== 'string') + return 'Take media must be a JSON object with an id'; + if (Object.hasOwn(clip, 'inputValues') && !object(clip.inputValues)) + return 'Clip inputValues must be a JSON object'; + } + if (!object(recipes)) return 'workflowRecipes must be a JSON object'; + for (const capture of Object.values(recipes)) { + if (!object(capture)) return 'The captured workflow must be a JSON object'; + const error = objectRows( + Object.hasOwn(capture, 'bindings') ? capture.bindings : [], + 'Capture bindings', + 'key', + 'nodeId', + 'input', + ); + if (error) return error; + for (const field of ['provenance', 'result', 'inputRecovery']) + if (Object.hasOwn(capture, field) && !object(capture[field])) + return 'Capture ' + field + ' must be a JSON object'; + if ( + Object.hasOwn(capture, 'inputRecovery') && + Object.hasOwn(capture.inputRecovery, 'recovered') && + !Array.isArray(capture.inputRecovery.recovered) + ) + return 'Capture inputRecovery.recovered must be a list'; + if (Object.hasOwn(capture, 'result') && typeof capture.result.nodeId !== 'string') + return 'Capture result needs a string nodeId'; + const builtin = capture.provenance?.builtinTemplate; + if (builtin != null && typeof builtin !== 'string') return 'Capture builtinTemplate must be a string'; + const graphs = capture.workflow == null ? [] : [capture.workflow]; + while (graphs.length) { + const graph = graphs.pop(); + if (!object(graph)) return 'Capture workflow must be a JSON object'; + const nodes = Object.hasOwn(graph, 'nodes') ? graph.nodes : []; + const nodeError = objectRows(nodes, 'Workflow nodes', 'type'); + if (nodeError) return nodeError; + if (nodes.some(node => !Object.hasOwn(node, 'id'))) return 'Workflow nodes need an id'; + const definitions = Object.hasOwn(graph, 'definitions') ? graph.definitions : {}; + if (!object(definitions)) return 'Workflow definitions must be a JSON object'; + const subgraphs = Object.hasOwn(definitions, 'subgraphs') ? definitions.subgraphs : []; + const subgraphError = objectRows(subgraphs, 'Workflow subgraphs', 'id'); + if (subgraphError) return subgraphError; + graphs.push(...subgraphs); + } + } + return null; + } + function requiredBody(body, ...fields) { + let value; + try { + value = JSON.parse(body); + } catch { + return { error: invalid('Request body must be valid JSON') }; + } + if (!value || typeof value !== 'object' || Array.isArray(value)) + return { error: invalid('Request body must be a JSON object') }; + for (const field of fields) + if (!Object.hasOwn(value, field)) + return { error: invalid('Request body is missing the required ' + field + ' field') }; + return { value }; + } function recorded(method, path) { const value = replies[method + ' ' + path]; if (value === undefined) return null; @@ -73,44 +164,97 @@ const deleted = new Set(document.deletedTakeIds || []); for (const clip of document.clips) clip.takes = [ - ...clip.takes.filter(t => !t.generated), + ...(clip.takes || []).filter(t => !t.generated), ...generated.filter(a => !deleted.has(a.takeId) && owns(clip, a)).map(a => copy(a.take)), ]; return document; } - const files = media => [media?.originalFile, media?.file, media?.poster].filter(Boolean); + const filePath = value => { + if (typeof value !== 'string' || !value) return null; + let path = value; + try { + path = decodeURIComponent(path); + } catch { + // A literal percent sign is valid in a recorded file name. + } + path = path.replaceAll('\\', '/'); + const parts = []; + for (const part of path.split('/')) { + if (!part || part === '.') continue; + if (part === '..' && parts.length && parts.at(-1) !== '..') parts.pop(); + else parts.push(part); + } + return (path.startsWith('/') ? '/' : '') + parts.join('/'); + }; + const original = media => filePath(media?.originalFile || media?.file); + const sidecar = path => (path ? path.replace(/\.[^/.]+$/, '') + '.json' : null); + const files = media => + [...new Set([media?.originalFile, media?.file, media?.poster].map(filePath)), sidecar(original(media))].filter( + Boolean, + ); // Delete takes as the host does: never the take in the edit, then record the // deletion so the takes stay gone. Nothing is on disk to remove. function deleteTakes(clipId, body) { const clip = project.document.clips.find(c => c.id === clipId), - ids = body.takeIds; - if (!clip || !Array.isArray(ids) || !ids.length) return reply(400, { error: 'Select the takes to delete' }); + requested = body.takeIds; + if (!clip || !Array.isArray(requested) || !requested.length || requested.some(id => typeof id !== 'string')) + return invalid('Select the takes to delete'); + const ids = [...new Set(requested)].sort(); const hidden = new Set(clip.hiddenTakeIds || []); const available = new Map(clip.takes.filter(t => !t.placeholder && !hidden.has(t.id)).map(t => [t.id, t])); - if (!ids.every(id => available.has(id))) - return reply(400, { error: 'Some selected takes are no longer available' }); + if (!ids.every(id => available.has(id))) return invalid('Some selected takes are no longer available'); if (ids.includes(clip.takeId)) - return reply(400, { error: 'The take in the edit is kept. Choose another take before deleting it.' }); + return invalid('The take in the edit is kept. Choose another take before deleting it.'); const document = project.document; + const originals = new Set(ids.map(id => original(available.get(id).media))); + if ( + [...originals].some( + path => + !/^(output|input|\.spool)\//.test(path || '') || + !/\.(mp4|mov|webm|mkv|m4v|png|jpg|jpeg|webp|wav|mp3|m4a|aac|ogg|flac|opus)$/i.test(path), + ) + ) + return invalid('Delete from disk only manages media owned by this project; archive this take instead.'); + // Match the host's deletion plan: original, sidecar and project preview files. + // Other referenced files are protected but are not automatically deleted. + const selected = new Set([...originals, ...[...originals].map(sidecar)]); + for (const id of ids) + for (const path of files(available.get(id).media)) if (path.startsWith('.spool/previews/')) selected.add(path); // A file another take or timeline audio still uses stays: a split shares its takes. - const selected = new Set(ids.flatMap(id => files(available.get(id).media))); for (const other of document.clips) for (const take of other.takes || []) if (!(other.id === clipId && ids.includes(take.id)) && files(take.media).some(f => selected.has(f))) - return reply(400, { - error: - 'A selected file is also used by another take in ' + - (other.name || 'this clip') + - '. Archive it instead.', - }); + return invalid( + 'A selected file is also used by another take in ' + (other.name || 'this clip') + '. Archive it instead.', + ); if ((document.audioClips || []).some(a => files(a.media).some(f => selected.has(f)))) - return reply(400, { error: 'A selected file is used by timeline audio. Archive it instead.' }); + return invalid('A selected file is used by timeline audio. Archive it instead.'); + for (const other of document.clips) + for (const binding of other.bindings || []) { + const value = other.inputValues?.[binding.key]; + if (!binding.asset || typeof value !== 'string') continue; + const staged = value.startsWith('spool-projects/') + ? value + .replace(/ \[input\]$/, '') + .split('/') + .slice(2) + .join('/') + : null; + const candidate = staged ? filePath(staged) : /^(\/|[A-Za-z]:[\\/])/.test(value) ? filePath(value) : null; + if (originals.has(candidate)) return invalid('A selected file is a generation input. Archive it instead.'); + } + const mediaIds = new Set(ids.map(id => available.get(id).media?.id).filter(Boolean)); + for (const media of document.media || []) + if (files(media).some(path => selected.has(path))) { + if (!originals.has(original(media))) + return invalid('A selected file is shared with another media-pool asset. Archive it instead.'); + if (media.id) mediaIds.add(media.id); + } document.deletedTakeIds = [...new Set([...(document.deletedTakeIds || []), ...ids])].sort(); - document.deletedMediaIds = [ - ...new Set([...(document.deletedMediaIds || []), ...ids.map(id => available.get(id).media?.id).filter(Boolean)]), - ].sort(); + document.deletedMediaIds = [...new Set([...(document.deletedMediaIds || []), ...mediaIds])].sort(); clip.takes = clip.takes.filter(t => !ids.includes(t.id)); + document.media = (document.media || []).filter(media => !mediaIds.has(media.id)); changed(); return reply(200, { project: copy(project), deleted: ids, pendingFiles: 0 }); } @@ -125,10 +269,9 @@ if (!clip) return reply(404, { error: 'Clip not found' }); const take = clip.takes.find(t => t.id === takeId && !t.placeholder); if (!take || ((clip.hiddenTakeIds || []).includes(takeId) && takeId !== clip.takeId)) - return reply(400, { error: 'This take is no longer available for the shot' }); + return invalid('This take is no longer available for the shot'); const takeBase = capture?.provenance?.takeBase; - if (capture && takeBase?.takeId !== takeId) - return reply(400, { error: 'Generation setup must belong to the chosen take' }); + if (capture && takeBase?.takeId !== takeId) return invalid('Generation setup must belong to the chosen take'); timeline.chooseTake(project.document, clipId, takeId); clip.generationBase = takeBase || { takeId, source: 'No executable metadata' }; for (const field of ['workflow', 'shotInputs', 'seedKey']) delete clip[field]; @@ -164,7 +307,13 @@ return reply(200, copy(project)); } if (method === 'PUT' && path === PROJECT) { - const value = JSON.parse(body); + const { value, error } = requiredBody(body, 'revision', 'document'); + if (error) return error; + const shapeError = saveShape( + value.document, + Object.hasOwn(value, 'workflowRecipes') ? value.workflowRecipes : {}, + ); + if (shapeError) return invalid(shapeError); if (value.revision !== project.revision) return conflict(); // The studio leaves out jobs, which the host owns and adds back to every reply. project.document = attach({ ...value.document, jobs: project.document.jobs }); @@ -174,13 +323,15 @@ if (method === 'POST' && path.startsWith(PROJECT + '/focus/')) return reply(200, { ok: true }); const deletion = method === 'POST' && path.match(/^projects\/[^/]+\/delete-takes\/([^/]+)$/); if (deletion) { - const value = JSON.parse(body); + const { value, error } = requiredBody(body, 'revision', 'takeIds'); + if (error) return error; if (value.revision !== project.revision) return conflict(); return deleteTakes(decodeURIComponent(deletion[1]), value); } const use = method === 'POST' && path.match(/^projects\/[^/]+\/use-take\/([^/]+)$/); if (use) { - const value = JSON.parse(body); + const { value, error } = requiredBody(body, 'revision', 'takeId'); + if (error) return error; if (value.revision !== project.revision) return conflict(); return useTake(decodeURIComponent(use[1]), value); } diff --git a/pages/serve.py b/pages/serve.py index 02c7f91..b09e43c 100644 --- a/pages/serve.py +++ b/pages/serve.py @@ -27,6 +27,7 @@ def translate_path(self, path): return super().translate_path("/" + clean[len(self.base) :]) def send_head(self): + self.remaining = None path = self.translate_path(self.path) if os.path.isdir(path): path = os.path.join(path, "index.html") @@ -34,10 +35,14 @@ def send_head(self): if not match or not os.path.isfile(path): return super().send_head() size = os.path.getsize(path) - start = int(match[1]) if match[1] else max(0, size - int(match[2] or 0)) - end = min(int(match[2]), size - 1) if match[1] and match[2] else size - 1 - if start >= size: - self.send_error(416) + if match[1]: + start = int(match[1]) + end = min(int(match[2]), size - 1) if match[2] else size - 1 + else: + start = max(0, size - int(match[2] or 0)) + end = size - 1 + if start >= size or end < start: + self.send_unsatisfiable_range(size) return None handle = open(path, "rb") handle.seek(start) @@ -49,6 +54,12 @@ def send_head(self): self.remaining = end - start + 1 return handle + def send_unsatisfiable_range(self, size): + self.send_response(416) + self.send_header("Content-Range", f"bytes */{size}") + self.send_header("Content-Length", "0") + self.end_headers() + def copyfile(self, source, outputfile): remaining = getattr(self, "remaining", None) if remaining is None: diff --git a/run_spool.bat b/run_spool.bat index 16abc05..6633b1f 100644 --- a/run_spool.bat +++ b/run_spool.bat @@ -14,5 +14,7 @@ set "PY=%~dp0..\..\.venv\Scripts\python.exe" if exist "%PY%" goto run set "PY=python" :run -"%PY%" %FLAGS% -m spool --open %* +rem The portable build's python_embeded has a ._pth file, so Python leaves this folder +rem off sys.path and "-m spool" alone cannot find Spool; add it before running. +"%PY%" %FLAGS% -c "import runpy, sys; sys.path.insert(0, '.'); runpy.run_module('spool', run_name='__main__', alter_sys=True)" --open %* if errorlevel 1 pause diff --git a/skills/spool-control/references/commands.md b/skills/spool-control/references/commands.md index 3b2ae8f..1701edd 100644 --- a/skills/spool-control/references/commands.md +++ b/skills/spool-control/references/commands.md @@ -100,7 +100,8 @@ Read `code` rather than parsing message text: `invalid_request`, `not_found`, An error does not prove nothing changed: sidecar writes can fail after a commit. Inspect status before retrying, preserving the exact pending operation envelope. The CLI reports non-JSON HTTP failures as `http_error`, connection failures as -`network_error`, and invalid JSON as `invalid_response` on stderr (exit 1). +`network_error`, invalid JSON as `invalid_response`, and a command file it cannot +read or parse as `invalid_request` on stderr (exit 1). Pass `--wire-json` to preserve tagged exact integers in machine stdout; the default prints readable integers. Versionless CLI/agent requests remain supported. diff --git a/spool/__main__.py b/spool/__main__.py index d81a199..81578d3 100644 --- a/spool/__main__.py +++ b/spool/__main__.py @@ -16,7 +16,8 @@ from . import cli # Installed under ComfyUI/custom_nodes, the standalone editor shares the plugin's projects and Comfy's folders. -COMFY = Path(__file__).resolve().parents[3] +# A checkout outside custom_nodes may sit too close to the drive root to have one. +COMFY = Path(__file__).resolve().parents[min(3, len(Path(__file__).resolve().parents) - 1)] def main(): diff --git a/spool/cli.py b/spool/cli.py index 97f7615..75eed08 100644 --- a/spool/cli.py +++ b/spool/cli.py @@ -11,6 +11,18 @@ from .wire import wire, unwire ACTIONS = {"status", "shot", "events", "focus", "command", "compare"} +LOOPBACK = {"localhost", "127.0.0.1", "::1"} + + +def urlopen(request, timeout): + """Reach a Spool on this computer directly, even when a proxy is set for other traffic. + + urllib sends loopback requests to `http_proxy` unless `no_proxy` names them, and a + proxy cannot reach this computer's Spool or would make the request look remote. + """ + if (urllib.parse.urlsplit(request.full_url).hostname or "").lower() in LOOPBACK: + return urllib.request.build_opener(urllib.request.ProxyHandler({})).open(request, timeout=timeout) + return urllib.request.urlopen(request, timeout=timeout) def run(): @@ -49,14 +61,21 @@ def run(): if not args.file: parser.error("command requires --file") path += "/commands" - body = json.loads(sys.stdin.read() if str(args.file) == "-" else args.file.read_text(encoding="utf-8")) + try: + body = json.loads(sys.stdin.read() if str(args.file) == "-" else args.file.read_text(encoding="utf-8")) + except (OSError, UnicodeError, ValueError) as error: + print( + json.dumps({"error": "Could not read the command file: " + str(error), "code": "invalid_request"}), + file=sys.stderr, + ) + return 1 request = urllib.request.Request( args.url.rstrip("/") + path, data=json.dumps(wire(body)).encode() if body is not None else None, headers={"Content-Type": "application/json"}, ) try: - with urllib.request.urlopen(request, timeout=120) as response: + with urlopen(request, timeout=120) as response: value = json.load(response) if not args.wire_json: value = unwire(value) diff --git a/spool/cloud.py b/spool/cloud.py index 789b630..d1b1466 100644 --- a/spool/cloud.py +++ b/spool/cloud.py @@ -23,6 +23,8 @@ class InvalidWorkflow(Exception): from .ownership import ExecutorLock, ExecutorBusy from .store import uid from .execution_targets import adapter_for +from .media import unique_partial +from .durable import atomic_replace FREE_TIER = "Running on Comfy Cloud needs a plan that allows API keys. To pay with Comfy credits instead, choose Execute on: Local ComfyUI · Partner Nodes." @@ -87,11 +89,22 @@ async def storage_call(function, *args, **kwargs): # Cancellation must not leave a SQLite/file operation running after the # server releases its executor lock or closes the project workspace. task = asyncio.create_task(asyncio.to_thread(function, *args, **kwargs)) - try: - return await asyncio.shield(task) - except asyncio.CancelledError: - await task - raise + cancelled = None + while True: + try: + result = await asyncio.shield(task) + except asyncio.CancelledError as error: + if task.cancelled(): + raise + cancelled = cancelled or error + except BaseException as error: + if cancelled is not None: + raise cancelled from error + raise + else: + if cancelled is not None: + raise cancelled + return result class CloudExecutor: @@ -197,19 +210,22 @@ async def collect(self, attempt, media, job, address): identity = uid() destination = await storage_call(media.result_path, attempt, identity, extension, True) await storage_call(destination.parent.mkdir, parents=True, exist_ok=True) - pending = destination.with_suffix(extension + ".partial") + pending = unique_partial(destination) try: await output.to_file(pending) await storage_call( self.finish_download, attempt, media, job, address, output, identity, destination, pending ) finally: - await storage_call(pending.unlink, missing_ok=True) + try: + await storage_call(pending.unlink, missing_ok=True) + except OSError: + pass # Preserve the download or replacement failure if cleanup is denied. def finish_download(self, attempt, media, job, address, output, identity, destination, pending): if pending.stat().st_size != output.size_bytes: raise ValueError("Comfy Cloud video download is incomplete") - pending.replace(destination) + atomic_replace(pending, destination) selection = attempt["request"]["capture"]["result"] with destination.open("rb") as handle: checksum = hashlib.file_digest(handle, "sha256").hexdigest() @@ -239,7 +255,24 @@ async def run_once(self): for a in store.scan_attempts(target="cloud", pending_only=True) ] ) - await asyncio.gather(*(self.observe(a) for a in pending)) + # Let every observation settle before reporting a failure. An early return + # would orphan the rest of this batch while the lifecycle starts the next + # one, observing the same attempts concurrently. + results = await asyncio.gather(*(self.observe(a) for a in pending), return_exceptions=True) + failures = [result for result in results if isinstance(result, BaseException)] + if not failures: + return + control = next((error for error in failures if not isinstance(error, Exception)), None) + if control is not None: + errors = [error for error in failures if isinstance(error, Exception)] + if errors: + cause = ExceptionGroup("Spool Cloud observations failed before cancellation", errors) + logging.error("Spool Cloud observations failed before cancellation", exc_info=cause) + raise control from cause + raise control + if len(failures) == 1: + raise failures[0] + raise ExceptionGroup("Spool Cloud observation batch failed", failures) async def lifecycle(self, app): try: @@ -265,4 +298,5 @@ async def run(): await task except asyncio.CancelledError: pass - self.ownership.close() + finally: + self.ownership.close() diff --git a/spool/collection.py b/spool/collection.py index 641ab55..06a9029 100644 --- a/spool/collection.py +++ b/spool/collection.py @@ -2,10 +2,13 @@ import asyncio import logging +import os import subprocess +import threading import time from . import attempts, joins, receipts, sidecars +from .cloud import storage_call from .farm import FarmCollector from .folder_takes import FolderTakes from .store import Conflict @@ -21,6 +24,16 @@ def __init__(self, store, media): self.history_checked = {} self.retained_at = {} self.playback_checkpoints = {} + self.project_locks = {} + self.project_locks_lock = threading.Lock() + + def project_lock(self, store): + # Routes, local observations and Cloud ingestion share this collector. + # Lock the synchronous file work in its worker thread, per project; a + # cancelled await must not let another worker reuse its partial files. + key = os.path.normcase(os.path.abspath(store.root)) + with self.project_locks_lock: + return self.project_locks.setdefault(key, threading.RLock()) def annotate_progress(self, project): for job in project["document"]["jobs"]: @@ -35,6 +48,10 @@ def annotate_progress(self, project): def ingest(self, attempt): store = self.store.for_project(attempt["project_id"]) + with self.project_lock(store): + return self._ingest(store, attempt) + + def _ingest(self, store, attempt): media = self.media.for_project(store, attempt["project_id"]) observed = store.attempt(attempt["id"])["status"] if observed == attempts.COMPLETE: @@ -61,6 +78,10 @@ def scan(self): logging.exception("Spool project folder is temporarily unavailable: %s", store.root) def scan_project(self, store): + with self.project_lock(store): + self._scan_project(store) + + def _scan_project(self, store): projects = store.list() project_id = projects[0]["id"] if projects else None media = self.media.for_project(store, project_id) @@ -73,7 +94,17 @@ def scan_project(self, store): if attempts.target_of(attempt["request"]) == attempts.DEADLINE: try: farm.collect(attempt) - except (ValueError, KeyError, OSError, RuntimeError, subprocess.SubprocessError) as error: + # Submission files on the farm share may hold valid JSON of the wrong shape. + except ( + ValueError, + KeyError, + OSError, + RuntimeError, + subprocess.SubprocessError, + TypeError, + AttributeError, + IndexError, + ) as error: store.transition( attempt["id"], attempts.COLLECTION_PENDING, error=str(error), expected=attempt["status"] ) @@ -96,17 +127,19 @@ def scan_project(self, store): async def lifecycle(self, app): task = asyncio.create_task(self.run()) - yield - task.cancel() try: - await task - except asyncio.CancelledError: - pass + yield + finally: + task.cancel() + try: + await task + except asyncio.CancelledError: + pass async def run(self): while True: try: - await asyncio.to_thread(self.scan) + await storage_call(self.scan) except Exception: logging.exception("Spool receipt scan failed; evidence retained for retry") await asyncio.sleep(2) diff --git a/spool/commands.py b/spool/commands.py index 323e20a..d05d1a4 100644 --- a/spool/commands.py +++ b/spool/commands.py @@ -398,18 +398,20 @@ def execute(store, media, project_id, data): if attempt["project_id"] != project_id or attempt["clip_id"] not in scope.get("clipIds", []): raise ValueError("Attempt is outside the cancellation scope") submission = store.submission_for(attempt["id"]) - with store.connect() as db: + with store.reading() as db: done = store.cancellation_done(db, attempt["id"]) adapter = adapter_for(attempt["request"], submission) cancellation = asyncio.run( adapter.cancel(attempt, {"stage": "check", "submission": submission, "done": done}) ).get("delivery") if action == "recover_submission": - attempt = store.attempt(arguments["attemptId"]) - if attempt["project_id"] != project_id or attempt["clip_id"] not in scope.get("clipIds", []): + # Named apart from apply()'s cancel locals: a name assigned anywhere in + # apply() is local to all of it, so reading `attempt` there would fail. + recovering = store.attempt(arguments["attemptId"]) + if recovering["project_id"] != project_id or recovering["clip_id"] not in scope.get("clipIds", []): raise ValueError("Submission is outside recovery scope") - submission = store.submission_for(attempt["id"]) - adopted = adapter_for(attempt["request"], submission).recover(attempt, {"submission": submission})["jobId"] + recovered = store.submission_for(recovering["id"]) + adopted = adapter_for(recovering["request"], recovered).recover(recovering, {"submission": recovered})["jobId"] def apply(db, document): clips = [*document["clips"], *document.get("audioClips", [])] if action in note_actions else document["clips"] @@ -524,9 +526,9 @@ def apply(db, document): return plan["result"], False elif action == "recover_submission": if adopted: - store.adopt_submission_job(db, submission["id"], adopted) + store.adopt_submission_job(db, recovered["id"], adopted) return { - "attemptId": attempt["id"], + "attemptId": recovering["id"], "jobId": adopted, "status": "adopted" if adopted else "submission_uncertain", "note": "No automatic resubmission", diff --git a/spool/credentials.py b/spool/credentials.py index 094ca7d..035850f 100644 --- a/spool/credentials.py +++ b/spool/credentials.py @@ -11,17 +11,23 @@ def key_path(): return Path(os.environ.get("LOCALAPPDATA") or Path.home() / ".config") / "Spool" / "comfy-cloud.key" -def cloud_key(): +def _effective_key(): + # One read resolves both the key and its source, so status always matches cloud_key. path = key_path() saved = path.read_text(encoding="utf-8").strip() if path.is_file() else "" - return saved or os.environ.get("COMFY_API_KEY", "").strip() + if saved: + return saved, "saved" + environment = os.environ.get("COMFY_API_KEY", "").strip() + return (environment, "environment") if environment else ("", None) + + +def cloud_key(): + return _effective_key()[0] def key_status(): - return { - "hasApiKey": bool(cloud_key()), - "apiKeySource": "saved" if key_path().is_file() else "environment" if os.environ.get("COMFY_API_KEY") else None, - } + key, source = _effective_key() + return {"hasApiKey": bool(key), "apiKeySource": source} def save_key(value): @@ -34,8 +40,15 @@ def save_key(value): if value: path.parent.mkdir(parents=True, exist_ok=True) pending = path.with_suffix(".tmp") - pending.write_text(value, encoding="utf-8") - pending.chmod(0o600) + # Private from creation: writing first and narrowing afterwards left the key + # readable by other users of the computer until the chmod. + handle = os.open(pending, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + try: + if hasattr(os, "fchmod"): + os.fchmod(handle, 0o600) # a leftover file keeps its old mode otherwise + os.write(handle, value.encode("utf-8")) + finally: + os.close(handle) pending.replace(path) else: path.unlink(missing_ok=True) diff --git a/spool/cuts.py b/spool/cuts.py index a916978..489d150 100644 --- a/spool/cuts.py +++ b/spool/cuts.py @@ -64,6 +64,8 @@ def scene_scores(ffmpeg, video, cache_dir=None): ], capture_output=True, text=True, + encoding="utf-8", + errors="replace", check=True, timeout=600, ) diff --git a/spool/execution.py b/spool/execution.py index 590edeb..a93d85e 100644 --- a/spool/execution.py +++ b/spool/execution.py @@ -9,6 +9,7 @@ from . import requests from .store import uid, Conflict from .collection import Collector +from .cloud import storage_call from .execution_targets import adapter_for @@ -105,7 +106,7 @@ async def queue(self, project_id, revision, items, operation_id=None, actor="hum store = self.store.for_project(project_id) media = self.media.for_project(store, project_id) async with self.lock: - project = await asyncio.to_thread( + project = await storage_call( requests.accept, store, media, @@ -119,8 +120,8 @@ async def queue(self, project_id, revision, items, operation_id=None, actor="hum return project for job in project["document"]["jobs"]: if job["status"] in {attempts.INTENT, attempts.WAITING}: - await self.dispatch(await asyncio.to_thread(store.attempt, job["id"])) - return await asyncio.to_thread(store.get, project_id) + await self.dispatch(await storage_call(store.attempt, job["id"])) + return await storage_call(store.get, project_id) async def dispatch(self, attempt): """Hand one frozen attempt to this Comfy's queue or to Deadline. @@ -137,16 +138,16 @@ async def dispatch(self, attempt): store = self.store.for_project(attempt["project_id"]) media = self.media.for_project(store, attempt["project_id"]) request = attempt["request"] - if self.managed and (await asyncio.to_thread(self.store.host))["sessionId"] != self.session_id: + if self.managed and (await storage_call(self.store.host))["sessionId"] != self.session_id: raise RuntimeError("Spool executor ownership changed") - if not await asyncio.to_thread(store.claim, attempt["id"], self.session_id): + if not await storage_call(store.claim, attempt["id"], self.session_id): return observed = attempts.DISPATCHING # claim atomically moved the intent. async def transition(status, **kwargs): nonlocal observed - changed = await asyncio.to_thread(store.transition, attempt["id"], status, expected=observed, **kwargs) + changed = await storage_call(store.transition, attempt["id"], status, expected=observed, **kwargs) if changed: observed = status return changed @@ -156,7 +157,7 @@ async def transition(status, **kwargs): prepared = adapter.prepare(attempt, {}) package = prepared.get("package") if package is not None: - submission = await asyncio.to_thread(store.submission, attempt["id"], package) + submission = await storage_call(store.submission, attempt["id"], package) package["submissionId"] = submission["id"] payload = prepared["payload"] payload = self.server.trigger_on_prompt(payload) @@ -172,7 +173,7 @@ async def transition(status, **kwargs): else payload.get("partial_execution_targets") ) valid = await self.validate(attempt["prompt_id"], prompt, targets) - await asyncio.to_thread( + await storage_call( store.dispatched, attempt["id"], payload, @@ -195,7 +196,7 @@ async def transition(status, **kwargs): if not checked[0] or check["output"] not in checked[2]: await transition(attempts.FAILED, error=check["error"] + json.dumps(checked[1])) return - await asyncio.to_thread(verify_frozen_inputs, media, request, prompt) + await storage_call(verify_frozen_inputs, media, request, prompt) if not await transition( attempts.DISPATCHING, result={"validation": {"acceptedOutputs": valid[2], "nodeErrors": valid[3]}} ): @@ -206,7 +207,7 @@ async def transition(status, **kwargs): attempt, {**prepared, "server": self.server, "payload": payload, "valid": valid} ) if "jobId" in submitted: - await asyncio.to_thread(store.acknowledge, package["submissionId"], submitted["jobId"]) + await storage_call(store.acknowledge, package["submissionId"], submitted["jobId"]) await transition(submitted["status"]) except Exception as error: await transition(attempts.UNCERTAIN, error=str(error)) @@ -228,6 +229,10 @@ def pending_attempts(self): def observe(self, store, media, attempt, running_ids, pending_ids): """Record what Comfy reports for one local attempt; True when it still needs dispatch.""" + with self.collector.project_lock(store): + return self._observe(store, media, attempt, running_ids, pending_ids) + + def _observe(self, store, media, attempt, running_ids, pending_ids): attempt_id = attempt["id"] observed = attempt["status"] @@ -297,17 +302,17 @@ async def reconcile(self): async with self.lock: running, pending = self.server.prompt_queue.get_current_queue() running_ids, pending_ids = {x[1] for x in running}, {x[1] for x in pending} - for store, media, attempt in await asyncio.to_thread(self.pending_attempts): + for store, media, attempt in await storage_call(self.pending_attempts): if attempts.target_of(attempt["request"]) == attempts.DEADLINE: if attempt["status"] in {attempts.INTENT, attempts.WAITING}: await self.dispatch(attempt) continue if attempt["status"] == attempts.CANCEL_REQUESTED and not ( (store.owner(attempt["id"]) or "").startswith(self.identity) - and await asyncio.to_thread(self.stop, store, attempt, running_ids, pending_ids) + and await storage_call(self.stop, store, attempt, running_ids, pending_ids) ): continue - if await asyncio.to_thread(self.observe, store, media, attempt, running_ids, pending_ids): + if await storage_call(self.observe, store, media, attempt, running_ids, pending_ids): await self.dispatch(attempt) async def poll(self, app): @@ -331,25 +336,29 @@ async def poll(self, app): task = asyncio.create_task(self.run()) yield finally: - if task is not None: - task.cancel() + try: + if task is not None: + task.cancel() + try: + await task + except asyncio.CancelledError: + pass + finally: try: - await task - except asyncio.CancelledError: - pass - if self.managed: - self.store.retire_host(self.session_id) - self.managed = False - self.ownership.close() + if self.managed: + self.store.retire_host(self.session_id) + finally: + self.managed = False + self.ownership.close() async def run(self): while True: try: if self.managed: - await asyncio.to_thread(self.store.publish_host, self.configuration, self.session_id) + await storage_call(self.store.publish_host, self.configuration, self.session_id) await self.reconcile() - await asyncio.to_thread(self.collector.scan) - await asyncio.to_thread(self.notify_progress) + await storage_call(self.collector.scan) + await storage_call(self.notify_progress) except Exception: logging.exception("Spool reconciliation failed; saved attempts remain pending") await asyncio.sleep(2) diff --git a/spool/execution_targets.py b/spool/execution_targets.py index d3cbd1b..388bd73 100644 --- a/spool/execution_targets.py +++ b/spool/execution_targets.py @@ -17,10 +17,16 @@ from typing import Protocol from . import attempts, deadline_job +from .errors import Unavailable from .media import check_frozen, contained from .credentials import cloud_key from .store import Conflict +DEADLINE_LOOKUP_UNAVAILABLE = ( + "Deadline did not answer the recovery lookup; no work was changed and nothing was resubmitted. " + "Retry submission recovery when Deadline is reachable again" +) + class ExecutionAdapter(Protocol): def prepare(self, attempt: dict, context: dict) -> dict: ... @@ -30,6 +36,14 @@ async def cancel(self, attempt: dict, context: dict) -> dict: ... def recover(self, attempt: dict, context: dict) -> dict: ... +def _deadline_lookup(call, *arguments): + """External lookup failures are service unavailability, not Spool bugs.""" + try: + return call(*arguments) + except (OSError, RuntimeError, subprocess.SubprocessError) as error: + raise Unavailable(DEADLINE_LOOKUP_UNAVAILABLE) from error + + def cancellation(attempt, target, submission=None): outcome = attempts.cancel_action(attempt["status"], target, bool(submission and submission["external_id"])) if outcome == "refused": @@ -244,12 +258,12 @@ def recover(self, attempt, context): submission = context.get("submission") if not submission: raise ValueError("This attempt has no Deadline submission") - found = farm.submission_jobs(submission["id"]) + found = _deadline_lookup(farm.submission_jobs, submission["id"]) if len(found) > 1: raise Conflict("Multiple jobs match; operator resolution required") if found and ( - farm.job_value(found[0], "SpoolSubmissionId") != submission["id"] - or farm.job_value(found[0], "SpoolAttemptId") != attempt["id"] + _deadline_lookup(farm.job_value, found[0], "SpoolSubmissionId") != submission["id"] + or _deadline_lookup(farm.job_value, found[0], "SpoolAttemptId") != attempt["id"] ): raise ValueError("Deadline job identity failed ownership verification") return {"jobId": found[0] if found else None} diff --git a/spool/export.py b/spool/export.py index f98e03a..83acb65 100644 --- a/spool/export.py +++ b/spool/export.py @@ -10,7 +10,7 @@ def run_ffmpeg(command, message): - result = subprocess.run(command, capture_output=True, text=True, timeout=180) + result = subprocess.run(command, capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=180) if result.returncode: raise ValueError(result.stderr[-1200:] or message) @@ -82,7 +82,10 @@ def export_sequence(data, *, root, resolve, probe, image_extensions, sources=Non run_ffmpeg(command, "Could not render a clip.") rendered.append(target) concat = Path(temp) / "sequence.txt" - concat.write_text("\n".join(f"file '{p.as_posix()}'" for p in rendered), encoding="utf-8") + # The concat demuxer reads quoted paths; a quote inside one (a Windows user named O'Brien) is '\''. + concat.write_text( + "\n".join("file '" + p.as_posix().replace("'", "'\\''") + "'" for p in rendered), encoding="utf-8" + ) video = Path(temp) / "video.mp4" run_ffmpeg( [ @@ -167,5 +170,12 @@ def export_sequence(data, *, root, resolve, probe, image_extensions, sources=Non else: command += ["-map", "0:v:0", "-an"] command += ["-c:v", "copy", "-t", str(duration), "-movflags", "+faststart", str(output)] - run_ffmpeg(command, "Could not render the audio mix.") + try: + run_ffmpeg(command, "Could not render the audio mix.") + except BaseException: + try: + output.unlink(missing_ok=True) + except OSError: + pass # Preserve the mux failure if cleanup is also denied. + raise return {"url": f"exports/{output.name}", "filename": output.name} diff --git a/spool/farm.py b/spool/farm.py index 0e7d254..0a97747 100644 --- a/spool/farm.py +++ b/spool/farm.py @@ -10,7 +10,8 @@ from pathlib import Path from . import attempts, farm_protocol, joins, sidecars, system -from .media import contained +from .durable import atomic_replace +from .media import contained, unique_partial from .execution_targets import adapter_for @@ -20,6 +21,21 @@ def __init__(self, store, media): self.observed_at = {} self.observations = {} self.verified_receipts = {} + self.submitting_at = {} + + def submission_overdue(self, attempt, row, directory): + identity = attempt["id"] + if row["external_id"] or attempt["status"] in attempts.TERMINAL | {attempts.SUBMISSION_UNCERTAIN}: + self.submitting_at.pop(identity, None) + return False + # The submitter writes this marker immediately before the external call, + # after host validation and asset staging. Attempt age includes queued time. + # First observation gives a conservative fresh grace period after restart, + # without depending on wall clocks or timestamps from the farm share. + if identity not in self.submitting_at and (directory / "submitting.json").is_file(): + self.submitting_at[identity] = time.monotonic() + started = self.submitting_at.get(identity) + return started is not None and time.monotonic() - started > 30 def receipt_checkpoint(self, package, receipt, root): files = [] @@ -38,16 +54,20 @@ def transition(status, **kwargs): changed = self.store.transition(attempt["id"], status, expected=observed, **kwargs) if changed: observed = status + if status in attempts.TERMINAL | {attempts.SUBMISSION_UNCERTAIN}: + self.submitting_at.pop(attempt["id"], None) return changed row = self.store.submission_for(attempt["id"]) if not row: + self.submitting_at.pop(attempt["id"], None) return root = Path(attempt["request"]["prompt"][attempt["request"]["launcherNodeId"]]["inputs"]["output_directory"]) directory = contained(root, "spool-submissions/" + row["id"]) request_file, ack_file = directory / "request.json", directory / "ack.json" + overdue = self.submission_overdue(attempt, row, directory) if not request_file.is_file(): - if attempt["status"] != attempts.COMPLETE and time.time() - attempt["created"] > 30: + if overdue: transition( attempts.SUBMISSION_UNCERTAIN, error="No durable submission package or acknowledgment yet; no automatic resubmission", @@ -79,6 +99,7 @@ def transition(status, **kwargs): if any(ack.get(k) != package[k] for k in ("schemaVersion", "submissionId", "attemptId", "fingerprint")): raise ValueError("Submission acknowledgment belongs to another request") self.store.acknowledge(row["id"], ack["jobId"]) + self.submitting_at.pop(attempt["id"], None) if attempt["status"] not in ( attempts.COMPLETE, attempts.CANCELLED, @@ -90,7 +111,7 @@ def transition(status, **kwargs): transition(attempts.FARM_QUEUED) elif row["external_id"]: ack = {"jobId": row["external_id"]} - elif attempt["status"] != attempts.COMPLETE and time.time() - attempt["created"] > 30: + elif overdue: transition( attempts.SUBMISSION_UNCERTAIN, error="Submission response or acknowledgment unavailable; no automatic resubmission", @@ -126,11 +147,12 @@ def transition(status, **kwargs): ): self.verified_receipts.pop(path, None) farm_protocol.verify(package, receipt, root) - except (ValueError, KeyError, OSError) as error: + # Valid JSON of the wrong shape fails like invalid JSON: the farm share is not trusted. + except (ValueError, KeyError, OSError, TypeError, AttributeError, IndexError) as error: self.verified_receipts.pop(path, None) self.store.reject_receipt(attempt["id"], str(path), str(error)) continue - if not job or job != receipt["jobId"]: + if not job or job != receipt.get("jobId"): self.store.reject_receipt( attempt["id"], str(path), "Worker receipt lacks the owned Deadline job association" ) @@ -172,11 +194,17 @@ def transition(status, **kwargs): receipt["executionId"], ) destination.parent.mkdir(parents=True, exist_ok=True) - pending = destination.with_name(destination.name + ".partial") - shutil.copyfile(source, pending) - if farm_protocol.checksum(pending) != artifact["sha256"]: - raise ValueError("Farm ingestion copy changed checksum") - pending.replace(destination) + pending = unique_partial(destination) + try: + shutil.copyfile(source, pending) + if farm_protocol.checksum(pending) != artifact["sha256"]: + raise ValueError("Farm ingestion copy changed checksum") + atomic_replace(pending, destination) + finally: + try: + pending.unlink(missing_ok=True) + except OSError: + pass # Preserve the copy, checksum or replacement failure if cleanup is denied. metadata = self.media.describe(destination, fps=attempt["request"].get("sequenceFps", 24)) artifacts.append({**metadata, **artifact}) result = { diff --git a/spool/farm_protocol.py b/spool/farm_protocol.py index ae5e8f9..382898c 100644 --- a/spool/farm_protocol.py +++ b/spool/farm_protocol.py @@ -3,6 +3,7 @@ import copy import hashlib import json +import re import shutil import uuid from pathlib import Path @@ -18,6 +19,8 @@ atomic_json = _durable.atomic_json SCHEMA = 2 +# Execution and artifact IDs are uuid4().hex; they name the files Spool writes on ingestion. +IDENTITY = re.compile(r"[0-9a-f]{32}") SAVERS = { "SaveImage": {"images"}, "PreviewImage": {"images"}, @@ -152,13 +155,17 @@ def finalize(package, execution, prompt_id, history, roots, worker, job_id): return receipt +def identity(value): + return isinstance(value, str) and IDENTITY.fullmatch(value) is not None + + def verify(package, receipt, result_root): if receipt.get("schemaVersion") != SCHEMA: raise ValueError("Unsupported farm receipt version") for key in ("projectId", "clipId", "takeId", "attemptId", "fingerprint", "submissionId", "variationIndex"): if receipt.get(key) != package[key]: raise ValueError("Farm receipt belongs to another request: " + key) - if receipt.get("outcome") != "success" or not receipt.get("promptId") or not receipt.get("executionId"): + if receipt.get("outcome") != "success" or not receipt.get("promptId") or not identity(receipt.get("executionId")): raise ValueError("Farm receipt lacks execution identity or success") history = receipt.get("history", {}) if history.get("status", {}).get("status_str") != "success" or not history.get("status", {}).get("completed"): @@ -174,6 +181,8 @@ def verify(package, receipt, result_root): ): raise ValueError("Farm observed prompt differs from the permitted execution copy") artifacts = receipt["artifacts"] + if not all(identity(a["artifactId"]) for a in artifacts): + raise ValueError("Farm receipt has an invalid artifact identity") if len({a["artifactId"] for a in artifacts}) != len(artifacts): raise ValueError("Duplicate artifact identity") reported = { diff --git a/spool/interchange.py b/spool/interchange.py index f619c37..e6e04bc 100644 --- a/spool/interchange.py +++ b/spool/interchange.py @@ -12,6 +12,7 @@ import re import uuid from pathlib import Path +from .durable import atomic_replace from .timing import rate @@ -170,7 +171,19 @@ def gap(frames): output_dir.mkdir(exist_ok=True) stem = re.sub(r"[^\w.-]+", "-", name or "spool").strip("-.") or "spool" output = output_dir / f"{stem}-{uuid.uuid4().hex[:10]}.otio" - pending = output.with_name(output.name + ".partial") - pending.write_text(json.dumps(timeline, indent=2, ensure_ascii=False), encoding="utf-8") - pending.replace(output) + pending = output.with_name(output.name + "." + uuid.uuid4().hex + ".partial") + pending_owned = False + try: + with pending.open("x", encoding="utf-8") as handle: + pending_owned = True + json.dump(timeline, handle, indent=2, ensure_ascii=False) + atomic_replace(pending, output) + pending_owned = False + finally: + if pending_owned: + try: + pending.unlink(missing_ok=True) + except OSError: + # A denied cleanup must not replace the write/publication error. + pass return {"filename": output.name, "path": str(output), "timeline": timeline} diff --git a/spool/joins.py b/spool/joins.py index fca4cf1..4ecf02b 100644 --- a/spool/joins.py +++ b/spool/joins.py @@ -8,7 +8,8 @@ The joined copy fixes all three in the take itself, so the viewer and both exports only play it. Its overlap is the shot's own frames, the last `BLEND` of them dissolve into the re-render, its new frames start exactly where the shot ends, and the first `SETTLE` -new frames are eased back to the shot's brightness. The studio shows it from `BLEND` +new frames are eased back to the shot's brightness. Its audio follows those new frames +on the sequence clock, so it keeps the copy's duration. The studio shows it from `BLEND` frames before the cut while the shot is still the one it was made from (ui/timeline.js:joinedTake). The render and its receipt stay untouched; the copy lives in the project cache and is recorded as the take media's `join`. @@ -21,6 +22,7 @@ from .media import _probe from .system import find_tool +from .timing import rate, tempo_filter BLEND = 2 # frames the shot dissolves into the extension over, before the cut SETTLE = 8 # new frames whose brightness is eased back to the shot's @@ -133,6 +135,25 @@ def _graded(image, gain): ) +def _audio_filter(shift, native, fps, duration): + """Time the render's audio as the joined copy plays its frames. + + Joined frame `j` shows render frame `j + shift`, so the audio starts `shift` + source frames in (or waits when it is negative) and is played at the sequence + rate, exactly as a plain take's source audio is retimed. `duration` cuts it to + the joined copy's own length, padding a short render with silence. + """ + speed = fps / native + offset = shift / native + filters = ["aresample=48000", "asetpts=PTS-STARTPTS"] + if offset > 0: + filters += [f"atrim=start={offset:.9f}", "asetpts=PTS-STARTPTS"] + elif offset < 0: + filters.append(f"adelay={round(-offset * 48000)}S:all=1") + filters += [tempo_filter(speed), f"apad,atrim=duration={duration:.9f}", "asetpts=PTS-STARTPTS"] + return ",".join(filters) + + def build(media, request, primary, joining): """Write the joined copy of `primary` and return its media record, or None.""" ffmpeg = find_tool("ffmpeg") @@ -145,7 +166,11 @@ def build(media, request, primary, joining): # The copy takes the shot's own size: the workflow stretched the shot to its render size, # and a copy that kept that size would be fitted into the shot's frame with bars beside it. shot_video = next(s for s in _probe(guide)["streams"] if s["codec_type"] == "video") + render_probe = _probe(render) + render_video = next((s for s in render_probe["streams"] if s["codec_type"] == "video"), None) overlap, frames = joining["overlap"], primary["frames"] + fps = request.get("sequenceFps", 24) + native = rate(primary.get("fps") or (render_video or {}).get("avg_frame_rate"), fps) size = (int(shot_video["width"]), int(shot_video["height"])) head = measure(ffmpeg, guide, render, overlap) shift, grade = head - overlap, gains(ffmpeg, guide, render, head) @@ -153,19 +178,21 @@ def build(media, request, primary, joining): target = media.cache / "joins" / f"{primary['artifactId']}.mp4" target.parent.mkdir(parents=True, exist_ok=True) pending = target.with_name(target.stem + ".partial.mp4") - encoder = subprocess.Popen( - [ - ffmpeg, "-v", "error", "-y", - "-f", "rawvideo", "-pix_fmt", "rgb24", "-s", f"{size[0]}x{size[1]}", - "-r", str(request.get("sequenceFps", 24)), "-i", "-", - "-i", str(render), "-map", "0:v:0", "-map", "1:a?", "-c:a", "copy", - "-vf", "scale=out_color_matrix=bt709:out_range=tv:flags=accurate_rnd+full_chroma_int,format=yuv420p", - "-c:v", "libx264", "-crf", "14", "-preset", "medium", - "-colorspace", "bt709", "-color_primaries", "bt709", "-color_trc", "iec61966-2-1", - "-movflags", "+faststart", str(pending), - ], - stdin=subprocess.PIPE, stderr=subprocess.PIPE, - ) # fmt: skip + command = [ + ffmpeg, "-v", "error", "-y", + "-f", "rawvideo", "-pix_fmt", "rgb24", "-s", f"{size[0]}x{size[1]}", + "-r", str(fps), "-i", "-", + "-i", str(render), "-map", "0:v:0", + ] # fmt: skip + if any(s["codec_type"] == "audio" for s in render_probe["streams"]): + command += ["-map", "1:a:0", "-af", _audio_filter(shift, native, fps, frames / fps), "-c:a", "aac", "-b:a", "192k"] # fmt: skip + command += [ + "-vf", "scale=out_color_matrix=bt709:out_range=tv:flags=accurate_rnd+full_chroma_int,format=yuv420p", + "-c:v", "libx264", "-crf", "14", "-preset", "medium", + "-colorspace", "bt709", "-color_primaries", "bt709", "-color_trc", "iec61966-2-1", + "-movflags", "+faststart", str(pending), + ] # fmt: skip + encoder = subprocess.Popen(command, stdin=subprocess.PIPE, stderr=subprocess.PIPE) source = _frames(ffmpeg, render, size) state = {"index": -1, "frame": None} diff --git a/spool/launcher.py b/spool/launcher.py index 110508a..9870bdc 100644 --- a/spool/launcher.py +++ b/spool/launcher.py @@ -10,6 +10,7 @@ from aiohttp import ClientSession, ClientConnectorError, ClientError, ClientTimeout +from .cloud import storage_call from .store import Conflict, encode, uid # Scripts that run through /bin/sh when they are not marked executable. @@ -98,8 +99,10 @@ def save(self, data): db.execute("INSERT OR REPLACE INTO settings VALUES(?,?)", ("comfy_startup", encode(settings))) return settings - async def probe(self): - url = local_url(self.settings()["comfyUrl"]) + async def probe(self, address=None): + if address is None: + address = (await storage_call(self.settings))["comfyUrl"] + url = local_url(address) try: async with ClientSession(timeout=ClientTimeout(total=5)) as client: async with client.get(url + "/spool/api/health", allow_redirects=False) as response: @@ -130,31 +133,44 @@ async def probe(self): "The Comfy address did not return its connection details. Check the address before starting another instance.", ) + def _startup_files(self): + # ComfyUI_windows_portable/ComfyUI/custom_nodes/Spool/spool; a shallower checkout is not portable. + above = Path(__file__).resolve().parents + portable = above[4] if len(above) > 4 else None + if not portable or not (portable / "python_embeded/python.exe").is_file(): + return [] + return [str(p) for p in sorted(portable.glob("run*.bat")) if "Deadline" not in p.name] + + def _replace_launch(self, expected, replacement): + """A delayed worker cannot replace a newer startup reservation or state.""" + with self.store.connect() as db: + db.execute("BEGIN IMMEDIATE") + if (self.store.setting("comfy_launch") or {}) != expected: + return False + db.execute("INSERT OR REPLACE INTO settings VALUES(?,?)", ("comfy_launch", encode(replacement))) + return True + async def status(self): - settings = self.settings() - state, error = await self.probe() - launch = self.store.setting("comfy_launch") or {} + settings = await storage_call(self.settings) + state, error = await self.probe(settings["comfyUrl"]) + launch = (await storage_call(self.store.setting, "comfy_launch")) or {} if state == "connected": if launch.get("state") == "starting": - self.store.set_setting("comfy_launch", {**launch, "state": "connected"}) + await storage_call(self._replace_launch, launch, {**launch, "state": "connected"}) elif state == "stopped" and launch.get("state") == "starting": if time.time() - launch["created"] > 180 or self.process and self.process.poll() not in (None, 0): + previous = launch launch = { **launch, "state": "error", "error": "Comfy did not become ready. Check the startup file, its port and the startup log.", } - self.store.set_setting("comfy_launch", launch) + await storage_call(self._replace_launch, previous, launch) else: state = "starting" if state == "stopped" and launch.get("state") == "error": state, error = "error", launch["error"] - portable = Path(__file__).resolve().parents[4] - files = ( - [str(p) for p in sorted(portable.glob("run*.bat")) if "Deadline" not in p.name] - if (portable / "python_embeded/python.exe").is_file() - else [] - ) + files = await storage_call(self._startup_files) return { "settings": settings, "state": state, @@ -170,50 +186,58 @@ async def start(self): return status if status["state"] in ("unpaired", "unavailable"): raise ValueError(status["error"]) - settings = self.save(status["settings"]) + settings = await storage_call(self.save, status["settings"]) if not settings["startupFile"]: raise ValueError("Choose a Comfy startup file in Comfy settings first") identity = uid() log_path = self.store.root / "logs" / ("comfy-startup-" + identity + ".log") - log_path.parent.mkdir(exist_ok=True) - with self.store.connect() as db: - db.execute("BEGIN IMMEDIATE") - launch = self.store.setting("comfy_launch") or {} - if launch.get("state") == "starting" and time.time() - launch["created"] < 180: - return {**status, "state": "starting"} - db.execute( - "INSERT OR REPLACE INTO settings VALUES(?,?)", - ( - "comfy_launch", - encode({"id": identity, "state": "starting", "created": time.time(), "log": str(log_path)}), - ), - ) - path = Path(settings["startupFile"]) - command = [str(path)] - if os.name == "nt" and path.suffix.lower() in (".bat", ".cmd"): - command = '"' + os.environ.get("COMSPEC", "cmd.exe") + '" /d /s /c ""' + str(path) + '""' - elif os.name != "nt" and not os.access(path, os.X_OK): - command = ["/bin/sh", str(path)] - try: - with log_path.open("ab") as log: - # Its own process group or session, so stopping Spool's terminal leaves Comfy running. - self.process = subprocess.Popen( - command, - cwd=path.parent, - env={**os.environ, "SPOOL_STORAGE": str(self.store.root)}, - stdin=subprocess.DEVNULL, - stdout=log, - stderr=log, - creationflags=0x08000200 if os.name == "nt" else 0, - start_new_session=os.name != "nt", - ) - except OSError as error: - self.store.set_setting( - "comfy_launch", {"id": identity, "state": "error", "error": str(error), "log": str(log_path)} - ) - raise ValueError("Comfy could not start: " + str(error)) from error + if not await storage_call(self._launch, settings, identity, log_path): + return {**status, "state": "starting"} return {**status, "state": "starting", "error": None, "logAvailable": True} + def _launch(self, settings, identity, log_path): + """Reserve startup and spawn the child in one worker; cancellation waits for both.""" + log_path.parent.mkdir(exist_ok=True) + with self.store.connect() as db: + db.execute("BEGIN IMMEDIATE") + launch = self.store.setting("comfy_launch") or {} + if launch.get("state") == "starting" and time.time() - launch["created"] < 180: + return False + reservation = {"id": identity, "state": "starting", "created": time.time(), "log": str(log_path)} + db.execute( + "INSERT OR REPLACE INTO settings VALUES(?,?)", + ( + "comfy_launch", + encode(reservation), + ), + ) + path = Path(settings["startupFile"]) + command = [str(path)] + if os.name == "nt" and path.suffix.lower() in (".bat", ".cmd"): + command = '"' + os.environ.get("COMSPEC", "cmd.exe") + '" /d /s /c ""' + str(path) + '""' + elif os.name != "nt" and not os.access(path, os.X_OK): + command = ["/bin/sh", str(path)] + try: + with log_path.open("ab") as log: + # Its own process group or session, so stopping Spool's terminal leaves Comfy running. + process = subprocess.Popen( + command, + cwd=path.parent, + env={**os.environ, "SPOOL_STORAGE": str(self.store.root)}, + stdin=subprocess.DEVNULL, + stdout=log, + stderr=log, + creationflags=0x08000200 if os.name == "nt" else 0, + start_new_session=os.name != "nt", + ) + except OSError as error: + self._replace_launch( + reservation, {"id": identity, "state": "error", "error": str(error), "log": str(log_path)} + ) + raise ValueError("Comfy could not start: " + str(error)) from error + self.process = process + return True + def log(self): launch = self.store.setting("comfy_launch") or {} if not launch.get("log"): @@ -223,7 +247,7 @@ def log(self): return handle.read().decode("utf-8", errors="replace") async def lifecycle(self, app): - if self.settings()["autoStart"]: + if (await storage_call(self.settings))["autoStart"]: try: await self.start() except ValueError as error: diff --git a/spool/maintenance.py b/spool/maintenance.py index 109e74e..62a5bcf 100644 --- a/spool/maintenance.py +++ b/spool/maintenance.py @@ -3,11 +3,13 @@ import argparse import json import os +import shutil from pathlib import Path from .projects import ProjectStore -COMFY = Path(__file__).resolve().parents[3] +# A checkout outside custom_nodes may sit too close to the drive root to have one. +COMFY = Path(__file__).resolve().parents[min(3, len(Path(__file__).resolve().parents) - 1)] def default_root(): @@ -19,6 +21,12 @@ def megabytes(value): return "%.1f MB" % (value / 1e6) +def discard_cache(store): + """Remove the private working copy a confirmed run made; the project folder keeps what it published.""" + if store.private_cache: + shutil.rmtree(store.private_cache, ignore_errors=True) + + def run_prune(argv=None): parser = argparse.ArgumentParser( prog="python -m spool prune", @@ -36,7 +44,10 @@ def run_prune(argv=None): if not (args.storage / "project.spool").is_file(): parser.error("Choose a project folder containing project.spool") store = ProjectStore(args.storage, migrate=False, writable=args.confirm) - report = store.prune(confirm=args.confirm, budget=int(args.budget_gb * 1024**3), grace=args.grace_minutes * 60) + try: + report = store.prune(confirm=args.confirm, budget=int(args.budget_gb * 1024**3), grace=args.grace_minutes * 60) + finally: + discard_cache(store) if args.json: print(json.dumps({**report, "unreferencedPreviews": None}, indent=2, default=str)) return 0 @@ -68,7 +79,10 @@ def run(argv=None): if not (args.storage / "project.spool").is_file(): parser.error("Choose a project folder containing project.spool") store = ProjectStore(args.storage, migrate=False, writable=args.confirm) - report = store.reclaim(confirm=args.confirm) + try: + report = store.reclaim(confirm=args.confirm) + finally: + discard_cache(store) if args.json: print(json.dumps(report, indent=2)) return 0 diff --git a/spool/media.py b/spool/media.py index bb23a5e..3560223 100644 --- a/spool/media.py +++ b/spool/media.py @@ -1,11 +1,14 @@ import copy import hashlib import json +import math import os import shutil import subprocess +import tempfile +from bisect import bisect_right +from fractions import Fraction from pathlib import Path -from urllib.parse import unquote from PIL import Image, ImageDraw, ImageOps @@ -21,33 +24,173 @@ PLAYBACK_VERSION = "playback-2" +# Windows CreateProcess counts the quoted command in UTF-16 code units, +# including its terminating NUL. Leave room below its 32767-unit limit. +_SPLIT_COMMAND_BUDGET = 24000 +_SPLIT_BATCH_FRAMES = 100000 + IMAGE_EXTENSIONS = {".png", ".jpg", ".jpeg", ".webp"} AUDIO_EXTENSIONS = {".wav", ".mp3", ".m4a", ".aac", ".ogg", ".flac", ".opus"} VIDEO_EXTENSIONS = {".mp4", ".mov", ".webm", ".mkv", ".m4v"} EXTENSIONS = VIDEO_EXTENSIONS | IMAGE_EXTENSIONS | AUDIO_EXTENSIONS +class UnreadableMedia(ValueError, subprocess.SubprocessError): + """ffprobe could not read a file: the caller's request is at fault, not Spool.""" + + def _probe(source): ffprobe = find_tool("ffprobe") if not ffprobe: raise ValueError("Install ffprobe for native media review") - result = subprocess.run( - [ffprobe, "-v", "error", "-show_streams", "-show_format", "-of", "json", str(source)], - capture_output=True, - text=True, - encoding="utf-8", - check=True, - timeout=30, - ) + try: + result = subprocess.run( + [ffprobe, "-v", "error", "-show_streams", "-show_format", "-of", "json", str(source)], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + timeout=30, + ) + except subprocess.CalledProcessError as error: + # ffprobe's message names this computer's path, so it stays in the traceback. + raise UnreadableMedia("This file is not readable video, image or audio") from error + except subprocess.TimeoutExpired as error: + raise UnreadableMedia("Reading this media took too long") from error return json.loads(result.stdout) def _rate(value): - numerator, _, denominator = str(value).partition("/") - rate = float(numerator) / float(denominator or 1) - if rate <= 0: + """A usable positive source rate from ffprobe, or a caller-facing validation error. + + ffprobe reports missing video clocks as `0/0`, and broken metadata may hold + `nan` or an infinity. Import needs a real source rate, so never propagate a + division error or a nonfinite timing. + """ + try: + numerator, separator, denominator = str(value).strip().partition("/") + if separator and not denominator.strip(): + result = math.nan + else: + result = float(numerator) / float(denominator or 1) + except (TypeError, ValueError, ZeroDivisionError, OverflowError): + result = math.nan + if not math.isfinite(result) or result <= 0: raise ValueError("The video has no usable frame rate") - return rate + return result + + +def _exact_rate(value, fallback): + """An exact frame rate for frame-number timing, preferring ffprobe's rational. + + A float approximation would drift against the proxy's rational frame clock + when seeking into later batches. ffprobe reports that clock exactly. + """ + try: + result = Fraction(str(value)) + except (TypeError, ValueError, ZeroDivisionError): + result = Fraction(str(fallback)).limit_denominator(1_000_000) + if result <= 0: + result = Fraction(str(fallback)).limit_denominator(1_000_000) + return result + + +def _command_length(command): + return len(subprocess.list2cmdline(command).encode("utf-16-le")) // 2 + 1 + + +def _split_command(ffmpeg, proxy, folder, cuts, start, end, first, frame_rate, time_base, start_pts): + """Encode one contiguous batch, with frame numbers local to its first shot.""" + command = [ffmpeg, "-v", "error", "-y", "-i", str(proxy)] + if start: + # Seek one frame early, then select the exact original timestamp. copyts + # prevents seek-time rounding from moving the trim boundary. The proxy + # has six-frame GOPs, so each batch decodes only a few preceding frames. + seek = (start - 1) * 1_000_000 // frame_rate + command = [ffmpeg, "-v", "error", "-y", "-copyts", "-ss", f"{seek}us", "-i", str(proxy)] + command += ["-vf", f"trim=start_pts={start_pts + round(start / frame_rate / time_base)},setpts=PTS-STARTPTS"] + command += [ + "-an", + "-frames:v", + str(end - start), + "-c:v", + "libx264", + "-preset", + "ultrafast", + "-pix_fmt", + "yuv420p", + "-g", + "6", + "-bf", + "0", + ] + if cuts: + local = [cut - start for cut in cuts] + # FFmpeg rejects deeply nested expressions, including long flat sums. + # Pair terms so parser depth grows logarithmically with the cut count. + terms = [f"eq(n,{cut})" for cut in local] + while len(terms) > 1: + terms = [ + f"({terms[index]}+{terms[index + 1]})" if index + 1 < len(terms) else terms[index] + for index in range(0, len(terms), 2) + ] + command += [ + "-force_key_frames", + "expr:" + terms[0], + "-f", + "segment", + "-segment_frames", + ",".join(map(str, local)), + "-segment_start_number", + str(first), + "-reset_timestamps", + "1", + str(folder / "%03d.mp4"), + ] + else: + command.append(str(folder / f"{first:03d}.mp4")) + return command + + +def _split_commands(ffmpeg, proxy, folder, cuts, frames, frame_rate, time_base, start_pts=0): + """Bound both explicit cut lists without losing short GOPs inside each shot. + + Batches meet at actual cuts and encode disjoint frame ranges. The frame cap + is a grouping target: a single longer shot stays in one batch. The command + budget is hard and includes quoted paths, Unicode and the terminating NUL. + """ + boundaries = [0, *cuts, frames] + if any(end <= start for start, end in zip(boundaries, boundaries[1:])): + raise ValueError("Detected cuts must increase inside the source") + first = 0 + while first < len(boundaries) - 1: + start = boundaries[first] + last = max(first + 1, bisect_right(boundaries, start + _SPLIT_BATCH_FRAMES) - 1) + low, high = first + 1, last + command = None + while low <= high: + middle = (low + high) // 2 + candidate = _split_command( + ffmpeg, + proxy, + folder, + boundaries[first + 1 : middle], + start, + boundaries[middle], + first, + frame_rate, + time_base, + start_pts, + ) + if _command_length(candidate) <= _SPLIT_COMMAND_BUDGET: + command, last = candidate, middle + low = middle + 1 + else: + high = middle - 1 + if command is None: + raise ValueError("The media path is too long for FFmpeg's command line") + yield command + first = last def _video_stream(probe): @@ -61,6 +204,27 @@ def _video_stream(probe): ) +def _video_display_size(stream): + """Pixel dimensions after ffmpeg's default autorotation, not the coded frame. + + A display matrix supersedes the older rotate tag. FFmpeg transposes quarter + turns (within one degree); its arbitrary-angle rotate keeps the coded canvas. + Reflections change the pixels but do not change this size rule. + """ + matrix = next( + (item for item in stream.get("side_data_list", []) if item.get("side_data_type") == "Display Matrix"), None + ) + rotation = matrix.get("rotation", 0) if matrix is not None else stream.get("tags", {}).get("rotate", 0) + try: + rotation = float(rotation) % 360 + except (TypeError, ValueError): + rotation = 0 + width, height = stream["width"], stream["height"] + if math.isfinite(rotation) and (abs(rotation - 90) < 1 or abs(rotation - 270) < 1): + return height, width + return width, height + + def _review_probe(path, fps, run=None): run = run or subprocess.run probe = _probe(path) @@ -127,6 +291,12 @@ def resolved_root(root): return held +def unique_partial(destination): + """A sibling temporary file only the copy that created it may publish or remove.""" + destination = Path(destination) + return destination.with_name(destination.name + "." + uid() + ".partial") + + def contained(root, relative): """Resolve a stored relative path, refusing anything outside its root. @@ -136,8 +306,10 @@ def contained(root, relative): containment depended on whether a directory had already been created. """ base = resolved_root(str(root)) - target = Path(os.path.normpath(os.path.join(str(base), unquote(relative)))) - key, prefix = os.path.normcase(str(target)), os.path.normcase(str(base)) + # Not URL-decoded: aiohttp decodes route paths once, and a stored name may hold a literal "%41". + target = Path(os.path.normpath(os.path.join(str(base), relative))) + # A drive or share root ("E:\\", "\\\\nas\\projects\\") already ends in a separator. + key, prefix = os.path.normcase(str(target)).rstrip(os.sep), os.path.normcase(str(base)).rstrip(os.sep) if key == prefix or not key.startswith(prefix + os.sep): raise ValueError("Media path escapes its storage root") return target @@ -166,6 +338,20 @@ def check_frozen(path, asset, message="A frozen input changed before execution") raise ValueError(message) +def encode(command, target, timeout): + """Run an ffmpeg command whose last argument is `target`, publishing the file only when it finishes. + + Cache files are reused whenever they exist, so an encode cut short by a timeout, + a stopped host or a full disk must not leave a truncated file at that name. + """ + pending = target.with_name(target.stem + "-" + uid() + target.suffix) + try: + subprocess.run([*command, str(pending)], check=True, capture_output=True, timeout=timeout) + pending.replace(target) + finally: + pending.unlink(missing_ok=True) + + class Media: def __init__(self, root, input_root, output_root, temp_root, project_id=None, playback_cache=None, playback=None): self.root = Path(root).resolve() @@ -277,8 +463,15 @@ def stage_input(self, source): source.stat().st_mtime_ns, ): pending = target.with_name(uid() + ".partial") - shutil.copy2(source, pending) - pending.replace(target) + try: + shutil.copy2(source, pending) + pending.replace(target) + except BaseException: + try: + pending.unlink(missing_ok=True) + except OSError: + pass # Preserve the staging failure if cleanup is also denied. + raise return name def prepare_input(self, value): @@ -333,11 +526,17 @@ def describe(self, path, name=None, fps=24): with Image.open(path) as image: if getattr(image, "n_frames", 1) > 1: raise ValueError("Animated image review is not supported yet. Import a video instead.") + width, height = image.size + # Pillow applies these orientations, including mirrored quarter + # turns, when preparing generation inputs and previews. Leave the + # original file untouched so its pixels are not rotated twice. + if image.getexif().get(274) in (5, 6, 7, 8): + width, height = height, width result.update( { "kind": "image", - "width": image.width, - "height": image.height, + "width": width, + "height": height, "frames": round(5 * fps), "poster": relative, } @@ -366,10 +565,11 @@ def describe(self, path, name=None, fps=24): if not ffmpeg: raise ValueError("Install ffmpeg for native media review") if video: + width, height = _video_display_size(video) result.update( { - "width": video["width"], - "height": video["height"], + "width": width, + "height": height, "fps": video.get("avg_frame_rate"), "timeBase": video.get("time_base"), "sourceFrameCount": video.get("nb_frames"), @@ -382,7 +582,7 @@ def describe(self, path, name=None, fps=24): ) proxy = previews / (path.stem + "-" + hashlib.sha256(signature.encode()).hexdigest()[:24] + ".mp4") if not proxy.exists(): - subprocess.run( + encode( [ ffmpeg, "-v", @@ -416,11 +616,9 @@ def describe(self, path, name=None, fps=24): "-shortest", "-movflags", "+faststart", - str(proxy), ], - check=True, - capture_output=True, - timeout=300, + proxy, + 300, ) result["file"] = proxy.relative_to(self.root).as_posix() proxy_probe = _review_probe(proxy, fps) @@ -442,22 +640,25 @@ def describe(self, path, name=None, fps=24): } poster = proxy.with_suffix(".jpg") if not poster.exists(): - subprocess.run( - [ffmpeg, "-v", "error", "-y", "-i", str(proxy), "-frames:v", "1", str(poster)], - check=True, - capture_output=True, - timeout=30, - ) + encode([ffmpeg, "-v", "error", "-y", "-i", str(proxy), "-frames:v", "1"], poster, 30) result["poster"] = poster.relative_to(self.root).as_posix() if audio: - wave = previews / ( - path.stem - + "-" - + hashlib.sha256((str(path.resolve()) + "-wave-1").encode()).hexdigest()[:24] - + "-wave.png" + wave = ( + previews + / ( + path.stem + + "-" + # Like the proxy's key: new bytes at the same path draw a new waveform. + + hashlib.sha256( + json.dumps( + [str(path.resolve()), path.stat().st_size, path.stat().st_mtime_ns, "wave-2"] + ).encode() + ).hexdigest()[:24] + + "-wave.png" + ) ) if not wave.exists(): - subprocess.run( + encode( [ ffmpeg, "-v", @@ -469,11 +670,9 @@ def describe(self, path, name=None, fps=24): "aformat=channel_layouts=mono,showwavespic=s=1280x80:colors=0x91dab1", "-frames:v", "1", - str(wave), ], - check=True, - capture_output=True, - timeout=60, + wave, + 60, ) result["wave"] = wave.relative_to(self.root).as_posix() if not video: @@ -481,8 +680,10 @@ def describe(self, path, name=None, fps=24): return result def import_footage(self, source, fps=None, detect=True): - """Reference a local video in place and split its review proxy into one file per detected shot. + """Split a local video's review proxy into one file per detected shot. + Project imports retain their owned copy using a project-relative original + path. Unscoped imports keep the existing absolute in-place reference. Shots carry no audio; the source's own audio track is returned separately for the soundtrack lane.""" source = Path(source) if not source.is_absolute() or not source.is_file() or source.suffix.lower() not in VIDEO_EXTENSIONS: @@ -490,132 +691,120 @@ def import_footage(self, source, fps=None, detect=True): video = _video_stream(_probe(source)) if not video: raise ValueError("The file has no video stream") - fps = fps or _rate(video["avg_frame_rate"]) + fps = fps or _rate(video.get("avg_frame_rate")) whole = self.describe(source, source.name, fps) + original = whole["originalFile"] if self.project_id else str(source) ffmpeg = find_tool("ffmpeg") proxy = self.root / whole["file"] cuts = detect_cuts(scene_scores(ffmpeg, proxy, self.cache / "previews" / "cut-scores"), fps) if detect else [] + frame_rate = time_base = None + start_pts = 0 + if cuts: + proxy_video = _video_stream(_probe(proxy)) or {} + frame_rate = _exact_rate(proxy_video.get("avg_frame_rate"), fps) + time_base = Fraction(proxy_video.get("time_base", "1/1000000")) + start_pts = int(proxy_video.get("start_pts", 0)) folder = self.cache / "shots" / uid() folder.mkdir(parents=True) - command = [ - ffmpeg, - "-v", - "error", - "-y", - "-i", - str(proxy), - "-an", - "-c:v", - "libx264", - "-preset", - "ultrafast", - "-pix_fmt", - "yuv420p", - "-g", - "6", - "-bf", - "0", - ] - if cuts: - command += [ - "-force_key_frames", - "expr:" + "+".join(f"eq(n,{cut})" for cut in cuts), - "-f", - "segment", - "-segment_frames", - ",".join(map(str, cuts)), - "-reset_timestamps", - "1", - str(folder / "%03d.mp4"), - ] - else: - command.append(str(folder / "000.mp4")) - subprocess.run(command, check=True, capture_output=True, timeout=600) - shots = [] - for index, (start, end) in enumerate(zip([0, *cuts], [*cuts, whole["frames"]])): - file = folder / f"{index:03d}.mp4" - count = int(_probe(file)["streams"][0]["nb_frames"]) - if count != end - start: - raise ValueError( - f"Shot {index + 1} has {count} frames instead of {end - start}; the source could not be split exactly" + track = None + try: + for command in _split_commands( + ffmpeg, proxy, folder, cuts, whole["frames"], frame_rate, time_base, start_pts + ): + subprocess.run(command, check=True, capture_output=True, timeout=600) + shots = [] + for index, (start, end) in enumerate(zip([0, *cuts], [*cuts, whole["frames"]])): + file = folder / f"{index:03d}.mp4" + count = int(_probe(file)["streams"][0]["nb_frames"]) + if count != end - start: + raise ValueError( + f"Shot {index + 1} has {count} frames instead of {end - start}; the source could not be split exactly" + ) + poster = file.with_suffix(".jpg") + subprocess.run( + [ + ffmpeg, + "-v", + "error", + "-y", + "-ss", + str(count / 2 / fps), + "-i", + str(file), + "-frames:v", + "1", + str(poster), + ], + check=True, + capture_output=True, + timeout=30, ) - poster = file.with_suffix(".jpg") - subprocess.run( - [ - ffmpeg, - "-v", - "error", - "-y", - "-ss", - str(count / 2 / fps), - "-i", - str(file), - "-frames:v", - "1", - str(poster), - ], - check=True, - capture_output=True, - timeout=30, - ) - shots.append( - { - "id": uid(), - "kind": "video", - "name": f"Shot {index + 1:02d}", - "file": file.relative_to(self.root).as_posix(), - "originalFile": str(source), - "poster": poster.relative_to(self.root).as_posix(), - "frames": count, - "duration": count / fps, - "audio": False, - "sequenceFps": fps, - "fps": whole["fps"], - "width": whole["width"], - "height": whole["height"], - "sourceRange": {"start": start, "frames": count}, - "sourceFrameCount": whole["sourceFrameCount"], - "sourceDuration": whole["sourceDuration"], - "frameRateMode": "interpret", - "review": {**whole["review"], "frames": count, "duration": count / fps}, - } - ) - audio = None - if whole["audio"]: - track = self.cache / "previews" / (source.stem + "-" + uid() + "-audio.m4a") - subprocess.run( - [ - ffmpeg, - "-v", - "error", - "-y", - "-i", - str(proxy), - "-map", - "0:a:0", - "-vn", - "-c:a", - "aac", - "-b:a", - "256k", - str(track), - ], - check=True, - capture_output=True, - timeout=300, - ) - audio = self.describe(track, source.stem + " audio", fps) - audio["originalFile"] = str(source) - audio.update( - fps=whole["fps"], - frames=whole["frames"], - duration=whole["duration"], - sourceFrameCount=whole["sourceFrameCount"], - sourceDuration=whole["sourceDuration"], - frameRateMode="interpret", - review={"frameRateMode": "interpret"}, - ) - return {"fps": fps, "source": whole, "shots": shots, "audio": audio} + shots.append( + { + "id": uid(), + "kind": "video", + "name": f"Shot {index + 1:02d}", + "file": file.relative_to(self.root).as_posix(), + "originalFile": original, + "poster": poster.relative_to(self.root).as_posix(), + "frames": count, + "duration": count / fps, + "audio": False, + "sequenceFps": fps, + "fps": whole["fps"], + "width": whole["width"], + "height": whole["height"], + "sourceRange": {"start": start, "frames": count}, + "sourceFrameCount": whole["sourceFrameCount"], + "sourceDuration": whole["sourceDuration"], + "frameRateMode": "interpret", + "review": {**whole["review"], "frames": count, "duration": count / fps}, + } + ) + audio = None + if whole["audio"]: + track = self.cache / "previews" / (source.stem + "-" + uid() + "-audio.m4a") + subprocess.run( + [ + ffmpeg, + "-v", + "error", + "-y", + "-i", + str(proxy), + "-map", + "0:a:0", + "-vn", + "-c:a", + "aac", + "-b:a", + "256k", + str(track), + ], + check=True, + capture_output=True, + timeout=300, + ) + audio = self.describe(track, source.stem + " audio", fps) + audio["originalFile"] = original + audio.update( + fps=whole["fps"], + frames=whole["frames"], + duration=whole["duration"], + sourceFrameCount=whole["sourceFrameCount"], + sourceDuration=whole["sourceDuration"], + frameRateMode="interpret", + review={"frameRateMode": "interpret"}, + ) + return {"fps": fps, "source": whole, "shots": shots, "audio": audio} + except BaseException: + shutil.rmtree(folder, ignore_errors=True) + if track is not None: + try: + track.unlink(missing_ok=True) + except OSError: + pass # Preserve the ffmpeg or verification failure if cleanup is denied. + raise def reported(self, record): file_type = record.get("type", "output") @@ -650,9 +839,15 @@ def collect(self, attempt, history, review=True): selected = (node_id, group, index) == (selection["nodeId"], selection["group"], selection["index"]) destination = self.result_path(attempt, artifact_id, source.suffix.lower(), selected) destination.parent.mkdir(parents=True, exist_ok=True) - pending = destination.with_suffix(destination.suffix + ".partial") - shutil.copyfile(source, pending) - pending.replace(destination) + pending = unique_partial(destination) + try: + shutil.copyfile(source, pending) + atomic_replace(pending, destination) + finally: + try: + pending.unlink(missing_ok=True) + except OSError: + pass # Preserve the copy or replacement failure if cleanup is denied. metadata = ( self.describe(destination, fps=request.get("sequenceFps", 24)) if review @@ -1052,6 +1247,8 @@ def bake_audio(self, segments, fps, frames): ], capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=1800, ) if result.returncode: @@ -1168,7 +1365,7 @@ def preview_result(self, metadata, width, height, fps, output=None): "offset": metadata.get("sourceRange", {}).get("start", 0) / fps, } - def export_frame(self, metadata, frame, fps): + def _write_frame(self, metadata, frame, output): frame = int(frame) if metadata["kind"] not in ("video", "image") or not 0 <= frame < metadata["frames"]: raise ValueError("Choose a frame inside the footage") @@ -1176,9 +1373,7 @@ def export_frame(self, metadata, frame, fps): if not ffmpeg: raise ValueError("Install ffmpeg to export frames") original = self.original_source(metadata) - folder = self.root / "exports" - folder.mkdir(exist_ok=True) - output = folder / f"spool-frame-{frame:06d}-{uid()[:8]}.png" + output.parent.mkdir(exist_ok=True) command = [ffmpeg, "-v", "error", "-y"] if metadata["kind"] == "video": command += [ @@ -1193,6 +1388,11 @@ def export_frame(self, metadata, frame, fps): subprocess.run(command, check=True, capture_output=True, timeout=60) if not output.is_file(): raise ValueError("The source has no frame at this position") + + def export_frame(self, metadata, frame, fps): + frame = int(frame) + output = self.root / "exports" / f"spool-frame-{frame:06d}-{uid()[:8]}.png" + self._write_frame(metadata, frame, output) return { "url": ("/spool/media/" + self.project_id + "/exports/" if self.project_id else "/spool/media/exports/") + output.name, @@ -1212,25 +1412,34 @@ def comparison(self, clip, take_ids, fps): canvas = Image.new("RGB", (columns * 384, ((len(takes) + columns - 1) // columns) * 256), "#181818") drawing = ImageDraw.Draw(canvas) evidence = [] - for index, take in enumerate(takes): - metadata = take["media"] - frame = min( - metadata["frames"] - 1, - max(0, (clip.get("in", 0) + min(clip.get("out", metadata["frames"]), metadata["frames"]) - 1) // 2), - ) - result = self.export_frame(metadata, frame, fps) - with Image.open(self.root / "exports" / result["filename"]) as image: - tile = ImageOps.contain(image.convert("RGB"), (384, 216)) - x, y = index % columns * 384, index // columns * 256 - canvas.paste(tile, (x + (384 - tile.width) // 2, y + (216 - tile.height) // 2)) - drawing.text((x + 8, y + 224), take["id"][:32], fill="white") - evidence.append( - { - "takeId": take["id"], - "sampleFrame": frame, - **{k: metadata.get(k) for k in ("kind", "duration", "width", "height", "fps", "audio")}, - } - ) + temporary = tempfile.TemporaryDirectory(prefix="spool-comparison-", dir=self.cache, ignore_cleanup_errors=True) + try: + for index, take in enumerate(takes): + metadata = take["media"] + frame = min( + metadata["frames"] - 1, + max(0, (clip.get("in", 0) + min(clip.get("out", metadata["frames"]), metadata["frames"]) - 1) // 2), + ) + output = Path(temporary.name) / (str(index) + ".png") + self._write_frame(metadata, frame, output) + with Image.open(output) as image: + tile = ImageOps.contain(image.convert("RGB"), (384, 216)) + x, y = index % columns * 384, index // columns * 256 + canvas.paste(tile, (x + (384 - tile.width) // 2, y + (216 - tile.height) // 2)) + drawing.text((x + 8, y + 224), take["id"][:32], fill="white") + evidence.append( + { + "takeId": take["id"], + "sampleFrame": frame, + **{k: metadata.get(k) for k in ("kind", "duration", "width", "height", "fps", "audio")}, + } + ) + finally: + try: + temporary.cleanup() + except OSError: + pass # Retain the comparison result or original failure if cleanup is denied. + (self.root / "exports").mkdir(exist_ok=True) target = self.root / "exports" / ("comparison-" + uid() + ".png") canvas.save(target) return { diff --git a/spool/ownership.py b/spool/ownership.py index 4110371..1920275 100644 --- a/spool/ownership.py +++ b/spool/ownership.py @@ -63,9 +63,6 @@ def file_lock(path): # One coarse lock covers refresh, transaction and atomic publication. No live # SQLite/WAL file is opened on the share, including by the offline editor. with path.open("a+b") as handle: - if handle.tell() == 0: - handle.write(b"0") - handle.flush() deadline = time.monotonic() + 30 while True: handle.seek(0) @@ -82,6 +79,11 @@ def file_lock(path): raise Conflict("Project is busy in another editor. Try again.") from error time.sleep(0.05) try: + # Byte locks may cover an empty file. Initialize only after taking + # the lock: a read-only maintenance copy may already own byte zero. + if handle.seek(0, os.SEEK_END) == 0: + handle.write(b"0") + handle.flush() yield finally: if os.name == "nt": diff --git a/spool/projects.py b/spool/projects.py index a1ef946..51c184c 100644 --- a/spool/projects.py +++ b/spool/projects.py @@ -1,6 +1,7 @@ """Project folders are durable; SQLite working files stay on the editing computer.""" import copy +import errno import hashlib import json import logging @@ -8,28 +9,110 @@ import re import shutil import sqlite3 +import stat import tempfile import threading +import time from datetime import datetime, timezone -from contextlib import contextmanager, closing +from contextlib import contextmanager, closing, nullcontext from pathlib import Path from .errors import NotFound from . import take_files from .media import Media from .ownership import file_lock, atomic_replace -from .store import Store, Conflict, uid, SCHEMA_VERSION, validate_import, read_only_uri +from .store import Store, Conflict, uid, SCHEMA_VERSION, encode, validate_import, read_only_uri from .durable import atomic_json, write as durable_write from . import __version__ +@contextmanager +def existing_project_lock(path): + """Coordinate a copy without creating or writing the project's lock file. + + Legacy folders can have no lock yet. They remain inspectable, but a writer + opening one concurrently can still encounter the source-copy handle. Once + a lock exists, hold its same byte until that handle has closed on Windows. + """ + try: + source = path.open("rb") + except FileNotFoundError: + yield + return + with source: + if os.name == "nt": + import ctypes + import msvcrt + from ctypes import wintypes + + kernel = ctypes.WinDLL("kernel32", use_last_error=True) + lock, unlock = kernel.LockFile, kernel.UnlockFile + for operation in (lock, unlock): + operation.argtypes = (wintypes.HANDLE, wintypes.DWORD, wintypes.DWORD, wintypes.DWORD, wintypes.DWORD) + operation.restype = wintypes.BOOL + handle = msvcrt.get_osfhandle(source.fileno()) + + def acquire(): + # Unlike CRT locking, LockFile accepts an rb handle, including + # an empty file: locking beyond EOF does not extend it. + if not lock(handle, 0, 0, 1, 0): + raise ctypes.WinError(ctypes.get_last_error()) + + def release(): + if not unlock(handle, 0, 0, 1, 0): + raise ctypes.WinError(ctypes.get_last_error()) + + else: + import fcntl + + def acquire(): + fcntl.flock(source, fcntl.LOCK_EX | fcntl.LOCK_NB) + + def release(): + fcntl.flock(source, fcntl.LOCK_UN) + + deadline = time.monotonic() + 30 + while True: + try: + acquire() + break + except OSError as error: + retryable = ( + getattr(error, "winerror", None) in (32, 33) + if os.name == "nt" + else error.errno in (errno.EACCES, errno.EAGAIN) + ) + if not retryable: + raise + if time.monotonic() >= deadline: + raise Conflict("Project is busy in another editor. Try again.") from error + time.sleep(0.05) + try: + yield + finally: + release() + + +@contextmanager +def private_snapshot(path): + """Copy one immutable, atomically published generation to a private directory.""" + with tempfile.TemporaryDirectory(prefix="spool-maintenance-read-") as directory: + local = Path(directory) / "read.sqlite3" + with existing_project_lock(path.parent / ".spool" / "project.lock"): + with path.open("rb") as source, local.open("xb") as target: + shutil.copyfileobj(source, target) + yield local + + def folder_name(name): - name = re.sub(r'[<>:"/\\|?*\x00-\x1f]', "-", str(name)).strip(" .")[:100] + name = re.sub(r'[<>:"/\\|?*\x00-\x1f]', "-", str(name)).strip(" .")[:100].rstrip(" .") if not name or name.upper().split(".")[0] in { "CON", "PRN", "AUX", "NUL", + "CONIN$", + "CONOUT$", *["COM" + str(i) for i in range(1, 10)], *["LPT" + str(i) for i in range(1, 10)], }: @@ -37,21 +120,95 @@ def folder_name(name): return name +def working_cache(workspace, folder): + """The local editing cache a ProjectStore derives from one project folder. + + Import cleanup needs this identity before a failed constructor leaves no store, + and it must stay exactly the path ProjectStore.__init__ uses. + """ + folder = Path(folder).resolve() + key = hashlib.sha256(str(folder).encode()).hexdigest() + return Path(workspace.root) / "cache" / key + + +def _redirected(path): + """Reject redirects and paths whose redirection status cannot be inspected.""" + try: + info = path.lstat() + except FileNotFoundError: + return False + except OSError: + return True # Unreadable metadata cannot authorize recursive cleanup. + if stat.S_ISLNK(info.st_mode): + return True + return bool(getattr(info, "st_file_attributes", 0) & getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0)) + + +def _discard_import(workspace, folder, destination, parent, cache, cache_existed): + """Remove only the folder and cache one failed import created. + + Constructor, import transaction and registration failures can leave the local + working cache behind even when the project folder is gone. Cleanup is best + effort so the caller still raises its original error. + """ + if cache is None: + # Cache setup failed before construction. Remove only the empty leaf + # mkdtemp created under the already resolved projects directory. + try: + if folder.parent == parent and not _redirected(folder): + folder.rmdir() + except OSError: + pass + return + try: + if destination.parent == parent and folder.resolve() == destination and not _redirected(folder): + shutil.rmtree(destination) + except Exception: + pass # Preserve the import failure if cleanup is also denied. + if cache_existed: + return + try: + cache_root = Path(workspace.root) / "cache" + # A redirected cache root or cache could make the delete reach outside this + # workspace. Requiring the exact child of the real cache root also keeps a + # broad path from ever reaching the root itself. + if _redirected(cache_root) or _redirected(cache) or cache != working_cache(workspace, destination): + return + root, target = cache_root.resolve(), cache.resolve() + if target.parent != root or target == root or not target.is_dir(): + return + shutil.rmtree(target) + except Exception: + pass # Preserve the import failure if cleanup is also denied. + + class ProjectStore(Store): def __init__(self, folder, workspace=None, *, migrate=True, writable=False): self.workspace = workspace self.migrate_on_open = migrate + self.publication_lock = threading.Lock() + self.publication_cache = None folder = Path(folder).resolve() self.manifest = folder / "project.json" self.check_manifest() self.snapshot = folder / "project.spool" + read_only = not (migrate or writable) # Refuse newer SQLite formats before creating even the lock/cache folders. if self.snapshot.exists(): - with closing(sqlite3.connect(read_only_uri(self.snapshot), uri=True)) as db: - if db.execute("PRAGMA user_version").fetchone()[0] > SCHEMA_VERSION: - raise ValueError("This Spool project needs a newer extension version") - key = hashlib.sha256(str(folder).encode()).hexdigest() - cache = (workspace.root / "cache" if workspace else Path(tempfile.gettempdir()) / "spool-maintenance") / key + with private_snapshot(self.snapshot) if read_only else nullcontext(self.snapshot) as path: + with closing(sqlite3.connect(read_only_uri(path), uri=True)) as db: + if db.execute("PRAGMA user_version").fetchone()[0] > SCHEMA_VERSION: + raise ValueError("This Spool project needs a newer extension version") + # Maintenance without a workspace trusts whatever working copy its cache holds, so + # a writable one gets a folder of its own: another user could fill a shared path. + self.private_cache = None + if workspace: + cache = working_cache(workspace, folder) + elif migrate or writable: + cache = self.private_cache = Path(tempfile.mkdtemp(prefix="spool-maintenance-")) + else: + key = hashlib.sha256(str(folder).encode()).hexdigest() + cache = Path(tempfile.gettempdir()) / "spool-maintenance" / key # never created when read-only self.runtime = cache / "working.sqlite3" self.stamp = cache / "checkpoint.json" self.read_runtime = cache / "read.sqlite3" @@ -93,26 +250,91 @@ def write_manifest(self, db): } atomic_json(self.manifest, value) - def signature(self): + def signature(self, *, locked=False): + """Name one published snapshot, not merely a timestamp and a size. + + FAT, HFS+ and some SMB servers give files a one- or two-second timestamp, and a + SQLite snapshot usually keeps its size. A save in the same tick therefore used to + look unchanged, so a reader kept an old revision and could later save over newer + work. Every publication puts a fresh UUID in the snapshot itself. The stat fields + still make an old snapshot without that marker detectable when it is replaced, and + they let a reader reuse the cached marker without opening the share on every poll. + Cache misses open SQLite under the project lock so its Windows handle cannot + prevent a concurrent replacement. Callers holding that lock pass locked=True. + """ stat = self.snapshot.stat() - return [stat.st_mtime_ns, stat.st_size] + fields = [stat.st_mtime_ns, stat.st_size, stat.st_dev, stat.st_ino, stat.st_ctime_ns] + key = (stat.st_dev, stat.st_ino, stat.st_mtime_ns, stat.st_ctime_ns, stat.st_size) + # Whole-millisecond or whole-second timestamps are the filesystems where two + # publications can share every visible stat field; there the marker is read each + # time instead of trusted from the cache. + cacheable = stat.st_ino != 0 and stat.st_mtime_ns % 1_000_000 and stat.st_ctime_ns % 1_000_000 + with self.publication_lock: + if cacheable and self.publication_cache is not None and self.publication_cache[0] == key: + return [*fields, self.publication_cache[1]] + if not locked: + with file_lock(self.lock_file): + # Publication may have changed while waiting: stat and marker must + # identify the same file, and a file lock must never be reacquired. + return self.signature(locked=True) + value = self.read_publication() + # A snapshot is never edited in place: publications replace the file. File identity + # lets unchanged polls avoid opening SQLite while a replacement still misses the cache. + with self.publication_lock: + if cacheable: + self.publication_cache = (key, value) + return [*fields, value] + + def read_publication(self): + """Read under the project lock; only an absent legacy table has no identity.""" + with closing(sqlite3.connect(read_only_uri(self.snapshot), uri=True)) as db: + exists = db.execute( + "SELECT 1 FROM sqlite_master WHERE type='table' AND name='spool_publication'" + ).fetchone() + if not exists: + return None + row = db.execute("SELECT publication FROM spool_publication WHERE id=1").fetchone() + if not row or not isinstance(row[0], str) or not row[0]: + raise sqlite3.DatabaseError("Spool snapshot publication identity is missing or invalid") + return row[0] + + def write_publication(self, path): + """Stamp the copy that is about to replace project.spool, before it is visible. + + The marker is an additive table, not a schema-version change: older Spool readers + ignore it, and the project document and manifest keep their existing shape. + """ + token = uid() + db = sqlite3.connect(path, timeout=30) + try: + db.execute("PRAGMA journal_mode=DELETE") + db.execute( + "CREATE TABLE IF NOT EXISTS spool_publication(" + "id INTEGER PRIMARY KEY CHECK(id=1), publication TEXT NOT NULL)" + ) + db.execute("INSERT OR REPLACE INTO spool_publication VALUES(1,?)", (token,)) + db.commit() + finally: + db.close() + return token @contextmanager def connect(self): if self.read_only: - with Store.connect(self) as db: + with self.reading() as db: yield db return notifications = [] with file_lock(self.local_lock), file_lock(self.lock_file): self.check_manifest() - signature = self.signature() if self.snapshot.exists() else None + signature = self.signature(locked=True) if self.snapshot.exists() else None try: saved = json.loads(self.stamp.read_text()) if self.stamp.exists() else None except json.JSONDecodeError: saved = None if signature is None or not self.runtime.exists() or saved != signature: - for suffix in ("", "-wal", "-shm"): + # A crash leaves "-journal" beside the old copy; SQLite would roll it into the new one. + for suffix in ("", "-journal", "-wal", "-shm"): self.runtime.with_name(self.runtime.name + suffix).unlink(missing_ok=True) if signature is not None: shutil.copyfile(self.snapshot, self.runtime) @@ -153,13 +375,14 @@ def connect(self): backup.execute("PRAGMA journal_mode=DELETE") finally: backup.close() + self.write_publication(local) with local.open("rb") as source, pending.open("wb") as target: shutil.copyfileobj(source, target) target.flush() os.fsync(target.fileno()) atomic_replace(pending, self.snapshot) self.write_manifest(db) - self.stamp.write_text(json.dumps(self.signature())) + self.stamp.write_text(json.dumps(self.signature(locked=True))) if notify: notifications = [ {"projectId": identity, "revision": token[0], "cursor": token[1]} @@ -201,8 +424,13 @@ def reading(self): Writers use a different local file and can publish while a read is open. """ if self.read_only: - with Store.connect(self) as db: - yield db + self.check_manifest() + with private_snapshot(self.snapshot) as path: + with closing(sqlite3.connect(read_only_uri(path), uri=True)) as db: + if db.execute("PRAGMA user_version").fetchone()[0] > SCHEMA_VERSION: + raise ValueError("This Spool project needs a newer extension version") + db.row_factory = sqlite3.Row + yield db return with file_lock(self.read_lock): self.check_manifest() @@ -214,7 +442,7 @@ def reading(self): if saved != signature or not self.read_runtime.exists(): with file_lock(self.lock_file): self.check_manifest() - signature = self.signature() + signature = self.signature(locked=True) pending = self.read_runtime.with_suffix(".partial") try: shutil.copyfile(self.snapshot, pending) @@ -251,15 +479,22 @@ def _write_document(self, db, project_id, document): t for t in clip.get("takes", []) if t["id"] not in deleted and t["media"]["id"] not in media_ids ] document["media"] = [m for m in document.get("media", []) if m["id"] not in media_ids] - folders = {c.get("outputFolder") for c in document.get("clips", [])} + + # Allocate portable names even on a case-sensitive host. Assigned paths + # stay unchanged, including shared folders in existing projects. + def folder_key(value): + return str((self.root / value).resolve()).casefold() + + folders = {folder_key(c["outputFolder"]) for c in document.get("clips", []) if c.get("outputFolder")} existing_folders = {c.get("outputFolder") for c in previous.get("clips", [])} for clip in document.get("clips", []): if not clip.get("outputFolder"): - name = "output/" + folder_name(clip.get("name", "Clip")) - if name in folders: - name += "-" + uid()[:8] + base = "output/" + folder_name(clip.get("name", "Clip")) + name = base + while folder_key(name) in folders: + name = base + "-" + uid()[:8] clip["outputFolder"] = name - folders.add(name) + folders.add(folder_key(name)) if clip["outputFolder"] not in existing_folders: path = (self.root / clip["outputFolder"]).resolve() if path == self.root / "output" or not path.is_relative_to(self.root / "output"): @@ -303,6 +538,7 @@ def __init__(self, root): super().__init__(root, filename="app.sqlite3") self.opened = {} self.opening = threading.RLock() # handlers open project stores from worker threads + self.unavailable = {} # folder -> (files, error, retry after, delay) for folders that failed to open def folders(self): return self.setting("project_folders") or [] @@ -316,17 +552,31 @@ def open_folder(self, folder): if len(projects) != 1: raise ValueError("A project folder must contain exactly one project") project_id = projects[0]["id"] - folders = self.folders() - old = next((row for row in folders if row["id"] == project_id), None) - if old and Path(old["folder"]) != folder and (Path(old["folder"]) / "project.spool").exists(): - raise Conflict("This project is already open from another folder") - self.set_setting( - "project_folders", - [row for row in folders if row["id"] != project_id] + [{"id": project_id, "folder": str(folder)}], - ) + + def check(folders): + old = next((row for row in folders if row["id"] == project_id), None) + if old and Path(old["folder"]) != folder and (Path(old["folder"]) / "project.spool").exists(): + raise Conflict("This project is already open from another folder") + + self.register_folder(project_id, folder, check) self.opened[project_id] = store return store.get(project_id) + def register_folder(self, project_id, folder, check=None): + """Add one project folder to the list in a single transaction. + + Creating, opening and importing run in worker threads (and maybe another + process); reading the list and writing it back separately lost entries. + """ + with self.connect() as db: + db.execute("BEGIN IMMEDIATE") + row = db.execute("SELECT document FROM settings WHERE key='project_folders'").fetchone() + folders = json.loads(row[0]) if row else [] + if check: + check(folders) + folders = [row for row in folders if row["id"] != project_id] + [{"id": project_id, "folder": str(folder)}] + db.execute("INSERT OR REPLACE INTO settings VALUES(?,?)", ("project_folders", encode(folders))) + def for_project(self, project_id): row = next((r for r in self.folders() if r["id"] == project_id), None) if row is None: @@ -336,13 +586,49 @@ def for_project(self, project_id): raise ValueError("Project folder is unavailable: " + str(folder)) with self.opening: if project_id not in self.opened or self.opened[project_id].root != folder: - self.opened[project_id] = ProjectStore(folder, self) + self.opened[project_id] = self.open_store(folder) return self.opened[project_id] + def open_store(self, folder): + """Open a project folder, waiting longer between attempts while it keeps failing. + + The receipt scan and dispatcher open every folder every few seconds, and each + attempt at a failing migration first writes a backup of the snapshot. A changed + snapshot or manifest is tried again at once. + """ + + def state(path): + if not path.exists(): + return None + stat = path.stat() + return (stat.st_mtime_ns, stat.st_size, stat.st_dev, stat.st_ino, stat.st_ctime_ns) + + files = tuple(state(path) for path in (folder / "project.spool", folder / "project.json")) + held = self.unavailable.get(str(folder)) + if held and held[0] == files and time.monotonic() < held[2]: + raise held[1].with_traceback(None) + try: + store = ProjectStore(folder, self) + except (ValueError, OSError, sqlite3.Error) as error: + delay = min(3600, held[3] * 2) if held and held[0] == files else 5 + self.unavailable[str(folder)] = (files, error, time.monotonic() + delay, delay) + logging.warning("Spool could not open the project folder %s: %s", folder, error) + raise + self.unavailable.pop(str(folder), None) + return store + def project_stores(self): - return [ - self.for_project(row["id"]) for row in self.folders() if (Path(row["folder"]) / "project.spool").is_file() - ] + """Every project folder that opens; one that cannot is skipped, not fatal to the others.""" + stores = [] + for row in self.folders(): + if (Path(row["folder"]) / "project.spool").is_file(): + try: + store = self.for_project(row["id"]) + store.check_manifest() # an already-open folder may have been upgraded since + stores.append(store) + except (ValueError, OSError, sqlite3.Error): + continue + return stores def create(self, document=None, folder=None): if folder is None: @@ -367,7 +653,17 @@ def list(self): {"id": held["id"], "name": Path(held["folder"]).name, "folder": held["folder"], "unavailable": True} ) continue - rows += [{**row, "folder": held["folder"]} for row in self.for_project(held["id"]).list()] + try: + listed = self.for_project(held["id"]).list() + except (ValueError, OSError, sqlite3.Error): + # A newer format or damaged folder is listed as unavailable, not a failed list. + listed = None + if listed is None: + rows.append( + {"id": held["id"], "name": Path(held["folder"]).name, "folder": held["folder"], "unavailable": True} + ) + continue + rows += [{**row, "folder": held["folder"]} for row in listed] return rows def get(self, project_id): @@ -386,14 +682,23 @@ def import_project(self, value): parent = Path(self.setting("projects_directory") or self.root / "projects").resolve() parent.mkdir(parents=True, exist_ok=True) folder = Path(tempfile.mkdtemp(prefix="Untitled Spool project-", dir=parent)) + destination, cache = folder, None + cache_existed = True try: + destination = folder.resolve() + cache = working_cache(self, folder) + try: + cache.lstat() + except FileNotFoundError: + cache_existed = False + except OSError: + pass # Only confirmed absence establishes this import's ownership. store = ProjectStore(folder, self) project = store.import_project(value) - self.set_setting("project_folders", [*self.folders(), {"id": project["id"], "folder": str(folder)}]) + self.register_folder(project["id"], folder) except Exception: - # Only this import's exclusively created destination belongs to us. - if folder.resolve().parent == parent: - shutil.rmtree(folder) + # The constructor creates the local working cache before it can fail. + _discard_import(self, folder, destination, parent, cache, cache_existed) raise self.opened[project["id"]] = store return project diff --git a/spool/requests.py b/spool/requests.py index 5e77640..3e9e2db 100644 --- a/spool/requests.py +++ b/spool/requests.py @@ -194,6 +194,9 @@ def audio_input_segments(document, clip, setting): ] +IMAGE_SAVERS = {"SaveImage", "PreviewImage"} + + def local_compatibility(prompt, result): if any(node["class_type"] in LAUNCHERS for node in prompt.values()): raise ValueError( @@ -261,6 +264,9 @@ def prepare(store, media, project, items, freeze=True): raise ValueError("Choose local ComfyUI, Comfy Cloud or Deadline execution") if target == attempts.CLOUD and not cloud.available(): raise ValueError(cloud.UNAVAILABLE) + if target == attempts.CLOUD and capture["prompt"][capture["result"]["nodeId"]]["class_type"] in IMAGE_SAVERS: + # Cloud results are collected as video files; an image result would render and never arrive. + raise ValueError("Comfy Cloud renders video only. Choose a SaveVideo result for this workflow.") local_compatibility({k: v for k, v in capture["prompt"].items() if k not in launchers}, capture["result"]) # A clip set to Deadline whose workflow has no submit node gets one in each # frozen request only; the saved workflow and its takes' workflows stay as made. @@ -422,7 +428,10 @@ def prepare(store, media, project, items, freeze=True): original = media.prepare_input(original) source = contained(media.host_roots["input"], original.removesuffix(" [input]")) if source not in frozen_assets: - replacement = media.freeze_asset(original) if freeze else original.removesuffix(" [input]") + # Resolve and validate the whole batch before making immutable + # snapshots. A later invalid clip or missing input must not + # leave copies from the requests that preceded it. + replacement = original.removesuffix(" [input]") path = contained(media.host_roots["input"], replacement) frozen_assets[source] = { "frozen": replacement, @@ -465,6 +474,16 @@ def prepare(store, media, project, items, freeze=True): }, ) ) + if freeze: + for asset in frozen_assets.values(): + replacement = media.freeze_asset(asset["frozen"]) + path = contained(media.host_roots["input"], replacement) + asset.update({"frozen": replacement, "byteSize": path.stat().st_size, "sha256": sha256(path)}) + for _, request in requests: + for asset in request["assets"]: + source = contained(media.host_roots["input"], asset["source"].removesuffix(" [input]")) + asset.update(frozen_assets[source]) + request["prompt"][asset["nodeId"]]["inputs"][asset["input"]] = asset["frozen"] return requests diff --git a/spool/routes.py b/spool/routes.py index 9b0bac9..2c0c1f8 100644 --- a/spool/routes.py +++ b/spool/routes.py @@ -12,6 +12,7 @@ from .errors import NotFound, SpoolError, Forbidden, RevisionConflict, StudioOutdated from .protocol import API_VERSION +from .store import validate_capture from .editor import editor_html from .media import EXTENSIONS, contained, staged_relative from .playback import PlaybackQueue @@ -98,10 +99,96 @@ def inside(path, root): """Lexical containment, matching `contained`; mapped drives are not resolved to UNC.""" - base = os.path.normcase(os.path.abspath(root)) + base = os.path.normcase(os.path.abspath(root)).rstrip(os.sep) # a drive root keeps its separator return os.path.normcase(os.path.abspath(path)).startswith(base + os.sep) +def required_fields(data, *fields): + """Read required top-level JSON fields as a client error, before any work. + + A missing field is a malformed request, not an internal failure. The route + still validates each value, so accepted types do not change here. + """ + if not isinstance(data, dict): + raise ValueError("Request body must be a JSON object") + for field in fields: + if field not in data: + raise ValueError("Request body is missing the required %s field" % field) + return [data[field] for field in fields] + + +def revision_header(request): + """The saved revision a workflow import is written against.""" + value = request.headers.get("X-Spool-Revision") + if value is None: + raise ValueError("The X-Spool-Revision header is required") + try: + return int(value) + except ValueError: + raise ValueError("The X-Spool-Revision header must be an integer") from None + + +def object_rows(value, field, *keys): + """Check containers consumed by save/preparation, without defining a project schema.""" + if not isinstance(value, list) or any(not isinstance(row, dict) for row in value): + raise ValueError(f"{field} must be a list of JSON objects") + if any(not isinstance(row.get(key), str) for row in value for key in keys): + raise ValueError(f"{field} entries need string fields: {', '.join(keys)}") + return value + + +def capture_shape(capture): + """Reject malformed capture containers before file preparation; Store owns their meaning.""" + if not isinstance(capture, dict): + raise ValueError("The captured workflow must be a JSON object") + object_rows(capture.get("bindings", []), "Capture bindings", "key", "nodeId", "input") + for field in ("provenance", "result", "inputRecovery"): + if field in capture and not isinstance(capture[field], dict): + raise ValueError(f"Capture {field} must be a JSON object") + if not isinstance(capture.get("inputRecovery", {}).get("recovered", []), list): + raise ValueError("Capture inputRecovery.recovered must be a list") + if "result" in capture and not isinstance(capture["result"].get("nodeId"), str): + raise ValueError("Capture result needs a string nodeId") + builtin = capture.get("provenance", {}).get("builtinTemplate") + if builtin is not None and not isinstance(builtin, str): + raise ValueError("Capture builtinTemplate must be a string") + graphs = [capture["workflow"]] if capture.get("workflow") is not None else [] + while graphs: + graph = graphs.pop() + if not isinstance(graph, dict): + raise ValueError("Capture workflow must be a JSON object") + nodes = object_rows(graph.get("nodes", []), "Workflow nodes", "type") + if any("id" not in node for node in nodes): + raise ValueError("Workflow nodes need an id") + definitions = graph.get("definitions", {}) + if not isinstance(definitions, dict): + raise ValueError("Workflow definitions must be a JSON object") + graphs.extend(object_rows(definitions.get("subgraphs", []), "Workflow subgraphs", "id")) + validate_capture(capture) + + +def save_shapes(document, recipes): + """Preflight every save container before any recipe can stage input files.""" + if not isinstance(document, dict) or not isinstance(document.get("clips"), list): + raise ValueError("Project document must be an object with a clips list") + clips = object_rows(document["clips"], "Document clips", "id") + object_rows(document.get("media", []), "Document media", "id") + for field in ("audioClips", "importedWorkflows"): + object_rows(document.get(field, []), "Document " + field) + for clip in clips: + takes = object_rows(clip.get("takes", []), "Clip takes", "id") + for take in takes: + if not isinstance(take.get("media"), dict) or not isinstance(take["media"].get("id"), str): + raise ValueError("Take media must be a JSON object with an id") + object_rows(clip.get("bindings", []), "Clip bindings", "key") + if not isinstance(clip.get("inputValues", {}), dict): + raise ValueError("Clip inputValues must be a JSON object") + if not isinstance(recipes, dict): + raise ValueError("workflowRecipes must be a JSON object") + for capture in recipes.values(): + capture_shape(capture) + + def register(server, store, media, executor=None): """Executor is None when Spool runs without ComfyUI; editing, import and export still work.""" routes = server.routes @@ -367,10 +454,9 @@ async def upload_input(request): async def generation_preview(request): store, media = await asyncio.to_thread(services, request) data = unwire(await request.json()) + revision, clip_id = required_fields(data, "revision", "clipId") return response( - await asyncio.to_thread( - requests.preview, store, media, request.match_info["project"], data["revision"], data["clipId"] - ) + await asyncio.to_thread(requests.preview, store, media, request.match_info["project"], revision, clip_id) ) @routes.get("/spool/api/projects/{project}/status") @@ -421,6 +507,10 @@ async def comparison(request): store, media = await asyncio.to_thread(services, request) data = await request.json() clip = await asyncio.to_thread(store.clip, request.match_info["project"], data["clipId"]) + chosen = data["takeIds"] if isinstance(data["takeIds"], list) else [] + # The sheet is served to the caller, so it reads only footage the caller may download. + records = [t["media"] for t in clip["takes"] if t["id"] in chosen and not t.get("placeholder")] + await asyncio.to_thread(guard_originals, request, store, media, records) fps = await asyncio.to_thread(store.frame_rate, request.match_info["project"]) return response(await asyncio.to_thread(media.comparison, clip, data["takeIds"], fps)) @@ -504,42 +594,45 @@ async def project_import(request): @routes.put("/spool/api/projects/{project}") @endpoint async def save(request): - store, media = await asyncio.to_thread(services, request) text = await request.text() data = await asyncio.to_thread(lambda: unwire(json.loads(text))) + revision, document = required_fields(data, "revision", "document") + workflow_recipes = data.get("workflowRecipes", {}) + save_shapes(document, workflow_recipes) + store, media = await asyncio.to_thread(services, request) recipes = {} - for version, capture in data.get("workflowRecipes", {}).items(): + for version, capture in workflow_recipes.items(): recipes[version] = await prepare_capture(capture, request) - for clip in data["document"]["clips"]: + for clip in document["clips"]: if clip.get("workflowVersion") == version and clip.get("inputValues"): operations.restage(capture, recipes[version], clip["inputValues"]) return await project_response( request, - await asyncio.to_thread( - store.save, request.match_info["project"], data["revision"], data["document"], recipes - ), + await asyncio.to_thread(store.save, request.match_info["project"], revision, document, recipes), ) @routes.post("/spool/api/projects/{project}/import-workflow") @endpoint async def import_workflow(request): store, media = await asyncio.to_thread(services, request) + revision = revision_header(request) imported = parse_import(await request.text(), unquote(request.headers.get("X-Filename", "Imported workflow"))) imported["data"] = await prepare_inputs(imported["data"], request) project_id = request.match_info["project"] project = await asyncio.to_thread(store.get, project_id) project["document"].setdefault("importedWorkflows", []).append(imported) - saved = await asyncio.to_thread( - store.save, project_id, int(request.headers["X-Spool-Revision"]), project["document"] - ) + saved = await asyncio.to_thread(store.save, project_id, revision, project["document"]) return response({"project": saved, "workflow": imported}) @routes.post("/spool/api/projects/{project}/capture/{clip}") @endpoint async def capture(request): - store, media = await asyncio.to_thread(services, request) data = unwire(await request.json()) - original = data["capture"] + revision, original = required_fields(data, "revision", "capture") + capture_shape(original) + if data.get("overrides") is not None and not isinstance(data["overrides"], dict): + raise ValueError("Capture overrides must be a JSON object") + store, media = await asyncio.to_thread(services, request) data["capture"] = await prepare_capture(original, request) if data.get("overrides"): operations.restage(original, data["capture"], data["overrides"]) @@ -547,7 +640,7 @@ async def capture(request): operations.apply_capture, store, request.match_info["project"], - data["revision"], + revision, request.match_info["clip"], data["capture"], data.get("overrides"), @@ -569,6 +662,19 @@ async def take_setup(request): ) return response(await prepare_inputs(capture, request, True) if capture else None) + def outside_footage(request, selected, project_media, path): + """Footage outside the project and Comfy folders is read only for this computer.""" + roots = [selected.root, *project_media.host_roots.values()] + if not any(inside(path, root) for root in roots): + local_control(request, "Use footage outside this project from the computer running Spool") + + def guard_originals(request, selected, project_media, records): + """Refuse a remote caller a result made from footage it could not download.""" + for metadata in records: + original = metadata.get("originalFile", metadata.get("file", "")) + if isinstance(original, str) and Path(original).is_absolute(): + outside_footage(request, selected, project_media, original) + def take_source(request): selected, project_media = services(request) clip = selected.clip(request.match_info["project"], request.match_info["clip"]) @@ -578,9 +684,7 @@ def take_source(request): if not take: raise NotFound("Take not found") path = project_media.original_source(take["media"])["path"] - roots = [selected.root, *project_media.host_roots.values()] - if not any(inside(path, root) for root in roots): - local_control(request, "Use footage outside this project from the computer running Spool") + outside_footage(request, selected, project_media, path) return path @routes.get("/spool/api/projects/{project}/take-file/{clip}/{take}") @@ -625,14 +729,15 @@ def folder(): async def use_take(request): store, media = await asyncio.to_thread(services, request) data = unwire(await request.json()) + revision, take_id = required_fields(data, "revision", "takeId") capture = await prepare_inputs(data["capture"], request, True) if data.get("capture") else None saved = await asyncio.to_thread( operations.use_take, store, request.match_info["project"], - data["revision"], + revision, request.match_info["clip"], - data["takeId"], + take_id, capture, ) return await project_response(request, saved) @@ -648,14 +753,15 @@ async def prepare_workflow(request): async def delete_take_files(request): selected, _ = await asyncio.to_thread(services, request) data = await request.json() + revision, take_ids = required_fields(data, "revision", "takeIds") return response( await asyncio.to_thread( take_files.delete, selected, request.match_info["project"], - data["revision"], + revision, request.match_info["clip"], - data["takeIds"], + take_ids, ) ) @@ -697,12 +803,13 @@ async def finish_handoff(request): async def queue(request): store, media = await asyncio.to_thread(services, request) data = unwire(await request.json()) + (revision,) = required_fields(data, "revision") project_id = request.match_info["project"] items = requests.queue_items(data) if executor is not None: value = await executor.queue( project_id, - data["revision"], + revision, items, data.get("operationId"), data.get("actor", "human"), @@ -713,7 +820,7 @@ async def queue(request): store, media, project_id, - data["revision"], + revision, items, data.get("operationId"), data.get("actor", "human"), @@ -847,7 +954,7 @@ async def reconcile(request): store, media = await asyncio.to_thread(services, request) if executor is not None: await executor.reconcile() - await asyncio.to_thread(collector.scan) + await cloud_module.storage_call(collector.scan) return web.json_response({"ok": True}) @routes.get("/spool/api/attempts/{attempt}") @@ -876,7 +983,12 @@ async def upload(request): raise ValueError("Import a video, image, or audio file") target = await asyncio.to_thread(media.input_destination, name, unique=True) await receive_file(request, target) - description = await asyncio.to_thread(media.describe, target, name, float(request.query.get("fps", 24))) + try: + description = await asyncio.to_thread(media.describe, target, name, float(request.query.get("fps", 24))) + except Exception: + # Nothing refers to a file that could not be described. + target.unlink(missing_ok=True) + raise if description.get("workflow"): description["workflowData"] = await prepare_inputs( {"workflow": description["workflow"], "prompt": description.get("prompt")}, request @@ -944,6 +1056,7 @@ async def detect_clip_cuts(request): take = next((t for t in clip["takes"] if t["id"] == data["takeId"]), None) if not take or take.get("placeholder"): raise ValueError("Select a video take in this project") + await asyncio.to_thread(guard_originals, request, store, media, [take["media"]]) fps = await asyncio.to_thread(store.frame_rate, request.match_info["project"]) document = await asyncio.to_thread(store.document, request.match_info["project"]) size = document.get("resolution") or {"width": 960, "height": 540} diff --git a/spool/store.py b/spool/store.py index ca53bdc..6394579 100644 --- a/spool/store.py +++ b/spool/store.py @@ -175,6 +175,21 @@ def forget_media_evidence(self, media_ids): ) +def json_strings(value): + """Every string in a decoded JSON value, keys included.""" + found, pending = [], [value] + while pending: + item = pending.pop() + if isinstance(item, str): + found.append(item) + elif isinstance(item, dict): + found.extend(k for k in item if isinstance(k, str)) + pending.extend(item.values()) + elif isinstance(item, list): + pending.extend(item) + return found + + class Store: def __init__(self, root, migrate=True, filename="spool.sqlite3"): self.focus_store = self @@ -416,6 +431,20 @@ def _pack(self, db, value): } def _dump(self, db, value, *, receipt=False, capture=False): + # Storage references are created below, never accepted as authored JSON. + # Reject before packing so a failed save rolls back instead of publishing + # a document that every subsequent read interprets as a missing blob. + def check_authored(held): + if isinstance(held, dict): + if set(held) == {"__spool_blob__"}: + raise ValueError("Authored values cannot contain a project evidence blob reference") + for child in held.values(): + check_authored(child) + elif isinstance(held, list): + for child in held: + check_authored(child) + + check_authored(value) if receipt: value = copy.deepcopy(value) if "primary" in value and "artifacts" in value: @@ -654,7 +683,14 @@ def referenced_names(self): except sqlite3.OperationalError: continue for row in rows: - for token in re.findall(r"[A-Za-z0-9_\-./\\]+", row[0]): + tokens = re.findall(r"[A-Za-z0-9_\-./\\]+", row[0]) + try: + # Whole string values too: file names may hold spaces, and JSON + # escapes non-ASCII characters, which the tokens alone would miss. + tokens += json_strings(json.loads(row[0])) + except (TypeError, ValueError): + pass + for token in tokens: names.add(token) if "/" in token or "\\" in token: names.add(token.replace("\\", "/").rsplit("/", 1)[-1]) @@ -665,7 +701,7 @@ def derived_storage(self): names = self.referenced_names() folders = {} for folder in ("previews", "exports", "uploads", "shots", "artifacts", "assets", "backups"): - directory = (self.cache if folder in ("previews", "shots") else self.root) / folder + directory = (self.cache if folder in ("previews", "shots", "backups") else self.root) / folder if not directory.is_dir(): continue files = [path for path in directory.rglob("*") if path.is_file()] @@ -1147,6 +1183,13 @@ def folder_inventory(self, project_id): pending.add(row["take_id"][:8]) result = self._load(row["result"], db, memo=memo) or {} known.update(a.get("originalFile", a.get("file")) for a in result.get("artifacts", [])) + # A retried farm task's copy is evidence of the same take, not a file dropped into the folder. + for row in db.execute( + "SELECT e.receipt FROM executions e JOIN attempts a ON a.id=e.attempt_id WHERE a.project_id=?", + (project_id,), + ): + receipt = self._load(row["receipt"], db, memo=memo) or {} + known.update(a.get("originalFile", a.get("file")) for a in receipt.get("artifacts", [])) return known, numbers, pending def get(self, project_id): @@ -1229,7 +1272,8 @@ def capture( row = db.execute("SELECT document FROM projects WHERE id=?", (project_id,)).fetchone() if not row: raise NotFound("Project not found") - document = json.loads(row[0]) + # Unpacked as editing() reads it: this document is saved, and saving packs it again. + document = self._load(row[0], db) generated = ( db.execute( "SELECT clip_id,take_id,result FROM attempts WHERE project_id=? AND take_id=? AND status='complete'", diff --git a/tests/comfy_link_browser_checks.js b/tests/comfy_link_browser_checks.js index 74d5591..fe443eb 100644 --- a/tests/comfy_link_browser_checks.js +++ b/tests/comfy_link_browser_checks.js @@ -88,6 +88,7 @@ ); const image = () => app.rootGraph.nodes.find(node => node.type === 'LoadImage').widgets.find(widget => widget.name === 'image'); + check('Refreshed loader choices include the current staged image', image().options.values.includes(chosenImage)); check( 'Opened graph uses the current clip input instead of a stale editor widget (' + image().value + @@ -272,6 +273,13 @@ comfy.comfyAPI?.app?.app?.extensionManager?.workflow?.activeWorkflow, ); await n.editWorkflow(); + const restoredImage = comfy.comfyAPI.app.app.rootGraph.nodes + .find(node => node.type === 'LoadImage') + .widgets.find(widget => widget.name === 'image'); + check( + 'Reloaded linked loader keeps its staged value in refreshed choices', + restoredImage.options.values.includes(restoredImage.value), + ); check( 'Reloaded Comfy keeps the existing editor bridge', popups.length === count && diff --git a/tests/demo_api_checks.mjs b/tests/demo_api_checks.mjs index 777fa38..2819e84 100644 --- a/tests/demo_api_checks.mjs +++ b/tests/demo_api_checks.mjs @@ -87,6 +87,103 @@ const send = (method, path, value, headers) => api(path, { method, body: JSON.st check('Recorded errors replay', (await api('projects/' + ID + '/takes/c'))[0] === 400); check('Static files still load', (await window.fetch('/site/spool/ui/editor.js').then(r => r.text())) === 'static'); } +{ + const path = 'projects/' + ID; + const [, before] = await api(path); + const cases = [ + ['PUT', path, { document: before.document }, 'revision'], + ['PUT', path, { revision: before.revision }, 'document'], + ['PUT', path, { revision: before.revision, document: null }, 'document'], + ['PUT', path, { revision: before.revision, document: { clips: null } }, 'document'], + ['POST', path + '/use-take/clip-1', { takeId: 'b' }, 'revision'], + ['POST', path + '/use-take/clip-1', { revision: before.revision }, 'takeId'], + ['POST', path + '/delete-takes/clip-1', { takeIds: ['c'] }, 'revision'], + ['POST', path + '/delete-takes/clip-1', { revision: before.revision }, 'takeIds'], + ]; + for (const [method, target, value, field] of cases) { + const [status, error] = await send(method, target, value); + check( + 'Missing/invalid ' + field + ' on ' + method + ' ' + target + ' is a client error', + status === 400 && error.code === 'invalid_request' && error.error.includes(field), + ); + } + for (const [method, target] of [ + ['PUT', path], + ['POST', path + '/use-take/clip-1'], + ['POST', path + '/delete-takes/clip-1'], + ]) { + for (const raw of ['null', '[]', '"wrong shape"', '{']) { + const [status, error] = await api(target, { method, body: raw }); + check('Malformed ' + method + ' ' + target + ': ' + raw, status === 400 && error.code === 'invalid_request'); + } + } + for (const [route, value] of [ + ['/delete-takes/clip-1', { takeIds: null }], + ['/delete-takes/clip-1', { takeIds: [] }], + ['/delete-takes/clip-1', { takeIds: {} }], + ['/delete-takes/clip-1', { takeIds: [1] }], + ['/delete-takes/clip-1', { takeIds: ['missing'] }], + ['/delete-takes/clip-1', { takeIds: ['a'] }], + ['/use-take/clip-1', { takeId: null }], + ['/use-take/clip-1', { takeId: 'missing' }], + ['/use-take/clip-1', { takeId: 'b', capture: { provenance: { takeBase: { takeId: 'a' } } } }], + ]) { + const [status, error] = await send('POST', path + route, { revision: before.revision, ...value }); + check( + 'Invalid demo take values carry the host error code: ' + JSON.stringify(value), + status === 400 && error.code === 'invalid_request' && typeof error.error === 'string', + ); + } + // Representative identical payload shapes are rejected by test_nested_request_shapes.py. + for (const [keys, replacement, field] of [ + [['document', 'clips'], [1], 'clips'], + [['document', 'clips'], [{}], 'clips'], + [['document', 'media'], null, 'media'], + [['document', 'media'], [1], 'media'], + [['document', 'audioClips'], [1], 'audioClips'], + [['document', 'importedWorkflows'], {}, 'importedWorkflows'], + [['document', 'clips', 0, 'takes'], [1], 'takes'], + [['document', 'clips', 0, 'takes'], [{ id: 'take', media: null }], 'media'], + [['document', 'clips', 0, 'bindings'], [{}], 'bindings'], + [['document', 'clips', 0, 'inputValues'], [], 'inputValues'], + [['workflowRecipes'], null, 'workflowRecipes'], + [['workflowRecipes'], [], 'workflowRecipes'], + [['workflowRecipes'], { recipe: null }, 'workflow'], + [['workflowRecipes'], { recipe: { bindings: [1] } }, 'bindings'], + [['workflowRecipes'], { recipe: { provenance: [] } }, 'provenance'], + [['workflowRecipes'], { recipe: { inputRecovery: null } }, 'inputRecovery'], + [['workflowRecipes'], { recipe: { inputRecovery: { recovered: {} } } }, 'inputRecovery'], + [['workflowRecipes'], { recipe: { inputRecovery: { recovered: null } } }, 'inputRecovery'], + [['workflowRecipes'], { recipe: { result: null } }, 'result'], + [['workflowRecipes'], { recipe: { result: { nodeId: [] } } }, 'nodeId'], + [['workflowRecipes'], { recipe: { provenance: { builtinTemplate: [] } } }, 'builtinTemplate'], + [['workflowRecipes'], { recipe: { workflow: [] } }, 'workflow'], + [['workflowRecipes'], { recipe: { workflow: { nodes: [1] } } }, 'nodes'], + [['workflowRecipes'], { recipe: { workflow: { nodes: [{ type: 'LoadImage' }] } } }, 'id'], + [['workflowRecipes'], { recipe: { workflow: { definitions: null } } }, 'definitions'], + [['workflowRecipes'], { recipe: { workflow: { definitions: { subgraphs: [1] } } } }, 'subgraphs'], + ]) { + const value = { revision: before.revision, document: structuredClone(before.document) }; + let target = value; + for (const key of keys.slice(0, -1)) target = target[key]; + target[keys.at(-1)] = replacement; + const [status, error] = await send('PUT', path, value); + check( + 'Nested demo save ' + field + ' matches host validation: ' + JSON.stringify(value.workflowRecipes ?? field), + status === 400 && error.code === 'invalid_request' && error.error.includes(field), + ); + } + const [unavailable, unavailableError] = await send('POST', path + '/queue', { revision: before.revision, items: [] }); + check( + 'Unsupported demo operations keep their distinct response', + unavailable === 403 && unavailableError.code === 'demo_unavailable', + ); + const [, after] = await api(path); + check( + 'Malformed demo requests preserve revision, document and sync token', + JSON.stringify(after) === JSON.stringify(before), + ); +} { const [, current] = await api('projects/' + ID); const document = structuredClone(current.document); @@ -108,7 +205,10 @@ const send = (method, path, value, headers) => api(path, { method, body: JSON.st { const path = 'projects/' + ID + '/delete-takes/clip-1'; const [refused, error] = await send('POST', path, { revision: 6, takeIds: ['a'] }); - check('The take in the edit is never deleted', refused === 400 && /kept/.test(error.error)); + check( + 'The take in the edit is never deleted', + refused === 400 && error.code === 'invalid_request' && /kept/.test(error.error), + ); const [status, value] = await send('POST', path, { revision: 6, takeIds: ['c'] }); check('Deleting takes replies with the project', status === 200 && value.pendingFiles === 0); check('Deleted takes are gone and recorded', value.project.document.clips[0].takes.length === 2); @@ -154,7 +254,109 @@ const send = (method, path, value, headers) => api(path, { method, body: JSON.st check('A split half reads the recorded setup of its take', status === 200 && Array.isArray(setup.bindings)); const path = 'projects/' + ID + '/delete-takes/clip-2'; const [refused, error] = await send('POST', path, { revision: saved.revision, takeIds: ['d'] }); - check('A file the other half uses is not deleted', refused === 400 && /c2/.test(error.error)); + check( + 'A file the other half uses is not deleted', + refused === 400 && error.code === 'invalid_request' && /c2/.test(error.error), + ); +} + +{ + // The demo mirrors host take_files.plan using recorded paths; no files are removed. + const deletion = 'projects/' + ID + '/delete-takes/clip-1'; + const edit = async mutate => { + const [, current] = await api('projects/' + ID); + const document = structuredClone(current.document); + mutate(document); + const [status, saved] = await send('PUT', 'projects/' + ID, { revision: current.revision, document }); + check('Deletion fixture saves at the current revision', status === 200); + return saved; + }; + const refused = async (label, current, ids, message) => { + const [status, error] = await send('POST', deletion, { revision: current.revision, takeIds: ids }); + check(label, status === 400 && error.code === 'invalid_request' && message.test(error.error)); + const [, after] = await api('projects/' + ID); + check( + label + ' leaves the document, revision and token unchanged', + JSON.stringify(after) === JSON.stringify(current), + ); + }; + let current = await edit(document => { + document.media = [ + { ...take('a').media, id: 'pool-a' }, + { ...take('unrelated').media, id: 'pool-keep' }, + ]; + document.clips[1].bindings = [{ key: 'source', asset: true }]; + document.clips[1].inputValues = { source: 'spool-projects/' + ID + '/output/a.mp4 [input]' }; + }); + await refused('A selected file used as a staged generation input is protected', current, ['a'], /generation input/); + current = await edit(document => { + document.clips[1].inputValues.source = 'spool-projects/' + ID + '/output/./a.mp4'; + }); + await refused('Generation input path aliases are protected', current, ['a'], /generation input/); + current = await edit(document => { + document.clips[1].inputValues.source = 'not-a-file'; + document.audioClips = [{ id: 'audio', media: take('a').media }]; + }); + await refused('Timeline audio protects the selected source', current, ['a'], /timeline audio/); + current = await edit(document => { + document.audioClips = []; + document.media.push({ id: 'pool-shared', file: 'output/another.mp4', poster: 'output/a.json' }); + }); + await refused('A pool asset sharing the deleted sidecar is protected', current, ['a'], /media-pool asset/); + current = await edit(document => { + document.media = document.media.filter(media => media.id !== 'pool-shared'); + document.clips[1].takes.push({ ...take('alias'), media: { ...take('alias').media, file: 'output/a.mov' } }); + }); + await refused('Another take sharing the selected sidecar is protected', current, ['a'], /another take/); + current = await edit(document => { + document.clips[1].takes = document.clips[1].takes.filter(take => take.id !== 'alias'); + document.clips[1].bindings = [{ key: 'text', asset: false }]; + document.clips[1].inputValues = { text: 'spool-projects/' + ID + '/output/a.mp4' }; + }); + const [status, value] = await send('POST', deletion, { revision: current.revision, takeIds: ['a', 'a'] }); + check('Non-asset text matching a file does not block deletion', status === 200); + check('Deleted take IDs are deduplicated as on the host', JSON.stringify(value.deleted) === '["a"]'); + check( + 'Media-pool aliases of a deleted original are recorded with the take media', + value.project.document.deletedMediaIds.includes('media-a') && + value.project.document.deletedMediaIds.includes('pool-a'), + ); + check( + 'Only media-pool records using deleted originals are removed', + value.project.document.media.length === 1 && value.project.document.media[0].id === 'pool-keep', + ); + const [, unchanged] = await api('projects/' + ID, { headers: { 'X-Spool-Since': value.project.syncToken } }); + check( + 'Deletion publishes a new poll token with the host reply shape', + !unchanged.document && unchanged.jobs.length === 1, + ); + + current = await edit(document => { + document.clips[0].takes.push({ ...take('external'), media: { ...take('external').media, file: '../outside.mp4' } }); + }); + await refused( + 'A take outside project-owned media folders cannot be deleted', + current, + ['external'], + /owned by this project/, + ); + await refused('Invalid take-ID types use the host validation message', current, [3], /Select the takes/); +} + +{ + const path = 'projects/' + ID; + const [, current] = await api(path); + const document = { project: 'Minimal valid save', clips: [{ id: 'empty', in: 0, out: 24 }] }; + const [status, saved] = await send('PUT', path, { revision: current.revision, document, workflowRecipes: {} }); + check( + 'Valid minimal saves keep the host defaults for omitted takes', + status === 200 && saved.revision === current.revision + 1 && saved.document.clips[0].takes.length === 0, + ); + const [stale, error] = await send('PUT', path, { revision: current.revision, document }); + check( + 'Nested shape validation preserves stale revision conflicts', + stale === 409 && error.code === 'revision_conflict', + ); } console.log(checks.length + ' demo API checks passed'); diff --git a/tests/html_escape_checks.mjs b/tests/html_escape_checks.mjs new file mode 100644 index 0000000..de501e3 --- /dev/null +++ b/tests/html_escape_checks.mjs @@ -0,0 +1,91 @@ +// Identity and number fields from a project reach HTML only through esc(). +// +// A project is data from whoever made it: an imported export or a shared project +// folder keeps clip, take and media IDs, take numbers and track numbers exactly as +// written. Joined raw into markup they ran script when the project was opened. +import assert from 'node:assert/strict'; +import { readdirSync, readFileSync } from 'node:fs'; +import { Linter } from 'eslint'; + +const FIELDS = new Set(['id', 'number', 'workflow', 'videoTrack', 'track', 'sourceLane']); +const CALLS = new Set(['videoTrack', 'sourceLane', 'takeName']); +const markup = node => + (node.type === 'Literal' && typeof node.value === 'string' && /[<>]|="/.test(node.value)) || + (node.type === 'TemplateLiteral' && node.quasis.some(q => /[<>]|="/.test(q.value.raw))); +const parts = (node, out = []) => { + if (node.type === 'BinaryExpression' && node.operator === '+') { + parts(node.left, out); + parts(node.right, out); + } else out.push(node); + return out; +}; +const name = node => + node.type === 'Identifier' ? node.name : node.type === 'MemberExpression' ? node.property.name : ''; +function raw(node) { + switch (node.type) { + case 'MemberExpression': + return !node.computed && FIELDS.has(node.property.name); + case 'CallExpression': { + const callee = name(node.callee); + if (CALLS.has(callee)) return true; + if (callee === 'String') return node.arguments.some(raw); + if (node.callee.type === 'MemberExpression' && ['padStart', 'toString'].includes(callee)) + return raw(node.callee.object); + return false; + } + case 'ConditionalExpression': + return raw(node.consequent) || raw(node.alternate); + case 'LogicalExpression': + return raw(node.left) || raw(node.right); + case 'BinaryExpression': + return node.operator === '+' && parts(node).some(raw); + case 'TemplateLiteral': + return node.expressions.some(raw); + default: + return false; + } +} +const rule = { + create(context) { + return { + BinaryExpression(node) { + if (node.operator !== '+' || (node.parent.type === 'BinaryExpression' && node.parent.operator === '+')) return; + const all = parts(node); + if (!all.some(markup)) return; + for (const part of all) if (raw(part)) context.report({ node: part, message: 'Escape with esc()' }); + }, + TemplateLiteral(node) { + if (!markup(node)) return; + for (const part of node.expressions) + if (raw(part)) context.report({ node: part, message: 'Escape with esc()' }); + }, + }; + }, +}; + +const linter = new Linter(); +const config = { + languageOptions: { ecmaVersion: 'latest', sourceType: 'module' }, + plugins: { spool: { rules: { 'escape-fields': rule } } }, + rules: { 'spool/escape-fields': 'error' }, +}; +const check = source => linter.verify(source, config).map(m => m.line + ':' + m.column + ' ' + m.message); + +// The check itself finds the shapes that were unescaped. +assert.equal(check(`h.innerHTML = '
';`).length, 1); +assert.equal(check(`h.innerHTML = '' + 'T' + String(t.number).padStart(2, '0') + '';`).length, 1); +assert.equal(check(`h.innerHTML = '';`).length, 1); +assert.equal(check(`h.innerHTML = '' : '') + - (!setupChosen ? '' : '') + + (!setupChosen ? '' : '') + '
'; return ( '
' + - (trial - ? 'T' + String(trial.number).padStart(2, '0') + ' trial' - : t.placeholder - ? 'No take' - : 'T' + String(t.number).padStart(2, '0')) + + esc( + trial + ? 'T' + String(trial.number).padStart(2, '0') + ' trial' + : t.placeholder + ? 'No take' + : 'T' + String(t.number).padStart(2, '0'), + ) + '' + @@ -367,7 +369,7 @@ export function createTimelineRenderer({ '" style="left:' + row.end * getPixelScale() + 'px;top:calc(var(--video-top-' + - cut.track + + esc(cut.track) + ') + 6px)" title="Add transition" aria-label="Add transition between ' + esc(getState().clips.find(c => c.id === cut.left)?.name || '') + ' and ' + @@ -387,9 +389,9 @@ export function createTimelineRenderer({ '" style="left:' + row.end * getPixelScale() + 'px;top:calc(var(--video-top-' + - T.videoTrack(parent) + + esc(T.videoTrack(parent)) + ') + var(--video-height-' + - T.videoTrack(parent) + + esc(T.videoTrack(parent)) + ') / 2)" title="' + esc(getState().clips.find(c => c.id === id)?.name + ' extends ' + parent.name) + '" aria-hidden="true">' + diff --git a/ui/viewer.js b/ui/viewer.js index f13ff49..75e2eb9 100644 --- a/ui/viewer.js +++ b/ui/viewer.js @@ -151,7 +151,7 @@ export function createViewer({ ? '' + esc(c.name) + ' · ' + - takeName(t) + + esc(takeName(t)) + ' · ' + (t.id === c.takeId ? 'in edit' @@ -159,7 +159,7 @@ export function createViewer({ : 'Media · ' + esc(review.source?.media.name || '') : trial ? 'Trying ' + - takeName(trial) + + esc(takeName(trial)) + ' in ' + esc(tried.name) + ' · not saved until you use it' @@ -234,7 +234,7 @@ export function createViewer({ '' + - (d.take.placeholder ? 'No take in edit' : takeName(d.take)) + + (d.take.placeholder ? 'No take in edit' : esc(takeName(d.take))) + (trial ? ' · trial' : '') + '', ); diff --git a/ui/workflow-coordinator.js b/ui/workflow-coordinator.js index 8046c1c..0ead7a6 100644 --- a/ui/workflow-coordinator.js +++ b/ui/workflow-coordinator.js @@ -196,7 +196,10 @@ export function createWorkflowCoordinator({ 'Add shot', '

Workflows

' + Object.entries(workflows) - .map(([id, w]) => '') + .map( + ([id, w]) => + '', + ) .join('') + '