From 58ededde6dc32b9e93e505fc160ccaa6a8a2b71d Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 23 Oct 2026 19:11:01 -0700 Subject: [PATCH] fix(maintenance): never remove the kept copy; move records before unlink Final review of #313: - C1: the duplicate find tells files apart by (st_dev, st_ino), so a source listed twice, a symlinked source inside another and a hardlink are one file. The preview and the job refuse a removal that os.path.samestat matches with its kept copy. - I1: each file's records move to its verified kept copy in one short transaction before that file is unlinked; a failed move deletes nothing and a failed unlink leaves the records on the identical kept copy. The status line counts files removed and not removed. - I2: links are looked up under every spelling of the source (the real path, the configured and monitored folders, ComfyUI's output). - I5: image tags join the kept copy as their union and the removed copy's pending, undecided Needs review items are settled; the plan says so. - I3: the VACUUM disk check measures SQLite's real temporary folder. - M1: the token is looked at again with no await before the job starts. - M2: the backup decision is made once and handed to the job. - M3: backups are linked into place; startup sweeps interrupted temps. - M4: Optimize runs alone (new "exclusive" admission) and its plan says the database is locked while VACUUM runs. - M5: the reviewed duplicate merge is strict: collisions and Library titles are protected, and errors roll back. The legacy cleanup is unchanged. - M7: 2,000 keep entries within a 1 MiB body. M8: event waits in tests. - M9: the preview binds files by size, mtime and inode without hashing; the job hashes right before each unlink. --- TESTING.md | 62 ++++-- database/duplicate_copies.py | 211 ++++++++++++++++++++ database/maintenance.py | 256 ++++++++++++------------ database/operations.py | 26 ++- py/api/__init__.py | 10 + py/api/admin.py | 37 +++- py/api/duplicate_files.py | 299 ++++++++++++++++++++--------- py/api/duplicate_review.py | 119 +++++++++--- py/api/jobs.py | 16 +- py/api/maintenance.py | 180 +++++++++++++---- tests/test_api_duplicate_files.py | 42 ++-- tests/test_api_duplicate_safety.py | 275 ++++++++++++++++++++++++++ tests/test_api_maintenance.py | 223 ++++++++++++++++++++- tests/test_job_admission.py | 4 +- tests/test_maintenance_plans.py | 2 +- 15 files changed, 1422 insertions(+), 340 deletions(-) create mode 100644 database/duplicate_copies.py create mode 100644 tests/test_api_duplicate_safety.py diff --git a/TESTING.md b/TESTING.md index 395e9dc5..0d231afa 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2519,7 +2519,7 @@ protected reasons are exactly the prune's rules) and `missing_links` (legacy `health.link_states`: only a link under a source that can be read now counts; one under an unavailable source or outside every source is protected). The operation enum is strict, `options` must be `{}` and the body is capped at 4 KiB (512 KiB for -`duplicate_files`, below). A preview meets the +`duplicate_files`: 1 MiB, below). A preview meets the read-only standard above on its own `mode=ro` snapshot (`test_preview_writes_nothing`, per operation, plus the media bytes) and stats files after the snapshot ended. The plan lists at most 50 detail rows and no server path. Its fingerprint (the ids, text @@ -2576,7 +2576,23 @@ pool. Each plan reports `backup.expected_bytes` (the backup's size, `page_count page_size`, or null with the policy off) and `disk: {free_bytes, required_bytes, sufficient}` for the database folder: the backup needs its size there, and Optimize needs the database's size twice (the WAL beside it and SQLite's temporary copy, added -up when the temporary folder is on the same disk), each plus a 16 MiB margin. Apply +up when the temporary folder is on the same disk), each plus a 16 MiB margin. The +temporary folder is the one SQLite really uses (`sqlite_temp_dir`): on Unix +`SQLITE_TMPDIR`, `TMPDIR`, then the first writable of `/var/tmp`, `/usr/tmp` and +`/tmp` (not Python's `tempfile` order, which picks a RAM-backed `/tmp` on many Linux +systems); on Windows `TMP`, `TEMP`, then `USERPROFILE`. Whether a backup is taken is +decided once, when Apply measured the disk, and handed to the job. Apply looks at the +token again with no await before it starts the job, so a token used or expired during +the measurement answers its code, never a 500. A backup is put in place with +`os.link` (atomic, refuses an existing name; reserve-and-replace where a filesystem +has no hard links), and startup removes the `.pm-safety-*.tmp` copies an interrupted +backup left. Optimize runs alone: its job has the `exclusive` admission class (refused +beside any job, and every job is refused while it runs), and its plan says the database +is locked while VACUUM runs and that run counts or captures waiting longer than the +30-second busy timeout are not recorded. The duplicate prompt merge is strict inside +the apply: a group whose copies link one image twice, or whose removed copy has its own +Library title, is protected in the plan, and any merge error rolls the transaction back +(`MergeConflict`); the legacy cleanup keeps its lenient behaviour. Apply measures again before it starts a job and refuses a short disk up front with 507 `insufficient-space` and a truthful message ("Not enough free disk space: the safety backup needs about … in the database folder and … is free. Nothing was changed."); no @@ -2589,31 +2605,49 @@ job starts and the token stays usable find: kind `duplicates`, "Find duplicate image files", admission `read`, cancellable, group `groups_found`, over every effective source (the legacy scan's `_hash_media_groups`: no symlinks, no `thumbnails`, bounded, each file once). The -server holds the groups (content hash, root-relative paths, source index) for the last -three finds; `GET /prompt_manager/maintenance/duplicates/{job_id}?offset&limit` pages +find lists a physical file once, by `(st_dev, st_ino)`: a source listed twice under two +spellings, a symlinked source inside another one and a hardlink are one file, never a +copy of themselves (`test_a_folder_listed_twice_never_loses_its_only_copy`, +`test_a_symlinked_source_inside_another_is_one_file`, +`test_a_hardlink_is_never_a_copy_to_remove`). The server holds the groups (content +hash, root-relative paths, source index, size, mtime and inode) for the last three +finds; `GET /prompt_manager/maintenance/duplicates/{job_id}?offset&limit` pages them ten at a time (`not-found` 404, `job-running` or `no-groups` 409). The preview `{operation:"duplicate_files", options:{job_id, keep:[{group_id, root_index, path}]}}` -(body cap 512 KiB, at most 5,000 keep entries) removes, in each group named, every copy +(body cap 1 MiB, at most 2,000 keep entries; `test_the_keep_limit_fits_the_body_cap`) removes, in each group named, every copy not kept; a keeper that is not in its group is `invalid-request`, and a source no longer listed at its index is `sources-changed`. Every keeper and removal is checked with `file_problem`: containment as text first, with no filesystem access (inside its source, not under `thumbnails`, a media extension), then `lstat` (a link or any -non-regular file is refused), the resolved path still inside the resolved source, the -same size and the same sha256. A removal that fails, or whose kept copies all fail, is +non-regular file is refused), the resolved path still inside the resolved source, not +the kept copy itself (`os.path.samestat`), and the size, mtime and inode the find saw. +The preview reads no file; the removal job hashes each file and its kept copy again +right before the unlink (`test_preview_reads_no_file_and_spots_a_touched_one`). A removal that fails, or whose kept copies all fail, is protected with its reason. The plan names each removal, its kept copy and how many database links move, says the safety backup covers the database and not the files, and writes nothing. Apply runs `duplicate_cleanup` (records, not cancellable, groups `removed`, `kept`, `errors`): sources unchanged, the promised safety backup, then per -file the kept copy and the file are checked again and the file is removed (every hash -handle closed first: `test_handles_closed_before_unlink`), with its thumbnail (a -video's `.jpg` too). A changed file is kept (`test_changed_content_skips_file`); the +file the kept copy and the file are checked again (hashes included, and never the same +file), the file's records move to the kept copy in one short `BEGIN IMMEDIATE` +transaction, and only then is the file removed (every hash handle closed first: +`test_handles_closed_before_unlink`), with its thumbnail (a video's `.jpg` too). A crash +or a failed unlink therefore leaves the records on the identical kept copy, and a failed +move leaves the file and its records alone: no row ever names a deleted file +(`test_a_failure_between_two_files_leaves_no_row_on_a_deleted_file`). The status line +counts the files removed and not removed. A changed file is kept (`test_changed_content_skips_file`); the first failed unlink stops the job, a locked or read-only file with "The file is open in another program or read-only; it was not deleted." and never the OS text, and the files after it are listed as not attempted (`test_partial_failure_reports_each_path`). -Only the files actually removed have their links moved: each image record moves to the -kept copy, or, when its prompt already links the kept copy, hands its saved-pair links -to that record and is deleted; a favorite moves with it -(`test_db_links_reconciled_only_for_removed_files`). The find refuses while a removal +Records move for the reviewed files only (`database/duplicate_copies.py`): each image +record, looked up under every spelling of its source (the real path, the configured and +monitored folders and ComfyUI's output folder that resolve to it: +`test_links_under_a_symlinked_source_spelling_move`), moves to the kept copy under the +same spelling, or, when its prompt already links the kept copy, hands its saved-pair +links to that record and is deleted; a favorite moves with it +(`test_db_links_reconciled_only_for_removed_files`); the copy's image tags join the kept +copy's as their union; and its pending Needs review items (still `pending` at revision +0 with no decision row) are settled (`test_tags_move_as_a_union_and_pending_items_settle`). +The plan states each of these. The find refuses while a removal runs and the removal while a find runs. The legacy gallery routes keep their shapes on the same kinds: `/scan_duplicates` diff --git a/database/duplicate_copies.py b/database/duplicate_copies.py new file mode 100644 index 00000000..30f57c83 --- /dev/null +++ b/database/duplicate_copies.py @@ -0,0 +1,211 @@ +"""Records of duplicate image files: count them and move them (#249 S6, T17). + +A removed copy's records move to its verified kept copy before the copy is +unlinked (``py/api/duplicate_files.py``), one file per short ``BEGIN +IMMEDIATE`` transaction, so no row ever names a deleted file: + +- its image records (``generated_images``), found under every spelling the + file is linked by. A record moves to the kept copy, unless its prompt + already links the kept copy: then its saved-pair links join that record + and it is deleted; +- its favorite; +- its image tags (``media_image_tags`` of its byte record), joined onto the + kept copy's byte record as their union, in order; +- its pending Needs review items, settled: only an item still ``pending`` at + revision 0 with no decision row is removed (``review._SETTLE_ITEM``), so a + decided item is never touched. + +Every statement is an index search (see ``FILE_LINK_SQL``). +""" + +import os + +from .annotation_schema import BYTES_TABLE, FILES_TABLE +from .annotations import _BUMP_INDEX +from .models import normalize_image_path, utc_now_iso +from .review import _BUMP_REVIEW_INDEX, _PENDING_OF_LOCATOR, _SETTLE_ITEM +from .review_changes import notify_review_change + +LINKS_BY_FILE_SQL = "SELECT id, prompt_id FROM generated_images WHERE file_path = ?" +LEGACY_LINKS_SQL = ( + "SELECT id, prompt_id FROM generated_images " + "WHERE image_path = ? AND file_path IS NULL" +) +KEEPER_LINK_SQL = ( + "SELECT id FROM generated_images WHERE prompt_id = ? AND file_path = ?" +) +MOVE_PAIR_LINKS_SQL = ( + "UPDATE OR IGNORE image_prompt_pairs SET image_id = ? WHERE image_id = ?" +) +MOVE_LINK_SQL = ( + "UPDATE generated_images SET image_path = ?, filename = ?, file_path = ? " + "WHERE id = ?" +) +DELETE_LINK_SQL = "DELETE FROM generated_images WHERE id = ?" +FAVORITE_SQL = ( + "SELECT created_at FROM image_favorites WHERE root_key = ? AND relative_path = ?" +) +INSERT_FAVORITE_SQL = "INSERT OR IGNORE INTO image_favorites VALUES (?, ?, ?)" +DELETE_FAVORITE_SQL = ( + "DELETE FROM image_favorites WHERE root_key = ? AND relative_path = ?" +) +COPY_BYTE_SQL = ( + f"SELECT b.id FROM {FILES_TABLE} f JOIN {BYTES_TABLE} b " # nosec B608 + "ON b.file_id=f.id WHERE f.locator=? AND b.sha256=?" +) +COPY_TAG_COUNT_SQL = ( + "SELECT COUNT(*) FROM media_image_tags WHERE byte_id=?" # nosec B608 +) +PENDING_COUNT_SQL = ( + f"SELECT COUNT(*) FROM {FILES_TABLE} f " # nosec B608 + f"JOIN {BYTES_TABLE} b ON b.file_id=f.id " + "JOIN media_review_items i ON i.byte_id=b.id " + "WHERE f.locator=? AND +i.state='pending' AND i.revision=0 " + "AND NOT EXISTS (SELECT 1 FROM media_review_decisions d WHERE d.item_id=i.id)" +) +FILE_LINK_SQL = ( + LINKS_BY_FILE_SQL, + LEGACY_LINKS_SQL, + KEEPER_LINK_SQL, + MOVE_PAIR_LINKS_SQL, + MOVE_LINK_SQL, + DELETE_LINK_SQL, + FAVORITE_SQL, + DELETE_FAVORITE_SQL, + COPY_BYTE_SQL, + COPY_TAG_COUNT_SQL, + PENDING_COUNT_SQL, +) + + +def _file_link_rows(conn, spelled): + """``(id, prompt_id)`` of the image records linking one spelling.""" + rows = list(conn.execute(LINKS_BY_FILE_SQL, (normalize_image_path(spelled),))) + rows += list(conn.execute(LEGACY_LINKS_SQL, (spelled,))) + return [tuple(row) for row in rows] + + +def _copy_byte(conn, locator, sha256): + row = conn.execute(COPY_BYTE_SQL, (locator, sha256)).fetchone() + return row[0] if row else None + + +class DuplicateCopyOperations: + """PromptDatabase mixin: the records of duplicate copies.""" + + def count_copy_records(self, copies) -> dict: + """``{path: {links, tags, review}}`` for each copy (one read snapshot). + + ``copies`` maps each path to ``{spellings, locator, sha256}``: + ``links`` counts the image records under any spelling, ``tags`` the + image tags of its bytes and ``review`` its pending, undecided Needs + review items. + """ + from .maintenance import _read_snapshot + + def read(conn): + found = {} + for path, copy in copies.items(): + ids = { + row[0] + for spelled in copy["spellings"] + for row in _file_link_rows(conn, spelled) + } + byte_id = _copy_byte(conn, copy["locator"], copy["sha256"]) + tags = ( + conn.execute(COPY_TAG_COUNT_SQL, (byte_id,)).fetchone()[0] + if byte_id + else 0 + ) + review = conn.execute(PENDING_COUNT_SQL, (copy["locator"],)) + found[path] = { + "links": len(ids), + "tags": tags, + "review": review.fetchone()[0], + } + return found + + return _read_snapshot(self.model.db_path, read) + + def move_copy_records(self, move) -> dict: + """Move one removed copy's records to its kept copy (one transaction). + + ``move``: ``removed`` and ``kept`` (the spellings each file is linked + by), ``targets`` (``{removed spelling: kept spelling}``, the spelling + a moved record takes; else ``kept_path``), ``favorite`` (``(root key, + removed key, kept root key, kept key)`` or None), ``removed_locator``, + ``kept_locator``, ``sha256`` and the kept copy's ``kept_size`` and + ``kept_mtime_ns``. Returns ``{moved, merged, favorites, tags, + review}``. + """ + counts = dict.fromkeys(("moved", "merged", "favorites", "tags", "review"), 0) + kept_keys = [normalize_image_path(path) for path in move["kept"]] + with self._pair_transaction() as conn: + for spelled in dict.fromkeys(move["removed"]): + target = move["targets"].get(spelled, move["kept_path"]) + keys = list(dict.fromkeys(kept_keys + [normalize_image_path(target)])) + for image_id, prompt_id in _file_link_rows(conn, spelled): + existing = None + for key in keys: + existing = conn.execute( + KEEPER_LINK_SQL, (prompt_id, key) + ).fetchone() + if existing is not None: + break + if existing is not None: + conn.execute(MOVE_PAIR_LINKS_SQL, (existing[0], image_id)) + conn.execute(DELETE_LINK_SQL, (image_id,)) + counts["merged"] += 1 + continue + conn.execute( + MOVE_LINK_SQL, + ( + target, + os.path.basename(target), + normalize_image_path(target), + image_id, + ), + ) + counts["moved"] += 1 + if move.get("favorite"): + counts["favorites"] = _move_favorite(conn, *move["favorite"]) + counts["tags"] = self._move_copy_tags(conn, move) + for (item_id,) in conn.execute( + _PENDING_OF_LOCATOR, (move["removed_locator"],) + ).fetchall(): + counts["review"] += conn.execute(_SETTLE_ITEM, (item_id,)).rowcount + if counts["review"]: + conn.execute(_BUMP_REVIEW_INDEX) + if counts["review"]: + notify_review_change(self.model.db_path) + return counts + + def _move_copy_tags(self, conn, move): + """Join the removed copy's image tags onto the kept copy's; the count added.""" + removed = _copy_byte(conn, move["removed_locator"], move["sha256"]) + if removed is None: + return 0 + tags = self._byte_tags(conn, removed) + if not tags: + return 0 + now = utc_now_iso() + file_id = self._annotation_owner(conn, move["kept_locator"], now) + byte_id, _revision, created, moved = self._annotation_byte( + conn, file_id, move["sha256"], move["kept_size"], move["kept_mtime_ns"], now + ) + current = self._byte_tags(conn, byte_id) + union = current + [tag for tag in tags if tag not in current] + if union != current: + self._write_image_tags(conn, byte_id, union) + elif created or moved: + conn.execute(_BUMP_INDEX) + return len(union) - len(current) + + +def _move_favorite(conn, root_key, removed_key, kept_root, kept_key): + row = conn.execute(FAVORITE_SQL, (root_key, removed_key)).fetchone() + if row is None: + return 0 + conn.execute(INSERT_FAVORITE_SQL, (kept_root, kept_key, row[0])) + conn.execute(DELETE_FAVORITE_SQL, (root_key, removed_key)) + return 1 diff --git a/database/maintenance.py b/database/maintenance.py index 1b88a0ba..7b0c19c1 100644 --- a/database/maintenance.py +++ b/database/maintenance.py @@ -41,7 +41,6 @@ from pathlib import Path from .execution_contexts import LIVE_HINT_CTES from .health import BATCH_ROWS, link_states, readable_roots, readonly_uri -from .models import normalize_image_path from .pairs import PairDependencyError MAINTENANCE_OPERATIONS = ( @@ -142,10 +141,33 @@ LINKS_ONLY = "Database links only; no image file is deleted." UNUSED_DETAIL = "No image, saved pair, Library state or recorded run uses it" +# Why a duplicate group is kept whole (``_duplicate_conflict``). +CONFLICT_REASONS = { + "collision": ("group links one image twice", "groups link one image twice"), + "title": ( + "group has its own Library title", + "groups have their own Library titles", + ), +} +CONFLICT_DETAILS = { + "collision": "Kept: two copies link the same image", + "title": "Kept: a copy has its own Library title", +} +OPTIMIZE_LOCK_NOTE = ( + "Runs alone: the database is locked while it runs, and on a large library " + "this takes a while. Prompt runs counted and images captured meanwhile " + "wait up to 30 seconds each; anything that waits longer is not recorded." +) + + class PlanChanged(Exception): """What a plan was reviewed on changed before it was applied.""" +class MergeConflict(Exception): + """A strict duplicate merge would drop an image link or a Library title.""" + + @dataclass class MaintenanceDraft: """A preview: the public plan fields plus what only the server keeps.""" @@ -300,9 +322,44 @@ class MaintenanceOperations: groups.append(sorted_ids) return groups, paired - def _merge_duplicate_group(self, conn, sorted_ids): - """Keep the oldest prompt of a group; returns (removed, images moved).""" + @staticmethod + def _duplicate_conflict(conn, sorted_ids): + """Why merging the group would lose something, or None. + + ``collision``: two members link the same file, so moving the images + to the oldest prompt would hit ``UNIQUE(prompt_id, file_path)``. + ``title``: a member that would be deleted has its own Library title. + """ + marks = _marks(sorted_ids) + collision = conn.execute( + "SELECT 1 FROM generated_images WHERE prompt_id IN " # nosec B608 + f"({marks}) AND file_path IS NOT NULL GROUP BY file_path " + "HAVING COUNT(DISTINCT prompt_id) > 1 LIMIT 1", + sorted_ids, + ).fetchone() + if collision: + return "collision" + duplicates = sorted_ids[1:] + titled = conn.execute( + "SELECT 1 FROM prompt_library_state WHERE prompt_id IN " # nosec B608 + f"({_marks(duplicates)}) AND trim(coalesce(title, '')) != '' LIMIT 1", + duplicates, + ).fetchone() + return "title" if titled else None + + def _merge_duplicate_group(self, conn, sorted_ids, strict=False): + """Keep the oldest prompt of a group; returns (removed, images moved). + + ``strict`` (the reviewed apply): a merge that would drop an image + link or a Library title raises MergeConflict, and every helper error + propagates, so the transaction rolls back. The legacy cleanup keeps + its old, lenient behaviour. + """ primary_id, duplicate_ids = sorted_ids[0], sorted_ids[1:] + if strict: + conflict = self._duplicate_conflict(conn, sorted_ids) + if conflict: + raise MergeConflict(conflict) self.logger.debug( f"Merging duplicates: keeping {primary_id}, removing {duplicate_ids}" ) @@ -312,11 +369,15 @@ class MaintenanceOperations: if not primary: return 0, 0 merged_metadata = self._merge_duplicate_metadata( - conn, primary_id, duplicate_ids + conn, primary_id, duplicate_ids, strict=strict + ) + moved = self._transfer_images_to_primary( + conn, primary_id, duplicate_ids, strict=strict ) - moved = self._transfer_images_to_primary(conn, primary_id, duplicate_ids) if merged_metadata: - self._update_primary_with_merged_metadata(conn, primary_id, merged_metadata) + self._update_primary_with_merged_metadata( + conn, primary_id, merged_metadata, strict=strict + ) removed = 0 for duplicate_id in duplicate_ids: conn.execute("DELETE FROM prompts WHERE id = ?", (duplicate_id,)) @@ -366,29 +427,53 @@ class MaintenanceOperations: return rows def _duplicates_fingerprint(self, conn): - groups, paired = self._duplicate_groups(conn) + """``(groups, paired, conflicted, fingerprint)`` of the duplicate merge. + + ``conflicted`` holds ``(ids, reason)`` of the groups a merge would + lose an image link or a Library title in (``_duplicate_conflict``); + the plan protects them. + """ + candidates, paired = self._duplicate_groups(conn) + groups, conflicted = [], [] + for ids in candidates: + reason = self._duplicate_conflict(conn, ids) + if reason: + conflicted.append((ids, reason)) + else: + groups.append(ids) fingerprint = _Fingerprint("duplicate_prompts") - fingerprint.add({"groups": groups, "paired": paired}) + fingerprint.add({"groups": groups, "paired": paired, "conflicted": conflicted}) _add_prompt_state(fingerprint, conn, [i for group in groups for i in group]) - return groups, paired, fingerprint.hexdigest() + return groups, paired, conflicted, fingerprint.hexdigest() def _plan_duplicate_prompts(self, conn): - groups, paired, fingerprint = self._duplicates_fingerprint(conn) + groups, paired, conflicted, fingerprint = self._duplicates_fingerprint(conn) removed = sum(len(group) - 1 for group in groups) - texts = _texts(conn, [group[0] for group in (groups + paired)]) - details = [ - { - "label": _excerpt(texts.get(group[0])), - "detail": f"Keeps the oldest of {len(group)} copies", - } - for group in groups - ] + [ - { - "label": _excerpt(texts.get(group[0])), - "detail": "Kept: belongs to a saved pair", - } - for group in paired - ] + held = [ids for ids, _reason in conflicted] + texts = _texts(conn, [group[0] for group in (groups + paired + held)]) + details = ( + [ + { + "label": _excerpt(texts.get(group[0])), + "detail": f"Keeps the oldest of {len(group)} copies", + } + for group in groups + ] + + [ + { + "label": _excerpt(texts.get(group[0])), + "detail": "Kept: belongs to a saved pair", + } + for group in paired + ] + + [ + { + "label": _excerpt(texts.get(ids[0])), + "detail": CONFLICT_DETAILS[reason], + } + for ids, reason in conflicted + ] + ) if groups: label = ( f"{_plural(removed, 'duplicate prompt')} in " @@ -401,13 +486,17 @@ class MaintenanceOperations: reasons.append("1 group belongs to a saved pair") elif paired: reasons.append(f"{len(paired):,} groups belong to saved pairs") + for reason, (one, many) in CONFLICT_REASONS.items(): + count = sum(1 for _ids, why in conflicted if why == reason) + if count: + reasons.append(f"{count:,} {one if count == 1 else many}") return MaintenanceDraft( operation="duplicate_prompts", fingerprint=fingerprint, targets=groups, scope={"count": removed, "label": label}, protected={ - "count": sum(len(group) for group in paired), + "count": sum(len(group) for group in paired + held), "reasons": reasons, }, media={"deletes_files": False, "label": NO_FILES_DELETED}, @@ -543,10 +632,15 @@ class MaintenanceOperations: "detail": f"About {_megabytes(free_pages * page_size)} " "can be reclaimed", }, + { + "label": "Lock", + "detail": OPTIMIZE_LOCK_NOTE, + }, { "label": "Free space needed", - "detail": f"About {_megabytes(size)} in the database folder; " - f"{_megabytes(free)} available", + "detail": f"About {_megabytes(size)} beside the database and " + f"{_megabytes(size)} in SQLite's temporary folder; " + f"{_megabytes(free)} available beside the database", }, ], ) @@ -579,12 +673,12 @@ class MaintenanceOperations: return appliers[operation](conn, fingerprint, targets, roots) def _apply_duplicate_prompts(self, conn, fingerprint, targets, _roots): - groups, _paired, current = self._duplicates_fingerprint(conn) + groups, _paired, _conflicted, current = self._duplicates_fingerprint(conn) if current != fingerprint or groups != targets: raise PlanChanged("duplicate_prompts") changed, rows = 0, [] for sorted_ids in groups: - removed, _moved = self._merge_duplicate_group(conn, sorted_ids) + removed, _moved = self._merge_duplicate_group(conn, sorted_ids, strict=True) changed += removed rows.extend( {"prompt_id": duplicate_id, "kept_id": sorted_ids[0]} @@ -628,107 +722,3 @@ class MaintenanceOperations: "changed": changed, "rows": [{"image_path": row[1]} for row in rows[:MAX_RESULT_ROWS]], } - - # ── duplicate image files (py/api/duplicate_files.py) ──────────── - - def count_file_links(self, file_paths) -> dict: - """``{path: n}``: the image records linking each file (one snapshot). - - A record links a file by its normalized ``file_path``, or, in rows - that predate that column, by ``image_path``. - """ - - def read(conn): - return { - path: sum( - 1 - for _row in _file_link_rows(conn, normalize_image_path(path), path) - ) - for path in file_paths - } - - return _read_snapshot(self.model.db_path, read) - - def move_file_links(self, moves, favorites=()) -> dict: - """Point the links of removed duplicate files at their kept copy. - - ``moves`` is ``[(removed path, kept path)]`` for files that were - removed; nothing else is touched. Each image record of a removed file - moves to the kept copy, unless its prompt already links the kept copy: - then its saved-pair links join that record and it is deleted. - ``favorites`` is ``[(root key, removed key, kept root key, kept key)]``: - a removed file's favorite moves to the kept copy. One ``BEGIN - IMMEDIATE`` transaction. Returns ``{moved, merged, favorites}``. - """ - counts = {"moved": 0, "merged": 0, "favorites": 0} - with self._pair_transaction() as conn: - for removed, kept in moves: - kept_key = normalize_image_path(kept) - rows = list( - _file_link_rows(conn, normalize_image_path(removed), removed) - ) - for image_id, prompt_id in rows: - existing = conn.execute(KEEPER_LINK_SQL, (prompt_id, kept_key)) - existing = existing.fetchone() - if existing is not None: - conn.execute(MOVE_PAIR_LINKS_SQL, (existing[0], image_id)) - conn.execute(DELETE_LINK_SQL, (image_id,)) - counts["merged"] += 1 - continue - conn.execute( - MOVE_LINK_SQL, - (kept, os.path.basename(kept), kept_key, image_id), - ) - counts["moved"] += 1 - for root_key, removed_key, kept_root, kept_key in favorites: - row = conn.execute(FAVORITE_SQL, (root_key, removed_key)).fetchone() - if row is None: - continue - conn.execute(INSERT_FAVORITE_SQL, (kept_root, kept_key, row[0])) - conn.execute(DELETE_FAVORITE_SQL, (root_key, removed_key)) - counts["favorites"] += 1 - return counts - - -# Link reconciliation of removed duplicate files: every statement is an -# index SEARCH (idx_image_file_path, idx_image_path, the (prompt_id, -# file_path) unique index, idx_image_prompt_pairs_image, the favorites key). -LINKS_BY_FILE_SQL = "SELECT id, prompt_id FROM generated_images WHERE file_path = ?" -LEGACY_LINKS_SQL = ( - "SELECT id, prompt_id FROM generated_images " - "WHERE image_path = ? AND file_path IS NULL" -) -KEEPER_LINK_SQL = ( - "SELECT id FROM generated_images WHERE prompt_id = ? AND file_path = ?" -) -MOVE_PAIR_LINKS_SQL = ( - "UPDATE OR IGNORE image_prompt_pairs SET image_id = ? WHERE image_id = ?" -) -MOVE_LINK_SQL = ( - "UPDATE generated_images SET image_path = ?, filename = ?, file_path = ? " - "WHERE id = ?" -) -DELETE_LINK_SQL = "DELETE FROM generated_images WHERE id = ?" -FAVORITE_SQL = ( - "SELECT created_at FROM image_favorites WHERE root_key = ? AND relative_path = ?" -) -INSERT_FAVORITE_SQL = "INSERT OR IGNORE INTO image_favorites VALUES (?, ?, ?)" -DELETE_FAVORITE_SQL = ( - "DELETE FROM image_favorites WHERE root_key = ? AND relative_path = ?" -) -FILE_LINK_SQL = ( - LINKS_BY_FILE_SQL, - LEGACY_LINKS_SQL, - KEEPER_LINK_SQL, - MOVE_PAIR_LINKS_SQL, - MOVE_LINK_SQL, - DELETE_LINK_SQL, - FAVORITE_SQL, - DELETE_FAVORITE_SQL, -) - - -def _file_link_rows(conn, file_key, raw_path): - """``(id, prompt_id)`` of the image records linking one file.""" - yield from (tuple(row) for row in conn.execute(LINKS_BY_FILE_SQL, (file_key,))) - yield from (tuple(row) for row in conn.execute(LEGACY_LINKS_SQL, (raw_path,))) diff --git a/database/operations.py b/database/operations.py index 6eaa557a..54a4a799 100644 --- a/database/operations.py +++ b/database/operations.py @@ -24,6 +24,7 @@ from .execution_contexts import ExecutionContextOperations from .execution_outputs import ExecutionOutputOperations from .history_reads import HistoryReadOperations from .health import HealthOperations, iter_consistency_issues +from .duplicate_copies import DuplicateCopyOperations from .maintenance import ( # noqa: F401 (ORPHANED_PROMPTS_SQL is re-exported) ORPHANED_PROMPTS_SQL, _LINK_ROWS_SQL, @@ -337,6 +338,7 @@ class PromptDatabase( SavedViewOperations, HealthOperations, MaintenanceOperations, + DuplicateCopyOperations, ): """Database operations class for managing prompts.""" @@ -1860,7 +1862,11 @@ class PromptDatabase( return obj def _merge_duplicate_metadata( - self, conn: sqlite3.Connection, primary_id: int, duplicate_ids: List[int] + self, + conn: sqlite3.Connection, + primary_id: int, + duplicate_ids: List[int], + strict: bool = False, ) -> Dict[str, Any]: """ Merge metadata from duplicate prompts, prioritizing non-empty values. @@ -1920,11 +1926,17 @@ class PromptDatabase( return merged except Exception as e: + if strict: # a reviewed apply rolls back instead + raise self.logger.error(f"Error merging metadata: {e}", exc_info=True) return {} def _transfer_images_to_primary( - self, conn: sqlite3.Connection, primary_id: int, duplicate_ids: List[int] + self, + conn: sqlite3.Connection, + primary_id: int, + duplicate_ids: List[int], + strict: bool = False, ) -> int: """ Transfer all images from duplicate prompts to the primary prompt. @@ -1956,11 +1968,17 @@ class PromptDatabase( return transferred_count except Exception as e: + if strict: # a reviewed apply rolls back instead + raise self.logger.error(f"Error transferring images: {e}", exc_info=True) return 0 def _update_primary_with_merged_metadata( - self, conn: sqlite3.Connection, primary_id: int, merged_metadata: Dict[str, Any] + self, + conn: sqlite3.Connection, + primary_id: int, + merged_metadata: Dict[str, Any], + strict: bool = False, ) -> None: """ Update the primary prompt with merged metadata. @@ -1991,6 +2009,8 @@ class PromptDatabase( ) except Exception as e: + if strict: # a reviewed apply rolls back instead + raise self.logger.error( f"Error updating primary prompt metadata: {e}", exc_info=True ) diff --git a/py/api/__init__.py b/py/api/__init__.py index 67e6ef82..bc4ac049 100644 --- a/py/api/__init__.py +++ b/py/api/__init__.py @@ -360,6 +360,16 @@ class PromptManagerAPI( # forgets them (py/api/maintenance.py). self._maintenance_plans = PlanStore() + # A safety backup interrupted by a crash leaves its temp copy behind. + try: + from .maintenance import sweep_safety_temps + + swept = sweep_safety_temps(self.db.model.db_path) + if swept: + self.logger.info(f"Removed {len(swept)} interrupted backup copies") + except Exception as e: + self.logger.warning(f"Could not sweep interrupted backup copies: {e}") + # Run cleanup on initialization to remove any existing duplicates try: removed = self.db.cleanup_duplicates() diff --git a/py/api/admin.py b/py/api/admin.py index 7e24e575..425ad279 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -135,6 +135,17 @@ def _collect_media_files(output_dirs, extensions): return found +def _physical_file_key(path, stat): + """What tells two physical files apart: ``(st_dev, st_ino)``. + + A filesystem that reports no inode (``st_ino`` 0, some network or FAT + volumes on Windows) falls back to the resolved, case-folded path. + """ + if stat.st_ino: + return ("inode", stat.st_dev, stat.st_ino) + return ("path", os.path.normcase(os.path.realpath(str(path)))) + + def _thumbnail_rel_path(rel_path, thumbnail_ext): """Relative path of the thumbnail the gallery would generate for ``rel_path``.""" return f"thumbnails/{rel_path.with_suffix('').as_posix()}_thumb{thumbnail_ext}" @@ -743,13 +754,18 @@ class AdminRoutesMixin: """Content-identical media files under ``roots`` (blocking). Each root is walked as ``_collect_media_files`` walks (no symlinks, - no ``thumbnails``, bounded); a file is listed once, under the first - root that holds it, and at most MAX_SCAN_FILES files are read. Files + no ``thumbnails``, bounded). A physical file is listed once, under + the first root that holds it: files are told apart by ``(st_dev, + st_ino)``, so a source listed twice under two spellings (a symlink, a + bind mount, a case variant), a source inside another one or a + hardlink is one file, never a copy of itself. At most MAX_SCAN_FILES + files are read. Files are hashed on ``PromptManagerConfig.WORKER_THREADS`` threads; ``progress(done, total)`` is called from those threads about every percent and once at the end, and ``stop()`` true skips the files not yet read. Returns ``[{hash, size, files:[{slot, abs, rel, size, - mtime}]}]`` for every hash shared by two or more files, ``slot`` + mtime, mtime_ns, inode}]}]`` for every hash shared by two or more + files (``inode`` is ``[st_dev, st_ino]``), ``slot`` being the root's place in ``roots`` and the files oldest first. """ from ..config import PromptManagerConfig @@ -759,10 +775,16 @@ class AdminRoutesMixin: seen = set() for slot, root in enumerate(roots): for path in _collect_media_files([root], extensions): - key = os.path.normcase(str(path)) + try: + stat = path.stat() + except OSError: + continue + key = _physical_file_key(path, stat) if key not in seen: seen.add(key) - media_files.append((slot, root, path)) + media_files.append((slot, root, path, stat)) + if len(media_files) >= MAX_SCAN_FILES: + break media_files = media_files[:MAX_SCAN_FILES] total = len(media_files) self.logger.info(f"Found {total} media files to analyze") @@ -771,17 +793,18 @@ class AdminRoutesMixin: lock = threading.Lock() def describe(item): - slot, root, media_path = item + slot, root, media_path, stat = item info = None if stop is None or not stop(): try: - stat = media_path.stat() info = { "slot": slot, "abs": str(media_path), "rel": Path(os.path.relpath(str(media_path), root)).as_posix(), "size": stat.st_size, "mtime": stat.st_mtime, + "mtime_ns": stat.st_mtime_ns, + "inode": [stat.st_dev, stat.st_ino], "hash": self._calculate_file_hash(media_path), } except Exception as e: diff --git a/py/api/duplicate_files.py b/py/api/duplicate_files.py index dd4a471f..4cdd85d9 100644 --- a/py/api/duplicate_files.py +++ b/py/api/duplicate_files.py @@ -57,11 +57,12 @@ from .duplicate_review import ( # noqa: F401 (CHANGED, OUTSIDE_SOURCE: public) KEEPER, KEEPER_CHANGED, KEPT_COPY, - LINKS_FAILED, LOCKED_FILE, NOT_ATTEMPTED, OUTSIDE_SOURCE, + RECORDS_FAILED, ROW_DETAILS, + SAME_FILE, SOURCES_CHANGED, SOURCES_CHANGED_REVIEW, HeldFind, @@ -88,9 +89,9 @@ from .jobs import ( from .maintenance import DUPLICATE_FILES, _fail_job, _failure, _invalid, backup_note try: - from ...database.maintenance import FILE_LINK_SQL + from ...database.duplicate_copies import FILE_LINK_SQL except ImportError: - from database.maintenance import FILE_LINK_SQL + from database.duplicate_copies import FILE_LINK_SQL DUPLICATE_FIND_TITLE = "Find duplicate image files" DUPLICATE_CLEANUP_TITLE = "Remove duplicate image files" @@ -432,10 +433,14 @@ class DuplicateFilesMixin: ) def _review_duplicate_files(self, db, selection): - """Check every keeper and removal now (blocking); the plan's parts.""" + """Check every keeper and removal now (blocking); the plan's parts. + + No file is read: each copy is bound to what the search saw by its + size, mtime and inode, and never to its kept copy itself. The removal + job hashes each file again right before it unlinks it. + """ listed = self._listed_sources() targets, kept, protected = [], [], {} - hasher = self._calculate_file_hash for group, keepers in selection.values(): removals = [r for r in group["files"] if r not in keepers] for record in removals: @@ -444,15 +449,7 @@ class DuplicateFilesMixin: ): raise SourcesChanged(record["root"]) keeper = next( - ( - k - for k in keepers - if file_problem( - k["abs"], k["root"], group["hash"], k["size"], hasher - ) - is None - ), - None, + (k for k in keepers if _bound_problem(k, group) is None), None ) reviewed = [] for record in removals: @@ -461,13 +458,7 @@ class DuplicateFilesMixin: elif record["root_index"] is None: problem = OUTSIDE_SOURCE else: - problem = file_problem( - record["abs"], - record["root"], - group["hash"], - record["size"], - hasher, - ) + problem = _bound_problem(record, group, keeper["abs"]) if problem: protected[problem] = protected.get(problem, 0) + 1 continue @@ -475,14 +466,55 @@ class DuplicateFilesMixin: if reviewed: targets.extend(reviewed) kept.append(self._copy_row(keeper, KEPT_COPY)) - links = db.count_file_links([t["abs"] for t in targets]) if targets else {} + records = {} + if targets: + spellings = self._source_spellings() + records = db.count_copy_records( + { + t["abs"]: { + "spellings": _file_spellings(spellings, t["root"], t["rel"]), + "locator": t["locator"], + "sha256": t["hash"], + } + for t in targets + } + ) return { "targets": targets, "kept": kept, "fingerprint": _fingerprint(targets), - "public": _public_plan(targets, protected, links), + "public": _public_plan(targets, protected, records), } + def _source_spellings(self): + """``{source identity: [spellings]}`` the monitor and config use (blocking). + + The search walks each source's real path, while the image monitor + links a file under the folder it watches: the configured spelling, + ComfyUI's output folder or a symlink to it. Each absolute candidate + is listed under the identity (``normcase(realpath)``) it resolves to. + """ + from ..config import GalleryConfig + + candidates = list(self._monitored_directories()) + candidates += list(GalleryConfig.MONITORING_DIRECTORIES) + try: + import folder_paths + + candidates.append(folder_paths.get_output_directory()) + except Exception: # nosec B110 - outside ComfyUI there is none + pass + candidates.append(self._find_comfyui_output_dir()) + spellings = {} + for candidate in candidates: + if not candidate or not os.path.isabs(str(candidate)): + continue + spelled = os.path.abspath(str(candidate)) + listed = spellings.setdefault(_identity(spelled), []) + if spelled not in listed: + listed.append(spelled) + return spellings + def _target(self, group, record, keeper): return { "group_id": group["group_id"], @@ -497,6 +529,8 @@ class DuplicateFilesMixin: "keeper_rel": keeper["rel"], "keeper_root": keeper["root"], "keeper_size": keeper["size"], + "locator": os.path.normcase(record["abs"]), + "keeper_locator": os.path.normcase(keeper["abs"]), } def _copy_row(self, record, detail): @@ -509,7 +543,7 @@ class DuplicateFilesMixin: # ── removal ─────────────────────────────────────────────────────── - def _start_duplicate_cleanup(self, db, plan): + def _start_duplicate_cleanup(self, db, plan, backup_on): """Register the removal job for a reviewed plan (on the loop).""" find = self._jobs.active(DUPLICATES_JOB_KIND) if find is not None: @@ -521,7 +555,7 @@ class DuplicateFilesMixin: cancellable=False, scope={"operation": DUPLICATE_FILES}, groups=DUPLICATE_CLEANUP_GROUP_NAMES, - runner=lambda job: self._run_duplicate_cleanup(job, db, plan), + runner=lambda job: self._run_duplicate_cleanup(job, db, plan, backup_on), ) def _sources_unchanged(self, targets): @@ -530,12 +564,12 @@ class DuplicateFilesMixin: self._source_listed(listed, t["root_index"], t["root"]) for t in targets ) - async def _run_duplicate_cleanup(self, job, db, plan): + async def _run_duplicate_cleanup(self, job, db, plan, backup_on): targets = plan.payload["targets"] unchanged = await self._run_in_executor(self._sources_unchanged, targets) if not unchanged: return _fail_job(job, "sources-changed", SOURCES_CHANGED) - ok, backup = await self._safety_backup_step(job, db, plan) + ok, backup = await self._safety_backup_step(job, db, backup_on) if not ok: return try: @@ -549,36 +583,60 @@ class DuplicateFilesMixin: for row in plan.payload["kept"]: job.record("kept", row, f"= {row['root']}/{row['path']} · {KEPT_COPY}") job.update(total=len(targets), status_line="Removing duplicate files") - removed, failure, keepers = [], None, {} + spellings = await self._run_in_executor(self._source_spellings) + removed, failure, keepers = 0, None, {} + totals = dict.fromkeys(("moved", "merged", "favorites", "tags", "review"), 0) for done, target in enumerate(targets, start=1): if failure is not None: self._record_target(job, "kept", target, NOT_ATTEMPTED) continue - outcome, code = await self._run_in_executor( - self._remove_one_duplicate, target, keepers + outcome, detail, counts = await self._run_in_executor( + self._remove_one_duplicate, db, target, keepers, spellings ) + for key, value in (counts or {}).items(): + totals[key] += value if outcome == "removed": - removed.append(target) - detail = f"A copy of {target['keeper_rel']}" - self._record_target(job, "removed", target, detail, "-") + removed += 1 + text = f"A copy of {target['keeper_rel']}" + self._record_target(job, "removed", target, text, "-") elif outcome == "kept": - self._record_target(job, "kept", target, ROW_DETAILS[code]) + self._record_target(job, "kept", target, ROW_DETAILS[detail]) else: - failure = code - self._record_target(job, "errors", target, code, "!") + failure = detail + self._record_target(job, "errors", target, detail, "!") job.update(processed=done, progress=int(done / len(targets) * 100)) - links_ok = await self._move_links(job, db, removed) if removed: self.invalidate_gallery_cache() - line = f"Removed {_plural(len(removed), 'duplicate file')}" + self._log_moved_records(job, totals) + line = f"Removed {_plural(removed, 'duplicate file')}" + if removed < len(targets): + line += f", {len(targets) - removed:,} not removed" job.log_line(line) - if failure is not None or not links_ok: - line = "Stopped after an error · " + line if failure else line - job.update(progress=100, status_line=f"{line} · {backup_note(backup)}") + if failure is not None: + line = f"Stopped after an error · {line}" + job.update(progress=100, status_line=f"{line} · {backup_note(backup)}") + if failure is not None: job.publish({"type": "error", "code": "delete-failed", "message": line}) job.finish("failed") - return - job.update(progress=100, status_line=f"{line} · {backup_note(backup)}") + + @staticmethod + def _log_moved_records(job, totals): + links = totals["moved"] + totals["merged"] + if links: + job.log_line(f"Moved {_plural(links, 'database link')} to the kept copies") + if totals["favorites"]: + job.log_line( + f"Moved {_plural(totals['favorites'], 'favorite')} to the kept copies" + ) + if totals["tags"]: + job.log_line( + f"Added {_plural(totals['tags'], 'image tag')} to the kept copies" + ) + if totals["review"]: + job.log_line( + f"Settled {_plural(totals['review'], 'Needs review item')} " + "of removed copies" + ) @staticmethod def _record_target(job, group, target, detail, mark="="): @@ -592,73 +650,126 @@ class DuplicateFilesMixin: group, row, f"{mark} {target['root_label']}/{target['rel']} · {detail}" ) - def _remove_one_duplicate(self, target, keepers): - """``(outcome, detail or code)`` for one reviewed removal (blocking). + def _remove_one_duplicate(self, db, target, keepers, spellings): + """``(outcome, detail, counts)`` for one reviewed removal (blocking). - ``keepers`` caches each group's kept-copy check for this job. + The kept copy (once per group; cached in ``keepers``) and the file + are checked again, hashes included, and the file must not be its kept + copy. Then the file's records move to the kept copy in one short + transaction, and only then is the file unlinked: a crash or a failed + unlink leaves its records on the identical kept copy, never on a + deleted file. A failed move leaves the file and its records alone. """ hasher = self._calculate_file_hash group_id = target["group_id"] if group_id not in keepers: - keepers[group_id] = ( - file_problem( - target["keeper_abs"], - target["keeper_root"], - target["hash"], - target["keeper_size"], - hasher, - ) - is None - ) - if not keepers[group_id]: - return "kept", KEEPER + keepers[group_id] = _kept_copy_state(target, hasher) + if keepers[group_id] is None: + return "kept", KEEPER, None problem = file_problem( - target["abs"], target["root"], target["hash"], target["size"], hasher + target["abs"], + target["root"], + target["hash"], + target["size"], + hasher, + keeper=target["keeper_abs"], ) if problem: - return "kept", problem + return "kept", problem, None + try: + counts = db.move_copy_records( + self._copy_move(target, keepers[group_id], spellings) + ) + except Exception: + self.logger.exception(f"Could not move the records of {target['rel']}") + return "error", RECORDS_FAILED, None try: _unlink(target["abs"]) except FileNotFoundError: - return "kept", GONE + return "removed", None, counts except PermissionError: - self.logger.warning(f"Duplicate file is locked: {target['rel']}") - return "error", LOCKED_FILE + self.logger.warning( + f"Duplicate file is locked: {target['rel']}; " + "its records now name the kept copy" + ) + return "error", LOCKED_FILE, counts except OSError: self.logger.warning( - f"Could not delete duplicate file {target['rel']}", exc_info=True + f"Could not delete duplicate file {target['rel']}; " + "its records now name the kept copy", + exc_info=True, ) - return "error", DELETE_FAILED + return "error", DELETE_FAILED, counts self.logger.info(f"Deleted duplicate file: {target['rel']}") _remove_thumbnail(Path(target["root"]), Path(target["rel"]), self.logger) - return "removed", None + return "removed", None, counts - async def _move_links(self, job, db, removed): - """Move the removed files' links to their kept copy; False on failure.""" - if not removed: - return True - moves = [(t["abs"], t["keeper_abs"]) for t in removed] - favorites = [ - ( - _favorite_root_key(t["root"]), - _favorite_relative_key(t["rel"]), - _favorite_root_key(t["keeper_root"]), - _favorite_relative_key(t["keeper_rel"]), + @staticmethod + def _copy_move(target, kept_stat, spellings): + """What ``move_copy_records`` moves for one removal.""" + removed = _file_spellings(spellings, target["root"], target["rel"]) + kept = _file_spellings(spellings, target["keeper_root"], target["keeper_rel"]) + same_source = _identity(target["root"]) == _identity(target["keeper_root"]) + native_rel = target["rel"].replace("/", os.sep) + native_kept = target["keeper_rel"].replace("/", os.sep) + targets = {} + for spelled in removed: + prefix = spelled[: len(spelled) - len(native_rel)] + targets[spelled] = ( + prefix + native_kept if same_source else target["keeper_abs"] ) - for t in removed - ] - try: - counts = await self._run_in_executor(db.move_file_links, moves, favorites) - except Exception: - self.logger.exception("Moving duplicate file links failed") - job.record( - "errors", {"path": "", "detail": LINKS_FAILED}, f"! {LINKS_FAILED}" - ) - return False - links = counts["moved"] + counts["merged"] - job.log_line(f"Moved {_plural(links, 'database link')} to the kept copies") - if counts["favorites"]: - job.log_line( - f"Moved {_plural(counts['favorites'], 'favorite')} to the kept copies" - ) - return True + return { + "removed": removed, + "kept": kept, + "targets": targets, + "kept_path": target["keeper_abs"], + "favorite": ( + _favorite_root_key(target["root"]), + _favorite_relative_key(target["rel"]), + _favorite_root_key(target["keeper_root"]), + _favorite_relative_key(target["keeper_rel"]), + ), + "removed_locator": target["locator"], + "kept_locator": target["keeper_locator"], + "sha256": target["hash"], + "kept_size": kept_stat.st_size, + "kept_mtime_ns": kept_stat.st_mtime_ns, + } + + +def _bound_problem(record, group, keeper=None): + """``file_problem`` without reading the file: size, mtime and inode.""" + return file_problem( + record["abs"], + record["root"], + group["hash"], + record["size"], + hash_check=False, + mtime_ns=record.get("mtime_ns"), + inode=record.get("inode"), + keeper=keeper, + ) + + +def _kept_copy_state(target, hasher): + """The kept copy's ``os.stat`` when it is still the reviewed bytes, else None.""" + problem = file_problem( + target["keeper_abs"], + target["keeper_root"], + target["hash"], + target["keeper_size"], + hasher, + ) + if problem: + return None + try: + return os.stat(target["keeper_abs"]) + except OSError: + return None + + +def _file_spellings(spellings, root, rel): + """Every path one file is linked by: its real path, then each spelling.""" + native = rel.replace("/", os.sep) + roots = [root] + spellings.get(_identity(root), []) + return list(dict.fromkeys(os.path.join(spelled, native) for spelled in roots)) diff --git a/py/api/duplicate_review.py b/py/api/duplicate_review.py index d7c70862..ea71e105 100644 --- a/py/api/duplicate_review.py +++ b/py/api/duplicate_review.py @@ -16,7 +16,9 @@ from pathlib import Path from .admin import DELETABLE_MEDIA_EXTENSIONS from .maintenance import DUPLICATE_FILES -MAX_KEEP = 5000 # keep entries one review takes +# Keep entries one review takes; with paths of a few hundred characters the +# list stays inside the preview body cap (DUPLICATE_PREVIEW_BODY_MAX, 1 MiB). +MAX_KEEP = 2000 MAX_DETAILS = 50 MAX_JOB_ID = 64 HASH_CHUNK = 1024 * 1024 @@ -28,7 +30,12 @@ CHANGED_CONTENT = "changed" GONE = "gone" UNREADABLE = "unreadable" KEEPER = "keeper" +SAME = "same" PROTECTED_REASONS = { + SAME: ( + "file is the same file as its kept copy", + "files are the same file as their kept copy", + ), OUTSIDE_SOURCE: ( "file is not inside a configured source", "files are not inside a configured source", @@ -48,7 +55,9 @@ PROTECTED_REASONS = { # Job outcome rows (never an OS error text). CHANGED = "The file changed since the review; it was kept." KEEPER_CHANGED = "The kept copy changed or is gone; this file was kept." +SAME_FILE = "This is the same file as its kept copy; it was kept." ROW_DETAILS = { + SAME: SAME_FILE, OUTSIDE_SOURCE: "The file is no longer inside its source; it was kept.", LINK: "The file is now a link, not a regular file; it was kept.", CHANGED_CONTENT: CHANGED, @@ -63,9 +72,9 @@ KEPT_COPY = "Kept copy" SOURCES_CHANGED = ( "The sources changed since the review; nothing was deleted. Review again." ) -LINKS_FAILED = ( - "The files were removed, but their database links could not be moved. " - "Review Missing-file links to tidy them." +RECORDS_FAILED = ( + "The database could not be updated, so the file was not deleted. " + "Its links and tags are unchanged." ) FIND_FAILED = "The duplicate search failed." FIND_ERROR_EVENT = "An internal error occurred. Check server logs for details." @@ -99,15 +108,30 @@ def _hash_file(path): return digest.hexdigest() -def file_problem(path, root, digest, size, hasher=None): +def file_problem( + path, + root, + digest, + size, + hasher=None, + *, + hash_check=True, + mtime_ns=None, + inode=None, + keeper=None, +): """Why ``path`` may not be treated as a copy in ``root``, or None (blocking). Containment is decided as text first, with no filesystem access: the normalized path must lie below the normalized root, outside ``thumbnails``, with a media extension. Only then is the file looked at: a regular file, not a link, whose resolved path is still inside the - resolved root, of ``size`` bytes and with content hash ``digest``. Every - handle is closed when this returns. + resolved root and that is not ``keeper`` itself (``os.path.samestat``: a + hardlink, or one file reached through two spellings), of ``size`` bytes. + ``mtime_ns`` and ``inode`` (``[st_dev, st_ino]``), when given, bind it to + what the search saw; ``hash_check`` compares its content hash with + ``digest`` (the removal job, right before an unlink). Every handle is + closed when this returns. """ # py.config imports ComfyUI's server module: only when it is in use. from ..config import _canonical_path, _lexical_path, _lexically_inside @@ -134,9 +158,15 @@ def file_problem(path, root, digest, size, hasher=None): try: if not _lexically_inside(_canonical_path(path), [_canonical_path(root)]): return OUTSIDE_SOURCE # a linked folder on the way + if keeper is not None and os.path.samestat(info, os.stat(keeper)): + return SAME if info.st_size != size: return CHANGED_CONTENT - if (hasher or _hash_file)(path) != digest: + if mtime_ns is not None and info.st_mtime_ns != mtime_ns: + return CHANGED_CONTENT + if inode is not None and [info.st_dev, info.st_ino] != list(inode): + return CHANGED_CONTENT + if hash_check and (hasher or _hash_file)(path) != digest: return CHANGED_CONTENT except FileNotFoundError: return GONE @@ -197,7 +227,7 @@ def _parse_options(options): if not isinstance(job_id, str) or not 0 < len(job_id) <= MAX_JOB_ID: return None, None, "job_id must be a duplicate search's job id" if not isinstance(keep, list) or not 0 < len(keep) <= MAX_KEEP: - return None, None, f"keep must list 1 to {MAX_KEEP} files to keep" + return None, None, f"keep must list 1 to {MAX_KEEP:,} files to keep" parsed = [] for entry in keep: key, message = _keep_entry(entry) @@ -246,7 +276,29 @@ def _fingerprint(targets): return "sha256:" + digest.hexdigest() -def _public_plan(targets, protected, links): +def _record_parts(records, target): + """What moves with one removal: its links, tags and Needs review items.""" + found = records.get(target["abs"], {}) + links, tags, review = (found.get(key, 0) for key in ("links", "tags", "review")) + parts = [ + ( + ( + f"{_plural(links, 'database link')} " + f"{'moves' if links == 1 else 'move'} to the kept copy" + ) + if links + else "no database links" + ) + ] + if tags: + parts.append("its image tags join the kept copy's") + if review: + item = "Needs review item is" if review == 1 else "Needs review items are" + parts.append(f"{review:,} {item} settled") + return parts + + +def _public_plan(targets, protected, records): count = len(targets) groups = len({t["group_id"] for t in targets}) size = sum(t["size"] for t in targets) @@ -254,32 +306,35 @@ def _public_plan(targets, protected, links): f"{n:,} {PROTECTED_REASONS[code][0 if n == 1 else 1]}" for code, n in sorted(protected.items()) ] - details = [] - for target in targets[:MAX_DETAILS]: - n = links.get(target["abs"], 0) - moves = ( - f"{_plural(n, 'database link')} " - f"{'moves' if n == 1 else 'move'} to the kept copy" - if n - else "no database links" - ) - details.append( - { - "label": target["rel"], - "detail": f"{target['root_label']} · a copy of {target['keeper_rel']}" - f" · {moves}", - } - ) + details = [ + { + "label": target["rel"], + "detail": f"{target['root_label']} · a copy of {target['keeper_rel']}" + + "".join(f" · {part}" for part in _record_parts(records, target)), + } + for target in targets[:MAX_DETAILS] + ] label = f"{_plural(count, 'duplicate file')} in {_plural(groups, 'group')}" + media = ( + f"Deletes {_plural(count, 'file')} ({human_bytes(size)}) from disk; the " + "kept copies stay. The safety backup covers the database, not these files." + ) + tagged = sum(1 for t in targets if records.get(t["abs"], {}).get("tags")) + review = sum(records.get(t["abs"], {}).get("review", 0) for t in targets) + if tagged: + media += ( + f" The image tags of {_plural(tagged, 'removed copy', 'removed copies')}" + " join their kept copy's." + ) + if review: + media += ( + f" {_plural(review, 'pending Needs review item')} of the removed " + f"{'copy is' if review == 1 else 'copies are'} settled." + ) return { "operation": DUPLICATE_FILES, "scope": {"count": count, "label": label}, "protected": {"count": sum(protected.values()), "reasons": reasons}, - "media": { - "deletes_files": True, - "label": f"Deletes {_plural(count, 'file')} ({human_bytes(size)}) from " - "disk; the kept copies stay. The safety backup covers the database, " - "not these files.", - }, + "media": {"deletes_files": True, "label": media}, "details": details, } diff --git a/py/api/jobs.py b/py/api/jobs.py index 53cbadf9..eace0b96 100644 --- a/py/api/jobs.py +++ b/py/api/jobs.py @@ -19,7 +19,11 @@ ones. One job of a kind runs at a time, and each job has an admission class - ``guarded`` must not overlap record changes but may overlap other guarded work (thumbnail generation and clearing, later backup). It is refused while a ``records`` job runs. -- ``read`` only reads; it is refused only by a running job of its own kind. +- ``read`` only reads; it is refused only by a running job of its own kind + or an ``exclusive`` one. +- ``exclusive`` runs alone (Optimize's VACUUM, which rewrites the whole + database under the write lock): it is refused while any job runs, and every + job is refused while it runs. A short legacy-page operation takes a ``hold``: a running, non-cancellable job that lasts as long as the request, so the Activity page lists it and a @@ -75,12 +79,14 @@ IMAGE_TAG_GROUP_NAMES = ("tagged", "unchanged", "skipped", "errors") # Thumbnail generation (#249 System S5): previews written, already fresh, failed. THUMBNAIL_GROUP_NAMES = ("generated", "skipped", "errors") -ADMISSIONS = ("read", "guarded", "records") +ADMISSIONS = ("read", "guarded", "records", "exclusive") # What each admission class is refused beside (besides its own kind). +# ``exclusive`` (Optimize's VACUUM) runs alone: beside no job of any class. _REFUSED_BESIDE = { - "read": frozenset(), - "guarded": frozenset({"records"}), - "records": frozenset({"records", "guarded"}), + "read": frozenset({"exclusive"}), + "guarded": frozenset({"records", "exclusive"}), + "records": frozenset({"records", "guarded", "exclusive"}), + "exclusive": frozenset(ADMISSIONS), } # Kinds of the legacy-page operations that hold the registry (D80; titles in diff --git a/py/api/maintenance.py b/py/api/maintenance.py index 38c8802d..62ac5853 100644 --- a/py/api/maintenance.py +++ b/py/api/maintenance.py @@ -66,7 +66,7 @@ except ImportError: PREVIEW_BODY_MAX = 4096 # Duplicate-file previews carry the reviewed keepers (py/api/duplicate_files.py). -DUPLICATE_PREVIEW_BODY_MAX = 512 * 1024 +DUPLICATE_PREVIEW_BODY_MAX = 1024 * 1024 DUPLICATE_FILES = "duplicate_files" PREVIEW_OPERATIONS = MAINTENANCE_OPERATIONS + (DUPLICATE_FILES,) # Room left over beyond what a step writes, so a disk is never filled to 0. @@ -112,6 +112,36 @@ def _device(path): return path +def sqlite_temp_dir(environ=None, *, windows=None, usable=None): + """The folder SQLite builds its temporary files in (VACUUM's copy). + + Mirrors SQLite's own search, which differs from ``tempfile``'s. On Unix + (``unixTempFileDir``): ``SQLITE_TMPDIR``, ``TMPDIR``, then ``/var/tmp``, + ``/usr/tmp`` and ``/tmp``, the first that is a writable directory, else + the current directory. On Windows (``GetTempPathW``): ``TMP``, ``TEMP``, + then ``USERPROFILE``, else the current directory. + """ + environ = os.environ if environ is None else environ + if windows is None: + windows = os.name == "nt" + if windows: + for name in ("TMP", "TEMP", "USERPROFILE"): + if environ.get(name): + return environ[name] + return os.path.abspath(".") + if usable is None: + + def usable(path): + return os.path.isdir(path) and os.access(path, os.W_OK | os.X_OK) + + candidates = [environ.get("SQLITE_TMPDIR"), environ.get("TMPDIR")] + candidates += ["/var/tmp", "/usr/tmp", "/tmp"] + for candidate in candidates: + if candidate and usable(candidate): + return candidate + return os.path.abspath(".") + + def database_bytes(db_path): """The size a safety backup of the database will be (blocking). @@ -135,8 +165,9 @@ def disk_plan(db_path, *, backup, vacuum): ``backup``: the safety backup, one database-sized file beside the database. ``vacuum``: VACUUM writes the rebuilt database through the WAL - beside it and builds it in a temporary copy in the temporary folder - (SQLite's documentation: up to twice the database in free space). Needs + beside it and builds it in a temporary copy in SQLite's temporary folder + (``sqlite_temp_dir``; SQLite's documentation: up to twice the database + in free space). Needs on one device add up. Returns ``(public, short)``: ``public`` is ``{free_bytes, required_bytes, sufficient}`` for the database folder, ``short`` None or ``(place, what, required, free)`` of the first place @@ -151,7 +182,7 @@ def disk_plan(db_path, *, backup, vacuum): needs.append(("database folder", folder, size, "the safety backup")) if vacuum: needs.append(("database folder", folder, size, "Optimize")) - needs.append(("temporary folder", tempfile.gettempdir(), size, "Optimize")) + needs.append(("temporary folder", sqlite_temp_dir(), size, "Optimize")) devices = {} for place, path, needed, what in needs: entry = devices.setdefault(_device(path), {"place": place, "path": path}) @@ -210,9 +241,7 @@ def _remove_quietly(*paths): def _reserve_backup_name(db_path): """Create and return an unused ``.backup_[_n]`` (O_EXCL).""" - base = f"{db_path}.backup_{_now().strftime('%Y%m%d_%H%M%S_%f')}" - for attempt in range(_NAME_ATTEMPTS): - candidate = base if attempt == 0 else f"{base}_{attempt}" + for candidate in _backup_names(db_path): try: fd = os.open(candidate, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) except FileExistsError: @@ -222,6 +251,66 @@ def _reserve_backup_name(db_path): raise SafetyBackupFailed("No free backup name") +def _backup_names(db_path): + base = f"{db_path}.backup_{_now().strftime('%Y%m%d_%H%M%S_%f')}" + for attempt in range(_NAME_ATTEMPTS): + yield base if attempt == 0 else f"{base}_{attempt}" + + +def _place_backup(temp, db_path): + """Give the verified ``temp`` its unique backup name; return that path. + + ``os.link`` is atomic and refuses an existing name, so no empty or half + file ever carries a backup name, even across a crash. A filesystem + without hard links falls back to reserving the name (``O_EXCL``) and + ``os.replace``. The temp file is removed by the caller. + """ + for candidate in _backup_names(db_path): + try: + os.link(temp, candidate) + except FileExistsError: + continue + except (OSError, NotImplementedError): + break # no hard links here + else: + return candidate + else: + raise SafetyBackupFailed("No free backup name") + final = _reserve_backup_name(db_path) + try: + os.replace(temp, final) + except OSError: + _remove_quietly(final) + raise + return final + + +SAFETY_TEMP_PREFIX = ".pm-safety-" + + +def sweep_safety_temps(db_path): + """Remove the temp copies an interrupted safety backup left (blocking). + + Called once at startup, before any job can take a backup. Returns the + names removed. + """ + folder = os.path.dirname(os.path.abspath(db_path)) + removed = [] + try: + names = os.listdir(folder) + except OSError: + return removed + for name in names: + if not (name.startswith(SAFETY_TEMP_PREFIX) and ".tmp" in name): + continue + try: + os.remove(os.path.join(folder, name)) + removed.append(name) + except OSError: + continue + return removed + + def _standalone(path): """Make the copy one self-contained file (rollback journal, not WAL). @@ -250,7 +339,7 @@ def take_safety_backup(model): """ db_path = os.path.abspath(model.db_path) fd, temp = tempfile.mkstemp( - dir=os.path.dirname(db_path), prefix=".pm-safety-", suffix=".tmp" + dir=os.path.dirname(db_path), prefix=SAFETY_TEMP_PREFIX, suffix=".tmp" ) os.close(fd) final = None @@ -264,8 +353,7 @@ def take_safety_backup(model): _fsync(temp) if os.name != "nt": os.chmod(temp, 0o600) - final = _reserve_backup_name(db_path) - os.replace(temp, final) + final = _place_backup(temp, db_path) return os.path.basename(final) except OSError as exc: _remove_quietly(final) @@ -445,21 +533,10 @@ class MaintenanceRoutesMixin: token = data.get("token") if not isinstance(token, str) or not 0 < len(token) <= MAX_TOKEN_LENGTH: return _invalid("token must be the plan's token") + plan, refused = self._peek_plan(token) + if refused is not None: + return refused store = self._maintenance_plans - try: - plan = store.peek(token) - except PlanUsed as used: - job = self._jobs.get(used.job_id) - return _failure( - "This plan was already applied.", - 409, - "plan-used", - job={"id": used.job_id, "title": job.title if job else None}, - ) - except PlanExpired: - return _failure( - "This review expired. Review the plan again.", 410, "plan-expired" - ) db = self.db if plan.db_identity != self._database_identity(db): store.discard(token) @@ -482,36 +559,69 @@ class MaintenanceRoutesMixin: if short is not None: # Refused before any job starts: nothing fails halfway for space. return _failure(short_space_message(short), 507, "insufficient-space") + # The token may have been used, expired or dropped while the disk was + # measured: look again with no await before the job starts. + plan, refused = self._peek_plan(token) + if refused is not None: + return refused try: if plan.operation == DUPLICATE_FILES: - job = self._start_duplicate_cleanup(db, plan) + job = self._start_duplicate_cleanup(db, plan, backup) else: job = self._jobs.start( MAINTENANCE_JOB_KIND, MAINTENANCE_TITLES[plan.operation], - admission="records", + # VACUUM rewrites the whole file under the write lock: it + # runs alone, beside no other job of any class. + admission=( + "exclusive" if plan.operation == "optimize" else "records" + ), cancellable=False, scope={"operation": plan.operation}, groups=MAINTENANCE_GROUPS, - runner=lambda job: self._run_maintenance_plan(job, db, plan), + runner=lambda job: self._run_maintenance_plan( + job, db, plan, backup + ), ) except JobBusy as busy: return busy_response(busy) # the token stays usable - # Same loop turn as the start: the runner has not run yet. + # Same loop turn as the peek and the start: the runner has not run. store.consume(token, plan.operation, job_id=job.id) return web.json_response({"success": True, "job": job.summary()}, status=202) + def _peek_plan(self, token): + """``(plan, None)`` for a usable token, else ``(None, refusal)``.""" + try: + return self._maintenance_plans.peek(token), None + except PlanUsed as used: + job = self._jobs.get(used.job_id) + return None, _failure( + "This plan was already applied.", + 409, + "plan-used", + job={"id": used.job_id, "title": job.title if job else None}, + ) + except PlanExpired: + return None, _failure( + "This review expired. Review the plan again.", 410, "plan-expired" + ) + def _backup_promised(self, plan): - """A plan reviewed with the policy on keeps that promise; so does on now.""" + """A plan reviewed with the policy on keeps that promise; so does on now. + + Decided once, when the apply starts (the disk check measured it), and + handed to the job. + """ return plan.payload["backup"] == "on" or self._safety_backup_policy() == "on" - async def _safety_backup_step(self, job, db, plan): + async def _safety_backup_step(self, job, db, backup_on): """``(ok, name)``: take the promised safety backup, or report none. - ``name`` is the backup's file name, or None when the policy is off. - A failed backup ends the job ``backup-failed`` (``ok`` False). + ``name`` is the backup's file name, or None when no backup was + promised. A failed backup ends the job ``backup-failed`` (``ok`` + False). """ - if not self._backup_promised(plan): + if not backup_on: job.log_line("No safety backup (the policy is off)") return True, None job.update(status_line="Taking a safety backup") @@ -531,8 +641,8 @@ class MaintenanceRoutesMixin: if removed: job.log_line(f"Removed {len(removed)} older safety backups") - async def _run_maintenance_plan(self, job, db, plan): - ok, backup = await self._safety_backup_step(job, db, plan) + async def _run_maintenance_plan(self, job, db, plan, backup_on): + ok, backup = await self._safety_backup_step(job, db, backup_on) if not ok: return try: diff --git a/tests/test_api_duplicate_files.py b/tests/test_api_duplicate_files.py index a9899fa1..f8caffe7 100644 --- a/tests/test_api_duplicate_files.py +++ b/tests/test_api_duplicate_files.py @@ -126,6 +126,14 @@ class DuplicateFilesTestCase(ApplyTestCase): await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) return plan, cleanup + async def until(self, predicate): + """Wait for a server-side state (a frame at a time), never a fixed timer.""" + deadline = asyncio.get_running_loop().time() + WAIT + while not predicate(): + if asyncio.get_running_loop().time() > deadline: + self.fail("the expected server state never appeared") + await asyncio.sleep(0) + def gate_hashing(self): gate = threading.Event() calls = [] @@ -159,7 +167,8 @@ class TestFindJob(DuplicateFilesTestCase): self.assertEqual(summary["counters"], {"groups_found": 0}) # The legacy gallery attaches to the same job, and its status says so. legacy = asyncio.ensure_future(self.client.request("POST", STREAM)) - await asyncio.sleep(0.1) + job = self.api._jobs.get(summary["id"]) + await self.until(lambda: job.subscriber_count > 0) status = await (await self.client.request("GET", STATUS)).json() self.assertTrue(status["running"]) gate.set() @@ -220,7 +229,8 @@ class TestFindJob(DuplicateFilesTestCase): response = await self.client.request("POST", FIND) job_id = (await response.json())["job"]["id"] stream = asyncio.ensure_future(self.client.request("POST", STREAM)) - await asyncio.sleep(0.1) + found = self.api._jobs.get(job_id) + await self.until(lambda: found.subscriber_count > 0) cancel = await self.client.request( "POST", f"/prompt_manager/jobs/{job_id}/cancel" ) @@ -512,7 +522,11 @@ class TestCleanupJob(DuplicateFilesTestCase): self.assertEqual(cleanup.counters["removed"], 0) self.assertTrue((self.output_dir / "b.png").is_file()) self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) - self.assertTrue(cleanup.status_line.startswith("Removed 0 duplicate files · ")) + self.assertTrue( + cleanup.status_line.startswith( + "Removed 0 duplicate files, 2 not removed · " + ) + ) rows = {row["path"]: row["detail"] for row in cleanup.outcomes["kept"]} self.assertEqual(rows["b.png"], duplicate_files.KEEPER_CHANGED) @@ -544,8 +558,8 @@ class TestCleanupJob(DuplicateFilesTestCase): ) self.assertEqual( cleanup.status_line, - "Stopped after an error · Removed 1 duplicate file · Safety backup " - + self.backups()[0], + "Stopped after an error · Removed 1 duplicate file, 1 not removed · " + "Safety backup " + self.backups()[0], ) report = json.dumps(cleanup.report()) self.assertNotIn("Permission denied", report) @@ -660,7 +674,7 @@ class TestCleanupJob(DuplicateFilesTestCase): "Moved 2 database links to the kept copies", "\n".join(cleanup.log) ) - async def test_a_failed_removal_keeps_its_links(self): + async def test_a_failed_removal_leaves_its_links_on_the_kept_copy(self): self.trio() prompt = self.save("Stays on c") self.api.db.link_image_to_prompt(prompt, str(self.output_dir / "sub" / "c.png")) @@ -677,8 +691,10 @@ class TestCleanupJob(DuplicateFilesTestCase): body = await self.apply(plan["token"]) cleanup = self.api._jobs.get(body["job"]["id"]) await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + # Its records moved to the identical kept copy before the unlink failed. [image] = self.api.db.get_prompt_images(prompt) - self.assertTrue(image["image_path"].endswith("c.png")) + self.assertTrue(image["image_path"].endswith("a.png")) + self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) async def test_cleanup_and_find_exclude_each_other(self): self.trio() @@ -772,7 +788,7 @@ class TestLegacyDuplicateRoutes(DuplicateFilesTestCase): legacy = asyncio.ensure_future( self.client.request("GET", "/prompt_manager/scan_duplicates") ) - await asyncio.sleep(0.1) + await self.until(lambda: self.api._jobs.active("duplicates") is not None) [hold] = self.api._jobs.list() self.assertEqual( (hold.kind, hold.title, hold.admission, hold.cancellable), @@ -942,14 +958,14 @@ class TestReviewHelpers(DuplicateFilesTestCase): self.assertEqual(self.backups(), []) self.assertTrue((self.output_dir / "b.png").is_file()) - async def test_a_link_move_failure_is_reported(self): + async def test_a_records_move_failure_deletes_nothing(self): self.trio() job, group = await self.found_group() plan = await self.review(job.id, [self.keep_of(group, "a.png")]) with ( patch.object( self.api.db, - "move_file_links", + "move_copy_records", side_effect=sqlite3.OperationalError("x"), ), self.assertLogs("prompt_manager", "ERROR"), @@ -959,6 +975,8 @@ class TestReviewHelpers(DuplicateFilesTestCase): await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) self.assertEqual(cleanup.state, "failed") self.assertEqual( - cleanup.outcomes["errors"][-1]["detail"], duplicate_files.LINKS_FAILED + cleanup.outcomes["errors"][-1]["detail"], duplicate_files.RECORDS_FAILED ) - self.assertEqual(cleanup.counters["removed"], 2) + self.assertEqual(cleanup.counters["removed"], 0) + self.assertTrue((self.output_dir / "b.png").is_file()) + self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) diff --git a/tests/test_api_duplicate_safety.py b/tests/test_api_duplicate_safety.py new file mode 100644 index 00000000..37f88f1a --- /dev/null +++ b/tests/test_api_duplicate_safety.py @@ -0,0 +1,275 @@ +"""Duplicate-file removal safety (#249 System S6 final review: C1, I1, I2, I5, M9). + +- C1: one physical file reached through two listed sources, a symlinked + source or a hardlink is never offered or removed as a copy of itself. +- I1: each file's records move to its verified kept copy before that file + is unlinked, so no database row ever names a deleted file. +- I2: links stored under another spelling of the source move too. +- I5: image tags move to the kept copy (their union) and the removed copy's + pending Needs review items are settled. +- M9: the preview binds files by size, mtime and inode without hashing. +""" + +import asyncio +import hashlib +import os +import sqlite3 +from pathlib import Path +from unittest.mock import patch + +try: + from tests import test_api_duplicate_files as dup_tests +except ImportError: # discovered with tests/ as the top-level directory + import test_api_duplicate_files as dup_tests + +from py.api import duplicate_files # noqa: E402 +from py.config import GalleryConfig # noqa: E402 + +WAIT = 10 + + +def _locator(path): + return os.path.normcase(os.path.realpath(str(path))) + + +class SafetyTestCase(dup_tests.DuplicateFilesTestCase): + def need_symlinks(self): + if not dup_tests.TestReview._can_symlink(self.tmpdir): # pragma: no cover + self.skipTest("symlinks are not available") + + def linked_files(self): + """Every image record's path that names a file that does not exist.""" + with self.api.db.model.get_connection() as conn: + rows = conn.execute("SELECT image_path FROM generated_images").fetchall() + return [row[0] for row in rows if not os.path.exists(row[0])] + + async def apply_plan(self, plan): + body = await self.apply(plan["token"]) + cleanup = self.api._jobs.get(body["job"]["id"]) + await asyncio.wait_for(asyncio.shield(cleanup.task), WAIT) + return cleanup + + +class TestOnePhysicalFile(SafetyTestCase): + """C1: the kept copy survives every alias of itself.""" + + async def test_a_folder_listed_twice_never_loses_its_only_copy(self): + self.need_symlinks() + only = self.write("a.png", b"the only copy") + alias = Path(self.tmpdir) / "Output-alias" + os.symlink(str(self.output_dir), str(alias)) + with patch.object( + self.api, "_get_all_output_dirs", return_value=[self.output_dir, alias] + ): + job = await self.find() + page = await self.groups(job.id) + self.assertEqual(page["total"], 0) + self.assertEqual(only.read_bytes(), b"the only copy") + + async def test_a_symlinked_source_inside_another_is_one_file(self): + self.need_symlinks() + only = self.write("sub/c.png", b"one file") + linked = self.output_dir / "linked" # walked by neither as a link + os.symlink(str(self.output_dir / "sub"), str(linked)) + GalleryConfig.MONITORING_DIRECTORIES = [ + self._root(self.output_dir), + str(linked), + ] + self.assertEqual(len(self.api._get_all_output_dirs()), 2) + job = await self.find() + self.assertEqual((await self.groups(job.id))["total"], 0) + self.assertTrue(only.is_file()) + + async def test_a_hardlink_is_never_a_copy_to_remove(self): + first = self.write("a.png", b"same bytes", mtime=1) + os.link(str(first), str(self.output_dir / "b.png")) + self.write("c.png", b"same bytes", mtime=3) + job, group = await self.found_group() + self.assertEqual(len(group["files"]), 2) # a.png and b.png are one file + # c.png becomes a hardlink of the keeper after the find: protected. + (self.output_dir / "c.png").unlink() + os.link(str(first), str(self.output_dir / "c.png")) + kept = group["files"][0]["path"] + plan = await self.review(job.id, [self.keep_of(group, kept)]) + self.assertEqual(plan["scope"]["count"], 0) + self.assertEqual( + plan["protected"]["reasons"], ["1 file is the same file as its kept copy"] + ) + + async def test_the_job_refuses_a_file_that_became_its_kept_copy(self): + first = self.write("a.png", b"same bytes", mtime=1) + self.write("b.png", b"same bytes", mtime=2) + job, group = await self.found_group() + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + (self.output_dir / "b.png").unlink() + os.link(str(first), str(self.output_dir / "b.png")) + cleanup = await self.apply_plan(plan) + self.assertEqual(cleanup.counters["removed"], 0) + rows = {row["path"]: row["detail"] for row in cleanup.outcomes["kept"]} + self.assertEqual(rows["b.png"], duplicate_files.SAME_FILE) + self.assertTrue((self.output_dir / "b.png").is_file()) + + +class TestRecordsMoveBeforeTheUnlink(SafetyTestCase): + """I1: a deleted file is never named by a database row.""" + + async def test_a_failure_between_two_files_leaves_no_row_on_a_deleted_file(self): + self.write("a.png", b"same", mtime=1) + self.write("b.png", b"same", mtime=2) + self.write("c.png", b"same", mtime=3) + prompt = self.save("Linked to every copy") + for name in ("b.png", "c.png"): + self.api.db.link_image_to_prompt(prompt, str(self.output_dir / name)) + job, group = await self.found_group() + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + real = self.api.db.move_copy_records + calls = [] + + def move(*args, **kwargs): + calls.append(args) + if len(calls) == 2: + raise sqlite3.OperationalError("database is locked") + return real(*args, **kwargs) + + with patch.object(self.api.db, "move_copy_records", side_effect=move): + with self.assertLogs("prompt_manager", "ERROR"): + cleanup = await self.apply_plan(plan) + self.assertEqual(self.linked_files(), []) + self.assertEqual(cleanup.counters["removed"], 1) + self.assertEqual(cleanup.state, "failed") + [error] = cleanup.outcomes["errors"] + self.assertEqual(error["detail"], duplicate_files.RECORDS_FAILED) + self.assertNotIn("Missing-file", cleanup.status_line) + self.assertEqual( + cleanup.status_line.split(" · ")[:2], + ["Stopped after an error", "Removed 1 duplicate file, 1 not removed"], + ) + + async def test_a_failed_unlink_leaves_its_links_on_the_kept_copy(self): + self.trio() + prompt = self.save("On c") + self.api.db.link_image_to_prompt(prompt, str(self.output_dir / "sub" / "c.png")) + job, group = await self.found_group() + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + real = duplicate_files._unlink + + def unlink(path): + if os.path.basename(path) == "c.png": + raise PermissionError(13, "denied") + real(path) + + with patch.object(duplicate_files, "_unlink", side_effect=unlink): + with self.assertLogs("prompt_manager", "WARNING"): + cleanup = await self.apply_plan(plan) + [image] = self.api.db.get_prompt_images(prompt) + self.assertEqual(os.path.basename(image["image_path"]), "a.png") + self.assertTrue((self.output_dir / "sub" / "c.png").is_file()) + self.assertEqual(self.linked_files(), []) + self.assertEqual( + cleanup.outcomes["errors"][0]["detail"], duplicate_files.LOCKED_FILE + ) + + +class TestEverySpelling(SafetyTestCase): + """I2: links under the monitor's spelling of a source move too.""" + + async def test_links_under_a_symlinked_source_spelling_move(self): + self.need_symlinks() + self.trio() + spelled = Path(self.comfy_dir) / "out-link" + os.symlink(str(self.output_dir), str(spelled)) + GalleryConfig.MONITORING_DIRECTORIES = [str(spelled)] + prompt = self.save("Linked through the symlink") + self.api.db.link_image_to_prompt(prompt, str(spelled / "b.png")) + job, group = await self.found_group() + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + self.assertIn("1 database link moves", plan["details"][0]["detail"]) + await self.apply_plan(plan) + [image] = self.api.db.get_prompt_images(prompt) + self.assertEqual( + _locator(image["image_path"]), _locator(self.output_dir / "a.png") + ) + self.assertEqual(self.linked_files(), []) + + +class TestTagsAndReview(SafetyTestCase): + """I5: tags join the kept copy; pending review items of the copy settle.""" + + def tag(self, path, tags): + data = Path(path).read_bytes() + info = os.stat(path) + return self.api.db.replace_image_tags( + locator=_locator(path), + sha256=hashlib.sha256(data).hexdigest(), + size=info.st_size, + mtime_ns=info.st_mtime_ns, + expected_epoch=self.api.db.get_execution_epoch(), + expected_tag_revision=0, + tags=tags, + ) + + def tags_of(self, path): + data = Path(path).read_bytes() + return self.api.db.read_image_tags( + _locator(path), hashlib.sha256(data).hexdigest() + )["tags"] + + def queue(self, path): + data = Path(path).read_bytes() + info = os.stat(path) + return self.api.db.record_review_item( + locator=_locator(path), + spelled_path=os.path.realpath(str(path)), + sha256=hashlib.sha256(data).hexdigest(), + size=info.st_size, + mtime_ns=info.st_mtime_ns, + epoch=self.api.db.get_execution_epoch(), + resolver_version=1, + evidence={"candidates": []}, + candidate_count=0, + limited=True, + ) + + def pending(self): + return self.api.db.review_queue_page(limit=50)["items"] + + async def test_tags_move_as_a_union_and_pending_items_settle(self): + self.write("a.png", b"same", mtime=1) + self.write("b.png", b"same", mtime=2) + keeper, removed = self.output_dir / "a.png", self.output_dir / "b.png" + self.tag(keeper, ["kept"]) + self.tag(removed, ["moved", "kept"]) + self.assertTrue(self.queue(removed)) + job, group = await self.found_group() + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + self.assertIn("image tags join the kept copy", plan["details"][0]["detail"]) + self.assertIn("1 Needs review item is settled", plan["details"][0]["detail"]) + await self.apply_plan(plan) + self.assertEqual(self.tags_of(keeper), ["kept", "moved"]) + self.assertEqual(self.pending(), []) + + +class TestPreviewDoesNotHash(SafetyTestCase): + """M9: the preview binds by size, mtime and inode; the job hashes.""" + + async def test_preview_reads_no_file_and_spots_a_touched_one(self): + self.trio() + job, group = await self.found_group() + calls = [] + real = self.api._calculate_file_hash + + def spy(path): + calls.append(path) + return real(path) + + self.api._calculate_file_hash = spy + os.utime(self.output_dir / "b.png", (5_000_000, 5_000_000)) + plan = await self.review(job.id, [self.keep_of(group, "a.png")]) + self.assertEqual(calls, []) + self.assertEqual(plan["scope"]["count"], 1) + self.assertEqual( + plan["protected"]["reasons"], ["1 file changed since the search"] + ) + cleanup = await self.apply_plan(plan) + self.assertEqual(cleanup.counters["removed"], 1) + self.assertGreaterEqual(len(calls), 2) # the job hashes keeper and file diff --git a/tests/test_api_maintenance.py b/tests/test_api_maintenance.py index bb416902..2066d2a7 100644 --- a/tests/test_api_maintenance.py +++ b/tests/test_api_maintenance.py @@ -588,11 +588,22 @@ class TestSafetyBackup(ApplyTestCase): async def test_a_failed_replace_leaves_no_file(self): from py.api import maintenance - with patch.object(maintenance.os, "replace", side_effect=OSError("busy")): + # A filesystem without hard links falls back to reserve-and-replace. + with ( + patch.object(maintenance.os, "link", side_effect=OSError("no hard links")), + patch.object(maintenance.os, "replace", side_effect=OSError("busy")), + ): with self.assertRaises(maintenance.SafetyBackupFailed): maintenance.take_safety_backup(self.api.db.model) self.assertEqual(self.backups(), []) self.assertEqual(self.strays(), []) + with patch.object(maintenance.os, "link", side_effect=FileExistsError): + with self.assertRaises(maintenance.SafetyBackupFailed): + maintenance.take_safety_backup(self.api.db.model) + with patch.object(maintenance.os, "link", side_effect=OSError("no hard links")): + name = maintenance.take_safety_backup(self.api.db.model) + self.assertEqual(self.backups(), [name]) + self.assertEqual(self.strays(), []) async def test_a_backup_that_cannot_be_finished_leaves_no_file(self): from py.api import maintenance @@ -689,7 +700,11 @@ class TestBackupPoolAndDiskSpace(ApplyTestCase): from py.api import maintenance self.save("Orphan") - with patch.object(maintenance, "_free_bytes", return_value=10 * 2**30): + # SQLite's temporary folder on the database's disk: the needs add up. + with ( + patch.object(maintenance, "_free_bytes", return_value=10 * 2**30), + patch.object(maintenance, "sqlite_temp_dir", return_value=self.tmpdir), + ): plan = await self.preview("unused_components") optimize = await self.preview("optimize") size = self.database_bytes() @@ -846,3 +861,207 @@ if __name__ == "__main__": # pragma: no cover import unittest unittest.main() + + +class TestFinalReviewFixes(ApplyTestCase): + """PR #313 final review: I3, M1-M5.""" + + def duplicate_of(self, kept_text, copy_text): + kept = self.save(kept_text) + with self.api.db.model.get_connection() as conn: + copy = conn.execute( + "INSERT INTO prompts(text, created_at) VALUES(?, '2999-01-01')", + (copy_text,), + ).lastrowid + return kept, copy + + def test_sqlite_temp_dir_follows_sqlite_search_order(self): + from py.api.maintenance import sqlite_temp_dir + + def usable(*good): + return lambda path: path in good + + posix = {"windows": False} + self.assertEqual( + sqlite_temp_dir({}, usable=usable("/var/tmp", "/tmp"), **posix), "/var/tmp" + ) + self.assertEqual( + sqlite_temp_dir({}, usable=usable("/usr/tmp", "/tmp"), **posix), "/usr/tmp" + ) + env = {"SQLITE_TMPDIR": "/fast", "TMPDIR": "/t"} + self.assertEqual( + sqlite_temp_dir(env, usable=usable("/fast", "/t", "/tmp"), **posix), + "/fast", + ) + self.assertEqual( + sqlite_temp_dir(env, usable=usable("/t", "/tmp"), **posix), "/t" + ) + self.assertEqual( + sqlite_temp_dir({}, usable=usable(), **posix), os.path.abspath(".") + ) + windows = {"windows": True, "usable": usable()} + env = {"TMP": "C:\\a", "TEMP": "C:\\b", "USERPROFILE": "C:\\u"} + self.assertEqual(sqlite_temp_dir(env, **windows), "C:\\a") + self.assertEqual(sqlite_temp_dir({"TEMP": "C:\\b"}, **windows), "C:\\b") + self.assertEqual(sqlite_temp_dir({"USERPROFILE": "C:\\u"}, **windows), "C:\\u") + + async def test_optimize_measures_sqlites_temp_folder(self): + from py.api import maintenance + + measured = [] + + def free(path): + measured.append(path) + return 10 * 2**30 + + other = os.path.join(self.tmpdir, "sqlite-temp") + os.mkdir(other) + with ( + patch.object(maintenance, "sqlite_temp_dir", return_value=other), + patch.object(maintenance, "_free_bytes", side_effect=free), + patch.object( + maintenance, "_device", side_effect=lambda path: path # another disk + ), + ): + plan = await self.preview("optimize") + self.assertIn(other, measured) + size = self.database_bytes() + self.assertEqual( + plan["disk"]["required_bytes"], 2 * size + maintenance.DISK_MARGIN_BYTES + ) + + async def test_a_plan_used_during_the_disk_check_is_plan_used(self): + from py.api import maintenance + + self.save("Orphan") + plan = await self.preview("unused_components") + real = maintenance.disk_plan + + def racing(*args, **kwargs): + # A parallel apply consumed the token while this one measured. + self.api._maintenance_plans.consume( + plan["token"], "unused_components", job_id="f" * 32 + ) + return real(*args, **kwargs) + + with patch.object(maintenance, "disk_plan", side_effect=racing): + reply = await self.apply(plan["token"], 409) + self.assertEqual(reply["code"], "plan-used") + self.assertEqual(self.api._jobs.list(), []) + + async def test_the_backup_decision_is_frozen_when_the_job_starts(self): + PromptManagerConfig.SAFETY_BACKUP = False + self.save("Orphan") + plan = await self.preview("unused_components") + real = self.api._jobs.start + + def start(*args, **kwargs): + job = real(*args, **kwargs) + PromptManagerConfig.SAFETY_BACKUP = True # switched on meanwhile + return job + + with patch.object(self.api._jobs, "start", side_effect=start): + body = await self.apply(plan["token"]) + job = self.api._jobs.get(body["job"]["id"]) + await asyncio.wait_for(asyncio.shield(job.task), WAIT) + self.assertEqual(self.backups(), []) + self.assertTrue(job.status_line.endswith("No safety backup")) + + async def test_startup_sweeps_interrupted_backup_copies(self): + stray = os.path.join(self.tmpdir, ".pm-safety-crashed.tmp") + with open(stray, "wb") as handle: + handle.write(b"half a backup") + PromptManagerAPI(db=self.api.db) + self.assertFalse(os.path.exists(stray)) + + async def test_a_backup_is_linked_into_place_without_a_reserved_name(self): + from py.api import maintenance + + linked = [] + real = os.link + + def link(source, target): + linked.append(target) + return real(source, target) + + with ( + patch.object(maintenance.os, "link", side_effect=link), + patch.object( + maintenance, "_reserve_backup_name", side_effect=AssertionError + ), + ): + name = maintenance.take_safety_backup(self.api.db.model) + self.assertEqual([os.path.basename(p) for p in linked], [name]) + self.assertEqual(self.strays(), []) + + async def test_optimize_runs_alone_and_says_the_database_is_locked(self): + from py.api.jobs import JobBusy + + plan = await self.preview("optimize") + self.assertIn( + "Runs alone: the database is locked while it runs", + " ".join(row["detail"] for row in plan["details"]), + ) + gate = asyncio.Event() + + async def held(job): + await gate.wait() + + read = self.api._jobs.start( + "health", "Health check", admission="read", runner=held + ) + busy = await self.apply(plan["token"], 409) + self.assertEqual(busy["running"]["id"], read.id) + gate.set() + await asyncio.wait_for(asyncio.shield(read.task), WAIT) + gate.clear() + body = await self.apply(plan["token"]) + self.assertEqual(body["job"]["admission"], "exclusive") + with self.assertRaises(JobBusy): + self.api._jobs.start( + "health", "Health check", admission="read", runner=held + ) + job = self.api._jobs.get(body["job"]["id"]) + await asyncio.wait_for(asyncio.shield(job.task), WAIT) + + async def test_a_link_collision_is_protected_not_swallowed(self): + kept, copy = self.duplicate_of("A cat", "A CAT") + path = os.path.join(str(self.output_dir), "both.png") + self.api.db.link_image_to_prompt(kept, path) + self.api.db.link_image_to_prompt(copy, path) + plan, _summary, job = await self.run_apply("duplicate_prompts") + self.assertEqual(plan["scope"]["count"], 0) + self.assertEqual( + plan["protected"]["reasons"], ["1 group links one image twice"] + ) + self.assertIsNotNone(self.api.db.get_prompt_by_id(copy)) + self.assertEqual(len(self.api.db.get_prompt_images(copy)), 1) + + async def test_a_duplicate_with_a_library_title_is_protected(self): + kept, copy = self.duplicate_of("A dog", "A DOG") + with self.api.db.model.get_connection() as conn: + conn.execute( + "INSERT INTO prompt_library_state(prompt_id,title) VALUES(?,?)", + (copy, "My dog recipe"), + ) + plan, _summary, _job = await self.run_apply("duplicate_prompts") + self.assertEqual(plan["scope"]["count"], 0) + self.assertEqual( + plan["protected"]["reasons"], ["1 group has its own Library title"] + ) + self.assertIsNotNone(self.api.db.get_prompt_by_id(copy)) + + def test_the_strict_merge_raises_instead_of_dropping_a_link(self): + from database.maintenance import MergeConflict + + kept, copy = self.duplicate_of("A bird", "A BIRD") + path = os.path.join(str(self.output_dir), "bird.png") + self.api.db.link_image_to_prompt(kept, path) + self.api.db.link_image_to_prompt(copy, path) + with self.assertRaises(MergeConflict): + with self.api.db._pair_transaction() as conn: + self.api.db._merge_duplicate_group(conn, [kept, copy], strict=True) + self.assertEqual(len(self.api.db.get_prompt_images(copy)), 1) + # The legacy cleanup keeps its old behaviour (it logs and goes on). + with self.assertLogs("prompt_manager.database", "ERROR"): + self.assertEqual(self.api.db.cleanup_duplicates(), 1) diff --git a/tests/test_job_admission.py b/tests/test_job_admission.py index 59e4821e..0bfb11c4 100644 --- a/tests/test_job_admission.py +++ b/tests/test_job_admission.py @@ -63,7 +63,7 @@ class TestAdmissionClasses(unittest.IsolatedAsyncioTestCase): ) async def test_admission_names_and_explicit_read(self): - self.assertEqual(ADMISSIONS, ("read", "guarded", "records")) + self.assertEqual(ADMISSIONS, ("read", "guarded", "records", "exclusive")) registry = JobRegistry() job = registry.start( "report", "Report", admission="read", scope={}, runner=_noop @@ -72,7 +72,7 @@ class TestAdmissionClasses(unittest.IsolatedAsyncioTestCase): self.assertEqual(job.admission, "read") self.assertFalse(job.record_changing) with self.assertRaises(ValueError): - registry.start("x", "X", admission="exclusive", scope={}, runner=_noop) + registry.start("x", "X", admission="bogus", scope={}, runner=_noop) with self.assertRaises(ValueError): registry.start( "x", diff --git a/tests/test_maintenance_plans.py b/tests/test_maintenance_plans.py index 4d8533f7..a8cbbfb8 100644 --- a/tests/test_maintenance_plans.py +++ b/tests/test_maintenance_plans.py @@ -398,7 +398,7 @@ class TestPreviews(MaintenanceDatabaseCase): self.assertEqual(draft.scope, {"count": 1, "label": "The whole database"}) self.assertEqual( [row["label"] for row in draft.details], - ["Database file", "Free pages", "Free space needed"], + ["Database file", "Free pages", "Lock", "Free space needed"], ) self.assertNotIn(str(self.root), str(draft.public()))