fix(id_order): server_not_running is unknown and cap 0 asks no herdr (Issue #63)
PR #69 review round 3 (rebuttal overruled; codex r2 applied as written). - A failed `agent list`, herdr's server_not_running included, is unknown and keeps an orphaned marker; only a successful list without the name prunes. - No probe floor: probes get only the time left until the wait deadline, so MAM_ID_ORDER_WAIT_S=0 asks herdr nothing (unknown, kept) and the refusal says so. - The timeout message reports the last definite answer of this wait, so an alive orphan whose final probe ran out of time still reads as alive. - Tests follow: server_not_running and cap 0 keep the marker and refuse; delayed-probe bound is the cap itself; probing tests use a small cap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
8f64174b6b
commit
7cac6f613e
2 files changed
+35
-34
No files matched your search
@@ -30,9 +30,6 @@ from lib_py.agents.session_ids import canonical_cwd, claiming_rows, own_id
|
|||||||
from lib_py.state import load_state_json
|
from lib_py.state import load_state_json
|
||||||
|
|
||||||
DEFAULT_WAIT_S = 600
|
DEFAULT_WAIT_S = 600
|
||||||
# herdr probes of dead-owner markers share this budget past the wait deadline,
|
|
||||||
# so MAM_ID_ORDER_WAIT_S=0 still gets one look at whether an orphan is gone.
|
|
||||||
PROBE_FLOOR_S = 2.0
|
|
||||||
PROBE_TIMEOUT_S = 30.0
|
PROBE_TIMEOUT_S = 30.0
|
||||||
|
|
||||||
|
|
||||||
@@ -58,25 +55,12 @@ def pid_alive(pid: int) -> bool:
|
|||||||
return False
|
return False
|
||||||
|
|
||||||
|
|
||||||
def _server_not_running(*texts: str) -> bool:
|
|
||||||
"""herdr's own error envelope for a server that is down (lib.sh ISSUE-9)."""
|
|
||||||
for text in texts:
|
|
||||||
for line in [text] + text.splitlines():
|
|
||||||
try:
|
|
||||||
doc = json.loads(line)
|
|
||||||
except ValueError:
|
|
||||||
continue
|
|
||||||
err = doc.get("error") if isinstance(doc, dict) else None
|
|
||||||
if isinstance(err, dict) and err.get("code") == "server_not_running":
|
|
||||||
return True
|
|
||||||
return False
|
|
||||||
|
|
||||||
|
|
||||||
def herdr_session_state(server: str, session: str, timeout: float) -> Optional[bool]:
|
def herdr_session_state(server: str, session: str, timeout: float) -> Optional[bool]:
|
||||||
"""Is `session` live on herdr server `server`: True, False, or None when
|
"""Is `session` live on herdr server `server`: True, False, or None when
|
||||||
herdr gave no authoritative answer. False only from a parsed `agent list`
|
herdr gave no authoritative answer. False only from a successful, parsed
|
||||||
without the name or herdr's server_not_running error; a failed, timed-out
|
`agent list` without the name; a failed (server down included), timed-out
|
||||||
or unparseable lookup is None. Never starts a server (MAM_NO_AUTOSTART)."""
|
or unparseable lookup, or no time left, is None. Never starts a server
|
||||||
|
(MAM_NO_AUTOSTART)."""
|
||||||
if timeout <= 0:
|
if timeout <= 0:
|
||||||
return None
|
return None
|
||||||
skills_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
|
skills_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
|
||||||
@@ -91,7 +75,7 @@ def herdr_session_state(server: str, session: str, timeout: float) -> Optional[b
|
|||||||
except (OSError, subprocess.SubprocessError):
|
except (OSError, subprocess.SubprocessError):
|
||||||
return None
|
return None
|
||||||
if res.returncode != 0:
|
if res.returncode != 0:
|
||||||
return False if _server_not_running(res.stdout, res.stderr) else None
|
return None
|
||||||
try:
|
try:
|
||||||
agents = json.loads(res.stdout)["result"]["agents"]
|
agents = json.loads(res.stdout)["result"]["agents"]
|
||||||
except (ValueError, KeyError, TypeError):
|
except (ValueError, KeyError, TypeError):
|
||||||
@@ -294,13 +278,17 @@ def wait(cwd: str, agent: str, session: str, owner_pid: int,
|
|||||||
os.makedirs(dpath, exist_ok=True)
|
os.makedirs(dpath, exist_ok=True)
|
||||||
lock_file = os.path.join(dpath, "lock")
|
lock_file = os.path.join(dpath, "lock")
|
||||||
deadline = time.monotonic() + cap
|
deadline = time.monotonic() + cap
|
||||||
probe_until = max(deadline, time.monotonic() + PROBE_FLOOR_S)
|
|
||||||
announced = False
|
announced = False
|
||||||
row_ref = {"name": session}
|
row_ref = {"name": session}
|
||||||
probe = pane_state or herdr_session_state
|
probe = pane_state or herdr_session_state
|
||||||
|
|
||||||
|
answers: Dict[str, bool] = {} # last definite answer per session, for the message
|
||||||
|
|
||||||
def bounded_probe(server: str, sess: str) -> Optional[bool]:
|
def bounded_probe(server: str, sess: str) -> Optional[bool]:
|
||||||
return probe(server, sess, probe_until - time.monotonic())
|
live = probe(server, sess, deadline - time.monotonic())
|
||||||
|
if live is not None:
|
||||||
|
answers[sess] = live
|
||||||
|
return live
|
||||||
|
|
||||||
while True:
|
while True:
|
||||||
with open(lock_file, "a", encoding="utf-8") as lf:
|
with open(lock_file, "a", encoding="utf-8") as lf:
|
||||||
@@ -344,8 +332,11 @@ def wait(cwd: str, agent: str, session: str, owner_pid: int,
|
|||||||
for m in markers:
|
for m in markers:
|
||||||
if m.get("_orphan") and m.get("session") in blks:
|
if m.get("_orphan") and m.get("session") in blks:
|
||||||
srv = m.get("herdr_server") or "default"
|
srv = m.get("herdr_server") or "default"
|
||||||
seen = ("its herdr session is alive" if m["_orphan"] == "alive"
|
alive = m["_orphan"] == "alive" or answers.get(m["session"]) is True
|
||||||
|
seen = ("its herdr session is alive" if alive
|
||||||
else "herdr could not tell whether its session is gone")
|
else "herdr could not tell whether its session is gone")
|
||||||
|
if not alive and cap == 0:
|
||||||
|
seen += " (MAM_ID_ORDER_WAIT_S=0 leaves no time to ask herdr)"
|
||||||
print(
|
print(
|
||||||
f"ERROR: {m['session']}'s create/resume exited before recording it, "
|
f"ERROR: {m['session']}'s create/resume exited before recording it, "
|
||||||
f"and {seen}; close it with: "
|
f"and {seen}; close it with: "
|
||||||
|
|||||||
@@ -1165,12 +1165,13 @@ def test_id_order_create_waits_for_orphan_herdr_session_of_a_killed_create(
|
|||||||
["bash", str(create_sh), "--workspace", str(ws), "--agent", "agy",
|
["bash", str(create_sh), "--workspace", str(ws), "--agent", "agy",
|
||||||
"--role", "reviewer", "--session", name_b, "--no-onboard"],
|
"--role", "reviewer", "--session", name_b, "--no-onboard"],
|
||||||
capture_output=True, text=True, cwd=str(mam_sandbox),
|
capture_output=True, text=True, cwd=str(mam_sandbox),
|
||||||
env=dict(os.environ, MAM_ID_ORDER_WAIT_S="0"), timeout=120,
|
env=dict(os.environ, MAM_ID_ORDER_WAIT_S="3"), timeout=120,
|
||||||
)
|
)
|
||||||
|
|
||||||
res = create_b()
|
res = create_b()
|
||||||
assert res.returncode == 1, res.stdout + res.stderr
|
assert res.returncode == 1, res.stdout + res.stderr
|
||||||
assert name_a in res.stderr and "kill-session -t " + name_a in res.stderr
|
assert name_a in res.stderr and "kill-session -t " + name_a in res.stderr
|
||||||
|
assert "its herdr session is alive" in res.stderr
|
||||||
assert name_b not in _read_rows(mam_sandbox)
|
assert name_b not in _read_rows(mam_sandbox)
|
||||||
assert name_b not in json.loads(Path(mock_herdr).read_text()).get("agents", {})
|
assert name_b not in json.loads(Path(mock_herdr).read_text()).get("agents", {})
|
||||||
|
|
||||||
@@ -1186,9 +1187,9 @@ def test_id_order_create_waits_for_orphan_herdr_session_of_a_killed_create(
|
|||||||
|
|
||||||
|
|
||||||
def test_id_order_herdr_probes_stay_within_the_wait_cap(tmp_path, monkeypatch):
|
def test_id_order_herdr_probes_stay_within_the_wait_cap(tmp_path, monkeypatch):
|
||||||
"""PR #69 review r2 P2: probes of dead-owner markers get the time left until
|
"""PR #69 review r2 P2: probes of dead-owner markers get only the time left
|
||||||
max(deadline, start + PROBE_FLOOR_S), shared by all markers, so a slow herdr
|
until the wait deadline, shared by all markers, so a slow herdr cannot hold
|
||||||
cannot hold the wait (and its lock) past the cap; an expired probe is unknown."""
|
the wait (and its lock) past the cap; cap 0 asks nothing (unknown, kept)."""
|
||||||
from lib_py.agents import id_order
|
from lib_py.agents import id_order
|
||||||
|
|
||||||
home = tmp_path / "home"
|
home = tmp_path / "home"
|
||||||
@@ -1219,17 +1220,20 @@ def test_id_order_herdr_probes_stay_within_the_wait_cap(tmp_path, monkeypatch):
|
|||||||
rc = id_order.wait(str(ws), "agy", "new-agy", os.getpid(), yaml_path=str(yaml_path),
|
rc = id_order.wait(str(ws), "agy", "new-agy", os.getpid(), yaml_path=str(yaml_path),
|
||||||
home=str(home), wait_s_val=cap, poll=0.05, pane_state=slow_probe)
|
home=str(home), wait_s_val=cap, poll=0.05, pane_state=slow_probe)
|
||||||
elapsed = time.monotonic() - t0
|
elapsed = time.monotonic() - t0
|
||||||
bound = max(cap, id_order.PROBE_FLOOR_S)
|
bound = cap
|
||||||
assert rc == 1, cap
|
assert rc == 1, cap
|
||||||
assert elapsed < bound + 0.6, (cap, elapsed)
|
assert elapsed < bound + 0.6, (cap, elapsed)
|
||||||
assert budgets and max(budgets) <= bound + 0.01, (cap, budgets)
|
assert budgets and max(budgets) <= bound + 0.01, (cap, budgets)
|
||||||
|
if cap == 0:
|
||||||
|
assert elapsed < 0.3 and all(b <= 0 for b in budgets), (elapsed, budgets)
|
||||||
assert len(list(Path(dpath).glob("orphan-*.json"))) == 2, cap
|
assert len(list(Path(dpath).glob("orphan-*.json"))) == 2, cap
|
||||||
|
|
||||||
|
|
||||||
def test_id_order_real_probe_against_mock_herdr_fails_closed(mam_sandbox, mock_herdr):
|
def test_id_order_real_probe_against_mock_herdr_fails_closed(mam_sandbox, mock_herdr):
|
||||||
"""PR #69 review r2 P1: lib.sh mam_id_order_wait with the real probe (sandbox
|
"""PR #69 review r2 P1: lib.sh mam_id_order_wait with the real probe (sandbox
|
||||||
copy, mock herdr). A listed orphan blocks; an unlisted one is pruned; a failed
|
copy, mock herdr). A listed orphan blocks; an unlisted one is pruned; a failed
|
||||||
`agent list` keeps the marker and refuses; herdr's server_not_running prunes."""
|
`agent list` (server_not_running included) keeps the marker and refuses;
|
||||||
|
with MAM_ID_ORDER_WAIT_S=0 herdr is not asked and the marker is kept."""
|
||||||
from lib_py.agents import id_order
|
from lib_py.agents import id_order
|
||||||
|
|
||||||
ws = mam_sandbox / "probe-ws"
|
ws = mam_sandbox / "probe-ws"
|
||||||
@@ -1249,14 +1253,14 @@ def test_id_order_real_probe_against_mock_herdr_fails_closed(mam_sandbox, mock_h
|
|||||||
state.update(kw)
|
state.update(kw)
|
||||||
Path(mock_herdr).write_text(json.dumps(state))
|
Path(mock_herdr).write_text(json.dumps(state))
|
||||||
|
|
||||||
def wait_b():
|
def wait_b(cap="2"):
|
||||||
for f in Path(dpath).glob(f"{name_b}.json"):
|
for f in Path(dpath).glob(f"{name_b}.json"):
|
||||||
f.unlink()
|
f.unlink()
|
||||||
id_order.write_marker(dpath, name_a, dead.pid, str(ws), "agy", herdr_server="probe-ws")
|
id_order.write_marker(dpath, name_a, dead.pid, str(ws), "agy", herdr_server="probe-ws")
|
||||||
return subprocess.run(
|
return subprocess.run(
|
||||||
["bash", "-c", 'source "$1" && mam_id_order_wait "$2" agy "$3"', "_", str(lib), str(ws), name_b],
|
["bash", "-c", 'source "$1" && mam_id_order_wait "$2" agy "$3"', "_", str(lib), str(ws), name_b],
|
||||||
capture_output=True, text=True, cwd=str(mam_sandbox), timeout=120,
|
capture_output=True, text=True, cwd=str(mam_sandbox), timeout=120,
|
||||||
env=dict(os.environ, MAM_ID_ORDER_WAIT_S="0", HERDR_SESSION_NAME="probe-ws"),
|
env=dict(os.environ, MAM_ID_ORDER_WAIT_S=cap, HERDR_SESSION_NAME="probe-ws"),
|
||||||
)
|
)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
@@ -1273,10 +1277,16 @@ def test_id_order_real_probe_against_mock_herdr_fails_closed(mam_sandbox, mock_h
|
|||||||
|
|
||||||
set_mock(force_error_code="server_not_running")
|
set_mock(force_error_code="server_not_running")
|
||||||
res = wait_b()
|
res = wait_b()
|
||||||
assert res.returncode == 0, res.stderr
|
assert res.returncode == 1, res.stderr
|
||||||
assert not marker_a.exists()
|
assert "herdr could not tell whether its session is gone" in res.stderr
|
||||||
|
assert marker_a.exists()
|
||||||
|
|
||||||
set_mock()
|
set_mock()
|
||||||
|
res = wait_b(cap="0")
|
||||||
|
assert res.returncode == 1, res.stderr
|
||||||
|
assert "MAM_ID_ORDER_WAIT_S=0 leaves no time to ask herdr" in res.stderr
|
||||||
|
assert marker_a.exists()
|
||||||
|
|
||||||
res = wait_b()
|
res = wait_b()
|
||||||
assert res.returncode == 0, res.stderr
|
assert res.returncode == 0, res.stderr
|
||||||
assert not marker_a.exists()
|
assert not marker_a.exists()
|
||||||
|
|||||||
Reference in new issue
Block a user