fix(agent): sweep orphaned agent runs on startup - #5261
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe runtime now reconciles durable non-terminal agent runs during ChangesOrphaned run reaping
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant CoreBuilder
participant reap_orphaned_runs
participant DurableStatusStore
CoreBuilder->>reap_orphaned_runs: reconcile configured workspace
reap_orphaned_runs->>DurableStatusStore: list active runs
DurableStatusStore-->>reap_orphaned_runs: pending, running, or interrupted runs
reap_orphaned_runs->>DurableStatusStore: save cancelled terminal status
reap_orphaned_runs-->>CoreBuilder: return reaped count
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
|
| Filename | Overview |
|---|---|
| src/openhuman/agent/tinyagents/reaper.rs | New module implementing the startup orphan sweep; well-structured with best-effort error handling, idempotent logic, and three targeted tests covering all non-terminal states, the empty-workspace path, and the build-path wiring. |
| src/core/runtime/builder.rs | Correctly places the sweep in build() after CoreContext::init() and before CoreRuntime is returned; gated on config.as_ref() so workspaceless builds skip it; return value intentionally discarded since logging is handled inside reap_orphaned_runs. |
| src/core/runtime/services.rs | Adds only an explanatory comment in start_boot_once_jobs clarifying why the sweep is absent here; no logic changes. |
| src/openhuman/agent/tinyagents/mod.rs | One-line module declaration exposing the reaper at pub(crate) visibility, consistent with adjacent modules. |
Sequence Diagram
sequenceDiagram
participant E as Embedder / serve()
participant CB as CoreBuilder::build()
participant CI as CoreContext::init()
participant R as reaper::reap_orphaned_runs()
participant S as FileStatusStore
E->>CB: build().await
CB->>CI: CoreContext::init(...)
CI-->>CB: (ctx, token, config)
CB->>R: "reap_orphaned_runs(&cfg.workspace_dir).await"
R->>S: list_active()
S-->>R: [Pending, Running, Interrupted runs]
loop For each orphaned run
R->>R: mark_orphaned(status) → Cancelled + Done + error + ended_at
R->>S: put_status(status)
S-->>R: Ok(())
end
R->>R: log::info! reaped N orphaned run(s)
R-->>CB: usize (ignored)
CB-->>E: Ok(CoreRuntime)
Note over E,S: First agent_runs_active RPC now sees a clean listing
Reviews (4): Last reviewed commit: "fix(agent): sweep on the build path, and..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/runtime/services.rs`:
- Around line 335-345: Add a boot-path test covering start_boot_once_jobs with
optional services disabled: seed an active agent run, invoke the boot sequence,
and assert the run is cancelled. Reuse the existing reaper test setup and verify
the unconditional startup sweep rather than calling reap_orphaned_runs directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e5f27d7-e1a0-4635-bc18-a1decd8ebe74
📒 Files selected for processing (3)
src/core/runtime/services.rssrc/openhuman/tinyagents/mod.rssrc/openhuman/tinyagents/reaper.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fb468b61b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Pushed Build-only runtimes (codex P2) — fixed, and it was the more serious of the three. You were right that Boot-path test (CodeRabbit) — added, and it landed on the same path this fix moved to: Duplicate info log (greptile) — fixed. Dropped the caller's summary rather than demoting the module's, since the module line carries the count and the module context. The pre-sweep "N orphaned run(s) to reap" line is now Tests: |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
1ce6eb7 to
8f5cd17
Compare
8f5cd17 to
f69d42f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/agent/tinyagents/reaper.rs`:
- Around line 39-100: Update reap_orphaned_runs and mark_orphaned to follow the
domain-log contract: generate one opaque sweep_id at sweep entry, use [domain]
or [rpc] for every sweep log, and emit a debug start event containing the
sweep_id. Include sweep_id in all subsequent events, and before put_status log
each transition with run_id, from_status, and to_status=Cancelled; preserve the
existing reaping behavior and counts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f52d5134-9d78-420b-bbdb-708bf9e9dccf
📒 Files selected for processing (4)
src/core/runtime/builder.rssrc/core/runtime/services.rssrc/openhuman/agent/tinyagents/mod.rssrc/openhuman/agent/tinyagents/reaper.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/runtime/builder.rs
- src/core/runtime/services.rs
f69d42f to
dc0dfb1
Compare
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…log lines The sweep logged progress but nothing tied its lines together, and a restart loop is exactly when two sweeps interleave with no way to tell whose line is whose. Adds a `sweep_id` (millisecond clock, so a log tail stays ordered) carried by every line, plus an entry event with the workspace, an exit event on each of the four branches, and the state transition spelled out (`Running->Cancelled phase=Done`) rather than left as "reaped". tinyagents::reaper tests pass. Reported by CodeRabbit on tinyhumansai#5261. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…log lines The sweep logged progress but nothing tied its lines together, and a restart loop is exactly when two sweeps interleave with no way to tell whose line is whose. Adds a `sweep_id` (millisecond clock, so a log tail stays ordered) carried by every line, plus an entry event with the workspace, an exit event on each of the four branches, and the state transition spelled out (`Running->Cancelled phase=Done`) rather than left as "reaped". tinyagents::reaper tests pass. Reported by CodeRabbit on tinyhumansai#5261. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
6c61012 to
7bcfc34
Compare
There was a problem hiding this comment.
yh928 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Maintainer review pass (comment only — no approval, and I am not pushing to this branch). Verdict: good change, but it will not compile on today's Blocking: the
|
What reaper.rs needs |
Status on main |
|---|---|
HarnessStatusStore::list_active |
present — journal.rs:243 |
FileStatusStore |
present — journal.rs, impl HarnessStatusStore at :207 |
open_session_stores + SessionStores.kv |
present — session_import/ops.rs:53, struct at :42 |
start_boot_once_jobs |
present — core/runtime/services.rs:354 |
CoreBuilder::build insertion point |
present, but the line above it changed: CoreContext::init(...) is now CoreContext::init_with_config(...) (builder.rs:596). Your hunk inserts before Ok(CoreRuntime { (:605), so the placement still holds — worth an eye after rebasing. |
On the design
No objection from me, and the reasoning in the PR body is unusually careful. Two things I want to record as read-and-agreed rather than have you re-argue them:
CancelledoverFailed— right call. A restart is not a run failure andFailedwould paint every reaped run red in the UI. WritingHarnessRunStatus's public fields directly instead of adding a cross-repomark_cancelledis the proportionate choice.- Moving the sweep from
start_boot_once_jobsintoCoreBuilder::build— this is the right answer to the codex thread about build-only embedders, and the comment you left behind inservices.rsexplaining why it is not there is the part that keeps it from being re-added later. Please keep that comment.
What to do
- Rebase onto current
main. - Fix the three
uselines as above. - Re-check the
builder.rsinsertion point after the rebase. - Push — CI on the new head is the first result that will mean anything.
Nothing here is a change of design; it is all drift in the tree underneath you. Sorry for the four weeks.
The core is a single in-process runtime, so when it exits — crash, restart, or deploy — every run still Pending/Running/Interrupted in the durable status store is orphaned: no executor remains to advance it, and no cancel path can reach it (the owning task is gone). Yet it stays in the `agent_runs_active` listing forever. A long-lived instance was observed carrying 50 such zombies. Add a startup sweep (`tinyagents::reaper::reap_orphaned_runs`) that lists the still-active runs in the persistent `FileStatusStore` and moves each to the terminal `Cancelled` state with a stable, grep-friendly error. It runs in `start_boot_once_jobs` right after the legacy migrations — before readiness is published and regardless of ServiceSet — so the first active-runs read reflects reality. Best-effort: a list/persist failure is logged and never blocks boot, and the sweep is idempotent (a clean listing reaps nothing). `Cancelled` (not `Failed`) is deliberate — a restart is not a run failure, and `Failed` would render every reaped run as a red error row. tinyagents has no `mark_cancelled`, but `HarnessRunStatus`'s fields are public, so the terminal state is set directly rather than taking a cross-repo dependency; the writer lives in its own module so the replay/status controllers stay read-only.
`start_boot_once_jobs` runs from `serve()`, so an embedder that only calls `CoreBuilder::build()` and then `invoke()` never reached the sweep — and `openhuman.agent_runs_active` is dispatchable the moment `build()` returns, so exactly that caller kept reading the previous process's graveyard. The sweep moves into `build()`, which every runtime goes through. Logging: the reaper and its caller each emitted an info line for the same count, and the reaper announced a sweep on every clean boot. One info line now, only when something was reaped. Tests: `a_build_only_runtime_is_swept_before_it_can_be_invoked` seeds a running run, builds with `ServiceSet::none()` + `DomainSet::harness()`, and asserts the run is cancelled — taking `TEST_ENV_LOCK` because `OPENHUMAN_WORKSPACE` is process-global, and asserting the build resolved the workspace it seeded so the test cannot pass by sweeping somewhere else. reaper 3 green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
…log lines The sweep logged progress but nothing tied its lines together, and a restart loop is exactly when two sweeps interleave with no way to tell whose line is whose. Adds a `sweep_id` (millisecond clock, so a log tail stays ordered) carried by every line, plus an entry event with the workspace, an exit event on each of the four branches, and the state transition spelled out (`Running->Cancelled phase=Done`) rather than left as "reaped". tinyagents::reaper tests pass. Reported by CodeRabbit on tinyhumansai#5261. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy
The branch was written against `tinyagents::harness::`, a path that no longer resolves: `vendor/tinyagents` is a virtual manifest now and the member crates are named directly, so the sweep compiled only on its old base. Point the three imports at `tinyagents_harness::`, matching how `journal.rs` — which owns the status store this reaps through — imports the same symbols today. The layout gate also grew a rule since: inline `#[cfg(test)] mod tests` is rejected in favour of a descriptive sibling. Move the sweep's tests to `reaper_tests.rs` unchanged.
7bcfc34 to
bde2633
Compare
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0083 · 123,355 in / 1,255 out · 39,936 cached (32%) · openrouter/openai/text-embedding-3-small, deepseek/deepseek-v4-flash · 600 embedded
critique: $0.0036 · 61,346 in / 347 out · 26,112 cached (43%) · deepseek/deepseek-v4-flash
security: $0.0025 · 38,293 in / 387 out · 13,824 cached (36%) · deepseek/deepseek-v4-flash
tests: $0.0015 · 15,802 in / 449 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0007 · 7,914 in / 72 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows2 changed behaviours across 2 relationships. 2 surrounding behaviours are shown (60 graph nodes walked). 46 further behaviours left out to keep the diagram readable. flowchart LR
n0["CoreBuilder<br/>changed"]:::changed
n1["start_bootstrap_jobs<br/>changed"]:::changed
n2["bootstrap_job_plan"]:::impacted
n3["ServiceSet"]:::impacted
n0 -->|uses| n3
n1 -->|calls| n2
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
|
Rebased onto The crate rename, applied. One thing your review predates. The layout gate grew a rule since ( Verified locally, since the checks on this PR were four weeks stale:
The reasoning for keeping the sweep in |
CI's coverage lane went red on `agent::triage::evaluator` tests failing with "ScriptedModel: response queue is exhausted" — in tests this branch does not touch. It was mine. `a_build_only_runtime_is_swept_before_it_can_be_invoked` calls `CoreBuilder::build()`, and `CoreContext::init` re-registers the native bus handlers as part of that. Running in parallel with a test holding a `mock_agent_run_turn` stub, the build reinstalls the *real* `agent.run_turn` handler over the stub; the triage test then reaches a real harness and hits its deliberately-empty `ScriptedModel`, which is the exhaustion message. The test already took `TEST_ENV_LOCK` for `OPENHUMAN_WORKSPACE`. That is a different lock. `bus_testing::BUS_HANDLER_LOCK` is the one that covers bus registration — its own docs say "any test that touches global native-bus registration state should acquire this lock first", and building a core is exactly that. Take both. Reproduced and verified rather than assumed: 3/3 red before, 3/3 green after, against `cargo test --lib -- 'core::runtime' 'openhuman::agent'`, the coverage lane's own filter. Clean `main` is 3/3 green on the same filter, which is what ruled out a pre-existing flake.
|
The Reproduced, and it is deterministic. Against the coverage lane's own filter,
A different triage test failed on some runs, which is what first suggested a race — but "3/3 red vs 3/3 green on main" is not a flake, and the honest reading of a differing-test-each-time failure is a race my change made deterministic, not one that was already there. The mechanism. The test already took
Building a core is exactly that. It now takes both. One hypothesis I chased and discarded, because it would have been a real defect: that Pushed. Sorry for the noise — the lane was right and I read it wrong the first time. |
…log lines\n\nThe sweep logged progress but nothing tied its lines together, and a restart\nloop is exactly when two sweeps interleave with no way to tell whose line is\nwhose. Adds a `sweep_id` (millisecond clock, so a log tail stays ordered)\ncarried by every line, plus an entry event with the workspace, an exit event on\neach of the four branches, and the state transition spelled out\n(`Running->Cancelled phase=Done`) rather than left as "reaped".\n\ntinyagents::reaper tests pass.\n\nReported by CodeRabbit on tinyhumansai#5261.\n\nClaude-Session: https://claude.ai/code/session_01SRSNnqQsokuGmkbpLoLCGy\n
…\n\nfix(agent): sweep orphaned agent runs on startup\n
Summary
The core is a single in-process runtime. When it exits — a crash, a restart, or a deploy — every agent run still in a non-terminal state (
Pending/Running/Interrupted) in the durable status store is orphaned: the task that owned it is gone, no executor remains to advance it, and no cancel path can reach it. Yet the run stays in theopenhuman.agent_runs_activelisting forever. A long-lived instance was observed carrying 50 such zombies.This adds a startup sweep that reconciles the persistent status store against that reality.
What it does
tinyagents::reaper::reap_orphaned_runs(workspace)lists the still-active runs in the persistentFileStatusStoreand moves each to the terminalCancelledstate, recording a stable, grep-friendly error (run orphaned: core restarted while the run was in flight).It is wired into
start_boot_once_jobs, right after the legacy migrations — before readiness is published and regardless ofServiceSet— so the very firstagent_runs_activeread reflects reality rather than a graveyard of the previous process's in-flight work.Design notes
Cancelled, notFailed— a restart is not a run failure, andFailedwould render every reaped run as a red error row in the UI. tinyagents exposesmark_completed/mark_failed/mark_interruptedbut nomark_cancelled;HarnessRunStatus's fields are public, so the terminal state is set directly rather than taking a cross-repo dependency on a new helper (mirrors exactly the field writesmark_failedperforms: terminal status +Donephase + error +ended_at/updated_at).tinyagents::replay) are documented as strict readers over the status seam. The sweep is the one writer, so it lives intinyagents::reaperrather than polluting that contract.0; a per-run persist failure is logged and the sweep continues.Tests
cargo test -p openhuman --lib openhuman::tinyagents::reaper— two tests:reap_cancels_every_active_run_and_spares_terminal_ones: seeds Pending/Running/Interrupted + one Completed run, asserts the three non-terminal runs becomeCancelledwith the reason and an end time, the Completed run is untouched, the active listing empties, and a re-sweep reaps0.reap_on_empty_workspace_is_a_noop: a workspace that never hosted a run reaps nothing and does not error.Summary by CodeRabbit
Closes #5298