Repository navigation
Conversation
|
Implementation, regression hardening, and production-like restart verification are complete.
Please provide a full maintainer review. This PR must remain unmerged. |
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head review of 74d90c3198736be42e3109e4af845f4c492999d2 against current main / base 4dac5f28af54001b899c9b6fc8ba81cb58da2f0e.
I read the full 17-path change, all three commits, the execution-ledger handoff, restart-safe scope construction, durable delivery queue, gateway profile drain, Kanban reuse, focused/live restart tests, existing PR discussion/reviews, the #60432 → #60612/#60631 → merged salvage #60711 lineage, and the currently-open worker-isolation work in #97739.
The core handoff shape is strong. In particular:
- the parent fences
claimedbefore launch and the worker must CAS-adopt that exact execution before it acknowledges or runs side effects; - in-process execution is also owner-fenced, so a lost claim does not fall through into user work;
- a dead adopted worker settles
unknownrather than pretending the run definitely failed before effects; - final delivery is keyed by execution id, claimed before transport I/O, tombstoned after terminal pruning, and an uncertain claimed send is never replayed;
- terminal payloads are scrubbed and transport errors are redacted;
- the production-like Linux test exercises the important whole path: kill the managed-gateway parent while the worker is active, keep the same worker alive outside the gateway cgroup, observe the side effect once, drain through the replacement gateway, and deliver once.
I do not see a source-level correctness blocker in that ownership/delivery transaction on this head. There are, however, three repository-level gates that need to close before this is a safe landing object.
1. Reconcile the scope substrate with #97739 before either PR lands
This PR and #97739 both modify cron/scheduler.py, gateway/run.py, and tools/process_registry.py, and they overlap semantically at the most load-bearing seam: how gateway-owned work escapes the gateway cgroup.
This head adds restart_safe_gateway_child_argv() as a user-scope-only contract: a managed Linux gateway directly requires _systemd_run_user_scope_available() and then builds the existing user scope. #97739 is simultaneously replacing that substrate with backend selection across user scope and a same-UID system scope. Its current production evidence is specifically the counterexample this PR needs to compose with: the runtime user has no usable user bus, while a same-UID system scope is available.
So these are complementary PRs, not duplicates: #101877 owns restart durability / execution transfer / delivery settlement; #97739 owns generalized worker-resource isolation and backend selection. But the shared seam needs one owner.
Required before merge:
- choose the backend-neutral scope-construction seam as the single authority;
- if #97739 lands first, rebase this PR and make cron + Kanban consume that generalized seam instead of reintroducing a direct user-scope check;
- if this PR lands first, #97739 must explicitly absorb this restart-survival contract when it replaces the helper layer;
- add a composition regression for
user scope unavailable + system scope availableproving both the cron external worker and the Kanban worker still receive an independent restart-safe scope; and - rerun the focused restart/ownership/delivery suites on the actual candidate merge graph.
Without that reconciliation, merge order can turn a valid system-scope deployment into a fail-closed cron/kanban dispatch even though an isolation backend is available.
2. Hosted CI has not executed on any of the three commits
The local and live receipts in the PR body are useful, but the hosted exact-object gate is currently empty:
4b6553150d9a9c184f935e2ac4521d55716d6061: no workflow runs;9c9bc68cc4781c788ed8405ad7fe2c4af344c7e2: no workflow runs;74d90c3198736be42e3109e4af845f4c492999d2: CI 33717412891, Docker 33717412230, and Nix 33717412239 all endedaction_required; the CI run contains zero jobs.
That is an unexecuted fork-workflow gate, not evidence of a code failure. It is still not green evidence. Please approve/retrigger the fork workflows and get the required matrix green for every commit, with the final required checks green again on the exact landing head after any interlock rebase/fix.
3. The live PR topology/credit needs to describe the system this code actually changes
The shutdown side here is a refinement of behavior already landed through #60711. That merged salvage preserved the work from #60612 by HexLab98 and #60631 by JoaoMarcos44: cron became visible to gateway drain, and forced shutdown marks still-running cron work interrupted so it cannot later publish a false success. This PR intentionally changes the restart-safe subset of that contract by excluding the parent waiter from forced interruption once durable ownership has moved to the external worker. That relationship should be explicit so a future reviewer does not read this as a replacement or regression of #60711.
The body also currently reads as cron-only, but this head changes Kanban worker spawning to use the same restart-safe scope. And the three commits are authored/committed by Brooklyn Nicholson (OutThisLife) while jayleaton is the PR carrier. Preserve both facts rather than flattening them. I also do not find a contributors/emails/brooklyn.bb.nicholson@gmail.com mapping on current main; run the repository attribution audit and add the mapping if the audit requires it before CI is considered complete.
Required body/interlock cleanup:
- link #60432 and merged #60711 as historical predecessor/ownership context, without claiming to re-fix the already-closed class;
- link #97739 as complementary overlapping scope infrastructure and record the merge-order rule above;
- call out the Kanban worker change as intentional scope;
- credit the salvaged predecessor authors and the implementation author on this carrier; and
- identify/link the canonical current residual issue if this restart-survival behavior is a new defect class rather than only a refinement of #60432.
This is a COMMENT review, not an approval. The hard engineering here is good—especially the ownership CAS, unknown settlement, and live parent-death test. Close the scope-backend interlock, provenance graph, and exact-object CI gates and this becomes a much easier object to trust.
|
Salvaged via #101940 — all three of @OutThisLife's commits cherry-picked with authorship preserved (carrier @jayleaton credited in the body). Thanks for the careful ownership-CAS / at-most-once design; the parent-death live test is exactly the right shape and it reproduced cleanly here. Two follow-up commits fold in the review:
Re @andrexibiza's gates: #60432/#60711 lineage, the #97739 scope-seam overlap + merge-order rule, and the intentional Kanban scope change are recorded in the #101940 body; |
Follow-up to the salvaged restart-safe worker (#101877): - delivery_queue: a row still `pending` at the worker's wait timeout was marked `failed` and never drained, so any gateway outage longer than the 300s budget (e.g. a restart that runs `hermes update`) silently lost the delivery. Unclaimed rows are certainly unsent, not uncertain — leave them queued for the next gateway; only mid-send rows are fenced `unknown`. - delivery_queue: stop running the full-table prune UPDATE+COUNT inside every transaction (each `get_status` poll paid for it; terminalizing paths already prune explicitly); poll at 1s instead of 250ms. - delivery_queue/executions: use `hermes_state.apply_wal_with_fallback` (bare `journal_mode=WAL` raises on NFS/SMB homes) and the race-safe `hermes_cli.sqlite_util.add_column_if_missing`; drop the copied owner-liveness helpers in favour of the ones in cron.executions. - scheduler: the parent waited on the worker by re-opening the executions ledger every 50ms for the whole run (~20 opens/s, hours). Wait on the process with a 1s timeout instead — the worker commits its terminal row before exiting — and reap stranded payload/ack files once terminal. - scheduler: skip the housekeeping drain until a worker has actually created deliveries.db, so non-systemd gateways never open it. - scheduler: set up hermes logging in the detached worker entrypoint; it runs with stdout/stderr on DEVNULL and previously logged nowhere. - tests: test_lost_fire_claim_stops_stale_delivery still mocked `mark_execution_running -> None`, which now means "ownership lost, return before run_job" — the test passed without ever reaching the path it names. Mocking `{}` restores it (mutation-checked).
…end is not a failure Review fold-in on the salvage of #101877: - `_deliver_result` routed to the durable queue whenever the worker's `_HERMES_CRON_EXTERNAL_WORKER` marker was set, regardless of WHICH job was delivering. A worker whose script dispatches another job in-process (`hermes cron run <other>`) inherits that env and would have queued the nested job's message under the outer execution id — `INSERT OR IGNORE` then drops it silently. Match the marker against the delivering job's own `execution_id`, as `run_one_job` already does. Regression test added (mutation-checked: fails with the guard removed). - A `pending` row left queued at the worker's wait timeout was still reported as a delivery error, so `mark_job_run` recorded `last_status=delivery_failed` for a message the next gateway's drain goes on to send, and nothing ever corrects the job record. Log and return success instead; the deliveries row is the authority for the send. - Reuse `cron.executions._TERMINAL_STATES` in the parent wait loop instead of a second hardcoded terminal set.
5 participants
Summary
Verification
scripts/run_tests.sh tests/cron tests/gateway/test_cron_delivery_housekeeping.py tests/hermes_cli/test_kanban_gateway_restart_handoff.py tests/tools/test_cronjob_run_background.py— 1236 passed, 3 skippedThis PR is intentionally left unmerged for full maintainer review.