fix: keep live agents registered through false process-exit observations - #3401
fix: keep live agents registered through false process-exit observations#3401caner-akca wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughAgent detection now requires two consecutive pane-shell observations before reporting an agent process exit. Pending confirmation forces foreground observation and process probing in both detection loops. The Suggested reviewers: Merge Risk: 🔵 Low · up to Reload recovery can replace an existing session owner in a specific mixed-state condition before normal ownership checks, potentially redirecting pane session authority. The impact is bounded and the change is mergeable with explicit owner awareness and follow-up coverage for this ownership interleaving. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryThe PR prevents transient foreground-process observations from immediately deregistering live agents and restores registration when a matching live agent subsequently reports a reload.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/agent_resume.rs | Extends session-start normalization to preserve reload, completing the public path required by the recovery logic. |
| src/terminal/state.rs | Adds narrowly scoped false-exit recovery for a reload from the same detected agent and returns a mutation that republishes registration. |
| src/pane.rs | Adds two-observation shell-exit confirmation and ensures pending confirmation forces the next process probe. |
| src/app/api.rs | Adds an application-level regression test verifying release and subsequent registration events across false-exit recovery. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent detected and registered] --> B{Pane shell observed once?}
B -->|No| A
B -->|Yes| C[Pending shell-exit confirmation]
C --> D[Force prompt process probe]
D -->|Agent found| A
D -->|Shell confirmed again| E[Report process exit and release registration]
E --> F{Matching live agent reports reload?}
F -->|Yes| G[Clear false-exit marker]
G --> H[Restore session and lifecycle authority]
H --> I[Republish agent registration]
F -->|No| E
Reviews (4): Last reviewed commit: "fix: restore lifecycle authority when a ..." | Re-trigger Greptile
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/terminal/state.rs (2)
1349-1349: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
reloadout of generic session-owner reconciliation.
session_start_source_is_recognizedalso permits a detected agent to replace a different persisted session owner. Aherdr:pireload can therefore replace a staleherdr:codexsession when Pi is detected, despitesession_report_allows_session_replacementintentionally excludingreload.Allow
reloadonly in the matching false-exit recovery path. Keep the generic arbitration predicate limited to sources that can replace session ownership.
1499-1503: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPublish the restored effective-agent change.
process_presentis evaluated before this block clearsrecent_agent_process_exit. When it isfalse, the method returnsNone, soupdate_terminal_statecannot publish the effective-agent change. Return a mutation that callsrecompute_effective_stateafter clearing the exit, without restoring hook authority.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cf971c03-1e40-47c4-b39d-8689eaf9cf35
📒 Files selected for processing (2)
src/agent_resume.rssrc/terminal/state.rs
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
|
@coderabbitai review |
|
608a8ee to
8ff3a6c
Compare
|
Both findings from the CodeRabbit review are addressed in 608a8ee. Publish the restored effective-agent change ( Keep One correction to the example in that finding: a |
6d3393b to
2779f16
Compare
|
Rebased onto current On the remaining Merge Risk from the CodeRabbit review — that the reload path "can restore a different caller-provided session identity for the same agent without proving continuity with the session that exited" — this is accepted deliberately rather than constrained, for three reasons. It is the existing replacement-session mechanism, not a new one. The reload lands in the same branch a genuine replacement process reaches after a real exit: Requiring session-ref equality would break the case the recovery exists for. A live process re-anchoring its session is authoritative about which session it now holds, and The trust boundary is unchanged, and reload is narrower than what already crosses it. The report arrives on the same user-owned control socket that already accepts Two limitations are now called out in the description rather than fixed here, to keep this change to the reported bug:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
The pane report API normalizes every session-start source before terminal arbitration sees it, and `reload` was not in the allowlist. The false-exit recovery added for this issue could therefore never run in production: the value was replaced with `None` before it reached the branch that clears a stale process-exit marker. Route the regression test through the public normalizer so the recovery cannot pass while bypassing the path real reports take. refs herdrdev#3225
2779f16 to
eeca5c1
Compare
Issue
A running agent (Pi on Windows, an assigned AGY name on macOS) abruptly disappears from
agent listwhile its process is still alive, and its registration never comes back. API clients and plugins that target the pane's agent getagent_not_foundfrom that moment on.refs #3225
Problem
Two mechanisms compound.
One observation is enough to publish a false exit. The foreground-process probe maps a previously detected agent plus a single observation of the pane shell to
ReportProcessExitimmediately (src/pane.rs), while the normal unidentified-job path requires six consecutive misses (AGENT_MISS_CONFIRMATION_ATTEMPTS). So one transient probe glitch — a sampled Windows process snapshot briefly missing the agent descendant, or a momentary foreground transition on macOS — publishes a process exit for a live agent.A false exit can then deadlock into permanent deregistration. The exit sets
recent_agent_process_exit, which masksdetected_agentout ofeffective_agent_label()— the sole basis ofis_agent_terminal(). The pane can reach a state no runtime path leaves on its own:detected_agent = Some(pi)together withrecent_agent_process_exit = Some(pi), so the pane resolves toagent_not_found.Some(pi)inagent_presencewhile its local exit flag isfalse, so every probe that re-finds Pi observes the same value.observe_process_probereports no change, no detection update is published, andset_detected_state_with_screen_signals_at— the only writer that clearsrecent_agent_process_exit— is never reached.set_full_lifecycle_authority_active(true)never fires the detection reset that would re-seedagent_presence.Every other post-exit shape self-heals: a probe that observes a changed presence value (
ReportReplacementProcess, or a freshNone → Someacquisition) publishes a detection update, and that clears the marker. The deadlock needs the loop-local exit flag and the terminal-side marker to desync — for example when a detection task starts from restored state withdetected_agentalready set (AgentDetectionPresence::from_agent(initial_state.detected_agent)) while the exit flag startsfalse.Because the process probe cannot break that state, a session-start report from the live process is the only signal left.
How did we fix it?
The shell-observation path now requires two consecutive confirmations before publishing a process exit (
FOREGROUND_SHELL_EXIT_CONFIRMATION_OBSERVATIONS), with the pending confirmation driving the same fast probe recheck as a pending shell clear, so genuine exits are still reported within one extra ~300ms probe tick. This removes a single-tick probe glitch as a way into the deadlock; it does not make a sustained probe failure impossible.On the recovery side, a
reloadsession start — a live process re-anchoring its existing session, which only a running agent can send — clears a recorded process exit for the same detected agent. The clear then takes the process-present path: the suppressed lifecycle report is reconciled, the session the false exit dropped is restored, and the transition is returned as aTerminalStateMutation.pane.agent_detectedis republished withreleased: false, and the agent's next state report is accepted rather than buffered, so registration recovers for direct queries and event subscribers on the same reload.The unmasking is deliberately narrow.
reloadis not added tosession_start_source_is_recognized, so generic session-owner arbitration is unchanged and a reload cannot replace a different persisted session owner. Recovery additionally requires the reported agent to match the detected agent and a recorded process exit for that same agent. Other session-start reasons such asstartupcan follow a genuine exit and keep the existing replay-on-process-evidence flow, and a late same-session hook report still does not silently reacquire authority after an exit.Known limitations
An assigned agent name is not restored. The false exit runs
clear_agent_name(), which drops the pane's name, its name owner, andmanaged_agent. The reload restores the agent label, the session, and lifecycle authority, so pane-target lookups (agent list,agent explain <pane>) recover — but a name assigned withagent renamedoes not come back, and name-based lookup still needs a manual rename. Requiring two observations before an exit is what keeps named panes out of this state to begin with; restoring an already-released name is a separate change.The recovery is agent-specific by construction. It is driven by the session-start report Pi emits on
/reload. An agent that never re-anchors its session cannot break the deadlock this way, and would need a recovery driven by process evidence instead.Performance
The confirmation adds at most one extra foreground probe per exit candidate per pane, bounded to a single ~300ms tick: the streak resets on any other action, and a published exit already forces the same probe on the following tick through
pending_foreground_shell_clear. On a false exit it removes the release and re-registration work entirely.shell_exit_confirmation_costs_at_most_one_extra_probe_tickpins that bound.Verification
issue_3225_reload_restores_live_pi_registration_after_false_exit: after a false process-exit observation, a live Pi'sreloadrestores the dropped session, publishes the effective-state change, and the following working report is accepted with live hook authority.reload_after_a_false_process_exit_republishes_agent_registration: at the App/API level the release emitspane.agent_detectedwithreleased: true, and the reload emitspane.agent_detectedforpiwithreleased: false— in the report order the pi integration emits, session report first and state report second.reload_does_not_take_over_a_different_session_owner: areloaddoes not decide session ownership when a different owner holds the session, whilestartupstill does.first_shell_observation_requires_confirmation_before_process_exitandshell_exit_confirmation_costs_at_most_one_extra_probe_tick: a single shell observation cannot publish an exit, and the confirmation streak resets once it resolves.pending_shell_exit_confirmation_forces_a_fast_recheckand the extendedwindows_foreground_observation_schedule_preserves_lifecycle_checks: a pending confirmation forces the foreground observation and the full probe, and is not skipped under lifecycle authority.late_full_lifecycle_hook_with_same_session_after_process_exit_does_not_reacquire_authorityandrapid_restart_replays_reports_that_arrive_before_process_evidence.Platform note
On Windows the pi integration can report a session ref only through the session id:
herdr-agent-state.tsaccepts a session file path only when it starts with/, so a Windows path is rejected. Any session-start recovery, including this one, depends on that id being present in the reload report.