Repository navigation
feat(filesystem): add put_batch primitive to RootFilesystem (1/5) - #5339
henrypark133 wants to merge 1 commit into
Conversation
…ndation) Pure-addition foundation for native hot-store decomposition (#5269 design). Adds the put_batch primitive with a default impl built from existing ops; no backend native overrides (PR-2/3/4) and no consumer migrations yet. - root.rs: BatchPut struct + put_batch default impl (N==1 routes through put; N>1 commits via begin() over the common dir prefix, rollback-on-error). The availability contract is documented: N>1 is atomic only on MultiKey backends (Postgres today; libSQL after PR-3; in-memory after PR-4), else typed Unsupported; atomic-requiring callers must gate on Capability::BatchPut. - types.rs: Capability::BatchPut bit + FilesystemOperation::PutBatch + Display. - catalog.rs: CompositeRootFilesystem::put_batch enforces same-mount (else PathOutsideMount, nothing written) and delegates; BatchPut added to NEW_AXES. - scoped.rs: ScopedBatchPut + ScopedFilesystem::put_batch (per-path permission check) + operation_allowed PutBatch arm. - lib.rs: crate-private common_dir_prefix helper. - Contract + in-memory tests assert ACTUAL behavior (Postgres atomic N>1; libSQL/in-memory N>1 = Unsupported until their native PRs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds batched put support across filesystem layers: new operation and capability types, root and composite batch-write flows, scoped authorization, and contract coverage for empty, single-leg, multi-leg, cross-mount, and rollback cases. ChangesFilesystem batch writes
Sequence Diagram(s)Root batch write flow: sequenceDiagram
participant RootFilesystem
participant common_dir_prefix
participant StorageTxn
alt N == 1
RootFilesystem->>RootFilesystem: put(puts[0])
else N > 1
RootFilesystem->>common_dir_prefix: compute shared prefix
RootFilesystem->>StorageTxn: begin(prefix)
loop each BatchPut
RootFilesystem->>StorageTxn: put(path, entry, cas)
end
alt success
RootFilesystem->>StorageTxn: commit()
else failure
RootFilesystem->>StorageTxn: rollback()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 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 |
![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 the put_batch interface and its associated capability to support batched multi-key writes across the virtual filesystem. It includes a default implementation in RootFilesystem that scopes transactions to the longest common directory prefix, mount-boundary validation in CompositeRootFilesystem, and per-path permission checks in ScopedFilesystem. The review feedback identifies an issue in common_dir_prefix where it returns None when paths only share the root directory /. This prevents batch writes on root mounts or single-backend setups (e.g., /a and /b) from deriving a common prefix, and the reviewer suggests returning Some("/") instead to resolve this limitation.
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.
| if common.is_empty() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Currently, common_dir_prefix returns None when the paths only share the root directory / (i.e., common.is_empty()). While this aligns with the tests expecting None for divergent virtual roots (like /secrets/a and /memory/b), it introduces a limitation: if a deployment uses a root mount /, or if a backend is used directly with paths that don't share a first component (e.g., /a and /b), put_batch will fail with BackendInfrastructure because no common prefix can be derived, even though they are on the same backend/mount and could be updated in a single transaction scoped at /.
Since cross-mount validation is already robustly handled by CompositeRootFilesystem::put_batch (which compares mount pointers), common_dir_prefix can safely return Some("/") when common.is_empty(). This would enable batch puts on root mounts and single-backend setups.
![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/root.rs`:
- Around line 190-252: The Postgres backend capability set is missing BatchPut
even though it already advertises TxnCapability::MultiKey, so callers following
the put_batch contract will incorrectly avoid the atomic multi-key path. Update
the capabilities() implementation in postgres.rs to include Capability::BatchPut
alongside the existing Events, IndexFts, IndexVector, and MultiKey transaction
support so the default put_batch logic in root.rs can use the transactional
batch 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: 826eb0a1-fdb5-49e5-aac3-664d840d25a6
📒 Files selected for processing (10)
crates/ironclaw_filesystem/CLAUDE.mdcrates/ironclaw_filesystem/src/catalog.rscrates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/lib.rscrates/ironclaw_filesystem/src/root.rscrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_filesystem/src/scoped/tests.rscrates/ironclaw_filesystem/src/types.rscrates/ironclaw_filesystem/tests/db_root_filesystem_contract.rscrates/ironclaw_first_party_extensions/src/coding/paths.rs
| /// Write several [`Entry`] values in one call. | ||
| /// | ||
| /// Availability contract: | ||
| /// - `puts.len() == 0` is a programmer error and returns | ||
| /// [`FilesystemError::BackendInfrastructure`] (no path is in scope for an | ||
| /// empty batch). | ||
| /// - `puts.len() == 1` is **always** available: it routes through | ||
| /// [`put`](Self::put) and needs no transaction, so every backend (even | ||
| /// CAS-only ones) serves it. | ||
| /// - `puts.len() > 1` is **all-or-nothing atomic only on | ||
| /// [`TxnCapability::MultiKey`](crate::TxnCapability::MultiKey) backends** | ||
| /// (Postgres today; libSQL after PR-3; the in-memory reference after | ||
| /// PR-4). The default impl below opens a [`begin`](Self::begin) | ||
| /// transaction over the longest common directory prefix; on a CAS-only | ||
| /// backend `begin` returns [`FilesystemError::Unsupported`] and that | ||
| /// propagates unchanged — callers that require atomic batching MUST gate | ||
| /// on [`Capability::BatchPut`](crate::Capability::BatchPut) and fall back | ||
| /// to per-key CAS when it is absent. | ||
| /// | ||
| /// Returns one [`RecordVersion`] per put, in input order. On any failure in | ||
| /// a multi-key batch the transaction is rolled back and nothing is written. | ||
| async fn put_batch(&self, puts: Vec<BatchPut>) -> Result<Vec<RecordVersion>, FilesystemError> { | ||
| match puts.len() { | ||
| 0 => Err(FilesystemError::BackendInfrastructure { | ||
| operation: FilesystemOperation::PutBatch, | ||
| reason: "empty put_batch".to_string(), | ||
| }), | ||
| 1 => { | ||
| let mut puts = puts; | ||
| let BatchPut { path, entry, cas } = puts.swap_remove(0); | ||
| Ok(vec![self.put(&path, entry, cas).await?]) | ||
| } | ||
| _ => { | ||
| if puts.len() > MAX_BATCH_PUTS { | ||
| return Err(FilesystemError::BackendInfrastructure { | ||
| operation: FilesystemOperation::PutBatch, | ||
| reason: "batch exceeds MAX_BATCH_PUTS".to_string(), | ||
| }); | ||
| } | ||
| let prefix = | ||
| crate::common_dir_prefix(puts.iter().map(|p| &p.path)).ok_or_else(|| { | ||
| FilesystemError::BackendInfrastructure { | ||
| operation: FilesystemOperation::PutBatch, | ||
| reason: "put_batch entries share no common directory prefix" | ||
| .to_string(), | ||
| } | ||
| })?; | ||
| let mut txn = self.begin(&prefix).await?; | ||
| let mut versions = Vec::with_capacity(puts.len()); | ||
| for BatchPut { path, entry, cas } in puts { | ||
| match txn.put(&path, entry, cas).await { | ||
| Ok(version) => versions.push(version), | ||
| Err(error) => { | ||
| txn.rollback().await; | ||
| return Err(error); | ||
| } | ||
| } | ||
| } | ||
| txn.commit().await?; | ||
| Ok(versions) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find which backends advertise Capability::BatchPut and which advertise MultiKey txn.
rg -nP --type=rust -C3 'Capability::BatchPut'
rg -nP --type=rust -C3 'TxnCapability::MultiKey'
# Locate postgres/libsql capability declarations.
fd -e rs -i postgres -x rg -nP -C3 '(capabilities|BackendCapabilities|with_txn|\.with\(Capability)' {}Repository: nearai/ironclaw
Length of output: 153
postgres backend advertises transactional support TxnCapability::MultiKey but fails to advertise Capability::BatchPut
In crates/ironclaw_filesystem/src/postgres.rs, the capabilities() method sets TxnCapability::MultiKey but omits Capability::BatchPut. Per crates/ironclaw_filesystem/src/root.rs (default put_batch impl) and types.rs, callers must gate on Capability::BatchPut to utilize the atomic N>1 write path provided by TxnCapability::MultiKey.
This mismatch causes callers following the documented contract to skip the atomic path for Postgres, incorrectly falling back to per-key CAS despite the backend supporting transactions.
Fix required: Add .with(Capability::BatchPut) to the capability set in postgres.rs:
BackendCapabilities::sql_typical()
.with(Capability::Events)
.with(Capability::IndexFts)
.with(Capability::IndexVector)
.with(Capability::BatchPut)
.with_txn(TxnCapability::MultiKey)🤖 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/root.rs` around lines 190 - 252, The Postgres
backend capability set is missing BatchPut even though it already advertises
TxnCapability::MultiKey, so callers following the put_batch contract will
incorrectly avoid the atomic multi-key path. Update the capabilities()
implementation in postgres.rs to include Capability::BatchPut alongside the
existing Events, IndexFts, IndexVector, and MultiKey transaction support so the
default put_batch logic in root.rs can use the transactional batch path.
Source: Learnings
|
🚅 Deployed to the ironclaw-pr-5339 environment in ironclaw-ci-preview
|
|
Closing as stale — no activity in over three weeks. The branch is untouched; reopen if this is still needed. |
This branch was successfully deployed
1 participant
Stack 1/5 · base
mainFoundation for the #5269 native hot-store decomposition: adds a
put_batchprimitive to theRootFilesystemtrait.BatchPut+put_batchdefault impl (N==1 →put; N>1 →begin()/StorageTxnover the common dir prefix, rollback-on-error). Availability contract documented: N>1 atomic only onMultiKeybackends, else typedUnsupported; atomic-requiring callers gate onCapability::BatchPut.Capability::BatchPut+FilesystemOperation::PutBatch;CompositeRootFilesystem::put_batch(same-mount enforced →PathOutsideMount);ScopedFilesystem::put_batch(per-leg permission check); crate-privatecommon_dir_prefix; sharedMAX_BATCH_PUTS.Capability::all()drift guard.FilesystemOperationvariant requires a workspace build (two exhaustiveoperation_allowedmatches) — added after this was caught.Reviewed (9-agent code-review + thermo), fixes applied.
cargo test -p ironclaw_filesystem+ workspace build green; clippy clean.🤖 Generated with Claude Code