docs(inference_optimizer): detach the optimizer the way the harness understands - #1400
Merged
Merged
Conversation
CI E2E report — ✅ Succeeded
|
…ands
The launch recipe says `setsid nohup ... &`, and gives the reason: Cursor's
background shell can die on SSH disconnect. That reason is sound and the form is
right where it applies. It is wrong under Claw, and the way it is wrong is not
visible from inside the run.
Claw deletes a sandbox once `lastActivity + 15m` passes; `lastActivity` only
moves for traffic through the Router, and Claw stops pinging the moment the
agent turn reaches a terminal state, on the rule that a finished turn leaves
behind at most a warm cache. A `setsid nohup` optimizer is invisible to all of
that -- it holds nothing open, it is registered nowhere, and a sandbox busy with
it looks exactly like an abandoned one. On 2026-09-02 four dispatches lost a live
sandbox to that, the cleanest reporting `task.completed` at 10:43:14 and losing
its pod at 10:57:46: no error in between, the optimiser still working.
So the recipe now branches on the harness. Under Claw the command goes to the
bash tool with `run_in_background=true`, which detaches it just as thoroughly but
through the door the platform can see. Everywhere else `setsid nohup ... &` is
unchanged.
Three things follow from it:
- `$PID_FILE` now comes from the launch-info JSON's `.pid` on both paths. The
tool returns a `shell_id` and never a pid, and `$!` under setsid is the
wrapper, which exits immediately -- the monitor reads that file and fires a
spurious resume on a dead pid. operations.md already said to reconcile it;
the two documents now agree on where the number comes from.
- The health check stops asking `/proc`. A zombie keeps its `/proc` entry and
sandbox PID 1 does not reap -- 18 of 56 processes were zombies in a sandbox
sampled while writing this -- so `test -d /proc/$pid` reports a dead
optimiser as alive for as long as the pod survives. It asks `ps` for the
state and rejects `Z`.
- The robustness monitor is detached by the same rule. It outlives the turn on
purpose, so under Claw it had the same problem the optimiser did.
Every document that taught the old form is updated together -- both packaged
docs, the three example skills, and the two pre-release prompts -- because one
left behind is one an agent will follow.
What this does not do, stated in the skill so nobody concludes it is broken:
`BG_SHELL_ENABLED` has to be on in the deployment or the tool refuses
`run_in_background` outright, and Claw's keepalive has to consult the shell
registry before treating a sandbox as idle (AMD-AGI/PrimusClaw#22). Until that
lands the shell is visible but the sandbox is still reclaimed on the old timer.
This is necessary for the fix and not by itself sufficient.
The cost is named too: Hands takes its registered shells down on SIGTERM, so a
Hands restart now ends a run that `setsid nohup` would have outlived. A sandbox
restart ends it either way, and being visible is worth more than surviving a
restart nothing would have noticed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Claw-vs-setsid launch branch landed in SKILL.md but operations.md's health check still used `test -d "/proc/$REAL_PID"`, which reports a zombie as alive forever (sandbox PID 1 does not reap). Ask `ps -o stat=` and reject `Z`, matching SKILL.md and the custom-advanced example. Also drop a duplicate `launch_info=` assignment in the SKILL.md health check and normalise the four new "optimiser" spellings to the tree-wide "optimizer". Docs only; no Python touched. Co-authored-by: omnigent <noreply@omnigent.ai>
…ool calls Cross-vendor review found four defects in the Claw launch branch: 1. The launch block `export`s RUN_TAG/RUN_DIR/PID_FILE and the health check reads them back, but under Claw the launch is its own background tool call and the health check is necessarily a separate foreground one, so those exports are gone. RUN_TAG is timestamped and cannot be recomputed, so the launch now persists the run-scoped vars to a session-scoped $RUN_ENV file (same naming rule as setup_env.sh, for the same WekaFS reason) and the health check, monitor, and poll blocks source it. 2. The custom-advanced example ran launch and health check as one block; under run_in_background=true the sleep 30 and its output would go to the background too. Split into a launch block and a separate health-check block, matching SKILL.md, so the harness branch applies to the launch line only. 3. `pgrep ... | head -1` silently adopted another session's pid when several optimizers share a host. It is now taken only when unambiguous and fails loud otherwise. 4. SKILL.md's health check called bare `jq` while operations.md states jq is not on every node. It uses the same python3 `read_json` helper as the rest. Docs only; no Python touched. Co-authored-by: omnigent <noreply@omnigent.ai>
…tale pid Round-2 review found the separate-shell fix was incomplete and the pidfile reconciliation had a silent failure mode. A. Audited every fenced bash block in SKILL.md, operations.md and the three example skills for reads of RUN_TAG/RUN_DIR/RUN_LOG/PID_FILE/ LAUNCH_INFO_FILE/SESSION(_DIR) that the block does not itself define. The monitoring poll block and the event-counts block in SKILL.md were still assuming carried-over state -- the latter inheriting $SESSION from a different block. Both now source $RUN_ENV, and the event-counts block re-resolves $SESSION instead of inheriting it. Audit is now clean across all five documents. B. When no authoritative pid can be found (no .pid in the launch-info JSON and a pgrep that is empty or ambiguous), the reconciliation left $PID_FILE holding whatever launch put there -- on the setsid path, the wrapper pid, already dead, which makes the monitor fire a resume against a session that never stopped. All three health checks now `rm -f "$PID_FILE"` in that branch and say why: the monitor guards its read with `[ -f "$PID_FILE" ]`, so absent means "unknown" and it falls through to the optimizer.lock owner pid, state.json freshness, and lease signals. The happy path is unchanged, so $PID_FILE still carries the real pid from launch-info on both paths. Also carried the concurrent-non-Claw-runs $RUN_ENV caveat into the custom-advanced example, which had the naming rule but not the warning. Docs only; no Python touched. Co-authored-by: omnigent <noreply@omnigent.ai>
_pr1400_v3.diff is a local dump of this PR's own diff, produced by the review tooling in the worktree. It was swept in by a `git add -A` in the previous commit and does not belong in the tree. Removed from version control; the file stays on disk locally. Co-authored-by: omnigent <noreply@omnigent.ai>
zoroyihan7
force-pushed
the
fix/optimizer-launch-via-claw-background-shell
branch
from
September 16, 2026 13:20
6152cf4 to
dc780ce
Compare
zoroyihan7
marked this pull request as draft
September 17, 2026 06:34
… the run The rule said: under Claw (`$CLAW_SESSION_ID` set) use the bash tool's `run_in_background=true`, everywhere else `setsid nohup ... &`. The intent was right and the test was unreachable -- `$CLAW_SESSION_ID` never reached the shell a model's command runs in, so every agent following this fell through to `setsid nohup`, which is the form the rest of this document exists to prevent, failing the way it warns: invisibly, from inside the run. That is fixed in AMD-AGI/PrimusClaw#51, so the test now answers. It is not sufficient on its own, though. Being on the platform does not say the deployment serves background shells: with them switched off the bash tool has no `run_in_background` parameter at all, and `setsid nohup` is then correct even there. So the rule takes both -- the session id for "something is reclaiming this sandbox on what it can see", the parameter for "and there is a tool call to hand the block to". Keying on the parameter alone was considered and is wrong in the other direction: agent harnesses off-platform have background modes of their own, and pointing a local run at one regresses exactly what `setsid` is there for, since nothing off-platform reclaims a sandbox and such a shell may not outlive the connection that started it. Every site that stated the rule moves together: the packaged SKILL.md (the definition, the launch block and the robustness monitor), operations.md (the detach rule, the pid reconciliation and the monitor), the three example skills, and the two pre-release demo prompts. The prompts keep the literal `setsid nohup`, which test_pre_release_stall_liveness asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ver had it The previous commit moved every site that stated the harness branch rule. These five never stated it at all -- they teach `setsid nohup` unconditionally -- so they were left contradicting the rule rather than merely restating it stale. The damaging one is the pre-release resume nudge. It is injected at runtime into the same demo legs whose prompts carry the rule, and it fires exactly when an agent has already failed to leave a run behind: the correction told it to detach with `setsid nohup`, unconditionally, overriding the rule at the one moment the rule matters most. The rest are siblings left behind by earlier passes: the forge pre-release prompt, whose 3h and 12h counterparts were updated; the -forge and -atom example skills, whose three sibling skills were updated; and the invocation comment in robustness_monitor.sh.example. Nothing asserts any of them, which is why they drifted -- the demo prompt test loops over (3, 12) and never sees the forge leg. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zoroyihan7
force-pushed
the
fix/optimizer-launch-via-claw-background-shell
branch
from
September 17, 2026 06:36
f5186b8 to
3fcc929
Compare
zoroyihan7
marked this pull request as ready for review
September 17, 2026 06:37
…nding The Launch section carried a caveat that this change was necessary but not sufficient, because Claw's keepalive did not yet consult the shell registry before treating a sandbox as idle. That capability has since landed, so the caveat now tells a reader to expect a failure that does not happen -- and the paragraph it sits in exists to tell them what to check before concluding the recipe is broken. Both directions were confirmed against a live deployment rather than inferred: a thirty-minute run launched through the tool held its sandbox across the end of the turn that started it, and the sandbox was reclaimed a few minutes after the run exited -- so the gate neither reclaims work that is still running nor holds a sandbox whose work has finished. What is left of the caveat is the half that is still real, and it is the same thing the launch rule's second condition tests: with BG_SHELL_ENABLED off there is no run_in_background parameter to use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chaojhou
approved these changes
Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The launch recipe says
setsid nohup ... &, and gives the reason: a harness's own background shell can die on SSH disconnect. That reason is sound and the form is right where it applies. It is wrong under Claw, and the way it is wrong is not visible from inside the run.Claw deletes a sandbox once
lastActivity + 15mpasses.lastActivityonly moves for traffic the platform can see, and it stops pinging the moment the agent turn reaches a terminal state, on the rule that a finished turn leaves behind at most a warm cache. Asetsid nohupoptimizer is invisible to all of that — it holds nothing open, it is registered nowhere, and a sandbox busy with it looks exactly like an abandoned one.On 2026-09-02 four dispatches lost a live sandbox to this. The cleanest reported
task.completedat 10:43:14 and lost its pod at 10:57:46 — no error in between, the optimiser still working. Reproduced deliberately on 2026-09-03: a turn that started a background job finished at 07:42:56, the keepalive stopped in the same second while the job still had four minutes to run, and the control plane deleted the sandbox at 07:57:56, fifteen minutes and one second after the last activity.So the recipe branches on the harness. Where it applies, the command goes to the bash tool with
run_in_background=true, which detaches it just as thoroughly but through the door the platform can see. Everywhere elsesetsid nohup ... &is unchanged.The branch takes two conditions, not one
$CLAW_SESSION_IDset and the bash tool takes arun_in_backgroundparameter. Both, orsetsid nohupstays correct.Each answers half the question, and each was found the hard way:
setsid nohup— the exact failure this PR exists to prevent. Fixed in Let a command see the session it is running for, and retire the alias that stood in for it PrimusClaw#51, which has landed; the test now answers.setsidis there for: nothing off-platform reclaims a sandbox, and such a shell may not outlive the connection that started it. Local mode must keepsetsid nohup, and with both conditions it does.Four cases, all correct: on-platform with background shells on → the tool; on-platform with them off →
setsid, since the tool offers no parameter; local with an agent harness →setsid, since the session id is unset; local bare shell →setsid.Three things follow
$PID_FILEcomes from the launch-info JSON's.pidon both paths. The tool returns ashell_idand never a pid, and$!under setsid is the wrapper, which exits immediately — the robustness monitor reads that file and fires a spurious resume on a dead pid.operations.mdalready said to reconcile it; the two documents now agree on where the number comes from.The health check stops asking
/proc. A zombie keeps its/procentry and sandbox PID 1 does not reap — 18 of 56 processes were zombies in a sandbox sampled while writing this — sotest -d /proc/$pidreports a dead optimiser as alive for as long as the pod survives. It askspsfor the state and rejectsZ.The robustness monitor is detached by the same rule. It outlives the turn on purpose, so it had the same problem the optimiser did.
Every document that taught the old form is updated together — both packaged docs, the example skills, and the pre-release prompts — because one left behind is one an agent will follow. The second commit covers five sites that never carried the branch at all, including the pre-release resume nudge, which is injected at runtime and told an agent to use
setsid nohupunconditionally at the exact moment the rule matters most.Verification
Docs only — no Python touched. The tests that parse these files pass (133), including
test_pre_release_stall_liveness, which asserts the literalsetsid nohupstill appears in the two demo prompts.Verified end to end on a cluster carrying the PrimusClaw fix, with a real GPU workload: an agent given no hint about detaching read the skill, checked both conditions, and launched a 30-minute optimizer run with
run_in_background=true. The job outlived the agent turn, ran to its budget and exited cleanly; the platform held the sandbox while it ran and reclaimed it shortly after it finished. The run manifest also recorded a realclaw_session_idfor the first time, the same PrimusClaw fix making both work.