Skip to content

fix(cron): stop multiplex ticker recreating archived profile homes - #94604

Closed
chelsealong wants to merge 2 commits into
NousResearch:mainfrom
chelsealong:fix/94590-multiplex-ticker-archived-profile
Closed

chelsealong wants to merge 2 commits into
NousResearch:mainfrom
chelsealong:fix/94590-multiplex-ticker-archived-profile

Conversation

@chelsealong

Copy link
Copy Markdown
Contributor

Fixes #94590.

Root cause

InProcessCronScheduler._start_multiplex() (cron/scheduler_provider.py)
ticks the profile_homes list it was handed once, when the ticker thread
starts (hermes_cli/web_server.py::_start_desktop_cron_ticker builds that
list from a single profiles_to_serve(multiplex=True) scan and never
refreshes it for the life of the desktop backend).

When an admin archives a profile — mv profiles/research profiles/_archived/research-... — while the ticker thread is already
running, the stale ("research", <path>) entry stays in that captured
list. Every subsequent tick still does:

with use_cron_store(home):
    record_ticker_heartbeat(success=ok)

record_ticker_heartbeat → _atomic_write_epoch → ensure_dirs() calls
store.cron_dir.mkdir(parents=True, exist_ok=True), where cron_dir = home / "cron". mkdir(parents=True) happily recreates home itself if it's
missing — so the very directory the admin just moved away reappears within
one tick interval (60s), containing only cron/ticker_heartbeat +
ticker_last_success, exactly as reported.

This hits all three places in _start_multiplex that walk profile_homes:
the initial recovery+heartbeat pass, the per-tick cron_tick loop, and the
post-tick heartbeat loop.

Fix

Before scoping to a profile's home in each of those three loops, skip the
entry if Path(home).is_dir() is now False. This is a cheap check (no
extra I/O beyond a stat) and self-heals on the very next tick once a
profile directory is gone — no ticker restart required. It does not touch
profiles_to_serve/directory scanning at all, since the recreation happens
downstream of that scan, in the ticker's per-iteration store access.

Test

Added test_multiplex_ticker_does_not_recreate_archived_profile in
tests/cron/test_scheduler_provider.py. It drives
InProcessCronScheduler._start_multiplex synchronously (no background
thread) via a fake stop_event whose wait() steps one loop iteration at a
time, so a profile's directory can be removed deterministically between
iteration 1 and iteration 2 with no race against a live ticker thread. It
then asserts the archived directory stays gone through two more iterations.

Confirmed the test fails without the fix and passes with it:

$ git checkout HEAD -- cron/scheduler_provider.py   # revert only the fix
$ python -m pytest tests/cron/test_scheduler_provider.py -q -k does_not_recreate
...
>       assert not p2.exists(), "archived profile directory must not be recreated by the ticker"
E       AssertionError: archived profile directory must not be recreated by the ticker
E       assert not True
E        +  where True = exists()
1 failed, 30 deselected in 0.24s

$ git checkout cron/scheduler_provider.py           # restore the fix
$ python -m pytest tests/cron/test_scheduler_provider.py -q
...............................                                          [100%]
31 passed in 1.25s

