[elisam0]

What does this PR do?

Salvages the partial trace when an OpenClaw rollout is cut short by timeout, instead of losing the whole transcript.

Previously, the agent only read OpenClaw's session file from the completion envelope, which is absent on a cut-short run, and the finally block deleted the workdir before anything could recover it — so a timed-out rollout came back empty. This PR handles both cut-short paths: our own config.timeout (TimeoutError), and an outer harness/sandbox timeout that SIGTERMs this process during its grace window before SIGKILL. On either path, we stop waiting, locate OpenClaw's incrementally-written session on disk (_find_partial_session), parse it, and return the partial output — so the harness records a partial transcript instead of an empty rollout.

Returning quickly on SIGTERM keeps the response inside the grace window, so no harness-side change is needed. Pairs with the apptainer provider's SIGTERM-grace change.

Checklist

• I have read the contributing guidelines.
• The change is focused; unrelated "drive-by" edits are tracked as separate issues/PRs.
• Tests added or updated and pass locally, or N/A for docs-only / non-code changes (so CI unit/server checks pass when applicable).
• Pre-commit checks pass locally (pre-commit run --all-files) (so CI lint/format/copyright pass).
• All commits have DCO sign-off (git commit -s) (so the DCO check passes).

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[cmunley1]

/claude review

[claude]

HOLD

The partial-session salvage is a reasonable feature, and the disk-scan fallback (_find_partial_session) plus the TimeoutError path are sound — each run gets an isolated home under a uuid workdir, so the "most recent parseable .jsonl" heuristic won't cross-contaminate, and the new tests exercise both salvage paths with real assertions. My concern is entirely the SIGTERM mechanism.

