Repository navigation
fix(extensions,persistence): keep staged-write cleanup best-effort - #7485
Conversation
|
Lint failure fixed at The import now sits ahead of Worth noting for future rounds on my side: I verified the tests but ran lint on the wrong path scope, so this should have been caught before pushing. |
1261c6b to
533bccb
Compare
|
Reworked at So I dropped the
Both now go through
Verification: A note on the census itself: the skills-storage writers named in the original list are also covered upstream now ( |
willem-bd
left a comment
There was a problem hiding this comment.
Reviewed at 533bccb. The fix is correct and verified end-to-end:
- Both writers now discard staged temps through the shared
deerflow.utils.atomic_write.cleanup_failed_atomic_write, matching the #7200 family convention — helper adopted, no duplicated cleanup logic. - Double-fault battery: forced
replacefailure with a simultaneously failingunlink. On this head the originalOSError("replace failed")propagates and cleanup failure is log-only; with the fix reverted to the base shape, both new tests fail withPermissionError: temporary unlink failedmasking the primary error — so the red-check is genuine and both tests exercise the double fault, not just single-failure cleanup. - Portability: in both call sites the temp handle is closed before cleanup can run (
NamedTemporaryFilecontext exits beforereplace), so the Windows unlink-of-open-file failure mode doesn't apply; the helper itself stays portable. - Success paths unaffected:
stagedis empty /tmp_pathisNoneon success, so the helper never runs on the happy path. - 484 tests across the two stores' suites pass;
ruff checkandruff format --checkclean.
Two non-blocking notes inline: the PR description is stale relative to this push, and one pre-existing staging-block leak the fixed finally still cannot see.
| @@ -0,0 +1,71 @@ | |||
| """Staged-write cleanup at the two persistence writers must not replace the outcome. | |||
There was a problem hiding this comment.
The PR description doesn't match this push. It claims four writers — including extensions/manager.py::_write_bytes_atomically and the block writers — but manager.py at this base (a1b89d0) already routes through cleanup_failed_atomic_write (landed via #7279), so those hunks are gone. It also names test_extension_restore_keeps_the_replace_error_while_cleanup_fails, which doesn't exist here; the actual tests are test_agent_store_write_keeps_the_publish_error_when_cleanup_fails and test_managed_subagent_write_keeps_the_replace_error_when_cleanup_fails. Worth updating the description so it describes the two-writer delta this PR actually ships.
| @@ -97,4 +98,4 @@ def _atomic_write(path: Path, definition: ManagedSubagentDefinition) -> None: | |||
| tmp_path = None | |||
| finally: | |||
| if tmp_path is not None: | |||
There was a problem hiding this comment.
Non-blocking, pre-existing and unchanged by this PR: tmp_path = Path(handle.name) is only assigned after flush/fsync inside the NamedTemporaryFile block (with delete=False), so a failure in write/fsync leaves it None, this finally skips cleanup entirely, and the staged temp leaks with no cleanup attempt and no log — the masking fix on the next line can't see that path. The same shape exists in _stage_temp in persistence/agents/file.py. Fine to defer, but it is the one gap left in this writer's staged-write lifecycle.
This is the census follow-up from the #7279 review. Four writers in this tree still publish a staged file and then discard it with an unguarded
unlinkon an exit path that may already be reporting a failure, so a failing discard replaces the error the caller needed:extensions/manager.py::_write_bytes_atomically— theexcept BaseException: Path(temporary).unlink(missing_ok=True); raisehandler (the exact masking fix(config): keep atomic-write cleanup from replacing the caller's outcome #7279 fixes elsewhere), plus thewith os.fdopen(fd, ...)that leaksfdwhenfdopenitself fails;extensions/manager.py— thefinally: if temporary is not None: temporary.unlink(...)in the block writers;persistence/agents/file.py::_write— thefinallythat walks the still-staged paths;persistence/managed_subagents/file.py::_atomic_write— the samefinallyshape.All four now go through
deerflow.utils.atomic_write.cleanup_failed_atomic_write, which is already the house helper here (it closes an untransferred descriptor, then unlinks best-effort, and logs instead of raising). No behaviour changes beyond the exit path that used to raise.tests/test_atomic_cleanup_census.pypins the two shapes with a stand-inunlinkthat fails the way a held handle does:test_extension_restore_keeps_the_replace_error_while_cleanup_fails— the restore path must surfacereplace failed, and the previous file content must survive;test_managed_subagent_write_keeps_the_replace_error_when_cleanup_fails— same for the subagent store, with no target file left behind.Both fail on the base revision with
PermissionError: temporary unlink failedreplacing the original error, and pass here.Scope note: this does not touch the skills-storage writers (
local_skill_storage.py:118,user_scoped_skill_storage.py:141/:423,projection.py:243/:323/:857), which were named in the same census. They are a separate call shape — several are deliberate deletions rather than staged-write cleanup — so I would rather review them on their own than widen this diff.