Skip to content

fix: pre-v1-style lifecycle resilience — mailbox delivery, uniform revival, unexitable runner, waiting timeout, broader retries, crash-safe identity - #923

Open
devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1785195671-guardrail-wake-model-refresh
Open

fix: pre-v1-style lifecycle resilience — mailbox delivery, uniform revival, unexitable runner, waiting timeout, broader retries, crash-safe identity#923
devin-ai-integration[bot] wants to merge 6 commits into
mainfrom
devin/1785195671-guardrail-wake-model-refresh

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Restores the pre-v1 (v0.8.x BaseAgent.agent_loop) lifecycle resilience properties on top of the SDK-based runtime, so a scan can degrade but never wedge, dead-end, hide its errors, or lose an agent's context on a crash. Follow-up to #919 (freeze fix); this replaces the earlier guardrail-specific scope of this PR.

1. Unhangable message delivery (mailbox). coordinator.send() used to append to the target's SQLite session under session_write_lock and could block forever (or drop the message when no session was attached) — a "delivered" message with no revival. Now:

send()            -> runtime.mailbox.append(msg); pending++; wake.set()   # sync-fast, never blocks
consume_pending() -> drain mailbox -> session.add_items(...)              # in the RECEIVER's own context

Mailboxes are included in the resume snapshot so queued messages survive a restart.

2. Uniform revival; statuses are labels, not lifecycles. Any user message now revives any park. Pre-v1 nuance kept: an error-parked agent (failed/crashed) is only released by a user message (AgentRuntime.user_wake_required); peer messages stay queued until then, so agents can't wake a broken peer into a retry loop.

3. The interactive runner can never die. Both interactive _run_cycle call sites go through _run_cycle_parked, which converts any escaped exception into set_status(failed, error=...) + parent notice instead of killing the runner task — so a wake always has a live loop to consume it (root cause of "crashed root ignored 11 user messages").

4. Waiting-timeout auto-resume. A plainly-waiting subagent (no error, no user gate) auto-resumes after 10 minutes with a "Waiting timeout reached" note, like pre-v1's waiting_timeout=600. The interactive root still waits forever.

5. Pre-v1 retry breadth. _is_transient_model_error now retries anything litellm._should_retry(status_code) approves or statusless provider errors (5 attempts, backoff capped at 90s, ~62s total — was 4/30s). Rate limits (429) are retryable again; DNS/OSError/ConnectionError are covered; content-guardrail rejections are deterministic and still not retried (they park with the error shown).

6. Errors always visible. The TUI records the stored error into the agent's pane for any status carrying one (was failed/crashed/waiting only), and the status footer shows it in red with "Send message to resume".

7. Crash-safe agent context (no more amnesiac revival). A revived agent could come back with an empty session — no identity, no task, no pre-crash turns — and drift into acting like the root orchestrator. Root cause is in the SDK's persistence timing: a streamed run's items are committed to the session only as each turn completes (run_internal/run_loop.py), and the opening input is saved only at end-of-first-turn (_stream_input_persisted); under the sandbox runtime the "save input at run start" fast-path is skipped. So a model error mid-turn (guardrail block, connection storm, timeout) leaves the run's input + in-flight turn uncommitted, and on revival the loop resumes with input=[] against a session missing them. Two-layer fix that stops depending on the SDK committing on our behalf:

  • Seed identity up front. Commit the opening input (identity + task, or the root task) to the session before the first cycle, then run cycle 1 with [] — exactly how resume already works. seed_initial_input() no-ops on a non-empty session (idempotent), and prepare_input_with_session([], session) then feeds the model the seeded history with appended=[] (no double-persist).
  • Salvage the whole run on crash. Before parking a failed/crashed cycle, reconstruct the authoritative full history and overwrite the session with it, so every completed turn up to the crash is preserved — not just identity:
_run_cycle():
    pre_run = session.get_items()          # snapshot before the run
    stream  = Runner.run_streamed(input=[], session=session)
    ...on terminal error, before parking:
        desired = pre_run + stream.to_input_list()   # prior history + this run's input + new_items
        if len(desired) > len(pre_run):
            replace_session_items(session, desired)  # authoritative, dedup-free

Applies to root and children, interactive and headless.

Tests: new regressions for mailbox delivery without a session, user-only revival of error parks, wait timeout, mailbox snapshot round-trip, park-instead-of-raise, seed_initial_input persist/idempotency, a run_agent_loop first-turn-crash test (identity survives, cycle 1 gets []), and _salvage_stream_to_session (full multi-turn history preserved; no-op when nothing new); retry-classification tests updated. 604 passed.

