From 8f7853188f0b3c7abbd9ebaa343764c37057b59a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 14 Sep 2026 17:10:13 -0700 Subject: [PATCH] fix(cron): keep deferred delivery exceptions from aborting ticks Catch unexpected delivery exceptions after claim, retain diagnostics and continue sibling admissions without authorizing replay. Preserve indefinite retention. Reproduced PermissionError at target traversal after discovery. Native Electron controlled-fault A/B confirms the healthy sibling settles and renders once. --- cron/bot_chat_delivery.py | 7 +++- evals/botmode-dm-delivery/README.md | 17 ++++++++ evals/botmode-dm-delivery/exception-tick.py | 41 ++++++++++++++++++ .../probe-dm-delivery.spec.ts | 17 ++++++-- tests/cron/test_bot_chat_pending.py | 42 +++++++++++++++++++ website/docs/user-guide/features/cron.md | 1 + 6 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 evals/botmode-dm-delivery/exception-tick.py diff --git a/cron/bot_chat_delivery.py b/cron/bot_chat_delivery.py index 458a6d3957..5d38f96f24 100644 --- a/cron/bot_chat_delivery.py +++ b/cron/bot_chat_delivery.py @@ -92,7 +92,12 @@ def _drain(root: Path) -> None: atomic_json_write(path, record, fsync_dir=True, mode=0o600) job = record["job"] job.pop("_bot_chat_delivery_receipts", None) - error = _deliver_to_bot_chat(job, record["content"], record["profile"], deferred=record) + try: + error = _deliver_to_bot_chat(job, record["content"], record["profile"], deferred=record) + except Exception as exc: + # The claim survives uncertainty; one failed attempt must not stop peers. + error = f"{type(exc).__name__}: {exc}" + logger.exception("Deferred Bot Chat delivery %s failed", record["id"]) receipt = job.get("_bot_chat_delivery_receipts", {}).get( f"bot-chat:{record['profile'] or '(own)'}") status = "transferred" if receipt else "ambiguous" if error else "settled" diff --git a/evals/botmode-dm-delivery/README.md b/evals/botmode-dm-delivery/README.md index 1f2679a8df..7d4b9bbdb8 100644 --- a/evals/botmode-dm-delivery/README.md +++ b/evals/botmode-dm-delivery/README.md @@ -59,6 +59,23 @@ was not created, so `--in ~` correctly refused); that failed receipt is retained in `native-green.log`, and both source legs were rerun with an existing HOME. Prior `/tmp/botmode-dm-recovery*` evidence remains untouched. +## Per-record delivery exception isolation + +Set `BOT_DM_EXCEPTION=1` and select `-g "cron output"` for the controlled native +exception probe. `exception-tick.py` raises `PermissionError` at the actual +post-discovery target `Path.is_dir()` boundary, not at the delivery helper. +The same exception was first reproduced with real directory traversal permission +loss on Python 3.11/Linux; the retained test uses a portable controlled fault. +Before the guard, native tick exits 1, leaving the head claimed and sibling queued. +Afterward, the head is ambiguous, the sibling settles and renders once, and a +second real tick replays neither. Logs: `/tmp/botmode-dm-exception-{red,green}.log`; +receipts and screenshot: `/tmp/botmode-dm-exception/{red,green}/`. + +The review's repeated-head starvation claim is not reachable: the claim commits +before delivery and later scans skip every non-queued record. One failed tick is +real; recurring replay of that same head is not. Indefinite queued/payload retention +is intentional, with no TTL or automatic ambiguous retry introduced here. + ## Ordinary custom-root fallback (#104066 / #104055) `probe-cron-root.spec.ts` adds the never-deferred sibling: copy it to diff --git a/evals/botmode-dm-delivery/exception-tick.py b/evals/botmode-dm-delivery/exception-tick.py new file mode 100644 index 0000000000..c0482dac9d --- /dev/null +++ b/evals/botmode-dm-delivery/exception-tick.py @@ -0,0 +1,41 @@ +"""Controlled filesystem fault; run only against the native disposable sandbox.""" +import json +import os +from pathlib import Path + +from cron import scheduler_delivery as delivery +from cron.bot_chat_delivery import _root +from cron.scheduler import tick + +root = _root() +original_which = delivery.shutil.which +original_is_dir = Path.is_dir +armed = False +raised = 0 + + +def resolve_cli(*args, **kwargs): + global armed + armed = raised == 0 + return original_which(*args, **kwargs) + + +def is_dir(path): + global armed, raised + if armed and path == Path(os.environ["HERMES_HOME"]) / "profiles" / "beta": + armed = False + raised += 1 + raise PermissionError("controlled target traversal denied after discovery") + return original_is_dir(path) + + +delivery.shutil.which = resolve_cli +Path.is_dir = is_dir +try: + tick(verbose=False) + tick(verbose=False) +finally: + Path.is_dir = original_is_dir + delivery.shutil.which = original_which + records = [json.loads(p.read_text(encoding="utf-8")) for p in root.glob("*.json") if p.name != "broken.json"] + Path(os.environ["BOT_DM_EXCEPTION_RECEIPT"]).write_text(json.dumps({"raised": raised, "records": records}, indent=2), encoding="utf-8") diff --git a/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts b/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts index 7ce033821d..6b57e5a027 100644 --- a/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts +++ b/evals/botmode-dm-delivery/probe-dm-delivery.spec.ts @@ -49,15 +49,26 @@ test('cron output waits for a CLI-only owner and arrives after owner release', a try { await fixture.mock.waitForHeldCompletion() const script = 'import json; from cron.scheduler_delivery import _deliver_to_bot_chat; j={"id":"cli-residual","name":"CLI residual","execution_id":"fixed-execution"}; result=_deliver_to_bot_chat(j,"CLI_OWNER_CRON_SENTINEL","beta"); print(json.dumps({"result":result,"job":j}))' - const result = JSON.parse(execFileSync(python, ['-c', script], { env: cronEnv, cwd: repo, encoding: 'utf8', timeout: 30_000 })) + const setup = process.env.BOT_DM_EXCEPTION ? 'from pathlib import Path; from cron.bot_chat_delivery import defer; from hermes_constants import get_hermes_home; defer("e"*64,{"id":"exception-head"},"EXCEPTION_MUST_NOT_RUN","beta",get_hermes_home()/"profiles"/"beta"); ' : '' + const result = JSON.parse(execFileSync(python, ['-c', setup + script], { env: cronEnv, cwd: repo, encoding: 'utf8', timeout: 30_000 })) console.log('CLI_OWNER_CRON_ADMISSION', JSON.stringify(result)) fs.writeFileSync(path.join(evidence, 'cli-owner-admission.json'), JSON.stringify(result, null, 2)) if (process.env.BOT_DM_CORRUPT) fs.writeFileSync(path.join(fixture.sandbox.hermesHome, 'cron', 'bot_chat_pending', 'broken.json'), '{') fixture.mock.releaseHeldStream() await expect.poll(() => child.exitCode, { timeout: 60_000 }).toBe(0) fs.mkdirSync(path.join(fixture.sandbox.root, 'changed-launch-home'), { recursive: true }) - const ticker = spawn(python, ['-c', 'import time; from cron.scheduler import tick; from cron.bot_chat_delivery import _running; tick(verbose=False);\nwhile _running: time.sleep(0.1)'], { env: { ...cronEnv, HOME: path.join(fixture.sandbox.root, 'changed-launch-home') }, cwd: repo, stdio: ['ignore', output, output] }) - await expect.poll(() => ticker.exitCode, { timeout: 90_000 }).toBe(0) + const tickArgs = process.env.BOT_DM_EXCEPTION ? [path.join(repo, 'evals/botmode-dm-delivery/exception-tick.py')] : ['-c', 'import time; from cron.scheduler import tick; from cron.bot_chat_delivery import _running; tick(verbose=False);\nwhile _running: time.sleep(0.1)'] + const receiptPath = path.join(evidence, 'exception-receipts.json') + const ticker = spawn(python, tickArgs, { env: { ...cronEnv, HOME: path.join(fixture.sandbox.root, 'changed-launch-home'), BOT_DM_EXCEPTION_RECEIPT: receiptPath }, cwd: repo, stdio: ['ignore', output, output] }) + await expect.poll(() => ticker.exitCode, { timeout: 90_000 }).not.toBeNull() + expect(ticker.exitCode).toBe(0) + if (process.env.BOT_DM_EXCEPTION) { + const receipts = JSON.parse(fs.readFileSync(receiptPath, 'utf8')) + expect(receipts.raised).toBe(1) + expect(receipts.records.find((r: { id: string }) => r.id === 'e'.repeat(64)).status).toBe('ambiguous') + expect(receipts.records.find((r: { id: string }) => r.id !== 'e'.repeat(64)).status).toBe('settled') + expect(dbMessages('beta').filter(([, text]) => text.includes('EXCEPTION_MUST_NOT_RUN'))).toHaveLength(0) + } await openBot('beta') expect(dbMessages('beta').filter(([role, text]) => role === 'user' && text.includes('CLI_OWNER_CRON_SENTINEL'))).toHaveLength(1) await expect(fixture.page.getByText(/CLI_OWNER_CRON_SENTINEL/).filter({ visible: true }).first()).toBeVisible({ timeout: 45_000 }) diff --git a/tests/cron/test_bot_chat_pending.py b/tests/cron/test_bot_chat_pending.py index ad811451fa..4cf3d751a0 100644 --- a/tests/cron/test_bot_chat_pending.py +++ b/tests/cron/test_bot_chat_pending.py @@ -1,5 +1,6 @@ """Only never-started cron delivery may wait for a CLI owner's release.""" import subprocess +from pathlib import Path from unittest.mock import Mock import pytest @@ -40,6 +41,47 @@ def test_cli_owner_deferral_and_attempt_fence(tmp_path, monkeypatch, error): db.close() +def test_delivery_exception_retains_attempt_and_continues_siblings(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + blocked_parent = tmp_path / "blocked" + blocked_home = blocked_parent / "recipient" + blocked_home.mkdir(parents=True) + for home in (blocked_home, tmp_path): + db = SessionDB(db_path=home / "state.db") + db.create_session(session_id="chat", source="cli") + db.set_session_title("chat", "Bot Chat") + db.close() + queue.defer("b" * 64, {"id": "bad"}, "bad output", "", blocked_home) + queue.defer("a" * 64, {"id": "good"}, "good output", "", tmp_path) + calls = [] + original_is_dir = Path.is_dir + armed = False + + def resolve_cli(_): + nonlocal armed + armed = True + return "/bin/hermes" + + def is_dir(self): + if armed and self == blocked_home: + raise PermissionError("target traversal denied after discovery") + return original_is_dir(self) + + def run(*args, **kwargs): + calls.append(kwargs["env"]["HERMES_HOME"]) + return subprocess.CompletedProcess([], 0, "", "") + + monkeypatch.setattr(delivery.shutil, "which", resolve_cli) + monkeypatch.setattr(delivery.subprocess, "run", run) + monkeypatch.setattr(Path, "is_dir", is_dir) + queue.drain() + assert queue.read_pending("b" * 64)["status"] == "ambiguous" + assert "PermissionError" in queue.read_pending("b" * 64)["error"] + assert queue.read_pending("a" * 64)["status"] == "settled" + queue.drain() + assert calls == [str(tmp_path)] + + def test_pending_queue_uses_admission_order_and_keeps_claims(tmp_path, monkeypatch): monkeypatch.setenv("HERMES_HOME", str(tmp_path)) job = {"id": "job"} diff --git a/website/docs/user-guide/features/cron.md b/website/docs/user-guide/features/cron.md index b96d26016a..c9f691deb8 100644 --- a/website/docs/user-guide/features/cron.md +++ b/website/docs/user-guide/features/cron.md @@ -537,6 +537,7 @@ error. A delivery failure does not count toward the job's `failure_streak` - Each delivery costs the target bot one full agent turn — mind the schedule frequency. - Composes with other targets (`bot-chat,telegram`) but is never included in `all`. - If the canonical chat is open in a mailbox-capable Desktop/TUI backend, delivery is **durably queued immediately**, whether the bot is idle or busy. Only that live owner runs the incoming turn; cron does not start a competing CLI writer. If a CLI-only or older unsupported owner holds the chat, cron retains the never-started output under the sending profile's `cron/bot_chat_pending/.json`. Later scheduler ticks deliver after that owner releases the chat, in admission order. Deferred work retains its admitted destination home and receipt ID even if the scheduler's launch root changes; a missing/renamed destination is not recreated or resolved to another profile. A `transferred` pending record points to the live-owner receipt, not a failed turn. Malformed JSON records are retained and logged without blocking other queued outputs. With no owner, the existing `hermes chat -c "Bot Chat" --create-if-missing` lane remains available (normal session ownership checks still apply). That child uses the exact destination home already checked by cron, including custom roots; inherited `HOME` or a changed active profile cannot redirect it. A missing destination directory is refused before launch, not recreated. A deferred request is claimed before launching that lane; interruption or an uncertain subprocess result never causes an automatic resend. +- Never-started outputs have no TTL: if an unsupported owner never releases, they remain queued rather than being silently dropped. Receipts retain their payloads indefinitely. An unexpected delivery exception is logged and retained as `ambiguous`, without stopping sibling deliveries in that drain; claimed/ambiguous attempts are never automatically replayed. - **Queued is not completed.** Cron records receipt IDs and `queued`/`claimed` statuses in `last_delivery_queued`, with delivery outcome `queued` (neither delivered nor failed). A successful job shows `delivery_queued`; genuine errors on other targets still take precedence as delivery failures. The bot may complete later. The durable receipt in the target profile's `runtime/bot_live_delivery/.json` is authoritative; cron's historical status is not automatically refreshed. - Rechecking the same execution inspects its existing receipt, even if the owner has disappeared. It never falls back to another writer after acceptance. `failed`, `cancelled`, or `ambiguous` receipts are not automatically replayed; inspect the chat and receipt before intentionally starting new work. Each new cron execution has a distinct delivery ID.