Skip to content

Commit 3752497

Browse files
committed
refactor(update): one deferral core, shared reason prefix, per-line Desktop hint; review folds
Folds the final-head review and /simplify-code pass on #141070: - update_serve_obligations: defer_manual_serve and defer_unit_serve share _defer_identified (pid/create-time guard, dead-pid discharge, reminder write) instead of two copies; a unit row's failed-write warning says "unit restart reminder", not "manual". - update_cmd_fleet: a positive _is_serve_or_dashboard_unit name gate, composed into _is_hermes_gateway_unit; _is_serve_unit uses it rather than "gateway gate minus SIGUSR1 support". _SERVE_SKIP_REASON names hermes-dashboard* units too. - update_cmd_fleet_verify: SERVE_ONLY_REASON_PREFIX and the gateway sentence are module constants, so the producer and the Desktop predicate share one spelling; the test now feeds the producer's own reasons to both scripts. - Desktop hand-offs (posix.sh, windows.ps1): drop the gateway hint only when EVERY gateway_restart line is serve/dashboard-only, so a serve-only line next to a raw restart-phase error keeps it; case-sensitive in PowerShell (-cmatch) like grep. - main_dashboard._format_restart_commands replaces three copies of the backticked " or " join. - Docstrings: already_restarted_units covers declined and failed units.
1 parent 3c23e52 commit 3752497

10 files changed

Lines changed: 109 additions & 81 deletions

