[HST Postgres 1/4] RootFilesystem latency substrate - #5688
serrrfirat wants to merge 2 commits into
Conversation
✅ IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted review state before this projection. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesFilesystem transaction and backend updates
Postgres and libSQL backend behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ScopedStorageTxn
participant PostgresStorageTxn
participant Postgres
Caller->>ScopedStorageTxn: reserve_sequence(path)
ScopedStorageTxn->>ScopedStorageTxn: check ReserveSeq permission
ScopedStorageTxn->>ScopedStorageTxn: check mount_prefix containment
ScopedStorageTxn->>PostgresStorageTxn: reserve_sequence(path)
PostgresStorageTxn->>Postgres: postgres_reserve_sequence_with_client (INSERT...ON CONFLICT...RETURNING)
Postgres-->>PostgresStorageTxn: reserved value
PostgresStorageTxn-->>ScopedStorageTxn: SeqNo
ScopedStorageTxn-->>Caller: SeqNo
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
github-actions
Bot
added
risk: low
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reserve_sequence method to the StorageTxn trait (with implementations for Postgres and Scoped transactions), refactors Postgres migrations to use advisory locks and track migrated schemas, optimizes Postgres stat queries, and adds a public helper to build event stores from a root filesystem. Feedback on the changes suggests addressing a potential collision risk in postgres_shared_projection_index_name where concatenating keys with a simple _ delimiter could cause different key sets to produce identical index names; using an injective length-prefixed encoding is recommended to prevent this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let keys = spec | ||
| .keys | ||
| .iter() | ||
| .map(|key| key.as_str()) | ||
| .collect::<Vec<_>>() | ||
| .join("_"); | ||
| sql_index_name(&format!("/shared/{kind}/{keys}"), spec.name.as_str()) |
There was a problem hiding this comment.
The generation of the shared projection index name concatenates the keys using a simple _ delimiter. This can lead to collisions where different sets of keys (e.g., ["a", "b"] vs ["a_b"]) produce the same index name, causing CREATE INDEX IF NOT EXISTS to silently skip creating the second index.\n\nTo prevent separator-collision attacks or accidental collisions, use an injective length-prefixed encoding or another injective scheme when combining the keys and kind before generating the index name.
References
- When generating deterministic identifiers or hashes from multiple string components, use an injective length-prefixed encoding rather than simple concatenation with a delimiter to eliminate the risk of separator-collision attacks or accidental collisions.
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 5b15dce48899c1b443ab21d1f80f986306f03ef5
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the changed filesystem backend and event-store wrapper code. The PR keeps the changes scoped to backend migration/index/sequence behavior and preserves libSQL/Postgres parity expectations in the reviewed paths.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
Reborn integration-tier coverageLine coverage (Reborn crates): 28.52% — 49276 / 172796 lines Per-crate breakdown (62 crates, lowest-covered first)
This signal is informational: coverage never gates the PR — not the percentage, not the per-crate holes, not the 0-coverage callout. Exemptions (0 file(s) excluded from the accounting above)No exemptions configured. |
|
Cycle 38 is the goat. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_filesystem/src/libsql.rs (1)
160-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate retry/backoff logic across both
Errarms.The
execute_batchfailure arm (162-167) and theopen()failure arm (170-175) both dolast_error = Some(error); if attempt + 1 < LIBSQL_CONNECT_ATTEMPTS { sleep(...) }. Worth extracting into a small helper so the two retry sources can't drift.♻️ Proposed refactor
+fn record_retry( + last_error: &mut Option<libsql::Error>, + error: libsql::Error, +) -> Option<std::time::Duration> { + *last_error = Some(error); + None // caller checks attempt bound and calls connect_backoff +}Simplest form: keep the
if attempt + 1 < LIBSQL_CONNECT_ATTEMPTS { sleep(...) }.awaitinline at the call site but movelast_error = Some(error)+ bound-check into one closure invoked from both arms, e.g.:- match conn.execute_batch(LIBSQL_CONNECTION_PRAGMAS).await { - Ok(_) => return Ok(conn), - Err(error) => { - last_error = Some(error); - if attempt + 1 < LIBSQL_CONNECT_ATTEMPTS { - tokio::time::sleep(connect_backoff(attempt)).await; - } - } - } + match conn.execute_batch(LIBSQL_CONNECTION_PRAGMAS).await { + Ok(_) => return Ok(conn), + Err(error) => last_error = Some(error), + }and hoist a single
if attempt + 1 < LIBSQL_CONNECT_ATTEMPTS && last_error.is_some() { sleep(...).await }after the outer match once both arms just record the error.As per coding guidelines, "Keep functions focused and extract helpers when logic is reused."
🤖 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 `@crates/ironclaw_filesystem/src/libsql.rs` around lines 160 - 176, The retry handling in the libsql connection loop duplicates the same “record error and maybe sleep” logic in both the `execute_batch` and `open()` failure branches. Refactor the `connect` flow in `libsql.rs` so both `Err` arms only capture the error source, then delegate the shared `last_error = Some(error)` plus `attempt + 1 < LIBSQL_CONNECT_ATTEMPTS` backoff check to a single helper or closure used by both paths, keeping the retry behavior identical.Source: Coding guidelines
🤖 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 `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 160-168: The retry branch in the libsql connection flow is not
covered when `execute_batch(LIBSQL_CONNECTION_PRAGMAS)` fails, so add a
regression test that forces PRAGMA execution to fail on an initial attempt and
then succeed on a later retry. Extend the existing
`connect_retries_transient_open_failures_before_succeeding` coverage or add a
nearby test around the same `libsql::connect` path to verify the retry logic and
eventual success after a PRAGMA failure, using the same
`LIBSQL_CONNECTION_PRAGMAS` and `LIBSQL_CONNECT_ATTEMPTS` behavior.
---
Outside diff comments:
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 160-176: The retry handling in the libsql connection loop
duplicates the same “record error and maybe sleep” logic in both the
`execute_batch` and `open()` failure branches. Refactor the `connect` flow in
`libsql.rs` so both `Err` arms only capture the error source, then delegate the
shared `last_error = Some(error)` plus `attempt + 1 < LIBSQL_CONNECT_ATTEMPTS`
backoff check to a single helper or closure used by both paths, keeping the
retry behavior identical.
🪄 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: 5255f9e1-f57a-45f9-94a8-5c579b1ccfa0
📒 Files selected for processing (5)
crates/ironclaw_filesystem/src/backend.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_reborn_event_store/src/lib.rs
|
🚅 Deployed to the ironclaw-pr-5688 environment in ironclaw-ci-preview
|
🗂️ Archived IronLoop Review: reviewerThis result is from an older PR head and is no longer the active review.
Archived summaryNo concrete blocking issues found in the changed filesystem backend and event-store wrapper code. The PR keeps the changes scoped to backend migration/index/sequence behavior and preserves libSQL/Postgres parity expectations in the reviewed paths. |
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 01db07815ab950adfe9df26d68514f43eaa8927f
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the reviewed diff. The changes keep the filesystem/event-store behavior scoped and add reasonable fail-closed/default handling for transaction sequence reservation and Postgres/libSQL backend paths.
Findings
None.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloop review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloop reviewwhen the fix may affect multiple areas. - Use
@ironloop statusto check queued/running/completed/stale/stalled state while reviewers run.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_filesystem/src/postgres.rs (1)
466-471: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winAvoid
prepare_cached()for this query.crates/ironclaw_filesystem/src/postgres.rs:466-471The statement cache is unbounded per connection, and this SQL text varies with the filter tree andLIMIT/OFFSETplaceholder positions, so hot connections will accumulate prepared statements. Useprepare()here or bound the cache around a normalized query shape.🤖 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 `@crates/ironclaw_filesystem/src/postgres.rs` around lines 466 - 471, The query path in the Postgres filesystem code is using prepare_cached for SQL generated from varying filter trees and pagination placeholders, which can grow the per-connection statement cache without bound. Update the logic in the query-building flow around the prepare_cached call to use prepare instead, or otherwise constrain caching to a normalized query shape, while keeping the existing db_error handling and subsequent client.query invocation intact.
🤖 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.
Outside diff comments:
In `@crates/ironclaw_filesystem/src/postgres.rs`:
- Around line 466-471: The query path in the Postgres filesystem code is using
prepare_cached for SQL generated from varying filter trees and pagination
placeholders, which can grow the per-connection statement cache without bound.
Update the logic in the query-building flow around the prepare_cached call to
use prepare instead, or otherwise constrain caching to a normalized query shape,
while keeping the existing db_error handling and subsequent client.query
invocation intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b9b48a6b-47d9-4266-b301-6780c78d0cee
📒 Files selected for processing (2)
crates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rs
This branch was successfully deployed
1 participant
Stack
1/4 for hosted-single-tenant Postgres latency parity.
Base:
mainNext:
codex/hst-postgres-02-turn-state(#5689)This first PR is intentionally the reviewer entry point for the whole stack. The code here is only the RootFilesystem substrate slice, but the journey, experiments, and final gains are summarized here so reviewers do not have to start from the large final harness PR.
Stack Map
/turns/state.json->/turns/rows/v1migration gate. Still no profile switch by itself.What This PR Changes
RootFilesystemsubstrate used by later stores:pathin exact/prefix indexesRootFilesystem.Hosted-Volume Safety
No hosted profile wiring changes here. This PR should not switch
hosted-single-tenant-volumeto any new store layout by itself.Live-data safety gates across the stack:
/turns/state.jsononly when row state is empty, appends the import through the row journal, and treats existing rows as authoritative afterward./resources/snapshot.jsonbefore replaying the new journal.mainalready has the per-record filesystem secret layout; [HST Postgres 3/4] Wire filesystem runtime stores #5690 keeps that path and removes direct DB-store bypasses rather than introducing a snapshot-to-row secret migration boundary.Starting Point
The production-shaped hosted-volume reference the stack had to beat:
Other important baselines discovered during the cycles:
chat-turnturn-store p95 climbed by quartile from 32.7ms -> 60.9ms -> 78.8ms -> 114.4ms as/turns/state.jsongrew.Ending Point
Final recorded push-gate numbers after the stack:
Gains
Experiment Ledger
The detailed journey is summarized here in PR text; the raw scratch log is intentionally not committed to the repo.
statto one cached query; control-plane probe stayed faster than libSQL.ironclaw_stress; found and fixed nested Postgres pool checkout deadlock via transaction-local sequence reservation.turn_lifecycle_blobworkload so blob contention became visible in the scorer./api/webchat/v2/sessionworkload; c1/c4 passed, c100 showed middleware/read-path pressure.append_batch; c100 turn lifecycle improved 8.67s -> 3.52s./turns/state.jsonimport gate and stale-blob no-remigrate tests so the stack has a live hosted-volume migration path before go-live.Verification For This PR
cargo check -p ironclaw_filesystem --features libsql,postgrescargo check -p ironclaw_reborn_event_store --features libsql,postgresStack-wide verification is listed in the dependent PRs and summarized above.