fix(cron): treat launches from a pruned live install as retryable skips - #9038
fix(cron): treat launches from a pruned live install as retryable skips#9038rubencu wants to merge 2 commits into
Conversation
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS The no-strike skip is right, but it tolerates an updater invariant violation with a disproportionate exactly-once debt engine bolted into the scheduler. Watch
Suggestions
[DESIGN-REVIEWED] ab75625 |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsThe working tree sits at the base commit, so the PR changes live only in the diff — I've validated against it. Candidate 1 is a real logic error in an added line: inside Candidate 2 dies under falsification: its outcome is stated as "can make the containment test disagree" and requires assuming a symlinked/ No blocking issues; one advisory finding. FINDING — src/kiro_crew/slack/gateway.py:4645 — the interrupted-sequence error uses [OPUS-REVIEWED] ab75625 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The strike/one-shot fix is real, but half the diff is an undeclared make-up-run machine riding along, while the nameable cause — a drained gateway the repo already knows how to restart — stays unfixed. What this change shipsIntent: stop auto-update install pruning from spending auto-pause strikes on (and deleting one-shots of) healthy cron jobs — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] ab75625 |
|
Addressed the GPT 5.6 blocking finding (F1, pruned-runtime skips consuming scheduled executions) in 9cd287e: all three pruned-launch sites (script cron, single-agent, agent-sequence acquisition) now book through a shared |
9cd287e to
805fed0
Compare
|
Round-2 findings addressed in 805fed0 (amended into the review-fix commit to stay within the two-commit gate): F1 (at-job refire loop): F2 (partial sequences): the sequence-loop acquisition now checks F3 (ENOENT scope): both handlers bind the exception and consult 322 targeted tests pass; flake8/isort/mypy clean. |
805fed0 to
39723b0
Compare
|
Round-3 findings addressed in 39723b0: F1 (never-started launches still consume their schedule): new F2 (pathless ENOENT): Also brought the touched files to black formatting (the Backend Lint failure). The Backend Tests shard-4 325 targeted tests pass; black/flake8/isort/mypy clean. |
|
Round-4 finding addressed in 99431b4: the outer |
39723b0 to
99431b4
Compare
6c2b510 to
4623817
Compare
|
Round-9 findings addressed in 4623817: F1 (cancellation replays a dispatched owed run): correct — round 8's unconditional restore was the mirror error of the bug it fixed. The CancelledError handler now restores the debt ONLY when the callback had already recorded F2 (truthy string "false" on disk): deserialization now uses a strict identity check ( 336 targeted tests pass; black/flake8/isort/mypy clean. |
4623817 to
3c014db
Compare
|
Round-10 findings addressed in 3c014db: F1 (swallowed store errors bypass owed-state recovery): confirmed — F2 (stale docstring): removed the "an operator re-arms it" clause — retained one-shots are retried automatically by the replacement gateway via 337 targeted tests pass; black/flake8/isort/mypy clean. |
Treat script and agent cron launches from a fully replaced live install as retryable skips instead of spending an auto-pause failure strike. Clear pre-acquisition reaper state, preserve unrelated FileNotFoundError failures, and let the replacement gateway launch a version-consistent child on the next tick.
9f7c058 to
ff6b051
Compare
|
Round-11 findings addressed in ff6b051 (both lanes converged on the same regression): Unreadable-store branch dropped the completed one-shot deletion: correct — my round-10 early return bypassed the base path's Also rebased onto current main and validated against the new upstream |
ff6b051 to
46cb5ac
Compare
|
Round-12 findings addressed in 46cb5ac: F1 (owed policy denials refire every poll): correct — restoring the debt on F2 (reaped make-up runs retain consumed debt): correct — the reaper's terminal merge does not carry 373 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
|
Round-13 finding addressed in a18100d: correct, and the fix goes further than the suggested merge-side reapply — reapplying 374 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
|
Round-14 findings addressed in f6a4a4c: F1 (manual triggers create scheduled debt): correct — a manual trigger (run_job / cron trigger) failing on the pruned install persisted an owed occurrence the schedule never owed, so resuming a paused job later would execute it unscheduled. The service's existing F2 (stale comment): the 375 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
|
Round-15 findings addressed in 8835ae9: F1 (quiesced overdue jobs hot-loop the timer): correct — the due-scan skipped quiesced ids but F2 (docstring precision): the drain's save condition is now stated as "once iff a queued id is still present". The Backend (Windows) shard failures on the previous head are in files this diff never touches ( 375 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
|
Round-16 finding addressed in 7f4eece: correct — a cancellation landing after the callback starts but before the prompt/launch goes out (session/context setup, the agent path's longest window) set neither signal, and the handler wrongly kept the debt consumed. New in-memory The recurring Backend shard-3/Windows-3 + Coverage Gate failures remain in files this diff does not touch (base breakage, flagged in round 15); this push re-rolls them. 376 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
…ng runs The pruned-runtime skip reused completed-Skip semantics, so the scheduler advanced last_run_ts as a success and _merge_job_result deleted a due one-shot delete_after_run job irrecoverably. Book all pruned-launch sites through a shared never-started helper: last_status 'error' blocks the success branch, run_never_started retains the one-shot, and no auto-pause strike is spent. Review-round hardening: - New CronJob.keep_overdue marker (in-memory, reset per run): a pruned skip neither advances last_run_ts nor fires _execute's at-job disable, and the merge skips its at-job enabled propagation — the replacement gateway sees the job exactly as due as before the drained process touched it. - Quiesce past-due at-jobs in memory only, killing the zero-delay refire loop on the drained gateway without persisting disabled. - A mid-sequence pruning after a prior agent completed records a normal failed run instead of never-started, so the retained one-shot cannot replay finished side effects. - Scope the ENOENT excuse to this install's own trees (interpreter prefix + package tree; never their common parent, which degrades to '/' on non-versioned layouts); pathless ENOENT stays a real failure. Regression tests cover the retention contract, the one-shot data-loss shape, schedule-owing semantics at the scheduler (_execute) level, at-job quiescence without durable parking, unrelated-path and pathless strictness, and the partial-sequence failure recording.
|
Round-17 findings addressed in ab75625: F1 (dispatch marked before prompt submission): correct — the marker sat before the stream await, so a cancellation inside session warmup ( F2 (drain failure aborts the tick): the tick's locked transaction now contains the drain's re-raise (log + continue) — the drain already requeued its claim, and a due job must not miss its minute over an unrelated persistence failure. Regression: 377 targeted tests pass (incl. the 33 upstream store-boundary contracts); black/flake8/isort/mypy clean. |
Problem / Motivation
When a managed-install auto-update promotes a new version, the updater unlinks the old version's entire tree — including its Python interpreter — while the old gateway is still running from mapped memory. Every cron child launch from that drained gateway then fails with
FileNotFoundError: [Errno 2] No such file or directory: '<install>/python3.12/bin/python3.12', logged as a hard cron ERROR and consuming an auto-pause failure strike against the job.Observed twice in the wild on the same host (two consecutive updates): the failing jobs were healthy, the runtime under them had simply been replaced. The strikes risk auto-pausing healthy jobs for an environmental race that resolves itself at the next gateway restart.
Why it matters
Cron jobs are the unattended backbone — a monitoring job that gets auto-paused because its runtime vanished mid-update silently stops watching. The update race is guaranteed to recur on every auto-update that lands while a gateway is live.
What changed (motivation → approach → change)
_running_install_was_pruned(): detects the handoff by requiring BOTHsys.executableand the module file itself to be absent — distinguishing a replaced install from an unrelated missing working directory, launcher, or user binary, which must remain real failures.FileNotFoundErrorwhile the running install is pruned are booked as never-started via a shared_record_pruned_launch_skip()helper:last_status = "error"keeps the scheduler from recording a success,run_never_started = Trueis the retention marker that stops_merge_job_resultdeleting a due one-shotdelete_after_runjob, andrecord_failure()is deliberately not called so no auto-pause strike is spent. The replacement gateway launches a version-consistent child on its next tick.FileNotFoundErrorfailures (bad cwd, missing user script interpreter) keep their existing hard-failure semantics.Tests
test/test_cron_gateway_integration.py: coverage for the pruned-runtime never-started contract (script + agent + agent-sequence paths), both-paths-absent detector semantics, strike preservation for unrelated FileNotFoundError, reaper state cleanup, and a dedicated one-shotdelete_after_runretention regression (test_pruned_install_retains_a_due_one_shot).Falsefailstest_pruned_install_requires_both_runtime_paths_to_be_absent; removing the never-started bookkeeping fails 4 retention-contract tests.test_cron_script_more_coverage.py+test_cron.py(334 total pass, including the owed-fire persistence and cancellation regressions). flake8/isort/mypy clean on touched files.Manual verification
Reproduced the race on a live host across two real auto-updates; confirmed the failing launches match the exact guarded path (interpreter file gone, module tree gone, gateway still serving).
Screenshots / video
N/A (backend behavior).
Related Issues
None open; failure class first captured 2026-09-04, recurred 2026-09-06.
Pattern harvest
Rule candidate: when a scheduled task is skipped for an environmental handoff (not a task defect), it must be booked as never-started — a bare skip that reads as success advances scheduler state and can irrecoverably delete due one-shot jobs.
Also: a live-updated daemon must treat "my own install vanished" as an environment handoff, not a task failure; requiring two independent paths to be absent avoids false positives from single missing files.
Checklist
Contribution License Agreement
I confirm my contribution is made under the project's contribution license terms.