‎hermes_cli/dashboard_procs.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -605,8 +605,8 @@ def _kill_stale_dashboard_processes(
605605
Manually-started dashboards are not auto-restarted because we don't know the original launch args
606606
(--host, --port, --insecure, --tui, --no-open). See #68934.
607607
*already_restarted_units* names units (no ``.service`` suffix) the caller already handled directly —
608-
e.g. ``hermes update``'s systemd fleet-restart loop, which restarts (or, without root, declines and
609-
names) ``hermes-serve*``/``hermes-dashboard*`` units before this function runs. Without excluding them,
608+
e.g. ``hermes update``'s systemd fleet-restart loop, which restarts, declines (without root) or fails
609+
``hermes-serve*``/``hermes-dashboard*`` units, reporting each, before this function runs. Without excluding them,
610610
a Serve-only install's freshly restarted process is found again here and restarted a second time for
611611
no benefit (review on #83595), and a declined system unit is retried into a polkit prompt (#141021).
612612
"""

‎hermes_cli/main_dashboard.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,11 @@ def _systemd_restart_commands(unit: str, scope: str | None) -> list[str]:
247247
return {"user": [user_cmd], "system": [system_cmd]}.get(scope or "", [user_cmd, system_cmd])
248248

249249

250+
def _format_restart_commands(commands: list[str]) -> str:
251+
"""Join restart commands as a user-facing hint: each backticked, separated by " or "."""
252+
return " or ".join(f"`{c}`" for c in commands)
253+
254+
250255
def _try_restart_systemd_service(svc_name: str, cgroup_path: str | None = None) -> bool:
251256
"""Restart *svc_name* via systemctl (``--user`` for user-scope units). True on success.
252257

‎hermes_cli/update_abort_recovery.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -244,11 +244,11 @@ def _warn_stale_serve_runtimes(rows) -> None:
244244
print(" Ask their owner to relaunch `hermes serve` / `hermes dashboard`, or reconnect Desktop for an SSH backend.")
245245
# A unit-managed backend restarts through its own unit and scope — never "ask the owner to
246246
# relaunch", and never a hardcoded user-scope hermes-serve (#141021).
247-
from hermes_cli.main_dashboard import _systemd_restart_commands
247+
from hermes_cli.main_dashboard import _format_restart_commands, _systemd_restart_commands
248248

249249
for row in unit_rows:
250250
commands = _systemd_restart_commands(row.get("systemd_unit") or "<unit>", row.get("systemd_scope"))
251-
print(f" pid {row.get('pid')} is managed by systemd: {' or '.join(f'`{c}`' for c in commands)}")
251+
print(f" pid {row.get('pid')} is managed by systemd: {_format_restart_commands(commands)}")
252252
if sys.platform == "darwin" and any(row.get("supervisor") == "launchd" for row in rows):
253253
print(" For launchd-managed backends: `launchctl kickstart -k gui/$UID/<label>`.")
254254

‎hermes_cli/update_cmd_fleet.py‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,19 @@ def _scoped_manual_gateway_pids(pids, *, keep=(), quiet: bool = False) -> list[i
735735
return [pid for pid in pids if pid in keep or pid in owned]
736736

737737

738+
def _is_serve_or_dashboard_unit(unit: str) -> bool:
739+
"""``hermes-serve*``/``hermes-dashboard*`` (``.service``) by the same exact-or-hyphenated shape."""
740+
return (
741+
unit == "hermes-serve.service"
742+
or unit.startswith("hermes-serve-")
743+
# #125297: ``hermes-dashboard*`` units are systemd-supervised dashboard backends — the
744+
# same fleet this pass restarts. Leaving them out meant a successful update reported
745+
# the dashboard ``deferred`` (still on pre-update code) while nothing ever restarted it.
746+
or unit == "hermes-dashboard.service"
747+
or unit.startswith("hermes-dashboard-")
748+
)
749+
750+
738751
def _is_hermes_gateway_unit(unit: str) -> bool:
739752
"""Exact base unit or hyphenated profile family only: ``startswith("hermes-serve")``
740753
would accept ``hermes-server.service``."""
@@ -743,13 +756,7 @@ def _is_hermes_gateway_unit(unit: str) -> bool:
743756
# cannot enter the restart path. See #83595.
744757
unit == "hermes-gateway.service"
745758
or unit.startswith("hermes-gateway-")
746-
or unit == "hermes-serve.service"
747-
or unit.startswith("hermes-serve-")
748-
# #125297: ``hermes-dashboard*`` units are systemd-supervised dashboard backends — the
749-
# same fleet this pass restarts. Leaving them out meant a successful update reported
750-
# the dashboard ``deferred`` (still on pre-update code) while nothing ever restarted it.
751-
or unit == "hermes-dashboard.service"
752-
or unit.startswith("hermes-dashboard-")
759+
or _is_serve_or_dashboard_unit(unit)
753760
)
754761

755762

@@ -1018,7 +1025,7 @@ def _surviving_gateway_pids_after_failed_restart():
10181025
# survivors reported by _surviving_pre_update_serve_runtimes.
10191026
_SERVE_SKIP_REASON = (
10201027
"no per-profile relaunch command reaches a serve/dashboard runtime; recovered by the fresh"
1021-
" systemd unit pass when it owns a hermes-serve* unit, else left running for explicit"
1028+
" systemd unit pass when a hermes-serve*/hermes-dashboard* unit owns it, else left running for explicit"
10221029
" operator restart"
10231030
)
10241031
# A launchd-owned backend (#116503): the fresh child has no per-label kickstart for serve/dashboard

‎hermes_cli/update_cmd_fleet_verify.py‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -183,13 +183,12 @@ def _named_gateways_still_owed() -> bool:
183183

184184

185185
def _is_serve_unit(name: object) -> bool:
186-
"""A serve/dashboard unit by the fleet loop's OWN name gate (``hermes-serve*``/``hermes-dashboard*``,
186+
"""A serve/dashboard unit by the fleet loop's own name gate (``hermes-serve*``/``hermes-dashboard*``,
187187
bare or ``.service``). Anything else it records as failed (gateway units, launchd labels,
188188
listing-failure markers) is gateway evidence."""
189-
from hermes_cli.update_cmd_fleet import _is_hermes_gateway_unit, _service_unit_supports_graceful_sigusr1_restart
189+
from hermes_cli.update_cmd_fleet import _is_serve_or_dashboard_unit
190190

191-
base = str(name).removesuffix(".service")
192-
return _is_hermes_gateway_unit(f"{base}.service") and not _service_unit_supports_graceful_sigusr1_restart(base)
191+
return _is_serve_or_dashboard_unit(f"{str(name).removesuffix('.service')}.service")
193192

194193

195194
def _restart_phase_owes_gateway(restart) -> bool:
@@ -210,6 +209,14 @@ def _split_unaccounted(outcomes) -> tuple[bool, set]:
210209
return any(o.get("kind") not in _SERVE_KINDS for o in missed), serve_pids
211210

212211

212+
# The Desktop hand-offs (scripts/desktop-update/posix.sh, windows.ps1) drop their "Run hermes gateway
213+
# restart" hint only when every gateway_restart reason starts with this prefix, so every serve-only
214+
# part below starts with it and the gateway part always comes first (#141071).
215+
SERVE_ONLY_REASON_PREFIX = "serve/dashboard "
216+
_GATEWAY_OWED_REASON = "gateways may still run pre-update code or were stopped without a successor"
217+
_GATEWAY_RECOVERY = "; recover with `hermes gateway restart`"
218+
219+
213220
def _owed_restart_reason(restart, stopped, *, gateway_owed: bool, stale_serves: set, down_serves: set) -> str:
214221
"""The ``gateway_restart`` follow-up's reason, naming what is actually owed (#141021).
215222
@@ -219,25 +226,24 @@ def _owed_restart_reason(restart, stopped, *, gateway_owed: bool, stale_serves:
219226
parts = []
220227
if gateway_owed:
221228
parts.append(
222-
"gateways may still run pre-update code or were stopped without a successor"
229+
_GATEWAY_OWED_REASON
223230
+ (f" (stopped PIDs {', '.join(map(str, stopped))})" if stopped else "")
224-
+ "; recover with `hermes gateway restart`")
231+
+ _GATEWAY_RECOVERY)
225232
units = sorted({str(u).removesuffix(".service") for u in restart.failed_or_stale_units if _is_serve_unit(u)})
226233
if units:
227234
parts.append(
228-
f"serve/dashboard unit(s) {', '.join(units)} were not restarted onto the new code;"
235+
f"{SERVE_ONLY_REASON_PREFIX}unit(s) {', '.join(units)} were not restarted onto the new code;"
229236
" see the systemctl advice above")
230237
if down_serves:
231238
parts.append(
232-
f"serve/dashboard process(es) {', '.join(map(str, sorted(down_serves)))} were stopped during the"
239+
f"{SERVE_ONLY_REASON_PREFIX}process(es) {', '.join(map(str, sorted(down_serves)))} were stopped during the"
233240
" update and not brought back; relaunch `hermes serve` / `hermes dashboard`")
234241
if stale_serves - down_serves:
235242
parts.append(
236-
f"serve/dashboard process(es) {', '.join(map(str, sorted(stale_serves - down_serves)))} still run"
243+
f"{SERVE_ONLY_REASON_PREFIX}process(es) {', '.join(map(str, sorted(stale_serves - down_serves)))} still run"
237244
" pre-update code; restart them as advised above")
238245
# Nothing classified (an unknown incompleteness): keep the historical, conservative wording.
239-
return "; ".join(parts) or (
240-
"gateways may still run pre-update code or were stopped without a successor; recover with `hermes gateway restart`")
246+
return "; ".join(parts) or (_GATEWAY_OWED_REASON + _GATEWAY_RECOVERY)
241247

