fix(rhai): restore the documented timeout ordering and honour a reused session's policy - #5288
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRhai workflows now use ordered timeout backstops, retain session policies across reuse, protect busy sessions from eviction, and track tool-call success or failure through call summaries. ChangesRhai execution safeguards
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RhaiEvaluation
participant RhaiToolAdapter
participant ToolResult
participant summarize_calls
RhaiEvaluation->>RhaiToolAdapter: dispatch tool call
RhaiToolAdapter->>ToolResult: execute tool
ToolResult-->>RhaiToolAdapter: return result or error
RhaiToolAdapter->>RhaiEvaluation: record outcome by call ID
RhaiEvaluation->>summarize_calls: pass outcome map
summarize_calls-->>RhaiEvaluation: return call summaries
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Comment |
…d session's policy (E-M1/E-M2) E-M1 — the README promised `inner deadline < outer backstop < harness timeout`, but `timeout_policy()` handed the harness the SAME clamped `secs` as the inner `ReplPolicy.timeout`, so the harness could win the race and drop the `run_cell` future — skipping the `RhaiError::Timeout` taxonomy, `finish_cell` accounting, `close_session`, and the outer-backstop cleanup, while the detached blocking thread kept the session mutex until the inner deadline and later calls got `SessionBusy`. The three bounds are now derived from one place: inner = `secs`, outer = `secs + 5`, harness = `secs + 10`, pinned by tests over the computed values and by the live `RhaiTool::timeout_policy()`. README updated to match. E-M2 — `get_or_create`'s build closure only runs for a FRESH session, so a reused session kept its original policy while the wrapper computed `outer_bound` and `limits_remaining` from the newly resolved one. A 300s session reused with `timeout_secs: 30` hit the 35s outer backstop while its inner deadline was still 300s, and the backstop arm calls `manager.close(&key)` — dropping the whole session and losing every binding it held. `SlotHandle` now carries the policy the session was actually built with, and every per-call bound is computed from that, warning when a caller requests a different one. The cache is deliberately NOT keyed on a policy fingerprint: that would fragment sessions and lose bindings by design rather than by bug. E-m5 — `summarize_calls` hardcoded `ok: true` because the vendor's `ReplCallRecord` carries no success flag, so a failed-but-caught capability call was reported to the model as successful. Outcomes are now tracked host-side as calls dispatch through the bridge. E-m6 — LRU eviction could evict a slot whose cell was still in flight, making `finish_cell` return `None` so `limits_remaining` reported a full budget for a session that had just done work. Busy slots are now ineligible for both the idle-TTL sweep and the cap eviction; if every slot is busy the incoming session is admitted one over cap rather than evicting a running cell. Nothing under vendor/ is modified. Worth filing upstream: a success flag on `ReplCallRecord`, and a `set_policy` on `ReplSession` (its absence is why E-M2 is fixed host-side). 42 rhai_workflows tests pass.
ed11b94 to
c70993c
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
graycyrus has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
coderabbitai
Bot
added
bug
rust-core
![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: c70993c547
ℹ️ 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".
| // would drop the whole tool-execution future, skipping the | ||
| // `RhaiError::Timeout` taxonomy, `finish_cell` accounting, | ||
| // `close_session`, and the outer-backstop session cleanup. | ||
| ToolTimeout::Secs(harness_backstop_secs(secs)) |
There was a problem hiding this comment.
When a later cell reuses a session that was created with a longer timeout_secs but supplies a shorter value, this still computes the harness deadline from the new arguments, while run_cell now computes the inner and outer deadlines from handle.policy. In the OpenHuman tool executor that harness timeout is enforced before execute can finish, so a 300s session reused with timeout_secs: 30 can be killed around the shorter harness budget while the session's own 300s deadline and 305s outer backstop are still running, again skipping finish_cell/close_session and leaving the session busy.
Useful? React with 👍 / 👎.
| None => { | ||
| tracing::warn!( | ||
| live_sessions = map.len(), | ||
| "[rhai_workflows] session cap reached but every session is in flight — \ | ||
| admitting one over cap rather than evicting a running cell" | ||
| ); |
There was a problem hiding this comment.
When all 16 slots are in flight and a new unique session_id arrives, this branch only logs and get_or_create immediately inserts the new slot; the next such call repeats, so the process-global map can grow 17, 18, ... despite the MAX_SESSIONS finite cap. In concurrent chats or background agent runs with long Rhai cells, this removes the LRU bound and can accumulate arbitrary sessions until they finish; return a cap-exceeded/busy error or add a hard overshoot limit instead of admitting every new key.
Useful? React with 👍 / 👎.
1 participant
Summary
rhai_workflows' own README promises — the three bounds had collapsed so the harness could win the race and drop the cell future mid-cleanup.summarize_callsno longer reports a failed capability call to the model asok: true.Problem
E-M1 — the layering was inverted.
README.mdstates the harnessToolTimeout::Secsbackstop sits above all other bounds and the tool's own timeout below it. In facttimeout_policy()handed the harness the same clampedsecsas the innerReplPolicy.timeout, while the outerspawn_blockingbackstop wassecs + 5. Soharness == inner < outer.When the harness won that tie it dropped the
run_cellfuture, skipping theRhaiError::Timeouttaxonomy,finish_cellaccounting,close_session, and the outer-backstopmanager.closecleanup — while the detached blocking thread kept holding the session mutex until the inner deadline, so follow-up calls gotSessionBusy.E-M2 — reused sessions ran on a stale policy while the wrapper enforced a new one.
get_or_create's build closure only runs for a fresh session, so a reused session keeps its originalReplPolicy— yetouter_boundandlimits_remainingwere computed from the newly resolved one.Concretely: a session built with 300s, reused by a cell passing
timeout_secs: 30that runs 60s, hits the 35s outer backstop while its inner deadline is still 300s. The outer-backstop arm callsmanager.close(&key)— dropping the whole session and losing all its bindings, which is exactly the "inner should fire first" invariant the code's own comment says it defends.E-m5 / E-m6 —
summarize_callshardcodedok: true(the vendor'sReplCallRecordcarries no success flag), so a failed-but-caught capability call was summarized to the model as successful; and LRU eviction could evict an in-flight slot, after whichfinish_cellreturnedNoneandlimits_remainingreported a full budget for a session that had just done work.Solution
outer_backstop_secs(inner) = inner + 5,harness_backstop_secs(inner) = outer + 5. Ordering is nowinner < outer < harnessfor every input, pinned both as a pure assertion over the computed values and through the liveRhaiTool::timeout_policy(). README rewritten in the same commit to match.SlotHandlecarries the policy its session was built with, and every per-call bound is computed from that, warning when a caller requests a different one. The session cache is deliberately not keyed on a policy fingerprint: that would fragment sessions and lose bindings by design rather than by bug.summarize_callsreports real success/failure.Nothing under
vendor/is modified. Two items are worth filing upstream instead: a success flag onReplCallRecord, and aset_policyonReplSession— the absence of the latter is precisely why E-M2 is fixed host-side.Submission Checklist
cargo test --lib openhuman::rhai_workflows= 42 passed, 0 failedN/A: behaviour-only bug fix, no feature rows added/removed/renamed## Related—N/A: no matrix feature rows affectedN/A: agent-tool internal; the rhai tool is orchestrator-surface only (ToolScope::AgentOnly)Closes #NNN—N/A: found by code review, no tracking issue filed yetImpact
openhuman::rhai_workflowsonly.RhaiError::Timeoutwith accounting and cleanup instead of a dropped future and a wedged mutex. A reused session with a differing requested policy keeps its bindings and logs a warning rather than being destroyed.Related
N/Atinyagents— add a success flag toReplCallRecordand aset_policytoReplSession.src/openhuman/rhai_workflows/, no overlap with the other PRs in this batch.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/rhai-timeout-and-session-policyed11b94ecValidation Run
pnpm --filter openhuman-app format:check— N/A, no frontend files changedpnpm typecheck— N/A, no TypeScript changedGGML_NATIVE=OFF cargo test --lib openhuman::rhai_workflows→ 42 passed, 0 failedGGML_NATIVE=OFF cargo check --manifest-path Cargo.tomlcleanapp/src-tauriuntouchedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
SessionBusy.Parity Contract
timeout_is_always_boundedand session-isolation tests still pass unmodified in intent.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Reliability
Documentation