[wannT-1]

Refs #7105

Sibling follow-up to the channel, project-document, and Buzz cleanup fixes. This does not close #7105; Lark CLI publication is still open.

Why

Unguarded Path.unlink inside an atomic-write failure path replaces the save error when cleanup itself fails. The caller then sees the cleanup error (often only str(exc)), the original cause survives only as __context__, and a staged file that could not be removed is easy to miss.

On current bytedance/deer-flow main this was still true for:

• extension snapshot rollback (_write_bytes_atomically in extensions/manager.py)
• the managed plugins: rewrite in the same module (finally unlink)
• Gateway skill-archive upload copy (_copy_uploaded_skill_archive)

skills/projection.py _write_manifest already uses cleanup_failed_atomic_write via merged #6871, so it is not in this diff. Channel publishers (#7106), project documents (#7141), and the Buzz seen-event store (#7157) are left to those PRs.

What changed

Those three writers now call cleanup_failed_atomic_write. Unlink and the warning log are independent best-effort steps, so a cleanup or logging failure cannot mask the original publication error, including KeyboardInterrupt on the extension writers.

Successful writes are unchanged: same bytes, same permission bits, same replace-before-visible behavior. A cleanup that succeeds still removes the temporary file and stays quiet.

Surface area

• Frontend UI — page / component / setting / interaction under frontend/
• Backend API — skill-archive upload copy failures keep the original size-limit or read error when temporary cleanup also fails. Success responses are unchanged.
• Agents / LangGraph — agent node, graph wiring, langgraph.json, or prompt change
• Sandbox — docker/ or sandboxed execution
• Skills — change under skills/
• Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json
• Default behavior change — success-path bytes, modes, and responses are unchanged
• Docs / tests / CI only — no runtime behavior change

Bug fix verification

• Test path that reproduces the bug:
  • backend/tests/test_extension_atomic_write_cleanup.py
  • backend/tests/test_skill_archive_upload_cleanup.py
• Did it go red on main and green on this branch? yes. With the production writers reverted, the unlink/logging cases failed because the caller observed temporary unlink failed instead of the publish/read/size-limit error. With this branch, those cases pass and the existing snapshot-restore suite still passes (28 passed).

Validation

cd backend
uv run ruff check packages/harness/deerflow/extensions/manager.py app/gateway/routers/skills.py tests/test_extension_atomic_write_cleanup.py tests/test_skill_archive_upload_cleanup.py
uv run ruff format --check packages/harness/deerflow/extensions/manager.py app/gateway/routers/skills.py tests/test_extension_atomic_write_cleanup.py tests/test_skill_archive_upload_cleanup.py
PYTHONPATH=. uv run pytest tests/test_extension_atomic_write_cleanup.py tests/test_skill_archive_upload_cleanup.py tests/test_extension_snapshot_restore.py -q

AI assistance

Tool(s) used: Cursor

How you used it: Implemented the helper reuse and regression tests from the existing cleanup_failed_atomic_write contract (#6871 / #7106).

• 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 dc1efca: the three converted sites are correct. cleanup_failed_atomic_write keeps descriptor close, unlink, and the warning log independent best-effort steps, so the original publication error (including KeyboardInterrupt) always surfaces; temporaries stay same-directory for os.replace, and success paths are untouched. The new tests force save failure and cleanup failure together and pin both the surfaced error and the warning (20 passed), and the snapshot-restore suite stays green (8 passed). Follow-ups from a sweep of the head tree: the same shadowing pattern survives in this file's endpoint-level finally unlink (see inline), in the two skills/storage write_custom_skill writers (see inline), plus two small leak windows in manager.py.

[willem-bd]

Same file, same bug class, one frame up: the finally in upload_and_install_skill (line 589) still does a bare await asyncio.to_thread(temporary_path.unlink, missing_ok=True). missing_ok only tolerates absence, so an EACCES/EBUSY unlink there is not tolerated: after a successful install it turns the response into a 500 (a retry then 409s), and on the 413/400 paths it replaces the mapped error — exactly what this PR fixes inside _copy_uploaded_skill_archive. The install path (ainstall_skill_from_archive) extracts the archive but never removes the uploaded copy, so this unlink runs on the success path too. Suggest routing it through the same helper: await asyncio.to_thread(cleanup_failed_atomic_write, temporary_path) (already imported at line 57).

[wannT-1]

Addressed in fdbfbc6d: upload_and_install_skill's finally now discards via await asyncio.to_thread(cleanup_failed_atomic_write, temporary_path), so a failing unlink/warning no longer turns a successful install into a 500 or replaces a mapped 413/400.

[willem-bd]

Completeness: the PR title says extensions AND skills, but two skills/ atomic writers still have the unguarded-cleanup pattern this PR removes elsewhere. backend/packages/harness/deerflow/skills/storage/local_skill_storage.py:118 and backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py:423 — both write_custom_skill implementations end with finally: tmp_path.unlink(missing_ok=True), so a failed unlink (Windows AV lock, EACCES) replaces the write/replace error. Adjacent: user_scoped_skill_storage._write_skill_states (lines 140-143) preserves the save error but swallows cleanup OSErrors with no warning log, unlike the helper's convention. Worth converting here too or listing them in #7105 so they are not lost.

[wannT-1]

Converted in this PR: both write_custom_skill sites and _write_skill_states now go through cleanup_failed_atomic_write (skill-state also warns on cleanup OSError instead of swallowing). Covered by tests/test_skill_storage_atomic_write_cleanup.py; utils/AGENTS.md caller list updated.

[willem-bd]

Nit: small ownership gap the surrounding contract covers. If os.fdopen(fd, "wb") itself raises, the raw descriptor is never closed — this call passes no fd, so the descriptor leaks, and on Windows the subsequent unlink then fails as well. The utils/AGENTS.md rule this PR extends ("pass a raw descriptor only until fdopen succeeds") and publish_bytes_atomically show the intended dance: fdopen outside the with, pass fd only on the fdopen-failure path. Very low likelihood.

[wannT-1]

Fixed: _write_bytes_atomically now does fdopen outside the with and passes the raw fd into cleanup_failed_atomic_write only on the fdopen-failure path, matching publish_bytes_atomically / the AGENTS.md note.

[willem-bd]

Nit: leak window in this same finally. temporary is assigned (line 999) only after stream.write(updated) completes, so a write failure inside the NamedTemporaryFile with-block — the classic case being ENOSPC — leaves temporary = None, skips this cleanup, and leaks the hidden delete=False temp beside config.yaml. Moving temporary = Path(stream.name) to the first statement inside the with closes it. Pre-existing on main, not a regression.

[wannT-1]

Fixed: temporary = Path(stream.name) is assigned as the first statement inside the NamedTemporaryFile with, so an ENOSPC (or other) write failure still hits the finally cleanup.

[wannT-1]

Rebased onto current main (2c880a31) and pushed follow-ups for the review threads (tip fdbfbc6d).

• Conflicts cleared — PR is mergeable again (still waiting on review/checks).
• upload_and_install_skill finally, both write_custom_skill writers, and _write_skill_states now use cleanup_failed_atomic_write.
• The two nits on _write_bytes_atomically / _write_plugins_block are in as well.

Happy to take another look when you have a moment.

[he-yufeng]

Verified on the PR head (fbfbc6d) with the worktree's own environment: all three new suites pass 42/42. The user_scoped_skill_storage.py hunk earns its pin — reverting _write_skill_states to the old unguarded tmp_path.unlink(missing_ok=True) fails test_skill_states_preserve_save_error_when_cleanup_fails[unlink], so the test really does watch the masking behavior.

Two details read correctly: fd = None right after fdopen matches the ownership-transfer contract documented in cleanup_failed_atomic_write (no double-close, and the helper still closes the raw descriptor when fdopen itself never happened), and extending the helper that already covers the channel/project-document/Buzz paths keeps one cleanup semantics across the tree.

Disclosure: my #6770 touches the same file (container_path resolution). The hunks are in different regions (_write_skill_states/write_custom_skill here vs. path resolution there), so no textual or semantic conflict either way.

[willem-bd]

Verified at fdbfbc6: the previous round is addressed correctly. The upload finally, both write_custom_skill sites and _write_skill_states now go through cleanup_failed_atomic_write; the descriptor is transferred only after fdopen succeeds in _write_bytes_atomically and _write_skill_states; and the plugins temporary path is recorded before the write, so a short write still reaches the cleanup. Success paths, modes and the snapshot-restore contract are unchanged, and the three new suites plus the documented AGENTS.md call sites match the code.

Three residual misses in the same bug class, none a regression: _FileSnapshot.restore's delete branch still unlinks unguarded inside the rollback loops (inline, manager.py), the upload route's finally still lets an unguarded form.close() mask the outcome and skip the archive discard (inline, skills.py), and _write_skill_states keeps except Exception so an interrupt still leaks its temp file (inline, nit).

[willem-bd]

Suggestion: the guard added here does not cover await form.close() at line 587, which runs first. FormData.close() closes each UploadFile, and SpooledTemporaryFile.close() can raise (a flush failure on ENOSPC being the realistic case). If it does, the mapped 413/400 or install error is replaced, a successful install still surfaces as a 500, and this discard never runs so the uploaded archive leaks onto disk - exactly the invariant the new routers/AGENTS.md paragraph claims for this finally. Wrap the close in contextlib.suppress(Exception) (or move the cleanup_failed_atomic_write call ahead of it).

[wannT-1]

Addressed in a01a001a: discard the archive with cleanup_failed_atomic_write before form.close(), and wrap close in contextlib.suppress(Exception), so a close/flush failure cannot replace the install outcome or skip the discard.

[willem-bd]

Same class, one branch over: _FileSnapshot.restore() (lines 140-144) still does a bare self.path.unlink(missing_ok=True) on the content is None branch - the branch that removes a file the failed install created, e.g. a uv.lock that did not exist before. It runs inside the except BaseException rollback loops (_install lines 329-330 and 342-343, _remove lines 443-444 and 458-459) ahead of the later restores, so an EBUSY/EACCES delete there still replaces operation_error and skips the remaining snapshots (for _remove, the config restore at line 446). Routing it through cleanup_failed_atomic_write(self.path) closes the last masking path in this rollback; the new tests only pin the content is not None branch, so a delete-branch case would cover it.

[wannT-1]

Addressed in a01a001a: the content is None branch now uses cleanup_failed_atomic_write(self.path). Added a two-snapshot rollback test covering a failing delete that must not mask the operation error or skip the later restore.

[willem-bd]

Nit: this is still except Exception, whereas the two writers this PR converts (_write_bytes_atomically, publish_bytes_atomically) use except BaseException so an interrupt also cleans up. A KeyboardInterrupt during json.dump/replace leaves the hidden .skill_states_*.json.tmp behind (the with handle closes the descriptor, but nothing unlinks the file), and an interrupt in the narrow window before fdopen leaks both descriptor and file. cleanup_failed_atomic_write already tolerates interruptions, so widening the clause would match the extension writers' contract. Low likelihood.

[wannT-1]

Addressed in a01a001a: widened to except BaseException so KeyboardInterrupt also cleans through the helper, matching the extension writers. Added a test that interrupt still propagates after cleanup.

[wannT-1]

Pushed a01a001a addressing the three residual items from the latest review: upload finally now discards before form.close() (close errors suppressed), _FileSnapshot.restore delete branch uses cleanup_failed_atomic_write, and _write_skill_states cleans up on BaseException. Tests cover the new paths; ruff clean.

[willem-bd]

Follow-up at a01a001 (the one commit since fdbfbc6): all three residuals from the 08:50 review are fixed as described. The upload route's finally now discards the archive through cleanup_failed_atomic_write before form.close() and suppresses close errors (skills.py 585-592), _FileSnapshot.restore's delete branch routes through the helper (manager.py 144) so a failing unlink can't replace the operation error or skip later restores, and _write_skill_states is widened to except BaseException (user_scoped_skill_storage.py 141). The AGENTS.md updates (routers, extensions, skills, utils, tests) match the code, including the discard-before-close and BaseException wording. The four findings from the 05:25 review remain fixed at this head. Verified on a tree built from this head with the repo venv (imports confirmed resolving into that tree): the three touched suites pass 51/51. One nit left, on the new delete-branch test.

[willem-bd]

Nit: this assertion is dead where it sits. It only runs for failure == "unlink" — the one param in which the log handler is never patched — so it cannot fail. The check it is aiming at (a failing logging handler must not leak into the surfaced error) belongs under if failure == "logging":, where it would actually bite: there the helper's boom_log RuntimeError is swallowed by the suppress(Exception) around the warning (atomic_write.py 34), and str(raised.value) stays "install failed". As written it reads as coverage for the logging-failure branch while covering nothing.

[wannT-1]

Addressed in b8b69c09: moved the log-handler assertion under if failure == "logging": so it actually covers a failing warning handler and keeps str(raised.value) == "install failed".

[wannT-1]

Pushed b8b69c09 for the delete-branch test nit: the logging-failure assertion now sits under failure == "logging".

[willem-bd]

Follow-up at b8b69c0 (one commit since a01a001, +2 lines in the delete-branch test only): the nit is fixed as suggested. The log-handler assertion now sits under if failure == "logging": together with assert str(raised.value) == "install failed" — the one param where boom_log is actually patched — so it now guards the real invariant: the helper's suppress around the warning keeps a failing handler from leaking into the surfaced install error. Nothing else changed since a01a001; all findings from the three earlier review rounds remain fixed. Nothing further from my side.