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.
This commit is contained in:
@@ -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"
|
||||
|
||||
@@ -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
|
||||
|
||||
41
evals/botmode-dm-delivery/exception-tick.py
Normal file
41
evals/botmode-dm-delivery/exception-tick.py
Normal file
@@ -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")
|
||||
@@ -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 })
|
||||
|
||||
@@ -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"}
|
||||
|
||||
@@ -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/<receipt-id>.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/<receipt-id>.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.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user