perf(reborn): parallelize cold identity-context reads - #5300
Closed
henrypark133 wants to merge 3 commits into
Closed
henrypark133 wants to merge 3 commits into
henrypark133 wants to merge 3 commits into
Conversation
`WorkspaceIdentityContextSource::load_identity_candidates` read the 5-7 identity files (SOUL/AGENTS/IDENTITY/TOOLS/BOOTSTRAP + USER/assistant- directives) in a serial `for` loop — one `read_primary` -> `get_document_by_path` SQL round-trip each, awaited one-by-one, hits and misses alike. On iteration 0 there is no `IdentityCandidateCache` hit, so every cold turn paid N serial cross-region round-trips against the hosted Postgres backend (~100-200ms/RTT) before the provider call. Replace the two serial loops with `futures::future::try_join_all` over an ordered candidate list, collapsing N round-trips to ~1 while preserving the deterministic candidate order. Each read is a self-contained query on its own pooled connection with no shared transaction, so concurrent dispatch is safe on all backends (postgres pool=30, libsql per-call connection, in-memory trivially independent). Add a backend-agnostic guardrail test (`ConcurrencyProbeDb` instruments the `Database` boundary and asserts max-in-flight >= 2) that failed on the serial base and passes after the fix, plus a `.claude/rules/database.md` invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code-review findings on the identity-parallel-read change: - Add `load_identity_candidates_propagates_read_failure`: proves a non- DocumentNotFound store read failure still surfaces as `HostIdentityContextBuildError::SourceUnavailable` through the concurrent `try_join_all` (its first-error short-circuit differs from the old serial loop, but the observable error contract is unchanged). Adds a `ConcurrencyProbeDb::new_failing` fail-injection variant. - Add `src/workspace/**` to `.claude/rules/database.md` `paths` so the new concurrent-read invariant auto-loads at its own canonical call site. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5300
June 26, 2026 06:16
Destroyed
github-actions
Bot
added
scope: workspace
📝 WalkthroughSummary by CodeRabbit
WalkthroughWorkspace identity candidate loading now reads independent paths concurrently. The database rule file now includes workspace sources, concurrent per-key read ordering, and test coverage guidance. New tests probe parallelism and failure propagation. ChangesWorkspace identity parallel reads
Sequence Diagram(s)sequenceDiagram
participant WorkspaceIdentityContextSource
participant try_join_all
participant WorkspaceStore
participant Database
WorkspaceIdentityContextSource->>try_join_all: build candidate reads for stable and personal paths
try_join_all->>WorkspaceStore: candidate_for_path(path) for each entry
WorkspaceStore->>Database: get_document_by_path(path)
Database-->>WorkspaceStore: document / DocumentNotFound / error
WorkspaceStore-->>try_join_all: candidate or SourceUnavailable
try_join_all-->>WorkspaceIdentityContextSource: flattened candidate list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/database.md:
- Line 6: The scope text in the database rule is inconsistent with the new
`src/workspace/**` matcher: update the prose under this rule so it explicitly
includes workspace callers, not just the legacy `src/db/`, `src/history/`, and
`migrations/` paths. Keep the wording in `.claude/rules/database.md` aligned
with the actual scope list so the guardrail matches what `src/workspace/**` is
meant to cover.
- Around line 58-82: Scope the concurrency guidance in database.md to fixed,
small fan-outs only, and exclude user-driven batch reads from the blanket
“independent records” rule. Update the wording around the load path examples and
the references to load_identity_candidates so the rule explicitly requires
bounded concurrency (for example via a semaphore or limited join strategy) when
the number of reads can grow with user input, while keeping
try_join_all/join_all guidance for known-small sets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 93709cf1-7d7e-4855-9fd5-c295b561e167
📒 Files selected for processing (3)
.claude/rules/database.mdsrc/workspace/reborn_identity_context.rstests/reborn_identity_parallel_read.rs
| - "src/db/**" | ||
| - "src/history/**" | ||
| - "migrations/**" | ||
| - "src/workspace/**" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the scope prose below.
Line 6 brings src/workspace/** under this rule, but the prose below still says it only loads for legacy src/db/, src/history/, and migrations/ paths. That leaves the guardrail self-contradictory for the workspace callers this PR is trying to cover.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/database.md at line 6, The scope text in the database rule is
inconsistent with the new `src/workspace/**` matcher: update the prose under
this rule so it explicitly includes workspace callers, not just the legacy
`src/db/`, `src/history/`, and `migrations/` paths. Keep the wording in
`.claude/rules/database.md` aligned with the actual scope list so the guardrail
matches what `src/workspace/**` is meant to cover.
Comment on lines
+58
to
+82
| ## Independent per-record reads must be concurrent, never a serial loop | ||
|
|
||
| When a caller fetches N *independent* records by key (no shared transaction, | ||
| no read-after-write ordering between them) — e.g. loading a fixed set of | ||
| identity/config files, or hydrating several entities by id — issue the reads | ||
| CONCURRENTLY (`futures::future::try_join_all` / `join_all`), never in a serial | ||
| `for path in … { store.get(path).await? }` loop. | ||
|
|
||
| Each `get_document_by_path`-style call is one self-contained query on its own | ||
| pooled connection (postgres: `self.conn().await?` per call; libsql: | ||
| `self.connect().await?` per call) and returns `DocumentNotFound` on a miss — | ||
| so a serial loop pays one full round-trip *per record, including misses*. | ||
| Against the hosted cross-region Postgres backend (~100-200 ms/RTT) that turned | ||
| 5-7 cold identity reads into multiple seconds of pre-provider latency on every | ||
| cold turn; concurrent dispatch collapses them to ~1 RTT. The pattern is | ||
| backend-agnostic and safe on all three backends (postgres pool=30 ≫ N, libsql | ||
| per-call connection, in-memory trivially independent — in-memory just saves | ||
| ~0 wall-clock). | ||
|
|
||
| Preserve deterministic output order (`try_join_all` keeps input order). Guard | ||
| the invariant with a `ConcurrencyProbeDb`-style test that instruments the | ||
| `Database` boundary and asserts max-in-flight ≥ 2 — see | ||
| `tests/reborn_identity_parallel_read.rs` and | ||
| `src/workspace/reborn_identity_context.rs::load_identity_candidates`. | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Scope this concurrency rule to bounded fan-outs.
“N independent records by key” is too broad now that this rule also loads for src/workspace/**. Full fan-out is right for fixed sets like the 5–7 identity files here, but user-sized batches need bounded concurrency rather than an unbounded read storm.
As per coding guidelines, “Tokio task fan-out — in-flight dedup or bounded semaphore on spawns driven by user input.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/rules/database.md around lines 58 - 82, Scope the concurrency
guidance in database.md to fixed, small fan-outs only, and exclude user-driven
batch reads from the blanket “independent records” rule. Update the wording
around the load path examples and the references to load_identity_candidates so
the rule explicitly requires bounded concurrency (for example via a semaphore or
limited join strategy) when the number of reads can grow with user input, while
keeping try_join_all/join_all guidance for known-small sets.
Source: Coding guidelines
|
🚅 Deployed to the ironclaw-pr-5300 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5300
June 26, 2026 16:29
Destroyed
Member
|
Closing this because it updates the legacy top-level Thanks for the contribution. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A 30-byte Reborn reply took 21.7s on an idle, zero-concurrency turn. 3.79s of that was spent assembling the model request after the capability surface was ready and before the provider call:
04:49:56.561ironclaw_agent_loop::executor::prompt: "agent loop prompt capability surface prepared" (crates/ironclaw_agent_loop/src/executor/prompt.rs:367)04:50:00.353ironclaw_agent_loop::executor::model: "agent loop model request prepared" (crates/ironclaw_agent_loop/src/executor/model.rs:88)Between those points the executor builds the prompt bundle (
build_prompt_bundle_for_surface→crates/ironclaw_turns/src/run_profile/prompt.rs:252load_loop_context), which loads identity context.Root cause (verified)
WorkspaceIdentityContextSource::load_identity_candidatesread the identity files in two serialforloops, awaiting each file one-by-one (src/workspace/reborn_identity_context.rs:131-151on base):context/assistant-directives.md) when personal context is allowed = 5-7 reads.candidate_for_path→read_identity_content→Workspace::read_primary(src/workspace/mod.rs:878) →get_document_by_path= exactly one SQL round-trip (libsqlsrc/db/libsql/workspace.rs:318-331; postgressrc/workspace/repository.rs:50-65). A missing file still pays a full RTT (returnsDocumentNotFoundafter the query).IdentityCandidateCacheOnceCell(crates/ironclaw_loop_support/src/lib.rs:363) is always cold, so every cold turn paid the full serial cost.Against the hosted, cross-region Postgres backend (~100-200ms/RTT) that is 5-7 serialized round-trips = multiple seconds of pre-provider latency on every cold turn.
Fix
Replace the two serial loops with
futures::future::try_join_allover an ordered candidate list (src/workspace/reborn_identity_context.rs). N round-trips collapse to ~1, whiletry_join_allpreserves the deterministic candidate order. TheRwLockwrite incandidate_for_pathis taken synchronously after each read and is never held across an.await, so the concurrent fan-out is safe.This is the minimal change: parallelization alone makes the formerly-wasted TOOLS.md read (filtered out later in TextOnly mode) effectively free, so no separate skip was needed. The
context_window_cache"written but never read" claim from the original triage was investigated and rejected — the context port writes it (lib.rs:346) and the model port consumes it viatake_matching(lib.rs:1410); it is correctly shared, so it was left untouched.Tri-backend safety
Each
get_document_by_pathis a self-contained query on its own pooled connection with no shared transaction:self.conn().await?per call, deadpool pool=30 ≫ 7 concurrent → N cross-region RTTs become ~1.self.connect().await?per call.Concurrent dispatch is identical and safe on all three.
Tests (red → green)
tests/reborn_identity_parallel_read.rs(backend-agnostic by construction — instruments theDatabasetrait boundary that every backend flows through; libsql is only the data store behind the probe):load_identity_candidates_is_parallel— aConcurrencyProbeDbcounts max in-flightget_document_by_pathcalls. FAILED on the serial base (max observed = 1, candidates = 7); passes after the fix (overlapping reads, run drops from ~280ms to ~0.17s). Permanent guardrail against serial regression.load_identity_candidates_propagates_read_failure— proves a non-DocumentNotFoundread failure still surfaces asHostIdentityContextBuildError::SourceUnavailablethroughtry_join_all's first-error short-circuit (observable error contract unchanged vs the serial loop).Existing 8
reborn_identity_contextunit tests still pass.cargo fmt+cargo clippy -p ironclaw --features libsql --testsclean. Multi-agent code review run; both surfaced findings addressed.Invariant
Added to
.claude/rules/database.md(now alsopaths-scoped tosrc/workspace/**): independent per-record reads must be concurrent, never a serial cross-region loop, guarded by aConcurrencyProbeDb-style test.Deferred follow-up
A host-build-time prefetch of identity candidates (warming the cache before the prompt stage) would eliminate even the ~1 RTT from the critical path, but lives in
crates/ironclaw_reborn/src/loop_driver_host.rswhich is outside this change's file lane and owned by a parallel workstream. Deferred.🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com