perf(storage): row-native sequence primitive + thread/turn append paths - #5455
Conversation
Reduces durable write pressure on the per-turn storage path that dominates p95 once the resource governor and journal mode are no longer the limit. - Add a `reserve_sequence` RootFilesystem primitive for path-local monotonic sequence allocation, implemented for libSQL, Postgres, and the in-memory backend (migration V32 adds the libSQL/Postgres sequence table), surfaced through the scoped dispatch fabric and capability set. - Rework thread storage to use sequence reservation + a finalized assistant-append path, collapsing the per-turn full-document rewrites in accept_inbound / append_assistant / finalize into smaller appends. - Carry the same append-native shape through the turn-state store and runner lease records, and update the stress harness's user-turn path to exercise the finalized-append flow. This lifts the storage portion of #5453, dropping that PR's resource-governor commits which are superseded by the already-merged #5447 (unlimited-budget durable-write skip). It builds on the WAL change in #5451. Measured (this machine, chat-turn, 30 ops/task, 200 users), WAL-only vs WAL + this change: c=8 : thread_store_writes p95 126.2ms -> 106.4ms ; throughput 33.6 -> 36.2 ops/s c=32: thread_store_writes p95 186.0ms -> 97.0ms ; throughput 19.5 -> 34.2 ops/s aggregate p95 276.4ms -> 183.6ms Throughput previously collapsed past c8 (19.5 ops/s at c32); with the append-native paths it holds flat from c8 to c32. Co-Authored-By: firat.sertgoz <firatsertgoz@alumni.sabanciuniv.edu> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
github-actions
Bot
added
scope: db/postgres
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds ChangesFilesystem sequence reservation and thread append flow
Stress benchmark artifacts
Sequence Diagram(s)sequenceDiagram
participant SessionThreadService
participant FilesystemSessionThreadService
participant ThreadFilesystem
participant AppendLog
SessionThreadService->>FilesystemSessionThreadService: append_finalized_assistant_message(request)
FilesystemSessionThreadService->>ThreadFilesystem: reserve_sequence / write message
FilesystemSessionThreadService->>AppendLog: append_message_event
AppendLog-->>FilesystemSessionThreadService: supported or fallback
FilesystemSessionThreadService-->>SessionThreadService: ThreadMessageRecord
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 a path-local monotonic sequence reservation mechanism (reserve_sequence) across multiple filesystem backends to avoid CAS bottlenecks, adds support for appending finalized assistant messages directly, and implements a process-local cache for runner lease heartbeats to reduce durable writes. Feedback on these changes highlights a critical serialization mismatch where StoredThreadMessageRecord is serialized but deserialized as ThreadMessageRecord, which will cause runtime failures. Additionally, the list_threads_for_scope operation should be optimized to avoid O(N) directory scans by using index-based queries, and its sorting logic should be updated to prioritize updated_at as the primary sort key.
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 latest_sequence_results: Vec<(ThreadId, Result<u64, SessionThreadError>)> = | ||
| futures::stream::iter( | ||
| listed | ||
| .iter() | ||
| .map(|(record, _, _)| record.thread_id.clone()) | ||
| .collect::<Vec<_>>(), | ||
| ) | ||
| .map(|thread_id| { | ||
| let scope = request.scope.clone(); | ||
| async move { | ||
| let result = | ||
| self.list_thread_messages(&scope, &thread_id) | ||
| .await | ||
| .map(|messages| { | ||
| messages | ||
| .into_iter() | ||
| .map(|message| message.sequence) | ||
| .max() | ||
| .unwrap_or(0) | ||
| }); | ||
| (thread_id, result) | ||
| } | ||
| }) | ||
| .buffer_unordered(LIST_THREADS_RECORD_READ_CONCURRENCY) | ||
| .collect() | ||
| .await; |
There was a problem hiding this comment.
The filesystem-backed listing operation in list_threads_for_scope performs O(N) directory scans by calling self.list_thread_messages for every thread. To optimize this, use index-based queries to avoid O(N) directory scans. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported.
References
- Optimize filesystem-backed listing operations by using index-based queries to avoid O(N) directory scans. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported.
| listed.sort_by(|(a, _, a_latest_sequence), (b, _, b_latest_sequence)| { | ||
| let a_key = a.updated_at.or(a.created_at); | ||
| let b_key = b.updated_at.or(b.created_at); | ||
| std::cmp::Reverse(a_key) | ||
| .cmp(&std::cmp::Reverse(b_key)) | ||
| std::cmp::Reverse(*a_latest_sequence > 0) | ||
| .cmp(&std::cmp::Reverse(*b_latest_sequence > 0)) | ||
| .then_with(|| { | ||
| std::cmp::Reverse(*a_latest_sequence) | ||
| .cmp(&std::cmp::Reverse(*b_latest_sequence)) | ||
| }) | ||
| .then_with(|| std::cmp::Reverse(a_key).cmp(&std::cmp::Reverse(b_key))) | ||
| .then_with(|| a.thread_id.as_str().cmp(b.thread_id.as_str())) | ||
| }); |
There was a problem hiding this comment.
The sorting logic in list_threads_for_scope uses latest_sequence as a proxy for activity recency. When sorting product-facing projections, use updated_at (descending) as the primary sort key to prioritize recent activity, and include a deterministic tie-breaker (e.g., last_cursor).
References
- When sorting product-facing projections, use
updated_at(descending) as the primary sort key to prioritize recent activity, and include a deterministic tie-breaker (e.g.,last_cursor).
| let mut messages = Vec::with_capacity(events.len()); | ||
| for event in events { | ||
| let message = deserialize::<ThreadMessageRecord>(&event.payload)?; | ||
| if &message.thread_id == thread_id { | ||
| messages.push((message, RecordVersion::from_backend(event.seq.get()))); | ||
| } | ||
| } |
There was a problem hiding this comment.
In append_message_event, the message is serialized as a StoredThreadMessageRecord:
let payload = serialize_pretty(&StoredThreadMessageRecord::from(message))?;However, in read_message_append_events, the event payload is deserialized directly as a ThreadMessageRecord:
let message = deserialize::<ThreadMessageRecord>(&event.payload)?;If StoredThreadMessageRecord is a wrapper struct, deserializing it directly as ThreadMessageRecord will fail at runtime due to structural mismatch.
Please deserialize as StoredThreadMessageRecord first, and then extract the inner ThreadMessageRecord.
| let mut messages = Vec::with_capacity(events.len()); | |
| for event in events { | |
| let message = deserialize::<ThreadMessageRecord>(&event.payload)?; | |
| if &message.thread_id == thread_id { | |
| messages.push((message, RecordVersion::from_backend(event.seq.get()))); | |
| } | |
| } | |
| let mut messages = Vec::with_capacity(events.len()); | |
| for event in events { | |
| let stored = deserialize::<StoredThreadMessageRecord>(&event.payload)?; | |
| if &stored.record.thread_id == thread_id { | |
| messages.push((stored.record, RecordVersion::from_backend(event.seq.get()))); | |
| } | |
| } |
There was a problem hiding this comment.
Code Review
This pull request introduces path-local monotonic sequence reservation across filesystem backends (in-memory, libSQL, and PostgreSQL) and adds a process-local caching mechanism for runner leases in the turns store to optimize heartbeat performance. It also introduces an API to append finalized assistant messages directly. However, the review highlights critical issues in the thread service: comparing raw sequence numbers across different threads breaks the 'newest activity first' sorting logic, subsequent CAS updates or redactions will fail on appended messages due to missing individual files, and listing threads introduces a severe O(N * M) performance regression by scanning all messages for every thread.
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.
| listed.sort_by(|(a, _, a_latest_sequence), (b, _, b_latest_sequence)| { | ||
| let a_key = a.updated_at.or(a.created_at); | ||
| let b_key = b.updated_at.or(b.created_at); | ||
| std::cmp::Reverse(a_key) | ||
| .cmp(&std::cmp::Reverse(b_key)) | ||
| std::cmp::Reverse(*a_latest_sequence > 0) | ||
| .cmp(&std::cmp::Reverse(*b_latest_sequence > 0)) | ||
| .then_with(|| { | ||
| std::cmp::Reverse(*a_latest_sequence) | ||
| .cmp(&std::cmp::Reverse(*b_latest_sequence)) | ||
| }) | ||
| .then_with(|| std::cmp::Reverse(a_key).cmp(&std::cmp::Reverse(b_key))) | ||
| .then_with(|| a.thread_id.as_str().cmp(b.thread_id.as_str())) | ||
| }); |
There was a problem hiding this comment.
Critical Bug: Incorrect Sorting Logic Across Threads
Comparing raw sequence numbers (a_latest_sequence and b_latest_sequence) directly across different threads is functionally incorrect.
When sorting product-facing projections, use updated_at (descending) as the primary sort key to prioritize recent activity, and include a deterministic tie-breaker (e.g., last_cursor). Comparing local sequence numbers across threads breaks the "Newest activity first" sorting behavior, causing older, highly-active threads to permanently pin to the top of the list while newer threads with fewer messages are pushed to the bottom.
References
- When sorting product-facing projections, use
updated_at(descending) as the primary sort key to prioritize recent activity, and include a deterministic tie-breaker (e.g.,last_cursor).
| else { | ||
| return Ok(None); | ||
| let Some(events) = self.read_message_append_events(scope, thread_id).await? else { | ||
| return Ok(None); | ||
| }; | ||
| return Ok(events | ||
| .into_iter() | ||
| .find(|(record, _)| record.message_id == message_id)); | ||
| }; |
There was a problem hiding this comment.
Critical Bug: CAS Update/Redaction Failures on Appended Messages
When a message is stored only in the append log (via append_message_event), there is no individual file created at message_virtual_path.
While read_message_versioned will successfully find the message in the append log and return its version derived from the append log's sequence number (RecordVersion::from_backend(event.seq.get())), any subsequent attempt to update or redact this message (e.g., via apply_message_update) will fail.
This is because apply_message_update attempts to write the updated message back to message_virtual_path using CasExpectation::Version(version). Since the individual file at message_virtual_path does not exist, the CAS write will fail with a NotFound or VersionMismatch error, making it impossible to update or redact any appended messages.
| let latest_sequence_results: Vec<(ThreadId, Result<u64, SessionThreadError>)> = | ||
| futures::stream::iter( | ||
| listed | ||
| .iter() | ||
| .map(|(record, _, _)| record.thread_id.clone()) | ||
| .collect::<Vec<_>>(), | ||
| ) | ||
| .map(|thread_id| { | ||
| let scope = request.scope.clone(); | ||
| async move { | ||
| let result = | ||
| self.list_thread_messages(&scope, &thread_id) | ||
| .await | ||
| .map(|messages| { | ||
| messages | ||
| .into_iter() | ||
| .map(|message| message.sequence) | ||
| .max() | ||
| .unwrap_or(0) | ||
| }); | ||
| (thread_id, result) | ||
| } | ||
| }) | ||
| .buffer_unordered(LIST_THREADS_RECORD_READ_CONCURRENCY) | ||
| .collect() | ||
| .await; |
There was a problem hiding this comment.
Performance Regression: O(N * M) Filesystem Reads on Thread Listing
This block introduces a severe performance regression in list_threads_for_scope. For every single thread in the scope (N), it calls list_thread_messages which reads and deserializes all messages (M) for that thread.
To resolve this, optimize filesystem-backed listing operations by using index-based queries to avoid O(N) directory scans. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported. This avoids loading and deserializing the entire message history of every thread.
References
- Optimize filesystem-backed listing operations by using index-based queries to avoid O(N) directory scans. Implement a fallback to directory scanning only when the backend explicitly reports the index operation as unsupported.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/ironclaw_stress/src/user_turn.rs (1)
1243-1263: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winKeep the governor calls off the Tokio worker In
tools/ironclaw_stress/src/user_turn.rs:1243-1263,with_unlimited_fast_path()still hits synchronousstore.inspect(...)on each reserve/reconcile/release, and the backingPersistentResourceGovernoris filesystem-backed. Inline calls here still do blocking I/O and can skew the stress numbers; keepspawn_blockinghere or cache the fast-path decision once up front.🤖 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 `@tools/ironclaw_stress/src/user_turn.rs` around lines 1243 - 1263, The resource governor path in with_unlimited_fast_path(), resource_reserve, resource_reconcile, and release_resources is still performing synchronous filesystem-backed work on the Tokio worker. Update these call sites so the store.inspect fast-path check is either cached once up front or the governor operations remain wrapped in spawn_blocking, and keep the blocking PersistentResourceGovernor interactions off the async worker to avoid skewing stress timing.
🤖 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 263-271: The ReserveSeq counter is now path-scoped, but the delete
paths still only remove entry data and leave sequence state behind, so
delete/recreate can reuse stale counters. Update the deletion flow in the
relevant filesystem implementations and the owning service path to also clear
the reserved sequence state for the exact path or any prefix-based subtree
before reuse, using the reserve_sequence/FilesystemOperation::ReserveSeq
contract and the existing delete handlers in in_memory, libsql, postgres, and
filesystem_service as the places to align.
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1498-1564: The append-only branch in
append_finalized_assistant_message is skipping the normal file and
sequence-index materialization, which leaves finalized assistant messages
invisible to later mutations and range reads. Update this path so that a
successful append_message_event also persists the message record and sequence
index, using the same write path as write_new_message, or otherwise ensure
apply_message_update and materialize_message_range can load and merge append-log
entries before relying on file/index state. Keep the existing draft-update path
in append_finalized_assistant_message and align the new-message path with the
contracts used by MessageSequenceIndexStore and apply_message_update.
- Around line 2209-2271: The activity sort in `list_threads_for_scope` is using
per-thread `max(message.sequence)` as `latest_sequence`, but that is not
comparable across threads and can mis-rank newer activity. Replace this
cross-thread ordering input with a real activity signal from
`SessionThreadRecord` (for example `updated_at`/`created_at` or another shared
timestamp) in the `latest_sequence_results`/`listed.sort_by` flow, and adjust
the fallback so the sort reflects true recency rather than transcript length.
In `@crates/ironclaw_threads/tests/filesystem_session_thread_contract.rs`:
- Around line 275-313: The test in
filesystem_append_finalized_assistant_message_finalizes_existing_draft_by_turn_run
should verify the persisted in-place finalize behavior, not only the returned
record from append_finalized_assistant_message. After finalizing, assert
finalized_assistant_message_by_run returns the same draft.message_id and that
list_thread_history shows exactly one message, so the contract rules out
creating a second finalized history row. Use the existing service methods and
turn_run_id/thread_id flow in this test to confirm the caller-visible single-row
invariant.
In `@crates/ironclaw_turns/src/filesystem_store.rs`:
- Around line 398-423: The cached heartbeat path in
try_heartbeat_runner_lease_from_cache updates only the in-memory lease, so
recover_expired_leases can still act on stale durable data and requeue a run
that was just heartbeated. Merge or flush the RunnerLeaseRecord from
try_heartbeat_runner_lease_from_cache into the durable RunnerLeaseOverlay before
expiry recovery runs, or force a durable refresh so recovery evaluates the
latest heartbeat state; make the corresponding change in the recovery flow that
uses RunnerLeaseOverlay::All. Add a contract test covering a cached heartbeat
followed by recovery before the next durable refresh, ensuring the run stays
active until resume/cancel/fail/complete.
- Line 166: The runner_lease_cache in FilesystemStore is keyed by String even
though it represents run identity; switch it to use TurnRunId directly and
update the related lookup/insert/remove logic in the FilesystemStore methods
that access this cache. If ordering is required for the BTreeMap, add Ord to
TurnRunId; otherwise consider a typed HashMap<TurnRunId, _>. Make sure the
affected code paths no longer stringify TurnRunId and instead pass the newtype
through consistently.
In `@crates/ironclaw_turns/tests/filesystem_turn_state_contract.rs`:
- Around line 190-209: `overwrite_runner_lease_sidecar_times` treats `None` as
“leave unchanged,” so callers cannot express a cleared JSON field for
`last_heartbeat_at` or `lease_expires_at`. Update this helper to use an explicit
tri-state for each sidecar field, such as `Keep` / `Clear` / `Set(...)` or
`Option<Option<DateTime<Utc>>>`, and adjust the callers in the recovery/CAS
tests to pass the desired state explicitly. This will let the tests cover
missing/null sidecar values instead of only preserving existing fields.
In `@migrations/V32__root_filesystem_sequences.sql`:
- Around line 7-11: Add the missing V32 entry to the migration checksum lock so
the released-migration immutability check passes. After creating or confirming
the V32__root_filesystem_sequences migration, update migrations/checksums.lock
to include its hash in sequence after V31__root_filesystem_path_collation, using
the same checksum format as the existing entries.
---
Outside diff comments:
In `@tools/ironclaw_stress/src/user_turn.rs`:
- Around line 1243-1263: The resource governor path in
with_unlimited_fast_path(), resource_reserve, resource_reconcile, and
release_resources is still performing synchronous filesystem-backed work on the
Tokio worker. Update these call sites so the store.inspect fast-path check is
either cached once up front or the governor operations remain wrapped in
spawn_blocking, and keep the blocking PersistentResourceGovernor interactions
off the async worker to avoid skewing stress timing.
🪄 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: 1f710ff6-1517-4d04-97ab-f2d490c97a18
📒 Files selected for processing (19)
crates/ironclaw_filesystem/src/catalog.rscrates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_filesystem/src/root.rscrates/ironclaw_filesystem/src/scoped.rscrates/ironclaw_filesystem/src/types.rscrates/ironclaw_threads/src/contract.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/src/in_memory.rscrates/ironclaw_threads/src/lib.rscrates/ironclaw_threads/src/service.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_threads/tests/session_thread_contract.rscrates/ironclaw_turns/src/filesystem_store.rscrates/ironclaw_turns/src/filesystem_store/runner_lease.rscrates/ironclaw_turns/tests/filesystem_turn_state_contract.rsmigrations/V32__root_filesystem_sequences.sqltools/ironclaw_stress/src/user_turn.rs
| async fn filesystem_append_finalized_assistant_message_finalizes_existing_draft_by_turn_run() { | ||
| let backend = Arc::new(InMemoryBackend::new()); | ||
| let scoped = scoped_threads_fs_at(backend, "tenant-finalized-existing-draft", "alice"); | ||
| let service = FilesystemSessionThreadService::new(scoped); | ||
| let scope = scope("finalized-existing-draft"); | ||
| let thread = service | ||
| .ensure_thread(EnsureThreadRequest { | ||
| scope: scope.clone(), | ||
| thread_id: Some(ThreadId::new("thread-finalized-existing-draft").unwrap()), | ||
| created_by_actor_id: "actor-a".into(), | ||
| title: None, | ||
| metadata_json: None, | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let draft = service | ||
| .append_assistant_draft(AppendAssistantDraftRequest { | ||
| scope: scope.clone(), | ||
| thread_id: thread.thread_id.clone(), | ||
| turn_run_id: "run-finalized-existing-draft".into(), | ||
| content: MessageContent::text("draft answer"), | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
| let finalized = service | ||
| .append_finalized_assistant_message(AppendFinalizedAssistantMessageRequest { | ||
| scope, | ||
| thread_id: thread.thread_id, | ||
| turn_run_id: "run-finalized-existing-draft".into(), | ||
| content: MessageContent::text("final answer"), | ||
| }) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(finalized.message_id, draft.message_id); | ||
| assert_eq!(finalized.status, MessageStatus::Finalized); | ||
| assert_eq!(finalized.content.as_deref(), Some("final answer")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the in-place finalize contract, not just the returned record.
This test still passes if the draft is finalized and a second finalized history row is materialized by the append-log path. Please also assert finalized_assistant_message_by_run(...) returns draft.message_id and list_thread_history(...).messages.len() == 1, since “finalize existing draft by turn_run_id” is a caller-visible single-row invariant here. As per path instructions, "Test through the caller: when a helper gates a side effect, require a test driving the real call site" — the side effect to lock in is the persisted/history shape, not only the returned value.
Suggested assertion expansion
+ let scope_for_reads = scope.clone();
+ let thread_id_for_reads = thread.thread_id.clone();
let finalized = service
.append_finalized_assistant_message(AppendFinalizedAssistantMessageRequest {
scope,
thread_id: thread.thread_id,
turn_run_id: "run-finalized-existing-draft".into(),
@@
assert_eq!(finalized.message_id, draft.message_id);
assert_eq!(finalized.status, MessageStatus::Finalized);
assert_eq!(finalized.content.as_deref(), Some("final answer"));
+
+ let by_run = service
+ .finalized_assistant_message_by_run(FinalizedAssistantMessageByRunRequest {
+ scope: scope_for_reads.clone(),
+ thread_id: thread_id_for_reads.clone(),
+ turn_run_id: "run-finalized-existing-draft".into(),
+ })
+ .await
+ .unwrap()
+ .expect("finalized assistant message should be indexed by run");
+ assert_eq!(by_run.message_id, draft.message_id);
+
+ let history = service
+ .list_thread_history(ThreadHistoryRequest {
+ scope: scope_for_reads,
+ thread_id: thread_id_for_reads,
+ })
+ .await
+ .unwrap();
+ assert_eq!(history.messages.len(), 1);
+ assert_eq!(history.messages[0].message_id, draft.message_id);📝 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.
| async fn filesystem_append_finalized_assistant_message_finalizes_existing_draft_by_turn_run() { | |
| let backend = Arc::new(InMemoryBackend::new()); | |
| let scoped = scoped_threads_fs_at(backend, "tenant-finalized-existing-draft", "alice"); | |
| let service = FilesystemSessionThreadService::new(scoped); | |
| let scope = scope("finalized-existing-draft"); | |
| let thread = service | |
| .ensure_thread(EnsureThreadRequest { | |
| scope: scope.clone(), | |
| thread_id: Some(ThreadId::new("thread-finalized-existing-draft").unwrap()), | |
| created_by_actor_id: "actor-a".into(), | |
| title: None, | |
| metadata_json: None, | |
| }) | |
| .await | |
| .unwrap(); | |
| let draft = service | |
| .append_assistant_draft(AppendAssistantDraftRequest { | |
| scope: scope.clone(), | |
| thread_id: thread.thread_id.clone(), | |
| turn_run_id: "run-finalized-existing-draft".into(), | |
| content: MessageContent::text("draft answer"), | |
| }) | |
| .await | |
| .unwrap(); | |
| let finalized = service | |
| .append_finalized_assistant_message(AppendFinalizedAssistantMessageRequest { | |
| scope, | |
| thread_id: thread.thread_id, | |
| turn_run_id: "run-finalized-existing-draft".into(), | |
| content: MessageContent::text("final answer"), | |
| }) | |
| .await | |
| .unwrap(); | |
| assert_eq!(finalized.message_id, draft.message_id); | |
| assert_eq!(finalized.status, MessageStatus::Finalized); | |
| assert_eq!(finalized.content.as_deref(), Some("final answer")); | |
| } | |
| async fn filesystem_append_finalized_assistant_message_finalizes_existing_draft_by_turn_run() { | |
| let backend = Arc::new(InMemoryBackend::new()); | |
| let scoped = scoped_threads_fs_at(backend, "tenant-finalized-existing-draft", "alice"); | |
| let service = FilesystemSessionThreadService::new(scoped); | |
| let scope = scope("finalized-existing-draft"); | |
| let thread = service | |
| .ensure_thread(EnsureThreadRequest { | |
| scope: scope.clone(), | |
| thread_id: Some(ThreadId::new("thread-finalized-existing-draft").unwrap()), | |
| created_by_actor_id: "actor-a".into(), | |
| title: None, | |
| metadata_json: None, | |
| }) | |
| .await | |
| .unwrap(); | |
| let draft = service | |
| .append_assistant_draft(AppendAssistantDraftRequest { | |
| scope: scope.clone(), | |
| thread_id: thread.thread_id.clone(), | |
| turn_run_id: "run-finalized-existing-draft".into(), | |
| content: MessageContent::text("draft answer"), | |
| }) | |
| .await | |
| .unwrap(); | |
| let scope_for_reads = scope.clone(); | |
| let thread_id_for_reads = thread.thread_id.clone(); | |
| let finalized = service | |
| .append_finalized_assistant_message(AppendFinalizedAssistantMessageRequest { | |
| scope, | |
| thread_id: thread.thread_id, | |
| turn_run_id: "run-finalized-existing-draft".into(), | |
| content: MessageContent::text("final answer"), | |
| }) | |
| .await | |
| .unwrap(); | |
| assert_eq!(finalized.message_id, draft.message_id); | |
| assert_eq!(finalized.status, MessageStatus::Finalized); | |
| assert_eq!(finalized.content.as_deref(), Some("final answer")); | |
| let by_run = service | |
| .finalized_assistant_message_by_run(FinalizedAssistantMessageByRunRequest { | |
| scope: scope_for_reads.clone(), | |
| thread_id: thread_id_for_reads.clone(), | |
| turn_run_id: "run-finalized-existing-draft".into(), | |
| }) | |
| .await | |
| .unwrap() | |
| .expect("finalized assistant message should be indexed by run"); | |
| assert_eq!(by_run.message_id, draft.message_id); | |
| let history = service | |
| .list_thread_history(ThreadHistoryRequest { | |
| scope: scope_for_reads, | |
| thread_id: thread_id_for_reads, | |
| }) | |
| .await | |
| .unwrap(); | |
| assert_eq!(history.messages.len(), 1); | |
| assert_eq!(history.messages[0].message_id, draft.message_id); | |
| } |
🤖 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_threads/tests/filesystem_session_thread_contract.rs` around
lines 275 - 313, The test in
filesystem_append_finalized_assistant_message_finalizes_existing_draft_by_turn_run
should verify the persisted in-place finalize behavior, not only the returned
record from append_finalized_assistant_message. After finalizing, assert
finalized_assistant_message_by_run returns the same draft.message_id and that
list_thread_history shows exactly one message, so the contract rules out
creating a second finalized history row. Use the existing service methods and
turn_run_id/thread_id flow in this test to confirm the caller-visible single-row
invariant.
Source: Path instructions
|
🚅 Deployed to the ironclaw-pr-5455 environment in ironclaw-ci-preview
|
Dated boundary update capturing the before/after for the WAL change (#5451) and the row-native sequence + thread/turn append-path rework (#5455), plus the headline c100 sweep: 100 concurrent writes complete with zero failures and p95 256.8ms / p99 294.3ms on this 4-core container. Includes the raw JSONL artifacts. Clearly labeled as a different machine from the M4 usable-boundary results so the numbers are not cross-compared. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
Stress results persistedAdded a dated boundary update under Headline —
100 concurrent writes complete with zero failures, p95/p99 well inside the usability SLO. Throughput peaks at c32 and eases to 21 ops/s at c100 with no collapse (WAL-only collapsed past c8). Remaining cost splits evenly across Compatibility note (for reviewers)Verified this is not a breaking change for upgrading existing libSQL instances: the sequences table is One rollback caveat to fold into the Rollback Plan: the upgrade is forward-safe, but a downgrade after new writes is asymmetric — messages written to the append-log while running this code are invisible to pre-rework code (it doesn't read the log). Reverting restores the old write path for new messages, but append-log messages from the new-code window would need manual recovery. Generated by Claude Code |
Adds the V32__root_filesystem_sequences entry to migrations/checksums.lock so the released-migration immutability check passes. Checksum computed via refinery::Migration::unapplied(...).checksum() (verified by reproducing the existing V31 entry with the same method). Addresses the CodeRabbit review finding on #5455. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
Resolves the real findings from the Gemini and CodeRabbit reviews: - Redaction/update of append-only finalized messages now works. Such messages live only in the per-thread append log (no individual file), so apply_message_update CAS-wrote to a nonexistent path and failed. Fix: materialize the message file on first mutation (CasExpectation::Absent), and make merge_message_append_events file-authoritative (a per-message file shadows its append-log entry) so the redacted record wins on reads. This matches read_message_versioned, which was already file-first. (Gemini critical / CodeRabbit major.) - Sequence counters are swept on delete. reserve_sequence state lives in a path-scoped side table; exact/prefix delete now clears it in the libSQL, Postgres, and in-memory backends so delete/recreate restarts from 1 instead of resuming stale state. Sibling paths sharing a string prefix are not swept. (CodeRabbit major.) - runner_lease_cache is keyed by TurnRunId instead of a stringified key. (CodeRabbit.) - try_write_new_message_transactionally now retries the transaction on an optimistic-concurrency conflict at commit, as the surrounding loop and the "CAS retries exhausted" error always intended (it previously returned the conflict without retrying — a never-loops latent bug). Postgres-only; libSQL/in-memory return Unsupported before the loop body. Regression tests: redact an append-only finalized assistant message and assert reads show the redaction with a single history row; finalize-existing- draft asserts the single-row in-place invariant; delete clears reserved sequences while preserving a string-prefix sibling. Not changed (false positives, verified against the code): - StoredThreadMessageRecord round-trip: #[serde(flatten)] makes it deserialize cleanly as ThreadMessageRecord. - materialize_message_range: list_thread_messages already merges the append log, so append-only messages are not missed. - Cached-heartbeat vs recovery: lease TTL (90s) exceeds the durable refresh interval (30s) plus expiry margin (30s), so a heartbeating run's durable expiry stays ahead of recovery — no premature requeue. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
|
Worked through the Gemini and CodeRabbit findings (commits Fixed
Not changed — false positives (verified)
Validation: Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/ironclaw_threads/src/filesystem_service.rs (1)
1600-1610: 🗄️ Data Integrity & Integration | 🟠 MajorAppend-only finalized writes still bypass the sequence index.
This branch still returns after
append_message_event()withoutwrite_message_sequence_index().materialize_message_range()continues to source indexed range reads fromMessageSequenceIndexStore, so append-only finalized assistant messages are still omitted from range-based callers unless they also go through the file-backed write path. Either persist the sequence index on successful append or merge append-log-only rows into the range materialization path.🤖 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_threads/src/filesystem_service.rs` around lines 1600 - 1610, The finalized assistant-message append path in `filesystem_service.rs` still skips the sequence index, so range-based reads miss messages written only through `append_message_event()`. Update the `append_message_event` success branch in the finalized write flow to also persist via `write_message_sequence_index()`, or adjust `materialize_message_range()` to include append-log-only rows from `MessageSequenceIndexStore` so append-only finalized messages are visible to range callers.
🤖 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/in_memory.rs`:
- Around line 150-158: Delete currently clears only state.entries and
state.sequences, but it must also remove matching state.event_logs so recreated
threads do not rehydrate stale append-log history. Update the delete path in the
in_memory state cleanup logic around clear_sequences_under to sweep event_logs
with the same exact-or-subtree prefix predicate used for entries/sequences,
preserving the same thread-scoped deletion behavior across all state
collections.
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 963-971: The delete path in libsql.rs currently removes filesystem
entries and sequence state but leaves append-log data behind, so recreate can
replay stale history. Update the same delete flow in the delete handler that
uses conn.execute and child_path_like_pattern(path) to also delete matching rows
from root_filesystem_events with the same path/prefix predicate before
returning. Keep the cleanup scoped consistently with the existing
FilesystemOperation::Delete handling and error mapping.
In `@crates/ironclaw_filesystem/src/postgres.rs`:
- Around line 1418-1427: The delete path in postgres.rs only clears
root_filesystem_sequences, but root_filesystem_events still retains append-only
history for the same path subtree. Update the delete logic in the same function
that calls cached_execute so it also deletes matching rows from
root_filesystem_events using the same path/subtree predicate, ensuring
delete/recreate does not resurrect stale thread history.
---
Duplicate comments:
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1600-1610: The finalized assistant-message append path in
`filesystem_service.rs` still skips the sequence index, so range-based reads
miss messages written only through `append_message_event()`. Update the
`append_message_event` success branch in the finalized write flow to also
persist via `write_message_sequence_index()`, or adjust
`materialize_message_range()` to include append-log-only rows from
`MessageSequenceIndexStore` so append-only finalized messages are visible to
range callers.
🪄 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: 487ceef5-ed4e-4b83-9b58-9e2f6756eaf9
⛔ Files ignored due to path filters (1)
migrations/checksums.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/ironclaw_turns/src/filesystem_store.rs
…in permission matches The new `ReserveSeq` filesystem operation made two exhaustive `FilesystemOperation` permission matches non-exhaustive, breaking the full-workspace build (caught by the Railway preview deploy and the all-features clippy gate; the per-crate local builds didn't reach these crates). `reserve_sequence` mutates the sequence counter, so it maps to `permissions.write` alongside the other record/event-plane writes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
Adopt the runner-lease heartbeat-in-memory implementation from the merged #5452, dropping this branch's overlapping reimplementation (carried over from #5453): take main's `crates/ironclaw_turns/src/filesystem_store.rs`, `runner_lease.rs`, and `filesystem_turn_state_contract.rs` verbatim. This PR's unique work — the `reserve_sequence` filesystem primitive and the thread append-path storage — is independent of the runner-lease code and is preserved. Verified ironclaw_turns/threads/filesystem/host_runtime/ first_party_extensions compile against merged main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
Collapse the `let Some(events) = ... else` binding onto one line per rustfmt. The Formatting CI gate did not run on the review-fix commit (only label workflows fired there), so this slipped through until the post-merge full CI run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
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 (3)
crates/ironclaw_threads/src/filesystem_service.rs (3)
1551-1601: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake finalized appends idempotent under concurrent retries.
This is check-then-append: two callers with the same
turn_run_idcan both missfind_assistant_message_by_run(), reserve different sequences, and append duplicate finalized assistant messages. The contract/test path is idempotent byturn_run_id, so the winner needs to be chosen by an atomic per-run record/index or a transactional write that makes the run key unique before appending.🤖 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_threads/src/filesystem_service.rs` around lines 1551 - 1601, The finalized assistant append flow in `filesystem_service.rs` is vulnerable to duplicate writes because `find_assistant_message_by_run()` is checked before `reserve_sequence()` and `append_message_event()`, so concurrent callers with the same `turn_run_id` can both append. Update the `append_assistant_message` path to make the `turn_run_id` lookup/write atomic, either by enforcing a unique per-run record/index or by performing the existing `ThreadMessageRecord` creation and append inside a transactional/conditional write so only one finalized message can win.
2284-2295: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail loud instead of ranking unreadable histories as empty.
On message-read failure this logs and assigns
latest_sequence = 0, which hides backend/read corruption and changes ordering semantics. Propagate the error, skip the affected thread with an explicit// silent-ok: ...rationale, or surface a safe degraded state. As per path instructions, "Fail loud: flag silent-failure patterns ... Errors propagate with ? into thiserror types with context."🤖 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_threads/src/filesystem_service.rs` around lines 2284 - 2295, The unreadable-history fallback in the thread sorting path is swallowing a read failure by logging and assigning latest_sequence = 0, which hides corruption and changes ordering; update the latest_sequence_results handling in filesystem_service.rs to either propagate the error with proper context from the surrounding request flow, or explicitly skip that thread with a clear silent-ok rationale instead of treating it as empty. Keep the fix localized around the latest_sequence_result match in the list_threads_for_scope activity sort logic and preserve a safe degraded behavior without silently ranking failed reads as oldest.Source: Path instructions
2258-2283: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftDo not scan every transcript before pagination.
list_thread_messages()now runs for every listed thread before slicing the page, and it can query message files plus tail append logs per thread. That contradicts the nearby guarantee that heavy probes only run for the sliced page and makes sidebar cost scale with total transcript volume, not just thread count. Use a persisted activity signal/index instead of full transcript scans. As per coding guidelines, "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent".🤖 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_threads/src/filesystem_service.rs` around lines 2258 - 2283, The pagination path in filesystem_service::list_threads is still doing a full transcript probe by calling list_thread_messages for every listed thread before slicing, which defeats the intended “only scan the page” behavior. Replace this eager latest-sequence computation with a persisted activity signal/index for each thread, and have the page-building logic read that metadata instead of scanning message files or append logs; update the surrounding guarantee/comment and add or adjust tests around list_threads to enforce the bounded-cost behavior.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.
Outside diff comments:
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1551-1601: The finalized assistant append flow in
`filesystem_service.rs` is vulnerable to duplicate writes because
`find_assistant_message_by_run()` is checked before `reserve_sequence()` and
`append_message_event()`, so concurrent callers with the same `turn_run_id` can
both append. Update the `append_assistant_message` path to make the
`turn_run_id` lookup/write atomic, either by enforcing a unique per-run
record/index or by performing the existing `ThreadMessageRecord` creation and
append inside a transactional/conditional write so only one finalized message
can win.
- Around line 2284-2295: The unreadable-history fallback in the thread sorting
path is swallowing a read failure by logging and assigning latest_sequence = 0,
which hides corruption and changes ordering; update the latest_sequence_results
handling in filesystem_service.rs to either propagate the error with proper
context from the surrounding request flow, or explicitly skip that thread with a
clear silent-ok rationale instead of treating it as empty. Keep the fix
localized around the latest_sequence_result match in the list_threads_for_scope
activity sort logic and preserve a safe degraded behavior without silently
ranking failed reads as oldest.
- Around line 2258-2283: The pagination path in filesystem_service::list_threads
is still doing a full transcript probe by calling list_thread_messages for every
listed thread before slicing, which defeats the intended “only scan the page”
behavior. Replace this eager latest-sequence computation with a persisted
activity signal/index for each thread, and have the page-building logic read
that metadata instead of scanning message files or append logs; update the
surrounding guarantee/comment and add or adjust tests around list_threads to
enforce the bounded-cost behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c19bea63-86d6-41f3-8bc5-7a6e6019a5e8
📒 Files selected for processing (1)
crates/ironclaw_threads/src/filesystem_service.rs
…ries Resolves the CodeRabbit/Gemini findings on `list_threads_for_scope` (and the cross-thread mis-ranking flagged earlier), realigning the filesystem service with the in-memory reference, which already sorts by `updated_at` and stamps it on activity. The native `reserve_sequence` path stopped rewriting the thread record, so `updated_at` went stale and the list fell back to sorting by `max(message.sequence)` — per-thread sequence, i.e. transcript length, not recency. That also forced a full per-thread transcript scan (`list_thread_messages` for every thread) before pagination, an O(N*M) sidebar cost, and silently ranked unreadable histories as oldest. Changes: - Add `touch_thread_updated_at`: a bounded-CAS stamp of `thread.updated_at` called once per turn boundary (inbound accept + finalized assistant append), best-effort under contention (a lost CAS race means a concurrent writer already advanced the stamp — the safe direction). - Sort `list_threads_for_scope` purely by `updated_at`/`created_at` desc with a stable thread_id tie-break; drop the per-thread transcript scan and the `latest_sequence = 0`-on-read-error fallback entirely. - Extend the activity-ordering contract test to pin the distinguishing case: a chattier-but-staler thread must not outrank a quieter, more-recently touched one (fails under the old sequence sort, passes now). Cost is one extra thread-record write per turn (turn boundary, not per token) — negligible against the per-token writes the sequence primitive removed, and invisible to users, who instead get correct recency ordering and a sidebar whose cost scales with thread count, not transcript volume. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
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_threads/src/filesystem_service.rs (1)
1629-1636: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
turn_run_iddedupe atomic before append.Two concurrent finalized appends with the same
turn_run_idcan both miss the pre-scan, reserve different sequences, and append different message IDs.append()is durable, not a uniqueness claim. Claim a per-(thread_id, turn_run_id)key with CAS/transaction before appending, or use a deterministic message path and read the winner on conflict. Add a concurrent caller-level test. As per path instructions, “Test through the caller”.Also applies to: 1682-1684
🤖 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_threads/src/filesystem_service.rs` around lines 1629 - 1636, The current pre-scan in append logic is not atomic, so concurrent finalized appends can both pass the `find_assistant_message_by_run` check and create duplicates. Update the append flow in `filesystem_service.rs` so `append()` first claims a unique per-`(thread_id, turn_run_id)` reservation using CAS/transaction semantics (or another atomic conflict check) before writing, and make the winner deterministic if a path conflict occurs. Apply the same fix in the other append site mentioned by the review, and add a concurrency-focused test through the caller that verifies only one message is persisted for the same `turn_run_id`.Source: Path instructions
🤖 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_threads/src/filesystem_service.rs`:
- Around line 1136-1143: `finalize_assistant_message()` updates a draft into a
finalized reply but does not refresh thread recency, leaving `updated_at` stale
for the draft/update/finalize path. Update `finalize_assistant_message()` in
`filesystem_service.rs` to invoke `touch_thread_updated_at_best_effort()` after
the finalize write succeeds, using the existing `ThreadScope` and `ThreadId`
flow, so direct draft finalization is included in sidebar recency updates.
---
Outside diff comments:
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1629-1636: The current pre-scan in append logic is not atomic, so
concurrent finalized appends can both pass the `find_assistant_message_by_run`
check and create duplicates. Update the append flow in `filesystem_service.rs`
so `append()` first claims a unique per-`(thread_id, turn_run_id)` reservation
using CAS/transaction semantics (or another atomic conflict check) before
writing, and make the winner deterministic if a path conflict occurs. Apply the
same fix in the other append site mentioned by the review, and add a
concurrency-focused test through the caller that verifies only one message is
persisted for the same `turn_run_id`.
🪄 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: b4a507ec-cb3a-4682-a24f-729dbd4c809f
📒 Files selected for processing (1)
crates/ironclaw_threads/src/filesystem_service.rs
…draft finalize Two correctness fixes for the append/finalize paths: - Append-only finalized messages now write the sequence index, not just the log event. `write_new_message` indexes; `append_message_event` did not, so a finalized assistant reply stored append-only was missing from indexed range reads (`list_thread_messages_range`, summaries, compaction) on threads that also had indexed messages — even though full-history and model-context reads (which merge the append log) already saw it. The id resolves through `read_message_versioned`'s append-log fallback. New regression test: `filesystem_store_range_read_includes_append_only_finalized_message`. - `finalize_assistant_message` (the draft -> finalized path) now stamps thread recency via `touch_thread_updated_at_best_effort`, matching `accept_inbound_message` and `append_finalized_assistant_message`. Without it, the draft/update/finalize path left active threads stale in the `updated_at`-sorted sidebar — the user-visible "latest doesn't come to top" symptom for runtimes that stream a draft then finalize. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
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 (2)
crates/ironclaw_threads/src/filesystem_service.rs (2)
226-231: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate append-log records against the requested thread.
The file-backed branch rejects mismatched
thread_id, but the append-log fallback returns onmessage_idalone. Filter both fields before returning so malformed/stale append events cannot cross the thread boundary.- return Ok(events - .into_iter() - .find(|(record, _)| record.message_id == message_id)); + return Ok(events.into_iter().find(|(record, _)| { + &record.thread_id == thread_id && record.message_id == message_id + }));As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on authority, state, memory, process, network, outbound, resource, and event records.”
🤖 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_threads/src/filesystem_service.rs` around lines 226 - 231, The append-log fallback in read_message_append_events handling is only matching on message_id, so it can return records from the wrong thread. Update the fallback in filesystem_service::read_message_append_events/read_message path to verify both thread_id and message_id before returning, mirroring the file-backed branch’s scope check, so stale or malformed append events cannot cross thread boundaries.Source: Coding guidelines
1638-1640: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepair the sequence index on idempotent retry.
If
append_message_event()succeeds butwrite_message_sequence_index()fails, the message is durable and the method returnsErr. A retry then hitsexisting.status != Draftand returns the append-log message without recreating the missing index, so indexed range reads can keep omitting it. Ensure the existing-finalized fast path idempotently writes/repairs the sequence index before returning.if existing.status != MessageStatus::Draft { + self.write_message_sequence_index( + &request.scope, + &request.thread_id, + &existing, + ) + .await?; return Ok(existing); }As per path instructions, “Fail loud” requires failures not to become silent poisoned state on retry.
Also applies to: 1682-1699
🤖 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_threads/src/filesystem_service.rs` around lines 1638 - 1640, The finalized-message fast path in the existing-message check is skipping recovery of a missing sequence index on retry, leaving a durable message unindexed. Update the existing-finalized path in the message handling flow (around the existing.status != MessageStatus::Draft return and the related finalized branch) so it idempotently calls write_message_sequence_index() or an equivalent repair step before returning the existing message. Make sure the retry path in append_message_event() / write_message_sequence_index() does not silently preserve a poisoned state and instead repairs the index whenever the message is already durable but the index is absent.Source: Path instructions
🤖 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_threads/tests/filesystem_message_range_contract.rs`:
- Around line 46-88: The test currently only proves range visibility and could
still pass even if the append-only finalize path is skipped. In
filesystem_message_range_contract.rs within
filesystem_store_range_read_includes_append_only_finalized_message, add an
assertion using the existing fixture/service helpers to verify the append-only
branch actually ran by checking the per-message record was not created or that
the append log contains the finalized event, before the range assertions. Use
the same symbols already in this test, such as
append_finalized_assistant_message, index_entry_names, and range_sequences, to
keep the check anchored to the append-only path.
---
Outside diff comments:
In `@crates/ironclaw_threads/src/filesystem_service.rs`:
- Around line 226-231: The append-log fallback in read_message_append_events
handling is only matching on message_id, so it can return records from the wrong
thread. Update the fallback in
filesystem_service::read_message_append_events/read_message path to verify both
thread_id and message_id before returning, mirroring the file-backed branch’s
scope check, so stale or malformed append events cannot cross thread boundaries.
- Around line 1638-1640: The finalized-message fast path in the existing-message
check is skipping recovery of a missing sequence index on retry, leaving a
durable message unindexed. Update the existing-finalized path in the message
handling flow (around the existing.status != MessageStatus::Draft return and the
related finalized branch) so it idempotently calls
write_message_sequence_index() or an equivalent repair step before returning the
existing message. Make sure the retry path in append_message_event() /
write_message_sequence_index() does not silently preserve a poisoned state and
instead repairs the index whenever the message is already durable but the index
is absent.
🪄 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: 2f4d7c7e-726c-409b-aaa0-6128574b3413
📒 Files selected for processing (2)
crates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/tests/filesystem_message_range_contract.rs
…tion safety) Prevents the native path-local sequence counter from corrupting threads on instances that predate this change. The native `reserve_sequence` counter starts at 1 for any path with no row. An existing thread already has messages at sequences 1..N and `next_sequence = N+1` on its record, but no native counter row — so the first message after deploy would reserve sequence 1, colliding with the existing message and clobbering its sequence-index entry (orphaning it from range and summary reads). Fix: in the thread-store `reserve_sequence`, branch on the already-read `next_sequence`. Threads with `next_sequence > 1` (sequences already assigned under the legacy per-record counter) keep using that counter; only new/empty threads (`next_sequence == 1`, no messages yet) use the native fast path. Because the native path never rewrites `next_sequence`, a native thread's record stays at 1 and deterministically keeps using native, while a pre-existing thread stays on the legacy counter for its whole life — no thread ever switches counters mid-stream. No trait change, new field, or migration scan; existing-instance data is untouched. Regression test `reserve_sequence_resumes_existing_thread_counter_not_native_restart` simulates a pre-existing thread (`next_sequence = 5`, no native row) and asserts the next reservation is 5, not a native restart at 1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
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 (2)
crates/ironclaw_threads/src/filesystem_service.rs (2)
1656-1658: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRepair the sequence index on idempotent retry.
If Line 1701 appends successfully but Line 1716 fails, a retry finds the finalized append-log message and returns at Line 1657 without recreating the sequence index. That leaves a durable LLM message invisible to indexed range/context reads. Before returning an existing non-draft, idempotently ensure the sequence index exists or validate/repair it. This is the repo’s fail-loud invariant: don’t turn a partial persistence failure into silent missing data. As per path instructions, “Fail loud: flag silent-failure patterns … Errors propagate with ? into thiserror types with context.”
Also applies to: 1700-1717
🤖 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_threads/src/filesystem_service.rs` around lines 1656 - 1658, The early return in the retry path of the message append flow returns an existing non-draft message without restoring its sequence index, which can leave persisted LLM data undiscoverable. Update the idempotent retry logic around the existing-status check so that it verifies and, if needed, recreates/repairs the sequence index before returning the message. Use the append/retry path in FilesystemService and the sequence-index creation/validation logic to ensure a finalized append-log message is never silently left out of indexed reads.Source: Path instructions
226-231: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftBatch append-log fallback for indexed range reads.
Line 226 tails/deserializes the full append log for every missing per-message file.
list_thread_messages_range_indexedcalls this per sequence index, so ranges with append-only assistant messages become O(K×M) filesystem work. Read the append log once per range and fill missing message IDs from a map, or materialize per-message files on append.🤖 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_threads/src/filesystem_service.rs` around lines 226 - 231, The fallback in read_message_append_events currently scans and deserializes the full append log for each missing message, which makes list_thread_messages_range_indexed do repeated work across a range. Change the range-read path so the append log is read once per thread/range and its events are cached in a map keyed by message_id, then reuse that lookup for each missing file; alternatively, materialize per-message files when appending so the fallback is no longer needed. Keep the fix centered around read_message_append_events and list_thread_messages_range_indexed.
🤖 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_threads/src/filesystem_service.rs`:
- Around line 1656-1658: The early return in the retry path of the message
append flow returns an existing non-draft message without restoring its sequence
index, which can leave persisted LLM data undiscoverable. Update the idempotent
retry logic around the existing-status check so that it verifies and, if needed,
recreates/repairs the sequence index before returning the message. Use the
append/retry path in FilesystemService and the sequence-index
creation/validation logic to ensure a finalized append-log message is never
silently left out of indexed reads.
- Around line 226-231: The fallback in read_message_append_events currently
scans and deserializes the full append log for each missing message, which makes
list_thread_messages_range_indexed do repeated work across a range. Change the
range-read path so the append log is read once per thread/range and its events
are cached in a map keyed by message_id, then reuse that lookup for each missing
file; alternatively, materialize per-message files when appending so the
fallback is no longer needed. Keep the fix centered around
read_message_append_events and list_thread_messages_range_indexed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a90df2c8-f71b-40bc-9a89-b89bdcef3235
📒 Files selected for processing (1)
crates/ironclaw_threads/src/filesystem_service.rs
… retry Address CodeRabbit data-integrity findings on the finalized-assistant append path: - Backend delete now sweeps `root_filesystem_events` / `event_logs` alongside entries and sequences (libSQL, Postgres, in-memory). Without this, deleting and recreating the same thread path would rehydrate stale append-log history (append-only finalized assistant messages live in the event log). - `append_finalized_assistant_message` re-asserts the sequence index before returning an already-finalized message on idempotent retry. If a prior call appended the log event but died before writing the index, the durable message would otherwise stay invisible to indexed range/context reads. `write_new` is idempotent, so this is a no-op on the fully-persisted path and a repair on the partial-failure path. Regression tests: - in-memory delete sweeps co-located and subtree event logs - idempotent retry repairs a missing sequence index (range read sees it) - append-only finalize test now asserts no per-message file was written - finalize-existing-draft test asserts the run index resolves to the single in-place record Skipped CodeRabbit's batch-append-log-fallback suggestion: it is a perf optimization (O(K*M) bounded by append-only messages in a range), not a correctness issue, and a larger change better tracked separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb
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 (4)
crates/ironclaw_filesystem/src/in_memory.rs (2)
53-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
VirtualPathfor sequence-state keys.This is new internal path-scoped state, but it stores path identity as raw
String. Keep it typed likeentriesto avoid stringly path state.Targeted shape
- sequences: HashMap<String, SeqNo>, + sequences: HashMap<VirtualPath, SeqNo>,- .entry(path.as_str().to_string()) + .entry(path.clone())As per coding guidelines, “Inside the workspace, do not use raw
Stringas an internal domain value.”Also applies to: 441-446
🤖 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/in_memory.rs` at line 53, The new sequence-state map in InMemory is storing path identity as raw String, but it should be typed like entries and use VirtualPath for internal path-scoped state. Update the sequences field and the related code in InMemory to key sequence state by VirtualPath instead of String, and adjust any lookups/insertions in the affected methods to work with VirtualPath consistently.Source: Coding guidelines
154-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t mutate side tables on a NotFound delete.
In the subtree branch,
event_logsandsequencesare cleared before confirming any entry was deleted. An append-only/sequence-only path can be mutated whiledelete()returnsNotFound, diverging from SQL backends and poisoning retries.Minimal fix
state .entries .retain(|key, _| !key.as_str().starts_with(&prefix)); - clear_event_logs_under(&mut state.event_logs, path.as_str(), &prefix); - clear_sequences_under(&mut state.sequences, path.as_str(), &prefix); if state.entries.len() == before { return Err(FilesystemError::NotFound { path: path.clone(), operation: FilesystemOperation::Delete, }); } + clear_event_logs_under(&mut state.event_logs, path.as_str(), &prefix); + clear_sequences_under(&mut state.sequences, path.as_str(), &prefix);As per path instructions, “Fail loud: flag silent-failure patterns.”
🤖 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/in_memory.rs` around lines 154 - 160, The subtree delete logic in `delete()` mutates `event_logs` and `sequences` before confirming an entry was actually removed, so a `NotFound` delete can still change state. In the `in_memory` implementation, move the side-table cleanup in `delete` (including `clear_event_logs_under` and `clear_sequences_under`) so it only runs after the `retain` operation actually removed something, using the existing `before`/after entry count check to gate the cleanup. This keeps the behavior aligned with the `delete()` path and prevents silent mutation on `NotFound`.Source: Path instructions
crates/ironclaw_filesystem/src/libsql.rs (1)
951-981: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the multi-table delete atomic.
root_filesystem_entriesis deleted before the new event/sequence sweeps. If either later DELETE fails, the caller gets an error after the thread record is gone, leaving stale append-log/counter rows for recreate.Targeted shape
let conn = self.connect().await?; - let deleted = conn + let tx = conn.transaction().await + .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::Delete, error))?; + let deleted = tx .execute( "DELETE FROM root_filesystem_entries WHERE path = ?1 OR path LIKE ?2 ESCAPE '!'", libsql::params![path.as_str(), child_path_like_pattern(path)], ) @@ - conn.execute( + tx.execute( "DELETE FROM root_filesystem_events WHERE path = ?1 OR path LIKE ?2 ESCAPE '!'", libsql::params![path.as_str(), child_path_like_pattern(path)], ) @@ - conn.execute( + tx.execute( "DELETE FROM root_filesystem_sequences WHERE path = ?1 OR path LIKE ?2 ESCAPE '!'", libsql::params![path.as_str(), child_path_like_pattern(path)], ) .await .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::Delete, error))?; + tx.commit() + .await + .map_err(|error| libsql_db_error(path.clone(), FilesystemOperation::Delete, error))?; Ok(())As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on ... state ... and event records.”
🤖 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 951 - 981, The delete flow in libsql::delete is not atomic because root_filesystem_entries is removed before root_filesystem_events and root_filesystem_sequences, so a later failure can leave partial state behind. Update this method to run all three DELETE statements inside a single transaction on the connection, and commit only after every delete succeeds; if any step fails, roll back so the filesystem entries, append-log rows, and sequence counters stay in sync. Use the existing delete logic in delete and the shared child_path_like_pattern predicate to keep the subtree scope identical across all tables.Source: Coding guidelines
crates/ironclaw_filesystem/src/postgres.rs (1)
1408-1438: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWrap the delete sweeps in one transaction.
The entry DELETE commits before the new event/sequence cleanup. A failure on Line 1422 or Line 1432 leaves the thread deleted but stale append-log/counter rows durable, which is exactly the resurrected-history state this cleanup is meant to prevent.
As per coding guidelines, “Preserve tenant/user/agent/project/mission/thread scope on ... state ... and event records.”
🤖 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 1408 - 1438, Wrap the delete sweep logic in a single transaction so the entry, event, and sequence deletions succeed or fail together. Update the delete flow in the postgres delete path that uses cached_execute for root_filesystem_entries, root_filesystem_events, and root_filesystem_sequences so the first DELETE does not commit before the later cleanup steps. Use the existing delete function and its cached_execute calls to locate the change, and ensure any error rolls back the whole operation.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.
Outside diff comments:
In `@crates/ironclaw_filesystem/src/in_memory.rs`:
- Line 53: The new sequence-state map in InMemory is storing path identity as
raw String, but it should be typed like entries and use VirtualPath for internal
path-scoped state. Update the sequences field and the related code in InMemory
to key sequence state by VirtualPath instead of String, and adjust any
lookups/insertions in the affected methods to work with VirtualPath
consistently.
- Around line 154-160: The subtree delete logic in `delete()` mutates
`event_logs` and `sequences` before confirming an entry was actually removed, so
a `NotFound` delete can still change state. In the `in_memory` implementation,
move the side-table cleanup in `delete` (including `clear_event_logs_under` and
`clear_sequences_under`) so it only runs after the `retain` operation actually
removed something, using the existing `before`/after entry count check to gate
the cleanup. This keeps the behavior aligned with the `delete()` path and
prevents silent mutation on `NotFound`.
In `@crates/ironclaw_filesystem/src/libsql.rs`:
- Around line 951-981: The delete flow in libsql::delete is not atomic because
root_filesystem_entries is removed before root_filesystem_events and
root_filesystem_sequences, so a later failure can leave partial state behind.
Update this method to run all three DELETE statements inside a single
transaction on the connection, and commit only after every delete succeeds; if
any step fails, roll back so the filesystem entries, append-log rows, and
sequence counters stay in sync. Use the existing delete logic in delete and the
shared child_path_like_pattern predicate to keep the subtree scope identical
across all tables.
In `@crates/ironclaw_filesystem/src/postgres.rs`:
- Around line 1408-1438: Wrap the delete sweep logic in a single transaction so
the entry, event, and sequence deletions succeed or fail together. Update the
delete flow in the postgres delete path that uses cached_execute for
root_filesystem_entries, root_filesystem_events, and root_filesystem_sequences
so the first DELETE does not commit before the later cleanup steps. Use the
existing delete function and its cached_execute calls to locate the change, and
ensure any error rolls back the whole operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e235bad-0e67-49f9-8638-0e6a2cdce962
📒 Files selected for processing (6)
crates/ironclaw_filesystem/src/in_memory.rscrates/ironclaw_filesystem/src/libsql.rscrates/ironclaw_filesystem/src/postgres.rscrates/ironclaw_threads/src/filesystem_service.rscrates/ironclaw_threads/tests/filesystem_message_range_contract.rscrates/ironclaw_threads/tests/filesystem_session_thread_contract.rs
Resolve import-union conflict in ironclaw_threads/filesystem_service.rs vs #5455 (row-native sequence primitive): keep this branch's cas_update imports (CasApply/CasUpdateError/RecordKind/cas_update for the ensure_thread migration) plus main's SeqNo (row-native sequence). Rest auto-merged: main's row-native reserve_sequence coexists with this branch's cas_update ensure_thread. Refresh the put_with_cas exception note (comment + filesystem CLAUDE.md): #5455 made reserve_sequence row-native, so the local CAS loop's callers are now write_new_message / reserve_sequence_via_thread_record (legacy fallback) / apply_message_update / append_capability_display_preview / create_summary_artifact + message indices. Tracked by #5469. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
reserve_sequenceRootFilesystemprimitive for path-local monotonic sequence allocation (libSQL + Postgres + in-memory; migration V32 adds the sequence table), surfaced through the scoped dispatch fabric and capability set.accept_inbound/append_assistant/finalize) into smaller appends, and carries the same append-native shape through the turn-state store and runner-lease records.Why
After #5451 (WAL) and #5447 (governor fast-path), the remaining per-turn p95 is dominated by thread/turn storage shape: both rewrite a whole JSON document on every write. #5453's own c32 measurement reached the same conclusion (
thread_store_writesandturn_storeare the entire remaining p95) and named row/event-native storage as "the next root fix." This is that fix, scoped to just the storage work.Change Type
(Performance.)
Linked Issue
Supersedes the storage portion of #5453. Builds on #5451. Complements merged #5447.
Validation
Built and tested against the affected crates (rustc 1.96):
cargo test -p ironclaw_threads -p ironclaw_turns— all pass (incl. the newfilesystem_session_thread_contract,session_thread_contract,filesystem_turn_state_contractcases).cargo test -p ironclaw_filesystem --no-default-features --features libsql— pass, except one pre-existing failure (libsql_root_filesystem_migration_failure_surfaces_infrastructure_variant) that fails identically onmainbecause it injects failure viachmod 0o444, which the root build user ignores — environmental, not a regression.cargo build -p ironclaw_stress— clean.cargo fmt/clippy/cargo test --features integrationworkspace-wide — not run (see note).chat-turn, 30 ops/task, 200 users), WAL-only (currentmain) vs WAL + this change:thread_store_writesp95WAL-only throughput collapsed past c8 (19.5 ops/s at c32); with the append-native paths it holds flat from c8 to c32.
Security Impact
None. No change to permissions, network, secrets, file access, or sandbox policy.
reserve_sequenceis a storage primitive scoped through the existingScopedFilesystempermission check.Reborn Trust-Boundary Checklist
reserve_sequencereturns a typedSeqNo.serde(default): none added.i64-bounded conversion as the existingappend/SeqNopath.FilesystemErrorclasses.Database Impact
Adds migration
V32__root_filesystem_sequences.sql(libSQL + Postgres) — a sequence-allocation table backingreserve_sequence. In-memory backend implements the same primitive. No change to existing tables. Both backends supported per the dual-backend rule.Blast Radius
ironclaw_filesystem(new trait method + backend impls),ironclaw_threads(storage shape),ironclaw_turns(turn-state + runner-lease records), and the stress harness. EveryRootFilesystembackend implements the new method, so the trait stays total. Resource-governor and composition layers are untouched (governor work deliberately excluded — already onmainvia #5447).Rollback Plan
Revert this commit. The new
reserve_sequencepaths are additive; reverting restores the prior full-document write paths. Migration V32 only adds a table (leaving it in place after a code revert is harmless — nothing else references it).Review Follow-Through
The runner-lease record changes here overlap conceptually with #5452 (heartbeats-in-memory); if #5452 lands first this will need a small rebase on
runner_lease.rs. Flagging for whoever sequences the two.Review track: C (runtime/DB)
🤖 Generated with Claude Code
Generated by Claude Code