fix: complete recoverable file cleanup (2B)

[codeacme17]

Problem and behavior

Refs #1086.

The task-less collector could unlink a local upload before winning its SQL claim. Registered compensation and recovery could delete the upload row after durable-object absence while preview or local cleanup was still unfinished, losing their recovery handle.

This implements 2B complete, recoverable cleanup on the existing compensation lifecycle. An exact claim commits before destructive I/O. A focused JSON manifest retains resource locators, configuration, ownership/replacement evidence, quarantine names, and durable/local/preview phase receipts. The upload row remains unavailable and recoverable until every required phase succeeds and exact-generation settlement commits. Already absent resources succeed; uncertain outcomes remain pending.

A separate persistent execution lock serializes takeover, storage work, settlement, managed-copy restoration, and owned preview publication. Reference locks and SQL connections are released before slow storage work. Sources require managed owner-root containment and regular-file/inode/checksum evidence; external/shared sources are preserved. Owned partial copies and converter temporaries are included. Replacements and escaping symlinks remain identifiable for reconciliation. Retired SQL identities and standalone fences survive upload-row removal.

Request cancellation and existing compensation errors remain primary. Request finalization preserves protected/rebound uploads, unfinished claims, and replacement inodes. Direct deletion receives only a narrow guard against erasing a compensating row.

Verification

• 288 focused tests passed, including real SQLite/PostgreSQL lifecycle and caller cases: independent overlapping workers and task binders, exact-token takeover/stale settlement, committed claims and restarts between phases, separate durable/local/preview/settlement faults, storage presence uncertainty, external/shared sources, escaping symlinks and replacements, production collector/compensation/recovery, cancellation and SQL pool release.
• 316 additional affected regressions passed, covering storage/scoping, upload registration, reference protection, orphan collection, detached-file lifecycle, KB and preview callers. Earlier schema validation covers fresh installation, populated SQLite/PostgreSQL upgrades, interruption/retry, partial-schema bootstrap and downgrade constraints.
• New production regressions prove lock-free materialized /preview responses, claimed/retired file handling in Chat/WebSocket/agent registration, KB event-loop responsiveness, snapshot row removal and NULL-generation changes, and staged-path cleanup after a transient ownership-query failure. Fourteen new regression cases fail on the exact reviewed parent 951aa7b4; the corrected head passes them.
• PPTX contention uses a real upload record/resolver, absent source/cache, an independently held execution guard, the exact DurableStorageOperationError -> filelock.Timeout cause chain, and proof that no converter subprocess starts.
• All configured pre-commit hooks passed, including mypy across 880 source files; focused mypy passed for nine changed source files, and Ruff/format/whitespace checks passed.
• Full-suite execution stops at three Alembic INFO-log assertions. The exact parent 951aa7b4 reproduces 3 failed, 298 passed in an independent worktree with the same environment. The two affected Alembic files pass in isolation (40 tests), establishing collection/logging-order sensitivity rather than a cleanup regression.

Standards/Spec review found no blocker; the only judgement call was non-blocking duplication between the explicit sync/async turn-file result loops. The user explicitly chose to skip this round's pr-preflight before pushing 951aa7b4..2b2018be.

Caller and publication contracts

Available local copies and fresh preview caches use existing read paths. Materialized cache reads validate scoped storage routing/checksum and current persistent row generation before and after probing, without taking the execution guard. Corrupt cache repair still waits for the guard. ensure_local continues restoring its original target; dirty transactions remain intact and cannot probe storage or publish bytes.

Chat, WebSocket attachment resolution and KB ingestion snapshot metadata on the owning thread, return clean request connections and await detached storage work off-loop. Claimed/retired uploads map to each caller's existing missing-file behavior without exposing a claimed local path. Turn, KB and version snapshots retain persistent row identity and complete generation tokens, including nullable backend/URI/ETag changes. Publication lock contention remains a typed storage/503 boundary rather than an unhandled request error.

Uncached preview conversion holds its execution guard while owned temporary output can be created, drains work on cancellation and releases the guard before propagating it. Request finalization isolates ownership-query failures per staged path, preserves uncertain ownership/replacement evidence and continues cleaning the rest while keeping the primary error/cancellation contract. Configured-root aliases, external roots, bounded UTF-8 names and provider-specific cache namespaces retain their ownership gates. Deferred KB deletion publishes storage cleanup only after its exact generation CAS succeeds and the caller commits.

Deployment

Stop upload/local/preview producers, KB reference writers, collectors, and compensation workers; apply online migration 20261005_uploaded_file_cleanup_manifest; restart together on the same version. Preserve the database, resource roots, and shared LanceDB coordination directory. Mixed-version workers cannot honor publication/execution fencing. Downgrade refuses pending manifests; complete or reconcile those claims first.

