[HST Postgres v2 1/4] RootFilesystem latency substrate - #5724
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. |
github-actions
Bot
added
size: XL
![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: 19224b0fe8a7dc212956ce6123990a691cc96246
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
No concrete blocking issues found in the reviewed diff. The changes are scoped to RootFilesystem latency/concurrency improvements, transaction sequence reservation plumbing, and event-store filesystem wrapping. I could not run Rust tests because cargo is not installed in this environment.
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.
![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 several robustness and concurrency improvements to the ironclaw filesystem backends. Key changes include refactoring the libSQL backend to use explicit "BEGIN IMMEDIATE" transactions to prevent concurrent write races, implementing advisory locks and schema migration tracking for PostgreSQL, adding support for reserving sequences within storage transactions, and optimizing query performance via cached statements. Feedback on the changes highlights a potential conversion error in the PostgreSQL migration key generation if "current_schema()" returns NULL, which can be resolved by applying a COALESCE fallback.
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.
| .query_one( | ||
| "SELECT \ | ||
| current_database(), \ | ||
| current_schema(), \ |
There was a problem hiding this comment.
In PostgreSQL, current_schema() can return NULL if the search path is empty or contains no valid schemas. If it returns NULL, row.get(1) will fail with a conversion error when attempting to retrieve it as a String. Consider using COALESCE(current_schema(), 'public') to ensure a non-null fallback schema name is always returned.
| current_schema(), \ | |
| COALESCE(current_schema(), 'public'), \ |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a default-failing Changesreserve_sequence and filesystem hardening
Estimated code review effort: 4 (Complex) | ~60 minutes 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 |
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.28% — 273440 / 320643 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (4 entry/entries excluded from the accounting above)
|
|
🚅 Deployed to the ironclaw-pr-5724 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 reviewed diff. The changes are scoped to RootFilesystem latency/concurrency improvements, transaction sequence reservation plumbing, and event-store filesystem wrapping. I could not run Rust tests because |
There was a problem hiding this comment.
✅ IronLoop Review: reviewer
Verdict: ✅ Approved
Findings: 0 blocking / 0 notes
Next: No reviewer action needed.
Head: 94671744d09a806f0b511f16bccd48371e3c9bb1
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
Reviewed the filesystem/event-store persistence changes. I did not find a concrete correctness, security, or test-coverage issue that should block the PR.
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.
![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
🤖 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 1319-1328: The INSERT in the create_dir_all flow is currently
reporting errors against the full target path instead of the per-segment prefix
being written, which makes diagnostics misleading for intermediate failures.
Update the error mapping around the conn.execute call in libsql.rs so it uses
the same prefix-based context as the earlier SELECT/CREATE steps, keeping
libsql_db_error aligned with the current segment being processed rather than
path.
🪄 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: 9b7977f0-fe64-4f66-8462-09897a42cfa3
📒 Files selected for processing (6)
crates/ironclaw_filesystem/src/backend.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rscrates/ironclaw_reborn_event_store/src/lib.rs
| conn.execute( | ||
| r#" | ||
| INSERT INTO root_filesystem_entries (path, contents, is_dir, updated_at) | ||
| VALUES (?1, X'', 1, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | ||
| ON CONFLICT (path) DO NOTHING | ||
| "#, | ||
| libsql::params![prefix.as_str()], | ||
| ) | ||
| .await | ||
| .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::CreateDirAll, error))?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Per-prefix INSERT error attributes to path, not prefix. The SELECT above (1303/1306/1309) reports errors against prefix, but this INSERT reports against the full target path. In a multi-level create_dir_all, a failure on an intermediate segment will surface the leaf path, misdirecting diagnostics.
🩹 Align error context with the prefix under write
.await
- .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::CreateDirAll, error))?;
+ .map_err(|error| {
+ libsql_db_error(prefix.clone(), FilesystemOperation::CreateDirAll, error)
+ })?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| conn.execute( | |
| r#" | |
| INSERT INTO root_filesystem_entries (path, contents, is_dir, updated_at) | |
| VALUES (?1, X'', 1, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | |
| ON CONFLICT (path) DO NOTHING | |
| "#, | |
| libsql::params![prefix.as_str()], | |
| ) | |
| .await | |
| .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::CreateDirAll, error))?; | |
| conn.execute( | |
| r#" | |
| INSERT INTO root_filesystem_entries (path, contents, is_dir, updated_at) | |
| VALUES (?1, X'', 1, strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | |
| ON CONFLICT (path) DO NOTHING | |
| "#, | |
| libsql::params![prefix.as_str()], | |
| ) | |
| .await | |
| .map_err(|error| { | |
| libsql_db_error(prefix.clone(), FilesystemOperation::CreateDirAll, error) | |
| })?; |
🤖 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 1319 - 1328, The
INSERT in the create_dir_all flow is currently reporting errors against the full
target path instead of the per-segment prefix being written, which makes
diagnostics misleading for intermediate failures. Update the error mapping
around the conn.execute call in libsql.rs so it uses the same prefix-based
context as the earlier SELECT/CREATE steps, keeping libsql_db_error aligned with
the current segment being processed rather than path.
- Delete the pub current_version wrapper: main's #5724 independently removed it in favor of the free-function current_version_libsql used inside the transactional put_libsql_inner path, confirming it had no external caller. The pool-typed current_version_with_conn duplicate is dead after adopting that transactional structure; deleted too. - Add a libsql contract test mirroring postgres_put_cas_version_on_missing_path_reports_no_found_version: CasExpectation::Version against a missing path must report VersionMismatch { found: None }. - Add a pool checkout-timeout test via a new build_libsql_pool_with_config seam (tiny size-1/short-timeout pool) asserting the timeout maps to a FilesystemOperation::Connect infrastructure error through connect()'s debug!-logged fallback arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…E_MISUSE (#5466) (#5751) * fix(filesystem): pool libSQL connections to stop concurrent-CAS SQLITE_MISUSE (#5466) LibSqlRootFilesystem opened a fresh connection (sqlite3_open_v2 + PRAGMA batch) for every RootFilesystem operation. Under genuinely parallel CAS storms against one WAL database that unbounded open/PRAGMA/close churn intermittently fails inside the C library with SQLITE_MISUSE ("bad parameter or other API misuse") or spurious disk I/O errors — the ~10% failure / SIGABRT reported in #5466. A single shared connection is also wrong: the CAS rows-affected readback is per-connection state, and two tasks interleaving statements on one connection corrupt compare-and-swap into silent lost updates (reproduced during diagnosis). Fix: a bounded deadpool-managed pool (same pooling core the Postgres backend already uses) in the new libsql_pool module — each operation checks out one PRAGMA-initialized connection for exclusive use and returns it on drop; recycle() rejects connections left mid-transaction. put()'s three CasExpectation arms now drop their checkout before the nested current_version readback, upholding the documented one-checkout-per-call-stack invariant. Regression test: tests/concurrent_cas_storm.rs drives 16 spawned writers x 100 cas_update increments on a multi-thread runtime against in-memory, libSQL, and (env-gated) Postgres backends, asserting zero backend errors and an exact final count. Mutation-verified: re-injecting per-op churn (recycle always discarding) goes RED with the exact SQLITE_MISUSE signature; the shared-connection probe goes RED on the lost-update assertion. Closes #5466 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filesystem): address PR #5751 review findings on libSQL pool - put()'s CAS-mismatch/success version readbacks reuse the already checked-out connection via a new current_version_with_conn helper instead of drop-then-recheckout, making the one-checkout-per-call- stack invariant structural for that call site. - Add pool-internal tests: recycle rejects a connection returned mid-transaction, and connect_with_retry surfaces a Connect error with the final cause after exhausting its retry budget. - Log libSQL pool checkout failures at debug level, mirroring the Postgres backend's shape, for trace correlation on checkout timeouts. - Annotate the four silent-skip fallbacks in the Postgres CAS-storm test with per-site silent-ok rationale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(filesystem): address libSQL pool review findings on PR #5751 - Delete the pub current_version wrapper: main's #5724 independently removed it in favor of the free-function current_version_libsql used inside the transactional put_libsql_inner path, confirming it had no external caller. The pool-typed current_version_with_conn duplicate is dead after adopting that transactional structure; deleted too. - Add a libsql contract test mirroring postgres_put_cas_version_on_missing_path_reports_no_found_version: CasExpectation::Version against a missing path must report VersionMismatch { found: None }. - Add a pool checkout-timeout test via a new build_libsql_pool_with_config seam (tiny size-1/short-timeout pool) asserting the timeout maps to a FilesystemOperation::Connect infrastructure error through connect()'s debug!-logged fallback arm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(filesystem): cover permanent PRAGMA-failure path in connect_with_retry Every open succeeds but every PRAGMA batch fails across all retry attempts; final error must be FilesystemOperation::Connect carrying the PRAGMA cause. Addresses PR #5751 round-3 review finding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Replacement Stack
Replaces the old HST Postgres latency stack #5688, #5689, #5690, and #5691.
This v2 stack is rebuilt from the latest fixed head and rebased onto current
main. The fixes that restored the Cycle 49 benchmark gates have been folded down into the subsystem PRs that own them:What This PR Changes
RootFilesystemsubstrate used by later stores.RootFilesystem.Hosted-Volume Safety
No hosted profile wiring changes here. This PR should not switch
hosted-single-tenant-volumeto any new store layout by itself.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.