fix(reset): fence every job registry the reset reads, by its physical directory (#78)
Review round 3 R1 (codex): the registration guard chose the workspace marker from the path's spelling, so a symlink alias of .mam/jobs or a MAM_JOBS_DIR registry was not refused. The reset now writes .reset-in-progress into each registry its open-job check reads (.mam/jobs, MAM_JOBS_DIR, a row's other control_plane.mam_jobs_dir - the latter only for this workspace's sessions), each under that registry's lock and before the check, and removes it on exit; register_job reads the marker inside the registry it writes to. The precheck's separate lock barrier is subsumed by the fence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
548c6854ea
commit
568385c77a
6 files changed
+186
-75
No files matched your search
+92
-44
@@ -1655,7 +1655,7 @@ if args[:2] == ["pane", "close"] or args[:1] == ["kill-session"]:
|
||||
"--agent-session", "herdr:{name_b}", "--prompt", "hi"],
|
||||
cwd={repr(str(mam_sandbox))}, stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=120,
|
||||
)
|
||||
jobs = sorted(os.listdir({repr(str(jobs_dir))})) if os.path.isdir({repr(str(jobs_dir))}) else []
|
||||
jobs = sorted(n for n in os.listdir({repr(str(jobs_dir))}) if n.endswith(".json")) if os.path.isdir({repr(str(jobs_dir))}) else []
|
||||
with open(probe, "w", encoding="utf-8") as fh:
|
||||
json.dump({{"rc": res.returncode, "stdout": res.stdout, "stderr": res.stderr,
|
||||
"marker": os.path.isfile(marker), "jobs": jobs}}, fh)
|
||||
@@ -1790,17 +1790,32 @@ def test_mam_reset_admit_refuses_only_a_live_reset(mam_sandbox: Path):
|
||||
assert res_stale.returncode == 0, res_stale.stderr
|
||||
|
||||
|
||||
def test_registry_cli_is_refused_after_the_target_recheck(mam_sandbox: Path, mock_herdr: Path, mock_agents):
|
||||
"""Review round 2 R1 (codex): the documented registry CLI, run by a caller
|
||||
@pytest.mark.parametrize("registry", ["canonical", "alias", "custom"])
|
||||
def test_registry_cli_is_refused_after_the_target_recheck(
|
||||
mam_sandbox: Path, mock_herdr: Path, mock_agents, registry: str
|
||||
):
|
||||
"""Review rounds 2-3 R1 (codex): the documented registry CLI, run by a caller
|
||||
that is not a child of the reset, after the target's open-job re-check and
|
||||
before its stop, is refused by .mam/reset-in-progress and registers nothing;
|
||||
the reset's own create still registers its onboarding job."""
|
||||
before its stop, is refused and registers nothing - whether it names the
|
||||
registry by its canonical path, by a symlink alias, or the registry is the
|
||||
MAM_JOBS_DIR the reset runs with; the reset's own create still registers
|
||||
its onboarding job."""
|
||||
import threading
|
||||
|
||||
name = "reviewer-claude-01"
|
||||
_claude_transcript(mam_sandbox, mam_sandbox, UA)
|
||||
_mock_herdr_agent(mock_herdr, name, mam_sandbox, agent="claude")
|
||||
_write_rows(mam_sandbox, [_row(name, "claude", mam_sandbox, own_id=UA)])
|
||||
jobs_dir = mam_sandbox / ".mam" / "jobs"
|
||||
jobs_dir.mkdir(parents=True, exist_ok=True)
|
||||
env_extra = None
|
||||
cli_dir = jobs_dir
|
||||
if registry == "alias":
|
||||
cli_dir = mam_sandbox / "job-alias"
|
||||
cli_dir.symlink_to(jobs_dir)
|
||||
elif registry == "custom":
|
||||
jobs_dir = cli_dir = mam_sandbox / "elsewhere" / "jobs"
|
||||
env_extra = {"MAM_JOBS_DIR": str(jobs_dir)}
|
||||
stop = mam_sandbox / ".agents/skills/multi-agent-mux-stop/scripts/stop_session.sh"
|
||||
original = stop.with_name("stop-original.sh")
|
||||
stop.rename(original)
|
||||
@@ -1821,7 +1836,6 @@ os.execv('/bin/bash', ['bash', {str(original)!r}, *sys.argv[1:]])
|
||||
)
|
||||
stop.write_text("#!/bin/bash\nexec %s %s \"$@\"\n" % (shlex.quote(sys.executable), shlex.quote(str(gate))))
|
||||
stop.chmod(0o755)
|
||||
jobs_dir = mam_sandbox / ".mam" / "jobs"
|
||||
record: dict = {}
|
||||
|
||||
def writer():
|
||||
@@ -1831,9 +1845,9 @@ os.execv('/bin/bash', ['bash', {str(original)!r}, *sys.argv[1:]])
|
||||
time.sleep(0.02)
|
||||
record["marker"] = (mam_sandbox / ".mam" / "reset-in-progress").is_file()
|
||||
before = sorted(p.name for p in jobs_dir.glob("*.json"))
|
||||
registry = mam_sandbox / ".agents/skills/multi-agent-mux-delegate-job/scripts/registry.py"
|
||||
cli = mam_sandbox / ".agents/skills/multi-agent-mux-delegate-job/scripts/registry.py"
|
||||
res = subprocess.run(
|
||||
[sys.executable, str(registry), "--registry-dir", str(jobs_dir), "register",
|
||||
[sys.executable, str(cli), "--registry-dir", str(cli_dir), "register",
|
||||
"--prompt", "external concurrent work", "--agent", "claude-code", "--agent-session", "herdr:" + name],
|
||||
capture_output=True, text=True, timeout=30,
|
||||
)
|
||||
@@ -1845,7 +1859,7 @@ os.execv('/bin/bash', ['bash', {str(original)!r}, *sys.argv[1:]])
|
||||
worker = threading.Thread(target=writer)
|
||||
worker.start()
|
||||
try:
|
||||
res = _run_reset(mam_sandbox, "--yes")
|
||||
res = _run_reset(mam_sandbox, "--yes", env_extra=env_extra)
|
||||
finally:
|
||||
worker.join(timeout=90)
|
||||
_wait_reconcile_idle(mam_sandbox)
|
||||
@@ -1858,63 +1872,97 @@ os.execv('/bin/bash', ['bash', {str(original)!r}, *sys.argv[1:]])
|
||||
assert _result_of(res.stdout, name) == "done"
|
||||
onboarding = [json.loads(p.read_text(encoding="utf-8")) for p in jobs_dir.glob("*.json")]
|
||||
assert [j["agent_session"] for j in onboarding] == ["herdr:" + name], onboarding
|
||||
assert not (jobs_dir / ".reset-in-progress").exists()
|
||||
|
||||
|
||||
def _registry_marker(jobs_dir: Path, holder: subprocess.Popen, ws: Path, sessions: Optional[str] = None) -> None:
|
||||
_live_marker(jobs_dir / ".reset-in-progress", holder)
|
||||
with open(jobs_dir / ".reset-in-progress", "a", encoding="utf-8") as fh:
|
||||
fh.write("workspace=%s\n" % ws)
|
||||
if sessions is not None:
|
||||
fh.write("sessions=%s\n" % sessions)
|
||||
|
||||
|
||||
def test_register_job_refuses_only_a_live_reset_not_its_own(mam_sandbox: Path, monkeypatch):
|
||||
from lib_py.delegation import registry
|
||||
|
||||
jobs_dir = str(mam_sandbox / ".mam" / "jobs")
|
||||
marker = mam_sandbox / ".mam" / "reset-in-progress"
|
||||
jobs_dir = mam_sandbox / ".mam" / "jobs"
|
||||
other = mam_sandbox / "other-control-plane" / "jobs"
|
||||
jobs_dir.mkdir(parents=True, exist_ok=True)
|
||||
other.mkdir(parents=True)
|
||||
monkeypatch.delenv(registry.RESET_OWNER_ENV, raising=False)
|
||||
holder = subprocess.Popen(["sleep", "120"])
|
||||
try:
|
||||
_live_marker(marker, holder)
|
||||
with pytest.raises(registry.ResetInProgressError, match="a reset of %s is in progress" % mam_sandbox):
|
||||
registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=jobs_dir)
|
||||
assert not list(Path(jobs_dir).glob("*.json"))
|
||||
# Another control plane's registry is not this workspace's .mam/jobs
|
||||
assert registry.reset_refusal(str(mam_sandbox / "elsewhere")) is None
|
||||
_registry_marker(jobs_dir, holder, mam_sandbox)
|
||||
_registry_marker(other, holder, mam_sandbox, sessions="herdr:a,herdr:b")
|
||||
for d in (jobs_dir, other):
|
||||
with pytest.raises(registry.ResetInProgressError, match="a reset of %s is in progress" % mam_sandbox):
|
||||
registry.register_job(prompt="p", agent_session="herdr:b", registry_dir=str(d))
|
||||
# Another control plane's registry refuses only the reset's sessions
|
||||
assert registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=str(other))
|
||||
assert not list(jobs_dir.glob("*.json"))
|
||||
monkeypatch.setenv(registry.RESET_OWNER_ENV, str(holder.pid))
|
||||
assert registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=jobs_dir)
|
||||
assert registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=str(jobs_dir))
|
||||
monkeypatch.delenv(registry.RESET_OWNER_ENV)
|
||||
finally:
|
||||
holder.kill()
|
||||
holder.wait()
|
||||
assert registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=jobs_dir)
|
||||
assert len(list(Path(jobs_dir).glob("*.json"))) == 2
|
||||
assert registry.register_job(prompt="p", agent_session="herdr:x", registry_dir=str(jobs_dir))
|
||||
assert len(list(jobs_dir.glob("*.json"))) == 2
|
||||
|
||||
|
||||
def test_precheck_waits_for_a_registration_holding_the_registry_lock(tmp_path: Path):
|
||||
"""R1 lock order: a registration that read no reset marker holds the registry
|
||||
lock until its record is written; the precheck, run after the marker is
|
||||
published, takes that lock before it lists the open jobs, so it sees the job."""
|
||||
def test_fence_waits_for_a_registration_and_covers_every_registry_read(tmp_path: Path):
|
||||
"""R1 lock order and scope: a registration that read no marker holds the
|
||||
registry lock until its record is written; fence writes the marker under
|
||||
that lock, so it waits, and the precheck after it sees the job. Fenced are
|
||||
.mam/jobs, MAM_JOBS_DIR (one marker for an alias of the same directory) and
|
||||
a row's other control-plane registry (for this registry's sessions only)."""
|
||||
import fcntl
|
||||
|
||||
ws = tmp_path / "ws"
|
||||
jobs_dir = ws / ".mam" / "jobs"
|
||||
jobs_dir.mkdir(parents=True)
|
||||
state = {"herdr_sessions": [_row("reviewer-claude-01", "claude", ws, own_id=UA)]}
|
||||
env = dict(os.environ, PYTHONPATH=str(SKILLS), PYTHONDONTWRITEBYTECODE="1", MAM_STATE_JSON=json.dumps(state))
|
||||
env.pop("MAM_JOBS_DIR", None)
|
||||
lock = open(jobs_dir / ".lock", "a")
|
||||
fcntl.flock(lock, fcntl.LOCK_EX)
|
||||
(ws / "alias").symlink_to(jobs_dir)
|
||||
other = tmp_path / "cp" / "jobs"
|
||||
other.mkdir(parents=True)
|
||||
row = _row("reviewer-claude-01", "claude", ws, own_id=UA)
|
||||
row["control_plane"] = {"mam_jobs_dir": str(other)}
|
||||
state = {"herdr_sessions": [row]}
|
||||
env = dict(os.environ, PYTHONPATH=str(SKILLS), PYTHONDONTWRITEBYTECODE="1", MAM_STATE_JSON=json.dumps(state),
|
||||
MAM_JOBS_DIR=str(ws / "alias"))
|
||||
holder = subprocess.Popen(["sleep", "120"])
|
||||
try:
|
||||
proc = subprocess.Popen(
|
||||
[sys.executable, "-m", "lib_py.reset", "precheck", "--workspace", str(ws)],
|
||||
env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True,
|
||||
)
|
||||
time.sleep(1.5)
|
||||
assert proc.poll() is None, proc.communicate()
|
||||
(jobs_dir / "job78locked.json").write_text(
|
||||
json.dumps({"job_id": "job78locked", "agent_session": "herdr:reviewer-claude-01", "status": "pending"}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
_live_marker(ws / ".mam" / "reset-in-progress", holder)
|
||||
lock = open(jobs_dir / ".lock", "a")
|
||||
fcntl.flock(lock, fcntl.LOCK_EX)
|
||||
try:
|
||||
proc = subprocess.Popen(
|
||||
[sys.executable, "-m", "lib_py.reset", "fence", "--workspace", str(ws)],
|
||||
env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True,
|
||||
)
|
||||
time.sleep(1.5)
|
||||
assert proc.poll() is None, proc.communicate()
|
||||
assert not (jobs_dir / ".reset-in-progress").exists()
|
||||
(jobs_dir / "job78locked.json").write_text(
|
||||
json.dumps({"job_id": "job78locked", "agent_session": "herdr:reviewer-claude-01", "status": "pending"}),
|
||||
encoding="utf-8",
|
||||
)
|
||||
finally:
|
||||
fcntl.flock(lock, fcntl.LOCK_UN)
|
||||
lock.close()
|
||||
out, err = proc.communicate(timeout=30)
|
||||
assert proc.returncode == 0, out + err
|
||||
assert out.split() == [os.path.realpath(jobs_dir), os.path.realpath(other)]
|
||||
own = (jobs_dir / ".reset-in-progress").read_text(encoding="utf-8")
|
||||
assert "pid=%d\n" % holder.pid in own and "workspace=%s\n" % os.path.realpath(ws) in own
|
||||
assert "sessions=" not in own
|
||||
assert "sessions=herdr:reviewer-claude-01\n" in (other / ".reset-in-progress").read_text(encoding="utf-8")
|
||||
pre = subprocess.run([sys.executable, "-m", "lib_py.reset", "precheck", "--workspace", str(ws)],
|
||||
env=env, capture_output=True, text=True, timeout=30)
|
||||
assert pre.returncode == 1 and "job job78locked" in pre.stdout, pre.stdout + pre.stderr
|
||||
finally:
|
||||
fcntl.flock(lock, fcntl.LOCK_UN)
|
||||
lock.close()
|
||||
out, err = proc.communicate(timeout=30)
|
||||
assert proc.returncode == 1, out + err
|
||||
assert "job job78locked" in out
|
||||
holder.kill()
|
||||
holder.wait()
|
||||
|
||||
|
||||
@pytest.mark.parametrize("artifact_cwd", ["own", "foreign"])
|
||||
|
||||
Reference in new issue
Block a user