Reclaim resources from holders that ended, and bound the Ray round wait by zoroyihan7 · Pull Request #1595 · AMD-AGI/Hyperloom

zoroyihan7 · 2026-09-21T11:15:36Z

Two resource leaks that together deadlocked a live session, and the measurement that decides what to do next.

The incident

2026-09-21, no operator involved. A specialist ended at 07:04 with its cleanup unconfirmed, holding six lanes. Its process tree was in fact already gone — the GPUs read 0% VRAM and no server process survived — but nothing could establish that, so the lanes stayed held. Nineteen integrate_patch tasks starved behind them for two hours and the session made no progress. The main log went silent for 64 minutes with zero child processes.

Two independent defects, chained:

ray.wait with no wall-clock bound  ->  worker threads parked forever
   ->  their tasks stay 'running'  ->  their lane leases are never released
      ->  19 queued tasks starve   ->  the coordinator stalls

Neither was introduced by PR #1414: the lane query came from 0d8dd3b2f (2026-09-17, another line of work) and the ray.wait loop from 8231f2433 (2026-08-18). But enablement is very likely the first workload to push both into this state, because it repeatedly starts real inference servers.

What this changes

Ray — bound the wait, and do not hand the devices back. _await_or_cancel polled ray.wait inside a while True with no overall deadline, relying entirely on someone else cancelling the scope. A task Ray never schedules gives nobody that chance: ray status reported 8.0/8.0 GPU in use while the cards sat at 0%, and py-spy found two threads parked in that loop for over an hour.

There is now a wall-clock ceiling, overridable via INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC, and the caller still receives the subprocess.TimeoutExpired its existing handlers expect. On timeout the actor is deliberately not killed: ray.kill runs neither __ray_terminate__ nor atexit, so a round that really is still running keeps its server subprocesses — and killing the actor would return those GPUs to the scheduler and let the next round be placed on cards a live server still maps. The lease is quarantined instead, honoured by close(), ensure() and run_session_kill() alike, with the actor handle parked at session scope so reference counting cannot collect it either. Those devices stay out of circulation until the session ends; the log says so, and says what to check before killing the actor by hand.

Lanes — stop trying to prove a lane is free. Three proofs were implemented and each was refuted by probe:

Proof	Why it fails
The holder's task is terminal	Says the task stopped, not its children. This is the retained-lane contract, inverted.
Its spawn process group is empty	A served server calls `setsid` by design and leaves that group — `_server_lifecycle` says so itself where it reads a pidfile.
No pidfile names a live server	Written only after the server answers, leaving a 4-minute model-load window unnamed; unlinked unconditionally at teardown; matching a command line is a guess.

Each is an observation of expected behaviour, not an ownership boundary, and a process that daemonises is free to stop matching it. The asymmetry settles it: a lane held too long stalls a queue until an operator spends ninety seconds, while a lane released too early puts two rounds on the same cards and corrupts quietly.

So reclamation by inspection is gone. What remains is liveness, which settles itself (reap_dead_holders, untouched), plus a diagnostic for everything else: one warning per (lane, holder) naming the lane, the holder, why it cannot be verified, the spawn process group as a lead explicitly not offered as proof, and a paste-ready sqlite3 statement against this session's real database path. A count rides the maintenance summary each tick.

Every shape is reported, including ones earlier revisions kept quiet: a holder still winding down (no longer distinguishable from an abandoned one — the noise is the price), one pruned out from under its lease, one still on its GPU cards, one from another boot, and one that confirmed cleanup yet left its row behind.

A measurement. cleanup_unconfirmed: "2/3" now sits beside leases_unverifiable, reading what _write_terminal already records. The count says how many lanes are held now; the ratio says whether that is a rare accident or the ordinary outcome — which is what decides whether anyone should build an automatic release mechanism at all.

Honest boundaries

2026-09-21 would still wedge under this code. It would announce itself in the log within a tick, with the command that clears it, instead of taking an hour of py-spy and sqlite to find. Overlap is structurally impossible; an operator is still in the loop.

Three portable ways to close that gap were designed and measured against the environments Hyperloom actually runs in, and all three are undeployable on the production path — a per-task cgroup needs a delegated writable subtree claw mode does not have; a PID namespace needs a fresh procfs and mount() returns EPERM there; one Kubernetes pod per specialist does not exist in local mode. They fail alike because the deployment removes exactly the privileges needed to build an isolation domain from inside a container, which is the security model working. That work, the measurements, and the one remaining candidate are recorded separately in docs/task-containment.md (branch feat/task-containment), so nobody repeats it.

