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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
agent
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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
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.
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
`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
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.
1 participant
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