A hand-edited "completed": "2" still crashed every recorded run ("2" + 1),
and 1.0 was stored as 2.0 ("2.0/3"). load_jobs now coerces any non-int
counter to a non-negative int (0 when unparseable). Document the load-time
repair next to the direct-edit tip.
Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
42d76f5611 moved the repair-detail warnings to the locked pass so an
unlocked load_jobs (which re-runs itself under the lock) no longer logs
them twice, but nothing pinned it: reverting that hunk left every test
green. The folded junk-entries test already builds the list-junk and
scalar 'jobs' shapes through an unlocked load_jobs, so count the records
there: one detail warning and one "Auto-repaired" line per shape. Reverting
42d76f5611's cron/jobs.py hunk (scalar warning doubles) or re-emitting the
notes before the locked re-dispatch (list-junk warning doubles) now fails.
The raw-value check moves into the loop so it covers the list case after
caplog.clear().
The stack added three test functions and the budget is two. The race test
and the junk-entry test use the same fixture: create a job, then append
junk to jobs.json. Run the race on the junk test's first unlocked
list_jobs repair instead. It races a name update, which does not change
due-ness, and asserts the update survives on disk. The due-scan and
scalar-shape assertions are unchanged. It still fails if the locked
re-dispatch is removed from load_jobs.
A hand-edited "completed": null was patched with `.get("completed") or 0`
at five arithmetic readers. The display readers were still missed: the
cronjob tool listed a never-run one-shot as "1/1", and `hermes cron`
printed "None/3". Any new reader would bring the bug back.
Every reader gets its jobs through load_jobs: mark_job_run, update_job,
claim_dispatch, the due scan, merge_job_definition, list_jobs/get_job for
cronjob_job_args._repeat_display and hermes_cli/cron._job_rows. So reset a
null to 0 once in the load_jobs repair pass, which also persists the fix,
and change the per-site `or 0` patches back to their base form.
Co-authored-by: mochamgx <1114149@qq.com>
Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
Since load_jobs re-runs itself under the job-store lock whenever it finds
a repair, every detection-time warning ran twice: once on the unlocked
pass and again on the locked re-read. The id-keyed "Skipping N non-dict
entries" warning had logged once on base, so this was a new regression.
The scalar 'jobs' warning doubled too. Only the list-junk warning had a
lock-depth guard.
Collect the repair details as notes and emit them next to the existing
"Auto-repaired jobs.json" line, which only the locked pass reaches. With
that, the per-warning depth guard is no longer needed. Also build the
junk list once instead of scanning jobs three times; the
isinstance(jobs, list) check was always true at that point.
Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
#122114 guarded mark_job_run/claim paths against a hand-edited
"completed": null, but update_job and the job_definition merge still used
.get("completed", 0), which returns None when the key is present, so the
null was carried forward into the store. Use `or 0` there too, and pin the
behaviour with one test covering mark_job_run and update_job.
Fixes#123281
Co-authored-by: John Paul Soliva <soliva.johnpaul@icloud.com>
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
Co-authored-by: mochamgx <1114149@qq.com>
`repeat.get("completed", 0)` only falls back to 0 when the KEY is
absent. When the key exists with value None (a job created but never
successfully recorded a run), it returns None, and the following
`completed += 1` raises:
unsupported operand type(s) for +=: 'NoneType' and 'int'
Effect: the run is never recorded (last_run_at/last_status stay null)
and every fire logs a scheduling error. The job itself executes fine.
Same latent hazard at the `completed < times` comparison, which a None
value would also fail on.
Fix all three sibling sites with `repeat.get("completed") or 0`.
(cherry picked from commit 0c5024de901900a747849ca44750327b56477f3b)
The salvaged PR added four tests (one parametrized) for the load_jobs
boundary. Keep the stack to its invariant-test budget: the all-junk list and
scalar 'jobs' field cases now run inside the junk-entry test, alongside the
separate concurrent-update race test.
An unlocked load_jobs that finds junk entries hands off to a locked re-read,
which runs the same analysis again, so every repair logged the "Skipping N
non-object entries" warning twice. Log it only on the locked pass (the one
that actually saves the repair).
An unlocked reader (list_jobs) that found a repairable store saved its parse-time snapshot. The shrink-merge restores only missing ids, so a locked writer's update to a job already in that snapshot (enabled, next_run_at, run claims) was reverted. Outside the lock, the repair now re-reads and saves under _jobs_lock(); this covers every repair kind, including the pre-existing bare-list, id-map and control-character repairs.
(cherry picked from commit 0f90a10212dbc360007a59bb09acc1e0ce676f61)
{"jobs": null} (or a string, number or bool) escaped load_jobs unchanged, so every reader crashed the same way the non-object list entries did. Replace it with an empty list and persist the repair. Ported from #123405.
(cherry picked from commit 1f380ec62c2ce65fd1d31656850b611234e3385e)
An all-invalid list filtered to [] skipped save_jobs (the 'if jobs and repair' guard), so every tick repeated the warning. Save whenever a repair ran (the shrink-merge still keeps concurrent valid jobs), and log the dropped entries' types instead of their raw values.
(cherry picked from commit 3b5b1aa61332cf79554fc2a24ae925908c37fe5d)
A null, string or number in the canonical {"jobs": [...]} list reached
every reader: the due scan raised AttributeError on each tick, so no job
fired, and list/resolve crashed too. Drop the junk with a warning and
self-heal the file, as the id-keyed map flatten already does.
(cherry picked from commit 2e2438d160967392184b2193cfc2e2ca124c2b7a)
Round-4 pre-arm review warnings (ssh-only gaps, no local/docker change):
- A named ssh profile's cwd chain falls back to "~" before the launch
profile's host cwd, so a fresh or resumed TUI/dashboard session never
runs the remote shell in a host path.
- An ssh launch profile keeps a "~"/"~/x" cwd remote in session.create /
_completion_cwd (before main's host fast path), matching session.cwd.set
and workspace.move.
- One _is_remote_cwd_shape (reusing _is_ssh_remote_tilde_cwd) gates every
remote path: session.create no longer marks a relative remote path
explicit, and _completion_cwd returns the profile's own cwd (or ~) for
one, matching _workspace_cwd's rejection.
Live A/B over 6 launch modes x 7 profiles x 7 cwd shapes: every local,
docker and backendless cell is identical to origin/main.
Round-3 pre-arm gate findings:
- _completion_cwd falls back to the raw path for an ssh LAUNCH profile
after main's host fast path, so session.create with a remote-only
explicit cwd no longer marks the gateway's own os.getcwd() as the ssh
session's workspace (and eagerly persists it). workspace.move and
_set_session_cwd already treated launch ssh as remote.
- A deleted profile only fails _completion_cwd where main consulted it
(non-explicit client cwd, or no client/session cwd); one resolve.
- A named ssh profile with no terminal.cwd gets "~", never the launch
profile's TERMINAL_CWD / terminal.cwd host path.
- Heal/reconcile: only a named ssh profile is special-cased; every other
session keeps main's env check, so named docker/local sessions heal
exactly as on main.
- _workspace_cwd rejects a relative remote path (it was stored and
git-probed relative to the gateway's cwd).
- Reconcile checks the free explicit-cwd early return before resolving
the backend.
Round-2 pre-arm gate findings:
- A named ssh profile's cwd is checked before any host expansion:
_profile_workspace_cwd tries the declared remote cwd first, and
_completion_cwd returns an ssh-bound path raw before expanduser/isdir.
A `terminal.cwd: ~` (or ~/x) no longer resolves to THIS host's home
and gets pinned as the remote workspace.
- _terminal_task_cwd_with_source switches to ssh only for a named ssh
profile; other named backends keep main's process-backend branch, so a
named docker profile under a local launch keeps its "session" cwd
source (docker isolation mounts from it).
- Launch-profile heal/reconcile keeps main's env check and only adds the
config-says-ssh case, so a docker-in-config launch still heals dead
host worktrees as on main.
- _completion_cwd does not fail an explicit client cwd for a deleted
profile (main never resolved the profile on that path).
- One _workspace_cwd(profile_home, raw) validates a picked workspace for
both _set_session_cwd and session.workspace.move.
- The profile-policy helpers live beside their callers in
session_workdir; reuse hermes_cli.config._is_ssh_remote_tilde_cwd.
- session.create resolves the backend only when a cwd was sent.
- Tests write a real profile config.yaml instead of stubbing the backend
helper; pin the "~" case.
Pre-arm gate findings on the salvage stack:
- The "cwd lives on another host" exemption is ssh-only (_cwd_is_remote).
Docker and the other backends mount or copy HOST paths: _set_session_cwd
keeps the host isdir check and always calls cleanup_vm, so a docker
session moving workspaces gets a fresh container with the new mount
again (the non-local early return skipped it).
- _ensure_session_db_row no longer force-writes the row cwd. It runs on
every prompt, and each update_session_cwd bumps git_metadata_generation,
which made an in-flight git-meta probe fail to publish. The eager row at
session.create already lands the cwd before the agent's INSERT-OR-IGNORE.
- A named profile's backend and remote cwd come from the policy its turns
actually run under (tools/terminal_scope.build_profile_terminal_scope:
defaults <- .env <- config.yaml), so a .env-only TERMINAL_ENV=ssh
counts; _profile_configured_cwd is back to main's body.
- An ssh profile's own terminal.cwd is also used when the client sends no
cwd (_profile_workspace_cwd, shared with resume).
- session.workspace.move takes the backend from the live session's
profile (the same one _set_session_cwd uses) and validates once.
- _hydrate_session_cwd resolves the backend outside _sessions_lock.
- _completion_cwd keeps main's host fast path; the backend is only
resolved when the path is not a host dir.
- Delete the now-dead _is_local_terminal_backend; the cwd-follow fixture
patches _effective_terminal_backend instead (its old patch was a no-op).
- Trim three tests that re-asserted main's local behaviour or duplicated
the real-config tests.
- Parametrize the ssh-profile cwd test over the launch backend: the
desktop usually launches local, which is where the display heal
rewrote /home/kali to /home.
- A named profile with no terminal.backend stays local under an ssh
launch.
- Drop three env-level duplicates of the bound-profile tests.
- Fix the workspace.move fixture for _profile_db(writer=True) on main.
Follow-up to the #105749 + #123903 salvage:
- A named profile without terminal.backend is local. Both contributor
helpers fell back to the LAUNCH profile's backend, so a local profile
opened from an ssh launch had its terminal.cwd treated as remote.
- _bound_terminal_backend() is the single resolver (create, completion,
workspace.move, display heal, settle-follow, terminal tool). The
terminal tool now reads it too, so an ssh profile under a local launch
keeps /home/kali instead of the display heal persisting /home.
- _declared_remote_profile_cwd() only honours a profile that itself
declares backend: ssh; the placeholder set is gone (the ~/absolute
shape check already rejects ".", "auto", "cwd").
- One loader for a named profile's terminal section
(_profile_terminal_cfg), shared with _profile_configured_cwd.
- Eager row at session.create and "row cwd = explicit" on hydrate apply
to remote sessions only; local project drafts stay lazy and keep
settle-following, as on main.
- config.get project forwards the pinned profile (and marks a picked
path explicit) so the desktop gets the remote dir back; the renderer
keeps adopting the server's normalized cwd for local users (WSL
translation, abspath) instead of bypassing it.
- Dropped #123903's cwd_explicit-decides-intent change in session.create:
it made every local desktop new chat in a project lose its workspace
and AGENTS.md.
The app showed the profile's terminal.cwd, but SSH sessions still ran in the
launch profile's directory because a remote path that does not exist on the
desktop host was discarded.
(cherry picked from commit e4f4a43ebc4408adb6ac37e8de1ee7ff414a9158)
The project-overview + set the new-chat workspace target correctly, then adopted
config.get {key:project}'s returned cwd. For a remote/ssh project the dir does not
exist on the gateway host, so the server normalizes it to the launch cwd
(/opt/hermes), overwriting the target so the session opened there. An explicitly
chosen project path is authoritative -- keep it, adopt the server cwd only for the
path-less fallback.
(cherry picked from commit 70eea2f6de97d0d8ee08cd5510ca353fa13d4337)
A multiplexed gateway serves many profiles from one process; at session.create
HERMES_HOME is not yet rebound to the target profile, so the process-global
backend check reads the launch profile (usually local) for a session bound to an
ssh/docker profile. The local isdir gate then drops the session's remote project
cwd, and the sidebar/terminal fall back to Home / the profile's ~ dir. Read the
BOUND profile's terminal.backend and, when non-local, trust the remote path raw
across the whole cwd path:
- _completion_cwd / session.create explicit_cwd / _set_session_cwd / workspace-move
and a session-aware _session_is_local_backend (no launch-process backend reads);
- don't heal a live remote cwd down to /home (env-OR-config backend check);
- persist a project session's row eagerly with its cwd, and force the cwd on after
the AIAgent INSERT-OR-IGNORE, so the sidebar keeps it out of Home;
- mark a hydrated row cwd as explicit so the remote terminal uses it, not ~.
(cherry picked from commit 35511526c5633e28e275f231e1735c2394f6afec)
_safe_restore_db now also returns False when the snapshot copy fails its
SQLite integrity check, but restore_quick_snapshot still logged every False
as "live-safe restore refused", pointing users at running processes when
the real cause was a corrupt snapshot. Make the log line and comment
neutral and defer to the preceding detailed log line from
_safe_restore_db; no second integrity check is added on this path.
f55f937cee made run_import tell the user when an archived database fails
its integrity check instead of blaming a live holder, but nothing asserted
that message: reverting its prod hunk left the suite green. Extend the
existing corrupt-source test to require "failed its integrity check" in
the import output, so the misleading holder message cannot come back.
Verified: 24 pass on head; 9 fail with f55f937cee's prod hunk reverted.
The stack added four test functions against a budget of two, and two of
them duplicated coverage: the _count_session_rows test is exercised by the
import case's "3 session(s) / 3 message(s) -> 1 / 1" assertion, and the
valid-source test repeats the held/healthy restores already covered by the
literal-path test and tests/hermes_cli/test_backup.py.
Keep the literal-path and corrupt-source tests in one file, sharing a home
fixture parametrized over 'x#y', 'x%23y' and 'x y' (the names an unescaped
file: URI truncates or decodes to a different path), and assert no decoy
entry appears next to the home. Mutation-checked: reverting the URI
escaping turns the literal-path test red; removing the integrity gate turns
every corrupt-source case red.
Co-authored-by: liuzikaii <2319582736@qq.com>
The integrity gate added to _safe_restore_db makes it return False for a
corrupt archive member, but _import_db_member still reported every False as
a live-holder refusal ("Stop the gateway/dashboard processes ..."), sending
users after the wrong fix while the real cause only reached logger.error.
On failure, re-run the bounded integrity check on the extracted temp file
and raise a message naming the actual cause; the holder message is kept for
genuine refusals. The check runs only on the failure path, so successful
imports pay nothing extra. Also document the new False case in the
_safe_restore_db docstring.
Co-authored-by: liuzikaii <2319582736@qq.com>
The three percent-encoded mode=ro connects added earlier in this stack
re-implemented hermes_state_holders.read_only_db_uri, which already exists
for exactly this '#'/'?' truncation bug and is the canonical builder used by
hermes_state and doctor_state. Using it keeps a single definition of the
encoding so a future fix lands everywhere. Behaviour is unchanged
(timeout=1.0 kept at the backup.py site).
The source-integrity gate is only as good as the URI it opens. Run the
existing restore invariants with HERMES_HOME under 'x#y' so a regression
to an unescaped file: URI in _query_ro_sqlite (which validates an empty
pre-'#' decoy and lets the corrupt source through) fails here.
verify_sqlite_integrity() (now the source gate in _safe_restore_db) opens
the file through _query_ro_sqlite with a raw f"file:{path}?mode=ro" URI.
Under a path containing '#', SQLite treats the rest as a fragment: the
?mode=ro is dropped, the pre-'#' prefix is opened read-write (and created
empty), and the integrity check passes a corrupt source. Use
Path.as_uri(), matching the other restore-flow sites.
Salvaged from PR #123374.
Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
The fold that stops post-handoff waiter failures from raising a false
"dispatch failed" incident/ping had no regression coverage. Drive
run_one_job through a real _wait_for_external_cron_worker whose body raises
and assert: no cron incident, no delivery, one failed mark_job_run carrying
the post-handoff label, and the execution row terminalized as failed.
Red with the post-handoff routing reverted (an incident is opened).
run_one_job's dispatch-failure handler already routes an
_ExternalWorkerPostHandoffError through bookkeeping only (no incident, no
ping), but the recorded error still read "Restart-safe cron worker dispatch
failed", which is misleading in last_error / executions.db for a worker that
was spawned and may have run. Compute post_handoff first and label it
"Restart-safe cron worker failed after handoff: ..."; the pre-handoff prefix
is unchanged.
_launch_external_cron_worker returns through _wait_for_external_cron_worker
once the worker is spawned/acknowledged, and that waiter can raise (e.g. the
adopted worker died and recovery could not terminalize the row). Such an
error landed in the same run_one_job except branch as a genuine dispatch
failure, so a job that actually ran got a false "Restart-safe cron worker
dispatch failed" incident and failure ping on top of whatever the worker
itself delivered.
Wrap waiter failures in a dedicated _ExternalWorkerPostHandoffError and skip
_deliver_crash_failure for it, keeping the base bookkeeping-only behaviour
(mark_job_run + finish_execution, the latter a no-op on a worker-owned row).
Pre-handoff dispatch failures still open an incident and notify.
The dispatch-failure branch called _deliver_crash_failure before the
try/finally that closes the execution row. If delivery ever raised,
finish_execution was skipped and the row stayed claimed until dead-owner
recovery. Run delivery inside the try so the pre-existing finally still
terminalizes the attempt; delivery_error/outcome default to None when it
raises.
_deliver_crash_failure is already best-effort internally (incident upsert,
_deliver_result and _mark_incident_alerted each swallow their own errors),
and the sibling crash path in run_one_job calls it without a wrapper. The
extra try/except was a defensive layer that could only mask a genuine bug
in the notice helper; drop it so both call sites match. The existing
try/finally still guarantees finish_execution runs after mark_job_run.
webtecnica submitted an equivalent independent fix in #123433.
Fixes#123401
Co-authored-by: webtecnica <webtecnica@gmail.com>
A failed restart-safe handoff in run_one_job() recorded the failure on the
job and in the executions ledger, then returned before any incident or
delivery path ran: no cron_incidents row, no failure-lane notice. Route the
dispatch-failure branch through _deliver_crash_failure() so it opens the
same job+signature incident and delivers the same failure notice as any
other job failure, with the existing alerted-cooldown withholding repeats.
A notice-path exception no longer loses the bookkeeping: mark_job_run and
finish_execution still run with a "failed" delivery outcome (#123401).
(cherry picked from commit e5b5969df1b7ca212c5a6e27d30f4778fb835c52)
The comment claimed the notice goes out immediately. In production
will_retry runs before mark_job_run, so the first failure (5m rung beats
the 10m run) is held; only the attempt-1 failure asserted here yields.
plan_retry's exhausted-ladder warning re-listed _ladder_instant's gates
(recurring, not paused, retry enabled) by hand, so the two could drift and
the warning fire for a job the ladder never applied to. Both now call
_ladder_applies(job). _ladder_instant checks the attempt count first, so the
exhausted path loads config exactly once (in the warning gate).
Behaviour unchanged: old-vs-new equivalence over 1,728 job states
(will_retry, plan_retry result, mutated job, log levels) shows 0 diffs.
WHY: the ten-line docstring restated the commit narrative. Keep the
invariant and note why calling will_retry after mark_job_run is valid: the
predictor reads only persisted job state.
WHY: will_retry hand-copied plan_retry's decision (recurring/paused, enabled,
ladder exhausted, next-rung instant, yield to the natural run) - the exact
drift that let fast jobs hold failure notices forever. Both now call a pure
_ladder_instant(job, natural_next, now); will_retry keeps only its own checks
(final finite repeat, uncomputable natural run) and reads the clock once.
The final-repeat check is spelled `times is not None and times > 0` as in
_advance_after_run. Behaviour is unchanged: an old-vs-new differential over
1728 job states matches on will_retry, plan_retry result, job state and logs.
Co-authored-by: Yuan Li <dskwelmcy@163.com>
WHY: on the last run of a finite repeat, _advance_after_run completes the
job and mark_job_run skips plan_retry (is_terminal_job), but will_retry still
predicted a re-run, so that final failure notice was held forever. Mirror
the terminal guard. Folds in the edge reported by #109991 (Liuzikaii).
WHY: the salvaged docstring narrated the incident and referenced another PR;
replace it with the invariant the predictor must hold. The test only covers
the yield branch, so drop "terminal_paths" from its name.
will_retry gated notice suppression on recurring/paused/attempt/config only.
plan_retry has a yield branch: when the schedule's own next occurrence is at or
before the pending ladder rung it schedules nothing and clears state without
consuming an attempt. For a job on a cadence at or under a rung (<=5m, and the
15m/30m rungs for faster cadences) every failure hit that branch, the attempt
counter never advanced, and will_retry kept answering True — so during a
sustained outage every failure notice was held forever. The documented escape
('once the ladder is exhausted, the next failure alerts normally') was
unreachable: the ladder could never exhaust.
will_retry now recomputes the natural next occurrence exactly as
_advance_after_run will and answers True only when the rung precedes it — i.e.
exactly when plan_retry will actually park a re-run. Notices now go out on the
first failure for cadences the ladder cannot help, and slow jobs keep their
silent bounded re-runs.
(cherry picked from commit 9d3d6006204269103188a5ee366d8eb7c9f482a2)
Follow-up to the salvaged restart-wait commits:
- A drain or cron timeout of .inf now means "wait indefinitely" instead of
an OverflowError from the integer stop envelope, which crashed
`hermes gateway restart` and made `hermes update` silently fall back to
its 45s floor. The fleet "draining (up to Ns)" lines and the drain
progress report format the budget instead of int()-ing it, so an
unbounded wait no longer crashes them either (it did on main too).
- cron_drain_timeout is required: a 0.0 default meant "cron opted out",
the under-budget this fix exists to remove.
- Docstrings describe what the budget actually covers (PID exit, not
replacement startup).
- Tests assert the observer outlasts after-turn + the supervisor stop
envelope and that configured cron reaches the CLI wait, instead of
re-deriving the formula; the negative wording assertion on the pending
footer is dropped (change-detector).
Neither kept test reaches a pyvenv.cfg read: the committed-generation path
returns before the cron fall-through, and the gateway overlay never parses
it. The `version=` arg, base/home dirs and the legacy cfg content were setup
nothing consumed. The cron test's selected_venv patch looks inert on head but
is what makes it red on base, so it now says so. Both tests still pass on head
and fail with base prod files (fake-win32 harness).