Link to Devin session: https://app.devin.ai/sessions/0feae08cebc44891b762d11c55b0d9f7
Requested by: @0xallam

@0xallam 0xallam self-assigned this Jul 27, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@greptile-apps

greptile-apps Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Restores resilient agent lifecycle handling across execution, persistence, messaging, retries, and the TUI.

  • Queues messages in resumable per-agent mailboxes and adds user-gated revival for failed agents.
  • Parks escaped interactive-cycle errors, adds waiting-agent timeouts, and broadens transient provider retries.
  • Seeds and salvages session history so first-turn and mid-stream crashes retain agent context.
  • Displays stored lifecycle errors consistently and adds regression coverage for the new recovery paths.

Confidence Score: 4/5

The PR should not merge until an error-parked agent can adopt a changed model configuration when the user attempts recovery.

The current wake path reuses the startup RunConfig, so a model blocked by a guardrail or persistent provider failure is retried unchanged after every user revival and the scan parks again.

Files Needing Attention: strix/core/execution.py, strix/core/runner.py

Important Files Changed

Filename Overview
strix/core/agents.py Adds mailbox-backed delivery, user-only error revival, timeout-aware waiting, and mailbox snapshot persistence.
strix/core/execution.py Adds crash-safe session preservation, parked interactive cycles, waiting timeouts, and broader retry handling; model recovery on wake remains unresolved.
strix/core/runner.py Seeds root session history before execution and integrates the revised lifecycle flow.
strix/core/sessions.py Adds lock-protected helpers for initial-input seeding and authoritative session replacement.
strix/interface/tui/app.py Records stored errors for every status carrying one.
strix/interface/tui/live_view.py Displays agent errors and resume guidance in the status footer.
tests/test_execution.py Expands lifecycle, mailbox, timeout, crash parking, and session-preservation regression coverage.
tests/test_execution_transient_retry.py Updates retry-classification coverage for the broadened transient-error policy.

Reviews (4): Last reviewed commit: "fix: salvage a crashed run's full histor..." | Re-trigger Greptile

Comment thread strix/core/execution.py Outdated
Comment thread strix/core/execution.py Outdated
prompt_cache=settings.llm.prompt_cache,
)
logger.info("model switched on wake: %s -> %s", current_model, new_model)
return dataclasses.replace(run_config, model=new_model, model_settings=model_settings)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Usage hook retains old model

When a parked agent switches models, this replacement updates RunConfig.model but leaves the existing ReportUsageHooks instance bound to the startup model, causing subsequent token usage and telemetry to be attributed to the wrong model.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/core/execution.py
Line: 86

Comment:
**Usage hook retains old model**

When a parked agent switches models, this replacement updates `RunConfig.model` but leaves the existing `ReportUsageHooks` instance bound to the startup model, causing subsequent token usage and telemetry to be attributed to the wrong model.