See complete cleanup design and interruption protocol for phases, lock order, matching axes, and deployment constraints. This single combined PR exceeds the opening size/footprint budget with explicit user authorization.

Remaining delivery and known out-of-scope defects

This does not finish #1086. No detached collector, seven-day TTL, scan indexes, scheduling, or work budget is enabled; those remain 2C. Historical inventory and legacy/local-only backfill remain later work. Unknown historical resources are preserved for reconciliation.

• #2835: fresh canonical-path publication while old metadata compensates; existing safe conflict remains.
• #2836: per-file isolation of uncertain pre-claim reference checks in compensation batches.
• #2848: immediate direct-deletion SQL/row-lock ownership across storage I/O; existing safe guard remains until a separate redesign.
• #2849: consolidation of managed preview locators and cleanup ownership matchers.
• #2851: classify retained cleanup uncertainty for reconciliation and improve retry accounting. Existing compensating rows/manifests preserve every outstanding obligation until reconciled.
• #1242: whole-collection rollback versus concurrent ingestion.
• #1566: background-job takeover and duplicate execution.
• #1535: worker bookkeeping SQL sessions.
• #2781: backup replay reconciliation.
• #1576: account/team erasure. Other direct/standalone deletion is not redesigned.

SQL cleanup fences, .claimed markers, reference .lock files, and execution .cleanup.lock files remain monotonic. Safe compaction needs separate work; retired identities are not erased and live lock inodes are never replaced to reduce growth.

[rogercloud]

Approach: acceptable-with-reservations — the per-file execution lock sits on hot read paths as a blocking exclusive lock (first two Major items); the claim/phase-receipt/settlement model itself fits the existing compensation lifecycle.

Major

• src/xagent/web/services/uploaded_file_cleanup_publication.py:105 — blocking 15s file_cleanup_lock is taken on every ensure_local/materialize (even with a local copy) and called directly on the event loop from async handlers in src/xagent/web/api/files.py (download/preview/public_*); _convert_pptx_to_pdf holds the same lock across soffice. Fix: return an existing local copy lock-free, lock only to publish, use asyncio.to_thread, and narrow the PPTX lock to validate+publish.
  Trigger: PPTX preview-pdf in progress + any download/preview of the same file on one worker. Impact: loop blocks up to 15s, converter stalls, second request gets 503, all requests on that worker stall.
• src/xagent/web/services/uploaded_file_cleanup_resources.py:164 — uploads is resolved but path (from the unresolved upload path) is not, so with a symlinked uploads dir is_relative_to(owner_root) is False and the source is only "preserved". Fix: canonicalize realpath(parent)/name before containment and allowlist checks; add a symlinked-root test.
  Trigger: uploads dir behind a symlink. Impact: row settled "deleted" and removed while the local upload stays on disk with no handle.
• src/xagent/web/services/uploaded_file_cleanup_resources.py:102 — nested temp/quarantine names (.{name}.cleanup-{32hex} = N+42; restore temp in managed_file_ref.py + storage.py = N+53 vs N+28 on base) exceed 255 bytes for long names. Fix: bounded names from a hash of file_id/name plus a random suffix; add a 250-byte filename test.
  Trigger: uploads with ~203+ byte names (about 68+ CJK chars). Impact: restore returns 503; names >=214 bytes can never be quarantined and stay "pending" after the durable object is deleted.

Minor

• src/xagent/web/services/uploaded_file_store.py:1921 — delete CAS uses the full version snapshot (broader than a narrow guard) and KB delete callers (kb_collection_service.py, kb_file_service.py) neither skip compensating rows nor catch UploadedFileVersionConflict (500 on collection delete). The with_for_update() row lock is also held across delete_durable()/unlink when after_commit is None, blocking a concurrent claim on PostgreSQL. Fix: CAS on id/file_id/user_id + status != compensating, callers skip compensating rows, lock only the final conditional delete.
• uploaded_file_cleanup.py:111-121 — durable phase runs before already-recorded uncertainty (missing SHA-256, symlink source) is evaluated, leaving a compensating row that recovery re-warns on forever. Fix: fail or skip the claim before the durable phase, or persist a needs_reconciliation marker.
• uploaded_file_cleanup_publication.py:48-75 — claimed/fenced/version-mismatch raise DurableObjectMissingError, which files.py handlers treat as "serve file_ref.local_path". Fix: distinct exception mapped to 404.
• uploaded_file_cleanup_publication.py:94-101 — guard calls release_db_connection_if_clean on the caller's session and raises bare RuntimeError for a dirty one. Fix: don't release a session the adapter doesn't own; raise DurableStorageOperationError.
• uploaded_file_cleanup_resources.py:196-225 — os.listdir of the source parent per manifest is O(N*M) for flat user_{id}/ dirs at 500 per GC page. Fix: scandir with prefix match or per-directory cache.
• uploaded_file_cleanup_resources.py / storage.py — layout (materialize dirs, preview ownership, private storage._scoped) is re-derived in cleanup; a layout change silently leaks (S3 objects without xagent-sha256 metadata already leak cache copies). Fix: export path/matcher helpers from storage/preview layers.
• file_reference.py:69-78 — file_cleanup_lock on every cache hit creates a never-removed <sha>.cleanup.lock per file read and serializes readers; docs frame growth as cleanup-only. Fix: lock-free fast path, recheck under lock when publishing, document accurately.
• Tests: private filelock internals patched (FileLock._acquire, _context.lock_file_fd) while filelock>=3.0.0 is allowed; in-flight recovery test dropped the "exists keeps compensating, later absent settles" sequence; "external" case never exercises XAGENT_EXTERNAL_UPLOAD_DIRS; no symlinked-root test; pptx contention case stubs _resolve_file_path with file_record=None, so the asserted 503 never comes from the materialize guard.

