[codex] Skip durable resource writes when limits are unlimited - #5447
Merged
Merged
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an ChangesUnlimited Fast Path for PersistentResourceGovernor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 |
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5447
June 30, 2026 12:06
Destroyed
github-actions
Bot
added
size: L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an 'unlimited fast path' optimization to the PersistentResourceGovernor, allowing it to bypass durable snapshot writes and perform process-local bookkeeping when no finite resource limits are configured. To support this, read-only inspect methods were added to CasSnapshot and ResourceGovernorStore. Feedback on these changes highlights a design issue where the default implementation of inspect on ResourceGovernorStore delegates to update, causing a silent write-back that violates the read-only contract of the method. It is recommended to remove this default implementation to force implementors to provide a proper read-only version.
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.
Comment on lines
+808
to
+814
| fn inspect<T, F>(&self, inspect: F) -> Result<T, ResourceError> | ||
| where | ||
| T: Send + 'static, | ||
| F: FnOnce(&ResourceGovernorSnapshot) -> Result<T, ResourceError> + Send + 'static, | ||
| { | ||
| self.update(move |snapshot| inspect(snapshot)) | ||
| } |
Contributor
There was a problem hiding this comment.
The default implementation of inspect on ResourceGovernorStore delegates to self.update(...), which performs a write back to the store. This violates the semantic contract of inspect (which is documented as a read-only inspection path). If a developer implements ResourceGovernorStore for a new backend and doesn't override inspect, they will get a silent write-back behavior, which could cause unexpected performance issues or bugs. It is highly recommended to remove the default implementation to force implementors to provide a proper read-only inspect method.
fn inspect<T, F>(&self, inspect: F) -> Result<T, ResourceError>
where
T: Send + 'static,
F: FnOnce(&ResourceGovernorSnapshot) -> Result<T, ResourceError> + Send + 'static;
|
🚅 Deployed to the ironclaw-pr-5447 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5447
June 30, 2026 12:35
Destroyed
serrrfirat
marked this pull request as ready for review
June 30, 2026 12:37
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5447
June 30, 2026 12:48
Destroyed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_resources/src/lib.rs (1)
1164-1173: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe fast-path cutover drops the durable source of truth.
try_set_limit()only mutatessnapshot.state, butcan_use_unlimited_fast_path()flips reads and new reservations tounlimited_statewhenever every limit is unlimited. Becauseunlimited_stateis never seeded from durable state, an unlimited limit can disappear fromaccount_snapshot()immediately, and any finite→unlimited transition will stop enforcing durable reservations/usage that already exist. This needs a real state transfer/merge or a single authoritative store across cutovers. As per coding guidelines, "Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent."Also applies to: 1212-1248
🤖 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_resources/src/lib.rs` around lines 1164 - 1173, The fast-path cutover in try_set_limit()/can_use_unlimited_fast_path() is losing the durable source of truth because reads and new reservations switch to unlimited_state without transferring existing snapshot.state data. Fix this by making the cutover perform a real state transfer/merge from the durable snapshot into unlimited_state, or by consolidating account_snapshot(), set_limit_in_state(), and the fast-path reservation logic onto a single authoritative store so finite→unlimited transitions preserve existing usage/reservations. Update the relevant ResourceStore/ResourceAccount state-handling paths and add tests covering unlimited limit visibility and preservation of prior durable reservations across the cutover.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 4179-4195: The current tests only validate parse_bool_env_value
directly, but the env toggle should be covered at the real composition boundary.
Add a caller-level test around build_reborn_services (or the production factory
path it uses) that exercises an invalid value and an enabled value so the
resource-governor side-effect gate is verified through the actual wiring. Use
the existing parse_bool_env_value and build_reborn_services symbols to locate
the code, and keep the test focused on the real call site rather than the helper
parser.
- Around line 1707-1721: The wrapper function
apply_resource_governor_unlimited_fast_path is hidden behind the libsql/postgres
cfg, but it is called from unconditional code paths, so no-durable builds lose
the symbol and fail to compile. Make this helper available regardless of storage
backend by removing or relaxing the cfg gate on
apply_resource_governor_unlimited_fast_path in factory.rs, while preserving the
existing resource_governor_unlimited_fast_path_enabled_from_env and
with_unlimited_fast_path behavior.
In `@crates/ironclaw_resources/src/lib.rs`:
- Around line 808-814: The `ResourceGovernorStore::inspect` default
implementation still routes through `update`, which violates the intended
read-only contract for fast-path checks. Make `inspect()` mandatory by removing
the default body or otherwise forcing each implementation to provide a true read
path, and update the `ResourceGovernorStore`/`inspect` contract accordingly so
it no longer silently falls back to write/CAS behavior.
- Around line 1250-1254: The lock_unlimited_state helper currently clears poison
with into_inner(), which can resume from half-mutated unlimited_state and
silently fail open. Change this path to return a ResourceError instead of a
MutexGuard, and update the callers in ResourceState/governor code to propagate
the failure with ? so quota/accounting decisions fail closed. Keep the fix
localized around lock_unlimited_state and the surrounding unlimited_state access
logic.
In `@crates/ironclaw_resources/tests/resource_governor_contract.rs`:
- Around line 773-793: The post-limit check in resource_governor_contract tests
is not verifying the behavior of reserve(), since set_limit() can already make
path.exists() true before the reservation runs. Update the assertion around
governor.reserve() to exercise the caller-level effect directly, using the
resource_governor_contract test flow and symbols like set_limit, reserve, and
usage_for(&account): either confirm the prior usage remains reflected after
applying the finite limit, or add a second reserve that is rejected under the
new cap. Ensure the test fails if reserve stays on the process-local fast path
or does not trigger the durable governor transition.
---
Outside diff comments:
In `@crates/ironclaw_resources/src/lib.rs`:
- Around line 1164-1173: The fast-path cutover in
try_set_limit()/can_use_unlimited_fast_path() is losing the durable source of
truth because reads and new reservations switch to unlimited_state without
transferring existing snapshot.state data. Fix this by making the cutover
perform a real state transfer/merge from the durable snapshot into
unlimited_state, or by consolidating account_snapshot(), set_limit_in_state(),
and the fast-path reservation logic onto a single authoritative store so
finite→unlimited transitions preserve existing usage/reservations. Update the
relevant ResourceStore/ResourceAccount state-handling paths and add tests
covering unlimited limit visibility and preservation of prior durable
reservations across the cutover.
🪄 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: d9c05e1f-4345-4515-b24f-4b10b641951e
📒 Files selected for processing (9)
README.mdcrates/ironclaw_reborn_composition/src/error.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_resources/src/cas_snapshot.rscrates/ironclaw_resources/src/filesystem_store.rscrates/ironclaw_resources/src/lib.rscrates/ironclaw_resources/tests/resource_governor_contract.rstools/ironclaw_stress/src/main.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/ironclaw_stress/src/main.rs`:
- Around line 906-912: The sweep validation in the `max_concurrency` calculation
is incorrectly folding `args.concurrency` back in, which makes sweep-only checks
too strict when `--ramp-concurrency` is not set. Update the logic around the
`max_concurrency` computation in `main` so it only considers `sweep_concurrency`
and the optional `ramp_concurrency` (falling back to the ramp value or the sweep
max as appropriate), and does not automatically raise the threshold to
`args.concurrency` for validating sweep cases.
🪄 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: 903be89a-0953-42c4-a560-22e6fd3fb61f
📒 Files selected for processing (5)
tools/ironclaw_stress/README.mdtools/ironclaw_stress/src/main.rstools/ironclaw_stress/src/suite.rstools/ironclaw_stress/src/synthetic.rstools/ironclaw_stress/src/tests.rs
💤 Files with no reviewable changes (1)
- tools/ironclaw_stress/src/suite.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5447
June 30, 2026 13:36
Destroyed
github-actions
Bot
added
size: XL
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5447
June 30, 2026 14:36
Destroyed
This was referenced Jun 30, 2026
henrypark133
added a commit
that referenced
this pull request
Jun 30, 2026
Reconcile lock-convoy removal (PR #5234) with main: - resources (#5447): keep opt-in unlimited fast-path skip + lock-free inspect() reads; route durable writes through cas_update. update bound FnOnce->FnMut so cas_update can retry; fast-path closures clone instead of move. read_snapshot/decode_snapshot kept; dead mutex update_snapshot/ put_with_cas/PutError removed from cas_snapshot.rs. - turns (#5452): lease heartbeats stay memory-backed; durable state.json RMW goes through cas_update. Fixed latent break where apply() closure still built deleted RunnerLeaseSidecar (now RunnerLeaseStore over the in-memory map). Removed now-dead CAS-retry island from filesystem_store/ io.rs (put_with_cas/cas_retry_backoff/PutError + consts) — both callers gone (durable path -> cas_update, fs lease sidecar deleted by #5452). - test support (session_thread.rs): generify RebornThreadHarness for both InMemoryBackend (default tier) and CompositeRootFilesystem tiers. Invariants preserved: zero lock().await-across-await in durable RMW; secrets Arc-keyed lock-map stays deleted; all durable RMW via cas_update. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
serrrfirat
added a commit
that referenced
this pull request
Jun 30, 2026
…hs (#5455) * perf(storage): row-native sequence primitive + thread/turn append paths 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 * docs(stress): WAL + storage-rework libSQL write-concurrency results 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 * fix(migrations): pin V32 checksum in checksums.lock 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 * fix(threads,filesystem,turns): address PR #5455 review findings 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 * fix(extensions,host_runtime): handle FilesystemOperation::ReserveSeq 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 * style(threads): rustfmt filesystem_service append-event read 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 * fix(threads): order thread list by updated_at, stamped at turn boundaries 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 * fix(threads): make post-commit recency touch best-effort; narrow list comment Addresses two CodeRabbit findings on the Option A recency work: - `touch_thread_updated_at` ran with `?` at three after-commit call sites (inbound accept, draft finalize, finalized append). A non-CAS backend error there failed the call *after* the message was already durable, and since `accept_inbound_message` permits requests without an idempotency key, the caller's retry could duplicate the message. Route all three through a new `touch_thread_updated_at_best_effort` that logs and continues — the stamp is advisory once the write has committed. - Narrow the `list_threads_for_scope` comment: activity ordering never scans transcripts, but title derivation still reads the sliced page, so the prior "transcripts are never scanned here" overpromised. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQzj2447pq2BUfgY3BUSJb * fix(threads): index append-only finalized messages; stamp recency on 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 * fix(threads): keep existing threads on legacy sequence counter (migration 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 * fix(threads): sweep append-log on delete and repair sequence index on 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 --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: firat.sertgoz <firatsertgoz@alumni.sabanciuniv.edu>
This was referenced Jul 1, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PersistentResourceGovernorIRONCLAW_RESOURCE_GOVERNOR_UNLIMITED_FAST_PATHironclaw_stresson the fast path so stress runs can isolate unlimited-governor overheadWhy
Unlimited resource-governor reserve/reconcile/release does not need to persist a snapshot on every operation when there are no finite limits to enforce. The new fast path keeps same-process reservation lifecycle checks, but avoids durable mutation for unlimited-limit reserve/reconcile/release. If any finite limit exists, the governor falls back to the durable CAS path.
Production defaults remain conservative: the Reborn composition only enables this path when
IRONCLAW_RESOURCE_GOVERNOR_UNLIMITED_FAST_PATH=true(also accepts1,yes,on). Invalid values fail startup.Benchmarks
Rerun on 2026-06-30 against current
origin/main(b597ecf33) and this PR (2b677ddab). Both use local libSQL, release builds, one process,--concurrency 8,--operations 200,--users 200,--tenants 1,--progress-interval-seconds 0,--human-read, and--bottleneck-report.Resource Governor Only
Command shape:
origin/mainRealistic Distinct-Thread User Session
Command shape:
--active-thread-count 0means each synthetic user has their own active thread pool. This avoids the previous hot-thread benchmark where all workers intentionally raced one thread and produced expectedturn_thread_busyfailures.origin/mainThe old PR body included a
--active-thread-count 1table. That measured deliberate same-thread contention, not distinct-user capacity, so it has been removed from the capacity comparison.Interpretation
This PR improves the unlimited-governor path, but it does not remove the main libSQL write bottlenecks from realistic chat-like workloads. In
mixed-user-session, the remaining high-latency groups are still thread-store writes and turn-store writes. DB file growth is therefore expected and should not be read as proof that the governor is still doing durable reserve/reconcile writes.The practical result is a clean no-failure workload with roughly:
Validation
Functional checks run on this branch:
cargo check -p ironclaw_reborn_composition --features libsql --libcargo test -p ironclaw_resources --test resource_governor_contractcargo test -p ironclaw_stresscargo clippy -p ironclaw_resources -p ironclaw_stress --tests -- -D warningsFresh benchmark result files used for this body:
/tmp/ironclaw-bench-results/main-reserve.json/tmp/ironclaw-bench-results/pr-reserve.json/tmp/ironclaw-bench-results/main-mixed.json/tmp/ironclaw-bench-results/pr-mixed.json