fix(utils): atomic writers and their callers refuse to resurrect a deleted named profile home - #112601
fix(utils): atomic writers and their callers refuse to resurrect a deleted named profile home#112601kokhlo wants to merge 1 commit into
Conversation
…e home Background writers that still carry a tombstoned profile as their Hermes home (reasoning-caps warm thread, models cache, models.dev ETag, gateway lifecycle ledger, MCP OAuth tokens, memory store) re-created profiles/<name>/ with a bare mkdir right before an atomic write. Route the parent-dir creation through mkdir_under_hermes_home so a deleted named profile raises FileNotFoundError and stays gone, matching the tombstone contract already enforced for logging and state.
kvnloo
left a comment
There was a problem hiding this comment.
Deleting a profile should mean it's gone. Before this, a background daemon — the reasoning-caps warmer in the report — could re-create profiles/<name>/ about 0.3 s after a successful delete. The delete looks unreliable, and the zombie directory leaves stale state behind for a future profile of the same name. This closes the hole for the atomic writers and, importantly, for the caller sites that were mkdir'ing the parent ahead of them — the report's point that a guarded writer alone wouldn't help when the caller has already brought the home back. Approving.
The mechanism checks out. mkdir_under_hermes_home (hermes_constants.py:288) runs assert_named_profile_home_live before the mkdir, raising FileNotFoundError for tombstoned or missing named profile homes. named_profile_home correctly returns None for the default home even when the path merely contains a profiles segment, and the tests pin the positive controls (/srv/profiles/buildcache, default .hermes, plain tmp). I verified the legitimate recreate flow is safe: create_profile (hermes_cli/profiles.py:899) calls clear_named_profile_deleted and builds in a staging sibling published by rename — it never goes through the guarded writers, so refusing to materialize can't block a real re-creation. The best-effort claim holds too: _quietly (agent/models_dev.py:175) catches Exception and FileNotFoundError is an OSError, so the models.dev paths degrade to a debug log; atomic_write_json's docstring already documents "Raises OSError on failure; callers decide whether that is best-effort," so the new raise lands inside the existing contract.
Non-blocking: load_on_disk_store() (tools/memory_tool.py:48) documents "never raises," but store.load_from_disk() sits outside the try block and can now raise FileNotFoundError for a deleted named profile home. The CLI /memory path (hermes_cli/cli_commands_mixin.py:1861) calls it with no handler. Either move the load inside the try or correct the docstring — the contract as written is now false for exactly the case this PR introduces.
Non-blocking: the function-level from hermes_constants import mkdir_under_hermes_home is unnecessary in agent/models_dev.py, agent/secret_sources/_cache.py, and tools/mcp_oauth.py — all three already import from hermes_constants at module top level (openrouter_variant_base, secure_parent_dir). Extending the existing top-level import is exactly as safe as the lazy one.
Non-blocking: mkdir_under_hermes_home is check-then-mkdir, so a profile delete landing between the liveness assert and the mkdir still resurrects. The window shrank from "every background write" to "a write racing a concurrent delete" — inherent without a lock, just naming the residual.
One design thread, offered not required: this is the third PR chasing the same class (#97128 for logging/state, #112594 for the writers, this one for the caller mkdirs), and a grep still finds ~40 bare parent.mkdir(parents=True, ...) call sites. Per-site discipline will keep leaking; a lint rule flagging bare parent-mkdir on home-derived paths — or making the guarded mkdir the default import — would end the class instead of the instances.
|
Salvaged into #112653 with your commit cherry-picked (authorship preserved). That PR lands #111927 (rename identity migration) together with the #112592 atomic-writer cluster (#112594 first-in, #112601 caller sites, #112596 sweep) on current |
…-profile guard A long-lived serve process keeps a deleted profile as the context home of threads that outlive the delete. A bare `mkdir(parents=True)` right before an atomic write brings `profiles/<name>/` back after `hermes profile delete` has written the tombstone and removed the tree. The writers in `utils` and the seven callers named in #112592 are guarded by the preceding commits; this one applies the same `mkdir_under_hermes_home` idiom to the other pre-write directory creations found by the same mechanical rule (auth, personality, plugin catalog, skills sync, tool discovery cache, platform adapters, memory plugins, local runtime supervisor, process identity, breadcrumbs). The two sites that pass `mode=` keep their mkdir behind `assert_named_profile_home_live`. The guard is a no-op unless the target has a provable `profiles/<name>` ancestor. Salvaged from #112596 (30-file sweep) on top of #112594 / #112601; the overlapping files were resolved to the already-landed versions.
Summary
hermes profile deleteremoves the tree and writes a tombstone underprofiles/.deleted/<name>, but a background writer that still carries the dead profile as its Hermes home could bringprofiles/<name>/back: the atomic writers created the target's parent with a baremkdir(parents=True), and so did many callers right before invoking them. Observed asprofiles/<name>/cache/reasoning_caps.jsonreappearing ~0.3 s after a successful delete (thereasoning-caps-warmdaemon thread finishing its catalog fetch) — the same resurrection class #97128 closed for logging and state.Fixes #112592.
Complementary to #112594 (opened before my claim comment): that PR guards
_atomic_write+_write_json_cache; this one additionally covers the caller sites that mkdir the parent themselves right before a writer — the report calls out that "even a guarded writer would find the home already re-created" when the caller has already brought the home back. Surfaces here do not overlap beyond the two shared sites (rebased on the same main). Maintainer's call on the split; happy to rebase or drop either half.What changed
utils._atomic_write,atomic_roundtrip_yaml_update,atomic_roundtrip_yaml_save: parent-dir creation now goes throughmkdir_under_hermes_home, the fix(profiles): deleted named profiles stay deleted — tombstones block logging resurrection (#89438, salvage #90141) #97128 guard — a tombstoned (or missing) named profile home raisesFileNotFoundErrorinstead of being materialized. Non-profile paths are unaffected (named_profile_homereturnsNone-> plainmkdir).hermes_cli/models.py:_write_json_cache(model/pricing/reasoning-caps caches)agent/models_dev.py:_save_etag(models.dev ETag sidecar)gateway/lifecycle_ledger.py:_write_sentinel+_append_exit_diag(gateway lifecycle logs)agent/secret_sources/_cache.py:atomic_write_json(secret cache entries)tools/mcp_oauth.py(refresh fence, token/client JSON, CIMD marker, snapshot restore)tools/memory_tool_store.py(load, file lock, add/replace/remove writes)All touched write paths were already best-effort wrapped (
debug-log,_quietly, ortry/except OSError), so a background writer hitting a deleted profile now degrades to a debug log instead of resurrecting the directory.Tests
New
tests/utils/test_atomic_writers_deleted_profile.py(11 tests):FileNotFoundError/ leaves nothing on disk — including the exact late reasoning-caps save from the report (viaset_hermes_home_override, platform-neutral), the models cache, lifecycle sentinel, OAuth tokens, memory store and the roundtrip YAML updateprofilessegment (/srv/profiles/buildcache), the default.hermeshome, and a plain tmp path all still create parents and write — mirroringtest_unrelated_profiles_dir_still_mkdirs_atomic_write+_write_json_cachemkdir swaps fails exactly the four resurrection tests; the full fix passes 11/11Adjacent suites stay green:
test_deleted_profile_tombstone.py,test_reasoning_caps_disk_cache.py,test_utils_atomic_roundtrip_yaml_save.py(35 passed),test_mcp_oauth.py+test_memory_tool.py+test_multiplex_lifecycle.py(123 passed; 2 pre-existing macOS callback-port failures reproduced on clean main viagit stash).