fix(issue-50): review findings of PR #56 on macOS

- admission: a `submit --dry-run` record is marked dry_run and is not an
  open job of its session (review 1e25cf2f F1)
- create: the monitor-row adoption requires the row's server to be the one
  this create spawned on (review 1e25cf2f F2)
- pane lock: retry only on contention (BlockingIOError); the docs say the
  flock lasts while any fd-9 holder (the claim shell and the shim) lives
  (review 66680d9d)
- tests on macOS: real-herdr box HOMEs under /tmp (sun_path is 104 bytes),
  resolved paths, the fake claude reads non-canonical input (MAX_CANON 1024)
  and the pane is read unwrapped; lock holders exec their sleep so killing
  them frees the lock; the adoption tests name the server

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
GodopuandClaude Opus 5.5 committed 2026-10-12 09:03:08 +09:00
1 parent 320aa9f7a0
commit a9ffedac36
10 files changed
+75 -19

No files matched your search

+4 -2
View File
@@ -1495,8 +1495,10 @@ _herdr_scope_is_pane_inherited() {
# later `agent start` failed with agent_pane_busy. mam_herdr runs the whole # later `agent start` failed with agent_pane_busy. mam_herdr runs the whole
# new-session call holding one lock per server: a flock on fd 9, opened for # new-session call holding one lock per server: a flock on fd 9, opened for
# the block below and taken by lib_py.pane_lock (which also bounds the wait, # the block below and taken by lib_py.pane_lock (which also bounds the wait,
# MAM_PANE_CLAIM_WAIT_S). It ends with the block or with this process, however # MAM_PANE_CLAIM_WAIT_S). It ends with the block, or when every process that
# that ends, so a dead holder leaves nothing behind; the file is the target # holds fd 9 - this shell and the shim it runs - has ended, however that ends,
# so a dead holder leaves nothing behind. Nothing long-lived may inherit fd 9
# (the herdr server is spawned with close_fds; review 66680d9d); the file is the target
# server's, so other servers' creates are not held. The lock lives here, not # server's, so other servers' creates are not held. The lock lives here, not
# in the shim heredoc, which stays byte-identical (test_issue23_24 S11). # in the shim heredoc, which stays byte-identical (test_issue23_24 S11).
# #
@@ -90,6 +90,7 @@ def register_job(
transport: str = "herdr", transport: str = "herdr",
supersede_older_attempt: bool = False, supersede_older_attempt: bool = False,
parent_job: Optional[str] = None, parent_job: Optional[str] = None,
dry_run: bool = False,
) -> str: ) -> str:
"""Create a new ``pending`` job record and return its id. """Create a new ``pending`` job record and return its id.
@@ -106,6 +107,10 @@ def register_job(
Issue #34: ``parent_job`` names the job this one was delegated from, so a Issue #34: ``parent_job`` names the job this one was delegated from, so a
loop waiting on the parent can find and wait for it. None is a root job. loop waiting on the parent can find and wait for it. None is a root job.
Issue #50 (review 1e25cf2f F1): ``dry_run`` marks the record of a
``submit --dry-run``, which is never dispatched, so the registry admission
does not count it as an open job of its session.
""" """
job_id = job_id or generate_job_id(bits) job_id = job_id or generate_job_id(bits)
heartbeat_sec = max(1, int(heartbeat_sec)) heartbeat_sec = max(1, int(heartbeat_sec))
@@ -152,6 +157,8 @@ def register_job(
# Issue #34: the job this one was delegated from (None = a root job) # Issue #34: the job this one was delegated from (None = a root job)
"parent_job": parent_job, "parent_job": parent_job,
} }
if dry_run:
record["dry_run"] = True
with registry_lock(registry_dir): with registry_lock(registry_dir):
if mqtt_common._job_path(job_id, registry_dir).exists(): if mqtt_common._job_path(job_id, registry_dir).exists():
if not supersede_older_attempt: if not supersede_older_attempt:
@@ -393,6 +400,8 @@ def _build_parser() -> argparse.ArgumentParser:
p_reg.add_argument("--reviewer", default=None) p_reg.add_argument("--reviewer", default=None)
p_reg.add_argument("--reviewer-session", default=None) p_reg.add_argument("--reviewer-session", default=None)
p_reg.add_argument("--max-iterations", type=int, default=5) p_reg.add_argument("--max-iterations", type=int, default=5)
p_reg.add_argument("--dry-run", action="store_true",
help="the record of a submit --dry-run: never dispatched, not an open job for admission")
p_reg.add_argument("--parent-job", default=None, p_reg.add_argument("--parent-job", default=None,
help="the job this one is delegated from (Issue #34); must exist in this registry") help="the job this one is delegated from (Issue #34); must exist in this registry")
@@ -569,6 +578,7 @@ def main(argv: Optional[List[str]] = None) -> int:
attempt_id=args.attempt_id, attempt_id=args.attempt_id,
job_id=args.job_id, job_id=args.job_id,
parent_job=args.parent_job, parent_job=args.parent_job,
dry_run=args.dry_run,
) )
print(job_id) print(job_id)
return 0 return 0
@@ -297,6 +297,8 @@ def open_jobs(registry_dir: str, session: str, job_id: str, now: Optional[int] =
rec = _load_record(registry_dir, other) rec = _load_record(registry_dir, other)
if rec is None or _bare(str(rec.get("agent_session") or "")) != sess: if rec is None or _bare(str(rec.get("agent_session") or "")) != sess:
continue continue
if rec.get("dry_run"): # a submit --dry-run is never dispatched (review 1e25cf2f F1)
continue
status = rec.get("status") status = rec.get("status")
if status == "running": if status == "running":
idle = _int_field(rec, "idle_timeout_sec", DEFAULT_IDLE_TIMEOUT_SEC) idle = _int_field(rec, "idle_timeout_sec", DEFAULT_IDLE_TIMEOUT_SEC)
+9 -5
View File
@@ -19,9 +19,12 @@ one lock per server from the choice to the claim.
`locked`, or `timeout` once MAM_PANE_CLAIM_WAIT_S `locked`, or `timeout` once MAM_PANE_CLAIM_WAIT_S
(default 120, minimum 1) has passed (default 120, minimum 1) has passed
The lock is a BSD flock, which belongs to the open file the shim holds on fd 9: The lock is a BSD flock, which belongs to the open file on fd 9: it outlives
it outlives this helper and ends with the shim's process, however that ends this helper and ends when the last process holding that fd ends, however it
(exit, kill -9), so a dead holder leaves nothing to steal. flock(1) is not on ends (exit, kill -9), so a dead holder leaves nothing to steal. The holders are
the shell running the claim block and the command it runs (the shim); a child
that outlived them would keep it too, so nothing long-lived may inherit fd 9 -
the herdr server is spawned with close_fds (review 66680d9d). flock(1) is not on
macOS; fcntl.flock is. Stdlib only, Python 3.9. macOS; fcntl.flock is. Stdlib only, Python 3.9.
""" """
from __future__ import annotations from __future__ import annotations
@@ -67,13 +70,14 @@ def wait_s(env: Mapping[str, str]) -> int:
def acquire(fd: int, wait: float) -> str: def acquire(fd: int, wait: float) -> str:
"""`locked`, or `timeout` when the lock was not free within `wait` s. The """`locked`, or `timeout` when the lock was not free within `wait` s. The
deadline is on the monotonic clock; a step of the wall clock does not deadline is on the monotonic clock; a step of the wall clock does not
stretch or cut it.""" stretch or cut it. Only contention is waited out: any other error (a bad
fd) raises, which the shim reports as an undecided lock."""
deadline = time.monotonic() + wait deadline = time.monotonic() + wait
while True: while True:
try: try:
fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
return "locked" return "locked"
except OSError: except BlockingIOError:
if time.monotonic() >= deadline: if time.monotonic() >= deadline:
return "timeout" return "timeout"
time.sleep(0.2) time.sleep(0.2)
@@ -533,9 +533,13 @@ running_same = [s for s in sessions if s.get('name') == name and s.get('status')
# registered, with no job, for a pane created after this create began, is # registered, with no job, for a pane created after this create began, is
# this create's own pane - herdr keeps live names unique per server and the # this create's own pane - herdr keeps live names unique per server and the
# preflight refused an older session of this name - so it is replaced, not # preflight refused an older session of this name - so it is replaced, not
# refused as a second live session. # refused as a second live session. Names are unique per server only, so the
# row must be this server's (review 1e25cf2f F2: another server's row of the
# name was replaced). The pane pid is no identity: it is the pane's foreground
# process at the time it was read.
def _adopted_own(s): def _adopted_own(s):
return (s.get('last_visible_note') == 'auto-registered by monitor' and not s.get('delegate_job_id') return (s.get('last_visible_note') == 'auto-registered by monitor' and not s.get('delegate_job_id')
and (s.get('herdr_server') or s.get('herdr_session') or 'default') == server_name
and epoch.isdigit() and int(s.get('herdr_session_epoch') or 0) >= int(epoch) - 1) and epoch.isdigit() and int(s.get('herdr_session_epoch') or 0) >= int(epoch) - 1)
if running_same and all(_adopted_own(s) for s in running_same): if running_same and all(_adopted_own(s) for s in running_same):
print(f"NOTE: the monitor registered {name} while it was being created; that row is replaced by this one", flush=True) print(f"NOTE: the monitor registered {name} while it was being created; that row is replaced by this one", flush=True)
@@ -292,6 +292,10 @@ cmd_submit() {
fi fi
fi fi
# Issue #50: a dry-run's record is never dispatched, so admission skips it
local reg_dry=()
if [[ "$DRY_RUN" == "1" ]]; then reg_dry=(--dry-run); fi
# 1) register job (prints the new job id) # 1) register job (prints the new job id)
JOB_ID="$("$PY" "$SCRIPT_DIR/scripts/registry.py" --registry-dir "$REGISTRY_DIR" register \ JOB_ID="$("$PY" "$SCRIPT_DIR/scripts/registry.py" --registry-dir "$REGISTRY_DIR" register \
"${reg_prompt[@]}" --agent "$AGENT" --agent-session "$AGENT_SESSION" --role "$DELEGATE_ROLE" \ "${reg_prompt[@]}" --agent "$AGENT" --agent-session "$AGENT_SESSION" --role "$DELEGATE_ROLE" \
@@ -299,7 +303,7 @@ cmd_submit() {
--heartbeat-sec "$HEARTBEAT_SEC" --heartbeat-misses "$HEARTBEAT_MISSES" --ack-timeout "$ACK_TIMEOUT" \ --heartbeat-sec "$HEARTBEAT_SEC" --heartbeat-misses "$HEARTBEAT_MISSES" --ack-timeout "$ACK_TIMEOUT" \
--transport "$(_is_remote "$AGENT_SESSION" && echo remote || echo herdr)" \ --transport "$(_is_remote "$AGENT_SESSION" && echo remote || echo herdr)" \
--job-type "$TYPE" --reviewer "$REVIEWER" --reviewer-session "$REVIEWER_SESSION" \ --job-type "$TYPE" --reviewer "$REVIEWER" --reviewer-session "$REVIEWER_SESSION" \
--max-iterations "$MAX_ITERATIONS" ${reg_parent[@]+"${reg_parent[@]}"})" --max-iterations "$MAX_ITERATIONS" ${reg_parent[@]+"${reg_parent[@]}"} ${reg_dry[@]+"${reg_dry[@]}"})"
echo "registered job: $JOB_ID" echo "registered job: $JOB_ID"
# 1-1) Provision job directory and write direct brief.md (MAM Job Restructuring) # 1-1) Provision job directory and write direct brief.md (MAM Job Restructuring)
+1 -1
View File
@@ -620,7 +620,7 @@ def test_h26_new_session_claims_its_pane_under_the_servers_lock(mam_sandbox, moc
locks = glob.glob(str(tmp_path / ".local" / "state" / "mam" / "pane-lock" / "*.lock")) locks = glob.glob(str(tmp_path / ".local" / "state" / "mam" / "pane-lock" / "*.lock"))
assert len(locks) == 1, locks # one file per server, under the sandbox HOME assert len(locks) == 1, locks # one file per server, under the sandbox HOME
n_calls = len(_load_mock_state(mock_herdr).get("calls", [])) n_calls = len(_load_mock_state(mock_herdr).get("calls", []))
holder = subprocess.Popen(["bash", "-c", 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9; sleep 600', holder = subprocess.Popen(["bash", "-c", 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9; exec sleep 600',
"holder", locks[0]], stdout=subprocess.PIPE, text=True, "holder", locks[0]], stdout=subprocess.PIPE, text=True,
env=dict(os.environ, PYTHONPATH=str(tmp_path / ".agents" / "skills"))) env=dict(os.environ, PYTHONPATH=str(tmp_path / ".agents" / "skills")))
try: try:
+20
View File
@@ -159,6 +159,26 @@ def test_u1c_the_job_itself_and_unreadable_records_are_skipped(tmp_path):
assert decide.open_jobs(str(tmp_path), "herdr:s", me) == ([], []) assert decide.open_jobs(str(tmp_path), "herdr:s", me) == ([], [])
def test_u1e_a_dry_run_record_is_not_an_open_job(tmp_path):
"""review 1e25cf2f F1: `submit --dry-run` registers a record it never
dispatches; it stays pending (Issue #34 shows it as an open job), but the
admission must not wait for it."""
reg = tmp_path / "jobs"
env = dict(os.environ, HOME=str(tmp_path), MAM_ENV_FILE=str(tmp_path / ".mam.env.absent"))
res = subprocess.run(["bash", str(WRAPPER), "submit", "--agent", "human", "--agent-session", "herdr:s",
"--dry-run", "--registry-dir", str(reg), "--prompt", "preview only"],
capture_output=True, text=True, env=env, cwd=str(tmp_path), timeout=120)
assert res.returncode == 0, res.stdout + res.stderr
(dry,) = [p.stem for p in reg.glob("*.json")]
assert _rec(reg, dry)["status"] == "pending" and _rec(reg, dry)["dry_run"] is True
me = _register(reg)
assert decide.open_jobs(str(reg), "herdr:s", me) == ([], [])
assert decide.wait_open_jobs(_ns(reg, me)) == "clear"
# control: the same record without the mark is an `injected` job that holds the session
_edit(reg, dry, dry_run=None)
assert decide.wait_open_jobs(_ns(reg, me)) == f"busy {dry}"
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
# U2 - FIFO release and the bound (wait_s=0: an immediate decision) # U2 - FIFO release and the bound (wait_s=0: an immediate decision)
# --------------------------------------------------------------------------- # ---------------------------------------------------------------------------
+8 -3
View File
@@ -55,6 +55,9 @@ FAKE_CLAUDE = r'''#!/usr/bin/env bash
if [ "${1:-}" = "auth" ]; then echo '{"loggedIn": true}'; exit 0; fi if [ "${1:-}" = "auth" ]; then echo '{"loggedIn": true}'; exit 0; fi
printf '\n ▐▛███▛█ Claude Code v2.1.0 (fake)\n▝▜██████▀ Sonnet · Claude Max\n\n' printf '\n ▐▛███▛█ Claude Code v2.1.0 (fake)\n▝▜██████▀ Sonnet · Claude Max\n\n'
printf '───────────────────────────────────────────────\n❯ Try "how does <filepath> work?"\n───────────────────────────────────────────────\n ⏵⏵ bypass permissions on (shift+tab to cycle)\n' printf '───────────────────────────────────────────────\n❯ Try "how does <filepath> work?"\n───────────────────────────────────────────────\n ⏵⏵ bypass permissions on (shift+tab to cycle)\n'
# non-canonical input, as a real TUI reads it: a canonical tty line is capped at
# MAX_CANON (1024 bytes on macOS), shorter than one injected prompt
stty -icanon 2>/dev/null
while IFS= read -r line; do printf '\n● got: %s\n❯ \n' "$line"; done while IFS= read -r line; do printf '\n● got: %s\n❯ \n' "$line"; done
''' '''
@@ -99,7 +102,7 @@ class Box:
return "" return ""
def pane_text(self, name=SESSION) -> str: def pane_text(self, name=SESSION) -> str:
res = self.herdr("agent", "read", name, "--source", "recent") res = self.herdr("agent", "read", name, "--source", "recent-unwrapped", "--lines", "1000")
try: try:
d = json.loads(res.stdout)["result"] d = json.loads(res.stdout)["result"]
return (d.get("read") or d).get("text", "") return (d.get("read") or d).get("text", "")
@@ -192,7 +195,9 @@ def _row(sess: str, name=SESSION, ws="") -> str:
def _make_box(sess: str) -> Box: def _make_box(sess: str) -> Box:
home = Path(tempfile.mkdtemp(prefix="mam50-")) # short: a unix socket path lives under it # short: a unix socket path lives under it (macOS sun_path is 104 bytes and
# its TMPDIR is long); resolved, as the scripts print /private/tmp paths
home = Path(tempfile.mkdtemp(prefix="mam50-", dir="/tmp")).resolve()
box = Box(home, sess) box = Box(home, sess)
for d in (box.bin, box.ws, box.ctl / ".agents", box.jobs, box.ctl / ".mam" / "delegate_job_logs"): for d in (box.bin, box.ws, box.ctl / ".agents", box.jobs, box.ctl / ".mam" / "delegate_job_logs"):
d.mkdir(parents=True) d.mkdir(parents=True)
@@ -382,7 +387,7 @@ def test_c4_the_loops_first_job_waits_for_the_onboarding_to_end(box, monkeypatch
def test_c5_a_session_without_a_row_is_judged_by_the_records_alone(local_mqtt_broker): def test_c5_a_session_without_a_row_is_judged_by_the_records_alone(local_mqtt_broker):
# resolve_herdr_session falls back to the workspace slug when the yaml has no # resolve_herdr_session falls back to the workspace slug when the yaml has no
# row, so the server is named exactly that: mam-<parent>-<work> of the control root # row, so the server is named exactly that: mam-<parent>-<work> of the control root
home = Path(tempfile.mkdtemp(prefix="mam50-")) home = Path(tempfile.mkdtemp(prefix="mam50-", dir="/tmp")).resolve()
sess = f"mam-{home.name}-ctl".lower().replace("_", "-") sess = f"mam-{home.name}-ctl".lower().replace("_", "-")
shutil.rmtree(home) shutil.rmtree(home)
_TIMERS.clear() _TIMERS.clear()
+11 -6
View File
@@ -28,7 +28,7 @@ if str(SKILLS) not in sys.path:
from lib_py import pane_lock # noqa: E402 from lib_py import pane_lock # noqa: E402
# a bash holder, as the shim is: fd 9 on the file, the lock taken by the helper, then it sleeps # a bash holder, as the shim is: fd 9 on the file, the lock taken by the helper, then it sleeps
HOLDER = 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9; echo held; sleep "${2:-600}"' HOLDER = 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9; echo held; exec sleep "${2:-600}"'
def _holder(path: Path, seconds: str = "600", **env) -> subprocess.Popen: def _holder(path: Path, seconds: str = "600", **env) -> subprocess.Popen:
@@ -133,9 +133,12 @@ def test_p5_the_wait_knob_is_a_decimal_with_a_floor_of_one(monkeypatch):
def _seed_row(tmp_path, name, epoch, note="auto-registered by monitor", job=None): SERVER = "t50-srv" # the server _create names; a monitor row carries the server it found the pane on
def _seed_row(tmp_path, name, epoch, note="auto-registered by monitor", job=None, server=SERVER):
row = {"name": name, "status": "running", "role": "creator", "herdr_session_created_at": "2026-10-11T00:00:00Z", row = {"name": name, "status": "running", "role": "creator", "herdr_session_created_at": "2026-10-11T00:00:00Z",
"herdr_session_epoch": epoch, "herdr_session": "default", "herdr_server": "default", "herdr_session_epoch": epoch, "herdr_session": server, "herdr_server": server,
"pane": {"index": 0, "pid": 9999, "cmd": "claude", "cmd_full": "claude", "cwd": str(tmp_path)}, "pane": {"index": 0, "pid": 9999, "cmd": "claude", "cmd_full": "claude", "cwd": str(tmp_path)},
"last_visible_status": "running"} "last_visible_status": "running"}
if note: if note:
@@ -151,7 +154,7 @@ def _seed_row(tmp_path, name, epoch, note="auto-registered by monitor", job=None
def _create(tmp_path, name, role="creator"): def _create(tmp_path, name, role="creator"):
return subprocess.run(["bash", str(tmp_path / "skills" / "multi-agent-mux-create" / "scripts" / "create_session.sh"), return subprocess.run(["bash", str(tmp_path / "skills" / "multi-agent-mux-create" / "scripts" / "create_session.sh"),
"--workspace", str(tmp_path), "--agent", "claude", "--role", role, "--session", name, "--workspace", str(tmp_path), "--agent", "claude", "--role", role, "--session", name,
"--no-onboard"], capture_output=True, text=True, cwd=str(tmp_path), timeout=300) "--herdr-session", SERVER, "--no-onboard"], capture_output=True, text=True, cwd=str(tmp_path), timeout=300)
def _rows(yaml_path): def _rows(yaml_path):
@@ -161,7 +164,7 @@ def _rows(yaml_path):
def test_a1_create_replaces_the_monitors_registration_of_its_own_pane(mam_sandbox, mock_herdr, mock_agents): def test_a1_create_replaces_the_monitors_registration_of_its_own_pane(mam_sandbox, mock_herdr, mock_agents):
name = "t50-adopted-creator-claude" name = "t50-adopted-creator-claude"
yaml_path = _seed_row(mam_sandbox, name, int(time.time())) # registered after this create began yaml_path = _seed_row(mam_sandbox, name, int(time.time()) + 60) # registered after this create began
res = _create(mam_sandbox, name, role="Creator") # the monitor guessed "creator" off the name res = _create(mam_sandbox, name, role="Creator") # the monitor guessed "creator" off the name
assert res.returncode == 0, res.stdout + res.stderr assert res.returncode == 0, res.stdout + res.stderr
assert f"NOTE: the monitor registered {name} while it was being created" in res.stdout assert f"NOTE: the monitor registered {name} while it was being created" in res.stdout
@@ -170,7 +173,7 @@ def test_a1_create_replaces_the_monitors_registration_of_its_own_pane(mam_sandbo
assert rows[0].get("last_visible_note") != "auto-registered by monitor" and rows[0].get("start_command") assert rows[0].get("last_visible_note") != "auto-registered by monitor" and rows[0].get("start_command")
@pytest.mark.parametrize("kind", ["real", "old-adoption", "adoption-with-job"]) @pytest.mark.parametrize("kind", ["real", "old-adoption", "adoption-with-job", "other-server"])
def test_a2_another_live_session_of_that_name_is_still_refused(mam_sandbox, mock_herdr, mock_agents, kind): def test_a2_another_live_session_of_that_name_is_still_refused(mam_sandbox, mock_herdr, mock_agents, kind):
name = "t50-taken-creator-claude" name = "t50-taken-creator-claude"
now = int(time.time()) now = int(time.time())
@@ -178,6 +181,8 @@ def test_a2_another_live_session_of_that_name_is_still_refused(mam_sandbox, mock
yaml_path = _seed_row(mam_sandbox, name, now, note=None) yaml_path = _seed_row(mam_sandbox, name, now, note=None)
elif kind == "old-adoption": elif kind == "old-adoption":
yaml_path = _seed_row(mam_sandbox, name, now - 3600) # a pane from before this create yaml_path = _seed_row(mam_sandbox, name, now - 3600) # a pane from before this create
elif kind == "other-server": # review 1e25cf2f F2: names are unique per server
yaml_path = _seed_row(mam_sandbox, name, now + 60, server="different-server")
else: else:
yaml_path = _seed_row(mam_sandbox, name, now, job="deadbeef") yaml_path = _seed_row(mam_sandbox, name, now, job="deadbeef")
res = _create(mam_sandbox, name) res = _create(mam_sandbox, name)