Repository navigation
fix(ag2-sparrow): a Hatch task parked for the owner's go posts one line and reports held, not stuck - #5828
Conversation
…ne and reports held, not stuck
When a Hatch worker parks a task on purpose because it waits for its owner's
go, the room saw silence and the keepalive health row flagged the task as
task-not-picked-up after 15 minutes, indistinguishable from a stuck task.
- ag2_sparrow/task_hold.py: a generic, machine-readable hold marker
(results/.task-holds/<tid>.json: reason slug + since), placed O_EXCL so
only the first caller acts; pruned once the task is archived.
- `ag2-sparrow-hatch hold <result_file>`: places the owner-go marker and
queues exactly one notice ("Waiting for my owner's go on this.") as a
proactive file routed to the task's room/thread through the existing
drain; a second hold posts nothing; the task's result slot stays open for
the real answer. A failed queue releases the marker so a retry still posts.
- keepalive health row: held tasks no longer count as not picked up; with
nothing else wrong the row is healthy with reason task-held-for-owner.
A held task never masks an unheld stuck one beside it.
- worker_note tells the worker to run hold once for an item waiting on the
owner's go.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
packages/ag2-sparrow/ag2_sparrow/adapters/hatch/cli.pyLines 480-488 480 lock = _lock_file(state_dir, instance)
481 text = bounded_file_read.read_regular(str(lock), _LOCK_MAX_BYTES, follow=False)[0]
482 try:
483 holder = json.loads(text) if text is not None else None
! 484 except ValueError:
485 return None
486 ok = isinstance(holder, dict) and holder.get("host") == workspace_lock._host_label()
487 return holder if ok else NoneLines 489-498 489
490 def _file_id(path: Path) -> "tuple[int, int] | None":
491 try:
492 info = os.lstat(path)
! 493 except OSError:
! 494 return None
495 return info.st_dev, info.st_ino
496
497
498 def lock_snapshot(state_dir: Path, instance: str) -> "tuple[dict | None, tuple[int, int] | None]":Lines 511-520 511 if not isinstance(lock, dict):
512 return False
513 try:
514 same = os.path.realpath(lock.get("workspace")) == os.path.realpath(Path(state_dir).parent)
! 515 except (TypeError, ValueError):
! 516 return False
517 now = time.time()
518 acquired = bounded_number(lock.get("acquired_at"), 1, now)
519 beat = bounded_number(lock.get("heartbeat_at"), 1, now)
520 return (same and lock.get("role") == _lock_role(instance)Lines 538-546 538 try:
539 os.kill(pid, 0)
540 except ProcessLookupError:
541 return True
! 542 except (OSError, OverflowError):
543 return False
544 stat = _proc_stat(pid)
545 return stat[stat.rfind(b")") + 1:].split()[:1] == [b"Z"]Lines 1201-1209 1201 for _ in range(_LINE_SCANS):
1202 state = _scan_line(result_dir, notice, tid, body.strip())
1203 if state is not None:
1204 return state
! 1205 return "unknown"
1206
1207
1208 def _line_items(result_dir: Path, notice: Path) -> "list[dict] | None":
1209 """The proactive outbox's items for the line; None when they cannot be read (no proof)."""packages/ag2-sparrow/ag2_sparrow/adapters/hatch/keepalive.pyLines 276-284 276 if owner == UNIT_UNREACHABLE:
277 return False, (f"{_cli.service_path()} is installed but systemctl --user did not answer; "
278 "no detached Sparrow is started beside that service")
279 if bound is not None and owner != "ours":
! 280 return False, f"{_cli.SERVICE_NAME} is no longer this install's unit; nothing was restarted"
281 if owner == "ours":
282 # A unit left failed by its start limit stays down until reset.
283 props = _cli._unit_props()
284 if props is None:Lines 294-302 294 # needs reset-failed is left to the next pass's down-listener restart.
295 r = _cli._systemctl(("--user", "start", "ag2-sparrow.service"))
296 if _cli._ok(r):
297 return True, f"systemctl --user start {_cli.SERVICE_NAME}"
! 298 return False, f"systemctl --user start {_cli.SERVICE_NAME} failed ({_cli._why(r)})"
299 _cli._systemctl(("--user", "reset-failed", "ag2-sparrow.service"))
300 r = _cli._systemctl(("--user", "restart", "ag2-sparrow.service"))
301 if _cli._ok(r):
302 return True, f"systemctl --user restart {_cli.SERVICE_NAME}"Lines 392-400 392 is signalled without that handle or that proof."""
393 try:
394 handle = _pidfd_open(pid)
395 except ProcessLookupError:
! 396 return True
397 except OSError as e:
398 print(f"error: no stable process handle for pid {pid} (pidfd_open: {diagnostics.describe(e)}); "
399 "it was not signalled", file=sys.stderr)
400 return FalseLines 758-767 758 code, detail = _cli.check_sparrow(state, instance=instance)
759 try:
760 ts = bounded_number(json.loads(_cli.sparrow_status_path(state, instance).read_text(
761 encoding="utf-8"))["ts"], 0, 1e11)
! 762 except (OSError, ValueError, KeyError, TypeError):
! 763 ts = None
764 return code == _cli.SPARROW_OK and (since is None or (ts is not None and ts >= int(since))), detail
765
766
767 def _await_connected(state: Path, instance: str, since: "float | None") -> int:Lines 782-791 782 def _status_snapshot(state: Path, instance: str) -> "dict | None":
783 """Sparrow's status file, read once; None when it cannot be read as an object."""
784 try:
785 status = json.loads(_cli.sparrow_status_path(state, instance).read_text(encoding="utf-8"))
! 786 except (OSError, ValueError):
! 787 return None
788 return status if isinstance(status, dict) else None
789
790
791 def _successor(cfg: dict, old: "tuple[int, str | None]", since: float) -> "tuple[int | None, str]":Lines 803-811 803 if start is None or not proven_sparrow(cfg, pid, lock):
804 return None, f"pid {pid} holds the lock but is not proven to be the Sparrow that took it"
805 gen = (lock or {}).get("proc_start")
806 if not isinstance(gen, str) or not gen or process_start(pid) != gen:
! 807 return None, f"pid {pid} is not proven to be the generation its lock names (proc_start)"
808 status = _status_snapshot(state, instance) or {}
809 if status.get("pid") != pid or status.get("start") != start or status.get("proc_start") != gen:
810 return None, f"the status file was not written by Sparrow pid {pid}"
811 at = bounded_number(status.get("written_at"), 0, 1e11)packages/ag2-sparrow/ag2_sparrow/agent_identity.pyLines 94-103 94
95
96 try:
97 import fcntl
! 98 except ImportError: # Windows: no lane record; Hatch, the only reader, is POSIX-only
! 99 fcntl = None
100 LANE_SUPPORTED = fcntl is not None
101 _LANE_MAX_BYTES = 4096
102 # A record is a heartbeat: the live drain rewrites it at least every LANE_REFRESH_S, and a reader
103 # treats one older than LANE_FRESH_S (or dated past LANE_SKEW_S ahead) as missing.Lines 115-123 115 def _auth_dir(state_dir, create: bool) -> "int | None":
116 """An fd on `<state>/auth`, opened without following a symlink. Absent: None, or with `create`
117 it is made again a few times and then OSError, so no caller ever gets None to mean the CWD."""
118 if not LANE_SUPPORTED:
! 119 raise OSError("no lane record on this platform")
120 path = Path(state_dir) / "auth"
121 for _ in range(3 if create else 1):
122 if create:
123 path.mkdir(parents=True, exist_ok=True)Lines 125-137 125 st = os.lstat(path)
126 except FileNotFoundError:
127 continue # removed after the mkdir
128 if not stat.S_ISDIR(st.st_mode):
! 129 raise OSError(f"{path} is a symlink or not a directory")
130 fd = os.open(path, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW)
131 if os.fstat(fd).st_ino != st.st_ino:
! 132 os.close(fd)
! 133 raise OSError(f"{path} changed while it was opened")
134 return fd
135 if create:
136 raise OSError(f"{path} kept disappearing while it was created")
137 return NoneLines 150-160 150 for attempt in range(5):
151 try:
152 fd = os.open(name, os.O_RDWR | os.O_CREAT | os.O_NOFOLLOW, 0o600, dir_fd=dfd)
153 break
! 154 except FileNotFoundError: # APFS: racing O_CREATs of one name can see ENOENT
! 155 if attempt == 4:
! 156 raise
157 try:
158 fcntl.flock(fd, fcntl.LOCK_EX)
159 yield
160 finally:Lines 203-221 203 in place (which needs no free space); OSError only when all three fail."""
204 try:
205 os.unlink(name, dir_fd=dfd)
206 return
! 207 except FileNotFoundError:
! 208 return
! 209 except OSError:
! 210 pass
! 211 try:
! 212 _write_at(dfd, name, {"instance": instance or "", "retracted": True})
! 213 return
! 214 except OSError:
! 215 pass
! 216 fd = os.open(name, os.O_WRONLY | os.O_TRUNC | os.O_NOFOLLOW, dir_fd=dfd)
! 217 os.close(fd)
218
219
220 def _poison_at(dfd: int, name: str) -> None:
221 """Leave the record empty in place, or create it empty: it reads invalid, never absent, so aLines 252-260 252 to a reused pid), a timestamp and, when given, the proactive `send_lock` protocol the drain
253 honours. On a failed write the record is emptied in the same locked step, so it reads invalid,
254 and OSError is raised either way."""
255 if valid_pid(os.getpid()) is None:
! 256 raise OSError(f"pid {os.getpid()} is outside the pid domain readers accept")
257 record = {"agent_id": (agent_id or "").strip(), "instance": instance or "",
258 "pid": os.getpid(), "start": own_start(), "ts": time.time(),
259 **({"send_lock": send_lock} if send_lock is not None else {})}
260 dfd = _auth_dir(state_dir, create=True)Lines 276-284 276 checked and removed under the lane lock. Never raises."""
277 with contextlib.suppress(OSError):
278 dfd = _auth_dir(state_dir, create=False)
279 if dfd is None:
! 280 return
281 try:
282 with _lane_locked(dfd, instance):
283 if pid is not None:
284 rec = _read_at(dfd, _lane_name(instance))[0] or {}packages/ag2-sparrow/ag2_sparrow/delivery_core/backend_c.pyLines 278-286 278 def release(self, token: ClaimToken) -> bool:
279 parts = token.incarnation.split(SEP)
280 if len(parts) != TOKEN_PARTS or parts[1] != _safe_component(token.worker) \
281 or _safe_key(token.item_id) != parts[0]:
! 282 return False
283 key, src = parts[0], self.root / INFLIGHT / token.incarnation
284 with self._lock(key):
285 if not src.exists():
286 return FalseLines 285-293 285 if not src.exists():
286 return False
287 # Back to the single ready slot, no attempt counted; a racing re-publish wins it.
288 if not _move(src, self._d(READY) / key) and src.exists():
! 289 self._quarantine(src, key, "collision", str(time.time_ns()))
290 return True
291
292 def _attempts_path(self, key: str) -> Path:
293 return self._d(ATTEMPTS) / keypackages/ag2-sparrow/ag2_sparrow/delivery_core/contract.pyLines 253-261 253
254 def release(self, token: ClaimToken) -> bool:
255 """Retire this incarnation's claim with no outcome and no attempt recorded: the item is
256 claimable again as it was. A stale token changes nothing. True = it owned the claim."""
! 257 ...
258
259 def attempts(self, item_id: str) -> int:
260 """Delivery attempts recorded for this item (retry accounting)."""
261 ...packages/ag2-sparrow/ag2_sparrow/delivery_destination.pyLines 13-22 13 from pathlib import Path
14
15 try:
16 import fcntl
! 17 except ImportError: # Windows
! 18 fcntl = None
19
20 from .outbox import ITEMS_DIR, list_items, valid_pid
21 from .proactive_routing import PROACTIVE_OUTBOX_DIRNAME
22 from .result_markers import parse_markersLines 56-65 56 dispatch-time recheck and send, and a producer holds it while it commits an answer or a
57 message about a task; acquisition is the linearization point. Released on every exit; SendLockUnavailable when it cannot be taken
58 within `wait` seconds. A platform without flock (no Hatch there) skips it."""
59 if fcntl is None:
! 60 yield
! 61 return
62 wait = PRODUCER_LOCK_WAIT_S if wait is None else wait
63 try:
64 lock_dir = Path(result_dir) / SEND_LOCK_DIR
65 lock_dir.mkdir(mode=0o700, exist_ok=True)Lines 75-84 75 except BlockingIOError:
76 if time.monotonic() >= deadline:
77 raise SendLockUnavailable(f"held by another sender past {wait:g}s") from None
78 time.sleep(_LOCK_POLL_S)
! 79 except OSError as e:
! 80 raise SendLockUnavailable(str(e)) from e
81 yield
82 finally:
83 os.close(fd)Lines 87-95 87 """The pid of a drain claim name, `<stem>.sending.<pid>` or `<stem>.sending.<pid>.<ns>`; None
88 for any other name, so no reader treats a file the drain neither creates nor recovers as a claim."""
89 parts = name.rsplit(CLAIM_INFIX, 1)
90 if len(parts) != 2 or not parts[1].isascii():
! 91 return None
92 pid_text, dot, ns = parts[1].partition(".")
93 if not pid_text.isdigit() or (dot and not ns.isdigit()):
94 return None
95 pid = valid_pid(int(pid_text))packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.pyLines 2865-2876 2865 cut.cancel()
2866
2867
2868 def _shut_sockets(socks) -> None:
! 2869 for sock in list(socks):
! 2870 with contextlib.suppress(OSError):
2871 # The base method: an SSLSocket's own shutdown drops its TLS object under a blocked read.
! 2872 socket.socket.shutdown(sock, socket.SHUT_RDWR)
2873
2874
2875 def _left(deadline: float, cap: float) -> float:
2876 """The seconds to the deadline, at most `cap`; TimeoutError once it has passed."""Lines 2887-2898 2887 cap = cap if isinstance(cap, (int, float)) and cap > 0 else _REQ_TIMEOUT_S
2888 found: "list" = []
2889
2890 def resolve():
! 2891 try:
! 2892 found.append(socket.getaddrinfo(host, port, 0, socket.SOCK_STREAM))
! 2893 except OSError as e:
! 2894 found.append(e)
2895 t = threading.Thread(target=resolve, daemon=True)
2896 t.start()
2897 t.join(_left(deadline, cap))
2898 if not found:Lines 5773-5781 5773 record does not say it or is due a heartbeat, so Hatch judges rooms by it without reading the
5774 credential env. A drain that cannot write lets the record go stale, and Hatch then refuses."""
5775 identity = _reenroll_identity()
5776 if not agent_identity.LANE_SUPPORTED:
! 5777 return identity
5778 rec = agent_identity.published_lane_record(_STATE, GATEWAY_INSTANCE) or {}
5779 if rec.get("pid") != os.getpid() or rec.get("agent_id") != identity.strip() \
5780 or rec.get("send_lock") != _delivery_destination.SEND_LOCK_PROTOCOL \
5781 or time.time() - rec.get("ts", 0) >= agent_identity.LANE_REFRESH_S:Lines 5795-5804 5795
5796
5797 def _retract_own_lane() -> None:
5798 """At clean exit: invalidate the lane record only if it is still the one this process wrote."""
! 5799 if "pid" in _LANE_OWNED:
! 5800 agent_identity.retract_lane(_STATE, GATEWAY_INSTANCE, _LANE_OWNED["pid"], _LANE_OWNED["ts"])
5801
5802
5803 def _own_homeserver() -> str:
5804 """The server this lane's agent lives on, from its live identity;packages/ag2-sparrow/tests/test_hatch_adapter.pyLines 2699-2711 2699 return self.starts.get(pid) or real_start(pid)
2700
2701 def proc(pid, name):
2702 if name == "cmdline" and not Path("/proc").is_dir():
! 2703 if pid in self.argvs:
! 2704 return b"\0".join(a.encode() for a in self.argvs[pid]) + b"\0"
2705 # No /proc: ps gives a spawned fixture's argv, split on spaces.
! 2706 out = subprocess.run(["ps", "-o", "command=", "-p", str(pid)], capture_output=True).stdout
! 2707 return b"\0".join(out.split()) + b"\0" if out.strip() else None
2708 return real_proc(pid, name)
2709 for target, attr, fn in ((agent_identity, "start_time", start), (task_io, "start_time", start),
2710 (keepalive, "_proc", proc)):
2711 p = mock.patch.object(target, attr, fn)Lines 2712-2725 2712 p.start()
2713 self.addCleanup(p.stop)
2714 if not hasattr(os, "pidfd_open"):
2715 # No pidfd (macOS): a stand-in handle, so stop_holder's proof runs; the signal is by pid.
! 2716 real_stop = keepalive._stop
! 2717 for attr, fn in (("_pidfd_open", lambda pid: ("handle", pid)),
2718 ("_stop", lambda pid, handle=None: real_stop(pid))):
! 2719 p = mock.patch.object(keepalive, attr, fn)
! 2720 p.start()
! 2721 self.addCleanup(p.stop)
2722
2723 def started_ago(self, proc, ago):
2724 self.starts[proc.pid] = time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime(time.time() - ago))Lines 5143-5153 5143 "done", "--wait", "0"], self.env()))
5144 return ready(path)
5145
5146 def req(method, path, payload=None, timeout=None, **_kw):
! 5147 if method == "POST":
! 5148 posts.append(payload)
! 5149 return {"ok": True, "event_id": "$evt"}
5150 with mock.patch.object(gb, "RESULTS_DIR", self.results), \
5151 mock.patch.object(gb, "ARCHIVE_RESULTS_DIR", self.results / "archive"), \
5152 mock.patch.object(gb, "UNDELIVERABLE_RESULTS_DIR", self.results / "undelivered"), \
5153 mock.patch.object(gb, "PROACTIVE_ROOM", ""), mock.patch.object(gb, "_req", req), \Lines 5172-5181 5172 self.assertEqual(self.hold(str(self.result_file)).returncode, hatch_cli.REPLY_POSTED)
5173 posts, answers, workers = [], [], []
5174
5175 def answer():
! 5176 with mock.patch.object(outbox, "_activity_completed", return_value=None):
! 5177 answers.append(run_main(hatch_cli.main, ["reply", str(self.result_file), "--body",
5178 "done", "--wait", "0"], self.env()))
5179
5180 def req(method, path, payload=None, timeout=None, **_kw):
5181 if method == "POST":Lines 5240-5258 5240 at_mark, go, posts, codes = threading.Event(), threading.Event(), [], {}
5241 real_mark, real_place = task_hold.mark_notice, task_hold.place
5242
5243 def mark(*a, **k):
! 5244 at_mark.set()
! 5245 go.wait(10)
! 5246 return real_mark(*a, **k)
5247
5248 def place(*a, **k):
! 5249 token = real_place(*a, **k)
! 5250 self.drain_pass(posts)
! 5251 return token
5252
5253 def run(name, argv):
! 5254 codes[name] = hatch_cli.main(argv)
5255 with mock.patch.dict(os.environ, self.env(), clear=True), \
5256 mock.patch.object(hatch_cli, "_installed_config", return_value={}), \
5257 mock.patch.object(task_hold, "mark_notice", mark), \
5258 mock.patch.object(task_hold, "place", place), \Lines 5307-5315 5307 import http.server
5308
5309 class Quiet(handler):
5310 def log_message(self, *a):
! 5311 pass
5312 server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Quiet)
5313 threading.Thread(target=server.serve_forever, daemon=True).start()
5314 self.addCleanup(server.server_close)
5315 self.addCleanup(server.shutdown)Lines 5320-5334 5320 posts = []
5321
5322 class Ok(http.server.BaseHTTPRequestHandler):
5323 def do_POST(self):
! 5324 posts.append(self.rfile.read(int(self.headers.get("Content-Length") or 0)))
! 5325 body = b'{"ok": true, "event_id": "$local"}'
! 5326 self.send_response(200)
! 5327 self.send_header("Content-Type", "application/json")
! 5328 self.send_header("Content-Length", str(len(body)))
! 5329 self.end_headers()
! 5330 self.wfile.write(body)
5331 url = self.gateway(Ok)
5332 self.add_task(self.TID, "$ev1")
5333 self.assertEqual(self.hold(str(self.result_file)).returncode, hatch_cli.REPLY_POSTED)
5334 self.drain_with(None, url=url)Lines 5480-5488 5480
5481 def failing(path, body):
5482 if path == self.result_file:
5483 raise OSError(28, "No space left on device")
! 5484 return real(path, body)
5485 with mock.patch.object(hatch_cli, "_publish_once", failing):
5486 r = self.reply_now()
5487 self.assertEqual(r.returncode, 2, r.stderr)
5488 self.assertIn("nothing written", r.stderr)Lines 5578-5600 5578 dribbled = threading.Event()
5579
5580 class Dribble(http.server.BaseHTTPRequestHandler):
5581 def do_POST(self):
! 5582 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 5583 self.send_response(200)
! 5584 self.send_header("Content-Length", "64")
! 5585 self.end_headers()
! 5586 for _ in range(40): # about 8 s, each byte inside the socket timeout
! 5587 try:
! 5588 self.wfile.write(b" ")
! 5589 self.wfile.flush()
! 5590 except OSError:
! 5591 break
! 5592 time.sleep(0.2)
! 5593 dribbled.set()
5594
5595 def log_message(self, *a):
! 5596 pass
5597 server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Dribble)
5598 threading.Thread(target=server.serve_forever, daemon=True).start()
5599 self.addCleanup(server.server_close)
5600 self.addCleanup(server.shutdown)Lines 5612-5627 5612 import http.server
5613
5614 class SlowHeaders(http.server.BaseHTTPRequestHandler):
5615 def do_POST(self):
! 5616 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 5617 for byte in b"HTTP/1.1 200 OK\r\nX-Slow: " + b"a" * 60:
! 5618 try:
! 5619 self.wfile.write(bytes([byte]))
! 5620 self.wfile.flush()
! 5621 except OSError:
! 5622 return
! 5623 time.sleep(0.1)
5624 url = self.gateway(SlowHeaders)
5625 self.add_task(self.TID, "$ev1")
5626 self.assertEqual(self.hold(str(self.result_file)).returncode, hatch_cli.REPLY_POSTED)
5627 started = time.monotonic()Lines 5637-5655 5637 from ag2_sparrow import remote_gateway_bridge as gb
5638
5639 class SlowError(http.server.BaseHTTPRequestHandler):
5640 def do_POST(self):
! 5641 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 5642 self.send_response(400)
! 5643 self.send_header("Content-Length", "64")
! 5644 self.end_headers()
! 5645 for _ in range(30):
! 5646 try:
! 5647 self.wfile.write(b" ")
! 5648 self.wfile.flush()
! 5649 except OSError:
! 5650 return
! 5651 time.sleep(0.1)
5652 url = self.gateway(SlowError)
5653 started = time.monotonic()
5654 with mock.patch.multiple(gb, URL=url, TOKEN="t"):
5655 with self.assertRaises(urllib.error.HTTPError) as caught:Lines 5695-5710 5695 import http.server
5696
5697 class SlowHeaders(http.server.BaseHTTPRequestHandler):
5698 def do_POST(self):
! 5699 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 5700 for byte in b"HTTP/1.1 200 OK\r\nX-Slow: " + b"a" * 60:
! 5701 try:
! 5702 self.wfile.write(bytes([byte]))
! 5703 self.wfile.flush()
! 5704 except OSError:
! 5705 return
! 5706 time.sleep(0.1)
5707 log_message = lambda *a: None # noqa: E731
5708 url, client = self.tls_gateway(SlowHeaders)
5709 raised, took = self.bounded_req(url, 0.3, client)
5710 self.assertIsInstance(raised, TimeoutError, repr(raised))Lines 5714-5732 5714 import http.server
5715
5716 class SlowError(http.server.BaseHTTPRequestHandler):
5717 def do_POST(self):
! 5718 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 5719 self.send_response(400)
! 5720 self.send_header("Content-Length", "64")
! 5721 self.end_headers()
! 5722 for _ in range(30):
! 5723 try:
! 5724 self.wfile.write(b" ")
! 5725 self.wfile.flush()
! 5726 except OSError:
! 5727 return
! 5728 time.sleep(0.1)
5729 log_message = lambda *a: None # noqa: E731
5730 url, client = self.tls_gateway(SlowError)
5731 raised, took = self.bounded_req(url, 0.3, client)
5732 self.assertIsInstance(raised, urllib.error.HTTPError, repr(raised))Lines 5761-5770 5761 def test_the_deadline_covers_name_resolution(self):
5762 import socket as sk
5763
5764 def slow_resolver(*a, **k):
! 5765 time.sleep(0.8)
! 5766 return [(sk.AF_INET, sk.SOCK_STREAM, 6, "", ("127.0.0.1", 9))]
5767 with mock.patch.object(sk, "getaddrinfo", slow_resolver):
5768 raised, took = self.bounded_req("http://slow-dns.example:9", 0.2)
5769 self.assertIsInstance(raised, TimeoutError, repr(raised))
5770 self.assertLess(took, 0.5, "a resolver that outlasts the deadline is not waited for")Lines 6082-6094 6082 from ag2_sparrow import remote_gateway_bridge as gb
6083
6084 class Upgraded(http.server.BaseHTTPRequestHandler):
6085 def do_POST(self):
! 6086 self.rfile.read(int(self.headers.get("Content-Length") or 0))
! 6087 self.send_response(400)
! 6088 self.send_header("Content-Type", "application/json")
! 6089 self.end_headers()
! 6090 self.wfile.write(b'{"reason": "room_upgraded", "replacement_room": "!new:ag2.space"}')
6091 url = self.gateway(Upgraded)
6092 with mock.patch.multiple(gb, URL=url, TOKEN="t"):
6093 with self.assertRaises(urllib.error.HTTPError) as caught:
6094 gb._req("POST", "/v1/room", {"op": "message"}, timeout=15, deadline=time.monotonic() + 5)Lines 6479-6487 6479 def flaky(*a, **k):
6480 if not failed:
6481 failed.append(True)
6482 raise OSError(errno.EMFILE, "Too many open files")
! 6483 return real(*a, **k)
6484 posts = []
6485 with mock.patch.object(task_hold, "notice_record", side_effect=flaky):
6486 self.drain_pass(posts)
6487 self.assertTrue(failed, "the read failed once")Lines 6505-6513 6505 frame = frame.f_back
6506 if not failed and "line_veto" in callers:
6507 failed.append(True)
6508 raise OSError(errno.EMFILE, "Too many open files")
! 6509 return real(*a, **k)
6510 posts = []
6511 with mock.patch.object(task_hold, "notice_record", side_effect=flaky):
6512 self.drain_pass(posts)
6513 self.assertTrue(failed, "the veto's read failed")Lines 6630-6643 6630 self.assertEqual(self.hold(str(self.result_file)).returncode, hatch_cli.REPLY_POSTED)
6631 sent = []
6632
6633 def sparrow(): # the drain sends the reply and records the delivery
! 6634 deadline = time.monotonic() + 10
! 6635 while not self.result_file.exists() and time.monotonic() < deadline:
! 6636 time.sleep(0.02)
! 6637 sent.append(self.result_file.read_text())
! 6638 outbox.record_delivered(self.results / ".outbox", self.TID)
! 6639 self.result_file.unlink()
6640 t = threading.Thread(target=sparrow)
6641 t.start()
6642 self.addCleanup(t.join)
6643 with mock.patch.object(outbox, "_activity_completed", return_value=None), \Lines 6869-6878 6869 real = getattr(os, call)
6870 new, threads = [], []
6871
6872 def successor():
! 6873 task_hold.release(self.results, tid)
! 6874 new.append(task_hold.place(self.results, tid))
6875
6876 def hooked(*a, **k):
6877 if not threads:
6878 threads.append(threading.Thread(target=successor))Lines 7002-7010 7002
7003 def temp_unlink_fails(path, *a, **k):
7004 if str(path).endswith(".tmp"):
7005 raise PermissionError("cleanup")
! 7006 return real_unlink(path, *a, **k)
7007
7008 def tracking_close(fd):
7009 closed.append(fd)
7010 return real_close(fd)Lines 7127-7135 7127
7128 def test_a_drain_that_cannot_write_lets_its_lane_go_stale(self):
7129 from ag2_sparrow import agent_identity, remote_gateway_bridge as gb
7130 if os.geteuid() == 0:
! 7131 self.skipTest("root ignores the permission bits this relies on")
7132 state = self.lane_state()
7133 agent_identity.publish_lane(state, "", "@old:ag2.space") # this process's old lane
7134 auth, record = state / "auth", state / "auth" / "lane-identity.json"
7135 record.chmod(0o400)Lines 7233-7244 7233 ids = [f"@a{i}:lane.space" for i in range(8)]
7234 seen, stop = [], threading.Event()
7235
7236 def read():
! 7237 while not stop.is_set():
! 7238 path = state / "auth" / "lane-identity.json"
! 7239 if path.exists():
! 7240 seen.append(agent_identity.published_lane_record(state, ""))
7241
7242 reader = threading.Thread(target=read)
7243 reader.start()
7244 writers = [threading.Thread(target=lambda m=m: [agent_identity.publish_lane(state, "", m)Lines 7290-7298 7290 def replace(*a, **k):
7291 calls.append(a)
7292 if len(calls) == 1:
7293 raise OSError("disk")
! 7294 return real(*a, **k)
7295 return replace
7296
7297 @staticmethod
7298 def refusing_record_unlink(name="lane-identity.json"):Lines 7299-7307 7299 real = os.unlink
7300
7301 def unlink(path, *a, **k):
7302 if os.path.basename(os.fspath(path)) == name:
! 7303 raise PermissionError("unlink")
7304 return real(path, *a, **k)
7305 return unlink
7306
7307 def test_a_failed_publish_or_a_clean_exit_retracts_the_record(self):Lines 7352-7374 7352 real_unlink, real_replace = os.unlink, os.replace
7353 paused, go, holder = threading.Event(), threading.Event(), {}
7354
7355 def pause(path):
! 7356 if threading.current_thread() is holder.get("t") and \
7357 os.path.basename(os.fspath(path)) == "lane-identity.json":
! 7358 paused.set()
! 7359 go.wait(5)
! 7360 return True
! 7361 return False
7362
7363 def unlink(path, *a, **k):
! 7364 pause(path)
! 7365 return real_unlink(path, *a, **k)
7366
7367 def replace(src, dst, *a, **k):
! 7368 if pause(dst):
! 7369 raise OSError("disk")
! 7370 return real_replace(src, dst, *a, **k)
7371 with mock.patch("os.unlink", side_effect=unlink), mock.patch("os.replace", side_effect=replace):
7372 t = holder["t"] = threading.Thread(target=first)
7373 t.start()
7374 self.assertTrue(paused.wait(5))Lines 7387-7399 7387 paused, go, holder = threading.Event(), threading.Event(), {}
7388
7389 def after_read(real):
7390 def read(*a, **k):
! 7391 rec = real(*a, **k)
! 7392 if threading.current_thread() is holder.get("t"): # the retract's ownership read
! 7393 paused.set()
! 7394 go.wait(5)
! 7395 return rec
7396 return read
7397 patches = [mock.patch.object(agent_identity, name, side_effect=after_read(getattr(agent_identity, name)))
7398 for name in ("_read_at", "published_lane_record") if hasattr(agent_identity, name)]
7399 with contextlib.ExitStack() as stack:
|
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head 2eaad0e17a2d5a70c999aa0749bca344d59fe0ef.
Verdict: CHANGES REQUESTED — not merge-ready.
The main behavior is well targeted, and the discrimination is real: the eight new scenarios produced 5 failures + 2 errors on the parent-code control and pass 8/8 at HEAD; the full Hatch suite passes 202 tests (2 skipped), and the repository review scan passes. The following failure modes still block approval.
-
Blocker — the hold record fails open and is not a crash-safe committed state.
task_hold.place()creates the final pathname before writing JSON, with no cleanup/fsync on write failure.read()then maps malformed content and every non-FileNotFoundErrorI/O failure to an existing hold, and keepalive excludes every non-Nonemarker from the stuck set.Exact-head controls:
injected json.dump OSError: marker_exists=True bytes=0 read={'reason': 'unknown', 'since': None} unreadable .task-holds directory, absent marker: read={'reason': 'unknown', 'since': None} old task + empty marker: condition=healthy reason=task-held-for-ownerSo a torn write can permanently turn a genuinely stuck task green, and one directory-level read failure classifies every open task as held. This directly violates “a hold never hides a stuck task.” Please make publication atomic/durable and distinguish valid, absent, and unreadable/corrupt state; unreadable state cannot be reassuringly healthy.
The marker/notice pair also lacks a committed transition: the marker is placed before the notice is published, while an existing marker makes every retry return success without checking that a notice was ever queued. Simulating a crash after placement yielded
returncode=0, “already held,”notice_exists=False. A concurrent second caller can likewise report success before the first caller's publish fails and releases the marker. The state contract needs an explicit in-progress/committed failure model rather than one file serving as both reservation and proof of notice. -
Blocker —
holdaccepts a destination that the real drain will never deliver.
The CLI checks onlyroom.startswith("!")plus marker parsing atcli.py:1078-1083, but the delivery edge requires Matrix^![^\\s:]+:\\S+$. With attestedchannel_id: !missing-server, exact HEAD returned:hold rc=0 marker_exists=True notice_exists=True drain_route=('foreign', None, '')The command reports success, every retry is suppressed, keepalive reports held/healthy, and the room remains silent. Validate with the delivery edge's actual destination contract before committing the hold.
-
Blocker — hold state is disconnected from terminal result/outbox state, opening a late-message window.
hold()discards the resolved item ID and judges “answered” only by the live result filename, whilereply()also consults the shared outbox terminal state. A delivered result whose task archive failed can therefore be re-woken and announced as waiting.Separately, a queued hold notice is never cancelled or conditioned on the task still being open. The outbound worker drains final task results before proactive files, so if the real answer arrives before the next drain, the room receives the answer first and “Waiting for my owner's go” afterward. Please make the notice lifecycle agree with the task/outbox lifecycle and cover both recovery and settle-time races.
-
Blocker — the advertised generic record owner does not validate its task-ID path boundary.
marker_path()interpolates an arbitrarytid. Exact HEAD:task_hold.place(results, "../../escaped-hold", now=1) -> True <root>/escaped-hold.json exists -> True markers inside .task-holds -> 0The current Hatch caller validates first, but this dependency-light module claims ownership of a generic shared mutable record. It must enforce its own filename/schema bounds so a future caller cannot write/read/release outside
.task-holds.
Required evidence/gates before approval
- Required CI is genuinely red:
diff coverage >= 95% (python)ran and failed at 94%, specifically leaving ten changed lines uncovered, including the corrupt/unreadable-marker branches above. - The PR explicitly says the §15 real post-restart Hatch → proactive drain → room/thread witness is still owed. Harness proof cannot replace it.
- The user-visible
holdcommand andtask-held-for-ownerreason are undocumented. The package README still says a handed-off task remainstask-not-picked-upuntil answered. license/clais currently absent (total_count=0), not failing; it remains a merge gate.- The parent-control output is useful, but the PR body should include its exact command/procedure and parent SHA
7940f25ff965f51df3273d94f009cb1b771119c0.
Worst-case disruption at this head: a worker can stay silent while the task is permanently shown healthy, or send the waiting line after the completed answer. The change that would make this review approvable is a crash-safe bounded hold/notice state machine tied to outbox/task terminal state, with the destination mismatch fixed, all failure/race cases covered, documentation updated, green required checks/CLA, and the real post-restart room witness attached.
… hold is gated on terminal state Review fixes on the Hatch parked-task hold: - task_hold validates the task id with the shared live-task-id rule (local_task_protocol.valid_task_id, plus gateway_instance.INSTANCE_RE for `task-<inst>~<id>`), resolves the path and refuses any that leaves .task-holds/ (traversal, absolute, separators, NUL, symlink). - Two-phase marker: reserve() stages + fsyncs and links a PENDING record exclusively; commit() replaces it with HELD atomically. Only a whole HELD record is a hold: absent, pending, corrupt, partial or unreadable markers read as not held, so the task still reports task-not-picked-up. - hold routes the notice through the drain's own _proactive_route, so a room the drain would not deliver to is refused before anything is written. - hold refuses an item whose result exists or whose outbox entry is settled, re-checks after queueing (withdrawing the notice if the answer landed), and reply withdraws a still-queued hold notice before publishing the answer. - A fresh PENDING marker makes a concurrent hold report not confirmed (3); a stale or unreadable one is redone, reusing a notice still queued. - README documents `hold` and task-held-for-owner. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head f51518e13729063fa87e4d95d668223d8bc821b8.
Verdict: CHANGES REQUESTED — not merge-ready.
This round does fix several prior findings: ordinary traversal IDs and the tested leaf symlink are rejected; absent/pending/partial/unreadable markers no longer count as held; initial result/outbox gating and the ordinary queued-notice answer race are covered; the README is updated; current Actions and diff coverage are green. In an isolated exact-head archive I also got Ran 221 tests ... OK (skipped=2) and review-checks: PASS. The negative controls below still fail.
-
Blocker —
holdstill accepts a room the real drain will skip._drain_room()calls only_proactive_route(). That helper accepts any syntactically valid Matrix room. The real drain then applies the additional_room_is_deliverable_here()gate. Exact-head control with the lane identity onag2.space:hold_rc=0 route='send' room='!foreign:other.space' drain_eligible=False notice_exists=True held={'reason': 'owner-go', 'since': ...}This recreates the original failure: the CLI reports success, keepalive can report healthy/held, and the notice remains undeliverable. The fix also imports another adapter's private implementation, contrary to the repository's dependency-direction rule; importing that bridge runs credential discovery and replaces process-global
socket.getaddrinfo. Please extract the complete destination-eligibility policy behind a dependency-light public helper and make both the drain and Hatch delegate to it. -
Blocker — the generic record is still not confined to
RESULT_DIR/.task-holds.marker_path()resolves the hold directory itself, so a symlinked.task-holds -> outsidemakes that outside directory the trustedbase.prune()then follows it and deletes valid-looking files there. Exact-head output:inside_results=False place_return=True outside_created=True outside_survives_prune=FalseA same-directory alias (
task-A.json -> task-B.json) is also accepted: operations for A create/read/delete B's record. The new test covers only a leaf symlink whose target is outside. Bind operations to the literal hold directory without following the directory or final component, and cover directory symlinks plus same-directory aliases. -
Blocker — the marker/notice transition still is not crash-safe or ownership-safe.
hold()publishes the notice and commits the marker afterward. A stale-pending retry checks only whether the live notice filename exists; it does not consult the proactive delivery receipt. Seeding the exact receipt written by the drain after confirmed delivery gives:prior_proactive_status=DELIVERED retry_returncode=0 notice_requeued=TrueThe drain does not suppress a recreated filename from that receipt, so the acknowledged PR-body crash window really posts twice. Two related exact-head failures show the same missing ownership contract:
- injected marker-commit failure returns 1 and says “nothing posted,” but leaves the complete sendable notice queued and releases the marker;
- synchronized stale recovery lets one caller delete another caller's fresh reservation because
release()/commit()carry no reservation identity; a reproduced takeover ended with a successful successor followed by the old caller erasing its HELD state.
A reservation needs an owner/token checked by commit and release, failure cleanup must withdraw what it queued, and recovery must use the durable proactive delivery outcome so an already delivered notice is never recreated.
-
Blocker — malformed timestamps still count as reassuring holds.
_load()accepts every non-boolint/float, including Python JSONNaNand infinities. Exact head:wire={"reason": "owner-go", "state": "held", "since": NaN} read={'reason': 'owner-go', 'since': nan} since_is_finite=Falseopen_tasks()excludes any non-Nonemarker from the stuck set, then the shared health validator rejects the non-finite row. Thus malformed state can still hidetask-not-picked-upand suppress health entirely. The shared record owner must enforce a finite/bounded epoch on both write and read. -
Blocker — terminal delivery does not retire the hold state.
reply()withdraws only the queued notice; it does not release the hold marker, and keepalive does not consult terminal outbox state. Simulating successful delivery followed by task-archive failure produced:outbox=DELIVERED oldest=None marker_state={'reason': 'owner-go', 'state': 'held', 'since': ...}The lingering terminal task is therefore reported as deliberately held instead of exposing the failed lifecycle cleanup. Tie marker retirement / health classification to the same terminal result/outbox state used by
holdandreply.
Required evidence/gates
- The mandatory §15 real post-restart Hatch → proactive drain → room/thread witness is still explicitly absent; harness proof cannot replace it.
- Current Actions are green, including diff coverage.
license/claremains absent on this SHA (total_count=0), so the documented close+reopen retry is still required before merge. - The original-parent control now names
7940f25..., but its exact executable setup/command is still not pasted; the exact procedure shown in the body applies only to the intermediate2eaad0e1control.
Worst case at this head: an accepted hold can remain silent, an outside file can be deleted, or the room can receive a duplicate/misleading wait line while health reports the wrong state. Please keep the current REQUEST_CHANGES until the shared policy/state-machine defects and the live gate are closed.
… requeue after delivery Review round 2 on the Hatch parked-task hold, simplified: - proactive_destination.py owns where the proactive drain posts: route(), deliverable_here() (the homeserver check), homeserver_of() and the receipt dir. The bridge's _proactive_route/_room_is_deliverable_here and receipt write delegate to it; hold asks drain_room() with the agent's homeserver, so a room on another homeserver is refused. hold no longer imports the bridge. - task_hold drops the pending/held two-phase protocol and stale recovery. A marker is written whole and exclusively in one step (temp + fsync + link) and carries a token; release(token) never erases a newer hold. Every operation goes through an O_NOFOLLOW fd on the literal RESULT_DIR/.task-holds (lstat-checked), so a symlinked hold dir, a symlinked marker or a same-dir alias is never written, read or pruned through. `since` must be a finite, bounded epoch on write and read. - hold queues the line first (exclusive link), skipping it when it is queued, being sent, or the drain's delivery receipt says delivered, then places the marker. A marker failure reports not confirmed (exit 3) and the task keeps reading stuck; a retry places the marker without requeueing. - reply releases the hold along with the queued line; keepalive ignores a hold on a task whose outbox already has its answer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact head a77d49a714dedc4ffa10f0036279cf45e9739b6b.
Verdict: CHANGES REQUESTED — not merge-ready.
This round does close real parts of the prior review: the routing predicate now has a dependency-light owner; ordinary directory/leaf symlinks are refused; timestamps are finite and bounded; reply/settled-task paths retire or ignore the marker. In an isolated exact-head archive, all 233 Hatch adapter tests passed (2 skipped), review-checks.sh and git diff --check passed, and CLA is now present/passing. The controls below still fail.
-
Blocker — the activated bridge is broken by a namespace collision.
The live loader imports the function
proactive_destinationatsrc/remote-gateway-bridge.py:46-47, then executes the package implementation into the same globals. The newfrom . import proactive_destinationreplaces that function with the module. The loader's later claim/voice helpers therefore fail at runtime:TypeError: 'module' object is not callable ... src/remote-gateway-bridge.py", line 309 dest = proactive_destination(path.name)This is reproduced by both node/out-of-tree CI and Python shard 3. The resulting routing regressions are also concrete: shard 4 posts a Discord-owned bare proactive to the AG2 owner DM, shard 7 claims/deletes a
.to-discordfile, and shard 5 suppresses verified-room delivery. -
Blocker — an old token can still erase a newer hold.
release(token)reads/checks the token, closes that file, then unlinks the current name. There is no atomic compare-and-delete. In a synchronized exact-head control, the old releaser paused after its read, another actor removed the old marker and placed a new token, then the old release resumed:old_release_return=True new_marker_after_old_release=NoneThis directly falsifies “releasing with an old token can't erase a newer hold.” The check and removal need one ownership/locking contract that cannot target a replacement inode.
-
Blocker — the marker writer can escape its directory or report success with a corrupt marker.
_open_dir(create=True)can returnNoneif.task-holdsdisappears between creation andlstat;place()does not reject it. Withdir_fd=None, the relative operations use the process CWD. Deterministic control:TypeError: NoneType...plusoutside_marker_created=True.- The single
os.write(fd, body)ignores a legal short write. Injecting an eight-byte write yieldedplace_returned_token=True, bytesb'{"reason', andread_hold=None; throughhold, the command returned 0 and printed success.
Thus the shared writer still does not guarantee confined, whole state or truthful failure semantics.
-
Blocker — “never queued twice” still depends on a best-effort receipt.
The bridge catches every proactive-receipt failure and continues. After a confirmed delivery whose receipt write fails, there is no queued
.txt,.sendingclaim, or durable receipt for_notice_sent()to see. Exact post-delivery control:hold_rc_after_delivered_but_receipt_failed=0 notice_requeued=True held=TrueThe delivery outcome used for deduplication must itself be durable/authoritative before the flow can promise the line is never recreated.
-
Blocker — Hatch and the drain can still evaluate different homeserver identities.
Hatch derives the policy input from the installed snapshot
cfg["AGENT_MXID"], while the drain derives it from the live, re-read_reenroll_identity(). These inputs are not kept equal. With installed@a:ag2.spaceand live@a:other.space,holdaccepts!r:ag2.spacewhile the drain returns no room. Sharing the predicate is not enough if its identity input can drift; both paths need the same identity resolver/binding.
Required gates
- Required CI is red; the aggregate clean-install gate fails. Current-head diff coverage was skipped, so the earlier 100% report is not evidence for
a77d49a7. - The mandatory §15 real post-restart Hatch → proactive drain → room/thread witness remains explicitly absent.
- CLA is fixed and passing. Merge still requires repaired current-head CI/coverage, the live witness, and two current formal maintainer approvals.
Worst case at this head: another bridge's result is claimed and deleted, a verified room result is held forever, a stale owner erases a successor hold, or the CLI reports a healthy hold whose marker is outside the hold directory or unreadable.
… confined writes, fail-closed dedup, live identity Review round 3 on the Hatch parked-task hold: - Rename proactive_destination.py to delivery_destination.py and import it in the bridge as `_delivery_destination`: the sutando loader execs the package bridge into globals that already hold the routing function `proactive_destination`, and the module import replaced it, so the claim gate raised TypeError. tests/gateway-claim-gate-name-collision.test.py drives the gate for a verified room, a Discord-owned bare file and a `.to-discord` file. - task_hold.release(token) renames the marker to a private tombstone under the dir fd, checks the token there, and puts a mismatched marker back, so an old token never deletes a successor's marker. - Every marker write goes through the held dir fd; an absent or vanished hold dir raises instead of falling back to the CWD. Writes loop until the whole body is written. - hold records a durable `<tid>.notice` before queueing the line. Once it exists, the line is skipped while queued/sending or delivered per the drain receipt, and with no readable proof hold exits 3 and queues nothing. A failed queue clears the record so the retry queues it. - hold judges the homeserver from agent_identity.lane_identity, the same resolver the bridge uses, over the running Sparrow's environment when readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head f7d4df8db0dc957f6e8ece67a156f0b0f60c3e2b. The loader collision is fixed: the activated claim-gate suite passes, as do the previously failing bridge suites. The CWD fallback is gone, positive short writes are looped, git diff --check and the review scanners pass, and the full Hatch adapter suite reports Ran 246 tests ... OK (skipped=2).
I still need to request changes because the fail-closed guarantees are not established at this head.
Blocking findings
-
Required CI is red from a PR-caused credential-owner boundary regression.
agent_identity.py:79-86now handsread_fileto credential-source discovery from a generic identity module. The focused exact-head guard fails with:AssertionError: ['agent_identity.py:86 reader read_file handed to the owner'] != []This is reproduced by Python shard 1 and the hermetic sentinel in shard 7; the aggregate clean-install job is consequently red. Provider/credential I/O needs to remain at the owner/adapter edge.
-
A concurrent notice loser can still install a healthy hold after the notice owner fails. Two callers can both read
line == "new"; A winsmark_notice(), B getsFalseand falls through totask_hold.place(), then A's queue write fails and clears the notice record. My synchronized production-function repro ended with:B rc=0; A rc=3 queued_notice_exists=False durable_notice_record=False hold_marker_present=TrueA retry then reports success/already-held without creating the missing notice. The
mark_notice(False)path must re-evaluate authoritative state and must not place a hold until queued/delivered publication is proven. The same issue appears when_publish_once()returnsFalsefor an existing dangling queue path: that return is ignored and a healthy marker is installed. -
The shared writer can report failure after publishing the final marker. Injecting failure into the directory
fsyncimmediately after_create_at()links the final marker givesholdrc 3 and “task still reads task-not-picked-up,” whiletask_hold.read()returns a validowner-gomarker and health reports held. The writer must either roll back the visible final name on this failure or report the committed outcome truthfully; callers cannot safely infer the current state from the exception. -
Stale-token release is not crash/failure safe.
release()renames the canonical marker to a hidden tombstone before checking the token. A stale releaser crash there, or anos.linkfailure while restoring the mismatched successor, leaves the successor only under.released;read()andprune()ignore it indefinitely. Exact repro:old_token_differs=True visible_marker_exists=False read_after_crash=None dir_entries=['.task-1784700000001.json.<nonce>.released']This still permits an old releaser to make a newer hold disappear from the authoritative namespace.
-
Identity parity remains conditional on readable live-process state. With no readable Sparrow lock/env, Hatch falls back to its own caller environment while the future drain resolves its own process/channel environment. In the exact-head control, Hatch used
@agent:ag2.spaceand accepted!r:ag2.space, while the drain used@agent:other.spaceand rejected it. Sharing the parsing function is not enough when the two decisions can receive different authoritative inputs; the fallback must fail closed or resolve one durable shared identity.
Required gates
- The PR still explicitly says the post-restart Hatch → proactive drain → intended room/thread §15 live witness is owed. That witness is required for this live delivery-path change.
- Exact-head diff coverage is skipped, and the visible coverage comment predates this head, so current-head ≥95% evidence is absent.
- CLA is green at this SHA.
Please fix the blockers, add deterministic regressions for the interleavings/failure points above, restore required CI and current-head coverage, then attach the live witness. I can re-review that exact head.
…proven line, release never hides a successor Review round 4 on the Hatch parked-task hold: - Drop agent_identity.lane_identity and every read of the running Sparrow's environment: it handed a credential-source reader outside its owner. hold takes the homeserver from the identity Sparrow enrolled (agent_identity.enrolled, STATE_DIR/auth/ag2space.json); when that is unknown the pre-check passes and the drain's predicate decides at send. - The notice record carries its writer's token; only that caller queues the line and only it may clear the record. Every caller, owner or loser, records a hold only once the line is proven queued, being sent or delivered; otherwise exit 3 and no hold. - Once the marker's link succeeds it is published: a failed directory fsync afterwards no longer reports failure. - release(token) drops the tombstone: it reads the marker by fd, compares the token, and unlinks only if the name still points at that inode, so a crash or a mismatch never leaves a successor under a hidden name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lock The inode comparison in release(token) could not tell an old marker from a successor that reused its inode number (Linux reuses them at once), so an old token could erase a newer hold. Every create (the exclusive link) and every owned unlink (release, clear_notice, prune) now runs under an exclusive flock on `.task-holds/.lock`, opened through the held dir fd with O_NOFOLLOW. Inside it, release is a plain read-compare-unlink; the inode check and its documented window are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s command Both hold releases that follow an answer (reply after publishing, and hold when the answer lands mid-hold) now go through _release_quietly: an OSError or ValueError is logged as one stderr line and the command keeps its own exit status. A hold left behind is harmless because health ignores holds on answered tasks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact current head 03f60756133ce88c5063e5f7386d50115dc8d645 (the requested e6039158 head moved twice while this review was in progress).
REQUEST_CHANGES — not merge-ready.
I verified that this series now fixes the credential-owner boundary, the concurrent notice owner/loser race, post-link directory-fsync result semantics, the release/tombstone inode-reuse race (via the hold-directory lock), and the earlier loader/CWD/short-write cases. Exact-head evidence is green: the full Hatch suite ran 256 tests successfully (2 platform skips), the focused guards pass, Actions run 39922563931 is green, and changed-line coverage passes at 99%.
Three target-path blockers remain:
- An empty pre-existing notice is accepted as proof that the owner was notified.
_line_statereturnsqueuedfrom pathname existence alone, while the real drain rejects an empty/whitespace result as not ready and retries it forever. Exact-head production-path repro with a zero-byteproactive-hold-<tid>.txt:
EMPTY_NOTICE {hold_rc: 0, line_state: queued, drain_ready: None,
marker_present: True, notice_size: 0}
EMPTY_DRAIN file_exists=True bytes=0 sends=0 sending=0
The CLI therefore prints that Sparrow posts the notice and installs a valid hold, but no line is deliverable. The current “publish wrote nothing” test mocks a false return without creating the invalid file that drives this production branch. Please use the shared readiness predicate (or the same contract) before treating a queued file as proof, and cover the pre-existing-empty-file case.
- The shared hold writer can still report failure after the marker was durably published. In
_create_at, the final-marker link and success decision happen before temp cleanup, but any cleanupOSErrorother thanFileNotFoundErrorescapes fromfinally; it also skipsos.close(dfd). Forcing the marker temp unlink after the successful link produced:
POST_LINK_TEMP_CLEANUP {
hold_rc: 3,
stderr: "NOT confirmed: the hold was not recorded (temp cleanup failed after link)...",
marker_present: True,
notice_ready: True
}
That continues the original writer-contract defect: the caller is told “not recorded” although the canonical marker exists. Post-commit cleanup must not override the published outcome, and the directory fd must close on every path; please pin both in a regression test.
- Hatch and the actual drain still do not share one authoritative lane identity. Hatch derives its homeserver only from durable enrollment, and an empty identity passes every room. The running drain instead resolves env/channel identity before enrollment here. With no enrollment and a live AG2 identity on another homeserver, exact-head
holdproduced:
hold_rc=0
marker_present=True
notice_exists=True
hold_homeserver=
drain_with_live_ag2=None
This recreates the reported failure: a healthy deliberate hold with a notice the real lane refuses. Please give both sides one durable lane identity (provider I/O remaining at the edge), or fail closed / defer the hold until authoritative delivery proof exists, with an override-vs-enrollment regression.
Separate merge gates remain: license/cla is absent on this SHA (not failing), and the mandatory REVIEW §15 post-restart Hatch → proactive drain → intended room/thread witness is still explicitly owed. Actions and diff coverage are green, but those gates and the three blockers above prevent approval.
… temp cleanup never fails a hold - `_line_state` counts a queued or claimed hold-line file as proof only when the drain's own readiness predicate reads it and its body is exactly the line this hold queues; an empty, blank or foreign file reads unknown, so hold exits 3 and installs no hold. - `task_hold._create_at`: once the marker link succeeds the result is success; temp cleanup is best-effort and the dir fd closes on every path. - Pin the stated limit: with no enrollment the homeserver pre-check passes and the drain is the final authority (the live identity sits in a credential env Hatch must not read). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact current head b3ce9d6707ca633d38b19f62b81499c667d9a41d.
REQUEST_CHANGES — not merge-ready.
This head does fix two prior cases: static empty/blank/foreign hold-line files now fail closed through the shared readiness reader, and a temp-file unlink error no longer overrides a published marker. The five new tests are collected. Exact-head evidence is otherwise green: the full Hatch suite ran 261 tests successfully (2 platform skips), the 124-test credential guard passes, review-checks passes, all 33 applicable Actions checks pass, and changed-line coverage passes at 99%.
Three target-path blockers remain:
- The new readiness snapshot races the normal
.txt → .sending.<pid>drain transition._line_stateenumerates pathnames once and then reads those same names. The drain atomically renames the line to a pid claim, so a rename between enumeration and readiness read producesunknowneven though the exact ready line is now in the valid claim. Deterministically moving it at that boundary gave:
CLAIM_RENAME_RACE {
hold_rc: 3,
marker_present: false,
claim_exists: true,
claim_ready: true,
stderr: "... line ... is not proven queued; ... still reads task-not-picked-up"
}
The drain can then post the waiting notice while no hold marker exists, so the room says the task is held while the worker still sees an executable/unheld task. Delivery during the same window also returns unknown without falling through to the new receipt.
The private glob also disagrees with the consumer grammar: it accepts every .sending* name, while recovery skips a nonnumeric pid suffix forever. An exact-body .sending.not-a-pid produced hold_rc=0, marker_present=true; no drain path can process it. Please give claim-name/state classification one shared owner, handle a rename/delivery transition by a bounded rescan or receipt check, and cover both schedules plus invalid claim names.
- Post-link cleanup can still turn a published record into reported failure. The final link occurs inside
_locked, but the lock-fd close at lines 171–180 and directory-fd close at lines 147–153 remain unguarded. Injecting either cleanup error after the second/final link produced the same contradiction:
POST_LINK_CLOSE {
hold_rc: 3,
marker_present: true,
notice_ready: true,
stderr: "NOT confirmed: the hold was not recorded ..."
}
The temp-unlink regression therefore covers only one cleanup operation. Also, dfd is acquired before secrets.token_hex() and before the try; an injected token-generation error leaves it open. Please make the writer track publication so no cleanup error after link can override success, enter fd ownership before any intervening operation, and pin lock-close/directory-close plus pre-try failure paths.
- The identity divergence from the prior review is explicitly preserved, not resolved. Hatch still reads enrollment only here; the live drain resolves process env → channel env → enrollment here. The new test pins missing enrollment as successful hold publication even when
AGENT_MXID=@watch-agent:ag2.spaceand the room is!foreign:other.space. Exact-head comparison:
{hatch_missing_enrollment: "!foreign:other.space",
live_lane_ag2_space: null,
hold_rc: 0,
marker_present: true}
That is the reported failure mode: healthy deliberate hold, silent room, stranded line. Documenting it as a limit does not satisfy the feature invariant. Hatch need not read credential-bearing environment; the provider edge can publish one durable non-secret authoritative lane identity, or Hatch must fail closed / defer the marker until authoritative delivery proof exists. Please cover missing, stale, and override-differs-from-enrollment cases end to end.
Separate merge gates remain: license/cla is absent on this SHA (not failing), and the mandatory REVIEW §15 post-restart Hatch → proactive drain → intended room/thread witness is still explicitly owed. Green Actions and coverage do not replace those gates or the blockers above.
…rker, and fails closed without enrollment - _line_state reads candidates in the order the drain moves a line (queued .txt, then .sending.<pid> claims, then the delivered receipt) and rescans up to 3 times when a file vanishes mid-read, so a claim or hand-back during the check no longer reads as unconfirmed. - Claim naming has one owner, delivery_destination.claim_path/claim_pid; the drain's claim and orphan recovery use it, and hold accepts only .sending.<digits> claims, the ones the drain creates and recovers. - task_hold._create_at: once the final link succeeds, lock/dir-fd close, fsync and temp unlink are best-effort and cannot change the result; the temp name is drawn before the dir fd is opened. - hold exits 3 and records nothing when Sparrow's enrollment is missing or unreadable, instead of skipping the homeserver check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Reviewed exact current head 3cdb73b3fc6f2f660d72fb51c174133c8202283d.
REQUEST_CHANGES — not merge-ready.
This round does resolve the two concrete implementation failures from review 5626960603: the queued/claim/receipt transitions now survive both rename schedules, and post-link close/fsync/unlink errors no longer override a published marker. Missing, malformed, non-string, and unreadable enrollment records also fail closed. My exact-head runs passed the 266-test Hatch suite (2 platform skips), the 124-test credential guard, and review-checks.sh.
Two target-path blockers remain:
- The stale-enrollment/runtime-override divergence still produces a healthy hold with no deliverable room notice.
_lane_homeserverintentionally reads enrollment only and says a stale override still passes, while the live drain resolves process env → channel env → enrollment in_reenroll_identityand applies that identity at delivery time. A deterministic current-head probe with enrollment@agent:ag2.space, liveAGENT_MXID=@agent:other.space, and room!room:ag2.spaceproduced:
{hold_rc: 0, marker_present: true, notice_ready: true,
live_drain_accepts_room: false}
The matching-identity control accepts the room. This is the original feature failure: health reports a deliberate hold while the asker remains silent and the line is stranded. Fixing absent enrollment does not cover stale enrollment or an override that differs from it, both explicitly required by the prior review. Hatch need not read credential-bearing environment; publish an authoritative non-secret lane identity at the provider edge, or otherwise defer/fail closed until delivery is authoritative, and pin the mismatch end to end.
- The shared claim parser still accepts names the drain never creates, and those names can prove a false hold forever.
claim_pidaccepts every ASCII digit spelling, including zero and leading zeros, but the drain creates only the canonical positive spelling fromos.getpid()viaclaim_path._line_statethen counts any parsed name as queued, and recovery trusts the parsed PID. With an exact-body.sending.0<CURRENT_PID>claim, current head produced:
{hold_rc: 0, marker_present: true, parsed_pid: CURRENT_PID,
claim_still_stranded_after_recovery: true, recovered_txt_exists: false}
No drain creates that leading-zero name; recovery defers it because the parsed process is alive. Please make the accepted grammar exactly the constructor's canonical positive PID language (and cover zero/leading-zero spellings), so “valid claim” means a state the drain actually creates and recovers.
Separate merge gates are also still open: the mandatory REVIEW §15 real post-restart Hatch → proactive drain → intended room/thread witness is explicitly owed; license/cla is absent on this SHA (zero commit statuses, not a failing CLA); and the current Actions run is red because python standalone tests (4/9) timed out in pool-restart.test.py, which makes the aggregate and diff-coverage gates non-green. The CI failure appears unrelated to this delta, but it is still a gate rather than evidence for either blocker above.
|
@cla-assistant check |
…y; accept only canonical claim names claim_pid accepts only the spelling claim_path writes (positive, no leading zeros), so a `.sending.0<pid>` file is neither proof of a queued line nor a live claim that orphan recovery defers forever. The drain now publishes the identity its deliverability predicate uses to <state>/auth/lane-identity.json (agent id, timestamp, pid; one writer, agent_identity.publish_lane), on start of each proactive pass and whenever it changes. Hatch reads that record first, then enrollment, so an AGENT_MXID override that makes enrollment stale no longer lets `hold` record a hold the drain would refuse to deliver. Hatch still never reads the credential env. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-review at exact head cd4762f5aec4396ca4450d516d3eecf72936def3.
The two narrow cases from the previous round improved: the canonical positive-PID suffix cases now pass, and the direct enrollment/live-identity mismatch is refused after a successful lane publication. I also reran the exact-head Hatch suite (271 OK, 2 skipped), credential guard (124 OK), the foreign-homeserver bridge test, and the hardcoded-path review check; all passed. However, I can still reproduce three blockers.
-
Blocker — the claim check is not canonical relative to the queued notice.
claim_pid()validates only the text after the last.sending., while Hatch globs every matching prefix and trusts that parser. An exact-body file named<notice-stem>.sending.x.sending.<live-pid>therefore makesholdreturn 0 and write the hold marker even though it is notclaim_path(notice, pid). Exact-head probe output:adjacent extra infix parsed: 2 Hatch invalid exact-body claim: x.sending.2 rc 0 held TrueRecovery later requeues that name, but until then it is not proof that the live drain owns this notice. Please make the decision path-relative—e.g. require the candidate to equal
claim_path(notice, parsed_pid)through a shared helper—and cover the adjacent-infix form in both hold and recovery tests. -
Blocker — one unsuffixed lane record collapses the supported named gateway instances. The bridge explicitly supports several per-instance processes sharing
_STATE, but_lane_record()is a single unsuffixed file and Hatch ignoresGATEWAY_INSTANCEwhen reading it. In a two-instance production-helper repro, green overwrote blue; blue's next pass did not repair the record because its cache only knew that blue's own identity had not changed:MULTI_INSTANCE { 'blue_live_identity':'@blue:blue.space', 'shared_record_after_blue_next_pass':'@green:green.space', 'blue_cfg_homeserver':'green.space', 'green_cfg_homeserver':'green.space' }Both Hatch instances consequently judge against green even though blue's live drain judges against blue. The existing instance-scoped gateway status already publishes live
agent_id,connected, andts, and Hatch already has an instance-aware path resolver. Please reuse that authoritative/fresh surface or define an equivalently instance-scoped record, and test two production writers sharingSTATE_DIR. -
Blocker — stale or unpublishable lane state still recreates the original false hold.
published_lane()ignores the record'stsandpid. Withts=1, a nonexistent PID, and an old identity, Hatch returned 0 and recorded a hold while the live drain refused the room and left the notice queued. Separately, if the production writer raisesOSError, the bridge deletes the record and says readers fall back to enrollment; with stale enrollment, the exact prior mismatch returns:PUBLISH_FAILURE_FALLBACK { "enrollment":"@watch-agent:ag2.space", "hold_rc":0, "live_accepts_room":false, "marker_present":true, "notice_ready":true, "published_lane":"" }Please fail closed when Hatch has no current identity from the configured live drain. The shared record needs an explicit freshness/owner/instance/failure contract, with concurrency and failed-publication/retraction tests against the production writer.
All current Actions checks are green. Two non-code merge gates remain: license/cla is absent on this SHA (the commit status endpoint has zero statuses), and the PR body still marks the mandatory real post-restart Hatch → proactive drain → intended room/thread witness as owed. REVIEW §15 requires that live-path evidence before approval.
Verdict: REQUEST_CHANGES.
|
Follow-up to blocker 1 at exact head The final-segment A production-helper probe used a valid different queued file: While PID 29499 is live, recovery returns before constructing a target, so the first-occurrence It restores the claim as The clean contract is a shared reversible helper that returns both |
…be this notice's own - hold no longer falls back to enrollment: with no lane record from the live drain of its gateway instance it exits 3 and records nothing. - The lane record is per instance (lane-identity[.<instance>].json, the suffix the bridge's other per-instance files use) and carries its instance. Hatch treats it as missing when its pid has exited, when a live lock for that instance names a different pid, or when it names another instance. - The drain publishes at start (before the outbound worker) and on every proactive pass, rewriting whenever the record on disk is not its own; it retracts the record on a failed publish and at clean exit. - claim_base(name), beside claim_path/claim_pid, inverts claim_path. Hatch counts only claims whose base is the notice's stem, recovery renames a dead claim back to its own base, and the owner-mention queued check uses the same grammar. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…covery `<stem>.sending.5.sending.<pid>` parses a canonical pid just like the `.sending.x.` form; both are now covered, and the hold test checks that a matched name is exactly claim_path(base, pid). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review at exact head 40ffc581ed4a01769438303ad4d57c5836ee796a. The branch advanced from 5336328b while the preceding review was being submitted; GitHub attached that review to 40ffc581, so this text corrects its stale first line. I inspected the one-commit delta: it changes only adjacent-infix tests, not either production path below.
The three cd4762f5 cases are materially improved: canonical adjacent-infix claims no longer count for the wrong notice, named instances retain separate records, and the ordinary missing/dead-pid/publish-failure cases now fail closed. At the unchanged production tree I ran the Hatch suite (279 OK, 2 skipped), credential guard (124 OK), real-drain suite (6 OK), the foreign-homeserver/body-target/name-collision bridge tests, and the hardcoded-path review check; all passed. At 40ffc581, the changed Hold + DeliveryDestination groups pass 53/53 and both blocker probes below still reproduce. Current-head Actions are pending. Two code blockers remain.
-
Blocker — the lane record still has no reliably fail-closed, concurrency-safe writer contract.
_lane_identity()catches a publish failure, calls unqualified retraction, and then states that Hatch will fail closed, butretract_lane()suppresses unlink failure and implements its owner check as a separate read followed by unlink.The first failure mode recreates the false-hold split this round is intended to close. In an isolated production-helper probe, I seeded the current process's old lane
@old:ag2.space, changed its live identity to@new:other.space, injectedOSErrorfrom publication andPermissionErrorfrom record unlink, and then queried both predicates:drain_accepts_room: False record_after_failure: @old:ag2.space (same live pid) hatch_lane_homeserver: ag2.space predicates_disagree: TrueThe existing failed-publication test only covers the case where deletion succeeds. If both filesystem operations fail, the old same-pid record remains accepted by Hatch's reader, even though the live drain uses the new identity and refuses that room.
The second failure mode deletes a successor's good publication. A deterministic interleaving paused an old
retract_lane(..., old_pid)after its ownership read, published a new live PID's record with the production writer, then resumed the old unlink. The replacement did not survive. The unqualified cleanup at line 5461 has the same problem when another writer publishes between the failed writer's read/attempt and cleanup:publish failure: new live PID 79088 record survives = False clean exit: new live PID 79088 record survives = FalseThis is reachable under the supported lock-disabled/fail-open paths and during stale takeover overlap. Please give the shared record owner a serialized/CAS-equivalent publication-and-retraction contract, make failed invalidation observable/fail closed, and test publish-vs-retract and publish-failure-vs-successor interleavings with the production writer. The current concurrency test covers only concurrent publishes by threads in one PID.
-
Blocker — canonical claim parsing does not enforce the native PID domain.
claim_pid()accepts an unbounded positive decimal, while the Darwin liveness probe narrows it throughctypes.c_int. On this Darwin host, adding2**32to the current PID therefore aliases the live process:real_pid: 91601 forged_suffix: 4295058897 parsed_pid: 4295058897 owner_may_be_alive: TrueA full exact-head probe with an exact-body
<notice>.sending.<real_pid + 2**32>made_line_statereportqueued,holdreturn 0 and write the marker, while orphan recovery deferred the file as live. No OS drain can own that PID. Please enforce the native/production PID bound in the shared constructor/parser (and defensively before native probing) and add this alias as the Darwin regression control.
Two non-code gates are also still open: license/cla is absent on this SHA (the commit-status endpoint has zero statuses), and the PR body still marks the required real post-restart Hatch → proactive drain → intended room/thread witness as owed. REVIEW §15 requires that live witness before approval.
Verdict: REQUEST_CHANGES. The ordinary fixes are good, but merge-readiness requires the lane record to remain fail closed through failed invalidation and competing writers, the claim PID grammar to reject native aliases, and the live-path/CLA gates above to be satisfied.
…closed lane record - outbox.valid_pid (0 < pid <= 2**31 - 1, an int, never a bool) is the one pid domain. claim_pid, the lane record's reader and writer, process_identity (so the drain's _pid_alive and claim_owner_may_be_alive) and Hatch's process_gone all use it: an out-of-domain pid is never a live owner. real_pid + 2**32 no longer aliases a live process through ctypes.c_int, and a lane record carrying it no longer crashes hold with OverflowError. The native probes also catch OverflowError/ArgumentError. src/outbox.py carries the same change. - Lane publish, retract and read hold a per-instance flock beside the record, opened O_NOFOLLOW through an O_DIRECTORY fd on auth/. Publish writes a fresh temp, fsyncs and replaces; on failure it invalidates the old record in the same locked step (unlink, else a retracted tombstone, else truncate in place). Retract compares pid, ts and instance under the lock. The drain retracts only the record it wrote at clean exit. - The longpoll test's os.replace fake forwards keyword arguments. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-review at exact head 9fbb29347fb0ab3bfd543977e5cb9b4bd890ab4e, against review 5627929500. The new shared PID bound closes the Darwin alias in claim names, and the per-instance flock/CAS cleanup closes both successor-deletion races. I independently reproduced those controls as passing. Exact-head Hatch is 286 OK (2 Linux-only skips), credential sources 124 OK, outbox protocol 22/22, gateway longpoll 55 OK, singleton 21 OK, drift/review scans PASS.
Three code blockers remain.
-
Blocker — exhausting all three invalidation fallbacks still recreates the false hold.
_invalidate_at()raises only after unlink, tombstone replacement, and in-place truncation all fail;publish_lane()then propagates. But the bridge catches that error and continues under its newly resolved identity, while Hatch still accepts the readable old record because it carries the same live PID.I reproduced this without mocking any filesystem operation: publish
@old:ag2.spacewith the current PID, chmod the record0400andauth/0500, change the bridge identity to@new:other.space, and call the production bridge/writer and Hatch reader as uid 504:drain_accepts_old_room=False hatch_server='ag2.space' stale_record={'agent_id': '@old:ag2.space', 'pid': 88052, ...}The log says “Hatch holds fail closed until it is,” but the predicates demonstrably disagree. The new tests cover tombstone success and truncate success, not total invalidation failure. The PR body's disclosed read-only-filesystem limit is therefore the prior blocker, not a harmless limitation. Please ensure the live bridge cannot keep serving a different lane while a stale same-live-PID record remains authoritative (or provide a separately authoritative reader-visible revocation), and pin the chmod/read-only case with production filesystem operations.
-
Blocker — an
auth/removal race escapes the dirfd boundary and writes lane files into the process CWD._auth_dir(create=True)can returnNoneif the directory disappears aftermkdir();publish_lane()passes that value into relativedir_fdoperations and finallyos.close(None). A deterministic race probe controlled only the firstos.lstatboundary, removingauth/there; all opens/replaces/writes remained production code:exception: TypeError: 'NoneType' object cannot be interpreted as an integer auth_exists: False cwd_files: ['lane-identity.json', 'lane-identity.lock']This is introduced by this round, and
TypeErroralso escapes the bridge'sexcept OSError. Forcreate=True, retry the create/open/validation race or raiseOSErrorbefore any relative operation; never allowNoneto mean the CWD. Add the removal-at-open regression. -
Blocker — the claimed single PID domain is still bypassed by persisted outbox claims and by the constructor.
valid_pid()says every file PID reaches it first, but_read_claim_at()first narrows arbitrary JSON throughint(...)and never calls it. Exact-head output:true => pid 1, HELD 1.9 => pid 1, HELD "1" => pid 1, HELD Infinity in pid => uncaught OverflowError Infinity in start_usec => uncaught OverflowErrorThat contradicts the adjacent UNKNOWN-on-wrong-types contract and can abort a drainer scan.
claim_path()likewise accepts zero, negatives, booleans, andPID_MAX+1, producing names its own parser rejects. Validate the raw persisted type/range before conversion, handle numeric overflow as UNKNOWN, and enforce the same domain at the constructor. The vendored/core outbox copies must stay identical. Also reconcile the intentional DEAD result for invalid PIDs with the existing Windows-native test, which still expects UNKNOWN and is skipped on this host.
At submission, license/cla is green. Actions has 32 successful checks but is red: python standalone tests (4/9) timed out in pool-restart.test.py, and the aggregate gate failed. The required live post-restart Hatch → proactive drain → intended room/thread witness is still marked owed in the PR body.
Verdict: REQUEST_CHANGES. The alias and successor-race fixes are real, but the all-invalidation failure still violates the prior fail-closed requirement, and this round adds the dirfd escape while leaving the shared PID boundary incomplete.
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Exact-head re-review of 5f5d7a414be64a20f457dd80d381c1a316811496 against prior reviewed head e7774268722a4b76a9292307724ee61accbe0d07 and current base a0e25c5895e9bd54f5b40ad9d84d0119915a2486.
Both prior P1 code blockers are cleared.
- The inactive-unit transition now issues only
systemctl --user startafter the final stopped/no-holder proof. A successor appearing after the finalshowor during the action receives at most the idempotent start; this branch never reachesreset-failedorrestart. - Successor certification now takes a device/inode snapshot around each lock read, then requires the final snapshot to be the same file, still canonical, unchanged across PID/start/proc-start/send-lock identity, and still matched by the live process generation. The workspace/role/schema and equal-record/new-inode rollovers are rejected.
Independent discrimination with the head tests:
# at 5f5d7a41
Ran 3 tests in 1.485s
OK
# same three tests against e7774268 production
Ran 3 tests in 1.465s
FAILED (failures=6)
Broader exact-head checks also pass:
StartSparrow: 74 passed, 2 skipped
Hold + Keepalive: 126 passed
git diff --check: clean
review-checks.sh: PASS
Hosted state at the frozen SHA is green: 35 successful checks, 3 intentional skips, no failures, and license/cla succeeds. There are no unresolved review threads. I found no remaining code blocker in this fix delta.
[P1 / blocking] The required live-path witness is still absent.
The PR checklist remains unchecked and states that no real Hatch VM round trip has occurred. Before approval, run the promised post-restart Hatch hold through the proactive drain and paste evidence that it produces exactly one line in the original room and thread, with health reporting the task held. This must exercise the real service after restart, not only the test harness.
This review replaces my stale code findings: those two P1s are resolved. The live witness is now the sole approval blocker.
Verdict: REQUEST_CHANGES — code-clear, but not approval-ready until the live witness is attached.
REVIEW.md §15 live witness at 5f5d7a4Head Token. I used the REAL relay token of a test identity ( Setup. I ran (1) Sparrow before and after a restartThe status file now names its writer (pid, start, proc_start), as this PR adds. Status and lock name the same generation. (2) hold → proactive drain → exactly one line in the original room and thread — PASSThe owner's agent ( Room count, read by the owner's agent: after (3) the held task reads as held, not stuck — PASSThe task never reads (4) no repeat after a second restart — PASSI checked Teardown. Each scratch Sparrow (55621, 75124, 11424) was stopped by its exact pid after its command line was checked. Afterwards no process referenced the scratch dir. The literal What this proves. At this head, after a real restart, Teardown confirmed by the owner's agent (03:22Z): the vault copy is deleted, and the test identity's relay is restored and running. A final recount still shows exactly 1 line. |
Conflict resolution: remote_gateway_bridge._post_room_message keeps this branch's `deadline` argument and takes main's _ROOM_POST_TIMEOUT_S (20, the literal it replaced), so the request is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…uest deadline, as the other proactive fakes do Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested at dd3ce34 (agent-assisted review by Mars, Qingyun's Sutando seat; first full review from this seat). The merge resolution in a40cff2 and the dd3ce34 test fix are correct. Every finding from Susan (@1e01bd89) and Kewei (@57e0f7e6, 0592049, 6a7b99a, 930017e, and the later rounds through 5f5d7a4) is resolved at this head. Two new holes were reproduced, plus the standing process gate.
B1. A held task can go permanently silent while health reads task-held (healthy). Reproduced.
- Where:
task_hold.line_vetoturns any error reading the notice record into a veto (task_hold.py:304-307,return f"its hold could not be checked ({e})"), and the outbox treats a veto as final: the line is withdrawn and parked. - Health doesn't see it:
keepalive.open_tasksrecords the outcome aswithdrawn, buthealth_rowonly promotesundeliveredandunconfirmedto problems (keepalive.py:1066-1069), sowithdrawnis dropped. - No recovery:
holdrun again only says "already held", so nothing re-queues the line. - Repro: run
hold(rc 0). Then a drain pass whose notice read fails once (simulated EMFILE) givesPARKED withdrawn:its hold could not be checked. The next drain posts nothing, the marker still stands,open_tasksreports{'withdrawn': …}, and the row reads healthy. - Why "withdrawn" is always wrong here:
open_taskshas already excluded settled tasks (task_settled), so a withdrawn line on an open held task is always an anomaly, never "answered". - Fix, either or both:
- Treat
withdrawnon an unsettled hold as a problem, e.g. fold it intoTASK_HELD_UNDELIVERED. - Make a read error in the veto defer, handing the file back for a later pass instead of withdrawing it. Also let
holdre-queue a new generation when the marker stands but its line was withdrawn.
- Treat
- Please add a regression: hold, then a drain with a one-shot read failure, then assert the health row.
B2. A failed marker write after arming leaves hold unrecoverable, despite "Run hold again". Reproduced.
- Where:
cli.py:1408-1409arms the notice (arm_notice), then writes the marker (place). Ifplaceraises, the line is armed with no hold behind it, and the message at:1412says "Run hold again". - What the drain then does: it withdraws the line ("no hold stands on it").
- What a retry then does:
unarmedis False (:1383), so_line_stateruns. With no notice file or claim and only a PARKED item, it returnsunknown, and the retry exits rc 3 ("queued before and nothing proves it was delivered"). So does every later retry, and the marker is never placed. - Why the test misses it:
test_a_failed_marker_reports_not_confirmed_and_a_retry_does_not_requeue(test_hatch_adapter.py:6440) retries with no drain pass in between. In production the drain runs every few seconds. - Fix: on a failed
place, un-arm the record under the same lock. Alternatively, treat "armed, no marker, line withdrawn" as unarmed and advance to a new generation. Add a drain pass between the failure and the retry in that test.
B3. Process gate (REVIEW.md §15), unchanged: the "Live path?" box is still unchecked, with no Hatch VM round trip attached. Kewei's 5f5d7a4 review and Susan's condition both name it.
Non-blocking
- Pruning is lazy:
task_hold.pruneonly runs insidehold(cli.py:1320), so marker and notice files for archived tasks accumulate. On Hatch, a cancelled (archived-without-answer) task's armed line has no gate stopping it from posting. - A delivered hold has no maximum age. If the HANDOFF never reaches the owner, the row reads
task-heldforever; consider an escalation after N hours. - Every proactive send now takes the shared
.proactive-send-lock, on core installs too. If that directory can't be created,replyfails with rc 2, which is a new way for answering to break.
Verified fine: routing (the drain's own drain_room, and the thread from the task's own headers); the served line can't carry markers, @ or backticks; no path where a hold blocks or swallows a real answer (reply commits under the send lock, the dispatch check withdraws an unsent line once a result exists, and the task_settled re-check releases a hold that races an answer).
Tests at dd3ce34, in an isolated scratch worktree:
test_hatch_adapter.py: 410 OK (2 skipped).test_hatch_served_setup.py: 226 OK.test_gateway_status.py: pass.- The 18 gateway and proactive test files the diff touches all pass, including credential-sources (156), durable-ack (37), longpoll-timeout (55), voice-room-gate (20) and send-failure-delegation (11).
- The B1 and B2 repros build on the PR's own
Holdclass.
Conflict resolution: - bounded_file_read (src/ and the vendored copy, byte-identical): read_regular keeps this branch's dir_fd/follow arguments and unreadable(), and takes main's `private` refusals (symlink, another owner, group/other bits) and read_private. - agent_identity: this branch's lane-record block stays; resolve() takes main's `strict` signature. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Correction to my review (agent-assisted, Mars, Qingyun's Sutando seat): B3 is withdrawn. The REVIEW.md §15 live witness is posted (#5828 (comment), at 5f5d7a4, real relay token through a scoped shim, nothing installed touched). I had read only the PR body's unchecked "Live path?" box. Sorry for the noise. B1 and B2 still stand at the new head 2f5af2e. The latest merge of main doesn't touch
These are code bugs the witness wouldn't exercise. Both fixes are small and described in the review above. I'll re-review promptly once they land. |
…ecovers after a failed marker - B1: a drain whose read of the hold behind a hold line fails (task_hold. line_unchecked) hands the line back for a later pass instead of letting the dispatch veto withdraw it. A line withdrawn unsent while its hold still stands on an open task now reads task-held-line-undelivered in the health row, not healthy task-held. - B2: hold re-run after its marker write failed and the drain withdrew the armed line takes the arming back (task_hold.disarm_notice) under the send lock, so the retry queues the next generation and places the marker instead of exiting 3 forever. - tests: the seat-startup test records only the bridge's own sleeps, not subprocess's wait loop, which shares the time module. Regressions, each failing at 2f5af2e: a one-shot read failure at the drain is deferred and the line posts exactly once on the next pass; a withdrawn line on a standing hold reports undelivered; a failed marker, then a drain pass, then hold again gives rc 0, a marker and exactly one line in the room. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflict resolution: main() keeps main's order (lock heartbeat, then the proactive-room settle) and this branch's lane publication and its atexit retraction right after the settle, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at 35f6879 (agent-assisted review by Mars, Qingyun's Sutando seat). My change request is resolved.
- B1 fixed (89104a7).
- The drain hands back a hold line whose notice record can't be read (
task_hold.line_unchecked, with_hand_backin the bridge) instead of lettingline_vetowithdraw it. - Health turns a
withdrawnoutcome on an open held task intoundelivered(task-held-line-undelivered), so a lost line is visible, not healthy. - One residual, non-blocking:
task_hold.read()swallows its own OSError and returns None. A transient error reading the marker (not the notice) therefore still withdraws the line, but it's no longer silent, since health now flags it.
- The drain hands back a hold line whose notice record can't be read (
- B2 fixed.
holddisarms a record whose line was withdrawn because its marker never landed (disarm_noticeunder the record token), so the next hold queues a fresh generation and places the marker instead of rc 3 forever. - B3 was withdrawn earlier (the live witness is on the PR).
- Merge 35f6879: the only conflict is in
main(). It keeps main's heartbeat-before-settle order and adds the PR's_lane_identity()andatexitretraction. No commits under qingyun-wu. - Tests at this head, in a scratch worktree with an isolated HOME:
test_hatch_adapter.py -k hold/held/withdrawngives 156 passed (127 subtests), including the new deferral, withdrawn-reads-undelivered and failed-place-then-drain-then-hold cases. CI: 36 pass.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approved at 35f6879e. My CHANGES_REQUESTED from 1e01bd89 is resolved, and an approval from another reviewer (Qingyun) already exists at this exact head.
My two asks from 1e01bd89, re-verified here with mutations
- The deadline covers HTTPS.
remote_gateway_bridge.pystill records the wrapped socket (socks.append(wrapped), line 2831) after the merges. With that line removed, the HTTPS headers and error-body regressions fail (2 failures). - A requeue keeps the earlier "may have landed" outcome.
outbox.pystill setsprior_outcome_unknown, andcli.pyhold_line_outcomestill reads it (line 1228). With that read dropped,Hold.test_a_requeue_keeps_the_proof_that_an_earlier_send_may_have_landedfails.
What changed since my comment at e7774268, also verified with mutations
- Kewei's two P1s at
e7774268(978fd4da,5f5d7a41): a stopped unit now getssystemctl --user startalone, and_successorrequires the final lock to be the same file (lock_snapshot, device and inode) and stillcanonical_lock. Sending the stopped path back to reset-failed/restart failstest_keepawake_never_restarts_a_generation_that_came_up_after_the_final_check. Dropping the final file-id and canonical check failstest_keepawake_certifies_no_successor_whose_lock_stopped_being_canonical(4 failures). - Qingyun's B1 and B2 at
dd3ce341(89104a78): each fix fails its own regression when removed. The fixes are the withdrawn-to-undelivered health mapping, theline_uncheckedhand-back, and thedisarm_noticeretry. - The three merges from main: the only conflict is in
main(), and it keeps both sides. Vendored copies (outbox,bounded_file_read,workspace_lock,gateway_credential_sources,dotspace) are byte-identical tosrc/.
Runs at this head
- Hatch adapter suite: 413 tests OK, 2 skipped. I used a space-free interpreter path and an isolated HOME/TMPDIR.
review-checks.shon the PR diff passes. The onlySUTANDO_WORKSPACEhit in added lines is a test pointing it at its own temp dir, which is not a live default.- CI: 36 pass, 3 skipped.
license/cla: success (1 context). No unresolved review threads.
Kewei
His latest review (5f5d7a41) is still CHANGES_REQUESTED, but it says both remaining code P1s are cleared and that the live witness is the sole blocker. His three P1s from 1d4989f0 and his two from e7774268 look fixed in the cases I ran. The witness has since been posted (issuecomment-6407496412). He should confirm and clear his review.
Live witness
The witness was run at 5f5d7a41, on macOS, with keepawake run by hand. After a real restart, it shows hold → drain → exactly one line in the original room and thread, health reads task-held-for-owner, and nothing repeats after a second restart. It does not cover the Linux systemd and pidfd paths or a Hatch-scheduled keepawake (CI covers the code paths). It was also not run again after 89104a78. That is named here, not blocking.
Non-blocking: if a notice read fails persistently (for example corrupt JSON rather than a transient error), line_unchecked hands the line back on every pass. Health does surface it as task-held-line-undelivered after 10 minutes, so it is visible, not silent.
— Sutando (susans-MacBook-Pro)
…rs its line instead of withdrawing it The delivery core gains a third revalidate answer. `Defer(reason)` releases the claim unsent with no attempt recorded (ClaimBackend.release, on both backends, validating the incarnation like complete), and deliver_one reports DrainStatus.DEFERRED. The proactive drain's existing generic path hands the file back for the next pass. task_hold.line_veto answers Defer when the notice record or the marker cannot be read, so a read error at the send, even after an earlier check passed under the send lock, neither sends nor withdraws the line. The bridge-local pre-check from 89104a7 is removed; the outbox owns the outcome. Regressions: EMFILE inside the dispatch veto's own read gives no send and no withdrawal on that pass, then exactly one line on the next (fails at 35f6879). Contract suite, for both backends: a deferring revalidation makes no provider call, parks nothing and counts no attempt, then the next pass sends; a stale token's release changes nothing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at c08cf74 (agent-assisted review by Mars, Qingyun's Sutando seat). Re-reviewed after the delivery-core Defer.
- Contract: a revalidation may return
Defer(reason).DeliveryCore.deliver_onethen callsbackend.release(token)and reportsDEFERRED, so there is no provider call and no attempt is recorded. Both backends'releasevalidate the claim token's incarnation: A checks the drainer and the current incarnation under the item lock; C checks the token's key and worker parts and that the inflight file exists, then moves it back to ready, quarantining on a collision. A stale token changes nothing. The bridge logsDEFERREDand keeps the file for the next pass. The bridge-local pre-check from 89104a7 is removed in favour of this. - Hold line:
line_vetoreads the notice record and the marker inside onetry, and anOSError/ValueErrorreturnsDefer, not a withdrawal. The new regressions cover EMFILE on the first check and inside the veto's own read under the send lock: nothing sent, nothing withdrawn, exactly one line on the next pass. - Residual, non-blocking (as noted before):
task_hold.read()still returns None, rather than raising, when the marker is unreadable: an_open_dirOSError such as EMFILE, or aread_regularrefusal in_json_at. So an unreadable marker reads as "no hold stands on it" and the line is withdrawn. Health reports that astask-held-line-undelivered, so it's visible, not silent. The tests inject only intonotice_record. If the marker should defer too,read()needs a raising variant forline_veto. - Tests at this head, in a scratch worktree with an isolated HOME:
test_hatch_adapter.pyhold/held/withdrawn/defer gives 158 passed (127 subtests); the delivery-core and backend tests give 6 passed. CI: 36 pass. No commits under qingyun-wu.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at head c08cf7477df1931165d15ed0153c5f1f67308741. The delta from my approval at 35f6879e is one commit across 9 files.
What is verified.
- The new EMFILE test fails against the production code at
35f6879e(AssertionError: False is not true : and nothing withdrawn) and passes at this head. The contract file gives 48 OK, 1 skip. Hold + TaskHold give 121 OK. - Mutations at the head that the tests catch:
- Defer treated as a withdraw (
core.py:160-162removed) - Defer treated as a send
- the
releasecall dropped line_vetoreturning a plain string instead ofDefer
- Defer treated as a withdraw (
_proactive_dispatch_vetoreturnsheldunchanged, so theDefertype reachesdeliver_one.mark_dispatchingruns after the revalidation, so a released claim leaves no dispatch mark.- CI is green and
license/clasucceeds (1 context). There are 0 unresolved threads.
Blocking 1 — incomplete fix: an unreadable marker still withdraws the line for good.
- The commit says an unreadable notice record or marker defers.
task_hold.read()(task_hold.py:165-177) catchesOSError/ValueErrorfrom_open_dirand returnsNone.line_veto(task_hold.py:313, then:321) reads thatNoneas "no hold stands on it" and withdraws.- Repro: the head's own EMFILE test, with the fault moved from
notice_recordtotask_hold._open_dirwhen called fromreadinsideline_veto. Result: nothing posted on that pass, the notice is gone, andhold_line_outcomeiswithdrawn. Two more passes post nothing (0 != 1 : exactly one line on the next pass). - This is the same EMFILE drop on the sibling read. @qingyun-wu's review notes it too, as non-blocking.
- Ask: give
line_vetoa marker read that raises on an unreadable marker and returnsNoneonly when the marker is absent. Add the marker-side EMFILE regression next to the existing one.
Blocking 2 — DesignCClaimBackend.release has no test. The "contract test on both backends" does not run on C.
tests/delivery-core-contract.test.py:455and:472sit inCorePolicy. ItssetUpbuilds onlyDesignAClaimBackend. OnlyContractCaseis run perBACKENDSentry.- No test calls
backend_c.py:278-290. Making itreturn Truewith no move, or dropping its token worker/key check, passes the whole contract file and Hold + TaskHold unchanged. - The same two tests pass against C in a scratch subclass, and fail on the
return Truemutation. - Ask: move both tests into
ContractCase.
Non-blocking. Removing backend A's incarnation/drainer check (backend_a.py:186-187) also passes every test. The stale-token case re-claims as a different worker, and _release_locked refuses that by itself. A same-worker re-claim would pin the check.
Owed, not blocking. The REVIEW.md §15 live witness on record is at 5f5d7a41, not this head.
Verdict: REQUEST_CHANGES. My approval at 35f6879e does not carry to this head.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c08cf7477df1931165d15ed0153c5f1f67308741.
Verdict: REQUEST_CHANGES — this head is not merge-ready.
-
Blocking: a transient hold-marker read failure still permanently withdraws the notice.
task_hold.read()collapses_open_dir'sOSErrortoNone.line_veto()can defer only an exception, so thatNonebecomes"no hold stands on it"; delivery then parks the line as withdrawn. I injectedEMFILEonly into_open_dirwhen called byread()fromline_veto. Exact-head output was:first_pass= {'hold_rc': 0, 'fault_fired': True, 'posts': [], 'notice_exists': False, 'outcome': 'withdrawn'} after_three_passes= {'posts': [], 'notice_exists': False, 'outcome': 'withdrawn'}The new tests exercise
notice_record()failures, not this sibling marker-read path. Please preserve the absent/unreadable distinction for the marker too, and add this production-path EMFILE case: first pass must neither send nor withdraw; a later pass must send exactly once. -
Blocking coverage gap: the claimed two-backend release contract runs only on A. The new Defer/release cases are in
CorePolicy, whose setup hardwiresDesignAClaimBackend; onlyContractCaseis expanded overBACKENDSbyload_tests. Positive mutation control: replacingDesignCClaimBackend.release()withreturn Truestill producedRan 48 tests ... OK(1 skipped). Thus a completely broken C release passes the repository contract. Move these invariants into the backend-parameterized case. Use a same-worker successor in the stale-token case as well, so the incarnation fence is independently pinned rather than masked by the worker-name check.
I otherwise verified the intended exact-head ordering and backend implementations: Defer precedes provider dispatch and attempt accounting; direct A/C probes passed; the 48-test delivery contract and 121 Hatch Hold/TaskHold tests pass; review scanner, required CI, and CLA are green.
Worst case today is permanent user-visible silence from a temporary descriptor/filesystem error. After the code and contract tests are fixed, REVIEW.md §15 still requires a real post-restart round trip on the then-current head; the existing room witness is at 5f5d7a41, before the new Defer/release/hand-back path.
…ase contract runs on every backend - task_hold.read_checked: like read, but an unreadable hold dir or marker raises instead of reading as absent. line_veto uses it, so the EMFILE fault moved onto the marker read (file or dir) gives Defer, not a withdrawal. read itself is unchanged for its other callers. - The deferral and release contract tests move from CorePolicy (backend A only) into ContractCase, so they run on A and C. They also pin incarnation fencing: a stale token, from another worker or from the same worker's earlier claim, releases nothing and never a successor's claim. A no-op `release` fails both tests on either backend; A's release without its incarnation check fails the stale-token test. Regression (fails at c08cf74 for both cases): EMFILE on the marker file or the hold dir inside the dispatch veto gives no send and no withdrawal on that pass, then exactly one line on the next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No conflicts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@cla-assistant check |
REVIEW.md §15 live re-witness at 15dacdeHead What changed since the 5f5d7a4 witness. These are the delivery-path changes this re-witness covers:
The happy path keeps its behavior: a readable hold sends exactly one line. The new branches run only on a read error, and the CI regressions cover them. Token. I used the REAL relay token of a test identity ( (1) Sparrow before and after a restart(2) hold → proactive drain → exactly one line in the original room and thread — PASSThe owner's agent ( Room count, read by the owner's agent: after (3) the held task reads as held, not stuck — PASS
(4) no repeat after a second restart — PASSI checked Teardown. Each scratch Sparrow (88451, 8098, 37987) was stopped by its exact pid after its command line was checked. Afterwards no process referenced the scratch dir. The literal What this proves. At this head, which includes the delivery-core Teardown confirmed by the owner's agent (06:37Z): the vault copy is deleted, and the test identity's relay is restored. |
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Re-review at 15dacde0. Both blockers from my review at c08cf747 are fixed. I checked each with a real run, not by reading the tests. I am not approving yet because no other approval exists at this head. qingyun-wu's approval is on c08cf747, and keweichen's latest review there is still CHANGES_REQUESTED.
Blocker 1: an unreadable marker no longer withdraws the line. Fixed.
- I wrote my own repro. It patches the real
os.openinstead of the module's helpers, places and arms a real hold, and callstask_hold.line_veto.- At
15dacde0, EMFILE on the marker file returnsDefer, and so does EMFILE on the second hold-dir open. Once the fault clears, the veto returnsNone, so the line sends. - At
c08cf747, both cases return"no hold stands on it", so the line was withdrawn.
- At
- The PR's new test
Hold.test_a_marker_read_that_fails_inside_the_dispatch_veto_defers_the_linefails at "and nothing withdrawn" in both subtests when run againstc08cf747'stask_hold.py. It passes at head. _open_dirlets anos.openorlstaterror propagate, and onlyFileNotFoundErrorreads as absent. So the hold-dir mock in that test matches what the real code does.
Blocker 2: Backend C's release is now covered. Fixed.
- Mutations run on
tests/delivery-core-contract.test.py:return Trueat the top ofbackend_c.releasefailsContractCase_C.test_a_deferring_revalidation_neither_sends_nor_withdrawsandContractCase_C.test_release_by_a_stale_token_changes_nothing.- Dropping the incarnation clause from
backend_a.release(backend_a.py:186-187) failsContractCase_A.test_release_by_a_stale_token_changes_nothing. That pins A's fencing, which was my non-blocking item. - Making C return
Trueon a missing in-flight file failsContractCase_C.test_stale_incarnation_completes_nothing.
- At head, unmodified: the contract suite has 50 tests, all OK with 1 intentional skip, and
tests.test_hatch_adapter.Holdhas 94 tests, all OK.
Other gates.
- CI: 38 pass, 3 skipped.
license/cla: success, 1 context.- No unresolved review threads. The PR is mergeable.
- A fresh §15 live witness at this head is posted: #5828 (comment)
One sibling to consider (not blocking from my side).
read_checkedis used only inline_veto. These callers still read an unreadable marker as no hold:task_fields.py:123(theheldflag on a wake item)keepalive.py:1027cli.py:1321,1375and1421
task_fields.py:123matters most. A transient EMFILE there leaves the wake item unflagged, and a fresh worker session could answer a task parked for the owner's go. Please either route it throughread_checked, or say in the PR why it is fine there.
Missing before approval: a second approval at 15dacde0, and keweichen's change-request at c08cf747 re-reviewed.
— Sutando (susans-MacBook-Pro)
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Approved at 15dacde (agent-assisted review by Mars, Qingyun's Sutando seat). The residual from my c08cf74 review is closed.
task_hold.read_checkedlets an_open_dirOSError propagate, and raises when the marker read comes backbounded_file_read.unreadable(why). An absent or malformed marker still returns None, asreaddoes.line_vetonow uses it, so an unreadable hold dir or marker returnsDefer(nothing sent, nothing withdrawn) instead of "no hold stands on it".- The Defer/release contract now runs per backend (A and C), with stale-token fencing pinned both for another worker and for the same worker's earlier claim.
- 15dacde is a clean merge (
git show --ccis empty). No commits under qingyun-wu. - Tests at this head, scratch worktree, isolated HOME:
test_hatch_adapter.pyhold/held/withdrawn/defer/marker: 164 passed (129 subtests).tests/delivery-core-contract.test.py: 50 OK (1 skipped).- Locally,
test_cloud_session_interlock.pytimed out 5× waiting for adapters to hold a dir. That file is unrelated to this PR, timing-sensitive under local load, and green in CI.
- CI: 38 pass. The §15 re-witness at this head is on the PR.
liususan091219
left a comment
liususan091219
left a comment
There was a problem hiding this comment.
Approving at 15dacde0. Supersedes my CHANGES_REQUESTED at c08cf747.
Both blockers verified fixed at this exact head (details in my COMMENT review 5648258590):
- Unreadable marker / hold directory — my own EMFILE repro on the real
os.opennow returns Defer for both the marker and the directory, and the line sends once the fault clears; atc08cf747both withdrew. The PR's regression fails againstc08cf747'stask_hold.pyand passes here. - Backend C
release— a no-op release fails twoContractCase_Ctests; removing A's incarnation check failsContractCase_A.test_release_by_a_stale_token_changes_nothing.
Gates: qingyun-wu APPROVED at 15dacde0; CI 38 pass / 3 skipped; license/cla success (1 context); no unresolved threads; live witness at this head.
Still noted, not blocking: read_checked is only used in line_veto; task_fields.py:123 still reads an unreadable marker as "no hold", so a wake item could miss its held flag. Kewei's CHANGES_REQUESTED at c08cf747 is his to clear.
— Sutando (susans-MacBook-Pro)
keweichen
left a comment
keweichen
left a comment
There was a problem hiding this comment.
Approved at exact head 15dacde09fa79299502cd5f8596761bc06ef18c7. This replaces my c08cf747 CHANGES_REQUESTED; both blockers are resolved, and this head is ready to merge.
- Unreadable marker:
task_hold.read_checked()now preserves the missing/unreadable distinction andline_veto()converts directory or marker read failures toDefer. The committed marker-file/hold-directory regression fails againstc08cf747and passes here. My independent matrix covered directory open, marker open, and marker read withEMFILE,EACCES, andEIO: all 9 cases made no provider call and did not withdraw; the first healthy pass sent exactly one line, and a later pass did not repeat it. Replacingread_checkedwithreadfailed all 9. - A/C release contract: the Defer/release and stale-token cases are now in backend-parameterized
ContractCase; the runner visibly collected them for A and C. The contract suite ran 50 tests (49 pass, 1 documented skip), and Hold + TaskHold ran 122/122. A no-op release failed all four A/C cases; removing incarnation fencing failed the same-worker earlier-claim case on both backends. - Live path: the current-head §15 witness shows a real restarted Sparrow delivered one threaded line in the original room, health reported
task-held-for-owner, and a second restart produced no resend: #5828 (comment)
I also re-read the activated drain path: Defer reaches DeliveryCore unchanged, releases before any provider dispatch/attempt mark, and the bridge hands the proactive file back for retry. Persistent filesystem unreadability can delay the notice, but it now fails closed without the prior permanent user-visible silence. Required CI and CLA are green, the review scanner passes (with a firing hardcoded-path positive control), and there are no unresolved review threads.
The Hatch held-task line (#5828), Hatch self-upgrade to the served target_version (#5676), the served-setup fetch from the broker (#5855), served Hatch wording (#5665), seat-bridge env and token fixes (#5893, #5859), and the claude-local/codex-local connect commands (#5860, #5761). Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Priority
Level: high
Reason: tracker item: a deliberately parked Hatch task leaves the asker in silence and is flagged as stuck.
Closes
Tracker item "Hatch parked task: one-line room reply when waiting for the owner, and health not flagging deliberate holds as stuck". No GitHub issue.
Summary
When the Hatch worker parks a task on purpose because it is waiting for its owner's go, two things go wrong. The room gets no reply, so the asker sees silence. And the keepalive health row reports
task-not-picked-upafter 15 minutes, so a deliberate hold looks the same as a stuck task.This PR adds three pieces:
ag2_sparrow/task_hold.pyis a generic hold record,results/.task-holds/<tid>.jsonholding{reason, since}. It is created withO_EXCL, so only the first caller acts, and it is pruned once the task is archived. It contains no Hatch-specific logic.ag2-sparrow-hatch hold <result_file>. This places theowner-gomarker and queues exactly one line, "Waiting for my owner's go on this.", tagged to the sender. The line goes out as aproactive-hold-<tid>.txtthat carries[channel: <room>]and, for a threaded message,[thread: <root>], so the existing proactive drain delivers it. No new delivery code is added. The routing is checked with the drain's ownparse_markersgrammar before anything is written.holdposts nothing.reply.healthywith reasontask-held-for-ownerandsinceset to the hold time, and it passeshealth_command.validate. A held task never masks an unheld stuck task beside it.The worker_note gains one sentence telling the worker to run
holdonce for an item that waits on the owner's go, then hand it off as before.Overlap check: #5665 (served wording, credential owner) and #5670 (core limit hold, which is a core-relaunch hold in
src/) touch neither the parked-task notice nor the Hatch keepalive stuck check. No other open PR covers this.Checklist
gh pr list --state open, plus feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665 and fix(core): hold a core its provider refused for a limit instead of relaunching it #5670)main(the new tests fail against main's sources, shown below)holdreaching a room is still owed before merge._dirsBefore / after evidence
Before: the new tests, run against
origin/main'scli.py,keepalive.pyand hook script, with only the tests and the newtask_hold.pypresent:After, each run on its own:
The stuck-versus-held kill control is
test_a_deliberately_held_task_reports_held_not_stuck. The held task readshealthy / task-held-for-owner, and the same task with its marker released readstask-not-picked-up.Review 5643381781 (head
b3e6028a, on3bea4d4a)keepawake now replaces a legacy drain through one function,
replace_old_drain:systemctl showreports it active, with the holder as itsMainPID. If the unit is inactive or has anotherMainPID, the holder runs outside it. It is then stopped through_stop, with that function's own lock and generation proof, before the unit starts. A hand-made, unreachable or unreadable unit fails closed and nothing is restarted.drain_predates_send_lockruns again right before acting. If the lock no longer names the selected holder, for example because it rolled to a live generation that declares protocol 3, nothing is touched and keepawake logsrestarted=0with exit 0.restarted=0.The test Sparrow now takes the lock like a real one. The unit regressions drive the production
keepawake()againstFakeSystemd, which reportsActiveStateandMainPID; a restart starts a real successor process that takes the lock and connects.Parent control at
3bea4d4a07be677203b9b72ca23c4d5acde5eecd:What each one shows:
stops_an_outside_legacy_holder_before_starting_the_inactive_unit: at the parent the old holder was never signalled, and the test'sold.wait(5)timed out.restarts_the_unit_that_runs_the_legacy_holder_in_place: a positive control that passes at both heads.reports_no_success_until_a_successor_owns_the_lock: the parent exited 0 and loggedrestarted: systemctl --user restart.leaves_a_generation_that_rolled_before_its_action: the parent restarted the unit.At
b3e6028aall four pass, along withtest_replace_old_drain_fails_closed_wherever_the_unit_or_the_holder_is_unproven. The test that mocked the unit's ownership and restart is replaced.Local results: Hatch adapter 386 OK (2 skipped), served setup 109 OK, crash-receipt, unknown-parks, lane-edges, workspace-lock, singleton, task-lifecycle and
hermetic-env-structuralpass, drift check passes,review-checks.sh --diffis clean. Still owed before approval: the live post-restart witness at the approved head.CI at
b3e6028a: 36 passed, 3 skipped, no failures, diff coverage gate green, on the first run.Review 5642900692, its code-clear matrix and the same-second follow-up (head
3bea4d4a, on166227ba;f1f3c1bbwas an intermediate push)began <= floor(acquired_at). This applies to a lock that declares no start. The process start (whole seconds) and the lock'sacquired_atmust both be bounded epochs.acquired_atandheartbeat_atmust be finite, positive and not ahead of now, withacquired_at <= heartbeat_at. A start in the same second passes: a main-built Sparrow takes its lock about 0.1 s after exec. The next second is refused, as are Infinity, far-future values and string times.f1f3c1bbhad briefly requiredbegan + 1 <= acquired_at, which refused every real legacy drain.cli._lock_recordreads throughbounded_file_read.read_regular(..., follow=False). That opens withO_NOFOLLOW | O_NONBLOCK, checks withfstaton the same descriptor that it is a regular file, and caps the bytes before parsing. A FIFO, a symlink, an oversized file or malformed JSON reads as no lock.cli.canonical_lockbinds the parsed record to this workspace's role, its workspace path (by realpath), schema 1, a valid pid and sane times.proven_sparrowrequires it at the candidate check and again inside_stop, so keepawake and install both send no signal and leave an ambiguous holder running.The matrix
StartSparrow.BAD_LOCKScontains: a start the second after the lock, acquired at Infinity, far future, acquired after its heartbeat, a string time, another workspace, no workspace, another role, another schema, another declared start, another pid, malformed JSON, a symlink to another workspace's lock, a FIFO, and an oversized lock. It runs through three regressions:test_stop_signals_only_the_proven_sparrow_of_the_lock_it_rereads(_stop,os.killmocked), with positive controls at the same second and earlier.test_keepawake_signals_no_holder_its_lock_cannot_prove(realkeepawake()), with a positive control: the lock's own legacy Sparrow is replaced.test_install_signals_no_outside_sparrow_its_lock_cannot_prove(install'sensure_sparrow), with a positive control: install replaces it.Two more regressions:
test_a_legacy_sparrow_that_took_its_lock_a_tenth_of_a_second_after_starting_is_replaced(lock at start +0.1 s and +0.9 s, through_stopand realkeepawake()), andtest_unmanaged_health_problem_reads_the_lock_holder_and_its_environment.Parent control: the new regressions run against
166227ba641a67db051cf539f214dd1de8218ac0. With the FIFO in the matrix, the keepawake and install runs block; they were rerun without that one case to show the rest:At
3bea4d4aall of them pass. Hatch adapter 383 OK (2 skipped); one run had anold.wait(5)error in the untouchedtest_keepawake_replaces_a_sparrow_with_a_different_health_command, which passed alone 3 times and in 2 further full runs. Served setup 109 OK; crash-receipt, unknown-parks, lane-edges, workspace-lock, singleton, credential-sources, task-lifecycle andhermetic-env-structuralpass; drift check passes;review-checks.sh --diffis clean. Still owed before approval: the live post-restart witness at the approved head.CI at
3bea4d4a: 36 passed, 3 skipped, no failures, diff coverage gate green. Two reruns were needed. The first cleared aworker-config-directorytiming flake, and both cleared thehermetic-env-structuralcoverage timeout that main shares.Review 5642776054 (head
166227ba, on930017e6)keepawake signals only a proven Sparrow of the lock it re-reads. The lock a released main takes carries no start time, so the pid it names may have been reused.
keepalive.proven_sparrow(pid, lock)requires two things. The live pid must run a Sparrow command line. Its start time must equal the one the lock declares, or, for a lock that declares none, be no later than the time the lock was taken.drain_predates_send_lockreturns no candidate without that proof._stopre-reads the lock and repeats the proof right before the signal, for every caller (keepawake's replace and install's outside-Sparrow replace), and sends nothing without it.Regressions, each run at
930017e6:930017e6166227baStartSparrow.test_keepawake_never_signals_a_recycled_pid_under_a_legacy_lock: realkeepawake(), a legacy lock naming a live non-Sparrow pid, or a Sparrow started after the lock_stopcalled once in both cases_stopnever called,restarted=0 reason=unknown-env, process aliveStartSparrow.test_keepawake_still_replaces_a_genuine_legacy_sparrow_stopcalled once, SIGTERMStartSparrow.test_stop_signals_only_the_proven_sparrow_of_the_lock_it_rereads_stoptakes no lock check (TypeError)os.killfor another pid, a newer start or another declared startThe test fixture now gives each fake Sparrow a Sparrow argv where
/procis missing, and a start time just before the lock it takes.Local results: Hatch adapter 379 OK (2 skipped), served setup 109 OK, crash-receipt, unknown-parks, lane-edges, workspace-lock, singleton, task-lifecycle and
hermetic-env-structuralpass, drift check passes,review-checks.sh --diffis clean, and every changed line inkeepalive.pyis covered (24 of 24). CI at166227ba: 36 passed, 3 skipped, no failures, diff coverage gate green. Still owed before approval: the live post-restart witness at the approved head.Review 5642460727 (head
930017e6, on6a7b99a5)task_hold's notice rewrite andagent_identity's record write now loop until every byte is written. Before, a short write could leave a truncated notice record, soholdreported success while its line was withdrawn.workspace_lock.acquire(declares=...)); heartbeats keep these fields.keepawakejudges a holder whose lock declares them by that alone. A lane record the drain could not write, or could not even create empty, is no longer evidence against it. A declaration under a reused pid (start mismatch) is not acted on. Locks that declare nothing (the released main) keep the lane-record and grace-period rules.agent_identity.owned_bydecides whether a lane record was written by the live process itself.hold's lane snapshot andkeepawakeboth use it, so a fresh record that another process left under this pid authorizes nothing.HTTPErrorresponse is closed once its body is buffered.Regressions, each run at
6a7b99a5:6a7b99a5930017e6Hold.test_a_hold_whose_writes_are_short_still_posts_its_one_line: realhold()and drain with 1-byte writesposts=0,PARKED withdrawn: ... not a recordStartSparrow.test_keepawake_keeps_a_drain_whose_lane_and_its_fallback_both_failed:_write_atand_poison_atboth ENOSPC, lane absentrestarted=1restarted=0over two passesStartSparrow.test_keepawake_judges_a_declaring_lock_by_its_protocol_and_its_processrestarted=1StartSparrow.test_the_drain_declares_its_protocol_and_start_in_the_lock_it_takes_lock_declaresHold.test_hold_refuses_a_fresh_lane_record_another_process_wrote_under_its_pid(pid, 'ag2.space', True)acceptedHold.test_the_drain_withdraws_a_hold_line_its_record_does_not_arm(legacy, no marker)DELIVEREDHold.test_the_deadline_covers_a_slow_https_error_bodyraised.fp.closedLocal results: Hatch adapter 376 OK (2 skipped), served setup 109 OK, crash-receipt, unknown-parks, lane-edges, workspace-lock and singleton suites pass,
hermetic-env-structuralOK, drift check passes,review-checks.sh --diffis clean. Still owed before approval: the live post-restart witness at the approved head.CI at
930017e6: 31 passed, 4 skipped, 3 failed, the same after one rerun. The first failure is thehermetic-env-structuralcoverage timeout that main shares. The second istests/skills/process-hygiene/reap-orphans.test.py, a timing case this PR does not touch; it also failed on three recent main runs and passes locally. The third is thetsc + tests (clean install)aggregate of those two.Review 5642068743 and the stale-record follow-up (head
6a7b99a5, on05920495, which was on59f15eb2)05920495(follow-up from a second review):keepawakejudges a stale lane record of the live holder by the protocol it declares, instead of falling through to the grace-period replace.3ba5d101merges main; the proactive outbox directory now has one owner,proactive_routing.PROACTIVE_OUTBOX_DIRNAME.The three blockers from review 5642068743 (fixes in
0502b87b, test hygiene in6a7b99a5):holdcannot take it back. The binding now lives in the notice record (task_hold), which holds a generation and an armed flag. The drain's dispatch-time recheck runs under the send lock. It sends a hold line only if it is the record's current generation and was armed by a hold that stands. A refused commit leaves its generation unarmed, so it is withdrawn when the drain tries to send it. The nextholdqueues a new generation under a new name. The old one can never become sendable again, and a failed advance, publish or arm can be retried.SEND_LOCK_PROTOCOLis now 3, soholdrefuses a drain without this recheck. The_retire_linerename rollback is gone.keepawakejudges a fresh or stale record only when it belongs to the live holder: the start time matches, or, when there is no start time to compare, the record was written after the holder took its lock. A predecessor's leftover record under a reused pid is no evidence in either protocol direction.publish_laneempties the record in place, or creates it empty. Truncation needs no free space. The record then reads as invalid andkeepawakereplaces nothing.Regressions (all in
packages/ag2-sparrow/tests/test_hatch_adapter.py, collected by the normal suite, run withpython3 packages/ag2-sparrow/tests/test_hatch_adapter.py <Class.test>), each run at05920495:059204956a7b99a5Hold.test_a_refused_hold_line_the_drain_claimed_before_the_lock_is_never_sent: real_post_proactive()claims line A, then waits on the send lock while the fence failshold_rc=3 successor_posts=1 outbox=['DELIVERED']PARKEDwithdrawn, and A put back under its live name still never sendsHold.test_the_next_hold_queues_a_new_generation_the_drain_sends_onceHold.test_a_hold_whose_arming_or_advance_fails_stays_retryableHold.test_the_drain_withdraws_a_hold_line_its_record_does_not_armDELIVEREDStartSparrow.test_keepawake_judges_a_stale_record_only_when_this_process_wrote_itrestarted=0; residue without the protocol, holder at 5 s:restarted=1StartSparrow.test_a_failed_lane_publication_reads_invalid_and_replaces_no_drain: the drain's own_lane_identity()on ENOSPCrestarted=0over two passesKept explicit from review 5641833625:
test_a_fifo_lane_record_is_refused_without_blocking_the_reader, andtest_lane_state_tells_absent_invalid_unreadable_fresh_and_stale_apart, which asserts that absent, invalid and unreadable are three distinct outcomes.Local results: Hatch adapter 371 OK (2 skipped), served setup 109 OK, crash-receipt, unknown-parks and lane-edges suites pass,
hermetic-env-structuralOK, drift check passes,review-checks.sh --diffis clean, and this round's changed lines are fully covered (the vendored copies are omitted by.coveragerc). The P2HTTPErrorclose is not changed. Still owed before approval: the live post-restart witness at the approved head.CI at
6a7b99a5: 32 passed, 4 skipped, 2 failed. The one real failure istests/hermetic-env-structural.test.pytiming out at its 240 s cap under coverage, in shard 6/9. It failed this way on the first run and on two reruns.tsc + tests (clean install)fails only because it aggregates that shard, and diff coverage is skipped because of the same failure. Main's own CI atb211ab9b, the base merged here, fails the same way. Locally the file passes in 92 s, against 113 s on main. Aworker-config-directoryflake on the first run passed on the rerun.Three blockers from review 5641833625 (head
59f15eb2, on57e0f7e6)agent_identity.lane_statereturns one of five states: fresh, stale, absent, invalid or unreadable.keepawakereplaces a drain only on positive evidence: a fresh record of the lock holder that lacks the send-lock protocol, or no record at all once the grace period has passed. A corrupt, emptied, tombstoned, symlinked or FIFO record reads as invalid, and the drain keeps running. The lane reader and the hold-marker reader both use the sharedbounded_file_read.read_regularreader, which now takesdir_fdandfollow=False. It opens files non-blocking and checks the type before reading, so a FIFO no longer hangsholdorkeepawake.holdrenames the line it queued out of the queue, matching it by exact inode, and clears its notice mark. No successor drain can post it, and a laterholdqueues it again. If a drain that ignores the lock already claimed the line,holdexits 3 and says the line may post. A line queued by an earlier run is left alone._connect_bynow builds each candidate's socket inside the candidate's own failure handling, so an unsupported first family falls through to the next address. Only sockets that were actually created are recorded and closed.Each regression was run at
57e0f7e6as a control, reproducing the reviewer's probe:57e0f7e6{/ emptied / tombstone / symlinkrestarted=1 reason=drain-predates-send-lock, drain killedposts=["@bob:ag2.space Waiting for my owner's go on this."]OSError errno=47, IPv4 never triedLocal results: Hatch adapter 364 tests OK (2 skipped), served setup 109 OK, crash-receipt and unknown-parks suites pass,
gateway-credential-sources156 OK, drift check passes,review-checks.sh --diffis clean. This round's changed lines are 99 of 99 covered. CI at59f15eb2: 36 passed, 3 skipped, diff coverage gate green. The P2HTTPErrorclose is not changed in this round. Still owed before approval: the live post-restart witness at this head.Four blockers from review 5641556731, plus one edge (head
57e0f7e6, fixes inee341d83, on1e01bd89)remote_gateway_bridge.py. Each connect candidate gets only the remaining budget, and the TLS handshake is cut by the same timer.prior_outcome_unknownmark insrc/outbox.pyand its vendored copy. After a requeue,replyreports "the room may already show it" instead of "will not post".agent_identity.lane_recordtells an unreadable record apart from a missing one.keepawakeleaves a drain alone when it cannot read the drain's record, and notes this on stderr.holdreads the lane's pid, homeserver and send-lock capability as one snapshot. It re-reads the snapshot under the send lock before the publish and again before the marker, and exits 3 if anything changed._publish_onceignores a failed unlink of the temp file once the result is linked.Each regression was also run at
1e01bd89, where all of them fail:1e01bd89replyafter a requeueundeliveredinstead ofunconfirmedLocal results: Hatch adapter 354 OK (2 skipped), served setup 109 OK, crash-receipt and unknown-parks suites pass, drift check passes,
review-checks.sh --diffis clean. CI at57e0f7e6: 36 passed, 3 skipped. Still owed: the live post-restart witness at the approved head.Port onto main's proactive outbox (head
1e01bd89)Why. CLAUDE.md gives delivery of an already-published result one implementation: the outbox. After #5857 (
99536576), main's proactive drain is aDeliveryCorelane. That lane owns the claim and the retries. It parks an uncertain send and never resends it, using a dispatch mark written before each send. It also rechecks a message about a task at send time and withdraws the message once the task is answered.History. No force-push. Three commits on top of
7bddd8a3:24888ed3: the merge of main, which carries the port;2b8b0c7c: the deadline change, kept as a separate commit so it can be reviewed on its own;1e01bd89: a comment within the two-line cap.Dropped as duplicates of #5857. The drain conflicts are resolved by taking main's drain whole.
_send_claimed(send and settlement)_send_proactiveand the outbox settle every send_send_proactive.proactive-attemptscount, its pre-send gate,park_attempts,_resolve_send_failureseedingRetryPolicy(park_interrupted_send=True);tests/proactive-retry-bound.test.py,tests/gateway-proactive-crash-receipt.test.py,tests/gateway-proactive-unknown-parks.test.py<stem>.withdrawnsentinel, its archive-failure rule, and reply's withdraw-and-undo of the line_proactive_dispatch_veto) withdraws a message about an answered task at its sendproactive-about-<task>--0.txtclaim_path, and this PR's changes to orphan recovery<stem>.sending.<pid>.<ns>and recovers its own claims_recover_orphan_proactiveKept. These are Hatch-specific, or something main lacks:
core.deliver_oneinsidesend_lock);hold's commit under the lock;reply's commit under the lock. It claims the task and publishes the answer, and releases the claim if the publication fails;send_lock(protocol bumped to 2, so a drain from an intermediate fix(ag2-sparrow): a Hatch task parked for the owner's go posts one line and reports held, not stuck #5828 head is replaced);About-task names now accept a named instance's task id (
~), in both copies ofproactive_routing.Deadline (
2b8b0c7c). Main's proactive send has the same gap. It calls_req(..., timeout=20), a per-socket timeout with no wall-clock bound, and that send now runs under the lock thatholdandreplywait 30 s for. The commit:_reqrecord its socket, with a timer that shuts the socket down at the deadline. That covers the status line, headers, body and an error's body;_read_bylook the socket up on each pass and useread1.Contracts as tests against the ported code. Each mutation below removes one kept piece; every named test fails under it:
The rest, now enforced by main's outbox. These tests point at it:
test_a_send_with_an_unknown_outcome_is_never_reported_as_withdrawn: a timeout is parked, one POST ever, and reply reports it as unconfirmed.test_a_send_that_began_counts_against_the_budget_before_the_post: child processes POST and die before settling, and only 1 POST is ever made.test_a_plain_fixed_length_success_is_delivered_and_settled: a real localhost 200 through main's path.test_a_requeued_parked_line_gets_a_fresh_budget: main's operator requeue command restores the body, and the line is sent once more.test_a_send_whose_dispatch_mark_cannot_be_written_is_never_sent.tests/proactive-retry-bound.test.py.Finding in main, not changed here.
outbox._read_itemreads an item file it cannot parse as a fresh QUEUED item with 0 attempts. A corrupt proactive item could therefore lose its dispatch mark and be sent again. The Hatch side fails closed on an unreadable item (lane_itemsraises,holdexits 3), but the outbox itself does not.After, at
1e01bd89:tests/_helpers/proactive_lane.py's fake_reqnow accepts the deadline keyword.Still owed: the real post-restart Hatch witness.
Five blockers from review 5641179085, plus two follow-ups (head
7bddd8a3, on9b4b26ef)_read_bycalledsettimeouton a socket the response had already closed after its last byte. It now looks up the response's socket on every pass, and a closed one simply ends the read._send_claimedalso settles any other error raised once a send has begun (OSError,http.client.HTTPException, or a non-JSON body asValueError). It treats each as an unknown outcome, under the same bounded retry as a timeout._reqwith a deadline opens through its own opener, whose connection classes record the request's socket. A timer shuts that socket down at the deadline, which ends any read in progress: status, headers, body, or an HTTP error's body. That error body is read inside the timer and kept for_http_error_body, and a cut request raisesTimeoutError. The two ack-drip tests ingateway-longpoll-timeoutnow patchOpenerDirector.open, since that is the path a deadline-bound request takes.MAX_TRANSIENT_ATTEMPTS), or the count cannot be read, it parks the line unsent inundelivered/<name>.record_attemptrefuses to overwrite an unreadable record. So a crash after the POST counts against the budget, and onlyMAX_TRANSIENT_ATTEMPTS + 1sends can ever begin.reply's commit is failure-atomic._commit_answer_lockedclaims the task, withdraws the line (recording an undo for each step) and publishes the answer. Any failure undoes the steps in reverse: the queued line is republished from its saved text, a sentinel it wrote is removed, and the reply claim is released. Then it raises, andreplyexits 2. A retry finds the task as it was and answers it. If another result already holds the name, the task is answered and its line stays withdrawn.keepawakereplaces main's drain.drain_predates_send_lockalso returns the pid of a live, connected Sparrow that has held its lock for more thanLANE_GRACE_S(120 s, from the lock'sacquired_at) with no lane record at all. That is how the released main's drain behaves. A Sparrow that started more recently is given time to publish its record.Follow-ups:
<stem>.parked. A file re-queued later therefore starts a fresh budget, whileattempts_ever, which reply's report reads, still counts every send that began. An unreadable count stays where it is and keeps refusing sends. A confirmed delivery clears both records.test_an_attempt_that_cannot_be_recorded_is_never_sent).Controls. With
9b4b26ef's bridge,delivery_destination,agent_identity,cliandkeepaliveswapped in, andLANE_GRACE_Sshimmed:_post_proactiveand_req.os._exit(77)before settling, with the bridge's orphan recovery running between them._publish_once, after a real hold.New follow-up tests:
test_a_requeued_parked_line_gets_a_fresh_budget,test_a_parked_line_that_was_sent_is_never_reported_unsent,test_an_attempt_that_cannot_be_recorded_is_never_sent,test_keepawake_gives_a_just_started_sparrow_time_to_publish_its_lane.After, at
eef50592:7bddd8a3: the error body survives the scrub.gateway_callscrubs the exception it re-raises and empties every bytes attribute. So the error body read inside the deadline arrived empty, and the room-upgrade 400 path never saw its replacement room. CI oneef50592caught this insrc/remote-gateway-bridge.test.py. The body is now kept in anio.BytesIO, which the scrub carries the same way it carries the response object.test_a_deadline_bound_error_keeps_its_body_through_the_scrubpins it and fails ateef50592(None != '!new:ag2.space').Blocked: conflicts with main. CI has not run at
7bddd8a3. The branch conflicts with main since #5857 (99536576), which moved the proactive drain onto an outboxDeliveryCore. #5857 brings:The conflicts are 7 hunks in
remote_gateway_bridge.py, plustests/gateway-disposal-delegation.test.py. They replace the code this PR's drain-side rounds changed. Resolving them means porting the hold line onto that outbox path, not merging mechanically; I have left it until that design is agreed.Four blockers from review 5640335453 (head
9b4b26ef, on64d59747)Producer transaction. Three steps now all run under the one send lock:
hold's commit (_hold_under_lock): its settled check, the line's publication and the hold marker;reply's commit: the line's withdrawal and the answer's_claim/outbox check/_publish_once;So an answer cannot land between hold's check and its publication, and a line published first is withdrawn by the answer that follows. The reply and the drain no longer interleave as in your reproduction.
Durable outcome.
.withdrawnsentinel:_retire_proactivenow returns whether it retired, and the sentinel goes only then..proactive-attempts/<stem>, written with temp, fsync and replace. If the count cannot be written, the line is handed back unsent._resolve_send_failureseeds its budget from that count, so a restart does not reset it.Progress bound.
send_locktakes a wait: 30 s forholdandreply, 2 s for the drain. Acquisition pollsLOCK_NBuntil the deadline.holdexits 3,replyexits 2 ("run reply again"). A drain that times out hands the claim back unsent.deadline = now + 15 s._read_bynow reads withread1, becauseread(4096)on a fixed-length body waited for every byte however slowly it arrived. That change also covers the otherdeadline=callers.Upgrade fence.
send_lock: 1in its lane record.holdrefuses (exit 3, with a restart hint) while the live Sparrow's record lacks it.keepalive keepawakerestarts a live Sparrow whose own lane record lacks the protocol: throughsystemctl --user restartwhen the install's unit runs it, by stop-and-start for a detached one, and with a message for a hand-made unit.Regressions. All drive the production
hold/reply/_post_proactive/keepawake. Each fails at64d59747; the control swaps in that head's bridge,delivery_destination,agent_identity,cliandkeepalive, with the three new constant names shimmed:holdpauses aftermark_notice, the reply starts,holdpublishes and places, and the drain runs betweenplaceand hold's last check._reqin the drain suites now accept**_kwfor the newdeadlineargument.After, at
9b4b26ef:Remaining boundary: a drain that has won the lock and begun its send puts the line out ahead of the answer, and the reply then reports that send's outcome.
CI at
9b4b26ef: 34 pass, 3 skipped. One unrelated flake was rerun once:tests/skills/process-hygiene/reap-orphans.test.py,RealOrphans.test_a_process_that_changes_between_scan_and_signal_is_left_alone (change='cwd'), "the orphan was not a candidate".Still owed: the real post-restart Hatch witness, at this head or later.
Lock semantics (head
64d59747, onaa90bce8), for Kewei's correctionThe linearization point is acquiring the send lock. It is not the moment the POST reaches the wire.
results/.proactive-send-lock/lock. It is opened withO_CREAT | O_NOFOLLOWand never unlinked, so every process and every restart locks the same inode; there is no unlink-and-recreate split. It is one lock for the results dir rather than one per stem. That gives the same stability without leaving a never-deleted lock file per message. The cost is that areplywaits behind at most the one send in progress._req, until the send is settled: receipt and archive, bounded retry, park, or room-upgrade hand-back. The send and settle code moved, unchanged, into_send_claimed, which runs insidewith send_lock(...).SendLockUnavailable, and the drain hands the claim back unsent.raise, leaves through the context manager, which closes the fd.reply, andhold's answered-meanwhile path._withdraw_linetakes the lock and returns where the line stands, and the reply prints that on stderr:withdrawn: the line was queued or claimed, so it is unlinked or the sentinel is written;posted: the drain's receipt says it was delivered;unsent: it was parked;unconfirmed: it left the queue with no receipt;none: no line was ever queued, and nothing is printed.posted, orwithdrawnwhen the send failed and was handed back for a retry the sentinel then stops. It never claims a withdrawal it did not make.Tests:
Hold.test_lock_order_a_the_reply_withdraws_first_and_the_drain_posts_nothing: the reply runs after the drain's claim and before its lock. The reply reportswas withdrawn; it will not post, and the drain makes zero POSTs.Hold.test_lock_order_b_the_drain_sends_first_and_the_reply_reports_the_post: the reply starts at_reqentry and blocks on the lock. There is exactly one POST, made while no answer existed. The reply then reportswas already posted before this answer, never says "withdrawn", and leaves no sentinel.Hold.test_every_send_outcome_releases_the_lock_and_keeps_its_inodecovers URLError, TimeoutError, HTTP 503, an unconfirmed 200, and HTTP 401 raised out of_post_proactive. For each it checks that exactly one send was made while the lock was held, that a non-blockingflocksucceeds afterwards, and that the lock file's inode is unchanged.delivery_destination.pyandcli.pyataa90bce8and the tests from this head, (a) and (b) both fail on the missing outcome report.CI follow-up,
64d59747. Two CI fixes for this round:tests/gateway-disposal-delegation.test.pylists_send_claimedas a known file mover, beside_post_proactivethat it came from.fcntlinside its POSIX-only helper. The hermetic structural gate's Python 3.9 stdlib fallback does not listfcntl, so it cannot be imported above the isolation.One unrelated flake was rerun once:
AdapterLifecycleTest.test_a_provider_child_outliving_the_adapter_produces_at_most_one_turn, in the node + out-of-tree job.After, at
924cd4c2:Withdrawal/send linearization (head
aa90bce8, on53182d5d), for Kewei's review 5640022116Bug (TOCTOU). The drain stat'ed
<stem>.withdrawnand then made a separate_reqcall, with nothing tying the two together. Areplycould commit its answer and the sentinel in between, and the stale "absent" still sent the held line, after the answer.Fix: one serialization point.
delivery_destination.send_lock(result_dir)is an exclusiveflockonresults/.proactive-send-lock/lock. It lives in a directory of its own, so code that lists the results dir sees no lock file._send_unless_withdrawn, checks the sentinel and makes the POST, all under that one lock.reply, andhold's answered-meanwhile path, both go through_withdraw_line, which takes the same lock to unlink the queued line and write the sentinel for a claimed one. That happens before the answer is queued.So either the drain's POST finishes before the withdrawal, which means before the answer is queued, or the drain sees the sentinel and retires the line unsent.
Edge cases:
replycannot take it, it still unlinks a queued line and prints a note.flock(Windows, where Hatch does not run), the lock is skipped.Remaining window. A POST already on the wire when the drain's 15 s timeout fires: the drain gives up, the answer proceeds, and the gateway may still deliver that line after the answer. A smaller leftover: a
replythat saw the claim just before the drain archived it can leave one stray.withdrawnfile. Nothing reads it again, because a task is held at most once.Regression test:
Hold.test_an_answer_after_the_drains_withdrawal_check_never_precedes_the_line._reqentry, after the drain's final check, in a separate thread with a 1 s barrier.delivery_destination.pyandcli.pyat53182d5dand the tests from this head, the test fails. The line is posted after the answer is committed:[(…, True)] != [(…, False)].After, at
aa90bce8:Fixture provenance (head
53182d5d, onbaf0b281)tests/hatch_hook_0_6_0.sh.provenance.jsonrecords where the copied hook fixture came from:sparrow-v0.6.0;ca6c54218aa0a3275d442c0fe07063be5760aa7f;packages/ag2-sparrow/ag2_sparrow/adapters/hatch/ag2-sparrow-hatch.sh;22be7d544488b77abaff70459fcd6a7c39f1f4cc7c5acdaa66b0bb7cce392258, which I checked equalsgit show ca6c5421:<path>.The old-hook test now asserts that the fixture still hashes to the recorded value. Appending one line to the fixture makes the test fail.
Rolling-upgrade fix (head
baf0b281, on060846db), for Kewei's review at060846dbBug.
install_hook()stages the new hook script beside the registered one, and a pip upgrade rewrites neither. So a hook registered from 0.6.0 can run against this package's Python. That hook has noHELD_NOTE, and its rewake note ends "otherwise answer it now". Python marked the itemheld: true, but nothing told the worker what that means, so a held item was re-woken as answerable.Fix: the hook declares the capability, and Python checks it.
task_fieldswithHATCH_HOOK_HELD_AWARE=1set on that invocation.task_fields' CLI leaves held tasks out of the list. Such a hook never wakes a worker for a held task. Once the hold is released, the item reaches it as an ordinary open item.hold,reply) still see held tasks.heldconcept then still works, where an unknown flag would make it exit 2 on every poll.Tests:
HookScript.test_a_hook_registered_before_the_held_branch_never_wakes_for_a_held_itemruns main's 0.6.0 hook, copied unchanged astests/hatch_hook_0_6_0.sh, against this package. One item is held after its first wake and both are made due for a rewake. Only the unheld item is woken. After the hold is released, the held item is woken again as an ordinary item.task_fields.pyand the hook at060846dband the tests from this head, the test fails. Both items are woken, as Kewei reproduced.HookScript.test_held_items_are_listed_only_to_a_hook_that_declares_it_explains_them: thetask_fieldsCLI lists a held task only withHATCH_HOOK_HELD_AWARE=1, not with the variable unset or set to0.test_a_held_item_is_rewoken_as_held_and_never_answeredstill passes with the new hook: held items are still listed withheld: trueandHELD_NOTE.After, at
baf0b281:Still owed: the live witness (a real post-restart Hatch
hold, through the proactive drain, posting exactly one line in the original room and thread, with health reporting the task as held).Review fixes 2 (head
060846db, on1cb676c8), for review 5639763810send-failure-delegation.test.pychecks behaviour.test_sparrow_parks_to_the_repo_wide_quarantineno longer matches source text. It imports the bridge in a subprocess with a hermetic temporary results directory, claims aproactive-*.txt, and passes a permanent (404) failure through the bridge's own_resolve_send_failure. It then asserts that the body lands, unchanged, inresults/undelivered/and that nothing else is left. The file now callsisolate_os_environ()at module top (060846db), whichhermetic-env-structuralrequires of any test that spawns the bridge. WithUNDELIVERED_DIRset to"archive", the test fails.runtime_text.held_linealso refuses any line whereresult_markers.neutralize_markers(line) != line. That function is the marker grammar's own inverse, so no second copy of the grammar is added. A mid-line[dm-only],[attach:],[file:],[send:],[channel:],[thread: …]or[no-send]is refused, while a plain[soon]still passes.@is refused, which covers@room,@user:serverand a bare@name.Tests:
test_only_one_plain_line_is_taken_from_the_served_textgains all the cases above.test_a_mid_line_marker_or_mention_in_the_served_line_falls_backcovers[dm-only],[attach: /etc/hosts],[channel: …],@roomand@alice:ag2.space. Each one posts the built-in line with exactly the task's own[channel:]and[thread:].Same rules as the server. ag2-space/ag2space-backend#2916 (
2d28109f) puts the rule in thepatternofRuntimeText.held_line. It refuses every[,@and backtick, every control, format or separator character except a plain space, and a leading space. That is stricter than this client check, so any line the server accepts is posted. Markdown or HTML is posted as plain text and is not refused.After, at
9b1e6605:Review fixes (head
1cb676c8, on8e2965ab)f8764790). Before,replyunlinked only a hold line that was still queued. Once the proactive drain had renamed the line to.sending.<pid>, the answer went out and the line could still post after it.<stem>.withdrawn(delivery_destination.withdrawn_path)._post_proactivechecks that sentinel immediately before the POST. If it exists, the drain retires the claim toarchive/unsent and removes the sentinel. A sentinel that arrives after a send is removed on the delivered path.reply, andhold's answered-meanwhile branch, both go through_withdraw_line. It unlinks a queued line and writes the sentinel when the line is claimed, before the answer is queued.1cb676c8).__version__is now0.6.1.dev1, soholdships in 0.6.1. ag2-space/ag2space-backend#2916 raises the served Muse list'smin_packagetoag2-sparrow>=0.6.1, because itsworker_notetells workers to runhold.Test:
Hold.test_an_answer_between_the_drains_claim_and_its_send_posts_no_line. The real drain claims the line, the worker answers throughreplyat that moment, and the drain continues. The test checks:delivered.Controls: disabling the drain's check fails the test, and so does disabling
reply's sentinel write. Each fails with the line posted.After, at
1cb676c8:Served held line and merge with main (head
8e2965ab, onc801e868)This round makes the parked-task line come from the backend's served Hatch text. It also merges
mainso the branch is no longer conflicted. No force-push; both are new commits on top ofc801e868.Merge
c2869345brings inorigin/mainat3f38f181, which includes feat(ag2-sparrow): serve the Hatch worker's wording from the backend, with a built-in fallback #5665's served runtime text. The conflicts were all of the "both sides added" kind and are resolved as unions:hold/wake-contextsubparsers;agent_identity's lane record beside main'spinned_environ/launch_environ;_log_linebeside_refresh_setup.The health row keeps main's served tunables (
hook_silent_age_s,stuck_task_age_s) around this PR'sopen_tasks.Semantic fixes the merge needed, each one caught by main's tests:
builtin-runtime-text.json'sworker_notegains the hook script'sholdsentence, soDocumentedCommands.test_built_in_notes_are_the_hook_scripts_notesholds.diagnostics.describe, which main's sink guard requires.open_tasksinstead of the removedoldest_open_task.valid_pid, which both sides added to the outbox, is now defined once (ruff F811), in bothsrc/outbox.pyand the vendored copy.Feature
8e2965ab.holdnow postsruntime_text.held_line(served by ag2-space/ag2space-backend#2916) from the same setup cachewake-contextreads, under the same revision and validity rules (current_cache,served_text). The line must be one printable line of at most 300 characters, with no backtick and no leading[or space. That way a served line can never add a[channel:]/[thread:]marker or a command.holdposts the built-inHOLD_NOTICE. That is the case on every install today:setup_fetchhas no transport until feat(ag2-sparrow): Sparrow fetches its served setup from the broker when a heartbeat announces a new revision #5855 lands, and the backend servesheld_lineonly once fix(health-check): report how long the gateway has been down #2916 merges. It is also the case when the cached revision is not the one Sparrow last heard announced, or the served text is invalid.Tests (
Hold, new):test_the_served_line_is_posted_once_in_the_tasks_own_thread: the body is exactly[channel: <task room>],[thread: <task root>]and@sender <served line>. After delivery, a re-park under a newer served revision posts nothing.test_without_a_usable_served_line_the_built_in_one_is_posted: a served"Waiting.\n[channel: !other:…]"falls back to the built-in line, and the only redirect is the task's own room.test_a_served_line_for_a_revision_sparrow_did_not_announce_is_not_used.test_only_one_plain_line_is_taken_from_the_served_text: newline, U+2028 and\rinjections, leading[channel:/[thread:/ space, empty, a backticked command, 301 characters, non-strings,text_schema_version2, and no cache all yieldNone; 300 characters pass.Control. With
holdreverted toHOLD_NOTICE,test_the_served_line_is_posted_once_in_the_tasks_own_threadfails. The two fallback tests are guards and pass either way.After, at
8e2965ab, each file run alone withniceat a 1-minute load near 11:Known limit. If a first
holdexits 3 and the served revision changes before the retry, the retry's line text differs from the queued one. It then readsunknownand exits 3 again. It never posts twice, and the task keeps readingtask-not-picked-upuntil it is answered.Overlap. #5676 (
installhunks ofcli.py, andkeepalive.py) and #5855 (refresh_cache's fetch call inruntime_text.py) are untouched.held_lineis a new function afterserved_text.Review round 1 (head
f51518e1, fixes on2eaad0e1)Addresses review 5624650007 and the second review from Qingyun's agent. Both were at
2eaad0e1.Task-id boundary (security).
task_hold.marker_pathnow validates the id with the shared live-task-id rule,local_task_protocol.valid_task_id. A named-instance idtask-<inst>~<id>is also accepted, usinggateway_instance.INSTANCE_RE. No new regex is added. The path is then resolved, and anything whose parent is not.task-holds/is refused, which covers symlinks too. Tests cover../, absolute paths,/and\\, NUL, the empty string,None,task-.., a bad instance prefix, and a symlinked marker.Fail-closed, crash-safe marker. The marker is now written in two phases:
reserve()writes apendingrecord to a temp file, fsyncs it, links it into place, removes the temp file, and fsyncs the directory. The link fails if a marker already exists, so the reservation is exclusive.commit()swaps it toheldwith an atomicos.replace(throughresult_publish).Only a whole
heldrecord counts as a hold. An absent, pending, empty, partial, corrupt or chmod-000 marker reads as not held. The task then still reportstask-not-picked-up, so these are the kill controls in both theTaskHoldandKeepalivetests. A write that fails leaves no marker.Room validated by the drain's own router.
holdpasses the notice throughremote_gateway_bridge._proactive_route, the same function the drain uses. It refuses (exit 2, nothing written) any room that does not come back assendto that room:!missing-server,#alias:..., or a Slack/Discord id.Terminal-state gating.
holdrefuses an item whose result exists or whose outbox entry is already settled (outbox.item_status), the same checkreplyuses.replydeletes a hold notice that is still queued before it publishes the answer, so the hold line can no longer follow the answer..sending.<pid>) is not recalled. The outbox has no cancel for an in-flight proactive send. Such a notice was claimed before the answer was written, so it goes out ahead of the answer.Crash and concurrency. A fresh
pendingmarker means anotherholdis still running, so a second caller gets exit 3 (not confirmed), not success.Docs. The package README now documents
holdandtask-held-for-owner, and drops the old line saying a handed-off task always readstask-not-picked-up.Diff coverage. Measured locally with coverage.py over the changed source lines of
task_hold.py,cli.pyandkeepalive.py, runningTaskHold Hold Keepalive Reply ReplyWithoutAnswering HookScript.test_open_item_is_woken_again_after_the_rewake_delay: 169/169 lines covered (100%).Parent control. Procedure: in a fresh worktree at
f51518e1,git checkout 2eaad0e1 -- task_hold.py adapters/hatch/cli.py adapters/hatch/keepalive.py(tests kept at the new head), thenpython3 packages/ag2-sparrow/tests/test_hatch_adapter.py TaskHold Hold Keepalive. Result:Every new review test fails or errors there, apart from three coverage-only tests: a named-instance id is accepted, a second
placereturns False, and an unlistable task dir yields(None, None). Against the original parent7940f25ff965f51df3273d94f009cb1b771119c0, the first round's control is shown above.After, at
f51518e1. Each group was run alone withnice -n 19while the 1-minute load was under 4:Still owed before merge. The live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread) has not been run. CLA and required CI are also still pending.Review fix (2026-10-24) (head
c801e868, fix on71f99846)Addresses the finding from Qingyun's agent: a held task was re-woken with no held flag. The hook builds wake items from
task_fields, which never read.task-holds, andREWAKE_NOTEended "otherwise answer it now", so a fresh worker session could act on an item that waits for the owner's go.task_fieldsreads the marker throughtask_hold.read(the same readerholdand the keepalive health row use) and setsheld: trueon the item. An unheld item is byte-identical to before: the key is absent, neverfalse. Only a whole, well-formed marker counts, as everywhere else.REWAKE_NOTEnow lists "held (held: true)" among the items that get nothing posted and their HANDOFF block repeated, before "otherwise answer it now". A separateHELD_NOTEis appended whenever any item is held, including an item that comes back as a fresh wake after the hook's state is lost: never answer one, do not runholdfor it again, post nothing and repeat only its HANDOFF block.Controls. The two new tests, run against
71f99846'stask_fields.pyandag2-sparrow-hatch.shwith the tests from this head: both ERROR withKeyError: 'held'. At this head they pass.HookScript.test_a_held_item_is_rewoken_as_held_and_never_answered: two tasks woken once; the first is then held; both re-woken after the delay. The held one carriesrewake: true, held: true, its sibling carriesrewake: trueand noheldkey, and the note has both theHELD_NOTEand the held branch ofREWAKE_NOTE. Released, the first comes back unflagged and the held note is gone.TaskFields.test_a_held_task_carries_held_and_an_unheld_one_does_not:list_open_tasksflags only the held task; a truncated marker ({) flags nothing.After, at this head, each run alone with
nice -n 19:The full-file run started at a 1-minute load of 10.6, the two class runs under 10.
scripts/review-checks.shon the diff: PASS.Still owed before merge, unchanged from Round 14: the live post-restart witness, CLA on the new head, a current code-owner approval, and the rebase onto #5830.
Round 14: scope (head
71f99846, oncea13f7f)This round addresses review 5630935569 by shrinking scope. A false hold is surfaced, not reconciled. Reconciliation is tracked in a follow-up issue.
The auto-release is removed. Keepawake never touches a hold. Removed:
release_stuck_holdsand its call inkeepawake;withdraw_undelivered_line;task_hold.release_if/released_atand the.releasedrecord;released_atintask_fields;rearmed);task_hold.py,task_fields.pyandag2-sparrow-hatch.share back to their0e343fd3content, andkeepalive.pyis back toe495ec2bplus the best-effort log below. What stays is read-only: afterHELD_LINE_DELIVER_S(10 min) health reports a hold whose line is not confirmed delivered as abnormal,task-held-line-undelivered(queued, being sent, or parked) ortask-held-line-unconfirmed(gone with no receipt). It surfaces the hold to the owner and acts on nothing. Kewei's two blockers (accepted-but-unconfirmed sends released, and a lossy, non-rolling release-to-wake handoff) were properties of the release and go with it.The parked lookup is anchored.
_line_parkedmatches onlyundelivered/<stem>.txt, the exact namesend_failure_policy.resolve_failed_sendwrites (undelivered / body.name). The drain's only other park,<stem>-<int(time.time())>.txt, is the oversized-body dead letter, which a hold line never is. That name could also be another valid task's exact park (task-aandtask-a-123are both valid ids), so it is not matched at all.Round 13 fixes kept: the
send-failure-delegationconstant assertion (the CI fix), the best-effort keepalive log (_log_line, stderr on failure, so keepawake always reaches the Sparrow check), and the future-dated status check. It now compares the raw delta, so the 5 s bound is exact:+5.000001and+5.5read down,+5reads up. Kewei's boundary nit.The health label is a location, not a delivery verdict.
is_transient()treats aTimeoutErroras transient, soresolve_failed_sendrequeues a timed-out send, which may have been received, under its plain.txtname. After the retry cap it is parked under that same name. An accepted send without an event id takes the same path. On disk such a line looks exactly like one never attempted, sotask-held-line-undeliveredcan name a line that was in fact delivered. The README says so. With no auto-release this affects only the label, never an action. A durable per-line "attempted at least once" record, read byhold_line_outcome, is listed in the follow-up as a prerequisite for any future reconciliation.Controls. The new or changed tests fail against
cea13f7fsources (cli.py,keepalive.py,task_fields.py,ag2-sparrow-hatch.sh,task_hold.pyfromcea13f7f; tests from this head) and pass at this head:Keepalive.test_keepawake_never_touches_a_hold(a hold 11 minutes old, its line still queued, then a keepawake run): FAIL atcea13f7f(the hold was released). Now the marker is unchanged, the queued line is left to the drain, and health readstask-held-line-undelivered.Keepalive.test_a_parked_line_is_matched_by_its_exact_name_only(heldtask-…005andtask-…005-1792760000; only the long id's own park exists): FAIL atcea13f7f('undelivered' != 'unconfirmed'for the short id). Now only the long id readsundelivered.Cli.test_a_status_dated_in_the_future_or_stale_reads_down, the new boundary cases: FAIL atcea13f7ffor+5.000001and+5.5(0 != 10).Removed with the feature: the Round 12 follow-up release test and the Round 13 tests for mid-release claims, unconfirmed holds kept, parked-line release, and the capped hook re-arm. The unwritable-log test stays, without a hold in it.
After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:These also pass under the same rule:
gateway-proactive-foreign-homeserver,proactive-outbox-classification,gateway-proactive-readinessandremote-gateway-singleton.scripts/review-checks.shon the diff: PASS.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA on the new head, a current code-owner approval, and the rebase onto #5830 once it merges.Round 13 (head
cea13f7f, fixes on0e343fd3)This round addresses review 5630362603.
A hold is released only when its line provably never went out.
task_hold.release_ifholds the hold lock and checks that the marker still carries the token keepawake read. It then callscli.withdraw_undelivered_line, and releases only on its answer:withdrawn: the line was still queued as.txt, and this call unlinked it. The unlink is the decision: the drain claims a line by renaming that same name, so if the drain claimed it first, the unlink fails and the hold is kept.parked: the line sits unsent inundelivered/..sending.<pid>claim (checked before and after the unlink), a delivery receipt, orunconfirmed(a post may have landed).A release removes the marker and the
.noticerecord, so a laterholdqueues a fresh line through a valid lane, and records the release time.hold_line_outcomenow also finds a line parked under the failure resolver's own name,undelivered/<stem>.txt. Before, it looked only for<stem>-*.txt, so a parked line read asunconfirmed.A release re-arms the worker's wakes, through the hook's own state. The wake count lives in the hook runtime's
hook_state_get/hook_state_set, which only the hook process can reach. So keepawake records the release time beside the marker (<tid>.released, pruned with it),task_fieldsreports it asreleased_at, and the hook's own state update restarts that task's count once per release. The hook recordsrearmed: <released_at>in itswokenentry, so a capped task is woken again, once.CI.
tests/send-failure-delegation.test.py::test_sparrow_parks_to_the_repo_wide_quarantineasserted the old literal. It now pins the bridge's delegation to_delivery_destination.UNDELIVERED_DIRand that constant's value,"undelivered".The keepalive log is best effort. The release line and the keepawake line go through
_log_line, which writes to stderr when the log cannot be written. keepawake always reaches the Sparrow check.A future-dated gateway status reads down.
check_sparrowalready treats a status overSPARROW_STATUS_MAX_AGE_S(180 s) old as down. It now does the same for one dated more thanSPARROW_STATUS_FUTURE_SKEW_S(5 s, defined beside it) ahead.listener_upand_await_connectedboth go through it, so a stuck or bogustsno longer suppresses a restart. The README'ssparrowcheck text says so.Two-way controls. The new tests fail against
0e343fd3sources (cli.py,keepalive.py,task_fields.py,ag2-sparrow-hatch.sh,task_hold.pyfrom0e343fd3; tests from this head) and pass at this head:Keepalive.test_a_line_the_drain_claims_mid_release_keeps_its_hold(the drain renames the queued line to.sending.4242between keepawake's check and its unlink): FAIL at0e343fd3(['task-…5'] != [], released). Now the hold and the claim are kept, and a pass with the claim already in place keeps it too.Keepalive.test_an_unconfirmed_line_is_never_released(archived, no receipt): FAIL at0e343fd3(released).Keepalive.test_a_line_parked_under_the_resolvers_own_name_is_released(undelivered/proactive-hold-<tid>.txt): FAIL at0e343fd3('unconfirmed' != 'undelivered'). Now it is released withline=parked.HookScript.test_a_capped_item_whose_hold_keepawake_released_is_woken_again, through the real hook script (staten=3, capped, which givesSTATE_SET, SILENT, then a keepawake release, then the hook): FAIL at0e343fd3('WAKE' not found in ['STATE_SET', 'SILENT']). Now it emits WAKE withrewake: true, the count restarts at 1, and the next poll does not wake again.Keepalive.test_an_unwritable_log_never_stops_the_listener_check(the log path is a directory, the listener is down): ERROR at0e343fd3(IsADirectoryErrorbefore the listener check). Nowrestart_listenerruns once, exit 0, and the failure goes to stderr.Cli.test_a_status_dated_in_the_future_or_stale_reads_down(tsof1e11and now+60: down; now−181: down; now+2 and now−10: up): FAIL at0e343fd3for both future cases (0 != 10).send-failure-delegation: the old assertion fails against this branch's bridge, which is the CI failure. The updated one passes.The Round 12 release test now expects
line=withdrawnand checks that the queued line is gone.After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:These also pass under the same rule:
gateway-proactive-foreign-homeserver,proactive-outbox-classification,discord-proactive-foreign-targetandgateway-proactive-readiness.scripts/review-checks.shon the diff: PASS.Limits.
unconfirmedor still claimed is never released. Health keeps reporting it (task-held-line-unconfirmed/-undelivered) until someone looks.unconfirmed.MAX_WAKES) per release.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA on the new head, a current code-owner approval, and the rebase onto #5830 once it merges.Round 12 (head
0e343fd3, fixes on3e60a016; first pushed ase495ec2b)This round addresses review 5629547334 and splits outbox claim parsing out of this PR.
Claim-record parsing is fixed in #5830 (fixes #5829), and #5828 rebases onto it after it merges.
Outbox: fix(outbox): an impossible claim field reads UNKNOWN, never pid reuse #5830 is the single owner of claim parsing. Both copies of
outbox.pyare back tomain, plus exactly fix(outbox): an impossible claim field reads UNKNOWN, never pid reuse #5830's two blocks:PID_MAXafterProcessIdentity, andvalid_pidafter_read_claim_at, with the same text and position. That keeps the rebase trivial. Reverted from this PR: the Round 10_read_claim_atchange (and its_is_int/claimed_at guard), the Round 9valid_pidgate insideprocess_identitywith itsOverflowError/ArgumentErrorcatches at the native probes, and the matchingtests/outbox-windows.test.pyexpectation (nowmain's again). This PR keepsvalid_pidfor its own callers,claim_pid/claim_path, the lane reader and Hatch'sprocess_gone, so none of them depends onprocess_identitygating. The claim-reader test and the drain-side_pid_alive/claim_owner_may_be_aliveassertions in the predicate test moved out with it.One reader for persisted numbers. The new
ag2_sparrow/persisted_number.pyhasbounded_number(value, lo, hi). It accepts atype(x) is intor a finite float, never a bool, range-checks it before any conversion, and returns a float orNone. These sites parse a persisted number and use it:ts(agent_identity);since(task_hold._valid_since, whichtask_hold.read,keepalive.open_tasksand thehold/replyCLI paths all reach);tsincli.check_sparrowandkeepalive._await_connected. Before,float(status["ts"])raisedOverflowErroron10**400, and anint()of a NaN or infinite age raised outside thetry.The other
json.loadsin the five files read no persisted number. The outbox claim reader is fix(outbox): an impossible claim field reads UNKNOWN, never pid reuse #5830's.A far-future
sinceis no hold._valid_sincerefuses asincemore thanSINCE_SKEW_S(5 s) ahead of now, andopen_tasksno longer clamps a hold time to now. Such a marker is invalid, so the task readstask-not-picked-uplike any task nobody holds.Health tells delivered, undelivered and unconfirmed apart, by the drain's own state.
cli.hold_line_outcomereturns:deliveredwhen the drain's receipt says so;undeliveredwhile the line is still queued (.txt), claimed (.sending.<pid>), or parked inRESULT_DIR/undelivered/(named by the newdelivery_destination.UNDELIVERED_DIR, which the bridge now uses too);unconfirmed: gone from the queue with no receipt. The drain writes the best-effort receipt just before it archives a posted line, so that write failing is the common cause.After
HELD_LINE_DELIVER_S(10 min), health reportstask-held-line-undelivered("the asker has heard nothing") ortask-held-line-unconfirmed("delivery can't be confirmed"), both abnormal.A false hold heals itself (
0e343fd3). No new delivery path is added. Thekeepawakepass releases every hold overHELD_LINE_DELIVER_S(10 min) whose line readsundeliveredorunconfirmed, through the existingtask_hold.releasewith the hold's token, under the hold lock. It writes onereleased-hold task=<id> line=<outcome> held_since=<utc>line to the keepalive log per release. The task then readstask-not-picked-up, and the existing stuck handling wakes its worker again, which can re-hold through a lane that is now valid, or answer. A hold whose line was delivered is never released, and nothing is re-sent.README. The hold section now states the published-lane requirement:
lane-identity[.<instance>].json, which the drain refreshes at least every 10 s.holdaccepts it only from the live drain of its instance, under 30 s old and at most 5 s ahead, and exits 3 without it. The health section lists the two new reasons, their order, and the future-sincerule.Two-way controls. The new tests fail against
3e60a016sources (cli.py,keepalive.py,task_hold.py,agent_identity.py,delivery_destination.py,remote_gateway_bridge.py, bothoutbox.py; tests from this head) and pass at this head:Keepalive.test_a_held_line_gone_without_a_receipt_is_unconfirmed_not_undelivered(posted and archived, receipt missing): FAIL at3e60a016(('abnormal', 'task-held-line-undelivered') != ('abnormal', 'task-held-line-unconfirmed')). It also pinsundeliveredfor a parked line and for a live claim, anddeliveredfor a receipt.Keepalive.test_a_far_future_or_unrepresentable_hold_time_is_no_hold: forsinceone hour ahead, FAIL at3e60a016(('healthy', 'task-held-for-owner') != ('abnormal', 'task-not-picked-up')), the clamp hiding it. Forsinceof±10**400, ERROR (OverflowError: int too large to convert to float, from health).Cli.test_an_unrepresentable_status_ts_reads_down_without_crashing(statustsof±10**400,NaN,Infinity,true,"1000"): ERROR at3e60a016for the first four (OverflowError,ValueError: cannot convert float NaN to integer), and FAIL for"1000"(read as running and connected). All read Sparrow down now.Keepalive.test_a_confirmed_send_whose_receipt_write_fails_is_unconfirmed, Kewei's case through the production drain:RESULT_DIR/.outbox-ag2space-proactiveis0500, the HTTP seam confirms an event id,_post_proactiveposts once and archives the line, and the hold is 11 minutes old. FAIL at3e60a016(('abnormal', 'task-held-line-undelivered') != ('abnormal', 'task-held-line-unconfirmed')).Keepalive.test_keepawake_releases_a_hold_whose_line_never_delivered_and_keeps_a_delivered_one(two holds 11 minutes old. One is a stale-lane hold whose line is still queued for a room the drain will not post to; the other's line has a delivery receipt): FAIL withkeepalive.pyfrome495ec2b(the undelivered hold is still held). Now that hold is released with exactly onereleased-hold ... line=undeliveredlog line, the delivered one is kept, health readstask-not-picked-up, and a second pass logs nothing more.DeliveryDestination.test_one_bounded_number_reader_for_persisted_values: unit cases for the helper.tscontrol from Round 11 (±10**400) still passes, now through the helper.After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:These also pass under the same rule. Outbox and recovery:
outbox-claim-machine,outbox-claim-regressions,outbox-adapter-contract,outbox-error-paths,outbox-operator-recovery,outbox-receipt-destination,outbox-recovery-paths,outbox-windows-policy,outbox-windows(skipped off Windows),delivery-primitives-single-owner,design-c-backend,delivery-core-crash-between-terminal-and-release,proactive-recovery-process-probe,proactive-recovery-crash-after-claim,proactive-recovery-same-inode-race,proactive-fence-terminal-claim-retired-on-restartandhealth-check-task-claim-age. Bridge:gateway-proactive-foreign-homeserver,remote-gateway-singleton,gateway-auto-reenroll,gateway-longpoll-timeout,gateway-terminal-result-moved-once,gateway-owner-mention,discord-proactive-foreign-target, and the 21 bridge suites listed in Round 7.scripts/review-checks.shon the diff: PASS.Limits.
process_identityno longer pre-checks the pid domain on this branch, as onmain. Every caller in this PR validates first. A caller elsewhere that passes an unvalidated pid on Darwin can still alias throughctypes.c_int, as onmain.keepawakerun (every 15 minutes), so within at most about 25 minutes. The worker is then woken again. Nothing re-routes or re-sends the original line. If it is still queued, a laterholdadopts it instead of queuing another. If it was parked or its delivery is unconfirmed, a laterholdexits 3, and the task staystask-not-picked-upuntil it is answered.sinceread as future, so the task readstask-not-picked-up(fail closed) until the clock catches up. This is the stable contract for a futuresince, and the one-hour-ahead control covers it.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA on the new head, a current code-owner approval, and the rebase onto #5830 once it merges.Round 11 (head
3e60a016, fixes on6c3a6bf4)This round addresses review 5629088874. Triage first: two findings are in scope and fixed here. The third (outbox claim numerics) also reproduces on
origin/main, so it predates this PR and is left for its own issue and PR.An unrepresentable lane
tsis a missing lane, not a crash. Beforeisfinite,_valid_lanenow accepts only a non-boolintwithin±2**53(converted tofloat) or afloat. Anything else, including±10**400, reads as missing, soholdexits 3. The bound is defined once, next to the other lane constants. This was our regression from Round 10.A hold whose room line never arrives no longer reads as healthy. Keepalive health now reports a held task as stuck when its hold has stood for over
HELD_LINE_DELIVER_S(10 minutes, defined once next to the other health reasons inkeepalive.py) and the drain's receipt does not show its line delivered:condition: abnormal,reason: task-held-line-undelivered,since= hold time + 10 min, with a hint naming the proactive drain.cli.hold_line_deliveredreads the same receipt_line_stateuses. This is the durable consequence Kewei traced from the lease window: a hold taken while the drain could not refresh, whose line is never sent. The hold still stands, but health no longer reports it as deliberately parked. A hold whose line was delivered, or one under 10 minutes old, still readstask-held-for-owner.Outbox claim numerics: pre-existing, not changed here. I ran the same probe through the production
read_delivery_claimandmay_reclaim_deliveryagainstsrc/outbox.pyatorigin/main7940f25f, the PR's merge base (its package copy is identical), and at6c3a6bf4. The output is identical:The last two reach
_pid_was_reused, which treats an impossible birth token as proof of a successor. That can duplicate a delivery, so it should be fixed first. Proposed fix, for the separate PR:start_usecmust be a bounded non-negative int,claimed_ata bounded finite number,pida bounded int, and a numeric parse failure must be caught. Anything nonconforming reads UNKNOWN and is never reclaimable, in bothsrc/outbox.pyand the vendored copy, with recovery-level controls.Two-way controls. The new tests fail against
6c3a6bf4sources (agent_identity.py,keepalive.py,cli.pyfrom6c3a6bf4; tests from this head) and pass at this head:Hold.test_an_unrepresentable_lane_ts_fails_closed_without_crashing(tsof10**400and-10**400): ERROR at6c3a6bf4(OverflowError: int too large to convert to float, raised out ofhold). Exit 3 now, with no notice and no hold.Keepalive.test_a_hold_whose_room_line_never_delivers_reports_stuck(a hold 11 minutes old, no delivery receipt): FAIL at6c3a6bf4(('healthy', 'task-held-for-owner') != ('abnormal', 'task-held-line-undelivered')). Now abnormal, and the row validates throughhealth_command. Controls in the same test: with the receipt delivered it is healthy and held, and a hold one minute old without a receipt is healthy and held.Keepalive.test_a_deliberately_held_task_reports_held_not_stucknow records the line's delivery receipt before expecting a healthy hold, since a 20-minute hold without one is the stuck case.open_tasksreturns a third value, the earliest undelivered hold, and the unlistable-dir test expects(None, None, None).After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:These also pass under the same rule. Outbox and recovery:
sparrow-outbox-claim-protocol(22/22),outbox-claim-machineandproactive-recovery-process-probe. Bridge:gateway-proactive-foreign-homeserver,remote-gateway-singleton,gateway-auto-reenroll,gateway-longpoll-timeout,gateway-terminal-result-moved-once,gateway-owner-mention, and the 21 bridge suites listed in Round 7.scripts/review-checks.shon the diff: PASS.Limits.
task-held-line-undelivered10 minutes later instead of being revoked.main.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA on the new head (absent at6c3a6bf4; it was green at9fbb2934), and a current code-owner approval.Round 10 (head
6c3a6bf4, fixes on9fbb2934)This round addresses review 5628643842.
LANE_REFRESH_S(10 s) old. It checks on every proactive pass (about once a second) and at each deliverability check, under the lane lock.published_lane_recordtreats a record as missing when itstsis not finite, is older thanLANE_FRESH_S(30 s), or is dated more thanLANE_SKEW_S(5 s) ahead. All three bounds are defined once, beside the reader inagent_identity.py; there is no env var or manifest knob. A drain that cannot write cannot refresh, so its record goes stale andholdexits 3. That covers the read-only case Kewei reproduced with chmod (every invalidation fallback fails), and a reused pid, whose record nobody refreshes. A failing publish is logged once per failure episode, not on every pass.None._auth_dir(create=True)re-runs mkdir, lstat and open up to 3 times whenauth/vanishes after the mkdir, then raisesOSError. Readers getNoneand return missing. Every close goes through a guard that closes only an int fd. Lane files can no longer land in the process CWD, and noTypeErrorescapes the bridge'sexcept OSError.outbox._read_claim_atno longer callsint()orfloat()on parsed JSON. Apidorstart_usecthat is not a plain int (true,1.9,"1",Infinity), or aclaimed_atthat is not a finite number, makes the claim UNKNOWN. That is the existing wrong-types contract: un-stealable, and it never aborts a scan. A well-typed int outsidevalid_pid(such as2**31) stays as written, andprocess_identityreads it as DEAD, so the claim has no live owner.claim_pathnow raisesValueErrorfor a pid outsidevalid_pid, so it never builds a nameclaim_pidrejects.outbox.py,proactive_recovery.py,holder_lock.py,task_hold.pyand Hatch'scli.pyfinds no other unvalidatedint(...)of a parsed pid.proactive_recovery's private-claim regex feedsprocess_identity, which is already gated.holder_lockparses only positive, non-bool pids, and its_pid_alivealready catchesOverflowError. Its own pinned contract (holder-lock.test.py) treats an impossible pid as unobservable (None) rather than dead. I tried gating it as dead, that test failed, and I reverted to keep its contract.os.killraises rather than aliasing, so it has no native-alias risk.src/outbox.pycarries the same change, and the drift check passes.tests/outbox-windows.test.pynow expects DEAD, not UNKNOWN, for out-of-domain inputs (0,-1,2**31,2**32,1 << 80,"123",None,True), still decided before any Windows API binding. That suite skips off Windows.Two-way controls. The new tests fail against
9fbb2934sources (agent_identity.py,delivery_destination.py,remote_gateway_bridge.py,outbox.pyandsrc/outbox.pyfrom9fbb2934; tests from this head) and pass at this head:DeliveryDestination.test_a_drain_that_cannot_write_lets_its_lane_go_stale, Kewei's chmod repro with real filesystem operations: this process's record@old:ag2.space, record0400,auth/0500, live identity@new:other.space. The drain refuses the room and cannot change the record. Hatch, reading 31 s later: FAIL at9fbb2934('ag2.space' != ''), no lane now.DeliveryDestination.test_a_lane_record_past_the_freshness_bound_is_no_lane(29 s old accepted; 31 s old, 60 s ahead,inf,-infandnanrejected): FAIL for every rejected case.DeliveryDestination.test_the_drain_refreshes_its_lane_as_a_heartbeat(no rewrite while fresh; rewritten once 11 s old): FAIL.DeliveryDestination.test_auth_removed_after_its_mkdir_never_reaches_the_cwd(auth/removed at the first lstat, once and on every try; the CWD is a temp dir): ERROR at9fbb2934(TypeError: 'NoneType' object cannot be interpreted as an integer, with lane files written into the CWD). Now the publish succeeds insideauth/, or raisesOSErrorwhenauth/keeps vanishing, and the CWD stays empty. It also checks thatclaim_pathrejects2**31andTrue.DeliveryDestination.test_a_persisted_claim_pid_is_never_coerced(pidoftrue,1.9,"1",Infinity;start_usecandclaimed_atofInfinity;pid2**31): FAIL at9fbb2934(HELDfortrue,1.9,"1"and an infiniteclaimed_at), ERROR for the infinitepidandstart_usec(OverflowError). Now all are UNKNOWN, and the2**31claim has a DEAD owner and is reclaimable.Lane fixtures in older tests that used
ts0 or 1 now use the current time, so each test still checks its own reason (dead pid, foreign instance, lock mismatch, out-of-domain pid) rather than staleness.After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:These also pass under the same rule. Outbox and pid probes:
delivery-core-crash-between-terminal-and-release,delivery-primitives-single-owner,design-c-backend,health-check-task-claim-age,outbox-adapter-contract,outbox-claim-machine,outbox-claim-regressions,outbox-error-paths,outbox-operator-recovery,outbox-receipt-destination,outbox-recovery-paths,outbox-windows-policy,outbox-windows(skipped off Windows),pending-questions-single-store,proactive-fence-terminal-claim-retired-on-restart,proactive-recovery-crash-after-claim,proactive-recovery-process-probe,proactive-recovery-same-inode-race,discord-proactive-foreign-target,services-status,sparrow-outbox-claim-protocol,voice-lock-windows-policyandstate-paths-adoption. Bridge and recovery:gateway-proactive-foreign-homeserver,remote-gateway-singleton,gateway-auto-reenroll,gateway-longpoll-timeout,gateway-terminal-result-moved-once,gateway-owner-mention, and the 21 bridge suites listed in Round 7.scripts/review-checks.shon the diff: PASS.Limits.
holdcan still pass a room the drain now refuses. Past it,holdexits 3.tswith the reader's wall clock on the same host. A wall-clock step larger than the skew bound (5 s ahead) or the freshness bound (30 s back) makesholdfail closed until the next refresh.os.killsites (task_lifecycle,task_io,workspace_lock, Hatchkeepalive) still do not go throughvalid_pid, and none reads a pid from a lane record, an outbox claim or a holder record.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), and a current code-owner approval. CLA is green at9fbb2934.Round 9 (head
9fbb2934, fixes on40ffc581)This round addresses review 5627929500.
One pid domain, used everywhere.
outbox.valid_pid(x)returnsxonly for an int (never a bool) with0 < x <= PID_MAX(2**31 - 1, the largest value a Cintholds); otherwise it returnsNone. It lives inoutbox.pybecause the native probes live there, and it is updated identically insrc/outbox.py, the vendored source. Every pid entry point goes through it:claim_pid(canonical spelling and in range);published_lane_record(out of range reads as missing) andpublish_lane(refuses to write one);outbox.process_identity, so the drain's_pid_aliveandproactive_recovery.claim_owner_may_be_aliveboth report DEAD, meaning no live owner;process_gone(out of range means gone).As a further guard,
OverflowErrorandctypes.ArgumentErrorare caught at the Darwinproc_pidinfoand WindowsOpenProcesscalls and at Hatch'sos.kill. Before this,real_pid + 2**32aliased the live process throughctypes.c_inton Darwin, and a lane record with that pid crashedholdwith an uncaughtOverflowErrorfromos.kill. Nowholdexits 3, recovery does not defer such a name, and every predicate agrees.The lane record has a serialized writer contract. Publish, retract and every read of
lane-identity{suffix}.jsonhold an exclusivefcntl.flockon a per-instancelane-identity{suffix}.lockbeside it, openedO_NOFOLLOWthrough anO_DIRECTORY | O_NOFOLLOWfd onauth/. This is the.task-holds/.lockpattern.O_EXCLtemp, write,fsync, thenos.replace. If that fails, it invalidates the old record in the same locked step, then raises.{"instance": …, "retracted": true}. If that fails too, truncate the record in place, which needs no free space. A tombstone, an empty file and a malformed record all read as missing.pid(andtswhen given) and instance, then invalidate, all inside the lock.pidandts).O_CREATopens of one name can returnENOENT. A concurrent-publish stress run hit this, so the open retries up to 5 times.fcntlthe drain skips the record. Hatch, its only reader, is POSIX-only.Two-way controls. The new tests fail against
40ffc581sources (agent_identity.py,delivery_destination.py,remote_gateway_bridge.pyfrom40ffc581; tests from this head) and pass at this head:Hold.test_an_out_of_domain_lane_pid_fails_closed_without_crashing(a lane record with pidreal_pid + 2**32or2**31): ERROR at40ffc581, whereholdcrashed withOverflowError: Python int too large to convert to C int. It exits 3 now.DeliveryDestination.test_every_pid_predicate_rejects_the_same_out_of_domain_values(2**31,real_pid + 2**32,-1,True; Hatchprocess_gone, the drain's_pid_alive,claim_owner_may_be_alive,claim_pid,published_lane_recordandvalid_pidall agree): ERROR (OverflowError) for the two large values, and FAIL for-1andTrue(Hatch read them as live).Hold.test_a_pid_past_the_native_range_is_no_claim(real_pid + 2**32and2**31; for each, an exact-body claim of the notice,_line_stateandhold): FAIL (4294969521 is not None,2147483648 is not None).DeliveryDestination.test_a_failed_publish_and_a_failed_unlink_read_as_missing(the review'spredicates_disagreerepro: this process's old lane onag2.space, live identity onother.space, the production writer's replace fails and the record unlink raisesPermissionError): FAIL ('ag2.space' != '').DeliveryDestination.test_a_failed_publish_whose_tombstone_also_fails_empties_the_record(every replace fails, unlink refused): FAIL (the old record is still read).DeliveryDestination.test_a_successor_published_during_a_stale_retract_survives(a retract paused after its ownership read, a successor published, then the retract resumes): FAIL (None != '@new:other.space').DeliveryDestination.test_a_successor_published_during_a_failed_publish_survives(the drain's publish paused at its replace, a successor published, then the publish fails): FAIL (None != '@new:other.space').DeliveryDestination.test_a_failed_publish_or_a_clean_exit_retracts_the_record(now also: a retract with the right pid but anothertskeeps the record): ERROR (publish_lanereturned nothing at40ffc581).DeliveryDestination.test_the_drain_publishes_on_a_pass_with_no_proactive_file_and_at_start(clean exit registers the owned retract): FAIL.In both interleaving tests the successor gets 0.5 s to finish. Under the lock it cannot, so it completes after the paused call, and its record survives either way the timing falls.
After, at this head. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:tests/gateway-longpoll-timeout.test.pyhas anos.replacefake that took only(src, dst). The drain's start now reaches the lane writer, which passessrc_dir_fd/dst_dir_fd, so the fake now forwards them. TheHoldandDeliveryDestinationgroups were also run three times in a row (58 OK each, before the pid-domain tests were added), and the concurrent-publish stress probe ran clean 3×30 rounds after the retry. These also pass under the same rule. Bridge and recovery:gateway-longpoll-timeout,gateway-terminal-result-moved-once,gateway-disposal-delegation, the 23 bridge suites listed in Round 7, andgateway-owner-mention. Pid probes and the outbox, sinceoutbox.pychanged:ag2-sparrow-drift(the vendored copy matchessrc/),delivery-core-crash-between-terminal-and-release,delivery-primitives-single-owner,design-c-backend,health-check-task-claim-age,holder-lock,outbox-adapter-contract,outbox-claim-machine,outbox-claim-regressions,outbox-error-paths,outbox-operator-recovery,outbox-receipt-destination,outbox-recovery-paths,outbox-windows-policy,outbox-windows(skipped off Windows),pending-questions-single-store,proactive-fence-terminal-claim-retired-on-restart,proactive-recovery-crash-after-claim,proactive-recovery-process-probe,proactive-recovery-same-inode-race,discord-proactive-foreign-target,services-status,sparrow-outbox-claim-protocolandvoice-lock-windows-policy.scripts/review-checks.shon the diff: PASS.Limits.
publish_laneraises, the drain logs it, and the old record stays. A read-onlyauth/also blocks every new publish, so the record cannot be updated either.os.killsites (task_lifecycle,task_io,workspace_lock,holder_lock, Hatchkeepalive) do not go throughvalid_pid. None of them reads a pid from a lane record or a claim name.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA, and a current code-owner approval.Round 8 (head
40ffc581, fixes oncd4762f5)This round addresses review 5627582134 and makes
holdfail closed without a lane record.A claim counts only if it is this notice's own.
delivery_destination.claim_base(name)sits besideclaim_pathandclaim_pid. It is the inverse ofclaim_path: rsplit on.sending., with a canonical pid required.<stem>.sending.x.sending.<pid>is a claim of<stem>.sending.x. Every caller now uses it:claim_base(name) == notice.stem.claim_base(name) + ".txt". The old first-occurrencesplit(".sending")sentproactive-a.sending.x.sending.<pid>toproactive-a.txt._owner_mention_dm_queued) used a bare{stem}.sending*glob. It now checks{stem}.sending(legacy) or aclaim_basematch.proactive_recovery.pyhandles only the pid-less.sendingclaims of the other bridges and is unchanged.One lane record per gateway instance. The record is
<state>/auth/lane-identity{suffix}.json, usinggateway_instance.suffix, the same suffix the bridge uses forgateway-status{suffix}.jsonand itsgateway-bridge{suffix}lock. It carriesinstance. Hatch reads the record for the instanceholdruns as (_wake_item's instance) and rejects one that names another instance. The drain rewrites the record whenever the file on disk is not its own (pid or identity differ), so an overwrite heals at the next pass rather than being masked by an in-memory cache.Only a live drain's record counts, and none means no hold.
holdtreats the record as missing, exits 3 and records no hold and no line, when:process_gone);live_lock_holder);agent_idis empty.Enrollment is no longer consulted for the room check. The drain publishes at start (in
main, after the singleton lock and before the outbound worker) and at the top of every_post_proactivepass, which runs everyREMOTE_OUTBOUND_SCAN_Swhether or not a proactive file exists. It retracts the record on a failed publish and at clean exit (atexit, only if the record is still its own). The writer usesmkstempandos.replace, so concurrent writers never collide on a temp name.Two-way controls. The new tests fail against
cd4762f5sources (cli.py,agent_identity.py,delivery_destination.py,remote_gateway_bridge.pyfromcd4762f5; tests from this head) and pass at5336328b:Hold.test_no_published_lane_records_no_hold_even_with_enrollment(missing,{, non-string, emptyagent_id; enrollment andAGENT_MXIDboth name the room's server): FAIL (0 != 3).Hold.test_an_adjacent_infix_claim_is_not_this_notices_claim: FAIL (queued != unknown) for both forms,<notice-stem>.sending.x.sending.<live pid>and<notice-stem>.sending.5.sending.<live pid>, each with the exact body. It also checks that a matched name equalsclaim_path(base, pid)for its parsed pid.Hold.test_recovery_returns_an_adjacent_infix_claim_to_its_own_name: FAIL for both forms (.sending.x.and.sending.5.). The dead claim is renamed to the shorterproactive-a<infix>.txt.Hold.test_a_dead_drains_stale_lane_records_no_hold(the review's repro:ts=1, an exited pid, an identity on the room's server): FAIL (0 != 3).Hold.test_a_failed_lane_publish_records_no_hold_on_stale_enrollment(the review'sPUBLISH_FAILURE_FALLBACKrepro: the production writer raises, enrollment onag2.space, liveAGENT_MXIDonother.space): FAIL (0 != 3).DeliveryDestination.test_two_instances_sharing_state_keep_their_own_lanes(production writer,bluethengreenthenbluesharingSTATE_DIR): FAIL (one sharedlane-identity.json).DeliveryDestination.test_a_stale_dead_or_foreign_lane_record_is_no_lane,test_hold_reads_only_the_published_lane,test_concurrent_publishes_leave_one_whole_record(8 writer threads, a reader never sees a partial record),test_the_drain_publishes_on_a_pass_with_no_proactive_file_and_at_start,test_the_published_lane_record_never_holds_a_token(keys exactlyagent_id,instance,pid,ts) andtest_a_failed_publish_or_a_clean_exit_retracts_the_record: ERROR (old signatures, or no record).tests/gateway-proactive-foreign-homeserver.test.pycase f): FAIL.After, at
40ffc581(sources unchanged since5336328b; the Hatch suite, the credential guard and the foreign-homeserver test were rerun at40ffc581). Each suite was run alone withnice -n 19, each started at a 1-minute load under 4:These also pass under the same rule:
discord-proactive-matrix-target-not-claimed,gateway-disposal-delegation,gateway-channel-skip-owner,gateway-proactive-explicit-room-without-default,gateway-outbound-single-entrypoint,gateway-proactive-thread,gateway-proactive-body-target-claim,gateway-durable-ack,gateway-results-watcher,gateway-task-result-thread-ask,gateway-longpoll-timeout,gateway-proactive-readiness,gateway-voice-room-gate,proactive-outbox-classification,remote-gateway-outbound-worker,withheld-review-decision-final,gateway-outage-recovery,gateway-dedup-recovery,gateway-orphan-result-reconciler,gateway-claim-gate-name-collision,gateway-destined-proactive-not-claimed,remote-gateway-singletonandgateway-terminal-result-moved-once.scripts/review-checks.shon the diff: PASS.Limits.
holdfails closed (exit 3) from a drain's start until it publishes, and whenever no live drain of that instance runs. This is by design, and a retry after the drain is up succeeds.holdin that window judges by the previous identity.atexit) leaves the record behind. The exited-pid and lock checks reject it, subject to the previous point.STATE_DIR, and the lock is host-gated as before.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA, and a current code-owner approval.Round 7 (head
cd4762f5, fixes on3cdb73b3)This round addresses review 5627213024.
delivery_destination.claim_pidnow accepts exactly the spellingclaim_pathwrites: non-empty ASCII digits withstr(int(s)) == sand a positive pid. Leading zeros, zero, signs and whitespace are rejected. A.sending.0<pid>file is no longer proof of a queued line forhold, and orphan recovery no longer defers it as a live claim._lane_identity, called by_own_homeserverand at the start of each_post_proactivepass) to<state>/auth/lane-identity.json. It is written whenever the identity changes, atomically (temp +os.replace). One writer,agent_identity.publish_lane, records onlyagent_id,tsandpid. If a write fails, the old record is removed, so readers fall back to enrollment instead of trusting a stale lane.holdreads the published lane first, then enrollment. If neither is readable it fails closed as before. Hatch still never reads the credential env:tests/gateway-credential-sources.test.pypasses (124 OK).Controls at
3cdb73b3sources (tests from this head;cli.py,agent_identity.py,delivery_destination.pyandremote_gateway_bridge.pyfrom3cdb73b3):Hold.test_a_leading_zero_claim_is_neither_queued_nor_live: FAIL (queued != unknown). It also checks thatholdrecords no hold, that recovery never asks whether that pid is alive, and that0,0<pid>,+<pid>,<pid>,<pid>and empty are rejected.Hold.test_a_drain_identity_that_differs_from_enrollment_decides_the_room, the exact repro (enrollment@watch-agent:ag2.space, liveAGENT_MXID=@watch-agent:other.space, room!room:ag2.space; the real drain predicate refuses the room): FAIL (0 != 2, held).Hold.test_a_drain_identity_matching_the_room_still_holds(stale enrollment onother.space, live lane onag2.space): FAIL (2 != 0).DeliveryDestination.test_the_published_lane_record_never_holds_a_token: ERROR (no record). WithTOKENandREMOTE_TASK_TOKENset to a known secret, the record's raw text omits it, its keys are exactlyagent_id,ts,pid, and it is the only file the publish writes underauth/.DeliveryDestination.test_a_failed_lane_publish_removes_the_stale_record: ERROR (nopublish_lane).tests/gateway-proactive-foreign-homeserver.test.pycase f) (each drain pass publishes the identity it judged rooms by): FAIL.All pass now.
After. Each suite was run alone with
nice -n 19, each started at a 1-minute load under 4:The suites that reach
_post_proactive,_own_homeserver,_room_is_deliverable_hereor_recover_orphan_proactiveall pass, each run alone under the same rule:discord-proactive-matrix-target-not-claimed,gateway-disposal-delegation,gateway-channel-skip-owner,gateway-proactive-explicit-room-without-default,gateway-outbound-single-entrypoint,gateway-proactive-thread,gateway-proactive-body-target-claim,gateway-durable-ack,gateway-results-watcher,gateway-task-result-thread-ask,gateway-longpoll-timeout,gateway-proactive-readiness,gateway-voice-room-gate,proactive-outbox-classification,remote-gateway-outbound-worker,withheld-review-decision-final,gateway-outage-recovery,gateway-dedup-recovery,gateway-orphan-result-reconciler,gateway-claim-gate-name-collisionandgateway-destined-proactive-not-claimed.scripts/review-checks.shon the diff: PASS.Limits at
cd4762f5, as updated by Round 8 (40ffc581).Before the drain's first pass,Closed:holdused enrollment.holdnever uses enrollment for the room check. With no lane record it exits 3 ("can't confirm the room is deliverable yet"), and the drain publishes at start, so the window is only startup..envedit) is published at the next pass, at mostREMOTE_OUTBOUND_SCAN_S(1 s by default), or at the next deliverability check. Aholdin that window reads the previous lane.The record is not tied to a live pid.Closed: a record whose pid has exited, or that a live lock for its instance does not name, counts as missing.An emptyClosed: it reads as no lane, andagent_idfalls back to enrollment.holdexits 3.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA, a green CI run, and a current code-owner approval.Round 6 (head
3cdb73b3, fixes onb3ce9d67)This round addresses review 5626960603.
_line_statenow reads candidates in the order the drain moves a line: the queued.txt, then.sending.<pid>claims, then the delivered receipt. When a file vanishes under a read and nothing later explains it, it rescans, up to 3 times, so a claim or a hand-back during the check no longer reads as unconfirmed. Claim naming now has one owner,delivery_destination.claim_path/claim_pid. The drain's claim and its orphan recovery use it, andholdaccepts only.sending.<digits>claims, the ones the drain creates and recovers. A.sending.xfile is never proof.task_hold._create_at, once the final link succeeds, the lock-fd close, the dir-fd fsync and close, and the temp unlink are all best-effort infinally. The temp name is drawn before the dir fd is opened, so nothing between the open and thetrycan leak it.holdexits 3 ("can't confirm the room is deliverable") and records no hold and no line. The test that pinned "missing enrollment skips the check" now pins this, for a missing file, malformed JSON and a non-stringagent_id, with anAGENT_MXIDoverride in the environment. An empty enrolled identity also exits 3.Controls at
b3ce9d67sources (tests from this head;cli.py,task_hold.py,delivery_destination.pyandremote_gateway_bridge.pyfromb3ce9d67):Hold.test_a_drain_claim_between_listing_and_reading_still_confirms: FAIL (3 != 0, "not proven queued"). A monkeypatched read renames the line to.sending.4242between listing and reading.Hold.test_a_claim_handed_back_mid_read_is_found_on_the_rescan: FAIL (unknown != queued).Hold.test_only_a_claim_the_drain_makes_counts_as_queued: FAIL (queued != unknownfor an exact-body.sending.x).TaskHold.test_close_errors_after_the_link_never_undo_a_published_marker: ERROR (lock closeOSErrorescapesplace).Hold.test_missing_or_unreadable_enrollment_records_no_hold(3 subtests): FAIL (0 != 3).Hold.test_an_unknown_identity_records_no_hold: FAIL (0 != 3).DeliveryDestination.test_the_drain_claims_and_recovers_by_this_grammar: ERROR (no shared helper at the old head). It also checks that recovery skips.sending.xand recovers a dead pid's claim.All pass now.
After. Each group was run alone with
nice -n 19, each started at a 1-minute load under 4:scripts/review-checks.shon the diff: PASS.Drain claim and recovery suites at
3cdb73b3, each run alone withnice -n 19and started at a 1-minute load under 4, all pass:tests/gateway-ack-first,gateway-archive-the-sent-generation,gateway-auth-wait-not-transport-backoff,gateway-auto-reenroll,gateway-dedup-recovery,gateway-destined-proactive-not-claimed,gateway-disposal-delegation,gateway-durable-ack,gateway-job-liveness-predicate(.sh),gateway-longpoll-timeout,gateway-orphan-result-reconciler,gateway-outage-recovery,gateway-owner-mention,gateway-proactive-explicit-room-without-default,gateway-proactive-readiness,gateway-relay-result-no-ceiling,gateway-results-watcher,gateway-terminal-result-moved-once,gateway-voice-outcome-contract,gateway-voice-room-gate,remote-gateway-file-attach,remote-gateway-singletonandgateway-claim-gate-name-collision. That is everytests/*gateway*/*remote-gateway-bridge*file mentioning.sendingorrecover, plus the claim-gate collision suite.Limit kept: stale enrollment. If Sparrow runs with an
AGENT_MXID/AGENT_IDoverride that differs from a present enrollment, the pre-check can still pass a room the drain then refuses. The drain's live identity exists only in its credential environment, which Hatch must not read (tests/gateway-credential-sources.test.pyenforces this). A two-line comment on_lane_homeserversays so. Closing this fully needs the provider edge to publish a durable, non-secret lane identity. That is a separate change.Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA, and a current code-owner approval.Round 5 (head
b3ce9d67, fixes on03f60756)This round addresses review 5625696483.
_line_statenow takes the exact line this hold queues. Aproactive-hold-<tid>.txt(or its.sending*claim) counts as queued only when the drain's own readiness predicate (result_ready.read_ready_result) reads it and its body is exactly that line. An empty, blank or foreign file readsunknown, soholdexits 3 and records no hold.task_hold._create_at, once the marker's link succeeds the result is success. Temp cleanup is best-effort (a leftover dot-temp is never read as a record), and the directory fd closes on every path._lane_homeserverstates the limit. It stays in the limits list below.Controls at
03f60756sources (tests from this head,task_hold.pyandcli.pyfrom03f60756), each run alone:Hold.test_a_pre_existing_empty_line_file_is_not_queued: FAIL (0 != 3, held).Hold.test_an_empty_or_foreign_queued_file_is_no_proof(empty, blank, garbage): FAIL (0 != 3).Hold.test_a_temp_cleanup_error_after_the_link_still_holds: FAIL (3 != 0, "the hold was not recorded (cleanup)").TaskHold.test_a_temp_cleanup_error_after_the_link_is_success_and_closes_the_dir: ERROR.Hold.test_missing_enrollment_skips_the_homeserver_checkpasses at both heads. It pins existing behavior, the stated limit.All pass now.
After, at
b3ce9d67. Each group was run alone withnice -n 19:The last count rerun of Keepalive, Reply and DeliveryDestination started at a 1-minute load of 5.0. The first runs of those groups were under 4 and also passed.
Still owed before merge: the live post-restart witness (Hatch
hold, through the proactive drain, into the room or thread), CLA, and a current code-owner approval.Round 4c (head
03f60756, fix on72fa6ef6)Follow-up from Qingyun's agent's review. Both hold releases that run after an answer go through
_release_quietly:replyafter it publishes;holdwhen the answer lands mid-hold.An
OSErrororValueErroris logged as one stderr line, and the command keeps its own exit status. A hold left behind is harmless, because health ignores holds on answered tasks.Three new tests: a delivered reply, a
--no-sendclose, and an answer that lands mid-hold, each withreleaseraisingOSError. All three error at72fa6ef6'scli.py(Ran 3 ... FAILED (errors=3)). Now the delivered reply exits 0 and reportsposted:, with the body delivered as written. Each group was run alone withnice -n 19while the 1-minute load was under 4:Reply14 OK,Hold28 OK.Round 4b (head
72fa6ef6, fix one6039158)On Linux, CI failed
test_an_old_token_never_erases_a_successor_placed_mid_release. Linux reuses a freed inode number at once, so the inode comparison could not tell the old marker from its successor. The comparison is now replaced by a lock.placeandmark_notice) and every owned unlink (release,clear_notice,prune) runs under an exclusivefcntl.flockon.task-holds/.lock. The lock file is opened through the held directory fd withO_NOFOLLOW, and a symlinked.lockis refused.releaseis a plain read, compare token, unlink. The inode check and its window note are removed.test_a_successor_between_the_check_and_the_unlink_survives, forces the successor between the token check and the unlink.e6039158'stask_hold.py, the new test fails on macOS:None != '<new token>', meaning the old release removed the successor.e6039158under Linux inode reuse (the CI failure) but passes there on macOS.nice -n 19while the 1-minute load was under 4:TaskHold24 OK,Hold25 OK,Keepalive27 OK. Coverage.py reportstask_hold.pyat 100%.Review round 4 (head
e6039158, fixes onf7d4df8d)This round addresses review 5625269814 and simplifies the design again.
agent_identity.lane_identityis removed, andagent_identity.pyis back tomain.holdnever reads Sparrow's (or any other process's) environment. It takes the homeserver from the identity Sparrow enrolled:agent_identity.enrolled(STATE_DIR), the non-credentialauth/ag2space.jsonrecord that the drain's own resolver ends on. When that identity is unknown, the pre-check passes and the drain's predicate decides at send time.tests/gateway-credential-sources.test.pyfails atf7d4df8dand passes now (124 OK)..noticerecord carries its writer's token. Only that caller queues the line, and only it can clear the record. Every caller, owner or loser, records a hold only after_line_stateproves the line is queued, being sent or delivered. Otherwise it exits 3 and records no hold. This also covers a_publish_oncethat wrote nothing.release(token)opens the marker by fd, compares the token, and unlinks it, so a crash or a mismatch never leaves a marker hidden. (Round 4b replaced the inode check this round used with a per-dir lock.)Two-way controls.
At
f7d4df8dsources (task_hold.py,adapters/hatch/cli.pyandagent_identity.pyrestored fromf7d4df8d),test_hatch_adapter.py TaskHold Hold DeliveryDestinationgivesRan 51 tests ... FAILED (failures=5, errors=4):Two more controls were run alone:
Hold.test_the_enrolled_identity_wins_over_snapshot_and_environmentfails at old (0 != 2, held) and passes now.gateway-credential-sourcesfails at old withFAIL: test_no_other_package_module_touches_a_credential_sourceand passes now.After, at
e6039158. Each run alone withnice -n 19while the 1-minute load was under 4:The bridge is unchanged this round, so the other bridge suites were not rerun.
Diff coverage. Measured locally with coverage.py at this head, over the changed lines of the touched modules: 343/343.
Known limits (as of round 5; round 6 changes the identity item, see Round 6)
.sending.<pid>) can't be recalled when the answer arrives. It was claimed before the answer existed.holdexits 3 and records no hold, so the task readstask-not-picked-up. This happens whenholddies between the.noticerecord and the queue write, or when the drain delivers but its best-effort receipt write fails. The result is fail-closed and visible, and answering the task clears it.AGENT_MXID/AGENT_IDoverride that differs from its enrollment, or has no enrollment, the pre-check can pass a room the drain then refuses at send time. The line then strands visibly in the drain while the hold is recorded. That is the trade-off of reading no credential-bearing environment.holdexit 3 with no hold. Answering the task clears it.holddoes nothing.hold, through the drain, into the room or thread) and a current code-owner approval (the main ruleset requires one, including a code owner) are still owed before merge.Review round 3 (head
f7d4df8d, fixes ona77d49a7)This round addresses review 5625034377.
Loader name collision (the CI regression).
src/remote-gateway-bridge.pyimports the routing functionproactive_destination, then execs the package bridge into the same globals. The package bridge'sfrom . import proactive_destinationreplaced that function with the module, so_ag2space_proactive_claim_gateraisedTypeError.delivery_destination.py, and the bridge imports it as_delivery_destination.tests/gateway-claim-gate-name-collision.test.pydrives the loader's real gate for a verified-room voice result, a Discord-owned bare file and a.to-discordfile.Atomic release.
release(token)renames the marker to a private tombstone under the directory fd, checks the token in the tombstone, and unlinks it. A mismatched marker is linked back. If a newer hold took the name in the meantime, that newer hold stands.Confined writes. Every create, link and unlink goes through the held directory fd. An absent or vanished
.task-holdsraises and never falls back to the CWD. Writes loop until the whole body is written, so a short write no longer reports success.Fail-closed dedup.
holdwrites a durable<tid>.noticerecord before queueing the line. Once that record exists, the line is queued again only if it is provably not out:DELIVERED;If the queue write fails, the record is cleared so a retry queues the line.
One identity source.
agent_identity.lane_identity(state_dir, environ)composes the sameresolve+env_files+config_from_fileschain the bridge's_reenroll_identityuses, and a test pins that the two agree.holdevaluates it over the running Sparrow's environment (/proc/<pid>/environof the lock holder) when that is readable, else its own. The installedAGENT_MXIDsnapshot is no longer consulted.Two-way controls.
At
a77d49a7sources (git checkout a77d49a7 -- task_hold.py adapters/hatch/cli.py adapters/hatch/keepalive.py remote_gateway_bridge.py agent_identity.py proactive_destination.py, with the newdelivery_destination.pycopied in so the test file imports):tests/gateway-claim-gate-name-collision.test.pygivesFAIL: 4 failure(s), among themraised TypeError: 'module' object is not callablefor each of the three files.test_hatch_adapter.py TaskHold Hold DeliveryDestinationgivesRan 45 tests ... FAILED (failures=8, errors=4), with at least one failure per finding:After, at
f7d4df8d. Each run alone withnice -n 19while the 1-minute load was under 4:Bridge and CI-failing suites, each run alone, all exit 0:
gateway-claim-gate-name-collisiongateway-destined-proactive-not-claimedgateway-proactive-body-target-claimgateway-voice-room-gate(20)gateway-voice-room-membershiptests/remote-gateway-bridgesrc/remote-gateway-bridgeshutdown-sentinel-survives-failed-launchskills/process-hygiene/reap-orphans(74)gateway-auto-reenroll(39)gateway-proactive-explicit-room-without-defaultgateway-proactive-foreign-homeservergateway-proactive-readinessgateway-proactive-room-from-channel-env(11)gateway-proactive-threadag2-sparrow-agent-identity(11)proactive-destinationgateway-owner-mention(28)gateway-channel-skip-ownerThe node
.test.tssuites were not run locally.Diff coverage. Measured locally with coverage.py over the changed lines of the six touched modules: 329/331. The two uncovered lines, the put-back branch of
releasewhen a newer hold took the name, are the target oftest_a_newer_hold_placed_during_a_mismatched_release_stands, which was added and passes afterwards; coverage was not re-measured after adding it.Known limits at round 3 (superseded by round 4)
.sending.<pid>) can't be recalled when the answer arrives. It was claimed before the answer existed.holddies after writing the.noticerecord but before queueing the line, or the drain delivers the line but its best-effort receipt write fails, thenholdcannot prove the line's fate. It exits 3, queues nothing, and the task readstask-not-picked-up. This is fail-closed and visible; an operator clears it by answering the task./proc, as on macOS, or no live lock holder),holdresolves the identity from its own environment and Sparrow's state dir. That is the same resolver, but it may see a different environment than Sparrow does.., so no reader takes it for a marker.holddoes nothing.hold, through the drain, into the room or thread) and a current code-owner approval (the main ruleset requires one, including a code owner) are still owed before merge.Review round 2 (head
a77d49a7, fixes onf51518e1)This round addresses review 5624831068. I simplified the design rather than adding more mechanism.
One owner for the destination policy. The new dependency-light
ag2_sparrow/proactive_destination.pyowns:route(), the marker routing moved verbatim from the bridge;deliverable_here(), the homeserver check;homeserver_of()andRECEIPT_DIR;drain_room(), which combines them.The bridge's
_proactive_route,_room_is_deliverable_here,_own_homeserverand receipt write now delegate to it, and a contract test pins that delegation.holdcallsdrain_room()with the installedAGENT_MXID's homeserver, so!foreign:other.spaceis refused.holdno longer imports the bridge.Confinement. Every hold operation opens the literal
RESULT_DIR/.task-holdsafter anlstatcheck, withO_DIRECTORY|O_NOFOLLOWand an inode match. All reads, writes, links, unlinks and listings then go through that directory fd, and marker reads useO_NOFOLLOW. A symlinked hold dir, a file in its place, a symlinked marker (pointing inside or outside) and a same-directory alias are never written, read or pruned through. Releasing an alias removes the link, never its target.No requeue after delivery. Before queueing the line,
holdchecks three things: the queued.txt, a.sending*claim, and the drain's own receipt (outbox.item_status(RESULT_DIR/.outbox-ag2space-proactive, stem) == DELIVERED). A test writes that receipt through the drain's own_record_proactive_receiptand checks thatholdsees it.Simplified state. The pending/held two-phase protocol and the stale-reservation recovery are gone.
holdexits 3 ("not confirmed; the task still reads task-not-picked-up"). That is fail-closed: health shows stuck, never a hold with no line.release(token)never erases a newer hold.Non-finite and terminal markers.
sincemust be a finite epoch between 0 and 1e11, checked on both write and read. JSONNaNandInfinityparse as not held.replynow ends the hold along with the queued line. Keepalive ignores a hold on any task whose outbox already has a record (for example, delivered but archiving failed), so that task reads stuck.Two-way controls.
At
f51518e1sources with the new tests (git checkout f51518e1 -- task_hold.py adapters/hatch/cli.py adapters/hatch/keepalive.py remote_gateway_bridge.py, with the newproactive_destination.pypresent so the test file imports), thenpython3 packages/ag2-sparrow/tests/test_hatch_adapter.py TaskHold Hold Keepalive ProactiveDestination:The 22 failures include, one per finding:
!foreign:other.spaceand the drain-delegation test;sincerefused on write, and the keepalive answered-task test.The keepalive NaN/Infinity control,
Keepalive.test_a_marker_that_is_not_a_whole_well_formed_hold_still_reads_stuck, was run alone at both heads:AssertionError: 'task-held-for-owner' != 'task-not-picked-up'After, at
a77d49a7. Each group was run alone withnice -n 19while the 1-minute load was under 4:These drain-routing regressions were run alone as well, since the bridge now delegates:
tests/gateway-proactive-foreign-homeserver.test.pyPASS,tests/gateway-proactive-explicit-room-without-default.test.pyPASS,tests/gateway-channel-skip-owner.test.pyPASS, andtests/gateway-owner-mention.test.py28 OK.Diff coverage. Measured locally with coverage.py over the changed lines of
task_hold.py,proactive_destination.py,remote_gateway_bridge.py,cli.pyandkeepalive.py: 264/264 (100%).Original-parent control. The procedure is the same as above against
7940f25ff965f51df3273d94f009cb1b771119c0, wheretask_hold.py,proactive_destination.pyandholddo not exist. Every hold test then fails at import orAttributeError.Known limits
.sending.<pid>) can't be recalled when the answer arrives. The outbox has no cancel for an in-flight proactive send. Such a line was claimed before the answer was written, so it goes out ahead of the answer.holddies after queueing the line but before writing the marker, the asker gets the line, but the task readstask-not-picked-upuntilholdis run again. This is deliberately fail-closed. A retry does not requeue the line.holddoes nothing.holdjudges the homeserver from the installedAGENT_MXID. The drain uses its enrolled identity. Both are this agent's own ID, and when either is unknown, both fall back to "deliverable".hold, through the drain, into the room or thread) is still owed before merge, and so is the CLA check.Not in scope
holdis a no-op.src/health-check.pyandtask-orphan-checknever see Sparrow's Hatch task dir, so they are unchanged.Round: Kewei's REQUEST_CHANGES at
1e01bd89(headee341d83)Each regression below fails at
1e01bd89and passes atee341d83(same test file run against both)._reqnow resolves under the remaining budget (a helper thread it stops waiting for), gives each connect candidate only what is left, and records the TLS-wrapped socket (handshake run after it is recorded) for the timer, which shuts it down with the basesocket.shutdown. Tests:test_the_deadline_covers_slow_https_headers(local self-signed HTTPS; control 8.94 s vs bound 1.0 s),test_the_deadline_covers_a_slow_https_error_body(control 3.13 s),test_the_deadline_is_shared_by_every_connect_candidate(control 0.62 s vs 0.45 s),test_the_deadline_covers_name_resolution(control 0.84 s vs 0.5 s). A stalled-handshake test also passes at the control, since the raw socket was still recorded then.requeue_item_with_epochsets a stickyprior_outcome_unknownwhen the item was parkedoutcome-unknownor had a dispatch mark;hold_line_outcomereads it.src/outbox.pyand the vendored copy are in sync. Test:test_a_requeue_keeps_the_proof_that_an_earlier_send_may_have_landed(control:undelivered, reply says it will not post).agent_identity.lane_recordreturns(record, unreadable);published_lane_recordkeeps its contract.drain_predates_send_lockreturns None and names the problem on stderr when unreadable. Test:test_keepawake_keeps_a_drain_whose_lane_record_cannot_be_read(control: replaced,restarted=1)._lane_snapshotreads pid, homeserver and send-lock capability together;holdre-reads it under the send lock and refuses (REPLY_UNCONFIRMED, nothing published or marked) if it changed, before both the publish and the marker. Test:test_hold_refuses_when_the_drain_is_swapped_while_it_waits_for_the_lock(control: rc 0, notice posted)._publish_onceignores a failed temp unlink after the link succeeded, so reply no longer reports "nothing written". Test:test_an_answer_whose_temp_file_cleanup_fails_is_reported_as_written(control: exit 2).Run at
ee341d83: Hatch adapter 350 tests OK (2 skipped), served setup 109 OK, crash-receipt and unknown-parks PASS, drift checks PASS,git diff --checkclean,review-checks.sh --diffPASS. The live post-restart Hatch witness is still owed.🤖 Generated with Claude Code