Also ran the wider tests/cron/ suite (939 passed, 4 pre-existing failures
unrelated to this change — confirmed identical on unmodified main:
test_media_send_timeout.py assertion-count mismatch and
test_script_claim_heartbeat.py's process-group kill guard, both artifacts
of the sandboxed process-group environment this was validated in rather
than this repo's own scripts/run_tests_parallel.py isolation).

ruff check cron/scheduler_provider.py tests/cron/test_scheduler_provider.py
passes clean.

AI assistance disclosure

This change was written by an autonomous coding agent (Claude), with the
root cause traced to a specific line (ensure_dirs()'s mkdir(parents=True, exist_ok=True)) and the fix/test verified locally as shown above before
pushing.

_start_multiplex() ticks a fixed profile_homes list captured once when
the ticker thread starts. When an admin archives a profile (moves its
directory out from under profiles/) while the ticker keeps running, the
stale entry's heartbeat write still calls ensure_dirs()'s
mkdir(parents=True, exist_ok=True), which silently recreates the
directory the admin just removed, within one tick interval (NousResearch#94590).

Skip an entry whose home no longer exists on disk before ticking or
heartbeating it, in all three loops that walk profile_homes. Self-heals
without a restart once the directory is gone.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/desktop Electron desktop app (apps/desktop/*) area/profiles Multi-profile isolation, HERMES_HOME scoping sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 25, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

This PR addresses a real bug (#94590): when an admin archives a profile by removing its directory while the multiplex cron ticker is running, the ticker's heartbeat path calls ensure_dirs() which does mkdir(parents=True, exist_ok=True), silently recreating the directory the admin just removed. The fix adds a Path(home).is_dir() guard at three tick points in _start_multiplex. The included test is thorough — it uses a deterministic _StepStopEvent to control loop iterations without threading races, archives a profile between iterations, and asserts the directory is not recreated.

Concern 1 — TOCTOU race on is_dir() check: At scheduler_provider.py:672, scheduler_provider.py:698, and scheduler_provider.py:723, the code checks Path(home).is_dir() and then proceeds to set_hermes_home_override, use_cron_store, and heartbeat writes. Between the check and the subsequent directory operations, another process could remove the directory. This is a narrow window and the existing ensure_dirs() would recreate it anyway, so the practical impact is minimal. However, the use_cron_store(home) context manager and set_hermes_home_override may still write state files into a directory that gets removed mid-operation. Consider wrapping the entire per-profile block in a try/except for FileNotFoundError to handle this gracefully.

Concern 2 — No cleanup of set_hermes_home_override on skip: When the is_dir() check fails and the code does continue, it skips the set_hermes_home_override / restore token pair entirely. This is correct behavior (no override is set, so no restore needed), but it means the loop variable home_token from a previous iteration remains in scope. In Python this is harmless, but it could be confusing for future maintainers. A comment noting that the token is intentionally not set when skipping would help.

Concern 3 — Test only covers one archive scenario: The test archives p2 once and verifies it stays gone. It would be valuable to also test the reverse: that re-creating the directory (un-archiving) causes the ticker to resume ticking that profile on the next cycle, since the docstring claims the approach "self-heals without a restart once the directory is gone." This would verify the self-healing claim.

The is_dir() check is cheap (single stat syscall) and the three insertion points cover all tick paths (initial recovery, main tick loop, and post-tick heartbeat). Good targeted fix with solid test coverage.

Adds test_multiplex_ticker_resumes_after_profile_unarchived per review
feedback on NousResearch#94604, covering the self-heal claim in
_start_multiplex's docstring: once an archived profile's directory
reappears, the next tick writes its heartbeat again.
@chelsealong

Copy link
Copy Markdown
Contributor Author

Added test_multiplex_ticker_resumes_after_profile_unarchived (per Concern 3) — archives research, then recreates its directory, and asserts the next tick writes a fresh ticker_heartbeat, verifying the self-heal claim. Left concerns 1 and 2 alone: the TOCTOU window is already narrow and self-heals on the next tick (matching the existing rationale), and the skip-path is already commented at each continue (scheduler_provider.py:673, 699, 724).

@teknium1

teknium1 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Thanks @chelsealong — correct diagnosis, and the _StepStopEvent-driven test was a nice way to avoid threading races. Reviewed against current origin/main: the same guard landed two days after this PR as 000d22b9db7 (2026-08-27, @helix4u): _existing_profile_homes() (cron/scheduler_provider.py:68-90) filters the snapshot on Path(home).is_dir() and is applied at all three tick points (:704, :729, :777), with test_existing_profile_homes_filters_deleted. Your PR predates the landed guard.

Closing as redundant. #94590 stays open for the maintainer to reconcile against that commit.

@teknium1 teknium1 closed this Sep 2, 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 comp/cron Cron scheduler and job management comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Hermes Desktop] multiplex cron ticker recreates archived profiles via profiles_to_serve scan

4 participants