Also in here: CodeQL flagged a deferred import as the start of a cycle and was right — the cycle is real, and the deferral only hid it from the runtime. The task state machine moves to a storage-free state/task_states, which task_registry re-exports, so every existing import site is untouched.

Relationship to #1605

#1605 landed a bound on the same wait this branch bounds. The two are not alternatives: they guard different windows of the same code path, and they answer "kill the actor on timeout?" in opposite directions -- each correctly for its window.

	Window	On timeout	Why that is right there
#1605	Before Ray places the actor	30s, then kill it	An actor that never acquired resources has spawned no server, so nothing survives the kill still mapping GPUs
this PR	Actor placed, round does not return	ceiling, then quarantine	The round may genuinely still be serving; killing it hands those GPUs straight back to the scheduler, and the next round is placed on cards a live server still maps

After acquired = True, #1605 returns to an unbounded while True -- which is exactly the window this branch's ceiling covers. The merge keeps both, in one loop: the acquire probe and its kill, then the ceiling and its quarantine.

One line of #1605 was deliberately dropped, and a reviewer should weigh it: the bare ray.get(ref) taken for an uncancellable round once the actor is placed. That get carries no wall-clock deadline, and it applies to precisely the rounds no cancel will ever reach -- the ones with nothing else to bound them. Those rounds keep polling instead, at one wakeup per _CANCEL_POLL_SEC, which puts them under the ceiling.

The ruff check is red, and not from this branch

src/kernelforge/gemm_tune/tier3/dispatch.py:659 carries a # noqa: BLE001 that ruff now rejects. Nothing in this PR touches kernelforge -- the diff against main is 18 files, all of them this PR's subject -- and an unmodified origin/main worktree reproduces the identical single error, on the same ruff version CI pins (0.16.2).

The cause is on main: #1620 added RUF100 to the ruff selection at 19:06, and PRs branched before that carry # noqa: BLE001 directives it now calls unused. Their own checks were green, because the rule did not exist when they ran. #1617 landed one at 19:07 and main went red; #1610 removed that one; #1609 landed another. Every PR merging main inherits whichever is current, and CI lints the PR merged with main rather than the branch alone, so this cannot be settled from here -- a fix pushed to this branch goes red again the next time such a PR lands.

Left alone deliberately, and better fixed on main.

Verification

