[llb555]

Why

attach_project_document_to_thread stages a copy of a shelf document and removes it in a finally. When that removal fails — a worker still holds the handle on Windows, a scanner intercepts it, the filesystem is read-only — two things go wrong at once:

• the raised PermissionError replaces whatever the endpoint was about to return: a successful 200 becomes a 500, and an in-flight 4xx is masked;
• service.aclose() is skipped, because it sits after the failing call in the same finally, so the only teardown this endpoint owns never runs.

The leftover staged file is the harmless part — it is what the staging sweep exists for. The masked outcome and the unclosed ingestion service are not.

Same class as #7200, at a site that PR does not touch.

What changed

Attach-to-thread now treats staged-copy removal as best-effort: an OSError there is logged as a warning and the endpoint still returns its real outcome, while aclose() moves into a nested finally so it is never skipped. This mirrors routers/uploads.py::_remove_staged_file, which already handles the identical situation for ordinary uploads.

No request or response shape changes.

Surface area

• Backend API — cleanup path only, for POST /api/projects/{project_id}/documents/{document_id}/attach-to-thread/{thread_id}

Bug fix verification

• Test path that reproduces the bug: backend/tests/test_project_document_attach_cleanup.py
• Did it go red on main and green on this branch? yes — 2 failed → 2 passed. Both cases inject a PermissionError from the staged path's unlink; the second also asserts the original HTTPException instance still propagates (is, not equality) and that cleanup_written() and aclose() both ran.

Validation

cd backend
python -m pytest tests/test_project_documents_router.py tests/test_project_documents_promotion.py \
  tests/test_project_document_char_cache.py tests/test_project_document_attach_cleanup.py -q
# 108 passed, 2 skipped

ruff check app/gateway/routers/project_documents.py tests/test_project_document_attach_cleanup.py
ruff format --check app/gateway/routers/project_documents.py tests/test_project_document_attach_cleanup.py
# All checks passed / 2 files already formatted

AI assistance

Tool(s) used: DeepSeek Harness (agentic coding agent)

How you used it: the agent located the site with a grep pass over finally-block cleanup patterns, wrote the two failing tests first, then the fix. I reviewed the diff and ran the checks above.

• I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

[willem-bd]

Verified at head 325cd58: the fix correctly makes the staged-copy unlink best-effort (except OSError + warning, matching uploads.py::_remove_staged_file) and moves aclose() into a nested finally so it can no longer be skipped — and aclose itself already swallows lease-release failures internally, so the cleanup path can no longer replace the caller's outcome. I reproduced the verification claim independently: the two new tests fail with PermissionError on main and pass at head, and the cited sibling suites are green (110 passed). On #7200 (still open): its cleanup_failed_atomic_write helper is sync/atomic-write-specific, so not reusing it here is right, and mirroring _remove_staged_file is the better fit — though the error-preservation family keeps growing (#6871, #7106, #7141, #7157, #7164, #7200, this), so a shared best-effort-unlink helper plus a sweep of the remaining sites would make a good consolidation follow-up. Two same-class gaps remain, in the comments below.

[willem-bd]

Same class, two more sites in this same file that this PR leaves unguarded: upload_document_to_shelf and promote_thread_file_to_shelf both call await run_file_io(staged.staging_path.unlink, True) on the empty-file rejection path (lines 220 and 336 at head) immediately before raising the 400. An OSError there — Windows handle still held by a worker or an AV scanner, the exact scenario in the PR description — replaces the intended 400 with a raw 500 and leaves the staged file behind. Since the best-effort pattern is now written, covering both (or filing them as the next family follow-up in the same change set) would close out the file.

[willem-bd]

Completeness check inside this same endpoint: the await service.cleanup_written() calls in the except blocks above (lines 412, 415, 418, 422) sit before each raise HTTPException(...), and cleanup_written can itself raise. _cleanup_uploaded_paths is per-path best-effort, but the companion leg goes through unregister_companion → record.unlink(missing_ok=True) (deerflow/uploads/companions.py), which only tolerates FileNotFoundError — a PermissionError there would replace the mapped 4xx/500 detail with a raw 500 and leave the remaining companions registered. Same masking class this PR fixes; consider making that leg best-effort too (here or in a tracked follow-up).

[willem-bd]

Nit: suppress(Exception) around a logger.warning call is redundant — logging already contains handler emit failures — and diverges from the mirrored uploads.py::_remove_staged_file, which logs bare. It would also silently hide a genuinely broken logging setup. logger.warning(..., exc_info=True) alone is sufficient here.

[llb555]

Thanks for the close read — all four addressed at ba8b934 (rebased onto current main, so the router diff is +20/−3).

1. [nit] Correct, the suppress was redundant and diverged from _remove_staged_file. The helper now logs bare: logger.warning(..., exc_info=True).
2. [two more sites] Both now go through one shared _discard_staged_copy() in this module, so the empty-file rejection can no longer turn its mapped 400 into a raw 500 — and the same worst case as the other routes. A test per site: both fail on main (the PermissionError escapes before the HTTPException) and pass here, asserting the 400 "Empty file" survives and the removal was attempted.
3. [cleanup_written / companion leg] Agreed it's the same class, and I left it out deliberately: it lives in uploads/companions.py, where record.unlink(missing_ok=True) tolerates only FileNotFoundError. Changing that module under a router-cleanup PR would blur the fix. Happy to take it as the next one in the family — same best-effort treatment plus a test that the mapped 4xx/500 survives a PermissionError from the companion leg.
4. [consolidation] Also agreed — a shared best-effort-unlink helper plus a sweep of the remaining finally sites is the right end state, and I'm glad to own it once this lands.

Validation at ba8b934: pytest tests/test_project_documents_router.py tests/test_project_documents_promotion.py tests/test_project_document_attach_cleanup.py → 107 passed, 2 skipped; ruff check / ruff format --check clean.

[llb555]

One correction on point 3, with the code at the reviewed head ae30b86 — _cleanup_written already guards the companion leg:

await run_file_io(uploads._cleanup_uploaded_paths, self._written_paths)
for original, _ in self._companion_pairs:
    try:
        await run_file_io(unregister_companion, original)
    except Exception:
        with suppress(Exception):
            logger.warning("Failed to remove upload companion record for %s", original, exc_info=True)

So a PermissionError from record.unlink(missing_ok=True) is caught there, and cleanup_written cannot replace the mapped 4xx/500. _cleanup_uploaded_paths is likewise per-path best-effort, so both legs are covered.

There is an unguarded caller, though, and it is the one that can mask a success: uploads/companions.py::invalidate_overwritten_upload() calls unregister_companion() directly, and client.py:1843 invokes it right after publish_upload_copy() returns — so on Windows a locked record would fail an upload whose file already landed. resolve_companion() validates source_identity, so a leftover record simply fails validation and cannot resolve to the wrong companion; dropping the unlink is safe.

I'll take that as the follow-up instead of touching cleanup_written.

[llb555]

Since #7200 just landed — quick status on this one: all four points from your review are addressed at ba8b934 (the router diff is +20/−3; the rest is the new test module).

Workflows don't start for fork PRs here, so there are no green checks to show: the only runs registered on this head are pr-labels, issue-triage and reviewing. Locally, pytest tests/test_project_documents_router.py tests/test_project_documents_promotion.py tests/test_project_document_attach_cleanup.py is 107 passed / 2 skipped, and ruff check / ruff format --check are clean on both touched files.

Happy to adjust anything else.