Simplification

• src/xagent/web/api/files.py L497: drop the written_identities None default (single caller passes it; guard at 517-518 is dead). Make it required.
• src/xagent/web/services/uploaded_file_cleanup.py L86/127/147: three _save_manifest calls repeat 7 identity args. Bind once with functools.partial.
• src/xagent/web/services/uploaded_file_cleanup_resources.py L198-217: root-equality is evaluated per entry. Hoist prefixes once.
• net: -19 lines possible

Blocking: yes — recommended event: REQUEST_CHANGES

• src/xagent/web/services/uploaded_file_cleanup_publication.py:105, Major, blocking lock on event loop stalls the worker and 503s concurrent preview/download, [new]
• src/xagent/web/services/uploaded_file_cleanup_resources.py:164, Major, symlinked uploads dir leaves local file orphaned while row is deleted, [new]
• src/xagent/web/services/uploaded_file_cleanup_resources.py:102, Major, long filenames hit ENAMETOOLONG on restore and can never be quarantined, [new]

[codeacme17]

Follow-up to review 5426049616.

Addressed this round in 951aa7b4.

• Existing managed local copies and fresh previews now validate availability/generation without taking the exclusive execution lock. All seven async managed-copy API sites snapshot ORM values and return clean request connections before offloading storage/lock waits. Cancellation drains the native operation before releasing its guard. An actual managed PPTX conversion barrier proves a concurrent cached request completes with no SQL connections held.
• Configured uploads-root aliases map to canonical roots while child symlinks still fail closed. Configured external roots, including nested aliases, remain preserved. Bounded quarantine/restoration/materialization names support long ASCII and CJK filenames. Real process-death tests cover short, 210-byte, 250-byte, and CJK nested copies on SQLite and PostgreSQL.
• Known captured ambiguity now keeps the durable object and pending manifest before deletion. Invalid/claimed/retired/generation-changed publication uses a distinct unavailable error mapped to 404, so it cannot enter the durable-missing local fallback. Dirty caller writes are retained and cache misses return the established typed storage failure.
• KB document/directory deletion yields to compensation claims and lost-generation CAS. After-commit callbacks are published only after the row CAS succeeds; local/preview work runs after the caller commits. The full generation predicate remains: status/identity alone would allow a stale caller to delete a replacement publication.
• Storage materialization locators and bounded temp prefixes are shared with the producers; cleanup discovers local backend-hash namespaces without provider reads under reference locks. Tests use public filelock APIs, retain exists/unknown recovery sequences, and exercise the actual configured-root and managed-PPTX callers. written_identities is required and temporary prefixes are hoisted.

The PPTX cache-miss producer retains its guard while its subprocess can write owned temporary output. Locking only the final rename would let cleanup settle and remove its recovery handle before the converter recreates output. Cached readers no longer wait and the event loop stays responsive; narrowing the remaining producer interval would require a separate producer/recovery contract.

Immediate direct-deletion SQL-lock scope is tracked in #2848, and existing preview locator/matcher consolidation in #2849. Per-manifest discovery still scales with directory size; batching/bounds remain #1086 2C work-budget scope. Retired fences and live lock inodes remain monotonic as required. The existing explicit _save_manifest calls are retained; partial binding is optional style, without a demonstrated lifecycle defect.

Self-review found and fixed gaps in the initial revision: the intermediate 210-byte nested copy could lose its owner prefix; manifest capture briefly depended on provider hash reads; filename/backend/URI generation checks were incomplete; and previously NULL generation tokens were skipped. Production-call/process regressions cover all four.

