fix(memory/sources): size ingest RPC budget for multi-window sessions - #5531
Conversation
The `ingest_coding_sessions` RPC computed its wall-clock ceiling as `120 + min(max_sessions, 1000) * 30` on the premise that "each session drives at most one LLM call". That premise is false: TinyCortex's persona pipeline splits an oversized session into `WINDOW_CHARS`-sized windows and issues one LLM call per window, so a multi-window session drives several sequential calls. A dense backfill blew the ceiling — 15 sessions hit the exact 570 s budget (`120 + 15*30`) and were killed mid-flight. Extract the computation into a pure `ingest_budget(max_sessions)` and size the per-session allowance for multiple windows (`PER_SESSION_SECS = 120`, ~4 sequential calls) rather than one, so a legitimate backfill runs to completion while a genuine infinite hang still terminates. The untrusted `max_sessions` cap (1000) is preserved, and the false comment is corrected. Adds unit tests for the new formula, including the 15-session regression and the cap. `ingest_budget` takes `usize` to match the request field type. This is the RPC-timeout half of tinyhumansai#5509. The digest-truncation half lives in tinycortex (retain observations when a digest is truncated); the vendored tinycortex submodule pointer is bumped in a follow-up once that lands. Part of tinyhumansai#5509
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe ingestion client now processes five sessions per batch with a 585-second timeout. The RPC calculates a per-session budget of 90 seconds plus a 120-second base, capped at 600 seconds. Tests cover scaling, overflow-safe capping, timeout ordering, and repeated drain passes. ChangesIngestion timeout scaling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The PR widens the bounded ingest timeout to support legitimate multi-window sessions without changing the API or permissions. No actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 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 |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0078 · 13,742 in / 2,050 out · 9,048 cached (66%) · z-ai/glm-5.2
critique: $0.0022 · 3,253 in / 831 out · 2,333 cached (72%) · z-ai/glm-5.2
security: $0.0023 · 3,232 in / 187 out · 896 cached (28%) · z-ai/glm-5.2
tests: $0.0021 · 3,147 in / 805 out · 2,366 cached (75%) · z-ai/glm-5.2
description: $0.0011 · 4,110 in / 227 out · 3,453 cached (84%) · z-ai/glm-5.2
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Review — comment only (no approval)Reviewed F1 — the client aborts at 585 s, 15 s after the old server ceilingThis raises the 15-session server ceiling 570 s → 1920 s. The only shipped caller imposes its own bound on the same call — CODING_SESSION_BATCH_MAX = 15;
CODING_SESSION_BASE_TIMEOUT_MS = 120_000;
CODING_SESSION_PER_SESSION_TIMEOUT_MS = 30_000;
CODING_SESSION_RPC_GRACE_MS = 15_000;
// timeoutMs = 120s + 15×30s + 15s = 585sThat is a mirror of the old server formula plus a grace, and the comment right above it ( #5509 reports
Net gain ≈ 15 s; the other 1335 s is never observed. And it can't be recovered by raising the client constants either — So #5509's acceptance criterion — "bulk ingest of 15 large Codex sessions completes without hitting the RPC timeout ceiling" — still isn't met. What would close it: raise the client constants and cut Counter-argument, for the record: the client abort is an F2 — the
|
… ceiling Address review on tinyhumansai#5531: the server-only budget raise did not reach the client. The frontend RPC client clamps every per-call timeout to PER_CALL_TIMEOUT_MAX_MS = 600s (coreRpcClient.ts), and memorySourcesService bounds the same call at 120s + 15*30s + 15s = 585s — a mirror of the *old* server formula. So raising the server ceiling to 1920s only moved the binding constraint from the server's 570s to the client's 585s (~15s gain), and any budget above 600s is unreachable by construction. Fix the real constraint on both sides of the wire so a pass fits under the 600s ceiling and large histories drain across passes: - Server (rpc.rs): size ingest_budget at 120 + N*90s (an honest multi-window estimate matching the 20-45s/window observed in tinyhumansai#5509) and hard-cap the Duration at 600s. The Duration cap replaces the multiplier cap, so an untrusted max_sessions=1000 can no longer pin the blocking worker for ~33h. - Client (memorySourcesService.ts): drop CODING_SESSION_BATCH_MAX 15 -> 5 and raise per-session 30s -> 90s, so a pass is 120 + 5*90 + 15 = 585s < 600s; drainCodingSessions already iterates passes (2000-pass cap covers ~10k sessions). Correct the stale "120s + 15*30s" comment. - Keep the server budget the tighter of the two (570s < client 585s) so the server returns a clean structured timeout before the client's fetch aborts. Tests: server_budget_is_reachable_and_tighter_than_the_client pins the cross-wire invariant (server <= client <= 600s) that would have caught the server-only gap; budget_is_capped_at_the_reachable_ceiling pins the cap; the client test guards timeoutMs <= 600s. Part of tinyhumansai#5509
|
Verified all three findings against the code and you're right — F1 is the load-bearing one. Reworked in F1 — reachable ceiling. Confirmed:
F2 — the cap. Fixed by capping the resulting F3 — the derivation. Dropped the "4 calls × 30s" figure; On "pin the invariant that actually broke" — done, and this is the part I'd most like your eyes on. Added One deliberate divergence from your framing. You leaned toward the server being the looser of the two (client aborts, server grinds on invisibly past it and the next pass finds more). I kept the server tighter (570 < 585) on purpose: with the batch now sized to fit under 600s, I'd rather the server hit its ceiling first and return a clean structured Unrelated CI note. The red |
coderabbitai
Bot
added
the
memory
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0558 · 29,733 in / 18,526 out · 17,405 cached (59%) · z-ai/glm-5.2
critique: $0.0261 · 9,762 in / 9,528 out · 6,442 cached (66%) · z-ai/glm-5.2
security: $0.0054 · 8,570 in / 1,386 out · 7,112 cached (83%) · z-ai/glm-5.2
tests: $0.0119 · 5,177 in / 4,360 out · 3,851 cached (74%) · z-ai/glm-5.2
description: $0.0125 · 6,224 in / 3,252 out · 0 cached (0%) · z-ai/glm-5.2
| // Every pass stays bounded to the timeout-safe per-call maximum. | ||
| expect(mockedCall).toHaveBeenLastCalledWith( | ||
| expect.objectContaining({ params: { backfill: false, max_sessions: 15 } }) | ||
| expect.objectContaining({ params: { backfill: false, max_sessions: 5 } }) |
There was a problem hiding this comment.
Align mock returns and pass count with the new max_sessions cap of 5
The test now asserts each call sends max_sessions: 5, but still expects 40 sessions processed in 3 passes (// 15 + 15 + 10). If max_sessions is a per-call batch cap — which is the entire purpose of clamping it to 5 for timeout safety (120s + 5*90s + 15s = 585s) — then 3 passes can process at most 15 sessions, not 40. The mock evidently returns ~15 per call regardless of the param, so the test passes but exercises a scenario the real backend (respecting max_sessions: 5) could never produce. The // 15 + 15 + 10 comment is now stale. If a future maintainer makes the mock realistic (return ≤5 per call), the passes: 3 and sessionsProcessed: 40 assertions will break. Confidence: 0.65 (the mock setup is not shown in the diff; inferred from the comment and totals).
[RULE] Tests must assert behavior that can actually occur ·
What this change touches3 files, +142 -20 across 2 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src/openhuman/memory/sources<br/>1 file +116 -6"]:::changed
n1["app/src/services<br/>2 files +26 -14<br/>1 finding"]:::flagged
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
priority: p2
2 participants
Summary
ingest_coding_sessionsRPC wall-clock ceiling so a legitimate multi-session backfill is not killed mid-flight.120 + N×30) assumed one LLM call per session; TinyCortex's persona pipeline splits an oversized session into windows and issues one LLM call per window, so a multi-window session drives several sequential calls and blows the budget.ingest_budget(max_sessions)and correct the false comment.Problem
Reported in #5509 (Bug 1).
src/openhuman/memory/sources/rpc.rscomputed the RPC timeout as120 + min(max_sessions, 1000) × 30on the premise, stated in its own comment, that "each session drives at most one LLM call." That premise is false:digest_sessionsplits each session intoWINDOW_CHARS-sized windows and fires one LLM call per window (each observed at 20–45 s). A dense backfill of 15 sessions therefore hit the exact120 + 15×30 = 570 sceiling and was killed mid-flight, dropping the remaining sessions.Solution
ingest_budget(max_sessions)— a pure function sized for multiple windows per session (PER_SESSION_SECS = 120, ~4 sequential per-window calls) instead of one. A healthy backfill now finishes well inside the ceiling while a genuinely wedged run still terminates.MAX_SESSIONS_FOR_BUDGET = 1000) so an inflatedmax_sessionscannot turn the ceiling into an effectively-infinite wait.ingest_budgettakesusizeto match the request field type (CodingSessionIngestRequest::max_sessions).This is the RPC-timeout half of #5509. The digest-truncation half (
DIGEST_MAX_OUTPUT_TOKENSraise + non-committable truncated windows) lives in TinyCortex — tinyhumansai/tinycortex#145. That half is what makes the extra per-window calls succeed; the vendoredvendor/tinymemory→tinycortexsubmodule pointer is bumped in a follow-up PR once #145 merges. #5509 should be closed by that follow-up (both halves present), not by this PR alone.Submission Checklist
budget_tests: the 15-session regression (proves the new ceiling exceeds the old 570 s) and the untrusted-max_sessionscap.ingest_budgetfunction (the only changed executable lines).N/A: behaviour-only change(timeout-budget arithmetic; no new feature row).## Related—N/A: no matrix feature touched.N/A: internal RPC timeout budget only.Closes #NNN— intentionally not closing here; see## Related(needs the digest half + submodule bump).Impact
Related
vendor/tinymemory/tinycortexsubmodule pointer once [Feature] Subconscious loop: local-model state review (replaces heartbeat) #145 merges → that PR carriesCloses #5509.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Summary by CodeRabbit