nodes/ComfyAssets-ComfyUI_PromptManager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(system): thumbnail job review fixes (#249 S5)

Browse Source 
- The legacy stream's first frame is job {id, started}, sent after the
  subscription so no runner frame is missed; it reads the registry after
  prepare, so a job started in that gap is attached to, not refused.
- Every exit sets a truthful final status line; nothing to do never
  reads "Processing file 0 of 0".
- Cancel is checked per file during the planning walk; a job cancelled
  there publishes no start frame.
- The ffmpeg fallback keeps the chosen quality (-q:v) and the aspect
  ratio; the placeholder uses the chosen JPEG settings.
- A locked preview on Windows reads "Preview in use by another program;
  the old one was kept", not the raw WinError.
- Thumbnail work no longer defers the monitor's Needs review recording:
  it writes previews only, never records.
- The disconnect test now proves the job outlives its stream (one worker,
  9 files, three batches); the attach test waits on an event.
This commit is contained in:
Vito Sansevero
2026-10-23 16:31:56 -07:00
parent 2b4b415638
commit 6f78b4f46f
8 changed files with 608 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
py/api/images.py
+55 -12
View File
@@ -282,6 +282,27 @@ THUMBNAIL_ENCODINGS = {
"high": {"quality": 95, "optimize": True},
}
DEFAULT_THUMBNAIL_ENCODING = THUMBNAIL_ENCODINGS["medium"]
# ffmpeg's JPEG scale (-q:v, 2 best to 31 worst) for each encoding's quality.
_FFMPEG_QSCALE = {95: 2, 85: 4, 70: 8}
# A preview that could not be replaced (#249 S5 M6). Windows refuses to rename
# over a file another process has open (WinError 5 or 32), such as a preview
# the gallery is serving; the old preview stays and the file counts as an error.
PREVIEW_IN_USE = "Preview in use by another program; the old one was kept"
PREVIEW_NOT_WRITABLE = "Permission denied writing the preview"
_WINDOWS_LOCKED = (5, 32) # ERROR_ACCESS_DENIED, ERROR_SHARING_VIOLATION
def _preview_write_error(exc):
"""The readable error for a PermissionError while writing a preview."""
if getattr(exc, "winerror", None) in _WINDOWS_LOCKED:
return PREVIEW_IN_USE
return PREVIEW_NOT_WRITABLE
def _ffmpeg_qscale(encoding):
"""ffmpeg's ``-q:v`` for a THUMBNAIL_ENCODINGS entry."""
return str(_FFMPEG_QSCALE.get(encoding["quality"], 4))
def _write_image_thumbnail(src, dst, thumbnail_size, encoding=None):
@@ -1840,12 +1861,13 @@ class ImageRoutesMixin:
return payload
def _plan_gallery_thumbnails(
self, output_dirs: list[Path], rebuild: bool = False
self, output_dirs: list[Path], rebuild: bool = False, stop=None
) -> dict:
"""Plan every gallery root with one shared per-run generation cap.
See ``_plan_thumbnails``; ``fresh_files`` keeps at most
MAX_FRESH_LISTED files across the roots.
MAX_FRESH_LISTED files across the roots. ``stop()`` true ends the walk
at the next file (a cancelled job); the partial plan is returned.
"""
combined = {
"targets": [],
@@ -1855,8 +1877,12 @@ class ImageRoutesMixin:
"fresh_files": [],
}
for root in output_dirs:
if stop is not None and stop():
break
root = Path(root)
plan = self._plan_thumbnails(root, root / "thumbnails", rebuild=rebuild)
plan = self._plan_thumbnails(
root, root / "thumbnails", rebuild=rebuild, stop=stop
)
available = max(0, MAX_THUMBNAIL_FILES - len(combined["targets"]))
combined["targets"].extend(plan["targets"][:available])
combined["skipped"] += plan["skipped"]
@@ -1895,7 +1921,7 @@ class ImageRoutesMixin:
{"success": False, "error": _safe_error(exc)}, status=500
)
def _plan_thumbnails(self, output_path, thumbnails_dir, rebuild=False):
def _plan_thumbnails(self, output_path, thumbnails_dir, rebuild=False, stop=None):
"""Decide which media files need a thumbnail (blocking walk).
Returns ``{"targets": [(src, dst, is_video)], "skipped": n,
@@ -1907,7 +1933,8 @@ class ImageRoutesMixin:
``rebuild`` every file is a target (nothing is skipped). The walk is
bounded like the gallery listing (MAX_SCAN_DEPTH, MAX_GALLERY_FILES,
no symlinks) and at most MAX_THUMBNAIL_FILES targets are queued per
run; ``remaining`` counts the targets beyond that cap.
run; ``remaining`` counts the targets beyond that cap. ``stop()`` true
ends the walk before the next file.
"""
output_path = Path(output_path)
targets = []
@@ -1916,6 +1943,8 @@ class ImageRoutesMixin:
fresh = 0
fresh_files = []
for src in _iter_media_files(output_path, MAX_GALLERY_FILES):
if stop is not None and stop():
break
suffix = src.suffix.lower()
is_video = suffix in VIDEO_EXTENSIONS
rel_no_ext = src.relative_to(output_path).with_suffix("")
@@ -2007,6 +2036,10 @@ class ImageRoutesMixin:
return {**result, "action": "generated"}
except UnidentifiedImageError:
return {**result, "action": "error", "error": "Not a valid image file"}
except PermissionError as e:
message = _preview_write_error(e)
self.logger.warning(f"Thumbnail failed for {src.name}: {message}")
return {**result, "action": "error", "error": message}
except Exception as e:
self.logger.warning(f"Thumbnail failed for {src.name}", exc_info=True)
return {**result, "action": "error", "error": _safe_error(e)}
@@ -2031,10 +2064,13 @@ class ImageRoutesMixin:
):
"""Generate thumbnail from video file. Returns True if successful.
``encoding`` is one of THUMBNAIL_ENCODINGS (medium when None). With
``keep_existing`` an existing preview is kept, and the run fails,
when no frame could be decoded, rather than replaced by the
placeholder.
``encoding`` is one of THUMBNAIL_ENCODINGS (medium when None); it
encodes the OpenCV frame, ffmpeg's frame (as ``-q:v``) and the
placeholder alike. Both decoders fit the frame inside
``thumbnail_size`` keeping its aspect ratio. With ``keep_existing`` an
existing preview is kept, and the run fails, when no frame could be
decoded, rather than replaced by the placeholder. A PermissionError
while replacing the preview propagates (``_generate_one`` names it).
"""
encoding = encoding or DEFAULT_THUMBNAIL_ENCODING
try:
@@ -2082,8 +2118,11 @@ class ImageRoutesMixin:
"00:00:01",
"-vframes",
"1",
"-s",
f"{thumbnail_size[0]}x{thumbnail_size[1]}",
"-vf",
f"scale={thumbnail_size[0]}:{thumbnail_size[1]}"
":force_original_aspect_ratio=decrease",
"-q:v",
_ffmpeg_qscale(encoding),
"-y",
str(tmp_path),
]
@@ -2148,18 +2187,22 @@ class ImageRoutesMixin:
except (OSError, AttributeError):
pass
_atomic_save(img, thumbnail_path, "JPEG", quality=85)
_atomic_save(img, thumbnail_path, "JPEG", **encoding)
self.logger.debug(
f"Generated placeholder video thumbnail: {thumbnail_path}"
)
return True
except PermissionError:
raise
except Exception as e:
self.logger.warning(
f"Failed to create placeholder thumbnail for {video_path}: {e}"
)
return False
except PermissionError:
raise
except Exception as e:
self.logger.error(
f"Video thumbnail generation failed for {video_path}: {e}"
py/api/jobs.py
+16 -7
View File
@@ -292,18 +292,24 @@ class Job:
for queue in list(self._subscribers):
queue.put_nowait(None)
async def events(self):
async def events(self, lead=None):
"""Yield the latest event, then each later one, until the job finishes.
``lead``, when given, is yielded first, after the subscription is in
place, so nothing published while the consumer sends it is missed.
Consume it under ``contextlib.aclosing`` so a consumer that stops early
(a closed socket) drops its queue at once instead of at garbage collection.
"""
queue = asyncio.Queue()
self._subscribers.add(queue)
# As of the subscription: a later event or the end reaches the queue.
snapshot, ended = self.last_event, self.finished
try:
if self.last_event is not None:
yield self.last_event
if self.finished:
if lead is not None:
yield lead
if snapshot is not None:
yield snapshot
if ended:
return
while True:
payload = await queue.get()
@@ -454,11 +460,14 @@ class JobRegistry:
self._jobs[job.id] = job
return job
def blocker(self, kind, *, admission=None, record_changing=None):
def blocker(self, kind, *, admission=None, record_changing=None, compatible=()):
"""The running job a new ``kind`` job would be refused for, or None.
The same kind refuses first; then the oldest running job whose class
the new job's admission class may not run beside.
the new job's admission class may not run beside. Running jobs of a
kind in ``compatible`` never refuse it: the caller knows that work
cannot conflict with its own (the monitor's Needs review recording
beside thumbnail work, which writes previews and no records).
"""
same_kind = self.active(kind)
if same_kind is not None:
@@ -468,7 +477,7 @@ class JobRegistry:
running = [job for job in tuple(self._jobs.values()) if job.running]
running.sort(key=lambda job: job._started_seq)
for job in running:
if job.admission in refused:
if job.admission in refused and job.kind not in compatible:
return job
return None
py/api/review_recording.py
+14 -4
View File
@@ -28,16 +28,23 @@ Recording changes records, so it follows the registry's ``records``
admission (#289). The scan records under its own ``records`` job. The
monitor's pass lasts one file and is not a job: it is refused while a
``records`` or ``guarded`` job runs (``deferred``), and the next scan reads
the file again.
the file again. Thumbnail work (the job and the gallery's generate and clear
holds, kind ``thumbnails``) is ``guarded`` but only writes and removes preview
files under ``thumbnails/`` and never touches the database, so it does not
defer the recording (#249 S5 M7): a long rebuild no longer holds Needs review
back for its whole run.
"""
import os
from pathlib import Path
from .admin import SCAN_LOG_PREFIXES, _plural, _scan_location, _scan_roots
from .jobs import THUMBNAILS_JOB_KIND
# The kind the monitor's recording checks admission under (never registered).
REVIEW_RECORDING_KIND = "review_recording"
# Guarded kinds that change no records, so the recording may run beside them.
PREVIEW_ONLY_KINDS = (THUMBNAILS_JOB_KIND,)
# record_ambiguous_file outcomes that leave the file in the pending queue.
QUEUED_OUTCOMES = ("recorded", "limited", "duplicate")
NEW_OUTCOMES = ("recorded", "limited")
@@ -185,11 +192,14 @@ class ReviewRecordingMixin:
links it); any other file is recorded when it is ambiguous with a
choice, and is ``untraced`` (its pending items without a choice
removed) when it is ambiguous without one. While a
job runs that a ``records`` job may not run beside, nothing is written
and the answer is ``deferred``. Errors propagate: the monitor logs
job runs that a ``records`` job may not run beside, other than
thumbnail work (``PREVIEW_ONLY_KINDS``), nothing is written and the
answer is ``deferred``. Errors propagate: the monitor logs
them and keeps its link.
"""
busy = self._jobs.blocker(REVIEW_RECORDING_KIND, admission="records")
busy = self._jobs.blocker(
REVIEW_RECORDING_KIND, admission="records", compatible=PREVIEW_ONLY_KINDS
)
if busy is not None:
self.logger.info(
f"Needs review was not updated for "
py/api/thumbnail_jobs.py
+73 -27
View File
@@ -19,7 +19,9 @@ old preview; originals are only read.
The legacy gallery stream ``POST /images/generate-thumbnails/progress``
starts this job (its quality preset still sizes the previews) or attaches to
the running one, and streams the job's legacy ``status``/``start``/
the running one. Its first frame, ``job {id, started}``, names that job and
whether this stream started it (the gallery cancels by that id, and only a
job it started); then come the job's legacy ``status``/``start``/
``progress``/``file_error``/``complete`` frames. A client that goes away no
longer stops the work: the job outlives the request, as a scan does.
"""
@@ -65,6 +67,8 @@ _OPTIONS_ERROR = (
f"to {MAX_THUMBNAIL_EDGE}"
)
CANCELLED_ERROR = "Thumbnail generation was cancelled"
CANCELLED_BEFORE_WRITING = "Cancelled before any preview was written"
NOTHING_TO_PREVIEW = "No images or videos to preview"
_MODE_LOG = {"missing": "missing previews", "rebuild": "every preview"}
# Outcome rows: what happened to each file's preview.
_DETAILS = {
@@ -139,6 +143,20 @@ def _frame(event, data):
return {"event": event, "data": data}
def _finished_line(plan, stats):
"""A finished job's status line: what it wrote, or that it had nothing to do."""
if not plan["targets"]:
if plan["skipped"]:
return f"No previews to generate · {plan['skipped']:,} already up to date"
return NOTHING_TO_PREVIEW
parts = [f"{_plural(stats['generated'], 'preview')} written"]
if stats["errors"]:
parts.append(_plural(len(stats["errors"]), "error"))
if plan["remaining"]:
parts.append(f"{plan['remaining']:,} more on the next run")
return " · ".join(parts)
class ThumbnailJobsMixin:
"""The thumbnail plan, the thumbnail job and the legacy stream on it."""
@@ -230,7 +248,11 @@ class ThumbnailJobsMixin:
job.finish(state)
async def _thumbnail_job_body(self, job, options):
"""Plan, then generate in worker batches; returns the final state."""
"""Plan, then generate in worker batches; returns the final state.
Every exit sets the job's final status line (I3): what was written,
that there was nothing to do, or what a cancel kept.
"""
output_dirs = await self._run_in_executor(self._get_all_output_dirs)
if not output_dirs:
message = "ComfyUI output directory not found"
@@ -252,20 +274,22 @@ class ThumbnailJobsMixin:
)
)
plan = await self._run_in_executor(
self._plan_gallery_thumbnails, output_dirs, rebuild=options.rebuild
self._plan_gallery_thumbnails,
output_dirs,
rebuild=options.rebuild,
stop=lambda: job.cancelled,
)
if job.cancelled: # M2: stopped during the walk, before any start frame
return self._end_cancelled(job, 0, CANCELLED_BEFORE_WRITING)
roots = _scan_roots(output_dirs)
self._record_fresh(job, plan, roots)
stats, start = await self._generate_thumbnail_targets(job, options, plan, roots)
self.invalidate_gallery_cache()
generated = _plural(stats["generated"], "preview")
errors = _plural(len(stats["errors"]), "error")
if job.cancelled:
job.log_line(f"Thumbnails cancelled · {generated} written")
job.publish(
_frame("error", {"error": CANCELLED_ERROR, "message": CANCELLED_ERROR})
return self._end_cancelled(
job, stats["generated"], f"Cancelled · {generated} written"
)
return "cancelled"
payload = _thumbnail_complete_payload(
len(plan["targets"]) + plan["skipped"] + plan["remaining"],
stats,
@@ -273,16 +297,27 @@ class ThumbnailJobsMixin:
remaining=plan["remaining"],
processed=len(plan["targets"]),
)
job.update(progress=100)
job.update(progress=100, status_line=_finished_line(plan, stats))
if plan["remaining"]:
job.log_line(
f"{plan['remaining']:,} more previews are left for the next run"
)
errors = _plural(len(stats["errors"]), "error")
job.log_line(f"Thumbnails finished · {generated} written · {errors}")
self.logger.info(payload["message"])
job.publish(_frame("complete", payload))
return "finished"
@staticmethod
def _end_cancelled(job, generated, status_line):
"""Close a cancelled job: its status line, log line and error frame."""
job.update(status_line=status_line)
job.log_line(f"Thumbnails cancelled · {_plural(generated, 'preview')} written")
job.publish(
_frame("error", {"error": CANCELLED_ERROR, "message": CANCELLED_ERROR})
)
return "cancelled"
def _record_fresh(self, job, plan, roots):
"""List the already fresh previews under Skipped (no log line each)."""
rows = []
@@ -310,7 +345,9 @@ class ThumbnailJobsMixin:
)
if plan["skipped"]:
message += f" ({plan['skipped']} already have thumbnails)"
job.update(total=total, status_line=_scan_status_line(0, total, 0, 0))
job.update(total=total)
if total: # never "Processing file 0 of 0" (I3)
job.update(status_line=_scan_status_line(0, total, 0, 0))
job.publish(
_frame(
"start",
@@ -409,40 +446,49 @@ class ThumbnailJobsMixin:
"Connection": "keep-alive",
},
)
running = self._jobs.active(THUMBNAILS_JOB_KIND)
job = running if running is not None and running.cancellable else None
busy = None
if job is None:
busy = self._jobs.blocker(THUMBNAILS_JOB_KIND, admission="guarded")
await response.prepare(request)
if busy is None and job is None:
# The gallery's preset sizes the previews; medium encodes them as
# the gallery always did.
options = ThumbnailOptions("missing", thumbnail_size[0], "medium")
try:
job = self._start_thumbnail_job(options)
except JobBusy as refused:
busy = refused.running
# M3: read the registry after the last await, so a job that started
# while the headers went out is attached to, not refused.
job, started, busy = self._legacy_stream_job(thumbnail_size)
if busy is not None:
message = f"{busy.title} already running"
await self._emit_sse(
response, "error", {"error": message, "message": message}
)
return response
# No await between the start and the first read: the stream subscribes
# before the job's runner publishes its first frame.
events = job.events()
# No await between the start and the subscription: the lead frame is
# sent after it, so the stream misses none of the runner's frames.
events = job.events(lead=_frame("job", {"id": job.id, "started": started}))
try:
async for payload in events:
frame = (
f"event: {payload['event']}\ndata: {json.dumps(payload['data'])}"
)
await response.write(f"{frame}\n\n".encode("utf-8"))
except Exception as exc: # the client went away; the job goes on
except Exception as exc: # a write failed: the client went away
self.logger.info(
f"Thumbnail progress client disconnected ({exc}); "
"the thumbnails continue in the background"
)
finally:
# Also when aiohttp cancels the handler on disconnect: the job is
# its own task and goes on.
await events.aclose()
return response
def _legacy_stream_job(self, thumbnail_size):
"""``(job, started, busy)`` for the legacy stream, synchronously.
Attaches to the running cancellable thumbnail job, or starts one with
the gallery's preset as the size (missing previews, medium encoding,
as the gallery always wrote), or names the job in the way.
"""
running = self._jobs.active(THUMBNAILS_JOB_KIND)
if running is not None and running.cancellable:
return running, False, None
busy = self._jobs.blocker(THUMBNAILS_JOB_KIND, admission="guarded")
if busy is not None:
return None, False, busy
# blocker() said free and nothing awaited since, so the start is admitted.
options = ThumbnailOptions("missing", thumbnail_size[0], "medium")
return self._start_thumbnail_job(options), True, None