Verification: 169 focused SQLite/PostgreSQL cases passed; 473 affected regressions passed with two KB-ingest-unavailable skips; Ruff, pinned isort/codespell, whitespace, and focused nine-file mypy passed. The full suite stopped at three pre-existing Alembic log assertions, reproduced on the exact reviewed head with the same environment (three failures, 298 passes). Parent-commit regressions fail on the actual API, collector, restoration, KB deletion and after-commit paths. This completes the fixes for this review round, not issue #1086 or its remaining 2C/legacy delivery.

The approved Standards/Spec preflight is open: no blocker; two nonblocking readability/field-list observations. Final configured pre-commit also passed, including its isolated mypy hook. Please re-review the current head.

[rogercloud]

Minor

• src/xagent/web/services/uploaded_file_cleanup_publication.py:149: the lock-free fast path skips the existing materialize cache, so concurrent reads return 503 during PPTX conversion (see inline comment).
• src/xagent/web/api/chat.py:349, src/xagent/core/tools/adapters/vibe/file_ingestion_tool.py:189, src/xagent/web/websocket.py:1362 (via resolve_turn_file_infos): async callers still run the guarded sync ensure_local on the event loop. Each call now runs a SQL validation, and on a cache miss with same-file contention it can wait up to 15s on FileLock, which blocks the loop. Offload these calls with asyncio.to_thread or async_managed_copy.
• src/xagent/web/services/uploaded_file_cleanup_publication.py:44: callers don't map FilePublicationUnavailable to missing-file handling, so a claim race returns 500 or fails the run (see inline comment).
• src/xagent/web/services/uploaded_file_cleanup_publication.py:59: snapshot-backed refs lose the row id, so the row-id check is skipped (see inline comment).
• src/xagent/web/services/uploaded_file_cleanup.py:108: an uncertain manifest leaves the claim committed, so every recovery sweep retries it and counts it as failed (see inline comment).
• src/xagent/web/api/files.py:694: one DB error in the finalizer aborts cleanup of the remaining staged uploads (see inline comment).
• tests/web/services/test_uploaded_file_recovery.py:294: the test no longer covers recovery deferring on exists and then settling on absent (see inline comment).
• tests/web/services/test_complete_upload_cleanup.py:1264: the PPTX contention test never reaches the lock guard (see inline comment).

Simplification

• src/xagent/web/services/uploaded_file_cleanup_resources.py L55: delete: _quarantine_name still has a readable .{name}.cleanup-… branch next to the hashed one. No released format wrote that form, and the manifest stores whichever name was chosen. Always return the hashed .cleanup-{sha24}-{gen16} form.
• src/xagent/web/services/uploaded_file_cleanup_publication.py L84: shrink: _prepare_copy returns (clone, sessions, read_only), but both extras are already stored on the clone and async_managed_copy ignores them. Return only the clone.
• src/xagent/web/services/uploaded_file_cleanup_publication.py L37: delete: "task_id" in _COPY_FIELDS is never read from the snapshot. Drop it.

net: -7 lines possible

Blocking: no — recommended event: APPROVE

[codeacme17]

Addressed this review round in one commit: 2b2018be.

• Valid materialization caches are read before the execution lock, with scoped checksum and generation validation.
• Chat, WebSocket attachment resolution and KB ingestion use off-loop storage/lock work without passing the owning ORM Session to worker threads. Claimed/retired results map to existing missing-file behavior; selected-file registration skips them.
• All affected snapshot producers retain persistent row identity and generation metadata, including row_id normalization and NULL transitions.
• Staged upload finalization isolates each uncertain ownership query and preserves the original caller error.
• The real-record PPTX regression now proves the Timeout cause and that no converter runs. The overlapping recovery test keeps its correct zero count; sequential exists/unknown-to-absent recovery is covered separately.
• The three simplifications are applied: always bounded hashed quarantine names, clone-only _prepare_copy, and no redundant task_id snapshot field. Existing manifests retain their persisted quarantine locators.

Retained uncertainty classification/accounting is explicitly deferred to P2 #2851; conservative retention stays intact.

Validation: 288 focused SQLite/PostgreSQL and caller tests passed; 316 additional affected regressions passed; all configured pre-commit hooks passed, including mypy across 880 source files. Full-suite execution encounters three Alembic INFO-log assertions, reproduced on the exact parent 951aa7b4 in an independent worktree with the same environment (3 failed, 298 passed); the two Alembic files pass in isolation, confirming collection/logging-order sensitivity. Standards/Spec review found no blocker (one non-blocking sync/async loop-duplication judgement call). The user explicitly chose to skip this round's pr-preflight before pushing.

Deployment requirements and the existing 2C/legacy/fence-compaction dependencies remain documented. This does not complete #1086. Refs #1086.