Every new test was checked to fail with its behavioural block surgically removed, then pass restored.
Both lint gates, ruff 0.16.2, whole tree: ruff check . and ruff format --check . clean.
914 passed, 1 skipped. The one failure, test_an_unreachable_framework_is_unavailable_not_invalid, reproduces identically on an unmodified origin/main checkout and is unrelated.
After merging origin/main (which carries Bring up vLLM on ROCm 10 and fix scheduler stalls found in the 6-leg run #1605), the full inference_optimizer suite was run twice in the same environment -- once on this merge, once on an unmodified origin/main worktree. Both report 24 failures, and the two failure sets are identical: nothing is unique to either side. The merge adds 19 passing tests (13843 vs 13824). The 24 are environment-dependent credential tests in files this branch never touches.

Review note

recover-session reads as "recover a session" but does not resume execution — its help text says so explicitly, and the actual resume path is optimize --resume-from. --confirm-stopped also sits oddly under it, mutating live ownership records from a command scoped to dead sessions, which is likely why it carries the owner_scope must-be-empty guard. Flagged, not changed: renaming is a breaking change and belongs on its own.

🤖 Generated with Claude Code

…thread _await_or_cancel polled ray.wait inside a `while True` with no wall-clock deadline, relying entirely on someone else cancelling the scope. When Ray cannot schedule the task at all, nobody ever does. Observed 2026-09-21: Ray reported 8.0/8.0 GPU in use while the physical GPUs sat at 0% with no serving processes -- ghost reservations left by a specialist that died without releasing. Two integrate_patch warmups requesting {'CPU': 1.0, 'GPU': 4.0, 'serving_slot': 1.0} stayed in PENDING_NODE_ASSIGNMENT forever, and py-spy found two worker threads parked in this loop for over an hour. Each held its pool slot, so its task stayed 'running', its lane leases lapsed unreclaimed, and the coordinator went silent for 64 minutes. The deadline is the round's own cap plus slack for what actually happens outside it, configurable via INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC. Bailing out goes through the cooperative cancel sequence the cancel path already uses -- ask the actor first, escalate only after the grace period -- because ray.kill runs neither __ray_terminate__ nor atexit and would strand the served process tree of a round that is genuinely still running. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Lane occupancy was decided by `SELECT lane, holder_id FROM leases` with no regard for whether anybody was still using the row. Rows carry an expires_at, but release happened only through an explicit DELETE, so a holder that ended outside its release path held its lanes for the life of the session. Observed 2026-09-21: a specialist raised ExecutionCleanupUnconfirmed and the dispatcher deliberately retained capacity rather than releasing. Its six lanes were still held two hours later with 19 integrate_patch tasks starved behind them; the session made no progress until the rows were deleted by hand, twice. Reclamation keys on the holder's task being terminal, not on the TTL. A TTL is a static per-action budget that nothing enforces -- `explore` budgets 7200s and holds server_lifecycle plus benchmark_lane across a benchmark that can exceed it -- so a lapse says only that a long run outlived its estimate, while the terminal state is the holder's own account of having stopped. Reading no timestamp also keeps clock skew out of the predicate entirely. Two guards come straight from reap_dead_holders: owner_scope must match this boot/PID namespace, and a task with a live gpu_leases row is left alone, because a lane records its coordinator rather than the specialist's GPU worker. The sweep shares the caller's cursor so it and the capacity read that follows sit in one BEGIN IMMEDIATE, and it runs as its own pass too -- the dispatcher's lane gate reads lane_holders before any acquire, so a leaked row starves the queue without an acquire ever running to reclaim it. Running ahead of the round rules, this also settles rounds that a leaked lane row used to wedge open indefinitely, either by hand-off or by expiry on the terminal-holder cap. Both outcomes are pinned, as is the bound: a holder whose cleanup never confirmed gets its lanes back without its round being advanced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

CodeQL flagged the deferred `from ..state.task_registry import TERMINAL_STATES` inside resource_lock as the start of an import cycle, and it is right: the cycle is real (task_registry imports resource_lock's SqliteLeaseBackend), the deferral only hid it from the runtime. The states are a vocabulary, not behaviour, so they move to a storage-free `state/task_states` that anything may import. task_registry re-exports them, so every existing import site is untouched, and resource_lock now imports at module level with no cycle to step around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The previous commit treated a terminal task as proof that nobody was using its lane. An external review showed that inverts the contract: when cleanup is unconfirmed, sub_agent_runner deliberately SKIPS the release and writes terminal anyway, because the process tree may still be alive and the lane is what stops conflicting work from starting. Reclaiming on terminal alone deletes exactly the ownership that was retained on purpose -- and the gpu_leases exemption does not cover it, since most of the nine ExecutionCleanupUnconfirmed raise sites have no GPU lease at all. Reclamation now needs one of two proofs: cleanup confirmed, or a recorded process group with nothing left under it. Both launch sites spawn with start_new_session, so the id is the group's and stays meaningful after the root exits -- which is why it catches the common survivor, a child left in the dead root's group. The evidence key and the helpers say "process group" rather than "pid" because that is what is actually checked; a targeted build records the pgid its handle already carries. Reclamation never signals a process; it only asks. What it cannot see is documented rather than papered over: a descendant that calls setsid leaves both the group and the tree, and nothing in /proc ties it back. A per-spawn cgroup would name it and is a separate project. A test pins the gap so it reads as known rather than missed. Anything unprovable keeps its lane, with no TTL or age heuristic anywhere in the predicate -- silence is not proof. Since lane leases live in a per-session database, a fresh run can never inherit such a row; only resuming an old session meets one. So the operator story is the diagnostic: one warning per row, de-duplicated, naming the lane, the holder, why it cannot be verified, and a paste-ready sqlite statement against this session's real database path, prefaced by the caution to confirm no process of that task is still running. A count rides along in the maintenance summary. That report deliberately does not reuse the reclamation query. Its INNER JOIN on tasks would drop a holder pruned out from under its lease, and its gpu_leases exemption would silence the GPU path -- the one release_resources fails on, and the shape of the 2026-09-21 incident. Reclamation must respect both filters; reporting must not. The rows leaked on 2026-09-21 are not healed by this: the code that wrote them recorded no group, so they take the unprovable path and keep their lanes. What this ends is the recurrence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three proofs were tried and each was shown by probe to be a proxy a real process slips out of. Terminal state says the task stopped, not its children. An empty spawn process group says nothing: a served process is setsid'd by design, as _server_lifecycle notes where it reads a pidfile, so it leaves that group. A pidfile naming no live server says nothing either -- it is written only after the server answers, leaving the whole model-load window unnamed, teardown unlinks it unconditionally, and matching a cmdline is the same kind of guess one level down. The asymmetry decides it. A lane held too long stalls a queue until an operator spends ninety seconds; a lane released too early puts two rounds on the same cards, which corrupts quietly and may never be noticed. Closing the gap for real needs an identity a descendant cannot escape -- a per-execution cgroup -- and that is its own project. So reclamation by inspection is gone. What remains is liveness, which settles itself (reap_dead_holders is untouched), and a diagnostic for everything else: one warning per (lane, holder), naming the lane, the holder, why it cannot be verified, the spawn process group as a lead explicitly not offered as proof, and a paste-ready sqlite statement against this session's real database path, prefaced by the caution to confirm no process of that task is still running. A count rides in the maintenance summary each tick. Every shape is reported, including ones earlier revisions kept quiet: a holder still winding down (no longer distinguishable from an abandoned one, and the noise is the price), a holder pruned out from under its lease, a holder still on its GPU cards, a row from another boot, and a holder that confirmed cleanup yet left its row behind -- which should not happen and so is worth hearing. 2026-09-21 would still wedge under this code. It would announce itself in the log within a tick, with the command to clear it, instead of taking an hour of py-spy and sqlite to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The wall-clock ceiling killed the actor once the grace passed, which returns its GPUs to the Ray scheduler. But ray.kill runs neither __ray_terminate__ nor atexit, so a round that really was still running keeps its server subprocesses -- and the next round is then placed on cards a live server still maps. That trades a bounded wait for concurrent GPU use, which corrupts quietly and may never be noticed. The timeout path now abandons the round and leaves the actor alive. Its devices stay reserved, so nothing else can be placed on them, and the caller still gets the subprocess.TimeoutExpired its handlers already expect. The cost is that those GPUs stay out of circulation until the session ends or an operator intervenes; the log says exactly that, and what to check before killing the actor by hand. This is the same asymmetry the lane leases settle on, applied to the same kind of resource: a resource stuck is recoverable in ninety seconds, a resource shared is not recoverable at all. The cancel path keeps the behaviour it had on main -- an operator or a shutdown asked for that teardown, and the caller is waiting on it rather than on a round. Also finishes removing the rejected inspection design: _ended_holder_rows was left behind as unused reclamation-only code, and the comments and tests around the recorded process group still described it as something a later reaper would probe. Nothing probes it. It is recorded, and now documented, as a lead for the operator who has to clear a retained lane by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Keeping the actor alive inside _await_or_cancel was not a quarantine. Every real caller — baseline, explore, integrate_patch — closes the lease in a finally:, and close() kills the actor once its stop request fails, handing those GPUs back to the scheduler while the served tree may still map them. run_grid then moves to the next variant and is placed on them. The state was declared in one method and undone in the next. ServingLease now carries the quarantine explicitly, and all three entry points honour it. close() leaves the actor alive and says why. ensure() raises ServingLeaseQuarantined rather than minting a replacement actor behind its back. run_session_kill refuses the round and reports it the way an ensure failure is reported — deliberately not raising, because run_grid moves to the next variant on a non-zero rc and each of those attempts has to be refused too, whereas raising would escape the variant loop and widen the blast radius past what this change is entitled to. The cancel path is untouched: an operator or a shutdown asked for that teardown and the caller waits on it, rather than on a round nobody answered. Also finishes the documentation cleanup. The evidence-key block still called the recorded group the only durable trace a later reaper has, and warned that key drift would stop lanes being freed; two tests were named and documented the same way. No reaper probes the group and these lanes are never freed automatically. Both are now described as what they are: the lead an operator gets for a lane that is retained on purpose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The quarantine guarded all three entry points but only for as long as the ServingLease object lived. Every real owner keeps that lease in an action-local variable, closes it in a finally: and drops it. A Ray actor lives as long as a handle to it does, so losing the last reference collects the actor and returns its GPUs to the scheduler exactly as ray.kill would have -- the quarantine was undone by ordinary garbage collection rather than by any code path. The handle is now parked in a module-level list when the quarantine is declared, which for these purposes is session scope: the process holding it is the session. Nothing removes entries, deliberately. A test drops the lease and forces a collection, and fails without that parking. Also finishes the documentation pass. Three places still said a recurrence carrying a process-group id could be settled by a probe -- the reconcile docstring, its report attribute, and the maintenance summary -- which the final design contradicts: nothing probes that id, and no coordinator-owned lane is reclaimed automatically. What changed on 2026-09-21's shape is not that it heals, but that it announces itself within a tick with the statement that clears it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

leases_unverifiable says how many lanes are held right now. It cannot say whether that is a rare accident or the ordinary outcome, and that is the question which decides what to do next. Three portable ways to release such a lane automatically were designed and measured against the environments this actually runs in, and all three are undeployable on the production path: a per-task cgroup needs a delegated writable subtree that claw mode does not have; a PID namespace needs a fresh procfs, and mount() returns EPERM there; one Kubernetes pod per specialist does not exist in local mode. The only candidate left -- an inherited descriptor sealed behind an unprivileged seccomp filter, because a bare descriptor can be closed by the workload -- is safety-critical to get right, since a filter that misses one fd-destruction route would release lanes on a guarantee that does not hold. So measure first. The ratio rides the maintenance summary next to the count it explains, reads what _write_terminal already records, costs one query and writes nothing. A task whose history carries no cleanup account at all counts as unconfirmed: that is exactly the shape which strands a lane. If stranding turns out to be rare, the diagnostic is sufficient and nobody should take the risk. If it is routine, this is the number that argues for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ZhengGong-amd · 2026-09-22T09:29:41Z

PR #1595 -- Reclaim resources from holders that ended, and bound the Ray round wait

What it does: Two independent defects — ray.wait looping with no wall-clock deadline, and lane-release logic that required proving a served process was gone — combined on 2026-09-21 to park pool threads for over an hour and starve 19 queued tasks. The PR bounds the wait with a ceiling derived from the round's own cap plus an hourly slack (overridable via INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC), quarantines timed-out leases rather than killing the actor so their GPUs stay reserved, removes all liveness-inspection branches from lane reclamation, and adds a per-lane diagnostic that logs a paste-ready sqlite3 DELETE once per (lane, holder) alongside a new leases_unverifiable / cleanup_unconfirmed count in the maintenance summary.

Blocking issues: 3

[X5] INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC is absent from the reference docs [verified]
Problem: _ray_serving.py adds ROUND_WAIT_TIMEOUT_ENV = "INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC" and the PR body calls it out as operator-overridable, but docs/reference/environment-variables.md and .env.template at head aa80a2b contain no entry for it.
Impact: An operator who needs to widen the ceiling (e.g. on a slow head node, or to reproduce the 2026-09-21 stall deliberately) has no reference to find the knob; the env var that exists to prevent an outage is invisible unless the operator reads the source.
Action: Add a row for INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC to docs/reference/environment-variables.md and .env.template following the existing INFERENCE_OPTIMIZER_RAY_* entries; state the unset default (round_timeout + 3600s), the <= 0 disabling semantics, and that non-finite values are rejected.
[X3] _unverifiable_holders docstring contains a broken duplicate fragment [verified]
Problem: resource_lock.py:288-289 reads "Nothing reclaims these rows any more, so everything it / so everything it returns is a row that survived reclamation." — two sentence halves merged into one garbled line, present verbatim in the shipped file at head aa80a2b.
Impact: The docstring is the entire operator story for a retained lane (_report_unverifiable logging the remedy IS the interface, as stated in the docstring itself); a garbled sentence in it undermines a function the PR explicitly puts forward as the operator's only path to a remedy.
Action: Fix line 288 in resource_lock.py — the intended text is "Nothing reclaims these rows any more, so everything it returns is a row that survived reclamation."
[S1] New broad except Exception: log.debug in maintenance.py violates AGENTS.md and silently drops the cleanup_unconfirmed stat [verified]
Problem: maintenance.py:50-52 adds a nested except Exception: log.debug(... exc_info=True) around the host.reconciler._locks.cleanup_confirmation_rate() call. AGENTS.md prohibits new broad except Exception. The catch demotes any failure — including an AttributeError if _locks is ever renamed on Reconciler — to a debug-level log invisible in production, silently omitting cleanup_unconfirmed from the maintenance summary with no operator signal.
Impact: cleanup_unconfirmed is the new measurement the PR introduces to decide whether an automatic lane-release mechanism is worth building. If the call breaks silently, operators see a summary that looks correct but is missing the one ratio that matters for that decision.
Action: Remove the nested try/except. The cleanup_confirmation_rate call is a pure read against the same _locks object the reconciler already exercises every tick; there is no error class it can raise that would not also indicate a defect worth surfacing. If a tick-safe pattern is required, demote only a specific exception (e.g. sqlite3.Error) or route through Reconciler rather than reaching its private _locks directly.

Checked: src/hyperloom/orchestrator/actions/executors/_ray_serving.py, src/hyperloom/orchestrator/bus/resource_lock.py, src/hyperloom/orchestrator/loop/maintenance.py, src/hyperloom/orchestrator/bringup/reconcile.py, src/hyperloom/orchestrator/loop/sub_agent_runner.py, src/hyperloom/orchestrator/state/task_registry.py, src/hyperloom/orchestrator/state/task_states.py, docs/reference/environment-variables.md, .env.template, all seven test files in the diff | Ran: gh api for head-tree contents, diff parsing, CI status | Base: b279e2fcbc6a9e9202278e47de3316c55501bab7 | Head: aa80a2ba504b9d08cbdfa25a01ff57f08f6a0642

…rop a swallow Three findings from review, all verified against the tree before acting. The wall-clock ceiling's override was invisible. INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC appeared only in source, so an operator who needed to widen it -- or to reproduce the stall deliberately -- had nowhere to look. Documented in both places, with the semantics taken from the code rather than the PR text: the unset default is the round's own timeout plus the slack, <= 0 disables the ceiling and is also what an uncapped round gets, and a non-finite value is rejected rather than tolerated, because nan > 0 is false and one typo would otherwise switch off the very ceiling that prevents the stall. _unverifiable_holders' docstring had two sentence halves merged into one line, left by an earlier edit. It is the function the PR puts forward as the operator's only route to a remedy, so a garbled sentence in it is worse there than elsewhere. maintenance.py wrapped the new cleanup_unconfirmed read in a broad except that demoted any failure to debug. AGENTS.md forbids that, and it would have hidden exactly the wrong thing: the summary would still look complete while missing the one ratio the retention decision rests on -- including if _locks were ever renamed, since the call reached into the reconciler's private attribute. The swallow is gone and the call now goes through a Reconciler method, so the summary loses its tick loudly instead. The maintenance test's reconciler double gains the method, which is what surfaced the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

zoroyihan7 · 2026-09-22T12:17:38Z

Thanks — all three verified against the tree and fixed in 6121997f0.

[X5] env var undocumented. Correct, it existed only in source. Added to both docs/reference/environment-variables.md and .env.template. I took the semantics from _round_wait_timeout_sec rather than from my own PR text: unset means the round's own timeout plus ROUND_WAIT_SLACK_SEC (3600s), <= 0 disables the ceiling and is also what a round with no cap of its own gets, and a non-finite value is rejected — worth stating explicitly, since nan > 0 is false and one ops typo would otherwise switch off the ceiling that exists to prevent the stall. The entry also notes that a timeout abandons the round but deliberately keeps the actor alive, so an operator understands why those GPUs do not come back.

[X3] garbled docstring. Correct, and it was mine — left by a text replacement during the rework. Fixed, and I took the opportunity to make the sentence say the useful thing rather than the tautological one: it now reads "everything it returns is a lane that will stay held until an operator acts on it."

[S1] broad except. Correct on both counts, and your second point is the sharper one: reaching into reconciler._locks meant a rename would have been swallowed as a debug line, leaving a summary that looks complete while missing the one ratio the whole retention decision rests on. Removed the try/except entirely and added Reconciler.cleanup_confirmation_rate() so the call goes through a public entry point. Removing the swallow immediately broke two maintenance tests whose reconciler double lacked the method — which is the behaviour you were asking for, so I fixed the double rather than restoring the catch.

Gates after the change: both ruff gates clean whole-tree, 310 passed across loop/, bringup/, test_resource_lanes.py and test_ray_backend_unit.py. The one failure, test_an_unreachable_framework_is_unavailable_not_invalid, reproduces on an unmodified origin/main checkout and is unrelated.

Separately, one finding from this investigation is relevant to #1604 — I have left the detail there.

ZhengGong-amd

PR #1595 -- Reclaim resources from holders that ended, and bound the Ray round wait (re-review at `6121997`)

What it does: Bounds the previously unbounded ray.wait loop in _await_or_cancel with a wall-clock ceiling derived from the round's own cap plus an hour of slack, and on timeout quarantines the lease — deliberately leaving the actor alive, with its handle parked at session scope, so its GPUs stay reserved rather than being handed back while a served tree may still map them. On the lane side it abandons every attempt to prove a lane free, since each candidate proof is a proxy a setsid-ing server escapes, and replaces reclamation-by-inspection with a once-per-(lane, holder) diagnostic carrying a paste-ready sqlite3 statement, plus leases_unverifiable and cleanup_unconfirmed counts on the maintenance summary. The task state machine also moves to a storage-free state/task_states, closing the import cycle CodeQL flagged rather than deferring around it.

Blocking issues: none

All three findings from the previous review are resolved at this head, each verified against the head tree rather than the commit message:

X5 — INFERENCE_OPTIMIZER_RAY_ROUND_WAIT_SEC now has an entry in both docs/reference/environment-variables.md and .env.template. The documented contract matches _round_wait_timeout_sec: unset yields timeout + ROUND_WAIT_SLACK_SEC (3600.0), <= 0 disables the ceiling and is also what an uncapped round gets, and a non-finite override raises rather than silently switching the ceiling off.
X3 — the garbled duplicate in the _unverifiable_holders docstring is gone; resource_lock.py:288-289 now reads as one sentence.
S1 — the broad except Exception around the statistic is gone. cleanup_confirmation_rate is now a public method on Reconciler and maintenance.py calls it there instead of reaching into ._locks, which is the better half of the fix: the private-attribute coupling that motivated the swallow no longer exists. Both call sites of run_lease_and_db_reclaim (loop/maintenance._run_maintenance and phases/macro_cycle) resolve reconciler through CoordinatorCollaborator.__getattr__ to the cached Reconciler built in loop/coordinator.py:1185, so no caller can reach a host lacking the new method. The test stub gained the method and states why the call site is deliberately unguarded.

CI note: test (py3.10 shard 5/6) and both coverage jobs failed on an infrastructure flake, not on this diff. The shard itself reported 3867 passed, 10 skipped, then Failed to CreateArtifact: Unable to make request: ECONNRESET while uploading its coverage data; the py3.10 coverage job then reported Missing shard(s) ... Coverage was intentionally NOT evaluated on a partial suite, and py3.11 measured TOTAL 90.63% against a fail_under = 90 floor before failing on the same matrix-completeness gate. The failed jobs have been rerun.

Checked: _ray_serving.py, bus/resource_lock.py, loop/maintenance.py, bringup/reconcile.py, loop/sub_agent_runner.py, state/task_registry.py, state/task_states.py, orchestrator/collaborator.py, loop/coordinator.py, phases/macro_cycle.py, docs/reference/environment-variables.md, .env.template, pyproject.toml, and the seven changed test files | Ran: head-tree reads at 6121997, aa80a2b...6121997 comparison, failing-job log retrieval, coverage-floor lookup, rerun of the failed jobs | Base: b279e2fcbc6a9e9202278e47de3316c55501bab7 | Head: 6121997f0e37698f4e669d4985e4fffe0311c7a0

Containment tries to make survivor processes impossible, and cannot be built from inside these containers. This reads the same incident differently: why did a lock outlive its holder at all? Measured from the incident session, the lane mutex has exactly one user. There is one coordinator per session, one event loop, and the serving lanes conflict with each other by design, so at most one task may serve at a time -- eleven of that session's twelve specialists never touch a GPU. GPU work in ENABLEMENT is already serial; the lanes describe that rather than enable it. Yet the mutex is persisted to SQLite, which buys nothing for a single in-process user and creates a failure mode that could not otherwise exist: a lock that outlives its holder. That is the whole incident. The cards were idle throughout -- 0% VRAM, no server process -- so the retention protected nothing, and 21 tasks starved behind six rows for two hours. The table serves a second, unrelated purpose that does need durability: resume_guard reads it to learn whether the previous run's processes are still alive. That is a crash record stored in the mutex table, not mutual exclusion. So: split them. Hold lanes in memory, owned by the asyncio task doing the work, and a lane cannot outlive its holder -- "leaked lane" stops being reachable. Leave resume its durable record, ideally in the tasks table that already carries the same evidence. Roughly speaking this deletes the TTL fields, the heartbeat, the reaper, the acquire-time sweep and the operator diagnostic that exists only because a lane can strand. It gives up one thing, stated plainly in the document: the deliberate retention when cleanup is unconfirmed. Whether that protection is worth keeping depends on how often anything is actually still running at that point -- never, in the incident. PR #1595's cleanup_unconfirmed ratio is the first half of that measurement; the document names the rest and asks for it before any code is written. Designing for an unobserved failure is what produced three refuted designs already. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

#1605 landed a bound on the same wait this branch bounds, and the two guard different windows rather than the same one. Its deadline covers the window before Ray places the actor, and kills the actor when it expires. That is safe there and only there: an actor that never acquired resources has spawned no server, so nothing survives the kill still mapping GPUs. Once it has been placed, #1605 returns to an unbounded wait, which is the window this branch's ceiling covers -- and where killing the actor is exactly what must not happen, because the round may still be serving. Keep both, in one loop: the acquire probe and its kill, then the ceiling and its quarantine. Dropped from #1605's version is the bare ``ray.get(ref)`` taken for an uncancellable round once placed; it carries no deadline, so it reopens the parked-thread failure for precisely the rounds no cancel will ever reach. Those rounds keep polling instead, which costs one wakeup per 0.25s and puts them under the ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

#1620 dropped PLC0415 from the ruff selection and stripped the now-unused noqa directives with it, which is where the one conflict was: this branch's _await_or_cancel still carried one on its deferred ``import ray``. Its signature and docstring stay; the directive goes, along with the one in _abandon_ref that no rule on this branch reaches either. BLE001 stays selected, so the deliberate broad excepts on the teardown paths keep their directives and their reasons. #1611 removed the ``_now_iso`` aliases in resource_lock and task_registry in favour of calling ``now_iso`` directly; nothing this branch added referenced the alias, and pyflakes over the merged tree reports no undefined name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

#1620 added RUF100 to the ruff selection at 19:06; #1617 added a `# noqa: BLE001` to opportunity_agent at 19:07. Each was green on its own branch -- RUF100 did not exist when #1617's checks ran -- and main has been red since they landed 97 seconds apart. BLE001 does not fire on this handler anyway: ruff exempts one that logs through `log.exception`. Only the directive goes; the author's reason stays as a plain comment. The change is comment-only, verified by comparing the token stream with comments stripped. This does not belong to this PR's subject and is here only because the merge brought main's red lint onto the branch, where it blocks review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

This reverts commit 77cb461.

Clean merge, once the previous commit's revert removed the reason for the conflict: this branch had rewritten the same line in opportunity_agent that #1610 deleted outright, and two edits to one line do not merge. The ruff failure on this PR comes from `main`, not from this branch -- kernelforge/gemm_tune/tier3/dispatch.py:659 carries a `# noqa: BLE001` that #1620's RUF100 rule now rejects, and an unmodified origin/main worktree reproduces it exactly. Nothing here touches kernelforge; the diff against main is 18 files, all of them this PR's subject. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

zoroyihan7 and others added 2 commits September 21, 2026 11:14

zoroyihan7 requested a review from a team as a code owner September 21, 2026 11:15

github-advanced-security Bot found potential problems Sep 21, 2026

View reviewed changes

Comment thread src/hyperloom/orchestrator/bus/resource_lock.py Fixed

zoroyihan7 and others added 7 commits September 21, 2026 11:38

zoroyihan7 mentioned this pull request Sep 22, 2026

Make the ENABLEMENT self-repair loop deliver, and fail loudly when it cannot #1604

Merged

ZhengGong-amd approved these changes Sep 22, 2026

View reviewed changes

zoroyihan7 and others added 5 commits September 23, 2026 09:55

Revert "Drop a noqa directive that became unused between two merges"

1a05380

This reverts commit 77cb461.

ZhengGong-amd merged commit 6ecdff3 into main Sep 23, 2026
32 of 33 checks passed

ZhengGong-amd deleted the fix/lease-and-ray-timeout branch September 23, 2026 15:39

Navigation Menu

Repository navigation

Reclaim resources from holders that ended, and bound the Ray round wait - #1595

Reclaim resources from holders that ended, and bound the Ray round wait#1595
ZhengGong-amd merged 15 commits into
mainfrom
fix/lease-and-ray-timeout

zoroyihan7 commented Sep 21, 2026 •

edited

Loading

Uh oh!

Uh oh!

ZhengGong-amd commented Sep 22, 2026

Uh oh!

zoroyihan7 commented Sep 22, 2026

Uh oh!

ZhengGong-amd left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

zoroyihan7 commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The incident

What this changes

Honest boundaries

Relationship to #1605

The ruff check is red, and not from this branch

Verification

Review note

Uh oh!

Uh oh!

ZhengGong-amd commented Sep 22, 2026

PR #1595 -- Reclaim resources from holders that ended, and bound the Ray round wait

Uh oh!

zoroyihan7 commented Sep 22, 2026

Uh oh!

ZhengGong-amd left a comment

Choose a reason for hiding this comment

PR #1595 -- Reclaim resources from holders that ended, and bound the Ray round wait (re-review at 6121997)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zoroyihan7 commented Sep 21, 2026 •

edited

Loading

PR #1595 -- Reclaim resources from holders that ended, and bound the Ray round wait (re-review at `6121997`)