fix(gateway): make cron ticker errors visible at default log level by briandevans · Pull Request #32616 · NousResearch/hermes-agent

briandevans · 2026-05-26T13:14:19Z

What does this PR do?

The inner try/except around cron_tick(...) inside _start_cron_ticker logged failures at DEBUG and only matched Exception. Two consequences from #32612:

Invisible at INFO. Tick errors never made it to the gateway log at the default level, so cron jobs could silently stop firing for hours with hermes cron status still reporting healthy.
No BaseException catch. A SystemExit / KeyboardInterrupt / BaseExceptionGroup (or anything thrown by a C extension) killed the ticker thread with zero log output.

This PR addresses the Immediate (low risk) part of the reporter's fix list:

Escalate the Exception arm to WARNING and switch to exc_info=True so the traceback surfaces at the default log level.
Add a BaseException arm that logs at ERROR with traceback, then re-raises so the thread exits as Python intends rather than dying silently.

The watchdog / hermes cron status thread-liveness fix (the issue's bug #3) is intentionally out of scope here — it overlaps with the in-flight #26734 (cron ticker watchdog) and is a much larger structural change. Splitting the two keeps this PR a 12-line surgical fix that can land independently.

Related Issue

Fixes #32612

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

gateway/run.py — split the except Exception block in _start_cron_ticker into a WARNING-level Exception arm (with exc_info) and an ERROR-level BaseException arm that re-raises. Both keep the existing single-call-site structure.
tests/gateway/test_cron_ticker_error_visibility.py — three regression tests driving _start_cron_ticker with a mocked cron.scheduler.tick: one each for Exception (logged at WARNING with traceback), SystemExit (ERROR + re-raised), and KeyboardInterrupt (ERROR + re-raised).

How to Test

uv run --with pytest --with pytest-xdist --with pytest-asyncio \
    python3 -m pytest tests/gateway/test_cron_ticker_error_visibility.py tests/cron/test_scheduler.py -v

Expected: 134 passed. All 3 new tests fail on main (confirmed via git stash + rerun); all 3 pass with this change applied.

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 run focused tests for the touched code and all pass
I've added tests for my changes (required for bug fixes, strongly encouraged for features)
I've tested on my platform: macOS 15.x

Documentation & Housekeeping

I've updated relevant documentation (README, docs/, docstrings) — N/A (in-line comments cover the WHY)
I've updated cli-config.yaml.example if I added/changed config keys — N/A
I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
I've considered cross-platform impact (Windows, macOS) per the compatibility guide — logging only; behaviour identical on all platforms
I've updated tool descriptions/schemas if I changed tool behavior — N/A

Related / Positioning

fix: harden URL safety and cron ticker watchdog #26734 (open, irislin2502) — adds a ticker-thread watchdog + restart loop. That PR addresses the issue's bug Architecture planning #3 (no liveness watchdog). This PR addresses bugs Terminal tool #1 and Support passing morph snapshot id #2 (invisible logging + missing BaseException catch). The two are orthogonal and compose: a watchdog needs visible error logging to be useful, and visible logging without a watchdog at least surfaces the failure for operator intervention.
perf(cron): narrow file lock scope + heartbeat ticker #21901 (open, gengwb) — narrows the file-lock scope inside cron.scheduler.tick. Different module, different concern; no overlap.

Audited siblings: the other except Exception: logger.debug(...) blocks lower in _start_cron_ticker cover periodic cache cleanup and channel-directory refresh — distinct failure profiles (hygiene tasks that genuinely don't need operator-visible failures on every miss), so this PR keeps scope to the cron-tick block specifically named in the issue. Happy to widen if preferred.

For New Skills

N/A.

Screenshots / Logs

Before (default INFO level, ticker dies silently):

May 25 13:51:38  Cron ticker started (interval=60s)
May 25 13:56:26  (no further ticker output for 15.5 hours)

After this change (default INFO level, same failure):

May 25 13:51:38  Cron ticker started (interval=60s)
May 25 13:51:38  Cron tick error
Traceback (most recent call last):
  File ".../gateway/run.py", line 18017, in _start_cron_ticker
    cron_tick(verbose=False, adapters=adapters, loop=loop)
  ...

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Improves visibility and handling of errors occurring inside the gateway cron ticker loop so operators can see failures at default log levels and fatal errors don’t silently kill the ticker thread.

Changes:

Log regular cron tick exceptions at WARNING with tracebacks.
Log and re-raise BaseException-derived failures so the ticker thread exits loudly/cleanly.
Add regression tests covering both behaviors (Exception vs BaseException).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
gateway/run.py	Adjusts cron ticker exception handling to log at higher severity with tracebacks and re-raise fatal errors.
tests/gateway/test_cron_ticker_error_visibility.py	Adds tests asserting correct log levels/tracebacks and re-raise behavior for fatal exceptions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+import pytest
+
+


+        except BaseException:
+            # `except Exception` does not catch SystemExit, KeyboardInterrupt,
+            # or BaseExceptionGroup — log a traceback first, then re-raise so
+            # the thread exits as Python intends rather than dying silently.
+            logger.error(
+                "Cron ticker fatal error; thread will exit", exc_info=True
+            )
+            raise


briandevans · 2026-05-26T14:10:01Z

@copilot All findings addressed in 697fbb288:

gateway/run.py: narrowed except BaseException to (SystemExit, KeyboardInterrupt, BaseExceptionGroup) so the handler matches the docstring intent and doesn't intercept GeneratorExit or other interpreter-shutdown signals.
tests/gateway/test_cron_ticker_error_visibility.py: dropped the unused pytest and unittest.mock.patch imports.

All three existing tests still pass.

The inner exception handler around `cron_tick(...)` in `_start_cron_ticker` swallowed errors at DEBUG and only caught `Exception`. Two consequences: 1. Tick failures were invisible at the default INFO log level, so cron jobs could silently stop firing for hours with `hermes cron status` still reporting healthy. 2. Any BaseException-subclass error (SystemExit, KeyboardInterrupt, BaseExceptionGroup, C-extension failures) killed the ticker thread with no log line at all. Escalate the Exception path to WARNING with `exc_info=True` so the traceback surfaces at the default log level, and add a BaseException arm that logs at ERROR and re-raises so the thread exits as Python intends rather than dying silently. Watchdog/health-check fixes (bug NousResearch#3 in the upstream issue) are out of scope here — they overlap with the in-flight NousResearch#26734. Fixes NousResearch#32612

Address Copilot review on NousResearch#32616: - gateway/run.py: narrow `except BaseException` to `(SystemExit, KeyboardInterrupt, BaseExceptionGroup)` so we don't intercept GeneratorExit or other interpreter-shutdown signals while still capturing the cases the docstring describes. - tests/gateway/test_cron_ticker_error_visibility.py: drop unused `pytest` and `unittest.mock.patch` imports. All three existing tests continue to pass; the change is type-narrowing on the exception filter, not a behavior change on the cases the tests exercise (SystemExit + KeyboardInterrupt are still re-raised).

liuhao1024 · 2026-05-30T01:44:55Z

I found one issue that looks worth fixing before merge.

Unreachable BaseExceptionGroup clause — exception ordering bug

gateway/run.py:18575-18585 — The second except clause is dead code:

except Exception:
    logger.warning("Cron tick error", exc_info=True)
except (SystemExit, KeyboardInterrupt, BaseExceptionGroup):
    logger.error("Cron ticker fatal error; thread will exit", exc_info=True)
    raise

BaseExceptionGroup is a subclass of Exception (confirmed in Python 3.11+ docs), so the first except Exception always catches it. The second clause's BaseExceptionGroup entry is unreachable — any exception group raised by cron_tick() will be logged at WARNING and suppressed, not re-raised as the comment's intent ("thread will exit") suggests.

SystemExit and KeyboardInterrupt are handled correctly (they are direct BaseException subclasses, not Exception, so they fall through to the second clause).

Why it matters: If cron_tick() ever raises a BaseExceptionGroup containing fatal exceptions (e.g., wrapped SystemExit), the group gets silently swallowed instead of re-raised. The ticker thread continues running in an inconsistent state.

Suggested fix: Remove BaseExceptionGroup from the second clause (it's already caught by Exception and suppressing it is fine for a ticker loop), or if the intent is to re-raise it, swap the clause order:

# Option A: accept that Exception groups are non-fatal for the ticker
except Exception:
    logger.warning("Cron tick error", exc_info=True)
except (SystemExit, KeyboardInterrupt):
    logger.error("Cron ticker fatal error; thread will exit", exc_info=True)
    raise

# Option B: re-raise exception groups as fatal
except BaseExceptionGroup:
    logger.error("Cron ticker fatal error; thread will exit", exc_info=True)
    raise
except Exception:
    logger.warning("Cron tick error", exc_info=True)
except (SystemExit, KeyboardInterrupt):
    logger.error("Cron ticker fatal error; thread will exit", exc_info=True)
    raise

Option A is simpler and correct for a ticker loop — BaseExceptionGroup from cron_tick() is non-fatal and should be logged+suppressed like any other Exception. Option B preserves the original intent but adds complexity. I'd recommend Option A since exception groups in synchronous cron code are unlikely, and the ticker should be resilient to any non-fatal error.

The rest of the PR (DEBUG→WARNING upgrade, exc_info=True, test coverage) looks solid.

briandevans · 2026-06-07T23:15:09Z

Closing to focus the queue on security/file-safety work where civilian merges are landing. Happy to reopen if maintainers want this picked up.

Copilot AI review requested due to automatic review settings May 26, 2026 13:14

Copilot AI reviewed May 26, 2026

View reviewed changes

alt-glitch added type/bug Something isn't working P1 High — major feature broken, no workaround comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery labels May 26, 2026

This was referenced May 26, 2026

fix: keep cron ticker failures visible #32666

Closed

fix: cron ticker thread stops silently, jobs never fire #32895

Closed

fix(cron): surface and recover cron ticker failures #33849

Closed

briandevans force-pushed the fix/cron-ticker-error-visibility-32612 branch from 697fbb2 to 5892805 Compare May 28, 2026 21:15

briandevans added 2 commits May 29, 2026 18:12

briandevans force-pushed the fix/cron-ticker-error-visibility-32612 branch from 5892805 to 2eaac64 Compare May 30, 2026 01:12

banditburai mentioned this pull request May 31, 2026

fix(cron): watchdog supervisor + honest status for the silently-dying in-process ticker #35616

Closed

briandevans closed this Jun 7, 2026

Navigation Menu

Uh oh!

fix(gateway): make cron ticker errors visible at default log level - #32616

fix(gateway): make cron ticker errors visible at default log level#32616
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/cron-ticker-error-visibility-32612

briandevans commented May 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

briandevans commented May 26, 2026

Uh oh!

liuhao1024 commented May 30, 2026

Uh oh!

briandevans commented Jun 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		import pytest

Uh oh!

Conversation

briandevans commented May 26, 2026

What does this PR do?

Related Issue

Type of Change

Changes Made

How to Test

Checklist

Code

Documentation & Housekeeping

Related / Positioning

For New Skills

Screenshots / Logs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

briandevans commented May 26, 2026

Uh oh!

liuhao1024 commented May 30, 2026

Uh oh!

briandevans commented Jun 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants