Skip to content

fix(cron): queue bot-chat delivery for CLI-only owners - #100319

Closed
686f6c61 wants to merge 1 commit into
NousResearch:mainfrom
686f6c61:fix/99956-bot-chat-live-owner
Closed

686f6c61 wants to merge 1 commit into
NousResearch:mainfrom
686f6c61:fix/99956-bot-chat-live-owner

Conversation

@686f6c61

@686f6c61 686f6c61 commented Sep 1, 2026 •

Copy link
Copy Markdown

What does this PR do?

#105442 landed the Desktop/TUI live-owner path and closed #99956. Cron can now admit into an advertised Desktop/TUI Bot Chat owner; that work is not repeated here.

This PR is the leftover that merge did not take: a general queue-before-subprocess for CLI-only / unsupported owners, plus scheduler-tick drain after the unowned CLI lane fails. While a live owner holds the session the one-shot hermes chat runner is skipped; after release (or after a CLI-lane failure) later ticks retry from the durable queue. It does not inject into the live transcript (that is #70406).

The branch still carries the original combined patch and conflicts with current main. It is not rebased onto the landed live-consumer path. At-least-once / ambiguous-retry behavior needs separate consideration before this residual should land.

Related Issue

#99956 (completed by #105442). Residual only: CLI-only / unsupported owners and unowned-CLI failure drain.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • cron/bot_chat_delivery.py: durable queue, live-owner probe, drain
  • cron/scheduler.py: _deliver_to_bot_chat admits then drains; idle ticks retry the queue
  • tests/cron/test_cron_bot_chat_delivery.py: live-owner defers without a second subprocess; drain after CLI failure

How to Test

uv run --extra dev python -m pytest tests/cron/test_cron_bot_chat_delivery.py -q
# 19 passed (on this branch, before the #105442 merge)

Not runtime-tested against a live Desktop or CLI owner. Not re-run against current main (CONFLICTING).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux (pytest on this branch)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A

deliver=bot-chat used to spawn hermes chat against a session CLI/TUI/
Desktop already owned, which failed with exit 1 and dropped the job
output. Admit the turn durably first, skip the second runner while
the lease is held, and retry on later scheduler ticks.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 1, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

This PR makes cron→Bot Chat delivery durable (#99956). A new cron/bot_chat_delivery.py module atomically writes each completed output to a cross-process-locked JSON queue before the scheduler acknowledges the job, checks the target profile's Bot Chat for a live session owner, and — if unowned — runs the canonical chat --in ~ -c "Bot Chat" ... --query-file lane, retrying retained records on every scheduler tick. _deliver_to_bot_chat in scheduler.py becomes a thin delegator.

  • Design is sound: queue writes are fsync'd and os.replaced under a .queue.lock, drains run under a separate .drain.lock (no lock-order nesting, so no deadlock), records are timestamp-prefixed for in-order retry, and per-profile blocked_profiles correctly prevents one blocked target from head-of-line blocking others.
  • Fail-safe ownership probe: _target_has_live_owner returns None on any uncertainty, and drain_queue treats non-False as "defer" (bot_chat_delivery.py:294), so a second process is never started against a session that might be leased. The compression-tip resolution and the end-to-end lease test (test_live_owner_probe_resolves_exact_canonical_session) cover the tricky wiring.
  • At-least-once is inherent here and worth being explicit about: a TimeoutExpired kill or a crash between successful subprocess delivery and path.unlink() re-delivers the same output as a second Bot Chat turn. The timeout comment already concedes "the bot's turn may still complete"; that is acceptable, but duplicate turns are a possible, if rare, outcome.
  • Behavior change to note: deliver() now returns None (job success) as soon as the record is durable — delivery failures that previously surfaced in last_delivery_error are now deferred silently to the queue and only visible in logs. Consistent with the intent, but job history no longer distinguishes "delivered" from "queued, pending".
  • Non-blocking: retained records retry forever with no max-age/dead-letter; a permanently blocked target (e.g., deleted profile) would accumulate records and retry every tick. Also, on Windows msvcrt.locking(LK_LOCK) only blocks ~10s before raising, which would surface as a queueing failure under heavy lock contention.
  • Tests cover failure→queue→drain-retry, timeout retention, live-owner deferral, and attribution; the refactor moved subprocess mocking cleanly to the new module.

Verdict: LGTM

teknium1 pushed a commit that referenced this pull request Sep 7, 2026
Adapt FalconOrtiz's owner-mailbox proposal from #101564 onto the current
notification poller and topical modules. The durable mailbox is cross-process
ingress only: the existing owner admits its normal prompt turn after the
current turn and human FIFO clear. Retain immutable receipts, stable admission
identities, capability and lease fencing, compression lineage, and disable
blind recovery replay of imported turns.

The original stale server hunks and expiring receipt protocol were rebuilt
rather than cherry-picked: the current facade decomposition and durable busy
admission contract differ. Credit the earlier owner-mailbox work in #100544
and durable producer work in #100319.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: 686f6c61 <github@00b.tech>
@teknium1

teknium1 commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks @686f6c61 for the durable producer-admission and ownership-aware retry work. #105442 has merged at 5280fe99872a58f1b9d25b00a79d240ecd035f7d and credits your contribution in the owner-ingress commit. Cron can now durably queue directly to an advertised Desktop/TUI owner, which consumes through its existing prompt-turn path; queued admission is reported separately from completion.

Leaving this PR open: its general queue-before-subprocess and later scheduler-tick drain also preserve/retry delivery after unsupported or CLI-only owners release, and after failures in the unowned CLI lane. The merged live-consumer path does not provide that general fallback queue. That surviving behavior needs separate consideration, including the at-least-once/ambiguous-retry concerns already discussed here; it is not fully superseded by the Desktop/TUI fix.

hermes-release-bot Bot pushed a commit to mekenthompson/hermes-agent that referenced this pull request Sep 8, 2026
Adapt FalconOrtiz's owner-mailbox proposal from NousResearch#101564 onto the current
notification poller and topical modules. The durable mailbox is cross-process
ingress only: the existing owner admits its normal prompt turn after the
current turn and human FIFO clear. Retain immutable receipts, stable admission
identities, capability and lease fencing, compression lineage, and disable
blind recovery replay of imported turns.

The original stale server hunks and expiring receipt protocol were rebuilt
rather than cherry-picked: the current facade decomposition and durable busy
admission contract differ. Credit the earlier owner-mailbox work in NousResearch#100544
and durable producer work in NousResearch#100319.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: 686f6c61 <github@00b.tech>
@686f6c61 686f6c61 changed the title fix(cron): queue bot-chat delivery when the session has a live owner fix(cron): queue bot-chat delivery for CLI-only owners Sep 8, 2026
@686f6c61

686f6c61 commented Sep 8, 2026

Copy link
Copy Markdown
Author

#105442 landed the Desktop/TUI live-owner path and closed #99956.

Leaving this open for the residual: queue-before-subprocess for CLI-only / unsupported owners, and drain after unowned-CLI failure. Not rebasing the landed live-consumer path onto this branch. At-least-once / ambiguous-retry stays open.

Dropped Fixes #99956 from the description so this is not a second closer for a completed issue.

@alt-glitch alt-glitch added the needs-decision Awaiting maintainer decision before any implementation label Sep 8, 2026
teknium1 added a commit that referenced this pull request Sep 15, 2026
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.
teknium1 added a commit that referenced this pull request Sep 15, 2026
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.
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @686f6c61. The surviving CLI-only-owner deferral now landed in #111273 at 8f78531 with credit. Real CLI owner + Electron + scheduler tick A/B passed, including custom roots, concurrent drain claims and exception isolation. Leaving this PR open only for its distinct arbitrary unowned-CLI failure retry proposal: the landed path deliberately never replays an already-claimed/uncertain attempt, so it is not a complete supersession of that policy.

@alt-glitch alt-glitch added P3 Low — cosmetic, nice to have and removed P2 Medium — degraded but workaround exists labels Sep 15, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @686f6c61 — the CLI-only-owner queue this PR proposed landed on main via 5d8390d (cron/bot_chat_delivery.py::defer + replay), alongside the live-owner ingress from 5280fe9. You noted the branch was unrebased and conflicting; closing as superseded with credit for the design.

@teknium1 teknium1 closed this Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping area/sessions Session lifecycle, resume, persistence, history comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cron): bot-chat delivery fails when target profile has active session lock

4 participants