feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1294) - #1303
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe exploration cadence now uses a persistent, store-level monotonic counter. The default cadence returns to 20 turns. Atomic allocation coordinates sessions and processes, and tests cover cadence, persistence, uniqueness, determinism, and locking. ChangesGlobal exploration cadence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant ExplorationHook
participant MemoryStore
participant SchemaMeta
Session->>ExplorationHook: consult exploration
ExplorationHook->>MemoryStore: next_exploration_fire_idx()
MemoryStore->>SchemaMeta: BEGIN IMMEDIATE
MemoryStore->>SchemaMeta: read and persist next index
SchemaMeta-->>MemoryStore: committed index
MemoryStore-->>ExplorationHook: global fire index
ExplorationHook-->>Session: apply cadence decision
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
Reviewer's GuideMakes the exploration fire index a global, monotonic store-level counter so Sequence diagram for global exploration fire index claim and slot firingsequenceDiagram
participant Hook as _substitute_exploration_slots
participant MemoryStore
participant should_explore
Hook->>MemoryStore: next_exploration_fire_idx()
MemoryStore->>MemoryStore: transaction(immediate=True)
MemoryStore->>MemoryStore: SELECT value FROM schema_meta
MemoryStore->>MemoryStore: INSERT OR REPLACE INTO schema_meta
MemoryStore-->>Hook: fire_idx
Hook->>should_explore: should_explore(fire_idx, cadence)
alt fire_idx % cadence == 0
Hook->>MemoryStore: record_exploration(...)
Hook-->>Hook: [substitute exploration slots]
else not a firing turn
Hook-->>Hook: [leave hits unchanged]
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
attn:review
|
[claim:review:Gylf:2026-08-01T05:39:22Z] |
Review — approving. All three decisions #1294 flagged are answered, and I verified the concurrency property under real contention rather than by inspection.I filed #1294 off the review of #1285, so I had a specific list to check this against. It answers all of it, and the Verified by running, not by readingThe counter is genuinely global. Two separate The knob now means what it says. 400 further turns at the restored Concurrency holds under actual contention. The PR pins the lock discipline and is candid that a sequential test cannot observe the interleaving — so I ran the interleaving. Four threads, four connections, 25 claims each: No duplicates and no The lock-discipline guard bites. Downgrading Asserting the transaction mode directly is the right call here, and the docstring says why: a behavioural test at this level stays green with the transaction downgraded, which is precisely the guard-that-passes-against-its-own-bug shape. The three decisions #1294 named
Also correct: the counter is claimed after the enabled check ( One observation, not a blockerWhen the lane is enabled, every UPS turn now takes a Verification I ran
Approving. Not labelling |
|
[release:review:Gylf:2026-08-01T05:42:04Z] |
|
[claim:review:Setr:2026-08-01T05:48:52Z] |
35e7e67 to
7377bdd
Compare
Second review — driving this to merge. One non-blocking finding on the "never decrease" invariant.The prior review approved and explicitly declined to label, having filed #1294 Checked
Finding — the corrupt-value fallback can re-issue an index
except (TypeError, ValueError):
# A hand-edited or corrupt value must not wedge the lane
# forever; restart the sequence rather than raise on a
# hot path whose caller is fail-soft anyway.
current = 0Not wedging is the right instinct. But the docstring twelve lines above offers
The fallback is a post-change path on which they do decrease — and worse, a Impact is small and I am not blocking on it. Replay reads the stored seed Suggested fix, for a follow-up rather than this PR: seed the restart from row = self._conn.execute(
"SELECT MAX(fire_idx) AS m FROM exploration_events"
).fetchone()
current = int(row["m"]) if row and row["m"] is not None else 0Still never wedges (an empty ledger falls back to 0), and never re-issues an One thing I did change: rebased itThe branch was 1 commit behind Verified the rebase is content-neutral: the only difference against the Heads-up — this branch is also checked out in another worktree, so whoever is VerdictApproving. Labelling |
|
[claim:review:Toug:2026-08-01T05:52:27Z] |
|
[release:review:Toug:2026-08-01T05:52:32Z] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aelfrice/store.py (1)
3774-3798: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument or guard the nested-transaction case for the atomicity guarantee.
next_exploration_fire_idx()states unconditionally that "the read-then-write runs underBEGIN IMMEDIATE." That is true only whenself._txn_depth == 0. Pertransaction()'s own docstring,immediate=Trueis Ignored on a nested block (the outermost transaction already holds the lock) and when a transaction is somehow already open, so it is safe to pass unconditionally. If this method is ever called from inside an outerstore.transaction()block on the same connection, the read-then-write silently degrades to a non-atomic, deferred operation — exactly the duplicate-index race the#1294fix andtest_the_claim_takes_the_write_lock_before_readingexist to prevent, and nothing in this method or its test would catch that regression.No current caller in the provided context nests this call, so this is a latent risk. Consider adding an explicit note in the docstring calling out the nested-transaction caveat, or asserting
self._txn_depth == 0at entry so a future caller that nests the call fails loudly instead of silently losing the atomicity guarantee.🔒️ Proposed guard
def next_exploration_fire_idx(self) -> int: """Claim the next global exploration fire index (`#1294`). ... + + Must not be called from inside an outer `transaction()` block on + this connection: `immediate=True` is a no-op on a nested + transaction, which silently drops the atomicity guarantee below. """ + if self._txn_depth != 0: + raise RuntimeError( + "next_exploration_fire_idx() must not run inside an " + "outer transaction() block; immediate=True is ignored " + "when nested, which would silently break atomicity" + ) with self.transaction(immediate=True):🤖 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 `@src/aelfrice/store.py` around lines 3774 - 3798, Update the docstring for next_exploration_fire_idx to explicitly state that its BEGIN IMMEDIATE atomicity guarantee applies only when no transaction is already active, and document that nested calls cause transaction(immediate=True) to reuse the outer transaction. Alternatively, add an entry guard that rejects nonzero self._txn_depth so nested callers fail loudly before performing the read-then-write.
🤖 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.
Nitpick comments:
In `@src/aelfrice/store.py`:
- Around line 3774-3798: Update the docstring for next_exploration_fire_idx to
explicitly state that its BEGIN IMMEDIATE atomicity guarantee applies only when
no transaction is already active, and document that nested calls cause
transaction(immediate=True) to reuse the outer transaction. Alternatively, add
an entry guard that rejects nonzero self._txn_depth so nested callers fail
loudly before performing the read-then-write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 28332043-84c2-4d19-bd1d-08b89b555177
📒 Files selected for processing (6)
CHANGELOG/v4.mddocs/user/CONFIG.mdsrc/aelfrice/exploration.pysrc/aelfrice/hook.pysrc/aelfrice/store.pytests/test_exploration_slot_1279.py
robotrocketscience
added
the
ready-to-merge
|
[claim:review:Toug:2026-08-01T06:05:09Z] |
|
[release:review:Toug:2026-08-01T06:05:14Z] |
…one turn in n (#1294) fire_idx came from session_ring.read_ring_state, which holds exactly one session and returns {} on a session-id mismatch, so the counter restarted constantly and the knob meant 'one turn in n of a session'. At the specified cadence of 20 the slot reached a firing turn on 8 of 956 turns all-time and 0 of 259 in the current regime; #1279 lowered the default to 3 as an interim, which made the lane reachable but left the units per-session, so the realised rate depended on how the operator segmented their work. MemoryStore.next_exploration_fire_idx() holds the counter in schema_meta -- a key-value row, not a new table, so there is no migration; an edges-adjacent migration is what left stores unopenable in #1161, and an absent key reads as 0. The read-modify-write takes BEGIN IMMEDIATE (the #1135 treatment) because sister sessions share one store: deferred, two writers both pass the SELECT before either UPDATEs and the ledger gains two rows claiming the same draw with the same seed. Default cadence returns to 20. The #1279 per-session reachability guard is replaced rather than deleted -- it asserted a typical session reaches a firing turn, which is the wrong property once the counter is global, and it turned red on the raise, which is what made this deliberate. Regime break for exploration_events: derive_seed is unchanged in form but fire_idx comes from a different sequence, so rows either side must not be pooled. Atomicity is pinned by asserting the lock discipline directly. A sequential uniqueness test stays green with the transaction downgraded to deferred -- verified -- so it is kept for the arithmetic and labelled as not covering atomicity.
…has a regime break (#1294) CONFIG.md said the cadence counts turns within a session and that a global 'one turn in twenty' was filed separately; it has landed. Records the schema_meta counter, the immediate-transaction claim, and the regime break that stops pre- and post-#1294 ledger rows being pooled.
…#1294) crate-ci/typos reads "UPDATEs" as "UPDAT" and failed the required check on two of my own lines. Rephrased in the CHANGELOG entry and the test docstring; no assertion or behaviour change.
7377bdd to
326ea5e
Compare
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 326ea5e → |
|
[release:review:Setr:2026-08-01T06:11:25Z] |
|
Merged. Note for whoever hits this next: the first The recovery is to push, not to cycle the label — the workflow also triggers on A The corrupt-value / "never decrease" finding from my review is unaddressed by design — filing it as a follow-up rather than landing store logic neither you nor the first reviewer saw. |
… mark `next_exploration_fire_idx` fell back to `current = 0` when the `schema_meta` value would not parse, so the next claim was 1 regardless of what `exploration_events` already carried. `derive_seed` is blake2b over `(scope_id, fire_idx, query)`, so a re-issued index under the same scope and query produces the same seed — two ledger rows claiming to be the same draw, the sequential version of the ambiguity `BEGIN IMMEDIATE` closed for concurrent writers. It also broke the "post-#1294 indices never decrease" property that #1303 offers as the reason the two `exploration_events` regimes need no schema marker. The clamp is UNCONDITIONAL, not corrupt-branch-only. A trigger such as `current < 0` leaves the issue's first acceptance bullet unmet for the likeliest hand-edit — a smaller *positive* value. A stored `5` in front of ledger rows at 20/40/60 parses fine and is not negative, so the narrow form re-issues 6..60. The hot-path argument does not survive either: the caller (`hook._substitute_exploration_slots`) claims at most once per UserPromptSubmit turn and only after the default-off enabled check, and `MAX` on `idx_exploration_events_fire` is one index seek. POOLING, stated rather than hidden: store.py's own docstring says pre-#1294 and post-#1294 `exploration_events` rows are not comparable and must not be pooled, the way #1016-B partitions the injection-pack series, and an unqualified `SELECT MAX(fire_idx)` pools across exactly that break. The schema genuinely cannot separate the eras — the table has no regime column, and the absent-key-reads-as-0 design means no crossover timestamp is recorded, so `created_at` cannot partition the rows either. Pooling is nonetheless safe in this direction: the value is used only as a floor, so a pre-change row can push the counter UP but can never hand back an index the ledger already carries. The cost is a bounded one-time skip on a store with pre-change rows, which the modulus test in `should_explore` and the self-contained ledger rows both tolerate. Both the code comment and the docstring say not to read that MAX as a measurement of the post-change series. POPULATION: the lane is default-OFF and the only writer of the key is `str(int(...))`, so no code path produces a corrupt value — this needs a hand-edit or disk corruption to trigger at all. A cheap invariant guard, not a live-incident fix. Mutations verified (staged before mutating; `checkout --` restores from the index): - Delete the clamp block (the pre-fix shape) -> test_a_corrupt_counter_does_not_re_issue_a_ledger_index (got 1) and test_a_hand_edited_smaller_value_does_not_re_issue (got 6) both FAIL. - Replace it with the issue's suggested corrupt-branch-only version (high-water read inside `except`) -> the corrupt-value test passes and test_a_hand_edited_smaller_value_does_not_re_issue FAILS, which is the arm that rules out the first-pass fix. - Guard it on `unusable = current < 0` -> both re-issue tests FAIL, since the `except` branch sets 0 and 0 is not negative. - Drop the `high["m"] is not None` guard -> test_a_corrupt_counter_on_an_empty_ledger_still_returns_1 FAILS (TypeError), along with 10 others. - Drop the new `DELETE FROM exploration_events` from the `_fire` test helper -> test_the_draw_is_deterministic FAILS. The helper winds the counter back to a turn the ledger has recorded, which is precisely the state the clamp refuses, so it must truncate the ledger to stay meaningful; without that it would arm nothing and go quietly green against a non-firing turn.
1 participant
Closes #1294. Parent: #1176 (proposal 5), follow-up from the #1285 review.
What was wrong
exploration_cadenceis documented and read as "one turn in n". It was not.fire_idxcame fromsession_ring.read_ring_state(session_id)["next_fire_idx"], and that counter is per-session — the ring file holds exactly one session andread_ring_statereturns{}on a session-id mismatch, so it restarted constantly and never reached a firing multiple.fire_idx == 20At the specified cadence of 20 the slot fired on 8 of 956 turns all-time and 0 of 259 in the current regime. #1279 lowered the default to 3, which made the lane reachable but left the units per-session — the realised global rate still depended on how the operator happened to segment their work, and coverage of the never-injected pool could not be planned from the setting.
The counter
MemoryStore.next_exploration_fire_idx()— 1-based, monotonic, spans sessions and processes.It lives in
schema_meta, not a new table. A key-value row needs no migration, and anedges-adjacent migration is the operation that left stores unopenable-forever in #1161. An absent key reads as 0, so a store written by an older binary needs no upgrade pass and no backfill.BEGIN IMMEDIATEaround the read-then-write, the #1135 treatment. Sister sessions share one store; deferred, two writers both pass the SELECT before either UPDATEs, both compute the same successor, andexploration_eventsgains two rows claiming to be the same draw — with the same seed, so replay cannot tell them apart.Claimed after the enabled check, so a default-off install takes no write on the hot path.
Acceptance
fire_idxadvances across sessions.test_the_fire_index_accumulates_across_sessions— two store handles over one file, second sees[4, 5, 6].exploration_eventsstays replayable and the regime break is documented.derive_seedis unchanged in form (blake2b over(scope_id, fire_idx, query)); the break is recorded inCONFIG.mdbeside the feat(locks): bound lock injection + fix lock framing — frozen/reference tiers, locks-file manifest, provenance-aware framing #1016-B precedent and in the counter's own docstring. Rows are self-describing — pre-feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1176 proposal 5) #1294 indices restart from low values repeatedly, post-feat(exploration): make fire_idx global so exploration_cadence means one turn in n (#1176 proposal 5) #1294 ones never decrease.The atomicity test is not the obvious one, and I got it wrong first
My first attempt was two store handles claiming alternately and asserting the indices are unique. It passes. It also passes with
immediate=Trueremoved — verified by mutation — because each claim completes before the next begins, so there is no interleaving to observe. That is a guard that cannot fail against the bug it exists to catch, which is precisely the defect #1290 was opened to fix elsewhere in this repo.Replaced with
test_the_claim_takes_the_write_lock_before_reading, which asserts the lock discipline directly and does go red whenimmediate=Trueis dropped. A threaded test would have traded a real assertion for a flaky one. The sequential test is kept — it pins the increment arithmetic — but is renamed and documented as not covering atomicity, so the two are not confused.The #1279 reachability guard was replaced, not deleted
test_the_default_cadence_is_reachable_within_a_real_sessionasserted that a session of typical length reaches a firing turn. That is the wrong property once the counter is global. It is nowtest_the_cadence_is_reachable_and_means_one_turn_in_n:cadenceturns produce exactly one fire, at the right positions.It earned its keep on the way out — raising the default to 20 turned it red, which is what made the change deliberate rather than silent. Its docstring had asked for exactly that.
Verification
47 passedacrosstest_exploration_slot_1279.py+test_exploration_1176.py.immediate=True→ the lock test alone fails; make the counter non-accumulating → the cross-session and gapless tests fail; revert the cadence to 3 → the meaning test fails.MemoryStoreinstances.Summary by Sourcery
Make the exploration fire index a global, store-level counter so
exploration_cadencetruly controls one turn in n across sessions, and update the hook wiring, defaults, docs, and tests accordingly.New Features:
schema_metaand exposed asMemoryStore.next_exploration_fire_idx().Enhancements:
exploration_cadenceback to 20 now that it is defined globally rather than per session.exploration_eventsin CONFIG.md and the changelog.Tests:
Summary by CodeRabbit
New Features
Documentation
Tests