mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-11-06 05:17:15 +00:00
fix(profiles): a read-only profile copy can be deleted again
Mechanism: since 1672cf0b03 delete_profile waits for the profile to be unowned through
wait_profile_unowned -> exclusive_maintenance -> ProfileOwnership.reserve, which opens
gateway.lock O_RDWR|O_CREAT. In a NixOS-style read-only copy (0555 dirs, 0444 files) that
raises FileNotFoundError/EACCES (no lock: the 0555 home refuses the create) or
PermissionError (0444 lock), the OSError escapes the OwnershipConflict handler, and the
delete stops with "Could not remove" before _rmtree_make_writable ever runs. Main's
delete had no reservation step and removed such copies.
Fix: when the reservation cannot even be attempted (OSError), probe the lock read-only: a
missing lock, or one our own flock through an O_RDONLY descriptor wins, has no holder; a
held one still blocks the delete exactly like an OwnershipConflict (fail closed; Windows
refuses since a sharing violation is a live holder there).
Test: tests/hermes_cli/test_profile_delete_read_only_copy.py::
test_read_only_copy_is_unowned_unless_its_lock_is_held[False/True] (real 0555/0444 tree,
real flock holder subprocess); on r13-base2 both cases raise.
Review: dokterdok N106
This commit is contained in:
1 parent
8cc9e61d48
commit
e939044ae7
2 files changed
+70
-3
No files matched your search
@@ -23,9 +23,32 @@ def wait_profile_unowned(profile_dir: Path, *, timeout: float | None = None) ->
|
||||
with exclusive_maintenance([profile_dir]):
|
||||
return True
|
||||
except OwnershipConflict:
|
||||
if time.monotonic() >= deadline:
|
||||
return False
|
||||
time.sleep(0.25)
|
||||
pass
|
||||
except OSError:
|
||||
# A read-only copy (0555 home, 0444 lock) cannot host a reservation: probe it instead.
|
||||
if _lock_unheld_read_only(profile_dir / 'gateway.lock'):
|
||||
return True
|
||||
if time.monotonic() >= deadline:
|
||||
return False
|
||||
time.sleep(0.25)
|
||||
|
||||
|
||||
def _lock_unheld_read_only(path: Path) -> bool:
|
||||
"""A missing lock, or one our own flock through a read-only descriptor wins, has no holder."""
|
||||
if os.name == 'nt':
|
||||
return False # a sharing violation there is a live holder
|
||||
import fcntl
|
||||
try:
|
||||
fd = os.open(path, os.O_RDONLY | getattr(os, 'O_NOFOLLOW', 0) | os.O_NONBLOCK)
|
||||
except FileNotFoundError:
|
||||
return True
|
||||
try:
|
||||
fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||
return True
|
||||
except OSError:
|
||||
return False
|
||||
finally:
|
||||
os.close(fd)
|
||||
|
||||
|
||||
def _rmtree_make_writable(func, path, exc):
|
||||
|
||||
@@ -0,0 +1,44 @@
|
||||
"""A read-only profile copy (0555 dirs, 0444 files) can still be deleted (N106): the owner check
|
||||
cannot reserve gateway.lock there, so it proves the lock unheld through a read-only flock."""
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
|
||||
pytestmark = [pytest.mark.platforms("linux", "macos"),
|
||||
pytest.mark.skipif(hasattr(os, 'geteuid') and os.geteuid() == 0, reason="root writes 0555")]
|
||||
|
||||
|
||||
def _read_only_copy(home, *, lock):
|
||||
(home / 'sub').mkdir(parents=True)
|
||||
(home / 'config.yaml').write_text('model: x\n')
|
||||
if lock:
|
||||
(home / 'gateway.lock').write_text('')
|
||||
for parent, _dirs, files in os.walk(home, topdown=False):
|
||||
for name in files:
|
||||
os.chmod(os.path.join(parent, name), 0o444)
|
||||
os.chmod(parent, 0o555)
|
||||
|
||||
|
||||
@pytest.mark.parametrize('lock', [False, True])
|
||||
def test_read_only_copy_is_unowned_unless_its_lock_is_held(tmp_path, lock):
|
||||
from hermes_cli.profiles_remove import wait_profile_unowned
|
||||
home = tmp_path / 'profiles' / 'ro'
|
||||
_read_only_copy(home, lock=lock)
|
||||
try:
|
||||
assert wait_profile_unowned(home, timeout=0) is True
|
||||
if lock: # a live holder of the read-only inode still blocks the delete
|
||||
holder = subprocess.Popen(
|
||||
[sys.executable, '-c', 'import fcntl,sys;f=open(sys.argv[1]);fcntl.flock(f,fcntl.LOCK_SH);'
|
||||
'print(1,flush=True);sys.stdin.read()', str(home / 'gateway.lock')],
|
||||
stdin=subprocess.PIPE, stdout=subprocess.PIPE, text=True)
|
||||
try:
|
||||
assert holder.stdout.readline().strip() == '1'
|
||||
assert wait_profile_unowned(home, timeout=0) is False
|
||||
finally:
|
||||
holder.stdin.close()
|
||||
holder.wait(timeout=10)
|
||||
finally:
|
||||
for parent, _dirs, _files in os.walk(tmp_path):
|
||||
os.chmod(parent, 0o755)
|
||||
Reference in new issue
Block a user