242248

243249
def _verify_fleet_after_update(restart, *, _pre_update_plan, _windows_gateway_resume, update_complete):

‎hermes_cli/update_cmd_maint.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,8 +273,8 @@ def _refresh_dashboard_after_update(*, already_restarted_units: set[str] | None
273273
stopped and could not bring back, so the receipt records them ``failed`` (#109290).
274274
275275
*already_restarted_units*: systemd unit names (no ``.service``) the fleet-restart loop
276-
already handled (restarted, or declined and named), so a Serve-only install isn't
277-
restarted a second time here and a declined system unit isn't retried (#141021).
276+
already handled (restarted, declined, or failed; each already reported), so a Serve-only
277+
install isn't restarted a second time here and a declined or failed unit isn't retried (#141021).
278278
279279
See #83595.
280280
"""

‎hermes_cli/update_serve_obligations.py‎

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,19 @@
1616

1717
def defer_manual_serve(runtime: dict, *, require_alive: bool = False) -> bool:
1818
"""Transfer an identified manual runtime to its own durable restart reminder."""
19-
from hermes_cli.process_identity import _pid_alive_matches
20-
2119
if runtime.get("kind") not in ("serve", "dashboard") or runtime.get("supervisor") != "manual-serve" or runtime.get("restart_via") != "respawn-argv":
2220
return False
23-
pid = runtime.get("pid")
24-
detail = runtime.get("detail")
25-
if not isinstance(detail, dict):
21+
if not isinstance(runtime.get("detail"), dict):
2622
return False
27-
created = detail.get("create_time")
23+
return _defer_identified(runtime, require_alive=require_alive)
24+
25+
26+
def _defer_identified(runtime: dict, *, extra: dict | None = None, require_alive: bool = False) -> bool:
27+
"""Shared tail of the manual and unit deferrals: True when the incarnation is gone or its reminder
28+
(plus *extra* fields) was filed; False while it is still owed and nothing durable was written."""
29+
from hermes_cli.process_identity import _pid_alive_matches
30+
31+
pid, created = runtime.get("pid"), runtime["detail"].get("create_time")
2832
if type(pid) is not int or pid <= 0:
2933
return False
3034
identified = type(created) in (int, float) and math.isfinite(created) and created > 0
@@ -38,10 +42,11 @@ def defer_manual_serve(runtime: dict, *, require_alive: bool = False) -> bool:
3842
return False
3943
if alive is False:
4044
return True
41-
_write_reminder({"kind": runtime["kind"], "profile": runtime.get("profile", "unknown"), "pid": pid, "create_time": created})
45+
_write_reminder({"kind": runtime["kind"], "profile": runtime.get("profile", "unknown"), "pid": pid,
46+
"create_time": created, **(extra or {})})
4247
return True
4348
except (OSError, ValueError, TypeError) as exc:
44-
logger.debug("Could not preserve manual serve obligation: %s", exc)
49+
logger.debug("Could not preserve serve restart obligation: %s", exc)
4550
return False
4651

4752

@@ -84,26 +89,12 @@ def defer_unit_serve(runtime: dict) -> bool:
8489
``--no-gateway-restart``) is never mentioned again. True when the row is discharged (process gone)
8590
or the reminder was filed; False when it is still owed and nothing durable could be written."""
8691
from hermes_cli.main_dashboard import _systemd_restart_commands
87-
from hermes_cli.process_identity import _pid_alive_matches
8892

8993
if not is_unit_serve_row(runtime):
9094
return False
91-
detail, pid = runtime["detail"], runtime.get("pid")
92-
created = detail.get("create_time")
93-
if type(pid) is not int or pid <= 0:
94-
return False
95-
if not (type(created) in (int, float) and math.isfinite(created) and created > 0):
96-
return _pid_alive_matches(pid, None) is False
97-
try:
98-
if _pid_alive_matches(pid, created) is False:
99-
return True
100-
_write_reminder({"kind": runtime["kind"], "profile": runtime.get("profile", "unknown"), "pid": pid,
101-
"create_time": created,
102-
"restart": _systemd_restart_commands(str(detail["systemd_unit"]), detail.get("systemd_scope"))})
103-
return True
104-
except (OSError, ValueError, TypeError) as exc:
105-
logger.debug("Could not preserve unit serve obligation: %s", exc)
106-
return False
95+
detail = runtime["detail"]
96+
return _defer_identified(runtime, extra={
97+
"restart": _systemd_restart_commands(str(detail["systemd_unit"]), detail.get("systemd_scope"))})
10798

10899

109100
def retain_receipt_manual_serves(receipt: dict) -> list[dict]:
@@ -129,20 +120,20 @@ def retain_receipt_manual_serves(receipt: dict) -> list[dict]:
129120

130121
def warn_pending_manual_serves(*, startup: bool = False, pending_manual: list[dict] | None = None) -> None:
131122
"""Warn about manual debt independently of gateway evidence; optionally reuse a snapshot's failed transfers."""
123+
from hermes_cli.main_dashboard import _format_restart_commands, _systemd_restart_commands
132124
from hermes_cli.process_identity import _pid_alive_matches
133125
from hermes_cli.update_receipt import read_latest_receipt
134126

135127
stream = sys.stderr if startup else sys.stdout
136128
if pending_manual is None:
137129
pending_manual = retain_receipt_manual_serves(read_latest_receipt() or {})
138130
for row in pending_manual:
139-
print(f" ⚠ {row['kind']} [{row.get('profile', 'unknown')}] pid {row.get('pid', 'unknown')}: manual restart reminder could not be saved; restart remains pending in the update receipt.", file=stream)
131+
unit_row = is_unit_serve_row(row)
132+
print(f" ⚠ {row['kind']} [{row.get('profile', 'unknown')}] pid {row.get('pid', 'unknown')}: {'unit' if unit_row else 'manual'} restart reminder could not be saved; restart remains pending in the update receipt.", file=stream)
140133
detail = row.get("detail") if isinstance(row.get("detail"), dict) else {}
141-
if is_unit_serve_row(row):
142-
from hermes_cli.main_dashboard import _systemd_restart_commands
143-
134+
if unit_row:
144135
commands = _systemd_restart_commands(str(detail["systemd_unit"]), detail.get("systemd_scope"))
145-
print(f" Restart its unit: {' or '.join(f'`{c}`' for c in commands)}", file=stream)
136+
print(f" Restart its unit: {_format_restart_commands(commands)}", file=stream)
146137
elif type(detail.get("create_time")) in (int, float):
147138
print(" Ask its owner to relaunch `hermes serve` / `hermes dashboard`; check reminder storage permissions and free space.", file=stream)
148139
else:
@@ -158,7 +149,7 @@ def warn_pending_manual_serves(*, startup: bool = False, pending_manual: list[di
158149
restart = row.get("restart")
159150
if isinstance(restart, list) and restart:
160151
print(f" ⚠ {row['kind']} [{row['profile']}] pid {row['pid']}: unit restart still pending; this process may still serve pre-update code.", file=stream)
161-
print(f" Restart its unit: {' or '.join(f'`{c}`' for c in restart)}", file=stream)
152+
print(f" Restart its unit: {_format_restart_commands(restart)}", file=stream)
162153
continue
163154
print(f" ⚠ {row['kind']} [{row['profile']}] pid {row['pid']}: manual restart still pending; this process may still serve pre-update code.", file=stream)
164155
print(" Ask its owner to relaunch `hermes serve` / `hermes dashboard` (reconnect Desktop for an SSH backend).", file=stream)

‎scripts/desktop-update/posix.sh‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1279,11 +1279,13 @@ if [ "$CODE" -eq 0 ]; then FINAL_CODE=0 FINAL_MSG="Update complete." UPDATE_COMM
12791279
OWED_STEPS="$(owed_followup_steps)"
12801280
if [ -n "$OWED_STEPS" ]; then
12811281
# A gateway_restart owed only for serve/dashboard units (the reason then starts with
1282-
# "serve/dashboard") names their own command in the log; every other gateway_restart reason,
1283-
# including a raw restart-phase error, keeps the gateway hint (#141071).
1282+
# "serve/dashboard ", hermes_cli/update_cmd_fleet_verify.SERVE_ONLY_REASON_PREFIX) names their own
1283+
# command in the log. The hint drops only when EVERY gateway_restart line is serve-only; any other
1284+
# reason, including a raw restart-phase error, keeps it (#141071).
12841285
case " $OWED_STEPS " in *" gateway_restart "*) OWED_HINT=" Run hermes gateway restart to move the messaging gateway onto the new code now." ;; *) OWED_HINT="" ;; esac
1285-
if printf '%s\n' "$OUT" | grep -Eq "Update follow-up 'gateway_restart' did not finish: serve/dashboard " \
1286-
&& ! printf '%s\n' "$OUT" | grep -Eq "Update follow-up 'gateway_restart' did not finish: .*hermes gateway restart"; then
1286+
GW_REASONS="$(printf '%s\n' "$OUT" | grep "Update follow-up 'gateway_restart' did not finish: ")"
1287+
if [ -n "$GW_REASONS" ] && ! printf '%s\n' "$GW_REASONS" | grep -vq "did not finish: serve/dashboard " \
1288+
&& ! printf '%s\n' "$GW_REASONS" | grep -q "hermes gateway restart"; then
12871289
OWED_HINT=""
12881290
fi
12891291
if [ -n "$DONE_NOTE" ]; then DONE_NOTE="$DONE_NOTE Follow-up steps still owed: $OWED_STEPS.$OWED_HINT"

‎scripts/desktop-update/windows.ps1‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1686,10 +1686,11 @@ try {
16861686
ForEach-Object { $_.Groups[1].Value } | Select-Object -Unique)
16871687
if ($owedSteps.Count -gt 0) {
16881688
# A gateway_restart owed only for serve/dashboard units (the reason then starts with
1689-
# "serve/dashboard") names their own command in the log; every other gateway_restart reason,
1690-
# including a raw restart-phase error, keeps the gateway hint (#141071).
1691-
$owedText = $res.Output -join "`n"
1692-
$serveOnly = ($owedText -match "Update follow-up 'gateway_restart' did not finish: serve/dashboard ") -and -not ($owedText -match "Update follow-up 'gateway_restart' did not finish: .*hermes gateway restart")
1689+
# "serve/dashboard ", hermes_cli/update_cmd_fleet_verify.SERVE_ONLY_REASON_PREFIX) names their own
1690+
# command in the log. The hint drops only when EVERY gateway_restart line is serve-only; any other
1691+
# reason, including a raw restart-phase error, keeps it (#141071).
1692+
$gwReasons = @((($res.Output -join "`n") -split "`r?`n") | Where-Object { $_ -cmatch "Update follow-up 'gateway_restart' did not finish: " })
1693+
$serveOnly = ($gwReasons.Count -gt 0) -and -not @($gwReasons | Where-Object { ($_ -cnotmatch "did not finish: serve/dashboard ") -or ($_ -cmatch "hermes gateway restart") }).Count
16931694
$owedHint = if (($owedSteps -contains 'gateway_restart') -and -not $serveOnly) { " Run 'hermes gateway restart' to move the messaging gateway onto the new code now." } else { '' }
16941695
Add-Followup ("followup: owed by the committed update: " + ($owedSteps -join ', ')) ("some follow-up steps did not finish (" + ($owedSteps -join ', ') + "). The next launch or 'hermes update' retries them; the update log has the details." + $owedHint) -Manual
16951696
}

0 commit comments

Comments
 (0)