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)
This commit is contained in:
Vito
+304
-45
@@ -10,9 +10,12 @@ This module provides centralized logging configuration with support for:
|
||||
"""
|
||||
|
||||
import collections
|
||||
import fnmatch
|
||||
import logging
|
||||
import logging.handlers
|
||||
import os
|
||||
import re
|
||||
import stat
|
||||
import threading
|
||||
from datetime import datetime
|
||||
from typing import Dict, Any, Optional, List
|
||||
@@ -20,6 +23,160 @@ from pathlib import Path
|
||||
|
||||
from . import library_location
|
||||
|
||||
ACTIVE_LOG_NAME = "prompt_manager.log"
|
||||
# Rule R (#329): a rotated copy is prompt_manager.log.<n>, n >= 1, no leading 0.
|
||||
_ROTATED_NAME = re.compile(r"prompt_manager\.log\.([1-9][0-9]*)")
|
||||
# The legacy listing's pattern (GET /logs/files and /logs/stats).
|
||||
_LISTED_PATTERN = "*.log*"
|
||||
LOG_FILE_KINDS = ("active", "rotated", "link", "other")
|
||||
|
||||
|
||||
class RotatedChanged(Exception):
|
||||
"""A reviewed rotated copy changed since the review; nothing was deleted."""
|
||||
|
||||
def __init__(self, name):
|
||||
super().__init__(f"{name} changed since the review")
|
||||
self.name = name
|
||||
|
||||
|
||||
def _name_key(name: str) -> str:
|
||||
"""How a name compares: case-folded where the filesystem folds it (Windows)."""
|
||||
return os.path.normcase(name)
|
||||
|
||||
|
||||
def _rotated_number(name: str) -> Optional[int]:
|
||||
match = _ROTATED_NAME.fullmatch(_name_key(name))
|
||||
return int(match.group(1)) if match else None
|
||||
|
||||
|
||||
# Windows reparse tags with the name-surrogate bit name another file or
|
||||
# folder (symlinks, junctions, WSL symlinks); cloud placeholders do not.
|
||||
_NAME_SURROGATE = 0x20000000
|
||||
|
||||
|
||||
def _is_link(path: str, st: os.stat_result) -> bool:
|
||||
if getattr(st, "st_reparse_tag", 0) & _NAME_SURROGATE:
|
||||
return True
|
||||
return stat.S_ISLNK(st.st_mode) or os.path.isjunction(path)
|
||||
|
||||
|
||||
class LogFilePath(type(Path())):
|
||||
"""A listed log file's path, with the inode it had when it was listed."""
|
||||
|
||||
listed_ino = 0
|
||||
|
||||
|
||||
def _kind(name: str, path: str, st: os.stat_result) -> str:
|
||||
"""Rule R: ``active``, ``rotated``, ``link`` or ``other`` (never deleted)."""
|
||||
if _is_link(path, st):
|
||||
return "link"
|
||||
if not stat.S_ISREG(st.st_mode):
|
||||
return "other"
|
||||
if _name_key(name) == _name_key(ACTIVE_LOG_NAME):
|
||||
return "active"
|
||||
if _rotated_number(name) is not None:
|
||||
return "rotated"
|
||||
return "other"
|
||||
|
||||
|
||||
def scan_log_folder(log_dir) -> List[Dict[str, Any]]:
|
||||
"""Every direct entry of ``log_dir`` with its Rule R kind (blocking).
|
||||
|
||||
Rows are ``{name, kind, size, mtime_ns, ino, modified}``, read with
|
||||
``lstat`` so a link is described, never followed. An entry that cannot
|
||||
be read is left out. Never logs: the cleanup calls it while it holds the
|
||||
file handler's lock.
|
||||
"""
|
||||
rows = []
|
||||
try:
|
||||
names = [entry.name for entry in os.scandir(log_dir)]
|
||||
except OSError:
|
||||
return rows
|
||||
for name in names:
|
||||
path = os.path.join(log_dir, name)
|
||||
try:
|
||||
st = os.lstat(path)
|
||||
kind = _kind(name, path, st)
|
||||
except OSError:
|
||||
continue
|
||||
try:
|
||||
modified = datetime.fromtimestamp(st.st_mtime).isoformat()
|
||||
except (OSError, OverflowError, ValueError): # a time before 1970 on Windows
|
||||
modified = ""
|
||||
rows.append(
|
||||
{
|
||||
"name": name,
|
||||
"kind": kind,
|
||||
"size": st.st_size,
|
||||
"mtime_ns": st.st_mtime_ns,
|
||||
"ino": st.st_ino,
|
||||
"modified": modified,
|
||||
"_dir": stat.S_ISDIR(st.st_mode) and kind != "link",
|
||||
"_regular": stat.S_ISREG(st.st_mode) and kind != "link",
|
||||
}
|
||||
)
|
||||
return rows
|
||||
|
||||
|
||||
def public_row(row: Dict[str, Any]) -> Dict[str, Any]:
|
||||
"""A scan row without its private keys."""
|
||||
return {key: value for key, value in row.items() if not key.startswith("_")}
|
||||
|
||||
|
||||
def rotated_sort_key(row: Dict[str, Any]) -> int:
|
||||
return _rotated_number(row["name"]) or 0
|
||||
|
||||
|
||||
def log_file_entry(log_dir, name) -> Path:
|
||||
"""The path of a downloadable log file named ``name`` (blocking).
|
||||
|
||||
Only a regular, non-link file listed directly in ``log_dir`` under that
|
||||
exact name; a separator, ``..``, a stream suffix or any unlisted name
|
||||
raises FileNotFoundError, as a missing file does.
|
||||
"""
|
||||
if not isinstance(name, str) or not name or name in (".", ".."):
|
||||
raise FileNotFoundError("Log file not found")
|
||||
if any(ch in name for ch in ("/", "\\", "\x00", ":")):
|
||||
raise FileNotFoundError("Log file not found")
|
||||
for row in scan_log_folder(log_dir):
|
||||
if row["name"] == name and row["kind"] in ("active", "rotated", "other"):
|
||||
if not row["_regular"]: # a folder, FIFO, socket or device
|
||||
break
|
||||
entry = LogFilePath(log_dir, row["name"])
|
||||
entry.listed_ino = row["ino"]
|
||||
return entry
|
||||
raise FileNotFoundError("Log file not found")
|
||||
|
||||
|
||||
def _stream_is(stream, ino: int) -> bool:
|
||||
"""Whether ``stream`` is open on the file with inode ``ino`` (0: unknown)."""
|
||||
try:
|
||||
open_ino = os.fstat(stream.fileno()).st_ino
|
||||
except (OSError, ValueError, AttributeError):
|
||||
return False
|
||||
return not (ino and open_ino and open_ino != ino)
|
||||
|
||||
|
||||
def _error_text(error: BaseException) -> str:
|
||||
"""An OSError's reason and file name, without the folder it is in."""
|
||||
if isinstance(error, OSError):
|
||||
parts = [error.strerror or type(error).__name__]
|
||||
if error.filename:
|
||||
parts.append(os.path.basename(str(error.filename)))
|
||||
return ": ".join(parts)
|
||||
return str(error) or type(error).__name__
|
||||
|
||||
|
||||
def _matches_review(row: Optional[Dict[str, Any]], reviewed: Dict[str, Any]) -> bool:
|
||||
if row is None or row["kind"] != "rotated":
|
||||
return False
|
||||
if row["size"] != reviewed.get("size") or row["mtime_ns"] != reviewed.get(
|
||||
"mtime_ns"
|
||||
):
|
||||
return False
|
||||
ino = reviewed.get("ino") or 0
|
||||
return not (ino and row["ino"] and row["ino"] != ino)
|
||||
|
||||
|
||||
class _OwnedRotatingFileHandler(logging.handlers.RotatingFileHandler):
|
||||
"""Keep a retired owned sink closed when an in-flight record resumes."""
|
||||
@@ -87,6 +244,9 @@ class PromptManagerLogger:
|
||||
# Memory buffer for recent logs (for web viewer)
|
||||
self._log_buffer = collections.deque(maxlen=self.config["buffer_size"])
|
||||
self._buffer_lock = threading.Lock()
|
||||
# Held while a handler is replaced or log files are deleted (#329):
|
||||
# taken before any file handler's own lock, never the other way round.
|
||||
self._files_lock = threading.Lock()
|
||||
|
||||
# Initialize loggers
|
||||
self._setup_loggers()
|
||||
@@ -238,6 +398,14 @@ class PromptManagerLogger:
|
||||
# Return most recent first
|
||||
return list(reversed(logs[-limit:]))
|
||||
|
||||
def scan_log_folder(self) -> List[Dict[str, Any]]:
|
||||
"""Every entry of the log folder with its Rule R kind (blocking)."""
|
||||
return [public_row(row) for row in scan_log_folder(self.log_dir)]
|
||||
|
||||
def log_file_entry(self, name: str) -> Path:
|
||||
"""A downloadable (regular, non-link) log file; FileNotFoundError else."""
|
||||
return log_file_entry(self.log_dir, name)
|
||||
|
||||
def get_log_files(self) -> List[Dict[str, Any]]:
|
||||
"""Get information about available log files.
|
||||
|
||||
@@ -245,26 +413,27 @@ class PromptManagerLogger:
|
||||
List of dictionaries containing file information:
|
||||
- filename: Name of the log file
|
||||
- path: Full path to the log file
|
||||
- size: File size in bytes
|
||||
- size: File size in bytes (a link's own size: links are not followed)
|
||||
- modified: ISO formatted modification timestamp
|
||||
- is_main: True if this is the main log file
|
||||
- kind: ``active``, ``rotated``, ``link`` or ``other`` (Rule R)
|
||||
"""
|
||||
log_files = []
|
||||
|
||||
for log_file in self.log_dir.glob("*.log*"):
|
||||
try:
|
||||
stat = log_file.stat()
|
||||
log_files.append(
|
||||
{
|
||||
"filename": log_file.name,
|
||||
"path": str(log_file),
|
||||
"size": stat.st_size,
|
||||
"modified": datetime.fromtimestamp(stat.st_mtime).isoformat(),
|
||||
"is_main": log_file.name == "prompt_manager.log",
|
||||
}
|
||||
)
|
||||
except Exception as e:
|
||||
self.logger.error(f"Error getting info for log file {log_file}: {e}")
|
||||
for row in scan_log_folder(self.log_dir):
|
||||
if row["_dir"]:
|
||||
continue
|
||||
if not fnmatch.fnmatchcase(_name_key(row["name"]), _LISTED_PATTERN):
|
||||
continue
|
||||
log_files.append(
|
||||
{
|
||||
"filename": row["name"],
|
||||
"path": str(self.log_dir / row["name"]),
|
||||
"size": row["size"],
|
||||
"modified": row["modified"],
|
||||
"is_main": row["kind"] == "active",
|
||||
"kind": row["kind"],
|
||||
}
|
||||
)
|
||||
|
||||
# Sort by modification time, newest first
|
||||
log_files.sort(key=lambda x: x["modified"], reverse=True)
|
||||
@@ -302,14 +471,44 @@ class PromptManagerLogger:
|
||||
self.logger.error(f"Error reading log file {filename}: {e}")
|
||||
raise
|
||||
|
||||
def _truncate_main_log(self, log_file: Path) -> None:
|
||||
def _file_handlers(self) -> List[logging.Handler]:
|
||||
return [h for h in self.logger.handlers if isinstance(h, logging.FileHandler)]
|
||||
|
||||
def _acquire_file_handlers(self) -> List[logging.Handler]:
|
||||
"""Take every file handler's lock: no rollover renames a file meanwhile.
|
||||
|
||||
The caller holds ``_files_lock`` (so no handler is replaced) and must
|
||||
not log until it released them: the lock is re-entrant, so a record
|
||||
logged here could roll the log over under the caller's feet.
|
||||
"""
|
||||
held = []
|
||||
try:
|
||||
for handler in self._file_handlers():
|
||||
handler.acquire()
|
||||
held.append(handler)
|
||||
except BaseException:
|
||||
self._release_handlers(held)
|
||||
raise
|
||||
return held
|
||||
|
||||
@staticmethod
|
||||
def _release_handlers(held) -> None:
|
||||
for handler in reversed(held):
|
||||
handler.release()
|
||||
|
||||
def _truncate_main_log(self, log_file: Path, expected_ino: int = 0) -> None:
|
||||
"""Empty the main log through the handler that writes to it.
|
||||
|
||||
Opening the file a second time with ``"w"`` leaves the handler's own
|
||||
stream at its old offset: later records land past the new end
|
||||
(zero-padded on POSIX) and ``tell()`` keeps reporting the old size, so
|
||||
the next record rolls the file over. Truncating the handler's stream
|
||||
keeps position and size in step.
|
||||
keeps position and size in step. Without a handler on it (file logging
|
||||
off) the existing file is emptied in place, never through a link.
|
||||
|
||||
A handler whose open stream is not the listed file (``expected_ino``:
|
||||
the user renamed the log, which the handler still writes to) is left
|
||||
alone: the file under the active name is emptied instead.
|
||||
"""
|
||||
handled = False
|
||||
target = os.path.normcase(os.path.realpath(str(log_file)))
|
||||
@@ -320,6 +519,8 @@ class PromptManagerLogger:
|
||||
continue
|
||||
if os.path.normcase(os.path.realpath(base)) != target:
|
||||
continue
|
||||
if not _stream_is(stream, expected_ino):
|
||||
continue
|
||||
handler.acquire()
|
||||
try:
|
||||
stream.flush()
|
||||
@@ -329,14 +530,65 @@ class PromptManagerLogger:
|
||||
finally:
|
||||
handler.release()
|
||||
if not handled:
|
||||
with open(log_file, "w", encoding="utf-8", errors="replace") as f:
|
||||
f.write("")
|
||||
flags = os.O_WRONLY | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0)
|
||||
os.close(os.open(str(log_file), flags))
|
||||
|
||||
def delete_rotated(self, expected: List[Dict[str, Any]]) -> Dict[str, Any]:
|
||||
"""Delete exactly the reviewed rotated copies, or none of them (blocking).
|
||||
|
||||
``expected`` are scan rows (``name``, ``size``, ``mtime_ns``,
|
||||
``ino``). Under ``_files_lock`` and every file handler's lock the
|
||||
folder is scanned again; when any expected file is missing, is no
|
||||
longer a rotated copy (Rule R) or differs in size, ``mtime_ns`` or a
|
||||
non-zero ``ino``, RotatedChanged is raised with nothing deleted.
|
||||
Otherwise each is unlinked in ``<n>`` order.
|
||||
|
||||
Returns ``{"removed": [names], "failed": [(name, OSError)]}``. One
|
||||
summary line is logged once both locks are released.
|
||||
"""
|
||||
removed, failed = [], []
|
||||
changed = None
|
||||
with self._files_lock:
|
||||
held = self._acquire_file_handlers()
|
||||
try:
|
||||
current = {
|
||||
_name_key(row["name"]): row for row in scan_log_folder(self.log_dir)
|
||||
}
|
||||
targets = []
|
||||
for reviewed in sorted(expected, key=rotated_sort_key):
|
||||
row = current.get(_name_key(str(reviewed.get("name", ""))))
|
||||
if not _matches_review(row, reviewed):
|
||||
changed = str(reviewed.get("name", ""))
|
||||
break
|
||||
targets.append(row["name"])
|
||||
if changed is None:
|
||||
for name in targets:
|
||||
try:
|
||||
os.unlink(os.path.join(self.log_dir, name))
|
||||
removed.append(name)
|
||||
except OSError as error:
|
||||
failed.append((name, error))
|
||||
finally:
|
||||
self._release_handlers(held)
|
||||
if changed is not None:
|
||||
self.logger.warning(
|
||||
"Rotated log cleanup refused: %s changed since the review", changed
|
||||
)
|
||||
raise RotatedChanged(changed)
|
||||
self.logger.info(
|
||||
"Deleted %d rotated log copies%s",
|
||||
len(removed),
|
||||
f", {len(failed)} could not be deleted" if failed else "",
|
||||
)
|
||||
return {"removed": removed, "failed": failed}
|
||||
|
||||
def truncate_logs(self) -> Dict[str, Any]:
|
||||
"""Truncate all log files.
|
||||
"""Empty the active log and delete its rotated copies (Rule R).
|
||||
|
||||
Clears the main log file and deletes rotated log files.
|
||||
Also clears the memory buffer.
|
||||
Every other file in the log folder, and every link, is kept. Also
|
||||
clears the memory buffer. Runs under ``_files_lock`` and the file
|
||||
handlers' locks, so no rollover recreates a just-deleted copy, and
|
||||
logs only once they are released.
|
||||
|
||||
Returns:
|
||||
Dictionary with:
|
||||
@@ -345,28 +597,31 @@ class PromptManagerLogger:
|
||||
"""
|
||||
results = {"truncated": [], "errors": []}
|
||||
|
||||
# Collect first and stay silent until every file has been handled:
|
||||
# a log line written mid-way can roll the just-emptied main log over
|
||||
# and recreate the backup that was deleted a moment earlier.
|
||||
log_files = sorted(self.log_dir.glob("prompt_manager.log*"))
|
||||
for log_file in log_files:
|
||||
with self._files_lock:
|
||||
held = self._acquire_file_handlers()
|
||||
try:
|
||||
if log_file.name == "prompt_manager.log":
|
||||
self._truncate_main_log(log_file)
|
||||
else:
|
||||
# For rotated files, delete them
|
||||
log_file.unlink()
|
||||
|
||||
results["truncated"].append(log_file.name)
|
||||
|
||||
except Exception as e:
|
||||
results["errors"].append(
|
||||
f"Failed to truncate {log_file.name}: {str(e)}"
|
||||
rows = scan_log_folder(self.log_dir)
|
||||
active = [row for row in rows if row["kind"] == "active"]
|
||||
rotated = sorted(
|
||||
(row for row in rows if row["kind"] == "rotated"),
|
||||
key=rotated_sort_key,
|
||||
)
|
||||
|
||||
# Clear memory buffer
|
||||
with self._buffer_lock:
|
||||
self._log_buffer.clear()
|
||||
for row in active + rotated:
|
||||
path = os.path.join(self.log_dir, row["name"])
|
||||
try:
|
||||
if row["kind"] == "active":
|
||||
self._truncate_main_log(Path(path), row["ino"])
|
||||
else:
|
||||
os.unlink(path)
|
||||
results["truncated"].append(row["name"])
|
||||
except OSError as error:
|
||||
results["errors"].append(
|
||||
f"Failed to truncate {row['name']}: {_error_text(error)}"
|
||||
)
|
||||
with self._buffer_lock:
|
||||
self._log_buffer.clear()
|
||||
finally:
|
||||
self._release_handlers(held)
|
||||
|
||||
if results["truncated"]:
|
||||
self.logger.info("Truncated log files: %s", ", ".join(results["truncated"]))
|
||||
@@ -409,7 +664,9 @@ class PromptManagerLogger:
|
||||
"backup_count",
|
||||
]
|
||||
):
|
||||
self._setup_loggers()
|
||||
# No cleanup may hold the old handler while it is replaced.
|
||||
with self._files_lock:
|
||||
self._setup_loggers()
|
||||
|
||||
self.logger.info(f"Updated logging configuration: {new_config}")
|
||||
|
||||
@@ -440,7 +697,9 @@ class PromptManagerLogger:
|
||||
level = entry["level"]
|
||||
level_counts[level] = level_counts.get(level, 0) + 1
|
||||
|
||||
log_files = self.get_log_files()
|
||||
# A link's target is not a log file of this folder: neither counted
|
||||
# nor sized.
|
||||
log_files = [f for f in self.get_log_files() if f["kind"] != "link"]
|
||||
total_log_size = sum(f["size"] for f in log_files)
|
||||
|
||||
return {
|
||||
|
||||
@@ -0,0 +1,18 @@
|
||||
"""The project version, read from ``pyproject.toml`` (#329)."""
|
||||
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
|
||||
def project_version(root):
|
||||
"""The version in ``root``'s ``pyproject.toml``; ``"unknown"`` on failure."""
|
||||
try:
|
||||
pyproject_path = Path(root) / "pyproject.toml"
|
||||
if pyproject_path.exists():
|
||||
content = pyproject_path.read_text()
|
||||
match = re.search(r'version\s*=\s*["\']([^"\']+)["\']', content)
|
||||
if match:
|
||||
return match.group(1)
|
||||
except Exception: # nosec B110 - an unreadable file reads as unknown
|
||||
pass
|
||||
return "unknown"
|
||||
Reference in New Issue
Block a user