fix(create): one pane claim at a time per herdr server; a create keeps the pane the monitor registered for it
Three `create --onboard` at once into one folder (the control root, so no control-plane prefix reaches the pane and the shim reuses an idle pane): the shim's new-session chooses a pane - an idle one in a workspace on the same cwd, a split, a fresh workspace's root pane - and then claims it with `agent start`; two shims could choose the pane one of them had just made, and the later start failed with agent_pane_busy (one create in three). mam_herdr now runs every new-session call holding one lock per server: a flock on fd 9, opened for the block and taken by lib_py.pane_lock, which also bounds the wait (MAM_PANE_CLAIM_WAIT_S, default 120). It is released with the block or with the process, however it ends. The shim heredoc is unchanged (S11). Other servers' creates are not held. Serialising the claims exposed a second race: a create's end-of-run `reconcile.sh --once` adopted the pane of another create that had not yet written its row, and that create refused its own name as already running (exit 4). Such a row - registered by the monitor, no job, for a pane created after this create began - is now replaced, not refused. Tests: real herdr - shim-level and create-level three-way races into one folder (the real herdr's pane-creating replies held back so the window is hit on every run; both fail on the base with the orchestrator's error), the different-folder test kept; pane_lock unit tests (per-server file, timeout, kill -9 holder, handover, knob); the adoption rule and its three refusals; shim contract h26 (held lock: no pane calls). The herdr box gets a ~/.zshrc: without it every first `agent start` typed into zsh's first-run menu and timed out at 30 s. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
30c25f193c
commit
7844ae2c5f
9 files changed
+480
-27
No files matched your search
@@ -1486,6 +1486,54 @@ _herdr_scope_is_pane_inherited() {
|
|||||||
[ -n "${HERDR_WORKSPACE_ID:-}" ] && [ -n "$pane" ] && [ "${pane%%:*}" = "$HERDR_WORKSPACE_ID" ]
|
[ -n "${HERDR_WORKSPACE_ID:-}" ] && [ -n "$pane" ] && [ "${pane%%:*}" = "$HERDR_WORKSPACE_ID" ]
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# Issue #50 follow-up: one pane claim at a time per herdr server.
|
||||||
|
#
|
||||||
|
# The shim's new-session chooses a pane - an idle one in a workspace on the
|
||||||
|
# same cwd (BUG-4), a split of one, or the root pane of a fresh workspace - and
|
||||||
|
# then claims it with `agent start`. Two shims on one server (several creates
|
||||||
|
# at once into one folder) could choose the same idle pane in between, and the
|
||||||
|
# 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
|
||||||
|
# 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
|
||||||
|
# that ends, so a dead holder leaves nothing behind; the file is the target
|
||||||
|
# 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).
|
||||||
|
#
|
||||||
|
# _mam_pane_claim_target <herdr argv...>: prints the server a new-session call
|
||||||
|
# targets ("" for the default one); rc 1 for any other call.
|
||||||
|
_mam_pane_claim_target() {
|
||||||
|
local sess="${HERDR_SESSION_NAME:-}"
|
||||||
|
while [ "${1:-}" = "-L" ]; do sess="${2:-}"; shift 2 || return 1; done
|
||||||
|
[ "${1:-}" = "new-session" ] || return 1
|
||||||
|
[ "$sess" = "default" ] && sess=""
|
||||||
|
printf '%s\n' "$sess"
|
||||||
|
}
|
||||||
|
|
||||||
|
# _mam_with_pane_claim <server> <cmd...>: run <cmd> holding <server>'s pane lock.
|
||||||
|
_mam_with_pane_claim() {
|
||||||
|
local sess="$1" f=""
|
||||||
|
shift
|
||||||
|
f=$(PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" python3 -m lib_py.pane_lock path --session "$sess") || f=""
|
||||||
|
if [ -z "$f" ]; then
|
||||||
|
echo "ERROR: no pane lock path for herdr session ${sess:-default} (lib_py.pane_lock); not placing this session" >&2
|
||||||
|
return 1
|
||||||
|
fi
|
||||||
|
mkdir -p "$(dirname "$f")"
|
||||||
|
{
|
||||||
|
case "$(PYTHONPATH="$SKILL_DIR:${PYTHONPATH:-}" python3 -m lib_py.pane_lock acquire --fd 9)" in
|
||||||
|
locked) ;;
|
||||||
|
timeout)
|
||||||
|
echo "ERROR: another pane claim on herdr session ${sess:-default} did not end within MAM_PANE_CLAIM_WAIT_S; not placing this session" >&2
|
||||||
|
return 1 ;;
|
||||||
|
*)
|
||||||
|
echo "ERROR: the pane lock of herdr session ${sess:-default} could not be decided (lib_py.pane_lock); not placing this session" >&2
|
||||||
|
return 1 ;;
|
||||||
|
esac
|
||||||
|
"$@"
|
||||||
|
} 9>>"$f"
|
||||||
|
}
|
||||||
|
|
||||||
mam_herdr() {
|
mam_herdr() {
|
||||||
_init_herdr_isolation
|
_init_herdr_isolation
|
||||||
# Built as an argv array and decided inside `if`: this runs under
|
# Built as an argv array and decided inside `if`: this runs under
|
||||||
@@ -1503,6 +1551,11 @@ mam_herdr() {
|
|||||||
echo "mam_herdr: '$1' looks up by name; the caller pane's own workspace ${HERDR_WORKSPACE_ID} is not a scope" >&2
|
echo "mam_herdr: '$1' looks up by name; the caller pane's own workspace ${HERDR_WORKSPACE_ID} is not a scope" >&2
|
||||||
fi
|
fi
|
||||||
fi
|
fi
|
||||||
|
local _claim_sess=""
|
||||||
|
if _claim_sess=$(_mam_pane_claim_target "$@"); then
|
||||||
|
_mam_with_pane_claim "$_claim_sess" ${_scope_strip[@]+"${_scope_strip[@]}"} "$WORKSPACE_ROOT/.mam/shim/herdr" "$@"
|
||||||
|
return
|
||||||
|
fi
|
||||||
${_scope_strip[@]+"${_scope_strip[@]}"} "$WORKSPACE_ROOT/.mam/shim/herdr" "$@"
|
${_scope_strip[@]+"${_scope_strip[@]}"} "$WORKSPACE_ROOT/.mam/shim/herdr" "$@"
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -115,6 +115,8 @@ def atomic_dump_yaml_main():
|
|||||||
d['herdr_sessions'] = []
|
d['herdr_sessions'] = []
|
||||||
|
|
||||||
old_sessions = get_all_sessions_status(d)
|
old_sessions = get_all_sessions_status(d)
|
||||||
|
# a mutation that replaces a row the monitor registered for a pane it
|
||||||
|
# created itself (create_session.sh) drops that name from old_roles
|
||||||
old_roles = {s.get('name'): s.get('role') for s in db_sessions if s.get('role')}
|
old_roles = {s.get('name'): s.get('role') for s in db_sessions if s.get('role')}
|
||||||
|
|
||||||
# --- caller mutation (module scope: sees d, yaml, os, glob, subprocess) ---
|
# --- caller mutation (module scope: sees d, yaml, os, glob, subprocess) ---
|
||||||
|
|||||||
@@ -0,0 +1,75 @@
|
|||||||
|
"""Issue #50 follow-up: one pane claim at a time per herdr server.
|
||||||
|
|
||||||
|
The herdr shim's `new-session` chooses a pane - an idle one in a workspace on
|
||||||
|
the same cwd (BUG-4), a split of one, or the root pane of a fresh workspace -
|
||||||
|
and then claims it with `agent start`. Two shims on one server (several
|
||||||
|
`create` at once into one folder) could choose the same idle pane in between,
|
||||||
|
and the later `agent start` failed with agent_pane_busy. The shim now holds
|
||||||
|
one lock per server from the choice to the claim.
|
||||||
|
|
||||||
|
pane_lock.py path --session S the lock file of herdr session S, under the
|
||||||
|
HOME whose socket the session uses
|
||||||
|
pane_lock.py acquire --fd N flock(LOCK_EX) on the inherited fd N; prints
|
||||||
|
`locked`, or `timeout` once MAM_PANE_CLAIM_WAIT_S
|
||||||
|
(default 120, minimum 1) has passed
|
||||||
|
|
||||||
|
The lock is a BSD flock, which belongs to the open file the shim holds on fd 9:
|
||||||
|
it outlives this helper and ends with the shim's process, however that ends
|
||||||
|
(exit, kill -9), so a dead holder leaves nothing to steal. flock(1) is not on
|
||||||
|
macOS; fcntl.flock is. Stdlib only, Python 3.9.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import argparse
|
||||||
|
import fcntl
|
||||||
|
import os
|
||||||
|
import sys
|
||||||
|
import time
|
||||||
|
from typing import List, Mapping, Optional
|
||||||
|
|
||||||
|
from lib_py import ints
|
||||||
|
|
||||||
|
DEFAULT_WAIT_S = 120
|
||||||
|
|
||||||
|
|
||||||
|
def lock_path(session: Optional[str], env: Mapping[str, str]) -> str:
|
||||||
|
home = env.get("HOME") or env.get("HOME_DIR") or os.path.expanduser("~")
|
||||||
|
return os.path.join(home, ".local", "state", "mam", "pane-lock", (session or "default") + ".lock")
|
||||||
|
|
||||||
|
|
||||||
|
def wait_s(env: Mapping[str, str]) -> int:
|
||||||
|
return ints.positive_int(env.get("MAM_PANE_CLAIM_WAIT_S"), 1) or DEFAULT_WAIT_S
|
||||||
|
|
||||||
|
|
||||||
|
def acquire(fd: int, wait: float) -> str:
|
||||||
|
"""`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
|
||||||
|
stretch or cut it."""
|
||||||
|
deadline = time.monotonic() + wait
|
||||||
|
while True:
|
||||||
|
try:
|
||||||
|
fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
|
||||||
|
return "locked"
|
||||||
|
except OSError:
|
||||||
|
if time.monotonic() >= deadline:
|
||||||
|
return "timeout"
|
||||||
|
time.sleep(0.2)
|
||||||
|
|
||||||
|
|
||||||
|
def main(argv: Optional[List[str]] = None) -> int:
|
||||||
|
p = argparse.ArgumentParser(prog="pane_lock.py")
|
||||||
|
sub = p.add_subparsers(dest="cmd", required=True)
|
||||||
|
s = sub.add_parser("path")
|
||||||
|
s.add_argument("--session", default="")
|
||||||
|
s = sub.add_parser("acquire")
|
||||||
|
s.add_argument("--fd", type=int, required=True)
|
||||||
|
a = p.parse_args(argv)
|
||||||
|
if a.cmd == "path":
|
||||||
|
print(lock_path(a.session, os.environ))
|
||||||
|
else:
|
||||||
|
print(acquire(a.fd, wait_s(os.environ)))
|
||||||
|
return 0
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
sys.exit(main())
|
||||||
@@ -178,13 +178,15 @@ bash .agents/skills/multi-agent-mux-create/scripts/create_session.sh --workspace
|
|||||||
|
|
||||||
`MAM_ONBOARD_WAIT_S` (default 600): how long `--onboard` waits for the onboarding job to end after the `started` handshake.
|
`MAM_ONBOARD_WAIT_S` (default 600): how long `--onboard` waits for the onboarding job to end after the `started` handshake.
|
||||||
|
|
||||||
|
`MAM_PANE_CLAIM_WAIT_S` (default 120): creates (and resumes) on one herdr server choose and claim their pane one at a time; how long one waits for another's claim to end before giving up (exit 1, nothing spawned).
|
||||||
|
|
||||||
| Exit | Meaning |
|
| Exit | Meaning |
|
||||||
|---|---|
|
|---|---|
|
||||||
| 0 | Created; or `--dry-run` |
|
| 0 | Created; or `--dry-run` |
|
||||||
| 1 | Pre-flight failure, nothing was spawned: workspace not a directory, herdr not installed, agent CLI not in `PATH`, agent not authenticated. After spawn: the task brief could not be written or the instruction injection failed (the session is rolled back) |
|
| 1 | Pre-flight failure, nothing was spawned: workspace not a directory, herdr not installed, agent CLI not in `PATH`, agent not authenticated. After spawn: the task brief could not be written or the instruction injection failed (the session is rolled back) |
|
||||||
| 2 | Bad arguments; control-plane binding refused for a worktree; whitespace in a control-plane path that must be forwarded |
|
| 2 | Bad arguments; control-plane binding refused for a worktree; whitespace in a control-plane path that must be forwarded |
|
||||||
| 3 | A herdr session of that name already exists (`multi-agent-mux-resume`, or `multi-agent-mux-stop` first) |
|
| 3 | A herdr session of that name already exists (`multi-agent-mux-resume`, or `multi-agent-mux-stop` first) |
|
||||||
| 4 | The YAML already has a `running` row of that name. The YAML write raises it after spawn, and the herdr session is rolled back |
|
| 4 | The YAML already has a `running` row of that name. The YAML write raises it after spawn, and the herdr session is rolled back. A row the monitor registered for this very pane while it was being created is replaced instead |
|
||||||
| 42 | The TUI never became ready and the process is gone, or it is alive and `MAM_KILL_ON_READY_TIMEOUT=1` (rolled back) |
|
| 42 | The TUI never became ready and the process is gone, or it is alive and `MAM_KILL_ON_READY_TIMEOUT=1` (rolled back) |
|
||||||
| 43 | The rollback could not kill the herdr session; it is still running |
|
| 43 | The rollback could not kill the herdr session; it is still running |
|
||||||
| 44 | The onboarding job ended with `error` (or was cancelled); the session is kept (Issue #50) |
|
| 44 | The onboarding job ended with `error` (or was cancelled); the session is kept (Issue #50) |
|
||||||
|
|||||||
@@ -527,6 +527,20 @@ sessions = d.setdefault('herdr_sessions', [])
|
|||||||
# P0-D: 같은 이름 엔트리가 status=running 이면만 거부. terminated/archived 는
|
# P0-D: 같은 이름 엔트리가 status=running 이면만 거부. terminated/archived 는
|
||||||
# 재사용 가능 — 낡은 엔트리를 제거하고 새로 append (create -> delete -> create).
|
# 재사용 가능 — 낡은 엔트리를 제거하고 새로 append (create -> delete -> create).
|
||||||
running_same = [s for s in sessions if s.get('name') == name and s.get('status') == 'running']
|
running_same = [s for s in sessions if s.get('name') == name and s.get('status') == 'running']
|
||||||
|
# Issue #50 follow-up: a reconcile pass (another create's end-of-run `--once`,
|
||||||
|
# the subscriber's cycle) can adopt the pane this create started moments ago,
|
||||||
|
# before this row lands. A running row of this name that the monitor
|
||||||
|
# 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
|
||||||
|
# preflight refused an older session of this name - so it is replaced, not
|
||||||
|
# refused as a second live session.
|
||||||
|
def _adopted_own(s):
|
||||||
|
return (s.get('last_visible_note') == 'auto-registered by monitor' and not s.get('delegate_job_id')
|
||||||
|
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):
|
||||||
|
print(f"NOTE: the monitor registered {name} while it was being created; that row is replaced by this one", flush=True)
|
||||||
|
running_same = []
|
||||||
|
old_roles.pop(name, None) # its role was read off the name; --role is the fact (atomic_yaml's role guard)
|
||||||
if running_same:
|
if running_same:
|
||||||
print(f"ERROR: {name} already running in agent-sessions.yaml", flush=True)
|
print(f"ERROR: {name} already running in agent-sessions.yaml", flush=True)
|
||||||
raise SystemExit(4)
|
raise SystemExit(4)
|
||||||
|
|||||||
@@ -88,8 +88,8 @@
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
"skill": "multi-agent-mux-create",
|
"skill": "multi-agent-mux-create",
|
||||||
"start": 202,
|
"start": 204,
|
||||||
"end": 206,
|
"end": 208,
|
||||||
"lang": "bash",
|
"lang": "bash",
|
||||||
"sha": "38836d31bf38",
|
"sha": "38836d31bf38",
|
||||||
"class": "c",
|
"class": "c",
|
||||||
@@ -103,8 +103,8 @@
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
"skill": "multi-agent-mux-create",
|
"skill": "multi-agent-mux-create",
|
||||||
"start": 228,
|
"start": 230,
|
||||||
"end": 250,
|
"end": 252,
|
||||||
"lang": "bash",
|
"lang": "bash",
|
||||||
"sha": "d90cfd2555fd",
|
"sha": "d90cfd2555fd",
|
||||||
"class": "b",
|
"class": "b",
|
||||||
@@ -617,7 +617,7 @@
|
|||||||
"other"
|
"other"
|
||||||
],
|
],
|
||||||
"passthrough": {
|
"passthrough": {
|
||||||
"4": "multi-agent-mux-create/scripts/create_session.sh:532"
|
"4": "multi-agent-mux-create/scripts/create_session.sh:546"
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -604,3 +604,37 @@ kill "$_mam_server_pid" 2>/dev/null || true
|
|||||||
f"daemon child stayed in the caller's process group (not detached): {fields}"
|
f"daemon child stayed in the caller's process group (not detached): {fields}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
def test_h26_new_session_claims_its_pane_under_the_servers_lock(mam_sandbox, mock_herdr, mock_agents):
|
||||||
|
"""Issue #50 follow-up: from the pane choice to `agent start` the shim holds
|
||||||
|
one lock per herdr server (lib_py.pane_lock, a flock on fd 9). While
|
||||||
|
another shim holds it, this one waits MAM_PANE_CLAIM_WAIT_S and then gives
|
||||||
|
up without creating, splitting or starting anything; once it is free the
|
||||||
|
same call goes through as before."""
|
||||||
|
import glob
|
||||||
|
import os
|
||||||
|
tmp_path = mam_sandbox
|
||||||
|
res = _run_lib(tmp_path, f'herdr new-session -d -s "test-creator-claude" -c "{tmp_path}" "claude --dangerously-skip-permissions"')
|
||||||
|
assert res.returncode == 0, res.stderr
|
||||||
|
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
|
||||||
|
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", locks[0]], stdout=subprocess.PIPE, text=True,
|
||||||
|
env=dict(os.environ, PYTHONPATH=str(tmp_path / ".agents" / "skills")))
|
||||||
|
try:
|
||||||
|
assert holder.stdout.readline().strip() == "locked"
|
||||||
|
os.environ["MAM_PANE_CLAIM_WAIT_S"] = "1"
|
||||||
|
res = _run_lib(tmp_path, f'herdr new-session -d -s "test-reviewer-claude" -c "{tmp_path}" "claude --dangerously-skip-permissions"')
|
||||||
|
assert res.returncode == 1 and "did not end within MAM_PANE_CLAIM_WAIT_S" in res.stderr, (res.returncode, res.stderr)
|
||||||
|
new_calls = [c[2:] if c[:1] == ["--session"] else c for c in _load_mock_state(mock_herdr).get("calls", [])[n_calls:]]
|
||||||
|
assert not [c for c in new_calls if c[:2] in (["workspace", "create"], ["pane", "split"], ["agent", "start"])], new_calls
|
||||||
|
assert not [n for n in _load_mock_state(mock_herdr).get("agents", {}) if "test-reviewer-claude" in n]
|
||||||
|
finally:
|
||||||
|
os.environ.pop("MAM_PANE_CLAIM_WAIT_S", None)
|
||||||
|
holder.kill()
|
||||||
|
holder.wait()
|
||||||
|
res = _run_lib(tmp_path, f'herdr new-session -d -s "test-reviewer-claude" -c "{tmp_path}" "claude --dangerously-skip-permissions"')
|
||||||
|
assert res.returncode == 0, res.stderr
|
||||||
|
assert [n for n in _load_mock_state(mock_herdr).get("agents", {}) if "test-reviewer-claude" in n]
|
||||||
@@ -196,6 +196,9 @@ def _make_box(sess: str) -> Box:
|
|||||||
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)
|
||||||
(box.ctl / ".agents" / "skills").symlink_to(SKILLS)
|
(box.ctl / ".agents" / "skills").symlink_to(SKILLS)
|
||||||
|
# a fresh HOME without ~/.zshrc puts zsh's first-run menu in every new
|
||||||
|
# pane: the first `agent start` would type into it and time out (30 s)
|
||||||
|
(home / ".zshrc").touch()
|
||||||
(box.bin / "claude").write_text(FAKE_CLAUDE)
|
(box.bin / "claude").write_text(FAKE_CLAUDE)
|
||||||
(box.bin / "claude").chmod(0o755)
|
(box.bin / "claude").chmod(0o755)
|
||||||
box.yaml.write_text("herdr_sessions:\n" + _row(sess, ws=str(box.ws)))
|
box.yaml.write_text("herdr_sessions:\n" + _row(sess, ws=str(box.ws)))
|
||||||
@@ -608,32 +611,71 @@ def _drive_onboardings(box: Box, names: list, delays: list, result: dict):
|
|||||||
_after(delay, box.publish, jid, "completed", "Onboarding complete; aligned with role creator")
|
_after(delay, box.publish, jid, "completed", "Onboarding complete; aligned with role creator")
|
||||||
|
|
||||||
|
|
||||||
|
def _hold_pane_creation(b: Box, seconds: int) -> None:
|
||||||
|
"""The real herdr on the box's PATH, with the reply of a pane-creating call
|
||||||
|
(workspace create, pane split) held back `seconds`. The shim's window -
|
||||||
|
a pane exists idle, `agent start` has not claimed it yet - is the one the
|
||||||
|
orchestrator hit; this makes it long enough for a shim started a moment
|
||||||
|
later to walk into it on every run, not one run in three."""
|
||||||
|
(b.bin / "herdr").write_text(f'''#!/usr/bin/env bash
|
||||||
|
case " $* " in
|
||||||
|
*" workspace create "*|*" pane split "*)
|
||||||
|
out=$("{HERDR}" "$@"); rc=$?
|
||||||
|
sleep {seconds}
|
||||||
|
printf '%s\\n' "$out"; exit $rc ;;
|
||||||
|
esac
|
||||||
|
exec "{HERDR}" "$@"
|
||||||
|
''')
|
||||||
|
(b.bin / "herdr").chmod(0o755)
|
||||||
|
|
||||||
|
|
||||||
|
def _three_creates(b: Box, names: list, workspaces: list, stagger: float = 0.0):
|
||||||
|
"""create --onboard x3 at once in one server; (outcome by name, driver result, elapsed)."""
|
||||||
|
procs = []
|
||||||
|
for name, ws in zip(names, workspaces):
|
||||||
|
if procs and stagger:
|
||||||
|
time.sleep(stagger)
|
||||||
|
out = tempfile.TemporaryFile("w+", dir=str(b.home))
|
||||||
|
err = tempfile.TemporaryFile("w+", dir=str(b.home))
|
||||||
|
procs.append((name, subprocess.Popen(
|
||||||
|
["bash", str(CREATE_SH), "--workspace", str(ws), "--agent", "claude", "--role", "creator",
|
||||||
|
"--session", name, "--onboard", "--herdr-session", b.sess],
|
||||||
|
stdout=out, stderr=err, text=True, cwd=str(b.ctl), env=b.env(MAM_ONBOARD_WAIT_S="120")), out, err))
|
||||||
|
t0 = time.time()
|
||||||
|
result = {}
|
||||||
|
drv = threading.Thread(target=_drive_onboardings, args=(b, names, [10, 15, 20], result), daemon=True)
|
||||||
|
drv.start()
|
||||||
|
outcome = {}
|
||||||
|
for name, proc, out, err in procs:
|
||||||
|
rc = proc.wait(timeout=240)
|
||||||
|
out.seek(0); err.seek(0)
|
||||||
|
outcome[name] = (rc, out.read(), err.read())
|
||||||
|
elapsed = time.time() - t0
|
||||||
|
drv.join(timeout=5)
|
||||||
|
return outcome, result, elapsed
|
||||||
|
|
||||||
|
|
||||||
|
def _agent_panes(b: Box) -> dict:
|
||||||
|
"""agent name -> (pane_id, pane cwd) from the server."""
|
||||||
|
panes = {p["pane_id"]: p for p in json.loads(b.herdr("pane", "list").stdout)["result"]["panes"]}
|
||||||
|
out = {}
|
||||||
|
for a in json.loads(b.herdr("agent", "list").stdout)["result"]["agents"]:
|
||||||
|
pid = a.get("pane_id") or (a.get("pane") or {}).get("pane_id")
|
||||||
|
out[a["name"]] = (pid, (panes.get(pid) or {}).get("cwd"))
|
||||||
|
return out
|
||||||
|
|
||||||
|
|
||||||
def test_a_three_concurrent_creates_with_onboarding(local_mqtt_broker):
|
def test_a_three_concurrent_creates_with_onboarding(local_mqtt_broker):
|
||||||
b = _make_box(f"t50-{uuid.uuid4().hex[:8]}")
|
b = _make_box(f"t50-{uuid.uuid4().hex[:8]}")
|
||||||
b.yaml.write_text("herdr_sessions: []\n")
|
b.yaml.write_text("herdr_sessions: []\n")
|
||||||
names = [f"t50-{i}-creator-claude" for i in (1, 2, 3)]
|
names = [f"t50-{i}-creator-claude" for i in (1, 2, 3)]
|
||||||
procs = []
|
|
||||||
try:
|
try:
|
||||||
for i, name in enumerate(names, 1):
|
workspaces = []
|
||||||
ws = b.home / f"ws{i}"
|
for i in (1, 2, 3):
|
||||||
ws.mkdir()
|
(b.home / f"ws{i}").mkdir()
|
||||||
out = tempfile.TemporaryFile("w+", dir=str(b.home))
|
workspaces.append(b.home / f"ws{i}")
|
||||||
err = tempfile.TemporaryFile("w+", dir=str(b.home))
|
outcome, result, elapsed = _three_creates(b, names, workspaces)
|
||||||
procs.append((name, subprocess.Popen(
|
t0 = time.time() - elapsed
|
||||||
["bash", str(CREATE_SH), "--workspace", str(ws), "--agent", "claude", "--role", "creator",
|
|
||||||
"--session", name, "--onboard", "--herdr-session", b.sess],
|
|
||||||
stdout=out, stderr=err, text=True, cwd=str(b.ctl), env=b.env(MAM_ONBOARD_WAIT_S="120")), out, err))
|
|
||||||
t0 = time.time()
|
|
||||||
result = {}
|
|
||||||
drv = threading.Thread(target=_drive_onboardings, args=(b, names, [10, 15, 20], result), daemon=True)
|
|
||||||
drv.start()
|
|
||||||
outcome = {}
|
|
||||||
for name, proc, out, err in procs:
|
|
||||||
rc = proc.wait(timeout=240)
|
|
||||||
out.seek(0); err.seek(0)
|
|
||||||
outcome[name] = (rc, out.read(), err.read())
|
|
||||||
elapsed = time.time() - t0
|
|
||||||
drv.join(timeout=5)
|
|
||||||
assert all(o[0] == 0 for o in outcome.values()), {n: (o[0], o[2][-1500:]) for n, o in outcome.items()}
|
assert all(o[0] == 0 for o in outcome.values()), {n: (o[0], o[2][-1500:]) for n, o in outcome.items()}
|
||||||
# The three start-ups overlap, and the onboarding waits do too: once the
|
# The three start-ups overlap, and the onboarding waits do too: once the
|
||||||
# last prompt was delivered (its `started`), the whole run ends with the
|
# last prompt was delivered (its `started`), the whole run ends with the
|
||||||
@@ -655,6 +697,75 @@ def test_a_three_concurrent_creates_with_onboarding(local_mqtt_broker):
|
|||||||
_teardown(b)
|
_teardown(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_a3_three_concurrent_creates_in_one_folder_take_distinct_panes(local_mqtt_broker):
|
||||||
|
"""The same --workspace for all three - the control root itself, where
|
||||||
|
several reviewers are made at once: no control-plane binding is forwarded
|
||||||
|
to the pane, so the shim reuses an idle pane when it sees one (BUG-4). The
|
||||||
|
pane choice and its `agent start` are serialised per server, so no two
|
||||||
|
creates claim one pane (agent_pane_busy)."""
|
||||||
|
b = _make_box(f"t50-{uuid.uuid4().hex[:8]}")
|
||||||
|
b.yaml.write_text("herdr_sessions: []\n")
|
||||||
|
_hold_pane_creation(b, 8)
|
||||||
|
names = [f"t50-{i}-creator-claude" for i in (1, 2, 3)]
|
||||||
|
try:
|
||||||
|
assert b.sh("herdr agent list").returncode == 0 # the server is up: the creates race on panes only
|
||||||
|
outcome, result, elapsed = _three_creates(b, names, [b.ctl] * 3, stagger=2.0)
|
||||||
|
assert all(o[0] == 0 for o in outcome.values()), {n: (o[0], o[2][-1500:]) for n, o in outcome.items()}
|
||||||
|
assert "agent_pane_busy" not in "".join(o[1] + o[2] for o in outcome.values())
|
||||||
|
for name in names:
|
||||||
|
jid = result["jobs"][name]
|
||||||
|
assert f"onboarding completed: job {jid} (" in outcome[name][1], outcome[name][1]
|
||||||
|
assert b.record(jid)["status"] == "completed"
|
||||||
|
rows = re.findall(r"^- name: (\S+)", b.yaml.read_text(), re.M)
|
||||||
|
assert sorted(rows) == sorted(names), rows
|
||||||
|
panes = _agent_panes(b)
|
||||||
|
assert sorted(panes) == sorted(names), panes
|
||||||
|
assert len({p for p, _c in panes.values()}) == 3, ("two agents on one pane", panes)
|
||||||
|
assert all(Path(c).resolve() == b.ctl.resolve() for _p, c in panes.values()), panes
|
||||||
|
assert len(b.server_pids()) == 1
|
||||||
|
finally:
|
||||||
|
_teardown(b)
|
||||||
|
|
||||||
|
|
||||||
|
def test_s_three_concurrent_new_sessions_in_one_folder_take_distinct_panes():
|
||||||
|
"""The shim alone, no create around it: three `new-session` into one cwd on
|
||||||
|
one running server at once. Before the pane claim was serialised, two of
|
||||||
|
them chose the same idle pane and the later `agent start` failed with
|
||||||
|
agent_pane_busy."""
|
||||||
|
b = _make_box(f"t50-{uuid.uuid4().hex[:8]}")
|
||||||
|
_hold_pane_creation(b, 2)
|
||||||
|
names = [f"t50-{i}-creator-claude" for i in (1, 2, 3)]
|
||||||
|
try:
|
||||||
|
assert b.sh("herdr agent list").returncode == 0 # the server is up before the race
|
||||||
|
procs = []
|
||||||
|
for name in names:
|
||||||
|
if procs:
|
||||||
|
time.sleep(0.7)
|
||||||
|
out = tempfile.TemporaryFile("w+", dir=str(b.home))
|
||||||
|
err = tempfile.TemporaryFile("w+", dir=str(b.home))
|
||||||
|
procs.append((name, subprocess.Popen(
|
||||||
|
["bash", "-c", f'source "{SKILLS}/lib.sh"\nherdr new-session -d -s {name} -c "{b.ws}" '
|
||||||
|
'"claude --dangerously-skip-permissions"'],
|
||||||
|
stdout=out, stderr=err, text=True, cwd=str(b.ctl), env=b.env()), out, err))
|
||||||
|
outcome = {}
|
||||||
|
for name, proc, out, err in procs:
|
||||||
|
rc = proc.wait(timeout=120)
|
||||||
|
out.seek(0); err.seek(0)
|
||||||
|
outcome[name] = (rc, out.read(), err.read())
|
||||||
|
assert all(o[0] == 0 for o in outcome.values()), {n: (o[0], o[2][-800:]) for n, o in outcome.items()}
|
||||||
|
panes = _agent_panes(b)
|
||||||
|
assert sorted(panes) == sorted(names), panes
|
||||||
|
assert len({p for p, _c in panes.values()}) == 3, ("two agents on one pane", panes)
|
||||||
|
assert all(Path(c).resolve() == b.ws.resolve() for _p, c in panes.values()), panes
|
||||||
|
for name in names:
|
||||||
|
deadline = time.time() + 30
|
||||||
|
while time.time() < deadline and b.agent_status(name) != "idle":
|
||||||
|
time.sleep(0.5)
|
||||||
|
assert b.agent_status(name) == "idle", (name, b.herdr("agent", "list").stdout)
|
||||||
|
finally:
|
||||||
|
_teardown(b)
|
||||||
|
|
||||||
|
|
||||||
def _git(repo: Path, *args):
|
def _git(repo: Path, *args):
|
||||||
subprocess.run(["git", "-c", "user.email=t@example.com", "-c", "user.name=t", *args], cwd=str(repo), check=True,
|
subprocess.run(["git", "-c", "user.email=t@example.com", "-c", "user.name=t", *args], cwd=str(repo), check=True,
|
||||||
capture_output=True, text=True)
|
capture_output=True, text=True)
|
||||||
|
|||||||
@@ -0,0 +1,162 @@
|
|||||||
|
"""Issue #50 follow-up: the herdr shim claims one pane at a time per server.
|
||||||
|
|
||||||
|
- P: lib_py.pane_lock - the lock file per server, the acquire decision and its
|
||||||
|
bound, a lock that ends with its holder's process (no stale lock after
|
||||||
|
kill -9), the wait knob.
|
||||||
|
- H: the race itself, on a real herdr server: test_issue50_dispatch_herdr.py
|
||||||
|
(test_s_three_concurrent_new_sessions_in_one_folder_take_distinct_panes,
|
||||||
|
test_a3_three_concurrent_creates_in_one_folder_take_distinct_panes).
|
||||||
|
- the shim's use of the lock against the mock herdr: test_herdr_shim_contract.py h26.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import json
|
||||||
|
import os
|
||||||
|
import signal
|
||||||
|
import subprocess
|
||||||
|
import sys
|
||||||
|
import time
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
||||||
|
SKILLS = REPO_ROOT / ".agents" / "skills"
|
||||||
|
if str(SKILLS) not in sys.path:
|
||||||
|
sys.path.insert(0, str(SKILLS))
|
||||||
|
|
||||||
|
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
|
||||||
|
HOLDER = 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9; echo held; sleep "${2:-600}"'
|
||||||
|
|
||||||
|
|
||||||
|
def _holder(path: Path, seconds: str = "600", **env) -> subprocess.Popen:
|
||||||
|
p = subprocess.Popen(["bash", "-c", HOLDER, "holder", str(path), seconds], stdout=subprocess.PIPE, text=True,
|
||||||
|
env=dict(os.environ, PYTHONPATH=str(SKILLS), **env))
|
||||||
|
assert p.stdout.readline().strip() == "locked"
|
||||||
|
assert p.stdout.readline().strip() == "held"
|
||||||
|
return p
|
||||||
|
|
||||||
|
|
||||||
|
def _acquire(path: Path, **env) -> subprocess.CompletedProcess:
|
||||||
|
return subprocess.run(["bash", "-c", 'exec 9>>"$1"; python3 -m lib_py.pane_lock acquire --fd 9', "x", str(path)],
|
||||||
|
capture_output=True, text=True, env=dict(os.environ, PYTHONPATH=str(SKILLS), **env), timeout=60)
|
||||||
|
|
||||||
|
|
||||||
|
def test_p1_the_lock_file_is_per_server_under_the_sessions_home(tmp_path):
|
||||||
|
env = {"HOME": str(tmp_path)}
|
||||||
|
assert pane_lock.lock_path("t50-x", env) == str(tmp_path / ".local/state/mam/pane-lock/t50-x.lock")
|
||||||
|
assert pane_lock.lock_path("", env) == str(tmp_path / ".local/state/mam/pane-lock/default.lock")
|
||||||
|
assert pane_lock.lock_path("t50-x", {"HOME_DIR": str(tmp_path / "h")}).startswith(str(tmp_path / "h"))
|
||||||
|
assert pane_lock.lock_path("t50-x", env) != pane_lock.lock_path("t50-y", env)
|
||||||
|
res = subprocess.run([sys.executable, "-m", "lib_py.pane_lock", "path", "--session", "t50-x"], capture_output=True,
|
||||||
|
text=True, env=dict(os.environ, PYTHONPATH=str(SKILLS), HOME=str(tmp_path)), timeout=60)
|
||||||
|
assert res.stdout.strip() == pane_lock.lock_path("t50-x", env), res.stderr
|
||||||
|
|
||||||
|
|
||||||
|
def test_p2_a_free_lock_is_taken_at_once_and_a_held_one_times_out(tmp_path):
|
||||||
|
lock = tmp_path / "s.lock"
|
||||||
|
t0 = time.time()
|
||||||
|
assert _acquire(lock).stdout.strip() == "locked" and time.time() - t0 < 5
|
||||||
|
holder = _holder(lock)
|
||||||
|
try:
|
||||||
|
t0 = time.time()
|
||||||
|
res = _acquire(lock, MAM_PANE_CLAIM_WAIT_S="2")
|
||||||
|
assert res.stdout.strip() == "timeout", res.stderr
|
||||||
|
assert 1.5 <= time.time() - t0 < 10
|
||||||
|
assert _acquire(tmp_path / "other-server.lock", MAM_PANE_CLAIM_WAIT_S="2").stdout.strip() == "locked"
|
||||||
|
finally:
|
||||||
|
holder.kill()
|
||||||
|
holder.wait()
|
||||||
|
|
||||||
|
|
||||||
|
def test_p3_the_lock_ends_with_its_holders_process(tmp_path):
|
||||||
|
lock = tmp_path / "s.lock"
|
||||||
|
holder = _holder(lock)
|
||||||
|
os.kill(holder.pid, signal.SIGKILL)
|
||||||
|
holder.wait()
|
||||||
|
t0 = time.time()
|
||||||
|
assert _acquire(lock, MAM_PANE_CLAIM_WAIT_S="30").stdout.strip() == "locked"
|
||||||
|
assert time.time() - t0 < 5, "nothing stale to wait out"
|
||||||
|
|
||||||
|
|
||||||
|
def test_p4_a_waiter_gets_the_lock_when_the_holder_ends(tmp_path):
|
||||||
|
lock = tmp_path / "s.lock"
|
||||||
|
holder = _holder(lock, "2")
|
||||||
|
t0 = time.time()
|
||||||
|
res = _acquire(lock, MAM_PANE_CLAIM_WAIT_S="30")
|
||||||
|
assert res.stdout.strip() == "locked"
|
||||||
|
assert 1 <= time.time() - t0 < 10
|
||||||
|
holder.wait()
|
||||||
|
|
||||||
|
|
||||||
|
def test_p5_the_wait_knob_is_a_decimal_with_a_floor_of_one(monkeypatch):
|
||||||
|
assert pane_lock.wait_s({}) == 120
|
||||||
|
assert pane_lock.wait_s({"MAM_PANE_CLAIM_WAIT_S": "7"}) == 7
|
||||||
|
assert pane_lock.wait_s({"MAM_PANE_CLAIM_WAIT_S": "0700"}) == 700
|
||||||
|
for bad in ("0", "abc", "", "-1", " "):
|
||||||
|
assert pane_lock.wait_s({"MAM_PANE_CLAIM_WAIT_S": bad}) == 120, bad
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# A - create keeps its own pane when the monitor registered it first. With
|
||||||
|
# pane claims serialised, one create can reach its end-of-run reconcile while
|
||||||
|
# another is between `agent start` and its row write; the adopted row is that
|
||||||
|
# create's own pane, not a second live session of its name.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
|
||||||
|
def _seed_row(tmp_path, name, epoch, note="auto-registered by monitor", job=None):
|
||||||
|
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",
|
||||||
|
"pane": {"index": 0, "pid": 9999, "cmd": "claude", "cmd_full": "claude", "cwd": str(tmp_path)},
|
||||||
|
"last_visible_status": "running"}
|
||||||
|
if note:
|
||||||
|
row["last_visible_note"] = note
|
||||||
|
if job:
|
||||||
|
row["delegate_job_id"] = job
|
||||||
|
yaml_path = tmp_path / ".mam" / "agent-sessions.yaml"
|
||||||
|
yaml_path.parent.mkdir(parents=True, exist_ok=True)
|
||||||
|
yaml_path.write_text("herdr_sessions:\n- " + json.dumps(row) + "\n")
|
||||||
|
return yaml_path
|
||||||
|
|
||||||
|
|
||||||
|
def _create(tmp_path, name, role="creator"):
|
||||||
|
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,
|
||||||
|
"--no-onboard"], capture_output=True, text=True, cwd=str(tmp_path), timeout=300)
|
||||||
|
|
||||||
|
|
||||||
|
def _rows(yaml_path):
|
||||||
|
import yaml
|
||||||
|
return yaml.safe_load(yaml_path.read_text())["herdr_sessions"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_a1_create_replaces_the_monitors_registration_of_its_own_pane(mam_sandbox, mock_herdr, mock_agents):
|
||||||
|
name = "t50-adopted-creator-claude"
|
||||||
|
yaml_path = _seed_row(mam_sandbox, name, int(time.time())) # registered after this create began
|
||||||
|
res = _create(mam_sandbox, name, role="Creator") # the monitor guessed "creator" off the name
|
||||||
|
assert res.returncode == 0, res.stdout + res.stderr
|
||||||
|
assert f"NOTE: the monitor registered {name} while it was being created" in res.stdout
|
||||||
|
rows = [r for r in _rows(yaml_path) if r["name"] == name]
|
||||||
|
assert len(rows) == 1 and rows[0]["status"] == "running" and rows[0]["role"] == "Creator"
|
||||||
|
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"])
|
||||||
|
def test_a2_another_live_session_of_that_name_is_still_refused(mam_sandbox, mock_herdr, mock_agents, kind):
|
||||||
|
name = "t50-taken-creator-claude"
|
||||||
|
now = int(time.time())
|
||||||
|
if kind == "real":
|
||||||
|
yaml_path = _seed_row(mam_sandbox, name, now, note=None)
|
||||||
|
elif kind == "old-adoption":
|
||||||
|
yaml_path = _seed_row(mam_sandbox, name, now - 3600) # a pane from before this create
|
||||||
|
else:
|
||||||
|
yaml_path = _seed_row(mam_sandbox, name, now, job="deadbeef")
|
||||||
|
res = _create(mam_sandbox, name)
|
||||||
|
assert res.returncode == 4, res.stdout + res.stderr
|
||||||
|
assert f"ERROR: {name} already running in agent-sessions.yaml" in res.stdout
|
||||||
|
rows = [r for r in _rows(yaml_path) if r["name"] == name]
|
||||||
|
assert len(rows) == 1 and rows[0]["status"] == "running"
|
||||||
Reference in new issue
Block a user