fix(cron): desktop ticker defers to a live gateway instead of racing it - #67418
fix(cron): desktop ticker defers to a live gateway instead of racing it#67418Sora-bluesky wants to merge 3 commits into
Conversation
|
Good flag — comparing the four approaches for the maintainers, factually:
All four fix the same race; the choice is genuinely the maintainers' (ownership model vs. liveness gate). If one of the earlier PRs is preferred, happy to close this one — I'd only suggest carrying over the per-tick auto-resume property, whichever lands. Also noting for triage: #66629 is effectively the same underlying race as #52202 / #43965. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the desktop/gateway delivery split. The underlying race is present on current main: the desktop starts a provider without adapters at hermes_cli/web_server.py:141-159, while the gateway starts one with live adapters at gateway/run.py:21659-21682.
Problems
- The new gate at
hermes_cli/web_server.py:191is a liveness observation, not an ownership claim. The built-in provider checks it beforetick()(cron/scheduler_provider.py:195), andtick()checks it again only after taking.tick.lock(cron/scheduler.py:3911). A gateway can become live after that final check and before desktop execution claims the due work, so the degraded desktop delivery path remains possible during the handoff.
Suggested changes
- Establish a shared atomic scheduler-owner/claim protocol for the desktop and gateway paths, and add a deterministic gateway-start-during-dispatch test.
Automated hermes-sweeper review.
| # CronScheduler ABC — only pass it where the signature accepts it. | ||
| try: | ||
| if "can_dispatch" in inspect.signature(provider.start).parameters: | ||
| kwargs["can_dispatch"] = _no_live_gateway |
There was a problem hiding this comment.
This supplies only a non-atomic liveness observation. The built-in provider checks it before tick(), then tick() checks it again after .tick.lock is acquired; a gateway can become live after that final read and before desktop execution claims a due job. Please use a shared ownership/claim protocol and cover that transition deterministically.
|
You're right that the gate is a liveness observation, not an ownership claim — a gateway starting inside the check→dispatch window can still take delivery quality for that one handoff tick. That residual TOCTOU is inherent to any liveness-gate approach; closing it fully requires the atomic owner/claim protocol, which is exactly #44049's design, and I'd rather not duplicate that surface in a fourth PR. As laid out in the comparison above: this PR is the minimal harm-reduction option (shrinks the degraded-delivery window from "every tick" to "at most one handoff tick per gateway start") with per-tick auto-resume; #44049 is the complete-fix option. If the maintainers pick the ownership model, happy to close this in its favor — ideally carrying the auto-resume property over. |
ca5d836 to
185cd62
Compare
|
Thanks for the detailed trace. The gate is evaluated inside The residual you're pointing at is a gateway going live during |
df14721 to
5a8cbec
Compare
|
Redesigned around your ownership-vs-liveness point. The gate now probes gateway.lock directly, read-only, instead of the cached PID: held -> defer, free -> dispatch, unknown -> a read-only PID identity fallback that requires a matching start_time, so a recycled PID belonging to a different HERMES_HOME's gateway can't false-positive. It opens the lock "r+" (no O_CREAT), so unlike the shared is_gateway_runtime_lock_active it never creates or unlinks the file. Dropped the startup drain barrier from the earlier attempt: .tick.lock + advance_next_run already dedupe a valid occurrence under the lock, so it was redundant, and its "provable ordering" claim was false on a drain timeout anyway. Decision-table and probe-classifier tests included; the two residuals (a hand-off micro-race, and the blind fail-open biased to cron liveness) are written up in the commit body. |
27bf919 to
0c8e715
Compare
0c8e715 to
38a9275
Compare
SummaryNine PRs are associated with this issue complex: six change Desktop-versus-Gateway cron arbitration, while #67135, #67198, and #67204 add scheduler-provider tests without changing ownership or delivery behavior. The fix diffs range from obsolete direct-ticker guards and lifecycle suppression to current-provider deferral; the recorded cross-issue best-fix verdict selects #44050 for #43965, #52202, and #66629. Related pull requests
Duplicates#44049 and #44050 implement substantially the same runtime-lock-backed scheduler deferral. #46207 and #47358 are obsolete direct-ticker variants; the ownership portions of #52259 and #67418 address the same race through broader lifecycle, timeout, or status-probe changes. #67135, #67198, and #67204 are independent test PRs, not ownership-fix duplicates. Suggested consolidationAuthor action: rebase #44050 onto current main, retaining its built-in-only provider integration and regression coverage; this preserves the recorded best-fix verdict without making a merge recommendation. Then close #44049, #46207, and #47358 as duplicates of #44050; for #52259, split out the standalone-send timeout before closing its ownership portion as duplicate, and for #67418, split any independently justified status/profile hardening before closing its ownership gate as duplicate. These closures explicitly differ from the keep_open reviews on #46207, #47358, #52259, and #67418 because the first two patch the obsolete direct-ticker path, while the latter two combine the same ownership fix with broader salvageable work. Keep #67135 and #67198 in their separate scheduler-test triage, and request the documented comment correction on #67204. Complex graphflowchart TD
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I43965(["issue #43965 (open)"])
I52202(["issue #52202 (open)"])
I66629(["issue #66629 (open)"])
subgraph Dup44049 ["PRs duplicating each other"]
P44049["PR #44049 (open)"]
P44050["PR #44050 (open)"]
P46207["PR #46207 (open)"]
P47358["PR #47358 (open)"]
P52259["PR #52259 (open)"]
P67418["PR #67418 (open)"]
end
P67418 -.->|partial| I43965
P67418 -.->|partial| I52202
P67418 -.->|partial| I66629
class I43965 open
class I52202 open
class I66629 open
class P44049 open
class P44050 open
class P46207 open
class P47358 open
class P52259 open
class P67418 open
class P44050 best
class P44050 best
class P44050 best
class P67418 target
click I43965 "https://github.com/NousResearch/hermes-agent/issues/43965"
click I52202 "https://github.com/NousResearch/hermes-agent/issues/52202"
click I66629 "https://github.com/NousResearch/hermes-agent/issues/66629"
click P44049 "https://github.com/NousResearch/hermes-agent/pull/44049"
click P44050 "https://github.com/NousResearch/hermes-agent/pull/44050"
click P46207 "https://github.com/NousResearch/hermes-agent/pull/46207"
click P47358 "https://github.com/NousResearch/hermes-agent/pull/47358"
click P52259 "https://github.com/NousResearch/hermes-agent/pull/52259"
click P67418 "https://github.com/NousResearch/hermes-agent/pull/67418"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 9 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 114 kB of PR diffs, 42 kB of issue/PR text, 15 kB of discussion (22 comments), 25 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Additional repro on Linux + SSH mode: the race causes jobs to fail, not just duplicate We hit this on Linux with Hermes Desktop in SSH mode, where the symptom is worse than duplicate delivery. Worth capturing here since the existing reports (#57191, #66629) focus on Windows duplicates and Feishu cards. Setup: Symptom: cron jobs fail non-deterministically with Why it fails rather than duplicates: the two tickers do not share an environment. A probe run from inside an actual cron job returned: {"ppid": 2266888,
"parent_cmdline": "... hermes serve --isolated ... --ssh-session-token-file ...",
"path": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin",
"hermes_home": null,
"which": {"yt-dlp": null, "node": null, "simplex-chat": null}}The gateway's own environment has Diagnostic trap worth noting: On the code comment at # Desktop-spawned backends (HERMES_DESKTOP=1) fire cron jobs themselves,
# since the app has no gateway running the scheduler.
if os.getenv("HERMES_DESKTOP") == "1":That assumption does not hold in SSH mode — the Desktop connects to a host where the gateway is running, and the ticker starts unconditionally anyway. This PR's "defer to a live gateway" approach fixes our case too. Workaround in the meantime: put tools that jobs invoke by bare name on the system |
|
@boblin Thanks for this — the SSH-mode repro is the first report showing the race producing failures rather than duplicates, and the ppid diagnostic trap you describe (gateway's One clarification so the thread stays accurate for the maintainers: your case is covered by the per-tick deferral approach generally, not by this PR uniquely. I checked #44050 (the fix the 2026-08-03 triage above records as "best fix") against your scenario:
So your report confirms the deferral approach holds up under SSH mode, but it doesn't by itself pick between the open candidates. I should also correct something I wrote on this thread back in July: I described some of the other PRs as startup-time guards. That was wrong. #46207 rechecks gateway liveness on every ticker iteration, and #52259's head adds a per-tick Two factual differences that do separate them, for whoever decides:
The triage's suggestion — land one ownership fix, split anything independently useful — still seems right to me, and which gate lands is the maintainers' call. If #44050 is preferred, the read-only probe here is the piece I'd suggest carrying over. |
38a9275 to
8504984
Compare
When `hermes gateway run` and `hermes serve` share a HERMES_HOME, the desktop cron ticker's standalone delivery path has no live platform adapters, so Feishu interactive cards silently degrade to plain text (NousResearch#66629). Add a read-only 3-state probe of the gateway runtime lock and wire the desktop ticker through the existing `can_dispatch` seam so it dispatches only when no live gateway owns this HERMES_HOME, and resumes on its own once the gateway stops. The probe never opens with O_CREAT, never unlinks, and never writes to the lock file, so a frequent caller cannot mutate the gateway's own lock. `held`/`free`/`unknown` map lock contention (measured errno on Windows = EACCES; EAGAIN on POSIX) apart from other OS errors — an unsupported filesystem or a stat failure reports `unknown` and the gate fails open with a warning, so a desktop-only cron never stalls on a probe error. The residual race — a sub-second window at gateway startup where an occurrence due at that instant can be delivered by the desktop once — is bounded by `advance_next_run` under `.tick.lock`, so no double delivery is possible. Serializing gateway startup on `.tick.lock` was considered and rejected: making startup fail from the desktop's lock would be worse than the original bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An earlier version of this branch had the desktop gate briefly acquire and release the same exclusive OS lock the gateway itself takes at startup, to infer held vs free. A microsecond-scale collision could make acquire_gateway_runtime_lock() return False and the gateway exit with "already held by another instance". The observer would kill the owner. A bounded retry inside acquire mitigated typical schedulings but did not close the race: under scheduler pressure a probe thread can be descheduled and hold the lock arbitrarily long, and any time-based retry window only moves the goalpost. Read the JSON lock record instead. probe_gateway_runtime_lock() never touches the OS lock. It classifies ownership from the record's pid + start_time + argv using the same 4-step verification the dashboard's get_running_pid already runs: pid alive, start_time matches, and _record_matches_live_gateway_pid confirms the process is a gateway. Any of those failing returns "free" so the desktop takes over. An empty or unparseable record (a crashed mid-write, or the sub-ms startup window between _try_acquire_file_lock and _write_gateway_lock_record) returns "unknown" and the gate fails open with a warning, so a desktop-only cron never stalls. Because the probe never contends for the OS lock, the concurrent gateway acquire cannot spuriously see contention. Observer-cannot- kill-owner is now a construction property, not a timing property, so the retry inside acquire is not needed and has been dropped. Tests: an OS-lock-forbidden probe test (monkeypatches _try_acquire_file_lock, msvcrt.locking and fcntl.flock to pytest.fail) pins the by-construction guarantee. Per-classification tests cover stale record from a dead pid, pid reuse (start_time mismatch), live pid that is not a gateway (fingerprint reject), empty/unparseable record, in-process fast path, and the Path.exists() OSError case. The headline behavioral test drives _start_desktop_cron_ticker with a record standing in for a live gateway and asserts it stays quiet then resumes on record removal. Adjacent defects intentionally not in scope for this PR: - is_gateway_runtime_lock_active() still opens with "a+" and unlinks on PermissionError. It is called only from hermes doctor and from get_running_pid inside the gateway's own startup, so no high-frequency observer amplifies the same race. Kept for a focused follow-up. - The standalone delivery path silently degrades interactive cards even when no gateway is up (desktop-only user). The gate does not help that case; a warning on the standalone card path is a follow-up. Fixes NousResearch#66629 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…top cron probe
The desktop cron probe could answer "held" from a stale lock record and
stall the losing profile's cron. A recorded pid and start_time that collided
with a live process, plus a recorded hermes_home that still named the probed
profile, was trusted even when the live process served a different profile,
because _command_line_belongs_to_profile matched the profile by substring
("--profile work" is contained in "--profile worker").
Make the probe's ownership positive and profile-exact:
- Require the record's hermes_home to be present and to match this lock's
home with the host's path and case semantics (_same_hermes_home). A record
that omits it reads as unknown (fail open, dispatch plus a warning).
- Match the live command line to a profile by exact argv token (--profile
and -p, space and = forms, or a normalized HERMES_HOME= token), with an
explicit --profile/-p authoritative over a conflicting HERMES_HOME=. This
is the root fix. It also corrects get_runtime_status_running_pid and a
cross-platform bug where a forward-slash HERMES_HOME= never matched on a
backslash host.
- Read the lock record recursion-safe: a deeply-nested corrupt record makes
json.loads raise RecursionError, a RuntimeError subclass that escaped the
reader's except and crashed the probe.
Every branch is covered by a fail-before test verified via git stash.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8504984 to
f435c87
Compare
What
When
hermes gateway runandhermes serve(Hermes Desktop) share aHERMES_HOME, the desktop cron ticker's standalone delivery path has no live platform adapters. So when serve wins the tick race, Feishu interactive cards are silently downgraded to plain text, with no error and no warning (#66629).This gates the desktop ticker on a read-only, record-based owner probe of the gateway runtime lock, through the
can_dispatchseam thatcron.scheduler.tick()already re-checks under.tick.lock. Desktop dispatches only when no live gateway owns thisHERMES_HOME, and resumes on its own once the gateway stops.Why read-only (observer must not kill owner)
The probe never opens the OS lock. It classifies ownership from the JSON lock record and a live-PID check, so a frequent desktop-side probe cannot serialize against
acquire_gateway_runtime_lock()and cannot make gateway startup lose its own lock. This is a property of the construction: the probe never touches the lock, so there is no timing window in which a collision could occur. An earlier attempt in this branch used an OS-lock probe with an acquire retry to paper over the collision, but a bounded retry cannot survive a probe descheduled by Defender, a debugger, or OS pressure. This design removes the mechanism instead.How
gateway/status.py:probe_gateway_runtime_lock() -> "held" | "free" | "unknown"establishes ownership only from positive evidence, in order:unknown(crashed mid-write, or the sub-millisecond window between_try_acquire_file_lockand the record write)_pid_exists(pid), elsefree(crash: OS released the lock, record stale)start_timepresent on BOTH the record and the live process, elseunknownrecorded start_time == live start_time, elsefree(PID reuse)record["hermes_home"]is a usable string and matches the probed lock's parent, compared with the host's path + case semantics (_same_hermes_home), elseunknown/freefreehermes_cli/web_server.py:_no_live_gateway()dispatches onfree/unknownand defers onheld. Anunknownresult logs a warning so a hidden probe failure stays visible. It is wired via aninspect.signatureguard so providers withoutcan_dispatchstill work.Exact-profile matching (the load-bearing correctness fix)
Step 6 previously used
_command_line_belongs_to_profile(), which matched the profile by substring (--profile work⊂--profile worker). Combined with a stale record whose honesthermes_homestill namedworkafter PID reuse plus a start-time collision, the probe could reportworkrunning when only aworkergateway was live, which stalledwork's cron. This rewrites the matcher to compare an exact argv token (--profile/-p, space and=forms, or a normalizedHERMES_HOME=token), with the explicit--profile/-pselector authoritative over a conflictingHERMES_HOME=. This is the root fix and also corrects every other caller of the matcher (get_runtime_status_running_pid); it incidentally fixes a cross-platform bug where a forward-slashHERMES_HOME=never matched on a backslash host.Scope and known residuals
advance_next_rununder.tick.lockguarantees at-most-once, so no double delivery is possible on anunknownfail-open.HERMES_HOMEcomes from the environment (invisible on argv), so a PID-reuse plus start-time collision can only be disambiguated by the self-assertedrecorded_home. That case is collision-only and already present inget_running_pid.hermes_homewas persisted reads asunknownuntil that gateway restarts (fail-open: dispatch + warning).is_gateway_runtime_lock_active()is intentionally left as a separate follow-up: it opens with"a+"and unlinks onPermissionError, so it is unsuitable for a hot-path probe, but it is authoritative for lifecycle commands and is not on the desktop cron path. Redesigning that observer/owner split is a distinct change.Test
NO_COLOR=1 uv run --extra dev python -m pytest tests/cron/test_desktop_cron_gateway_gate.py tests/gateway/test_status.py -q_start_desktop_cron_ticker: the ungated ticker fires while a live gateway is recorded; the gated one stays quiet and resumes when the record clears.RecursionErroron a deeply-nested corrupt record, cross-profilehermes_home, platform-aware home comparison, and the exact-token matcher (workmust not match--profile worker;--profileauthoritative overHERMES_HOME=; the sibling-profile + start-time-collision repro returnsfree).git stash, GREEN after. Full local run: green on Windows (pre-existing unix-permission / POSIX-path baseline failures excluded).