Repository navigation
fix(cron): Bot Chat output waits for CLI owners to release - #111273
Conversation
alt-glitch
added
type/bug
૮ >ﻌ< ა ci reviewran on bcbbc93 — fix(cron): keep deferred delivery exceptions from aborting t
|
kvnloo
left a comment
kvnloo
left a comment
There was a problem hiding this comment.
Solid design — claim-before-execution plus at-most-once is the right call for this, and the lock split (drain lock vs producer lock) is clean. Two things:
-
_drainhas no per-record exception guard (cron/bot_chat_delivery.py:79). If_deliver_to_bot_chatraises, the background drain thread dies and the remaining queued records wait for the next tick; in the sync path the exception propagates out oftick()itself (no except around the drain call, only thefinallyreleasing the tick lock), so the rest of that tick's dispatch dies too. Worse, records drain in sequence order, so a persistently-raising head record starves everything behind it every cycle. Worth wrapping the delivery call in try/except and marking the recordambiguouson exception, then continuing. -
If a CLI owner never releases, the pending record waits forever — no TTL, no aging signal beyond the job receipt. Is indefinite retention the intent, or should there be a staleness bound / surfacing?
Verified the lock ordering while I was in there: drain takes .drain.lock then per-record .lock, defer() takes only .lock, so no deadlock between producer and drain. The at-most-once semantics and the receipt-first paths look right.
Keep never-started output behind unsupported owners and drain in admission order after release. Persist claims before execution and never replay uncertain started turns. Existing supported-owner receipts keep their authority. Credits 686f6c61's residual queue proposal in #100319. This is a scoped implementation, not general retry of failed CLI subprocesses. Native Electron before/after: CLI-owned target previously returned SESSION_NOT_OWNED and remained empty after release/tick; now its queued output and reply appear once in the target Bot Chat. Nested quiet CLI message_agent delivery to a named Desktop owner also passes on base.
Carry the original destination home and delivery ID into deferred drain and its child, rather than re-resolving a mutable profile/root. Missing destinations fail closed; supported-owner handoffs remain transferred, not ambiguous failures. Capture the producer root before the background thread starts, and retain/log malformed JSON without stopping healthy admissions or the whole cron tick. Two invariants reproduced failures on the published head. Real Electron root change and malformed-record cases are red before and green after; nested DM control remains passing. No automatic retry of claimed or uncertain turns.
Extend deferred dispatch's destination pin to ordinary CLI fallback, so custom-root and active-profile changes cannot redirect a checked target. Refuse a missing destination before launch and name the target on failure. Replace the old env-clearing expectation with two behavioral invariants and retain the native Electron custom-root reproduction. Adapted from the root-boundary fix and diagnosis in #104066. Related #104055, #104066. Co-authored-by: fangliquanflq <fangliquan@qq.com>
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.
6f7d6d2 to
bcbbc93
Compare
|
Thanks @kvnloo. Fixed and merged via 8f78531: a real permission-loss probe confirmed post-discovery Path.is_dir could raise and abort the tick. The claimed attempt now retains its ambiguous result while the healthy sibling continues; native Electron RED/GREEN and a second no-replay tick passed. Repeated head starvation was not present: claimed records are skipped on subsequent drains. Indefinite queued retention is intentional and documented; there is no silent TTL discard or retry of uncertain execution. |
3 participants
Cron output reaches the intended Bot Chat after CLI owners release and across custom-root CLI fallbacks.
Root cause: unsupported owners looked unowned to the cron fallback, so its CLI child refused ownership and no request survived for a later tick.
Live repro
Real Linux Electron, production backend and separate CLI/cron processes, disposable HOME/HERMES_HOME, loopback deterministic inference with real
message_agenttool execution. Harness and commands:evals/botmode-dm-delivery/.SESSION_NOT_OWNED; no output after release, native assertion failsmessage_agentto live named Alpha Desktop ownerEvidence:
/tmp/botmode-dm-recovery-cli-owner.log,/tmp/botmode-dm-recovery-final-native.log,/tmp/botmode-dm-recovery/cli-owner-after-release.png,/tmp/botmode-dm-recovery/nested-delivery-desktop.png.Targeted Python validation: pending deferral/attempt-fence, existing cron Bot Chat and live-owner suites, tool mailbox and gateway consumer: 32 passed across five files. Full
tests/crondirectory: 1338 passed, 1 skipped across 114 files (/tmp/botmode-dm-recovery-cron-directory.log). Ruff, compatibility pointers, subprocess stdin and diff checks passed. Independent review identified synchronous-tick exit, unordered filenames and concurrent drain hazards; each was corrected before publication.Scope and limitations
Credits @686f6c61 for #100319's surviving queue proposal. Related #100319; not fully superseded: arbitrary unowned CLI-failure retry is deliberately not added because a failed/timeout child may already have executed. A crash after claim but before launch remains claimed and is not replayed. Historical job delivery status is not automatically refreshed.
Related #105460 / #105323: named-profile live delivery is verified working; this PR does not change
--in ~or claim their workspace premise. Related #109750 / #109767: nested quiet CLI delivery works on this main, which already includes quiet-session notify binding; native Windows and long-running legacy-child cases remain unverified. The nested live transcript needed reload to show its incoming sender card in this fixture (reply and authoritative persistence already arrived); this PR does not fix that renderer-refresh behavior.No merges, issue closures or CI watchers performed.
Independent review follow-up
Head
c827ae179d67c71ff85a2f8f328652155e80a607fixes two reproduced defects in the published head:transferred, notambiguous; the target mailbox receipt remains authoritative.Profile 'beta' does not exist; zero output rowsJSONDecodeErroraborts real tickmessage_agentcontrolEvidence:
/tmp/botmode-dm-review/native-red2.log,corrupt-red.log,head-native.log,head-native/cli-owner-after-release.png,locking.json. Previous/tmp/botmode-dm-recovery*proof is unchanged. One intermediate fixture failure (--in ~against a nonexistent scratch HOME) is retained innative-green.log; both source legs were rerun with an existing HOME.Full cron directory: 1342 passed, 1 skipped across 115 files, verified by per-file receipt aggregation (
cron-aggregate.json). The first serial command hit the tool's 420s timeout after 112 completed files; its remaining three files passed in a second command, not silently counted as run. Related mailbox/DM/gateway-consumer suites: 57 passed, 1 skipped. Ruff, Windows-footgun, compatibility-pointer, subprocess-stdin and diff checks passed. Native Windows remains unverified.The pending store remains deliberately separate from
cron.delivery_queue: that queue owns whole execution-level adapter sends and terminalizes a claimed send, whereas this queue waits for a particular Bot Chat owner before claim and transfers into the existing owner mailbox. No second live-owner inference consumer or general retry loop was introduced. Claims remain non-expiring, including process death before spawn. Permanent receipts retain payloads and scans remain linear; this change does not add retention, auto-follow profile renames, or recovery of missing queue directories.Ordinary custom-root fallback completed
Head
6f7d6d2f29179c154175d49ae3bca5db26d94208extends the existing exact-home pin to the ordinary never-deferred CLI lane. The child cannot rediscover its destination through inheritedHOMEor a changedactive_profile. Missing directories are refused before launch, and CLI failure diagnostics name the exact home. Receipt identities, transfer/status semantics and the no-ambiguous-retry policy are unchanged.Credit @fangliquanflq for #104066's root-boundary diagnosis and anchoring fix (co-authored). Related #104055; #104066's fallback defect is now covered here. The older proposal anchors a second name lookup to the root; this branch already has a resolved destination, so uses that directly. Full source threads and overlapping #100319/#105442 were read before extending the fix.
c827ae179d67cand a scheduler-source swap to fetchedorigin/main:Profile 'alpha' does not exist, native assertion RED.hermescreatedmessage_agentHarness:
evals/botmode-dm-delivery/probe-cron-root.spec.ts. Evidence:/tmp/botmode-cron-root/{before2,origin-main,final,controls}.log;final/{owners.json,children.log,result.json,rows.json,missing.json,ordinary-recipient.png}. Screenshot vision-checked: Alpha selected,ORDINARY_CRON_SENTINELand loopback reply visible. The initial fixture attempt used the wrong default row label (actual label Hermes); its failure is retained inbefore.log, not counted as a product defect. All inference was deterministic loopback; production backend, CLI, ownership and rendering were real.Ruff, Windows-footgun, compatibility-pointer, subprocess-stdin and diff checks passed. Worktree clean after native harness teardown. Native Windows remains unverified. No watcher, merge, closure or issue comment was performed; CI monitoring remains with the parent campaign.
Infographic
Combined campaign verification
All four exact PR heads (#111240, #111283, #111273, #111298) were locally integrated onto main
1a990f30628c25fb83d29c4d3b3d18dcb085406ein unionee20e99def270fb4c160018358a938e2a344c822; parent verified zero missing commits from every head. Additive test-fixture conflict resolution preserves both group scripting and held-response behavior. 13 native Linux Electron tests passed, covering group handoffs/queue/Stop/late/error, cron owner deferral and custom-root fallback, nested/stale-launcher delivery, parallel side chats, and busy-DM FIFO. 602 Bot Mode unit tests passed; 155 Python invariants passed, 3 skipped; full Desktop build and typecheck/lint passed (179 existing warnings, no errors). Real backend and tool execution with scripted loopback inference; late-observation clocks accelerated only in the named harvest test. Native Windows remains unverified. Evidence and conflict-resolution patch:/tmp/botmode-campaign/union/verification.json,conflicts.patch, and neighboring logs/screenshots. This is local integration proof, not a merge to main.Pre-merge review: per-record exception isolation
Head
bcbbc9314ed6ad24c22b7728e1096dca6da02d15, cleanly rebased onto mainb8bf4843518c4dc1319d4476adda0cc9cc169045(including #111240).@kvnloo's full review was checked against reachable paths. Confirmed: the CLI fallback's post-discovery
home.is_dir()sits outside its exception guards; actual directory traversal permission loss on Python 3.11/Linux raisedPermissionErrorthrough_deliver_to_bot_chatand_drain. The six-line guard now logs that exception, retains the claimed attempt as ambiguous, and continues siblings in the same tick. Existing transfer receipts remain authoritative.Refuted: the same raising head cannot starve every later tick.
_draindurably writesclaimedbefore calling delivery and skips every non-queued record on subsequent scans. The existing interruption invariant proves that a claimed head is skipped and the next sibling proceeds. The reproduced defect is an aborted individual drain/tick, not automatic repeated-head execution.Retention policy unchanged: never-started output waits indefinitely if an unsupported owner never releases. No TTL silently discards accepted output; payloads/receipts remain inspectable, scans remain linear, and claimed/ambiguous attempts never expire into automatic retry. No aging UI or new recovery architecture added.
Live repro: real Linux Electron, production backend and independent real cron/CLI processes, deterministic loopback inference. A controlled filesystem exception at the verified post-discovery boundary makes pre-fix tick exit 1 with head claimed/sibling queued. Fixed tick retains head ambiguous and delivers its healthy sibling exactly once; a second real tick replays neither. Beta's output and reply render in Desktop. Fresh build of the final rebased head: 1 native passed (51.2s). This controlled-fault test is separate from the real chmod-based reachability probe; neither claims an observed user incident or native Windows parity.
Validation: one portable invariant RED/GREEN; full
tests/cron: 1351 passed, 1 skipped across 117 files; Desktop build, Ruff, Windows-footgun, compatibility-pointer and diff checks passed. Evidence:/tmp/botmode-dm-review-exception-unit-red.log(real permissions),/tmp/botmode-dm-exception-portable-red.log,/tmp/botmode-dm-exception-cron.log,/tmp/botmode-dm-exception-{red,green,final}.log, and/tmp/botmode-dm-exception/final/{exception-receipts.json,cli-owner-after-release.png}. Native screenshot vision-checked. Earlier 13-test union proof remains historical, not rerun for this six-line backend guard.No merge, watcher, closure or review comment performed; current-head CI remains with the parent campaign.