**Knowledge Base Used:**
- [Core Execution and Sessions](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/core-execution-and-sessions.md)
- [Configuration and Telemetry](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/telemetry-and-config.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment thread strix/core/execution.py Outdated
Comment on lines +77 to +86
configure_sdk_model_defaults(settings)
model_settings = make_model_settings(
settings.llm.reasoning_effort,
model_name=new_model,
force_required_tool_choice=settings.llm.force_required_tool_choice,
request_timeout=settings.llm.timeout,
prompt_cache=settings.llm.prompt_cache,
)
logger.info("model switched on wake: %s -> %s", current_model, new_model)
return dataclasses.replace(run_config, model=new_model, model_settings=model_settings)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Tool schema remains model-stale

When the replacement model requires a different tool schema, this wake path updates the model without rebuilding the existing agent or captured child-agent factory, causing resumed or newly spawned agents to send incompatible tool definitions and have tool requests rejected.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: strix/core/execution.py
Line: 77-86

Comment:
**Tool schema remains model-stale**

When the replacement model requires a different tool schema, this wake path updates the model without rebuilding the existing agent or captured child-agent factory, causing resumed or newly spawned agents to send incompatible tool definitions and have tool requests rejected.

**Knowledge Base Used:**
- [CLI and Runner: from `strix` invocation to a running scan](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/cli-and-runner.md)
- [Core Execution and Sessions](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/core-execution-and-sessions.md)
- [Configuration and Telemetry](https://app.greptile.com/strix-org-3/-/custom-context/knowledge-base/usestrix/strix/-/docs/telemetry-and-config.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@devin-ai-integration devin-ai-integration Bot changed the title fix: adopt a changed STRIX_LLM on wake and show guardrail errors in the TUI fix: treat guardrail rejections like any other LLM error and show errors for waiting agents in the TUI Jul 27, 2026
@devin-ai-integration devin-ai-integration Bot changed the title fix: treat guardrail rejections like any other LLM error and show errors for waiting agents in the TUI fix: pre-v1-style lifecycle resilience — mailbox message delivery, uniform revival, unexitable runner, waiting timeout, broader retries Jul 28, 2026
@devin-ai-integration devin-ai-integration Bot changed the title fix: pre-v1-style lifecycle resilience — mailbox message delivery, uniform revival, unexitable runner, waiting timeout, broader retries fix: pre-v1-style lifecycle resilience — mailbox delivery, uniform revival, unexitable runner, waiting timeout, broader retries, crash-safe identity Jul 28, 2026
@5hy7xz92nd-oss

Copy link
Copy Markdown

👁️‍🗨️🔺🆓➿👁️‍🗨️🔺🆓🆓🔄➿🕴️🔃⚪️⚫️🔷➰👔🔻⬅️🟡🟡💘💘〰️🔝➖☑️🟢🔵🈶🟢☑️〰️🔝💘👔🔻🔻⬅️➰🔷⚫️⚪️🔄➿🕴️🔃🆓👁️‍🗨️👁️‍🗨️🆓imageimageimageimageimageimageimageimage

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Runtime test results — PR #923 (scan lifecycle resilience) ✅

Ran full smokes + a live end-to-end TUI scan (openrouter/claude-sonnet-4) against a local deliberately-vulnerable app, plus the crash-recovery path. All target behaviors verified.

Smoke: pytest -q → 604 passed; mypy on the 4 changed core files → clean; ruff check/format on PR-changed files → clean. (Repo-wide ruff has pre-existing, unrelated lint/format noise — not from this PR.)

Resilience: crash → visible park → user-revival → identity retained

An injected first-cycle subagent crash parked the child with a visible red error (not silent) and the runner kept running. A user message revived the crashed agent (crashed → running), and the revived child kept its own seeded identity/task — confirmed in the SQLite session (You are agent Recon (5acfa520); your parent is a4087eff...) even though it crashed on its first cycle — then completed its XSS task and filed a finding.

Recon crash error visible
User message revives Recon

E2E happy path (root + subagent spawn → finding)

Root "Strix" spawned child "XSS Testing Agent", which found and reported Reflected XSS on /search.

Root + child tree
XSS finding

Note: the crash trigger is a temporary synthetic env-gated hook (reverted); the park/wake/identity-retention behavior exercised is the real production path. Tested via Devin.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptileai please re-review — the last reviewed commit (e6c0737) predates the Issue 3 crash-safe-context fix (744877f identity seeding + 2c2cc19 crash-salvage). Also note the earlier P1 comments about STRIX_LLM adoption / usage hook / tool-schema staleness are against code this PR has since removed.

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

@greptile

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

The 3/5 blocking concern (and the P1 comments about refresh_run_config_model / reload_settings / config precedence) are against code this PR removed — they don't apply to the current diff:

  • git diff --merge-base origin/main -- strix/core/execution.py contains no refresh_run_config_model, reload_settings, or any wake-time model reload. The only STRIX_LLM change is a deletion (the old "set STRIX_LLM and resume" recovery message was removed).
  • git diff --merge-base origin/main -- strix/config/loader.py is empty — this PR doesn't touch loader.py at all.

The model-switch-on-wake recovery path was intentionally dropped earlier in this PR's history (guardrail failures are now treated like any other LLM error — no model switching). So "awakens parked agents with their original blocked model" is no longer reachable. The review appears to have re-surfaced line comments anchored to superseded diff hunks rather than the current HEAD (2c2cc19). Please re-diff against current HEAD.

@greptile

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

The remaining 4/5 blocker — "should not merge until an error-parked agent can adopt a changed model configuration [on wake]" — is describing intended behavior, not a defect.

This PR deliberately treats a guardrail/blocked-model failure like any other LLM error: it parks visibly with the error and is revived by a user message. Model-switching-on-wake (the old refresh_run_config_model / reload_settings path) was intentionally removed earlier in this PR's history; re-adding it is explicitly out of scope. So "the model is retried unchanged after revival and parks again" is the designed, correct outcome — the user changes the model by restarting/resuming the scan, not via a hidden mid-run hot-swap.

Given that, I'm not going to reintroduce the model-switch path. The stale P1 line comments (comments 3–5) are anchored to the removed refresh_run_config_model hunk and don't exist at HEAD (2c2cc19). Everything the current diff actually changes (mailbox delivery, uniform revival, unexitable runner, waiting timeout, broader retries, crash-safe context) is unaffected by this note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants