Skip to content

fix: keep live agents registered through false process-exit observations - #3401

Draft
caner-akca wants to merge 3 commits into
herdrdev:masterfrom
caner-akca:issue/3225-agent-release-confirm
Draft

fix: keep live agents registered through false process-exit observations#3401
caner-akca wants to merge 3 commits into
herdrdev:masterfrom
caner-akca:issue/3225-agent-release-confirm

Conversation

@caner-akca

@caner-akca caner-akca commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Issue

A running agent (Pi on Windows, an assigned AGY name on macOS) abruptly disappears from agent list while its process is still alive, and its registration never comes back. API clients and plugins that target the pane's agent get agent_not_found from 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 ReportProcessExit immediately (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 masks detected_agent out of effective_agent_label() — the sole basis of is_agent_terminal(). The pane can reach a state no runtime path leaves on its own:

  • Terminal state holds detected_agent = Some(pi) together with recent_agent_process_exit = Some(pi), so the pane resolves to agent_not_found.
  • The detection loop still holds Some(pi) in agent_presence while its local exit flag is false, so every probe that re-finds Pi observes the same value. observe_process_probe reports no change, no detection update is published, and set_detected_state_with_screen_signals_at — the only writer that clears recent_agent_process_exit — is never reached.
  • The agent's own state reports are dropped behind the recorded exit, so lifecycle authority never returns, so set_full_lifecycle_authority_active(true) never fires the detection reset that would re-seed agent_presence.

Every other post-exit shape self-heals: a probe that observes a changed presence value (ReportReplacementProcess, or a fresh None → Some acquisition) 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 with detected_agent already set (AgentDetectionPresence::from_agent(initial_state.detected_agent)) while the exit flag starts false.

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 reload session 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 a TerminalStateMutation. pane.agent_detected is republished with released: 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. reload is not added to session_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 as startup can 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, and managed_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 with agent rename does 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_tick pins that bound.

Verification

  • issue_3225_reload_restores_live_pi_registration_after_false_exit: after a false process-exit observation, a live Pi's reload restores 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 emits pane.agent_detected with released: true, and the reload emits pane.agent_detected for pi with released: 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: a reload does not decide session ownership when a different owner holds the session, while startup still does.
  • first_shell_observation_requires_confirmation_before_process_exit and shell_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_recheck and the extended windows_foreground_observation_schedule_preserves_lifecycle_checks: a pending confirmation forces the foreground observation and the full probe, and is not skipped under lifecycle authority.
  • Existing process-exit arbitration tests pass unchanged, including late_full_lifecycle_hook_with_same_session_after_process_exit_does_not_reacquire_authority and rapid_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.ts accepts 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.

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 716e1d7f-8c73-4577-ba8c-3e22c21b7945

📥 Commits

Reviewing files that changed from the base of the PR and between d2cb096 and 2779f16.

📒 Files selected for processing (4)
  • src/agent_resume.rs
  • src/app/api.rs
  • src/pane.rs
  • src/terminal/state.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.


📝 Walkthrough

Walkthrough

Agent 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 reload session-start source is recognized. A matching reload clears a recent false process exit and republishes the live agent registration. Tests cover confirmation timing, probe scheduling, source normalization, session ownership, and Pi recovery.

Suggested reviewers: akbash-bot

Merge Risk: 🔵 Low · up to 2779f

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: preventing live agents from being deregistered after false process-exit observations.
Description check ✅ Passed The description is directly related to the changeset. It explains the false process-exit problem, the confirmation logic, reload-based recovery, scope limits, known limitations, and test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kangal-bot kangal-bot added the ai-review Trigger automated AI reviews for pull requests admitted by the PR gate label Aug 30, 2026
@caner-akca
caner-akca marked this pull request as ready for review August 30, 2026 11:34
@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown

Greptile Summary

The PR prevents transient foreground-process observations from immediately deregistering live agents and restores registration when a matching live agent subsequently reports a reload.

  • Requires two consecutive pane-shell observations before reporting a process exit.
  • Forces a prompt process recheck while shell-exit confirmation is pending.
  • Preserves reload through session-start normalization and uses it narrowly to clear a matching false-exit marker.
  • Republishes agent registration and restores lifecycle authority after recovery.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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
Loading

Reviews (4): Last reviewed commit: "fix: restore lifecycle authority when a ..." | Re-trigger Greptile

Comment thread src/terminal/state.rs Outdated
@caner-akca

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Keep reload out of generic session-owner reconciliation.

session_start_source_is_recognized also permits a detected agent to replace a different persisted session owner. A herdr:pi reload can therefore replace a stale herdr:codex session when Pi is detected, despite session_report_allows_session_replacement intentionally excluding reload.

Allow reload only 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 win

Publish the restored effective-agent change.

process_present is evaluated before this block clears recent_agent_process_exit. When it is false, the method returns None, so update_terminal_state cannot publish the effective-agent change. Return a mutation that calls recompute_effective_state after 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

📥 Commits

Reviewing files that changed from the base of the PR and between ef3f2bf and 687836d.

📒 Files selected for processing (2)
  • src/agent_resume.rs
  • src/terminal/state.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.

@caner-akca

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@caner-akca
caner-akca marked this pull request as draft August 30, 2026 12:36
@caner-akca
caner-akca force-pushed the issue/3225-agent-release-confirm branch from 608a8ee to 8ff3a6c Compare August 31, 2026 07:39
@caner-akca
caner-akca marked this pull request as ready for review August 31, 2026 07:40
@caner-akca

Copy link
Copy Markdown
Contributor Author

Both findings from the CodeRabbit review are addressed in 608a8ee.

Publish the restored effective-agent change (src/terminal/state.rs:1499-1503) — agreed and fixed. process_present was evaluated before the block cleared recent_agent_process_exit, so the method mutated state and returned None, and update_terminal_state had nothing to publish. The clear now takes the process-present path: the suppressed lifecycle report is reconciled, the dropped session is restored, and the transition is returned as a TerminalStateMutation, so pane.agent_detected is republished with released: false and the agent's next state report is accepted rather than buffered. Covered by issue_3225_reload_restores_live_pi_registration_after_false_exit and, at the App/API level, reload_after_a_false_process_exit_republishes_agent_registration.

Keep reload out of generic session-owner reconciliation (src/terminal/state.rs:1349) — agreed and fixed. reload is removed from session_start_source_is_recognized, so foreground_agent_confirms_different_owner_takeover is back to its previous behavior. Recovery runs through a dedicated predicate that additionally requires the reported agent to match the detected agent and a recorded process exit for that same agent. Pinned by reload_does_not_take_over_a_different_session_owner, which also checks that startup still decides ownership.

One correction to the example in that finding: a herdr:pi reload could not have replaced a stale herdr:codex persisted session. pi is a full-lifecycle source, so with a foreign persisted session session_anchored is false and the branch at src/terminal/state.rs:1445 returns before the takeover gate is reached. The reachable shapes were narrower — a different source holding hook authority with a session ref while the persisted session matches the reporting agent, and any non-full-lifecycle source reporting reload through the public session report. The narrowing covers both.

@caner-akca
caner-akca force-pushed the issue/3225-agent-release-confirm branch 2 times, most recently from 6d3393b to 2779f16 Compare August 31, 2026 19:13
@caner-akca

Copy link
Copy Markdown
Contributor Author

Rebased onto current master (d2cb0961); the three commits are unchanged apart from the new base. The description now documents the deadlock the false exit produces and the limitations the recovery keeps.

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: clear_full_lifecycle_hook_suppression_for_detected_agent takes replacement_session_ref, marks the exited ref stale through remember_stale_full_lifecycle_hook_session, and validates the replacement. The recovery does not add a way to adopt a session identity; it reuses the one that already runs whenever an agent re-registers after a process 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 /reload can legitimately land on a different session id. Gating recovery on the ref matching the one recorded at exit would reject exactly those reloads and leave the pane deregistered, which is the bug.

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 startup, resume and compact session reports, several of which may replace session identity for the same agent through session_report_allows_session_replacement. reload is in neither that allowlist nor session_start_source_is_recognized, so it cannot decide session ownership or replace a different owner — pinned by reload_does_not_take_over_a_different_session_owner. It additionally requires the reported agent to match the currently detected agent and a recorded process exit for that same agent. A local process able to forge a reload could already forge a startup with strictly wider effect, so this path does not widen what a same-user process can reach.

Two limitations are now called out in the description rather than fixed here, to keep this change to the reported bug:

  • A name assigned with agent rename is not restored. The false exit runs clear_agent_name(), which drops the name, its owner and managed_agent; the reload restores the label, session and lifecycle authority, so pane-target lookups recover, but name-based lookup still needs a manual rename. The two-observation confirmation is what keeps named panes out of this state to begin with.
  • The recovery is agent-specific by construction, since it rides on the session start Pi emits on /reload. An agent that never re-anchors its session would need a recovery driven by process evidence instead.
@caner-akca

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@caner-akca

Copy link
Copy Markdown
Contributor Author
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
✅ Action performed

Full 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
@caner-akca
caner-akca force-pushed the issue/3225-agent-release-confirm branch from 2779f16 to eeca5c1 Compare August 31, 2026 19:29
@caner-akca
caner-akca marked this pull request as draft September 1, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Trigger automated AI reviews for pull requests admitted by the PR gate

2 participants