fix(slm): retire a stalled update-all job instead of 409ing every future run (#14703)

[mrveiss]

Closes #14703

Thinking Path

Came out of the constraint that a test install still gets fixed in code and has to work out of the box. My plan for clearing the wedged job on the live box was "restart the service" — which is an out-of-band action, and if that is the only way out, the gap is in the code, not in the procedure.

Checking, it is: POST /code-sync/update-all 409s while a job is pending/running, a job status is only ever advanced by its own orchestration, and the router has no cancel, reset or expiry path at all. So one wedged job disables the builtin updater permanently.

The obvious fix — expire on job age — is wrong here. A fleet update across many nodes can legitimately run a long time, and reaping a job that is still working is worse than the lockout, because it would interrupt real deployments rather than a stuck one. So the job reports progress and what is bounded is the gap between two signs of life.

I also considered only adding a cancel endpoint. Rejected as insufficient on its own: it requires someone to know the state exists and go clear it, which is the same out-of-band recovery in a nicer shape.

What Changed

autobot-slm-backend/api/code_sync.py:

• last_progress_at on UpdateAllJob, stamped at every stage transition and once per fleet node.
• _job_is_stale() — no progress within _UPDATE_ALL_STALE_SECONDS (env-backed, floored, default 1800s).
• The guard retires a stale job and lets the request through, logging what it retired and why, and clears the resume plan so a stale plan cannot outlive it.

An unparseable or missing timestamp is deliberately not stale, and the rule falls back to created_at for a job that never stamped.

Verification

Mutation-checked in an extracted scratch copy — nothing run from the repository. Making the rule always report stale:

ORIGINAL: 6/6 pass  FAIL=[]
MUTANT:   4/6 pass  FAIL=[live_not_stale, old_but_progressing]

Those two are the ones that matter: they are what stops the fix from reaping live work. The extraction asserts the mutation target exists before mutating, so a missing target fails loudly instead of reporting a false pass.

Tests added to the file that already owns update-all job tests, reusing its harness rather than duplicating its tuned sys.modules restore. Covered: a stalled job is stale; a progressing one is not; a job created six hours ago but still stamping is not; fallback to created_at; unreadable and empty timestamps are never stale; and the fleet stage really does stamp before each node — without that last one the field would exist and nothing would set it during the one stage long enough to matter.

Relationship to #14685

Different defects, same symptom. #14685 stops the self-update stage hanging when the play does not restart the service. This one stops any wedged job — whatever the cause, including a process killed mid-stage — from locking out every future update. Neither subsumes the other.

Model Used

Claude Opus 5

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No hardcoded values detected that have SSOT config equivalents!

[mrveiss]

Both critical findings confirmed and fixed in 07dfadcd8. The first one was
right in the worst way — it needs no race at all, and I reproduced it before
touching anything.

Finding 1 — retirement deleted the evidence the next guard reads

Verified in the diff: await _clear_resume_plan() sat at line 5728, and
_check_persisted_plan_exists() at 5737. Nine lines apart, wrong order.

The scenario that makes it live rather than theoretical is your finding 3: the
slm_self_update stage detaches its playbook and stops stamping progress the
moment it fires, so a healthy long self-update is indistinguishable from a
dead job by timestamps alone. It would be judged stale, retired, its resume
plan deleted, and the C3-b guard would then find no plan — and start a second
full orchestration against nodes the first was still updating.

Fix: the resume plan is read before any staleness decision, and a live
plan makes the job not-stale regardless of timestamps. _clear_resume_plan()
now only runs on a path where no plan exists, so it can no longer destroy one.

Finding 2 — no lock

Added _update_all_start_lock around the whole decide-retire-create sequence,
and moved that sequence into _start_update_all_locked() so the lock has one
clean scope.

Finding 5 — function length

Extracted _retire_stale_job() as you suggested, which also gives the
retirement a directly testable entry point:

start_update_all           12 lines
_retire_stale_job          22 lines
_start_update_all_locked   73 lines   (was 76 before this PR)

The remaining 73 is mostly job construction and the _run closure.

Finding 4 — tests that actually drive the endpoint

Three added, all calling start_update_all() itself:

• a live resume plan blocks retirement — asserts 409, that
  _clear_resume_plan was not called, and that no orchestration started
• a stale job is still retired when no plan is live — so the fix cannot
  silently restore the a wedged update-all job locks out every future update with no in-band way to clear it #14703 lockout by blocking every retirement
• concurrent starts create only one orchestration

Verification

I modelled the control flow the same way you did, and it caught two errors in
my own model before it agreed with the code — worth stating, because the
first two runs looked like the fix had failed:

• I first checked status == "running" where the real guard is
  status in ("pending", "running")
• I then treated staleness as a global flag, so the second caller judged the
  newly created job stale — where _job_is_stale evaluates each job's own age

With the model faithful:

ORIGINAL ordering, live self-update in flight:
  check AFTER retire, no lock     started=['B','A']  cleared=1  409s=[]     <- your finding

FIXED ordering + lock:
  live plan                       started=[]         cleared=0  409s=[job running, job running]
  no plan, stale                  started=['A']      cleared=1  409s=[job running]

is the lock load-bearing? same fix, lock removed:
  fixed ordering, NO lock         started=['B','A']  cleared=1  409s=[]

The last row matters: the ordering fix alone does not close the race, so
both changes are necessary rather than belt-and-braces.

Finding 3 — not fixed here

The self-update stage still cannot stamp progress after it detaches. Treating a
live resume plan as an override covers the dangerous case — a genuinely running
self-update is no longer reapable — but a heartbeat from the detached side
would be the real answer, and that is a different change from this one. Filed
as #14739 rather than widened into this PR.

Finding 6 — noted, not changed

Agreed the retired job is not queryable afterwards, since _active_update_all
is single-slot and gets overwritten. The logger.warning is the durable record.
Worth a follow-up if update history is ever wanted; not something to bolt on
here.