docs(reborn): design — native hot-store primitives on the unified RootFilesystem trait - #5269
Conversation
…tFilesystem trait Design for making the reborn Postgres access pattern "native" (per-row, indexed, partial-update) WITHOUT changing the RootFilesystem abstraction or losing dual-backend (libSQL + Postgres). Generalizes the #5232 lease-sidecar pattern across the remaining hot stores. Root cause it addresses: the four hot stores funnel every mutation through a whole-blob read-modify-write CAS on one Entry, serializing logically-disjoint writers — worst of all the process-global /resources/snapshot.json under ResourceScope::system(), CAS-contended across all tenants with no retry. (v1 never had this: it used native per-row INSERT/UPDATE on a 30-deep pool.) The design keeps RootFilesystem frozen as the one contract and goes native via: - Decomposition with ZERO new ops: hot stores write fine-grained per-entity records (per-run, per-account) CAS'd individually instead of one giant blob. - TWO new optional native primitives — put_batch (atomic multi-put) and adjust_indexed (atomic conditional numeric delta) — each with a default impl built from existing ops (byte-only/in-memory backends unchanged) plus native Postgres AND native libSQL overrides, gated behind capability bits. - A stated migration-safety invariant + CI parity test: both backends store byte-identical logical records, so libSQL<->Postgres stays a COPY not a transform. Resolves the hard cases honestly: the CAS-only multi-account admission hole (atomic gate on the broadest tenant ledger), account-seg injectivity (sha256(canonical_json)), libSQL RETURNING-in-txn unavailability (init probe + in-txn SELECT fallback + advertise-only-after-probe), and the turn-state system()-scope premise correction. Per-store Snapshot->DualWrite->Native rollout, reversible, green on both backends; Governor first. Provenance: synthesized via a 5-phase multi-agent design workflow (ground -> 3 independent angles -> adversarial review -> synthesis -> invariant verify). Verify signoff: ACCEPT, all six invariants pass, no blockers. Design doc only — no code changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
scope: docs
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a storage design proposal for decomposing hot-store records under ChangesNative storage primitives proposal
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 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: 3
🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md`:
- Around line 69-77: Clarify the availability contract for put_batch across
RootFilesystem and ScopedFilesystem: the current text says the default
implementation works identically everywhere, but the later CAS-only N>1
Unsupported case makes that ambiguous. Update the §6.2 / §6.1 wording around
put_batch, adjust_indexed, Capability gating, and the mount-time requirement so
it clearly states when the primitive is callable, when it may fall back to the
default path, and when callers must treat the capability bit as required and
handle Unsupported explicitly.
- Around line 533-541: The thread append path still falls back to sequential CAS
when neither BatchPut nor begin is available, which conflicts with the stated
atomicity requirement. Update the native storage plan around the message write
path to remove the non-atomic reserve_sequence + sequential-CAS fallback from
the main flow and make TransactionalMessageWrite use only the
transactional/put_batch path, with ForceCas as the explicit opt-out escape
hatch. Keep the capability check in the store’s append logic, but ensure the
default behavior in the relevant message/thread write flow does not permit torn
multi-op writes.
- Around line 132-145: The capability expansion for in_memory_full() is too
aggressive for the current PR-4 state. Update the plan text so in_memory_full()
remains capped and do not add BatchPut or AdjustIndexed to NEW_AXES in the
catalog.rs validator yet; keep the wording aligned with the open per-op lock
retention audit and only describe the broader sql_typical_hotpath() coverage
once that guarantee is actually approved.
🪄 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: c79f4491-06d5-46aa-9419-d081f275701a
📒 Files selected for processing (1)
docs/plans/2026-06-26-native-storage-primitives.md
| `in_memory_full()` is widened to `sql_typical_hotpath()` (the in-memory backend serves both via default impls under its per-op lock — see parity notes). | ||
|
|
||
| **Mount validator (catalog.rs)** — add both bits to `NEW_AXES`: | ||
|
|
||
| ```rust | ||
| const NEW_AXES: &[Capability] = &[ | ||
| Capability::Records, Capability::Query, | ||
| Capability::IndexExact, Capability::IndexPrefix, | ||
| Capability::IndexFts, Capability::IndexVector, | ||
| Capability::Events, | ||
| Capability::BatchPut, Capability::AdjustIndexed, | ||
| ]; | ||
| ``` | ||
| That is the entire validator change; the existing `declared.has(cap) && !backend.has(cap) → DescriptorOverclaims` loop handles the new bits by construction. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '^(docs/plans/2026-06-26-native-storage-primitives\.md|CLAUDE\.md|AGENTS\.md|\.claude/rules|src/db/CLAUDE\.md|FEATURE_PARITY\.md)$'
echo
echo "== outline the plan doc =="
wc -l docs/plans/2026-06-26-native-storage-primitives.md
sed -n '110,170p' docs/plans/2026-06-26-native-storage-primitives.md
echo
echo "== search for in_memory_full/sql_typical_hotpath/BatchPut/AdjustIndexed =="
rg -n "in_memory_full|sql_typical_hotpath|BatchPut|AdjustIndexed|NEW_AXES|DescriptorOverclaims|per-op lock|holds the lock" docs src .claude CLAUDE.md AGENTS.md FEATURE_PARITY.md
echo
echo "== inspect relevant guidance files if present =="
for f in CLAUDE.md AGENTS.md src/db/CLAUDE.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
fi
doneRepository: nearai/ironclaw
Length of output: 41601
🏁 Script executed:
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '^(docs/plans/2026-06-26-native-storage-primitives\.md|CLAUDE\.md|AGENTS\.md|\.claude/rules|src/db/CLAUDE\.md|FEATURE_PARITY\.md)$'
echo
echo "== outline the plan doc =="
wc -l docs/plans/2026-06-26-native-storage-primitives.md
sed -n '110,170p' docs/plans/2026-06-26-native-storage-primitives.md
echo
echo "== search for in_memory_full/sql_typical_hotpath/BatchPut/AdjustIndexed =="
rg -n "in_memory_full|sql_typical_hotpath|BatchPut|AdjustIndexed|NEW_AXES|DescriptorOverclaims|per-op lock|holds the lock" docs src .claude CLAUDE.md AGENTS.md FEATURE_PARITY.md
echo
echo "== inspect relevant guidance files if present =="
for f in CLAUDE.md AGENTS.md src/db/CLAUDE.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
fi
doneRepository: nearai/ironclaw
Length of output: 41601
🏁 Script executed:
set -euo pipefail
echo "== docs/plans/2026-06-26-native-storage-primitives.md around the cited lines =="
nl -ba docs/plans/2026-06-26-native-storage-primitives.md | sed -n '120,155p'
echo
echo "== same doc: references to PR-4 and parity notes =="
rg -n -C 2 "PR-4|parity notes|in_memory_full|sql_typical_hotpath|BatchPut|AdjustIndexed" docs/plans/2026-06-26-native-storage-primitives.md
echo
echo "== any repo guidance about docs/plan assertions or parity docs =="
rg -n -C 2 "FEATURE_PARITY|parity notes|plan|docs/plans|advertise|capability" CLAUDE.md AGENTS.md .claude src docs | head -n 200Repository: nearai/ironclaw
Length of output: 273
Keep in_memory_full() capped for now. PR-4 still treats per-op lock retention as the open audit, so advertising BatchPut/AdjustIndexed here overstates the current guarantee.
🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md` around lines 132 - 145,
The capability expansion for in_memory_full() is too aggressive for the current
PR-4 state. Update the plan text so in_memory_full() remains capped and do not
add BatchPut or AdjustIndexed to NEW_AXES in the catalog.rs validator yet; keep
the wording aligned with the open per-op lock retention audit and only describe
the broader sql_typical_hotpath() coverage once that guarantee is actually
approved.
| 1. **Adopt `put_batch`** for the message append, replacing the hand-rolled `begin`/`StorageTxn` 4-op txn + `TransactionalMessageWrite::Unsupported` enum. The store **requires** atomicity, so it gates on the `BatchPut` capability (or `begin` availability) and keeps its existing `reserve_sequence`+sequential-CAS fallback for backends advertising neither. Public API unchanged. | ||
| 2. **Native query indexes** replace bespoke index files: | ||
| ```rust | ||
| ensure_index("/threads", IndexSpec{ name:"msg_by_seq", keys:["thread_seg","sequence"], kind: Exact }); | ||
| ensure_index("/threads", IndexSpec{ name:"msg_by_run", keys:["run_id"], kind: Exact }); | ||
| ensure_index("/threads", IndexSpec{ name:"threads_by_scope", keys:["scope_seg"], kind: Exact }); | ||
| ``` | ||
| Message `indexed` gains `{ "thread_seg": Text, "sequence": I64, "run_id": Text }`; thread `indexed` gains `{ "scope_seg": Text, "updated_at": I64, "owner_user": Text }`. `list_threads_for_scope` becomes `query(/threads, Eq{scope_seg})` + Rust sort by `updated_at`, retiring the N+1 `list_dir`+per-file `get` (`filesystem_service.rs:1992-2048`). Range "messages [lo,hi]" → `query(Range{key:"sequence", lo:I64, hi:I64})`. | ||
| 3. **`ForceCas` escape hatch:** an operational flag (`ThreadWritePath { TxnOrCas (default), ForceCas }`) to disable the transactional path instantly if a backend's `begin`/`put_batch` misbehaves. Zero data migration, reversible. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the non-atomic thread fallback.
This paragraph says the store requires atomicity, but the fallback for backends without BatchPut/begin is sequential CAS. That fallback cannot satisfy the stated requirement and reintroduces torn multi-op writes on the exact backends that need the escape hatch.
Suggested wording
- The store **requires** atomicity, so it gates on the `BatchPut` capability (or `begin` availability) and keeps its existing reserve_sequence+sequential-CAS fallback for backends advertising neither.
+ The store **requires** atomicity, so it gates on the `BatchPut` capability (or `begin` availability). If neither is available, reject the atomic write instead of falling back to sequential CAS.📝 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.
| 1. **Adopt `put_batch`** for the message append, replacing the hand-rolled `begin`/`StorageTxn` 4-op txn + `TransactionalMessageWrite::Unsupported` enum. The store **requires** atomicity, so it gates on the `BatchPut` capability (or `begin` availability) and keeps its existing `reserve_sequence`+sequential-CAS fallback for backends advertising neither. Public API unchanged. | |
| 2. **Native query indexes** replace bespoke index files: | |
| ```rust | |
| ensure_index("/threads", IndexSpec{ name:"msg_by_seq", keys:["thread_seg","sequence"], kind: Exact }); | |
| ensure_index("/threads", IndexSpec{ name:"msg_by_run", keys:["run_id"], kind: Exact }); | |
| ensure_index("/threads", IndexSpec{ name:"threads_by_scope", keys:["scope_seg"], kind: Exact }); | |
| ``` | |
| Message `indexed` gains `{ "thread_seg": Text, "sequence": I64, "run_id": Text }`; thread `indexed` gains `{ "scope_seg": Text, "updated_at": I64, "owner_user": Text }`. `list_threads_for_scope` becomes `query(/threads, Eq{scope_seg})` + Rust sort by `updated_at`, retiring the N+1 `list_dir`+per-file `get` (`filesystem_service.rs:1992-2048`). Range "messages [lo,hi]" → `query(Range{key:"sequence", lo:I64, hi:I64})`. | |
| 3. **`ForceCas` escape hatch:** an operational flag (`ThreadWritePath { TxnOrCas (default), ForceCas }`) to disable the transactional path instantly if a backend's `begin`/`put_batch` misbehaves. Zero data migration, reversible. | |
| 1. **Adopt `put_batch`** for the message append, replacing the hand-rolled `begin`/`StorageTxn` 4-op txn + `TransactionalMessageWrite::Unsupported` enum. The store **requires** atomicity, so it gates on the `BatchPut` capability (or `begin` availability). If neither is available, reject the atomic write instead of falling back to sequential CAS. Public API unchanged. | |
| 2. **Native query indexes** replace bespoke index files: | |
| ensure_index("/threads", IndexSpec{ name:"msg_by_seq", keys:["thread_seg","sequence"], kind: Exact }); | |
| ensure_index("/threads", IndexSpec{ name:"msg_by_run", keys:["run_id"], kind: Exact }); | |
| ensure_index("/threads", IndexSpec{ name:"threads_by_scope", keys:["scope_seg"], kind: Exact }); | |
| Message `indexed` gains `{ "thread_seg": Text, "sequence": I64, "run_id": Text }`; thread `indexed` gains `{ "scope_seg": Text, "updated_at": I64, "owner_user": Text }`. `list_threads_for_scope` becomes `query(/threads, Eq{scope_seg})` + Rust sort by `updated_at`, retiring the N+1 `list_dir`+per-file `get` (`filesystem_service.rs:1992-2048`). Range "messages [lo,hi]" → `query(Range{key:"sequence", lo:I64, hi:I64})`. | |
| 3. **`ForceCas` escape hatch:** an operational flag (`ThreadWritePath { TxnOrCas (default), ForceCas }`) to disable the transactional path instantly if a backend's `begin`/`put_batch` misbehaves. Zero data migration, reversible. |
🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md` around lines 533 - 541,
The thread append path still falls back to sequential CAS when neither BatchPut
nor begin is available, which conflicts with the stated atomicity requirement.
Update the native storage plan around the message write path to remove the
non-atomic reserve_sequence + sequential-CAS fallback from the main flow and
make TransactionalMessageWrite use only the transactional/put_batch path, with
ForceCas as the explicit opt-out escape hatch. Keep the capability check in the
store’s append logic, but ensure the default behavior in the relevant
message/thread write flow does not permit torn multi-op writes.
![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 proposes a design plan for decomposing hot-store whole-blob CAS operations into fine-grained per-entity records on the unified RootFilesystem trait to eliminate writer contention. The review feedback provides valuable improvements, including handling potential Postgres casting errors during adjust_indexed disambiguation, ensuring the libSQL version-readback probe rolls back its scratch transaction, implementing ownership checks for user-owned resources, and centralizing overflow-safe period rollover calculations.
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.
| RETURNING ((indexed->>$key)::bigint) AS value, version; | ||
| ``` | ||
| - 1 row ⇒ applied; `value`/`version` from `RETURNING`. | ||
| - 0 rows ⇒ disambiguate with one `SELECT (indexed->>$key)::bigint, version WHERE path=$path` on the same connection: |
There was a problem hiding this comment.
In the proposed Postgres native implementation of adjust_indexed, when the UPDATE statement matches 0 rows, the design suggests disambiguating with a SELECT (indexed->>$key)::bigint, version WHERE path=$path query.
However, if the row exists but the value stored at $key is non-numeric (e.g., a string or boolean), casting it directly to bigint via (indexed->>$key)::bigint will throw a database runtime error (invalid input syntax for type bigint) instead of allowing the code to handle it gracefully.
To prevent this, the disambiguation query should retrieve the raw JSONB value (or use jsonb_typeof to guard the cast) and let the Rust driver validate and parse the type.
| - 0 rows ⇒ disambiguate with one `SELECT (indexed->>$key)::bigint, version WHERE path=$path` on the same connection: | |
| - 0 rows ⇒ disambiguate with one SELECT indexed->$key, version FROM root_filesystem_entries WHERE path=$path on the same connection (validating the type and parsing in Rust to avoid a Postgres cast error if the value is non-numeric): |
| **Native libSQL impl (libsql.rs):** `BEGIN IMMEDIATE … COMMIT` (the dialect gotcha — *not* deferred, so the write lock is taken up front; a deferred txn that upgrades mid-statement can hit `SQLITE_BUSY` after partial work and violate all-or-nothing). Per-put SQL is the libsql `?N`/`is_dir=0`/`strftime` variant. | ||
|
|
||
| **Version-readback (review B — libsql `put()` never reads version back today).** libsql `put()` returns version arithmetically (`expected.next()` / `from_backend(1)`), and there is **no `RETURNING` usage in libsql.rs today**. `put_batch` needs the real assigned version. Resolution, in order: | ||
| 1. At store init, **probe once** whether the bundled libSQL build supports statement-level `RETURNING` inside `BEGIN IMMEDIATE` (run `INSERT … RETURNING version` against a scratch row in a txn). Cache the result. |
There was a problem hiding this comment.
The proposed libSQL version-readback probe runs an INSERT … RETURNING version against a scratch row in a transaction at store initialization.
To ensure that this probe does not leave any persistent garbage or scratch rows in the database, the design should explicitly specify that this transaction must be rolled back (ROLLBACK) rather than committed.
| 1. At store init, **probe once** whether the bundled libSQL build supports statement-level `RETURNING` inside `BEGIN IMMEDIATE` (run `INSERT … RETURNING version` against a scratch row in a txn). Cache the result. | |
| 1. At store init, probe once whether the bundled libSQL build supports statement-level RETURNING inside BEGIN IMMEDIATE (run INSERT … RETURNING version against a scratch row in a txn and then immediately roll it back). Cache the result. |
| ``` | ||
| Message `indexed` gains `{ "thread_seg": Text, "sequence": I64, "run_id": Text }`; thread `indexed` gains `{ "scope_seg": Text, "updated_at": I64, "owner_user": Text }`. `list_threads_for_scope` becomes `query(/threads, Eq{scope_seg})` + Rust sort by `updated_at`, retiring the N+1 `list_dir`+per-file `get` (`filesystem_service.rs:1992-2048`). Range "messages [lo,hi]" → `query(Range{key:"sequence", lo:I64, hi:I64})`. | ||
| 3. **`ForceCas` escape hatch:** an operational flag (`ThreadWritePath { TxnOrCas (default), ForceCas }`) to disable the transactional path instantly if a backend's `begin`/`put_batch` misbehaves. Zero data migration, reversible. | ||
|
|
There was a problem hiding this comment.
When implementing services that interact with user-owned resources (such as the newly decomposed thread and run records), we must always perform an ownership check (e.g., verifying that the authenticated user ID matches the resource owner) before any read or write operations.
Ensure that the underlying resource coordinator or filesystem query is not invoked if the ownership check fails. This is a critical security boundary to prevent unauthorized cross-tenant or cross-user access.
| Message indexed gains { "thread_seg": Text, "sequence": I64, "run_id": Text }; thread indexed gains { "scope_seg": Text, "updated_at": I64, "owner_user": Text }. list_threads_for_scope becomes query(/threads, Eq{scope_seg}) + Rust sort by updated_at (after performing an ownership check to verify the authenticated user ID matches the resource owner), retiring the N+1 list_dir+per-file get (filesystem_service.rs:1992-2048). Range "messages [lo,hi]" → query(Range{key:"sequence", lo:I64, hi:I64}). |
References
- When implementing services that interact with user-owned resources, always perform an ownership check (e.g., verifying the authenticated user ID matches the resource owner) before any read or write operations. Ensure the underlying resource coordinator is not invoked if the ownership check fails.
|
|
||
| ### 7.5 Period rollover must be persisted (review C) | ||
|
|
||
| `advance_period_if_rolled_over` mutates `ledger.usage`/`period_end_at_anchor` in memory during the Phase-1 read. In the per-record model this in-memory mutation **must be committed via the Phase-2 write even when the reserve check is otherwise non-mutating** (e.g. a check that ultimately denies, or a pure rollover) — otherwise rollover silently never persists and the period never advances. Concretely: if Phase-1 advanced an anchor, that account's ledger is included in the Phase-2 `put_batch`/admission write with its rolled-over `usage`+`anchor`, CAS'd by its read version, **even if `requested` is zero or the reserve is denied on a different account**. A rollover write that loses its CAS simply retries (the next read sees the advanced anchor). This is an explicit per-account CAS write inside the cascade, with the same retry/idempotency as the reserve. |
There was a problem hiding this comment.
When calculating sliding-window cutoffs or period rollovers (such as period_end_at_anchor or advance_period_if_rolled_over), extremely large windows or anchors must not cause underflow or overflow that collapses the cutoff to the current time (which would incorrectly trim all entries).
Instead, ensure they saturate or default to a timestamp far in the past/future so that nothing is incorrectly trimmed or rolled over. This logic should be centralized in a canonical shared function to prevent behavioral drift across different backend implementations.
| `advance_period_if_rolled_over` mutates `ledger.usage`/`period_end_at_anchor` in memory during the Phase-1 read. In the per-record model this in-memory mutation **must be committed via the Phase-2 write even when the reserve check is otherwise non-mutating** (e.g. a check that ultimately denies, or a pure rollover) — otherwise rollover silently never persists and the period never advances. Concretely: if Phase-1 advanced an anchor, that account's ledger is included in the Phase-2 `put_batch`/admission write with its rolled-over `usage`+`anchor`, CAS'd by its read version, **even if `requested` is zero or the reserve is denied on a different account**. A rollover write that loses its CAS simply retries (the next read sees the advanced anchor). This is an explicit per-account CAS write inside the cascade, with the same retry/idempotency as the reserve. | |
| advance_period_if_rolled_over mutates ledger.usage/period_end_at_anchor in memory during the Phase-1 read (using a centralized, overflow-safe function to prevent extremely large windows from collapsing the cutoff to the current time). In the per-record model this in-memory mutation must be committed via the Phase-2 write even when the reserve check is otherwise non-mutating (e.g. a check that ultimately denies, or a pure rollover) — otherwise rollover silently never persists and the period never advances. Concretely: if Phase-1 advanced an anchor, that account's ledger is included in the Phase-2 put_batch/admission write with its rolled-over usage+anchor, CAS'd by its read version, even if requested is zero or the reserve is denied on a different account. A rollover write that loses its CAS simply retries (the next read sees the advanced anchor). This is an explicit per-account CAS write inside the cascade, with the same retry/idempotency as the reserve. |
References
- When calculating sliding-window cutoffs, ensure that extremely large windows (which may cause underflow or overflow) do not collapse the cutoff to the current time (which would incorrectly trim all entries). Instead, ensure they saturate or default to a timestamp far in the past so that nothing is trimmed. Centralize this logic in a canonical shared function to prevent behavioral drift across different backend implementations.
…ployment)
The hosted profile is RebornProfile::HostedSingleTenant — one tenant
(reborn-cli), many users — with per-user resource limits and no tenant-wide
cap. The generic design's headline ("different tenants never contend") gives
nothing on a single-tenant instance, and as written every reserve still writes
the shared tenant ledger.
Add §7.8 with the principle "maintain a hot per-account counter only where there
is a limit to enforce" → the limitless tenant/system levels leave the hot path;
the broadest hot account becomes the user ledger. Result: different users CAS
disjoint paths (zero cross-user contention); same user serializes on their own
ledger (correct). Tenant/global totals, if needed, are derived-on-read, never a
hot counter. Reconcile §7.3/§7.4 (admission gate is the broadest *limited*
account = user) and Open Q1 accordingly. Also preserves per-user independence as
the precondition for a future per-user shard/cell split.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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)
docs/plans/2026-06-26-native-storage-primitives.md (1)
362-367: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDon't call a SHA-256 path key injective.
A hash is fixed-length and path-safe, but it is not injective. This overstates the guarantee, and the proposed test only checks examples, not the invariant. If injectivity is required, use a reversible escaped encoding; otherwise reword this to “collision-resistant” and keep the hash as an implementation detail.
Suggested wording
- account_seg = hex(sha256(canonical_json(&account))) // fixed-length, path-safe, structurally injective + account_seg = hex(sha256(canonical_json(&account))) // fixed-length, path-safe, collision-resistant🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md` around lines 362 - 367, The plan currently overclaims that the hashed storage key is “injective”; a SHA-256-derived account_seg is collision-resistant and path-safe, but not reversible, so adjust the wording and requirement in canonical_json/account_seg to avoid promising injectivity. If true injectivity is needed, switch to a reversible escaped encoding instead of hashing; otherwise keep the hash approach and describe it as collision-resistant while preserving body.account and indexed.tenant/indexed.kind_tag for identity.
🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md`:
- Around line 411-413: The libSQL admission-gate paragraph is using
tenant-ledger wording that conflicts with the per-user/no-tenant-cap design.
Update the `libSQL (CAS-only floor)` description to consistently say the
admission point is the broadest limited account, specifically the `user` ledger
for this deployment, and remove references to per-tenant
serialization/contention so the text matches §7.8 and the `adjust_indexed`
admission flow.
---
Outside diff comments:
In `@docs/plans/2026-06-26-native-storage-primitives.md`:
- Around line 362-367: The plan currently overclaims that the hashed storage key
is “injective”; a SHA-256-derived account_seg is collision-resistant and
path-safe, but not reversible, so adjust the wording and requirement in
canonical_json/account_seg to avoid promising injectivity. If true injectivity
is needed, switch to a reversible escaped encoding instead of hashing; otherwise
keep the hash approach and describe it as collision-resistant while preserving
body.account and indexed.tenant/indexed.kind_tag for identity.
🪄 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: 416a1d16-b42f-423f-8d75-7e7eb3aa33a9
📒 Files selected for processing (1)
docs/plans/2026-06-26-native-storage-primitives.md
| - **libSQL (CAS-only floor):** there is no multi-key transaction, so we introduce a **narrow atomic admission record at the broadest cascade account that carries a limit**. Under this deployment's **per-user limits with no tenant cap, that is the `user` ledger** (§7.8); it would be the tenant ledger only if a tenant-wide cap existed. The reserve does its Phase-1 check, then performs the **commit as a single `adjust_indexed` on the admission ledger's `reserved_*` counter with `AtMost(limit)` guard** — this one conditional `UPDATE` takes the libSQL write lock and is the linearization point. If it `applied=false` (guard violation under concurrency), the reserve is denied — **correctly, atomically, no overshoot**. Only after the admission leg succeeds does the reserve apply the narrower-account deltas (project/agent/…) and write the reservation record, each idempotent (see below). The deeper accounts are *subordinate* to the admission; they apply with ordinary CAS retries because a breach of a deeper, narrower limit is caught by *its own* `adjust_indexed` `AtMost` guard in the same way — the key property is that **each account's own limit is enforced by its own atomic guarded adjust**, and the broadest *limited* account is the single serialization point that all reserves **sharing that budget** funnel through. Because limits are per-user, reserves from *different* users funnel through *different* admission ledgers and never contend. | ||
|
|
||
| This re-introduces a contention point, but a **per-tenant** one (the tenant ledger), not a **global** one. That is the correct and minimal surface: reserves in *different* tenants still never contend; reserves in the *same* tenant serialize on the tenant admission, which they must, because they share the tenant budget. This is strictly better than today's global, no-retry blob. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the libSQL admission gate on the user ledger.
This paragraph still talks about a per-tenant contention point, but §7.8 already makes this deployment per-user/no-tenant-cap. As written, the implementation would serialize the wrong row. Rewrite it to say “broadest limited account” or “user ledger” here and drop the tenant wording.
Suggested wording
- This re-introduces a contention point, but a per-tenant one (the tenant ledger), not a global one. That is the correct and minimal surface: reserves in different tenants still never contend; reserves in the same tenant serialize on the tenant admission, which they must, because they share the tenant budget.
+ This re-introduces a contention point on the broadest limited account for this deployment: the user ledger. Different users still never contend; same-user reserves serialize on their own budget, which they must.📝 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.
| - **libSQL (CAS-only floor):** there is no multi-key transaction, so we introduce a **narrow atomic admission record at the broadest cascade account that carries a limit**. Under this deployment's **per-user limits with no tenant cap, that is the `user` ledger** (§7.8); it would be the tenant ledger only if a tenant-wide cap existed. The reserve does its Phase-1 check, then performs the **commit as a single `adjust_indexed` on the admission ledger's `reserved_*` counter with `AtMost(limit)` guard** — this one conditional `UPDATE` takes the libSQL write lock and is the linearization point. If it `applied=false` (guard violation under concurrency), the reserve is denied — **correctly, atomically, no overshoot**. Only after the admission leg succeeds does the reserve apply the narrower-account deltas (project/agent/…) and write the reservation record, each idempotent (see below). The deeper accounts are *subordinate* to the admission; they apply with ordinary CAS retries because a breach of a deeper, narrower limit is caught by *its own* `adjust_indexed` `AtMost` guard in the same way — the key property is that **each account's own limit is enforced by its own atomic guarded adjust**, and the broadest *limited* account is the single serialization point that all reserves **sharing that budget** funnel through. Because limits are per-user, reserves from *different* users funnel through *different* admission ledgers and never contend. | |
| This re-introduces a contention point, but a **per-tenant** one (the tenant ledger), not a **global** one. That is the correct and minimal surface: reserves in *different* tenants still never contend; reserves in the *same* tenant serialize on the tenant admission, which they must, because they share the tenant budget. This is strictly better than today's global, no-retry blob. | |
| - **libSQL (CAS-only floor):** there is no multi-key transaction, so we introduce a **narrow atomic admission record at the broadest cascade account that carries a limit**. Under this deployment's **per-user limits with no tenant cap, that is the `user` ledger** (§7.8); it would be the tenant ledger only if a tenant-wide cap existed. The reserve does its Phase-1 check, then performs the **commit as a single `adjust_indexed` on the admission ledger's `reserved_*` counter with `AtMost(limit)` guard** — this one conditional `UPDATE` takes the libSQL write lock and is the linearization point. If it `applied=false` (guard violation under concurrency), the reserve is denied — **correctly, atomically, no overshoot**. Only after the admission leg succeeds does the reserve apply the narrower-account deltas (project/agent/…) and write the reservation record, each idempotent (see below). The deeper accounts are *subordinate* to the admission; they apply with ordinary CAS retries because a breach of a deeper, narrower limit is caught by *its own* `adjust_indexed` `AtMost` guard in the same way — the key property is that **each account's own limit is enforced by its own atomic guarded adjust**, and the broadest *limited* account is the single serialization point that all reserves **sharing that budget** funnel through. Because limits are per-user, reserves from *different* users funnel through *different* admission ledgers and never contend. | |
| - | |
| **libSQL (CAS-only floor):** there is no multi-key transaction, so we introduce a **narrow atomic admission record at the broadest cascade account that carries a limit**. Under this deployment's **per-user limits with no tenant cap, that is the `user` ledger** (§7.8); it would be the tenant ledger only if a tenant-wide cap existed. The reserve does its Phase-1 check, then performs the **commit as a single `adjust_indexed` on the admission ledger's `reserved_*` counter with `AtMost(limit)` guard** — this one conditional `UPDATE` takes the libSQL write lock and is the linearization point. If it `applied=false` (guard violation under concurrency), the reserve is denied — **correctly, atomically, no overshoot**. Only after the admission leg succeeds does the reserve apply the narrower-account deltas (project/agent/…) and write the reservation record, each idempotent (see below). The deeper accounts are *subordinate* to the admission; they apply with ordinary CAS retries because a breach of a deeper, narrower limit is caught by *its own* `adjust_indexed` `AtMost` guard in the same way — the key property is that **each account's own limit is enforced by its own atomic guarded adjust**, and the broadest *limited* account is the single serialization point that all reserves **sharing that budget** funnel through. Because limits are per-user, reserves from *different* users funnel through *different* admission ledgers and never contend. | |
| This re-introduces a contention point on the broadest limited account for this deployment: the `user` ledger. Different users still never contend; same-user reserves serialize on their own budget, which they must. |
🤖 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 `@docs/plans/2026-06-26-native-storage-primitives.md` around lines 411 - 413,
The libSQL admission-gate paragraph is using tenant-ledger wording that
conflicts with the per-user/no-tenant-cap design. Update the `libSQL (CAS-only
floor)` description to consistently say the admission point is the broadest
limited account, specifically the `user` ledger for this deployment, and remove
references to per-tenant serialization/contention so the text matches §7.8 and
the `adjust_indexed` admission flow.
Multi-lens design review — native hot-store primitivesReviewed through five lenses (approach, maintainability, local-patterns, thermo-nuclear architecture, and a dedicated concurrency/CAS/TOCTOU pass). Every load-bearing code anchor in the doc was verified against the real tree (23/25 match — the two misses are called out below). First: this is a genuinely strong doc. The grounding is accurate, the parity invariant (§3.7) is well-argued, the honest downgrades (at-least-once events §8.3, premise correction §8.0, TL;DR verdict
Critical (governor)C1 — The libSQL "single linearization point" is architecturally false for multi-dimensional limits (§7.4). B now holds a committed C2 — The crash-sweeper that C3 — The gap widens monotonically with each reconcile → admission rate degrades over process lifetime. This is duplicate-truth (maintainability rubric: same value, 2 places, hot path writes only one, divergence breaks behavior). C4 — mid-cascade partial commit on libSQL has no compensation path (§7.3/§7.4). Reserve admits the broadest limited ledger, then applies deeper-account deltas. If a deeper guard fails (or a tenant cap is ever added per §7.8), the already-committed broader The convergent fix — pick one, in preference orderAll four non-local reviewers independently pointed at the same resolution space. C1–C4 are all artifacts of forcing an all-or-nothing, multi-dimensional, relational quota onto a single-key JSONB primitive. Option A (recommended) — Governor as a relational sidecar, exactly like triggers already did. UPDATE resource_account_ledgers
SET reserved_usd = reserved_usd + $d_usd, reserved_input_tokens = reserved_input_tokens + $d_tok, ...
version = version + 1
WHERE account_id=$id AND version=$v
AND reserved_usd + $d_usd <= $lim_usd
AND reserved_input_tokens + $d_tok <= $lim_tok ...
RETURNING version;One atomic multi-dimensional guarded UPDATE → C1, C2 (no sweeper), C3 (real columns, no dual-truth), C4 all vanish, plus Postgres HOT updates for free (no index churn — see P1). Cost: two native schemas instead of one trait copy, and the governor loses the generic Option B — Pull Open Question §12.4 into PR-3 as a prerequisite. Raise libSQL to Option C — Drop My read: A is the cleanest and matches the repo's own precedent; B is the smallest diff if the trait-freeze is treated as inviolable; C should fold in regardless. Performance gaps the doc under-sells (independent of the above)P1 — JSONB write-amplification on hot counters (§6.2). P2 — Expression indexes aren't prefix-partial → planner degradation at scale (§7.6/§8.4). P3 — Row-count explosion + event purge gap (§8.2). Decomposition multiplies 1 turn blob → 1 run row + 5–10 UPDATEs each → millions of dead tuples; the doc doesn't quantify VACUUM surface. And P4 — Vector honesty gap (§3.4 vs reality). Mounts advertise Correctness of the doc itself — fix before implementers startThese will cause compile errors or wasted diagnosis:
Naming (local-patterns)
Lower-severity concurrency (worth pinning)
What's already strong (keep)Postgres Review synthesized from a 5-lens agent pass (approach / maintainability / local-patterns / thermo-nuclear architecture / concurrency-TOCTOU), each grounding its claims against the live tree. |
|
🚅 Deployed to the ironclaw-pr-5269 environment in ironclaw-ci-preview
|
Address the 2026-06-25 multi-lens review (approach / maintainability / local-patterns / thermo-nuclear / concurrency-TOCTOU) plus CodeRabbit and Gemini. The governor's four critical concurrency/correctness defects are resolved by a single structural change rather than wording tweaks. Core change — governor commits via ONE put_batch path on both backends: - Drop the adjust_indexed primitive entirely. Its single-key conditional delta could not make the 8-dimensional ResourceTally admission atomic (concurrent same-user reserves could over-admit), re-introduced a body/indexed dual-truth hazard, and leaned on a crash-sweeper that does not exist in the tree. - Raise libSQL to MultiKey via the BEGIN IMMEDIATE the put_batch override already needs; route the governor cascade through put_batch (full tally per account record) on Postgres AND libSQL. This retires the libSQL admission-gate special case, applied_reservations, and the phantom sweeper, and keeps counters single-source in body. Also: honest libSQL file-global write-lock disclosure; correct the shared Postgres pool analysis (one pool, default size 2, must raise in PR-2); fold event-log into the turns write-mode flag (no split-brain state); prefix-partial expression indexes; IndexVector filtered via existing BackendCapabilities::without; period-rollover overflow-safe + denied-reserve posture; ownership checks on decomposed query paths; probe ROLLBACK; rename StoreModel -> StoreWriteMode; fix stale code anchors (HeadSeq already exists, cas_snapshot.rs:357, libsql.rs:1595, lifecycle.rs:530-567, cascade min 2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revision pushed (0e18f93) — all review feedback addressedRan the design through 3 iterative review rounds (approach / maintainability / local-patterns / thermo-nuclear architecture / concurrency-TOCTOU) plus the CodeRabbit and Gemini findings, revising until clean. Final architecture verdict: ship-the-plan. Summary of what changed: Governor — the 4 critical concurrency/correctness defects, fixed structurallyThe earlier Resolution — one mechanism, both backends: drop CodeRabbit
Gemini
Honesty / perf gaps now disclosed (not hidden)
Maintainability / naming / anchors
§12 records the relational-sidecar (triggers-style) alternative as considered and deferred — the |
This branch was successfully deployed
2 participants
What
A design doc (no code) for making the reborn Postgres access pattern native — per-row, indexed, partial-update — without changing the
RootFilesystemabstraction or losing dual-backend (libSQL + Postgres). Adds docs/plans/2026-06-26-native-storage-primitives.md. It generalizes the #5232 lease-sidecar pattern across the remaining hot stores.Why
The four hot stores funnel every mutation through a whole-blob read-modify-write CAS on one
Entry, serializing logically-disjoint writers. The worst is the process-global/resources/snapshot.jsonunderResourceScope::system()— CAS-contended across all tenants with no retry. (v1 never had this: it used native per-rowINSERT/UPDATEon a 30-deep pool. The unification PR #3659 explicitly deferred native-record adoption to per-consumer follow-ups, which the hot stores never did.)The design (keeps the trait frozen)
put_batch(atomic multi-put) andadjust_indexed(atomic conditional numeric delta) — each with a default impl built from existing ops (byte-only/in-memory backends unchanged) plus native Postgres AND native libSQL overrides, gated behind capability bits and mirrored onScopedFilesystem. (A third proposed primitive,merge_indexed, was dropped to avoid a PG/SQLite null-divergence hazard.)Hot stores
system()snapshot → one record per account (sha256(canonical_json(account))key) → two tenants CAS disjoint paths and never contend; same-tenant serializes only on the shared tenant ledger. The global hot row is eliminated.put_batch+ aForceCasescape hatch (no row decomposition).Hard cases resolved honestly (not hand-waved)
adjust_indexed; Postgres usesput_batchversion-CAS serialization. Per-tenant contention, never global.account_seginjectivity (Displayrenders absent slots as literal_→ collision) →hex(sha256(canonical_json(account))).RETURNING-in-txn today → init-time probe + in-txnSELECTfallback + advertise the capability only after the probe confirms./turns/state.jsonissystem()-scoped and multiplexes all tenants' runs (not per-tenant); migration re-keys each run by its ownTurnScope.Rollout
Per-store
StoreModelflag (Snapshot → DualWrite → Native), each store independent (disjoint prefixes), reversible at every step (rematerialize_snapshot()is a first-class artifact), green on both backends at every promotion. Order: 4 filesystem-primitive PRs first (trait+defaults+ScopedFilesystemwrappers → PG native → libSQL native+probe → in-memory atomicity audit), then Governor → Turn-state → Event-log → Threads.Provenance
Synthesized via a 5-phase multi-agent design workflow: ground (4 readers over the actual trait + backends + hot stores) → 3 independent design angles → adversarial review of each → synthesis → invariant verification. Verify signoff: ACCEPT, all six invariants pass, no blockers (it even verified
operation_allowedis an exhaustive match, so the new permission arms are compile-required). One residual to enforce at implementation time: in-memory default-path atomicity foradjust_indexed(PR-4).Status / next steps
Design only — accepted, not implemented. Natural first implementation step is PR-1: the trait + capability surface +
ScopedFilesystemwrappers + parameterized contract infra (pure addition, zero behavior change). Companion to the write-behind/lease design (#5249).🤖 Generated with Claude Code