Commit Graph
2 Commits
Author SHA1 Message Date
Vito d88cf9e7c6 fix(quality): sweep the Code Quality standard findings (#558)
* fix(quality): preserve cursor iteration and option validation

Keep prefetched rows on ManagedCursor so partial loops and explicit fetches share one position, and stop existing iterators after close or retirement. Preserve chunked reads and native SQLite result lifetime on reexecution and errors.

Catch OptionError before ValueError so legacy auto-tag requests receive the actual validation error. Remove the overwritten ViewerJS title option.

Regression tests failed before the fixes for lost rows, reads after close/retirement, and misleading option errors. Focused Python checks pass; cursor checks also pass on Python 3.13 and 3.14. Refs #321.

* fix(quality): make the test doubles legal raises and hashes

The fake queue no longer raises an attribute that starts as None. The
oversized kind records hashes instead of raising from __hash__, which
also catches a hash that production code swallows. Hostile.__bool__
raises the TypeError a truth test may raise.

* fix(quality): drop the extra saveCoverage argument

* refactor(quality): remove assignments that are overwritten before use

* refactor(quality): remove the remaining overwritten test assignments

* refactor(quality): drop imports repeated inside functions

* refactor(quality): give duplicate identity keys one shape

* refactor(quality): name the unused job event loop variable

* refactor(quality): release model gates in finally, not except BaseException

* refactor(quality): end the job runners with an explicit return

* refactor(quality): use the precise unittest asserts

* refactor(quality): import unittest.mock one way in the tests

* fix(quality): say why each empty except is safe, log the broad ones

* refactor(quality): stop binding values the tests never read

* refactor(quality): remove the unused preview aliases and tag page query

* refactor(quality): remove re-exports nothing imports

* refactor(quality): parenthesize deliberate string joins in lists

* refactor(quality): remove unused JavaScript variables and imports

* refactor(quality): use the precise assert in the model status test
2026-10-31 05:43:48 +00:00
Vito f35e010f79 feat(system): log cleanup rule, reviewed rotated-log cleanup and diagnostic report (#329, PR 1) (#367)
* fix(logs): delete and download only rotated copies under one rule

Rule R (#329): a file is a rotated copy only when it is a direct entry of
the log folder named prompt_manager.log.<n> (n >= 1, no leading zero,
compared after normcase) and a regular file that is not a symlink or a
junction. Everything else is never deleted.

- Clear (POST /logs/truncate) empties the active log and deletes only
  rotated copies; a .bak, a .1.gz, any other file and every link are kept.
  It runs off the event loop under one files lock and the file handler's
  lock, so no rollover renames a file mid-way, and logs only afterwards.
- delete_rotated(expected) deletes exactly the reviewed copies, or nothing
  when one of them changed (size, mtime_ns, inode, missing, now a link).
- The download serves only a listed regular, non-link file, opened without
  following links and read off the loop; a link or folder is a 404.
- /logs/files rows gain kind; files and stats read links with lstat and
  stats no longer count a link's target.
- The config update runs off the loop and swaps handlers under the files
  lock.

* feat(logs): review and apply rotated-log cleanup as a maintenance plan

POST /maintenance/preview {operation: "rotated_logs", options: {}} lists the
rotated copies Rule R allows, with size and time, and what it keeps (the
active log, links, other files); it writes nothing and answers the Plan
shape with basenames only, backup policy none and no disk key.

POST /maintenance/apply starts the "Rotated log copies" maintenance job
(admission read: runs beside a scan, refused beside another maintenance job
or an exclusive one). It deletes exactly the reviewed copies, or nothing and
fails plan-changed when any of them changed after the review; a copy that
cannot be deleted becomes an errors row. The token is consumed on the loop
turn the job starts. (#329)

* feat(diagnostics): add a shareable report without secrets or paths

GET /system/diagnostics builds the report on the server, off the event loop
and under a registry lease (409 busy while a restore runs): the project and
Viewer.js versions, Python and the platform, the four legacy checks (one
helper, legacy_checks, that GET /diagnostics also calls), public database
info, source counts, the log configuration and statistics, the last health
summary and the config.json-only keys from an explicit list. The CivitAI key
is reported only as has_civitai_api_key; every path is relative to ComfyUI
or reads "outside ComfyUI". A scrub replaces anything still absolute, the
key or the home folder and counts it in redacted (0 for a correct build).

The version parser moves unchanged to utils/version.py. (#329)

* fix(logs): never empty a renamed log or serve a swapped file; keep root anchors private

Review findings on #329 PR 1:
- Clear empties the active log through the handler only when the handler's
  open stream is the listed file (same inode); a log the user renamed while
  ComfyUI runs is no longer emptied.
- A download is refused when the opened file is not the one listed (inode),
  or has another name (a hard link).
- Windows reparse points that name another file (the name-surrogate bit:
  symlinks, junctions, WSL links) are links under Rule R; cloud placeholders
  stay regular files.
- The report's paths use the closest anchor that is not a filesystem root,
  and the scrub also catches the home folder without its root.

* fix(diagnostics): never anchor a report path above the home folder; serve only regular log files

Review findings on #329 PR 1:
- report_path never uses a filesystem root or a strict ancestor of the
  home folder (/home, C:\Users) as its anchor, so a path inside home needs
  an anchor at or below it and otherwise reads "outside ComfyUI"; no path
  starts with a user name. The scrub matches the home folder only as whole
  path parts (in full, without its root, and its name).
- A download serves only a regular file (a FIFO, socket or device is a
  404) and opens it non-blocking.
- One unreadable timestamp no longer breaks the log listing, stats,
  download, review or Clear.
- The report tests read a temporary log folder, never the process's own.

* fix(diagnostics): match the bare home folder name only inside paths

A home folder named like a word (/home/info) no longer removes the log
level INFO from the report; the name still goes when it is a part of a
path. Documents the accepted residual risk of another user's home. (#329)
2026-10-27 06:19:39 -07:00