[fix] Bound the session-records fetch and consume the persist-drop signal - #5501
Conversation
Two small robustness gaps in the reconstruction pipeline: - fetchSessionRecords sits on the turn's critical path (the prompt waits on it), but had no timeout, so a stalled query would wait on Undici's long request-level timeout instead of failing fast to the inbound-history fallback. Add an env-tunable AbortSignal.timeout (5s default). - takePersistFailures had no caller: in durable mode a dropped record was counted into a per-session map that nothing read, so the signal was lost and the map grew unbounded. Consume it at the turn-end drain (flush) — warn when the durable log is incomplete and clear the counter.
dosubot
Bot
added
the
size:S
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRunner persistence adds bounded environment-configured retries and turn-end dropped-record warnings. Shared environment helpers clamp numeric and timer values. Session record queries gain abort timeouts, and sandbox run limits normalize timer delays. ChangesSession reliability
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SessionTurn
participant buildPersistingEmitter
participant drainPersist
participant takePersistFailures
participant stderr
SessionTurn->>buildPersistingEmitter: flush()
buildPersistingEmitter->>drainPersist: await pending durable records
drainPersist-->>buildPersistingEmitter: drain complete
buildPersistingEmitter->>takePersistFailures: consume session drop count
takePersistFailures-->>buildPersistingEmitter: dropped count
buildPersistingEmitter->>stderr: emit WARN when count > 0
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Railway Preview Environment
|
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 452202da-8419-4099-beb1-53772089ca5d
📒 Files selected for processing (3)
services/runner/src/sessions/persist.tsservices/runner/src/sessions/records-query.tsservices/runner/tests/unit/session-persist.test.ts
The records-fetch timeout added in this branch could resolve to a value that aborts the request instantly — the opposite of what the bound is for. AGENTA_SESSIONS_RECORDS_QUERY_ TIMEOUT_MS=0.5 passes the `> 0` check, truncates to 0, and AbortSignal.timeout(0) rejects before the query completes, silently pinning reconstruction to the inbound-history fallback. The same read had no ceiling either: above 2^31-1 Node's timer overflows to a 1ms delay, and above 2^32-1 AbortSignal.timeout throws outright. That read was the fourth hand-rolled numeric-env parser in the runner, each encoding a different partial idea of what is valid, and every failure mode degrades toward no protection at all. State the domain once instead: src/env.ts reads a name as an integer inside [min, max], truncating before clamping (so a fractional value cannot sneak through as 0) and capping timer delays at Node's 32-bit ceiling. Unset stays silent; set-but-unusable falls back or clamps and warns once, since a typo in one env var must not break every run in the process. Routed through it: - records-query: the timeout above, floor and ceiling both. - persist: OPEN_TOOL_TTL_MS carried the same hole in NaN form — junk parsed to NaN and setTimeout(fn, NaN) fires at 1ms, collapsing the tool-call coalescing window this file exists to maintain. durableMaxRetries gains a ceiling, since each extra attempt doubles the worst-case turn-end drain. - run-limits: drops its private envMs, which accepted 0 (a deadline that trips immediately) and had no integer or overflow bound. Tests: env-num covers each input regime; session-records-query pins the bound at the call site, which had no coverage at all. Runner unit suite 1277 green, typecheck clean.
dosubot
Bot
added
size:L
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/runner/src/sessions/persist.ts (2)
86-120: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound ingest request timeouts separately from retry attempts.
drainPersist()awaits the in-flight persist chain, but thefetchhere has no abort signal. A never-settling request can hold the chain indefinitely instead of hitting the retry loop, so use a per-attempt deadline such asAbortSignal.timeout(...)for each fetch and add coverage for a fetch that never settles.
84-96: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMake every persisted ingest idempotent.
Only
tool_call,tool_result,interaction_request,interaction_response, and persisted out-of-band records get a stablerecord_id; coalescedmessageandthoughtrecords do. When retries resend without that ID, the documented upsert contract cannot protect the record. Send a deterministic record ID for all retried events, or add database/proxy deduplication based onsession_id+record_indexbefore relying on the lost-response durability guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80ab041d-3d19-4ab9-be4d-593eae25c34d
📒 Files selected for processing (6)
services/runner/src/engines/sandbox_agent/run-limits.tsservices/runner/src/env.tsservices/runner/src/sessions/persist.tsservices/runner/src/sessions/records-query.tsservices/runner/tests/unit/env-num.test.tsservices/runner/tests/unit/session-records-query.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/runner/src/sessions/records-query.ts
resolveRunLimits derives `idle` as half the total when a misconfigured idle would otherwise be unreachable. envTimerMs guarantees each limit is at least 1ms, but half of a total sitting at that floor rounds to 0, and a 0ms idle timer trips the run the moment it is armed — the guard this branch just added, escaping through a value the env reader never saw. Guarding the derivation would leave the next one exposed, so the contract moves onto the output: ResolvedRunLimits now states that every field is armable, and resolveRunLimits normalizes the whole record before returning it, which covers any future cross-limit adjustment for free. clampTimerMs is the shared primitive for that — env.ts already owned the domain for reads, and now exposes it for delays the code computes afterwards (a fraction of another limit, a backoff, a remaining budget). envInt clamps through the same path, so the domain has one definition rather than two that can drift. While wiring it: ±Infinity now clamps to the matching end of the range instead of the floor. NaN is the only input with no magnitude to clamp toward, and an infinite deadline collapsing to "fires instantly" is the exact inversion this is meant to prevent. Tests: the degenerate-total case is pinned in run-limits (it fails with idleMs=0 without the normalization), clampTimerMs is covered directly. Runner unit suite 1281 green, typecheck clean.
2 participants
Context
Two small robustness gaps in the last-message-only reconstruction pipeline (#5486), separate from the QA-finding PRs (#5488-#5491, #5494).
Changes
fetchSessionRecordssits on the turn's critical path (the prompt waits on it), so a stalled query would hang on Undici's long request-level timeout instead of falling back to the inbound history. Added an env-tunableAbortSignal.timeout(AGENTA_SESSIONS_RECORDS_QUERY_TIMEOUT_MS, 5s default).takePersistFailureshad no caller: in durable mode a dropped record was counted into a per-session map that nothing read, so the signal was lost and the map grew unbounded. Now consumed at the turn-end drain (flush) — it warns when the durable log is incomplete and clears the counter.Tests
flushnow consumes the count.Note
Neither is a QA finding; both surfaced while reviewing the reconstruction path. Independent of the other stacked PRs — merges into #5486 in any order.