[llb555]

Why

secret_file._create_exclusive publishes the generated value by hard-linking the temporary file onto the final name, then removes its own temporary name in a finally. When that removal fails — a scanner still holding the handle on Windows, a read-only mount, a peer on SMB — the function raises after the secret is already published, so a credential file that was created successfully is reported as a failure. _publish_under_claim has the same shape on its abort path, and ManagedModelStore._write lets the cleanup error mask a failed os.replace, hiding the reason the write did not land.

The repository already owns a helper for this: deerflow.utils.atomic_write.cleanup_failed_atomic_write, documented as "Release an owned temporary without replacing the caller's outcome", already used by integrations/lark_cli.py, integrations/lark_broker.py and uploads/companions.py. These three sites are the ones that still bypass it.

What changed

Those three cleanup calls now go through the shared helper, which logs an OSError as a warning and leaves the caller's outcome intact. No new abstraction, no signature, format or permission change.

Surface area

No user-facing surface change — this is the cleanup path of two config writers. None of the template checkboxes apply.

Bug fix verification

• Test path that reproduces the bug: backend/tests/test_config_atomic_cleanup.py
• Did it go red on main and green on this branch? yes — 4 failed → 4 passed.
• Each case injects a PermissionError from Path.unlink and asserts the specific outcome survives: the published secret value for the success paths, and the original OSError message ("publish failed" in str(...)) for the abort paths. Asserting merely "an OSError was raised" would pass before and after the fix.

Validation

cd backend
python -m pytest tests/test_secret_file.py tests/test_managed_models.py tests/test_config_atomic_cleanup.py -q
# 44 passed

ruff check  packages/harness/deerflow/config/secret_file.py \
            packages/harness/deerflow/config/managed_models.py \
            tests/test_config_atomic_cleanup.py
ruff format --check  # same files
# All checks passed! / already formatted

AI assistance

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

How you used it: the agent located the three sites with a grep pass over finally-block cleanup patterns, wrote the 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.

Validation: pytest tests/test_secret_file.py tests/test_managed_models.py tests/test_config_atomic_cleanup.py -q -> 44 passed; ruff check and ruff format --check clean. The two new tests fail on main (the cleanup error replaces the published value / the original OSError) and pass here.

[willem-bd]

Reviewed at head 6716212 (config slice of the atomic-cleanup family: #7200 merged; #7229/#7164 still open). All paths and line numbers validated against the head tree, not a local checkout.

Verdict: correct fix, right mechanism; one comment-accuracy nit inline.

• Adopts, not duplicates: both modules import the shared cleanup_failed_atomic_write from deerflow.utils.atomic_write. #7200 is merged into main, so there is no collision or local reimplementation.
• Census (config layer, this head): the three bare-unlink sites are exactly ManagedModelStore._write and the two secret_file finally blocks — all fixed here. The fourth config writer, atomic_write_extensions_config (config/extensions_config.py:748-757), already guards its unlink with try/except OSError + warning log, which satisfies the family convention, so leaving it alone is fine. There is no runtime config.yaml writer in backend/app (no yaml-dump path exists), so nothing else in scope is missed; the PR body's "these three sites" claim is accurate for bug-carrying sites.
• fd ownership: all three call sites pass fd=None and the descriptor is closed before the finally — NamedTemporaryFile's context manager in managed_models._write, explicit inner finally: os.close(fd) in both secret_file paths. Matches the family rule that fd is transferred only while actually owned.
• Tests verified red/green: ran backend/tests/test_config_atomic_cleanup.py with PYTHONPATH pinned per tree — imports confirmed resolving into each tree; against base 29ba806 all 4 failed, against head 6716212 all 4 passed. The PR body's "4 failed → 4 pass" reproduces exactly.
• Coordination for the family: the same bare pattern still exists outside the config module at this head — notably extensions/manager.py:117 (unguarded Path(temporary).unlink(missing_ok=True) inside except BaseException: before raise, i.e. exactly the masking this PR fixes) and extensions/manager.py:1003, plus persistence/agents/file.py:257, persistence/managed_subagents/file.py:100, and the skills-storage writers. Out of this PR's declared scope; extensions/manager.py overlaps open #7164. Worth a follow-up census PR so the long tail lands somewhere instead of fading.

[willem-bd]

The suppression is correct on every exit path, but the stated reason only covers one of them. This finally is also reached when _write_all or os.close(fd) failed (nothing was published at all) and via line 149, where a hard-link-less filesystem publishes through _publish_under_claim's rename — not through a hard link. A reader taking the comment at face value could conclude the unlink is only ever reached after successful publication. Suggest wording that states the real invariant, e.g.: "Removing our own temporary name is best-effort on every exit path — whether the link published the value, a peer won the name, or an earlier step already failed — and must never mask that outcome." (Same accuracy bar as the #7200 AGENTS.md overstatement follow-up: the docstring should not promise more than the code guarantees.)

[llb555]

Comment accuracy fixed at 81c9847 — you're right that the old wording only described the hard-link success path, while the finally also runs when _write_all/os.close(fd) failed and on the claim path taken by hard-link-less filesystems. It now states the invariant instead:

    finally:
        # Removing our own temporary name is best-effort on every exit path —
        # whether the hard link published the value, a peer won the name, a
        # hard-link-less filesystem published through the replacement claim, or
        # an earlier step already failed — and must never mask that outcome.
        cleanup_failed_atomic_write(Path(temporary))

I applied the same accuracy bar to the test module's docstring, which also said "after the value is already published": it now covers every exit path and says a cleanup error must not replace either a published value or an earlier error.

On the census note: agreed, and thanks for the exact sites — extensions/manager.py:117 (the except BaseException one), :1003, persistence/agents/file.py:257, persistence/managed_subagents/file.py:100, plus the skills-storage writers. I'll take that as the follow-up census PR so the long tail lands somewhere rather than fading; I'd rather file it than widen this diff.

Re-verified after the wording change: pytest tests/test_config_atomic_cleanup.py tests/test_secret_file.py tests/test_managed_models.py → 44 passed; ruff check and ruff format --check clean on both changed files.

[llb555]

All three points from your review are addressed at 81c9847:

• the finally comment now states the invariant for every exit path — hard-link success, a peer winning the name, the replacement-claim path on hard-link-less filesystems, and an earlier step that already failed — instead of describing only the hard-link case;
• the test module's docstring said the same narrow thing and now matches the code;
• #7229 has landed, so the family overlap you flagged is gone.

pytest tests/test_config_atomic_cleanup.py tests/test_secret_file.py tests/test_managed_models.py → 44 passed; ruff check and ruff format --check are clean on both changed files.

The census follow-up you listed (extensions/manager.py:117 and :1003, persistence/agents/file.py:257, persistence/managed_subagents/file.py:100) is prepared separately, with a regression test that fails on the unpatched tree, so it does not have to widen this diff.