BLOCKER — mutating the shared event loop's SIGTERM handler per-request. _run_openclaw runs on uvicorn's main-thread loop, which already owns a SIGTERM handler for graceful shutdown. The new block overwrites it with a per-run Event.set and then removes it in finally (never restoring uvicorn's). Consequences: (1) after the first run completes, the server permanently loses graceful shutdown; (2) with concurrency=32 and responses() ungated by the semaphore, overlapping runs stomp each other's handler (last-writer-wins install, first-finisher removes), so SIGTERM salvage silently works for at most one concurrent rollout. Inline comment has a concrete fix (install once at startup with a per-run registry, or drop SIGTERM and rely on harness write + existing TimeoutError salvage).

RISK — orphaned child process on SIGTERM cancel. run_task.cancel() cancels the awaiter, not the openclaw subprocess; _run_exec has no cancel-time proc.kill(). Leaks openclaw+node per salvaged run unless an outer group SIGKILL cleans up. Inline detail provided.

Non-blocking: the portable-python bump (3.12.8→3.13.14, PBS 20260805) and the new openclaw_agent_deps.sh (mirrors the existing hermes deps pattern, PORTABLE_PYTHON_SH injected by caller) and the lambda *args, **kwargs signature widening in anyterminal_agent all look fine.

Resolve the signal-handler lifecycle before merge — a broken graceful shutdown and unreliable salvage under concurrency are exactly the silent-in-production failures that page someone.

[cmunley1]

/claude review

[claude]

SHIP WITH CARE — partial-transcript salvage is a real robustness win, but the SIGTERM handling has a lifecycle regression.

The core salvage logic is sound and well-tested: _find_partial_session + _run_exec's CancelledError → proc.kill() path correctly avoid leaking orphaned children, and the config-timeout path recovers the on-disk transcript so a cut-short rollout scores instead of erroring. Good defensive coverage.

One RISK worth resolving before merge (inline on app.py:537 / _install_sigterm_handler):

• The "chain to uvicorn's graceful-shutdown handler" mechanism doesn't work on Linux. run_webserver runs uvicorn via uvicorn.run on the main thread; uvicorn installs its SIGTERM handler through loop.add_signal_handler, which lives in asyncio's internal _signal_handlers registry, not the process-level slot that signal.getsignal reads. So previous is asyncio's C-level no-op, and loop.add_signal_handler(SIGTERM, _on_sigterm) overwrites uvicorn's entry. Net effect: after the first run, an ordinary SIGTERM (operator/orchestrator restart) no longer triggers uvicorn's graceful shutdown or cancels in-flight rollouts — it hangs until SIGKILL. The SIGKILL-salvage case the PR targets still works; it's the normal shutdown path that regresses.

The unit test that would catch this instead patches signal.getsignal, so it passes against a mock (noted inline on the test). Fix is to capture/re-dispatch the loop-level handler (loop._signal_handlers[SIGTERM]) rather than the signal.getsignal result, and/or set server.should_exit.

Everything else — async structure (asyncio.wait with FIRST_COMPLETED, per-run Event registry, finally discard), subprocess decoding, workdir cleanup — looks correct. No async-HTTP or verifier-correctness concerns; this agent shells out and doesn't touch scoring.

[cmunley1]

/claude review

[claude]

SHIP — solid partial-transcript salvage for cut-short OpenClaw runs. No reliability blockers.

Scope is the agent harness only (no verifier/scorer, no async-HTTP, no ray.get), and the risky corners are handled correctly:

• No async hang. _run_exec always resolves via its own wait_for(timeout), so asyncio.wait({run_task, term_task}, FIRST_COMPLETED) cannot block indefinitely even if SIGTERM never arrives. On the SIGTERM branch run_task is cancelled and awaited under suppress(CancelledError), and the new CancelledError arm in _run_exec kills the child so no orphaned process leaks. finally still runs shutil.rmtree(work_dir).
• Signal handling is careful. _install_sigterm_handler chains to the previous handler so uvicorn still shuts down via the asyncio wakeup fd, guards the not-main-thread case (ValueError → timeout-only fallback), and installs once across concurrent runs while fanning out to all in-flight sigterm_events. Tests cover install-once + previous-handler chaining under two concurrent runs.
• Session isolation is safe. _find_partial_session rglobs only the per-run .openclaw-home (unique uuid workdir), so it can't pick up another run's transcript; onboarding writes openclaw.json, not .jsonl, so the newest .jsonl is the right file.

One NOTE (author's call, not blocking):

• Token usage is reported as 0 on the salvage path. app.py:604 seeds stdout="", and on both the SIGTERM and timeout branches stdout stays empty, so parse_openclaw_output("") returns {input_tokens:0, output_tokens:0} (app.py:632) even though output_items are recovered from disk and scored. Eval/training token reports will understate usage for cut-short runs. If token accounting on salvaged rollouts matters downstream, consider deriving usage from the recovered session events rather than stdout.

Behavior change worth flagging for reviewers: previously a timeout produced empty padded output (scored as a miss); now a partial transcript is scored. That's the intended feature (partial credit), but it will shift scores for runs that hit the timeout/SIGTERM window.

[cmunley1]

/claude review

[claude]

SHIP — LGTM, no reliability concerns.

Reviewed the OpenClaw partial-session salvage change. It's focused, careful, and well-tested. Verified points:

• Async correctness is sound. _run_exec now handles asyncio.CancelledError by killing the full process tree (_kill_process_tree) before re-raising, so the SIGTERM cancel path (run_task.cancel() → await run_task) cannot leak an orphaned OpenClaw subprocess. No httpx, no ray.get(), no missing awaits.
• Signal handling is deliberate. _install_sigterm_handler installs once, chains to the previously-registered handler (guarded if callable(previous) so SIG_DFL/SIG_IGN ints are skipped), and leaves the event-loop wakeup-fd path intact so uvicorn's graceful shutdown still fires. ValueError (non-main-thread) degrades to timeout-only salvage. The shared sigterm_events set is mutated/iterated only within the single event-loop thread, so no data race.
• Salvage fallback is safe. _find_partial_session scans the per-run isolated .openclaw-home only, and parse_openclaw_session_events tolerates a truncated final JSON line (per-line JSONDecodeError → raw), so reading a mid-write session won't crash.
• psutil is declared in pyproject.toml core deps (line 181).
• Tests cover config-timeout salvage, SIGTERM salvage, install-once/chaining across concurrent runs, and child + descendant process kill on cancellation — all asserting real behavior.

One intentional behavior change worth flagging for the reader, not a defect: a run timeout previously produced empty output (scored as empty); it now scores the partial on-disk transcript. That's the point of the PR and is documented in the inline comment, but it does shift what a timed-out rollout contributes to eval/training signal.