feat: search plumbing for autoresearch loops (budgets, batch, compact results) - #276
Conversation
…helpers Loops built on Alkahest are embarrassingly parallel at the candidate level, but every entry point is one-call-one-answer, so fan-out and per-item error handling had to be hand-rolled at each call site. Add a pure-Python batch/streaming layer (python/alkahest/_batch.py) that never raises for a single bad element: batch_map / batch_map_iter capture exceptions into a BatchItem carrying the failing exception's own E-* code (E-BATCH-001 fallback), always return results aligned to their input index, and support optional ThreadPoolExecutor fan-out. integrate_many / simplify_many / diff_many are thin batch_map wrappers over the three most common derivation entry points. Implements P1 search plumbing item 5 (temp-alkahest/planning/search-plumbing-p1.md). Co-authored-by: Cursor <cursoragent@cursor.com>
P1 search plumbing item 6: agents pay for every character a call returns. Adds DerivedResult.to_dict(mode="full"|"compact") / .to_json(...) on the PyO3 binding, combining .value/.verification/.certificate_status/.steps into one envelope with a stable "alkahest.derived_result" kind discriminator and independent RESULT_SCHEMA_VERSION / STEPS_SCHEMA_VERSION constants (module-level, and DerivedResult.SCHEMA_VERSION / .STEPS_SCHEMA_VERSION class attributes). Compact mode drops before/after step text and uses short step keys (r/s), and prunes verification/certificate_status to their essential fields, but never renames, hides, or drops verification["status"] and never includes Lean certificate source in either mode, so the honesty signal survives the token-budget cut. python/alkahest/_result_schema.py documents the field-name contract (STEP_FIELDS / STEP_FIELDS_COMPACT) and re-exports the version constants for a single canonical import. Co-authored-by: Cursor <cursoragent@cursor.com>
…rch plumbing item 4)
A loop fanning out many candidates (e.g. Groebner/integrate search) needs
to bound one candidate's cost instead of hanging on it or relying on an
OS kill. This adds a cooperative budget checkpoint in alkahest-core:
- alkahest_core::budget: Budget{wall, max_steps, seed}, a thread-local
nesting stack (enter()/BudgetGuard), check() for wall-clock/step/cancel
trips, and a process-wide AtomicBool cancel flag so an orchestrator
thread can stop a heavy call running elsewhere. BudgetError maps to
stable E-BUDGET-001 (wall clock), E-BUDGET-002 (step limit), and
E-BUDGET-003 (cancelled) via AlkahestError.
- integrate::engine checks the budget at its top-level entry and its
recursion boundary and returns IntegrationError::Budget; the Risch
engine propagates it immediately instead of continuing to spend budget.
- simplify::engine checks once per rewrite pass/batch and stops early
(like max_iterations) since simplify has no Result to raise through.
- PyO3 bindings push/pop the Rust budget stack from Python's context()
and map BudgetError to a new PyBudgetExceededError.
On the Python side: Budget(wall_ms=, max_steps=, seed=) dataclass,
context(budget=...) to scope it, BudgetExceededError in the exception
hierarchy, request_cancel()/clear_cancel()/is_cancelled(), budget_seed()
for RNG-consuming samplers that want reproducible runs, and
run_with_wall_fallback() as a documented Python-layer supplement (worker
thread + timeout) for calls without a Rust checkpoint on every path.
Adds docs/mdbook/src/budgets.md, Rust unit tests for the budget module,
and tests/test_budget.py covering nesting, seed round-trips, step/wall
trips, cross-thread cancellation, and run_with_wall_fallback.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR adds cooperative budgets and cancellation across Rust and Python, batch and streaming evaluation utilities, and versioned full or compact ChangesBudget and cancellation controls
Batch and streaming evaluation
DerivedResult serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonContext
participant NativeBudget
participant IntegrationEngine
PythonContext->>NativeBudget: enter budget scope
IntegrationEngine->>NativeBudget: check wall time, steps, and cancellation
NativeBudget-->>IntegrationEngine: BudgetError or success
sequenceDiagram
participant Caller
participant batch_map
participant ThreadPoolExecutor
participant Operation
Caller->>batch_map: submit items and options
batch_map->>ThreadPoolExecutor: schedule item calls when parallel
ThreadPoolExecutor->>Operation: evaluate one item
Operation-->>batch_map: value or exception
batch_map-->>Caller: BatchItem results
sequenceDiagram
participant Caller
participant DerivedResult
participant PythonJson
Caller->>DerivedResult: call to_dict(mode)
DerivedResult-->>Caller: versioned dictionary envelope
Caller->>DerivedResult: call to_json(mode)
DerivedResult->>PythonJson: encode dictionary envelope
PythonJson-->>Caller: JSON text
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
…riant Adding IntegrationError::Budget broke cargo-semver-checks on the exhaustive public enum. Carry E-BUDGET-* inside NotImplemented with a marker so Python still raises BudgetExceededError honestly. Co-authored-by: Cursor <cursoragent@cursor.com>
![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
🧹 Nitpick comments (8)
tests/test_derived_result_schema.py (2)
82-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRequire the complete envelope key set.
required <= set(full.keys())permits an undocumented top-level field without aRESULT_SCHEMA_VERSIONbump. Assert equality. Also assert the same key set for compact mode.Proposed test change
- assert required <= set(full.keys()) + assert set(full.keys()) == required + assert set(dr.to_dict(mode="compact").keys()) == required🤖 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 `@tests/test_derived_result_schema.py` around lines 82 - 92, Update the derived-result schema test around the required key set to assert exact equality with the full result keys rather than allowing undocumented extras, and add the same exact key-set assertion for compact mode. Use the existing required key set for both envelope variants so any top-level schema change requires an explicit RESULT_SCHEMA_VERSION update.
180-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest certificate-source exclusion in both modes.
The documented contract excludes Lean source from full and compact envelopes. This test only covers compact mode.
Proposed test change
-def test_compact_never_contains_lean_source_text(pool): +def test_serialized_envelopes_never_contain_lean_source_text(pool): dr = _multistep_derivation(pool) - compact_json = dr.to_json(mode="compact") - # Lean certificate source is theorem/proof syntax; make sure none of it - # leaked into the compact envelope regardless of derivation shape. - for marker in ("theorem ", "import Mathlib", ":= by"): - assert marker not in compact_json + for mode in ("full", "compact"): + serialized = dr.to_json(mode=mode) + for marker in ("theorem ", "import Mathlib", ":= by"): + assert marker not in serialized🤖 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 `@tests/test_derived_result_schema.py` around lines 180 - 186, Extend test_compact_never_contains_lean_source_text to validate both serialization modes, compact and full, using the existing Lean source markers. Keep the same derivation setup and assert that each marker is absent from both resulting JSON envelopes.alkahest-core/src/budget/mod.rs (2)
302-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe remediation strings are duplicated in the error registry.
alkahest-core/src/errors/codes.rslines 181-183 hold the same three strings. The two copies can drift, and a user then sees different advice depending on the source. Consider defining the strings once (for example asconstitems) and referencing them from both places, or add a test that assertsBudgetError::remediation()equals the matchingREGISTRYentry.Run the following script to check whether other error types already keep the registry and the trait implementation in sync through a test:
#!/bin/bash # Find any existing consistency test between REGISTRY and AlkahestError::remediation. rg -n -C5 'REGISTRY' --type=rust -g '!**/codes.rs' rg -n -C3 'fn remediation' --type=rust | head -80🤖 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 `@alkahest-core/src/budget/mod.rs` around lines 302 - 317, Eliminate the duplicated remediation strings between BudgetError::remediation() and the matching REGISTRY entries in codes.rs. Define each BudgetError message once using shared constants or another existing canonical symbol, then reference those values from both locations so the user-facing advice remains identical and cannot drift.
130-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making
BudgetGuard::droppop its own frame.
droppops the top frame, not the frame thatenterpushed. With normal scoped usage the drop order is LIFO, so this is correct today. If a caller drops guards out of order, or stores guards in a container that drops them front-to-back, the wrong frame is removed. Storing the stack depth in the guard and truncating to that depth removes this hazard.♻️ Optional hardening
pub struct BudgetGuard { + depth: usize, _not_send: std::marker::PhantomData<*const ()>, } impl Drop for BudgetGuard { fn drop(&mut self) { STACK.with(|s| { - s.borrow_mut().pop(); + let mut stack = s.borrow_mut(); + stack.truncate(self.depth); }); } }- STACK.with(|s| s.borrow_mut().push(frame)); + let depth = STACK.with(|s| { + let mut stack = s.borrow_mut(); + stack.push(frame); + stack.len() - 1 + }); BudgetGuard { + depth, _not_send: std::marker::PhantomData, }🤖 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 `@alkahest-core/src/budget/mod.rs` around lines 130 - 162, Update BudgetGuard and enter so each guard records the stack depth associated with its pushed frame, then have BudgetGuard::drop truncate the thread-local stack to that depth instead of unconditionally popping the top frame. Preserve normal nested-scope behavior while ensuring out-of-order guard drops remove the frame owned by that guard.alkahest-core/src/integrate/engine.rs (1)
2085-2090: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a checkpoint inside
integrate_rawrecursion.
integrateandintegrate_innercheck the budget only at their entry.integrate_rawrecurses through theAddandMularms (Lines 1848 and 1894) without any checkpoint. A large sum or product therefore runs an unbounded amount of work between two checkpoints, sowall_msoverruns andmax_stepsunder-counts for those shapes.From<BudgetError> for IntegrationErroralready exists, socrate::budget::check()?works at the top ofintegrate_raw.♻️ Proposed additional checkpoint (outside the selected range, at the top of `integrate_raw`)
pub(crate) fn integrate_raw( expr: ExprId, var: ExprId, pool: &ExprPool, log: &mut DerivationLog, ) -> Result<ExprId, IntegrationError> { // Bounds the sum/product recursion below, which otherwise runs without // any cooperative checkpoint. crate::budget::check()?; ... }Also applies to: 2159-2163
🤖 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 `@alkahest-core/src/integrate/engine.rs` around lines 2085 - 2090, Add a cooperative budget checkpoint at the start of the recursive integrate_raw function, before handling expression variants, by calling crate::budget::check()? and propagating its error. This must cover recursive Add and Mul processing while preserving the existing integration behavior.python/alkahest/exceptions.py (1)
485-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe constructor cannot express
E-BUDGET-002orE-BUDGET-003.The docstring documents three codes.
__init__hardcodesE-BUDGET-001. Any caller that constructs this stub for a step-limit trip or a cancellation must patch.codeafter construction, whichpython/alkahest/_budget.pyat Lines 140-144 does. Accept an optionalcodeargument so the class matches its own documented contract. Keep the default atE-BUDGET-001.♻️ Proposed refactor
def __init__( self, message: str, remediation: str | None = None, span: tuple[int, int] | None = None, + code: str = "E-BUDGET-001", ): - super().__init__(message, code="E-BUDGET-001", remediation=remediation, span=span) + super().__init__(message, code=code, remediation=remediation, span=span)Confirm this stays consistent with the other subclasses in this module before you apply it.
🤖 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 `@python/alkahest/exceptions.py` around lines 485 - 491, Update the budget exception class constructor around __init__ to accept an optional code argument, defaulting to E-BUDGET-001, and pass that value to the base exception instead of hardcoding the code. Preserve the existing message, remediation, and span handling and align the parameter behavior with other exception subclasses in the module.alkahest-py/src/lib.rs (1)
2105-2116: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid emitting the Lean certificate three times per
to_dictcall.
self.certificate(py)runs the Lean emitter.self.verification(py)runs the same emitter again internally.self.certificate_status(py)callsself.certificate(py)a third time. Each emission walks the pool and builds a string thatto_dictthen discards. The docstring targets hot loops and compact mode, so this cost is on the advertised fast path.
certificate_status_fullalready reports the same boolean, soto_dictcan drop its own call.♻️ Minimal change inside this range
- let has_certificate = self.certificate(py).is_some(); let verification_full = self.verification(py); let certificate_status_full = self.certificate_status(py); + let has_certificate: bool = certificate_status_full + .get_item("certifiable")? + .expect("certificate_status always sets certifiable") + .extract()?;Consider also extracting one private helper that emits the certificate once and sharing it between
certificate,verification, andcertificate_status.🤖 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 `@alkahest-py/src/lib.rs` around lines 2105 - 2116, Update DerivedResult::to_dict to reuse certificate_status_full for the certificate-status value and remove the redundant self.certificate(py) call, ensuring compact-mode serialization emits the Lean certificate only once through the existing verification/certificate-status flow. If needed, add one private certificate-emission helper shared by certificate, verification, and certificate_status so a single to_dict call reuses the same emitted result without changing output behavior.tests/test_budget.py (1)
270-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts a property of its own parameters.
codeis a literal from theparametrizelist.code.startswith("E-BUDGET-")therefore always passes and exercises no product code. The docstring says the test locks in the three stable codes, but a rename inalkahest-core/src/budget/mod.rswould not fail it. Assert the codes against the documentation page or against codes observed on real exceptions.💚 Proposed replacement
-@pytest.mark.parametrize("code", ["E-BUDGET-001", "E-BUDGET-002", "E-BUDGET-003"]) -def test_budget_error_codes_documented(code): - """Lock in the three stable codes from the module docstring / mdbook page.""" - assert code.startswith("E-BUDGET-") +@pytest.mark.parametrize("code", ["E-BUDGET-001", "E-BUDGET-002", "E-BUDGET-003"]) +def test_budget_error_codes_documented(code): + """Lock in the three stable codes against the mdbook error table.""" + page = Path(__file__).resolve().parents[1] / "docs" / "mdbook" / "src" / "errors.md" + assert code in page.read_text(encoding="utf-8")Add
from pathlib import Pathto the imports if you apply this.🤖 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 `@tests/test_budget.py` around lines 270 - 273, Replace the self-validating assertion in test_budget_error_codes_documented with a check that reads the documented error codes from the relevant documentation page and verifies the parametrized codes are present, or derives them from real budget exceptions. Add pathlib.Path if needed, ensuring the test fails when codes in alkahest-core/src/budget/mod.rs or its documentation are renamed.
🤖 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 `@alkahest-core/src/budget/mod.rs`:
- Around line 278-281: Update the BudgetError::WallClock display message to use
“exceeded” for the limit clause, while retaining “elapsed” for the measured
duration.
In `@alkahest-py/src/lib.rs`:
- Around line 496-511: Update py_push_budget to convert wall_ms using
Duration::try_from_secs_f64 instead of the panicking from_secs_f64 call. Map
conversion failures, including values outside Duration’s range, to PyValueError
while preserving the existing finite and non-negative validation.
In `@docs/mdbook/src/budgets.md`:
- Around line 119-124: Update the run_with_wall_fallback documentation to state
who clears the cancellation flag after a timeout, based on the implementation
behavior. If the helper does not clear it, instruct callers that catch the
timeout to call clear_cancel() before continuing; otherwise explicitly state
that run_with_wall_fallback performs the reset, while preserving the existing
cooperative-cancellation guidance.
- Around line 81-95: Add the missing time import alongside threading in the
cancellation example so watchdog can call time.sleep without raising NameError;
leave the cancellation flow unchanged.
In `@python/alkahest/_batch.py`:
- Around line 78-81: Use ok as the sole outcome discriminator for BatchItem: in
python/alkahest/_batch.py lines 78-81, remove the claim that value and error are
mutually non-None-exclusive and add coverage for a successful fn returning None;
in docs/mdbook/src/batch.md line 53, document that ok distinguishes success from
failure even when value is None.
- Around line 204-210: Update both batch_map (python/alkahest/_batch.py:204-210)
and batch_map_iter (python/alkahest/_batch.py:266-273) to catch BaseException
while collecting worker futures, cancel all pending futures, and re-raise
immediately without waiting for unrelated running work. Preserve normal result
ordering and iteration behavior when workers complete successfully.
In `@python/alkahest/_budget.py`:
- Around line 185-203: Update the timeout handling in run_with_wall_fallback to
manage ThreadPoolExecutor manually and call shutdown(wait=False) before
propagating _budget_exceeded, so the timeout path returns by the wall-clock
deadline instead of waiting for the worker. Also prevent request_cancel from
leaving unrelated calls affected by either clearing the flag after the worker
stops or documenting the required clear_cancel() contract; preserve cooperative
cancellation for the timed-out call.
In `@python/alkahest/_context.py`:
- Around line 176-189: Update the context manager around the stack append and
native budget setup so a failure from _native.push_budget does not leave ctx on
_state.stack. Move push_budget into the existing try/finally and ensure cleanup
pops both the native budget when successfully pushed and the context frame on
any setup failure or exit.
---
Nitpick comments:
In `@alkahest-core/src/budget/mod.rs`:
- Around line 302-317: Eliminate the duplicated remediation strings between
BudgetError::remediation() and the matching REGISTRY entries in codes.rs. Define
each BudgetError message once using shared constants or another existing
canonical symbol, then reference those values from both locations so the
user-facing advice remains identical and cannot drift.
- Around line 130-162: Update BudgetGuard and enter so each guard records the
stack depth associated with its pushed frame, then have BudgetGuard::drop
truncate the thread-local stack to that depth instead of unconditionally popping
the top frame. Preserve normal nested-scope behavior while ensuring out-of-order
guard drops remove the frame owned by that guard.
In `@alkahest-core/src/integrate/engine.rs`:
- Around line 2085-2090: Add a cooperative budget checkpoint at the start of the
recursive integrate_raw function, before handling expression variants, by
calling crate::budget::check()? and propagating its error. This must cover
recursive Add and Mul processing while preserving the existing integration
behavior.
In `@alkahest-py/src/lib.rs`:
- Around line 2105-2116: Update DerivedResult::to_dict to reuse
certificate_status_full for the certificate-status value and remove the
redundant self.certificate(py) call, ensuring compact-mode serialization emits
the Lean certificate only once through the existing
verification/certificate-status flow. If needed, add one private
certificate-emission helper shared by certificate, verification, and
certificate_status so a single to_dict call reuses the same emitted result
without changing output behavior.
In `@python/alkahest/exceptions.py`:
- Around line 485-491: Update the budget exception class constructor around
__init__ to accept an optional code argument, defaulting to E-BUDGET-001, and
pass that value to the base exception instead of hardcoding the code. Preserve
the existing message, remediation, and span handling and align the parameter
behavior with other exception subclasses in the module.
In `@tests/test_budget.py`:
- Around line 270-273: Replace the self-validating assertion in
test_budget_error_codes_documented with a check that reads the documented error
codes from the relevant documentation page and verifies the parametrized codes
are present, or derives them from real budget exceptions. Add pathlib.Path if
needed, ensuring the test fails when codes in alkahest-core/src/budget/mod.rs or
its documentation are renamed.
In `@tests/test_derived_result_schema.py`:
- Around line 82-92: Update the derived-result schema test around the required
key set to assert exact equality with the full result keys rather than allowing
undocumented extras, and add the same exact key-set assertion for compact mode.
Use the existing required key set for both envelope variants so any top-level
schema change requires an explicit RESULT_SCHEMA_VERSION update.
- Around line 180-186: Extend test_compact_never_contains_lean_source_text to
validate both serialization modes, compact and full, using the existing Lean
source markers. Keep the same derivation setup and assert that each marker is
absent from both resulting JSON envelopes.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 71f6fbaa-1272-4bea-a6a1-14125ef4281b
📒 Files selected for processing (22)
CHANGELOG.mdalkahest-core/src/budget/mod.rsalkahest-core/src/errors/codes.rsalkahest-core/src/integrate/engine.rsalkahest-core/src/integrate/risch/mod.rsalkahest-core/src/lib.rsalkahest-core/src/simplify/engine.rsalkahest-py/src/lib.rsdocs/mdbook/src/SUMMARY.mddocs/mdbook/src/batch.mddocs/mdbook/src/budgets.mddocs/mdbook/src/derivations.mddocs/mdbook/src/errors.mdpython/alkahest/__init__.pypython/alkahest/_batch.pypython/alkahest/_budget.pypython/alkahest/_context.pypython/alkahest/_result_schema.pypython/alkahest/exceptions.pytests/test_batch_workload.pytests/test_budget.pytests/test_derived_result_schema.py
| BudgetError::WallClock { limit, elapsed } => write!( | ||
| f, | ||
| "budget exceeded: wall-clock limit {limit:?} elapsed ({elapsed:?} elapsed)" | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the duplicated word in the wall-clock message.
The message reads "wall-clock limit 50ms elapsed (60ms elapsed)". The first "elapsed" describes the limit, which is confusing. Use "exceeded" for the limit clause.
✏️ Proposed wording fix
BudgetError::WallClock { limit, elapsed } => write!(
f,
- "budget exceeded: wall-clock limit {limit:?} elapsed ({elapsed:?} elapsed)"
+ "budget exceeded: wall-clock limit {limit:?} exceeded ({elapsed:?} elapsed)"
),📝 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.
| BudgetError::WallClock { limit, elapsed } => write!( | |
| f, | |
| "budget exceeded: wall-clock limit {limit:?} elapsed ({elapsed:?} elapsed)" | |
| ), | |
| BudgetError::WallClock { limit, elapsed } => write!( | |
| f, | |
| "budget exceeded: wall-clock limit {limit:?} exceeded ({elapsed:?} elapsed)" | |
| ), |
🤖 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 `@alkahest-core/src/budget/mod.rs` around lines 278 - 281, Update the
BudgetError::WallClock display message to use “exceeded” for the limit clause,
while retaining “elapsed” for the measured duration.
| fn py_push_budget(wall_ms: Option<f64>, max_steps: Option<u64>, seed: Option<u64>) -> PyResult<()> { | ||
| let mut budget = alkahest_core::budget::Budget::new(); | ||
| if let Some(ms) = wall_ms { | ||
| if !ms.is_finite() || ms < 0.0 { | ||
| return Err(PyValueError::new_err( | ||
| "wall_ms must be a finite, non-negative number of milliseconds", | ||
| )); | ||
| } | ||
| budget.wall = Some(std::time::Duration::from_secs_f64(ms / 1000.0)); | ||
| } | ||
| budget.max_steps = max_steps; | ||
| budget.seed = seed; | ||
| let guard = alkahest_core::budget::enter(budget); | ||
| PY_BUDGET_GUARDS.with(|g| g.borrow_mut().push(guard)); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Rust Duration try_from_secs_f64 stabilized version
💡 Result:
The function Duration::try_from_secs_f64 was stabilized in Rust 1.66.0 [1]. It is the checked, non-panicking alternative to from_secs_f64, returning a Result<Duration, TryFromFloatSecsError> [2][1]. The function will return an error if the provided value is negative, non-finite (NaN or infinity), or exceeds the range of a Duration [2][3]. The stabilization was finalized through PR #102271 [4][5].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.TryFromFloatSecsError.html
- 2: https://doc.rust-lang.org/std/time/struct.Duration.html
- 3: https://static.rust-lang.org/doc/master/core/time/struct.Duration.html
- 4: Stabilize
duration_checked_floatrust-lang/rust#102271 - 5: Tracking Issue for
Duration::try_from_secs_{f32, f64}rust-lang/rust#83400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository files =="
git ls-files | rg '(^|/)(Cargo\.toml|Cargo\.lock|pyproject\.toml|Cargo\.lock)$' || true
echo
echo "== Rust edition / MSRV in manifests =="
for f in $(git ls-files 'Cargo.toml'); do
echo "--- $f"
sed -n '1,120p' "$f"
done
echo
echo "== rust toolchain config =="
for f in $(git ls-files | rg '(^|/)(rust-toolchain|rust-toolchain\.toml)$'); do
echo "--- $f"
cat "$f"
done
echo
echo "== relevant lib.rs section =="
sed -n '450,530p' alkahest-py/src/lib.rsRepository: alkahest-cas/alkahest
Length of output: 4761
🌐 Web query:
Rust Duration from_secs_f64 behavior infinity NaN negative saturating panic source
💡 Result:
In Rust, the Duration::from_secs_f64(secs: f64) function is designed to convert a floating-point number of seconds into a Duration [1][2]. This function will panic if the provided float value is negative, infinite (NaN or infinity), or if the resulting duration would overflow the capacity of a Duration object [3][1]. The implementation relies on an internal call to Duration::try_from_secs_f64(secs), which performs the validation [3][4]. If the conversion fails due to any of these invalid conditions, it returns an error that triggers a panic with an error message [3][4]. If you need to perform this conversion without causing a panic (for example, when parsing user input), you can use the non-panicking Duration::try_from_secs_f64(secs) method instead, which returns a Result<Duration, TryFromFloatSecsError> [3][4]. This allows you to handle edge cases like negative values, non-finite numbers (NaN/Infinity), or overflows gracefully in your own code [5].
Citations:
- 1: https://doc.rust-lang.org/std/time/struct.Duration.html
- 2: https://doc.rust-lang.org/stable/core/time/struct.Duration.html
- 3: https://doc.rust-lang.org/stable/src/core/time.rs.html
- 4: https://github.com/rust-lang/rust/blob/024757fe/library/core/src/time.rs
- 5: https://users.rust-lang.org/t/duration-from-float-without-panic/47528
Check the wall-clock value before converting seconds to Duration.
std::time::Duration::try_from_secs_f64 is stable and rejects values outside the Duration range; it would have caught wall_ms = 1e30 safely. Convert with that checked API and return PyValueError instead of calling Duration::from_secs_f64(ms / 1000.0), which panics on overflow.
🤖 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 `@alkahest-py/src/lib.rs` around lines 496 - 511, Update py_push_budget to
convert wall_ms using Duration::try_from_secs_f64 instead of the panicking
from_secs_f64 call. Map conversion failures, including values outside Duration’s
range, to PyValueError while preserving the existing finite and non-negative
validation.
| ```python | ||
| import threading | ||
|
|
||
| def watchdog(): | ||
| time.sleep(0.05) | ||
| ak.request_cancel() | ||
|
|
||
| threading.Thread(target=watchdog, daemon=True).start() | ||
| try: | ||
| ak.integrate(hard_expr, x) | ||
| except ak.BudgetExceededError as e: | ||
| assert e.code == "E-BUDGET-003" | ||
| finally: | ||
| ak.clear_cancel() | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing time import to the cancellation example.
The example imports threading only. watchdog calls time.sleep, so the snippet raises NameError if a reader copies it.
📝 Proposed fix
import threading
+import time
def watchdog():
time.sleep(0.05)
ak.request_cancel()📝 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.
| ```python | |
| import threading | |
| def watchdog(): | |
| time.sleep(0.05) | |
| ak.request_cancel() | |
| threading.Thread(target=watchdog, daemon=True).start() | |
| try: | |
| ak.integrate(hard_expr, x) | |
| except ak.BudgetExceededError as e: | |
| assert e.code == "E-BUDGET-003" | |
| finally: | |
| ak.clear_cancel() | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mdbook/src/budgets.md` around lines 81 - 95, Add the missing time import
alongside threading in the cancellation example so watchdog can call time.sleep
without raising NameError; leave the cancellation flow unchanged.
| Python cannot forcibly kill a thread, so on a timeout the call keeps running in the | ||
| background until it either finishes or reaches a Rust cooperative checkpoint — | ||
| `run_with_wall_fallback` also calls `request_cancel()` on timeout so any checkpoint the | ||
| call reaches asks it to stop. Prefer the Rust cooperative check alone | ||
| (`context(budget=...)`) wherever a call already honors it (`integrate` today); reach for | ||
| this only when you need a hard deadline on a path that doesn't. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
State that the caller must clear the cancellation flag after a run_with_wall_fallback timeout.
This section says run_with_wall_fallback calls request_cancel() on timeout. The "Cancellation" section says a flag that stays set trips E-BUDGET-003 for every later call. A reader who catches the timeout and continues therefore breaks the next candidate. Tell the reader to call clear_cancel() after the timeout, or state that run_with_wall_fallback clears the flag itself.
Run the following script to confirm which side owns the reset:
#!/bin/bash
# Check whether run_with_wall_fallback clears the cancellation flag after the timeout.
rg -n -C15 'def run_with_wall_fallback' python/alkahest/_budget.py🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mdbook/src/budgets.md` around lines 119 - 124, Update the
run_with_wall_fallback documentation to state who clears the cancellation flag
after a timeout, based on the implementation behavior. If the helper does not
clear it, instruct callers that catch the timeout to call clear_cancel() before
continuing; otherwise explicitly state that run_with_wall_fallback performs the
reset, while preserving the existing cooperative-cancellation guidance.
| Exactly one of *value* / *error* is populated: ``ok=True`` implies | ||
| ``error is None`` and ``value`` is whatever *fn* returned (often a | ||
| :class:`~alkahest.DerivedResult`); ``ok=False`` implies ``value is None`` | ||
| and ``error`` describes what went wrong. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use ok as the sole outcome discriminator.
A valid fn can return None. The current result is then BatchItem(ok=True, value=None, error=None). This contradicts the stated exclusivity invariant.
python/alkahest/_batch.py#L78-L81: remove the non-None exclusivity claim and add coverage for a successfulNoneresult.docs/mdbook/src/batch.md#L53-L53: state thatokdistinguishes success from failure whenvalueisNone.
📍 Affects 2 files
python/alkahest/_batch.py#L78-L81(this comment)docs/mdbook/src/batch.md#L53-L53
🤖 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 `@python/alkahest/_batch.py` around lines 78 - 81, Use ok as the sole outcome
discriminator for BatchItem: in python/alkahest/_batch.py lines 78-81, remove
the claim that value and error are mutually non-None-exclusive and add coverage
for a successful fn returning None; in docs/mdbook/src/batch.md line 53,
document that ok distinguishes success from failure even when value is None.
| with ThreadPoolExecutor(max_workers=max_workers) as executor: | ||
| futures = { | ||
| executor.submit(_invoke, fn, item, i, kwargs): i for i, item in enumerate(materialized) | ||
| } | ||
| for future in futures: | ||
| results[futures[future]] = future.result() | ||
| return results # type: ignore[return-value] # every slot was filled above |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'python/alkahest/_batch.py$|python/alkahest/.*batch.*|_batch.py' || true
echo
echo "Outline:"
ast-grep outline python/alkahest/_batch.py --view expanded || true
echo
echo "Relevant lines:"
sed -n '1,330p' python/alkahest/_batch.py | cat -n
echo
echo "Imports and Thread pool references:"
rg -n "ThreadPoolExecutor|wait|FIRST_COMPLETED|_invoke|KeyboardInterrupt|cancel" python/alkahest/_batch.py || trueRepository: alkahest-cas/alkahest
Length of output: 16239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
from concurrent.futures import ThreadPoolExecutor, as_completed, FIRST_COMPLETED, wait
start = event = None
def worker(x):
event.wait()
return x + 1
executor = ThreadPoolExecutor(max_workers=1)
future = executor.submit(worker, 1)
event = threading.Event()
future2 = executor.submit(worker, 2)
try:
raise KeyboardInterrupt
except BaseException as exc:
assert exec_type(exc) is KeyboardInterrupt
result = None
future.cancel()
with ThreadPoolExecutor(max_workers=1) as e:
try:
for f in e.map(worker, range(10), timeout=0.1):
pass
except Exception as exc:
print(type(exc))
PYRepository: alkahest-cas/alkahest
Length of output: 276
🌐 Web query:
Python concurrent.futures ThreadPoolExecutor shutdown wait for all threads with KeyboardInterrupt future.result BaseException
💡 Result:
Handling KeyboardInterrupt in a Python ThreadPoolExecutor requires understanding that the Python interpreter will not exit until all thread pool threads have completed their tasks, even if you initiate a shutdown [1][2]. When a KeyboardInterrupt occurs in the main thread, the default behavior of the executor is to continue executing existing tasks until they finish, which can cause the program to appear to hang [3][4]. To properly manage shutdown during a KeyboardInterrupt, you should use the following strategies: 1. Use cancel_futures=True: When calling shutdown, set cancel_futures=True to cancel all pending (non-started) futures in the work queue [1][2]. Note that this does not stop tasks that are already currently running [1][3]. executor.shutdown(wait=True, cancel_futures=True) 2. Catch KeyboardInterrupt and Shutdown Explicitly: Wrap your execution logic in a try-finally block to ensure that the executor is shut down regardless of how the code exits [3]. try: # Submit tasks and wait except KeyboardInterrupt: executor.shutdown(wait=True, cancel_futures=True) raise finally: executor.shutdown(wait=True, cancel_futures=True) 3. Implement Cooperative Cancellation: Since ThreadPoolExecutor cannot forcibly kill a running thread, tasks must be written to periodically check for a cancellation signal (such as a threading.Event) if you require immediate termination of long-running operations [5][6]. 4. Understanding BaseException and Result Handling: If you are waiting on a future using future.result, note that if the task fails or is cancelled, accessing.result will raise the exception that occurred during execution [5]. If you catch KeyboardInterrupt in the main thread while waiting for results, simply calling.result on futures does not trigger a shutdown of the pool itself; you must explicitly call shutdown [5][3]. If you absolutely must exit immediately regardless of clean-up, some developers use os._exit(1) in the signal handler, though this is discouraged because it skips standard Python cleanup routines (like calling exit methods or atexit handlers) [5]. Relying on clean shutdown via executor.shutdown is the recommended practice for preserving data integrity [1][7].
Citations:
- 1: https://docs.python.org/3/library/concurrent.futures.html
- 2: https://docs.python.org/3.11/library/concurrent.futures.html
- 3: https://stackoverflow.com/questions/69691136/interrupt-threadpoolexecutor
- 4: https://discuss.python.org/t/graceful-exit-from-threadpoolexecutor-when-blocked-on-io-problem-and-possible-enhancement/80380
- 5: https://stackoverflow.com/questions/78583296/shutting-a-threadpoolexecutor-down-with-a-keyboardinterrupt
- 6: https://stackoverflow.com/questions/65832061/threadpoolexecutor-keyboardinterrupt
- 7: https://stackoverflow.com/questions/52081033/how-to-shutdown-process-with-event-loop-and-executor
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import threading
import time
from concurrent.futures import FIRST_COMPLETED, ThreadPoolExecutor, as_completed, wait
signal = threading.Event()
def unblock_event_work(_):
signal.set()
print("worker running")
time.sleep(100)
return 1
def long_then_interrupt(_):
while not signal.is_set():
pass
time.sleep(100)
print("worker completed")
return "interrupted"
def probe(batch_map-like):
def slow_worker(_):
time.sleep(100)
return "slow"
executor = ThreadPoolExecutor(max_workers=1)
future = executor.submit(long_then_interrupt)
executor.submit(slow_wait_work)
signal.wait()
try:
return future.result()
except Exception as exc:
print(type(exc).__name__)
executor.shutdown(wait=False)
return None
print("ThreadPoolExecutor uses threads:", ThreadPoolExecutor is not None)
executors = []
for parallel in [False, True]:
print(f"sequential_mode={not parallel} worker raises KeyboardInterrupt", parallel)
PY
sed -n '130,210p' python/alkahest/_batch.py | cat -nRepository: alkahest-cas/alkahest
Length of output: 269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
from concurrent.futures import ThreadPoolExecutor, FIRST_COMPLETED, wait
def describe(method):
body = inspect.getsource(method)
idx = body.find("def ")
if idx >= 0:
body = body[idx:]
for tag in ["__init__", "__exit__", "shutdown", "cancel", "result"]:
print(tag, "in" if tag in body else "not in")
print()
pool = ThreadPoolExecutor.__new__(ThreadPoolExecutor)
try:
describe(pool.__init__)
except TypeError:
describe(inspect.getsource(ThreadPoolExecutor.__init__))
try:
describe(pool.__exit__)
except TypeError:
describe(inspect.getsource(ThreadPoolExecutor.__exit__))
try:
describe(pool.shutdown)
except TypeError:
describe(inspect.getsource(ThreadPoolExecutor.shutdown))
print("cancel exists:", hasattr(ThreadPoolExecutor, "cancel"))
print("map timeout arg present:", "timeout" in inspect.signature(ThreadPoolExecutor.map).parameters)
PYRepository: alkahest-cas/alkahest
Length of output: 417
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from concurrent.futures import ThreadPoolExecutor
pool = ThreadPoolExecutor(max_workers=1)
try:
help_ = ThreadPoolExecutor.__exit__(pool, KeyboardInterrupt, KeyboardInterrupt("k"), None)
print("__exit__ returns for caught KeyboardInterrupt:", help_)
except Exception as exc:
print(type(exc).__name__, exc)
PYRepository: alkahest-cas/alkahest
Length of output: 212
🌐 Web query:
Python 3.13 concurrent.futures ThreadPoolExecutor __exit__ KeyboardInterrupt result cancel_futures
💡 Result:
In Python 3.13, the concurrent.futures.ThreadPoolExecutor context manager uses the exit method to shut down the executor, which by default calls self.shutdown(wait=True) [1]. This behavior ensures that the interpreter waits for all currently running and pending futures to complete before allowing the program to exit [2][1]. When a KeyboardInterrupt occurs while using a ThreadPoolExecutor as a context manager: 1. Default Behavior: The exit method is still executed as part of the context manager's cleanup process. Because it calls shutdown(wait=True), the main thread will block while attempting to join all worker threads, effectively ignoring the immediate impact of the KeyboardInterrupt until all in-flight tasks have finished [3][4][5]. 2. Lack of Automatic Cancellation: The default exit implementation does not accept arguments like cancel_futures [3][1]. Even if an exception (such as KeyboardInterrupt) is passed to exit, the current implementation simply calls shutdown(wait=True) without passing cancel_futures=True [3][1]. Consequently, any pending or running tasks are not automatically cancelled upon receiving a signal [3]. 3. Workarounds: - Manual Cancellation: You cannot directly enable cancel_futures within the standard context manager syntax. To achieve cancellation, you must manually call executor.shutdown(wait=True, cancel_futures=True) inside a try-except block or before the context manager exits [3]. - OS-level Exit: In scenarios where graceful shutdown is not feasible due to long-running tasks, some developers resort to os._exit(1) within a signal handler or the exception block to terminate the process immediately, bypassing the wait [4]. There is ongoing discussion in the Python community regarding updating the exit behavior to automatically cancel futures if an exception occurs, but as of Python 3.13, the blocking wait behavior remains the standard [3].
Citations:
- 1: https://github.com/python/cpython/blob/3.13/Lib/concurrent/futures/_base.py
- 2: https://docs.python.org/3/library/concurrent.futures.html
- 3: Cancel futures if concurrent.futures.Executor is used as a context manager and there is an exception? python/cpython#130975
- 4: https://stackoverflow.com/questions/78583296/shutting-a-threadpoolexecutor-down-with-a-keyboardinterrupt
- 5: https://stackoverflow.com/questions/65832061/threadpoolexecutor-keyboardinterrupt
🌐 Web query:
Python ThreadPoolExecutor map cancel pending futures result wait shutdown cancel_futures context manager
💡 Result:
In Python's concurrent.futures.ThreadPoolExecutor, the interaction between map, shutdown, and future cancellation involves specific mechanisms for managing task lifecycles [1][2]. 1. Executor.map and Cancellation The map method submits all tasks from the provided iterable to the executor [2]. If the returned iterator is dropped—particularly if it is only partially consumed—the remaining pending futures associated with those tasks may be cancelled [3]. This behavior occurs because of internal cleanup logic in the concurrent.futures module and can lead to unexpected results if the iterator is not fully exhausted or explicitly managed [3]. 2. Shutdown and cancel_futures The shutdown(wait=True, *, cancel_futures=False) method is used to clean up the executor [1][2]. - cancel_futures: If set to True, all pending futures that have not yet started executing are cancelled [1][2]. Futures that are already running or completed are unaffected [1][2]. - wait: If set to True (default), the method blocks until all pending futures have finished execution or been cancelled [1][2]. - When both are set to True, the executor ensures that currently running futures complete while all other pending futures are cancelled, before the method returns [1][2]. 3. Context Manager Using a ThreadPoolExecutor as a context manager (e.g., with ThreadPoolExecutor as executor:) is equivalent to calling shutdown(wait=True) upon exiting the block [4]. It does not automatically enable cancel_futures; it performs a blocking shutdown to ensure all submitted tasks are completed [4]. Because it behaves as a standard shutdown(wait=True), it does not cancel pending tasks that haven't started [4]. 4. Future Cancellation Individual futures can be cancelled using their cancel method [1][5]. This will return True if the task was successfully cancelled (i.e., it had not started running), or False if it was already running or completed [1][5]. If a future is cancelled, any threads waiting for that future (e.g., via as_completed or wait) will be notified [5]. Summary Table - shutdown(wait=True, cancel_futures=True): Blocks until completion/cancellation of all tasks; cancels tasks that haven't started. - Context Manager: Automatically calls shutdown(wait=True); waits for all tasks to finish. - Iterator from map: Partial consumption/dropping may implicitly cancel remaining pending tasks in that specific map submission [3].
Citations:
- 1: https://docs.python.org/3/library/concurrent.futures.html
- 2: https://docs.python.org/3.11/library/concurrent.futures.html
- 3: Dropping concurrent.futures.Executor.map result cancels pending futures python/cpython#136578
- 4: https://docs.python.org/release/3.9.22/library/concurrent.futures.html
- 5: https://peps.python.org/pep-3148/
Handle BaseException before waiting on futures.
When a worker raises KeyboardInterrupt, _invoke lets it escape, but ThreadPoolExecutor cleanup waits for all running work. In batch_map, the final loop also waits on all other futures after the first interrupt completes rather than returning that signal. Handle BaseException from workers, cancel pending futures, and avoid waiting for unrelated running work before propagating the signal in both batch_map and batch_map_iter.
📍 Affects 1 file
python/alkahest/_batch.py#L204-L210(this comment)python/alkahest/_batch.py#L266-L273
🤖 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 `@python/alkahest/_batch.py` around lines 204 - 210, Update both batch_map
(python/alkahest/_batch.py:204-210) and batch_map_iter
(python/alkahest/_batch.py:266-273) to catch BaseException while collecting
worker futures, cancel all pending futures, and re-raise immediately without
waiting for unrelated running work. Preserve normal result ordering and
iteration behavior when workers complete successfully.
| if budget.wall_ms is None: | ||
| return fn(*args, **kwargs) | ||
|
|
||
| with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool: | ||
| future = pool.submit(fn, *args, **kwargs) | ||
| try: | ||
| return future.result(timeout=budget.wall_ms / 1000.0) | ||
| except concurrent.futures.TimeoutError as exc: | ||
| # Best-effort: ask any cooperative Rust checkpoint the call has | ||
| # reached (or will reach) to stop, since we can't stop the | ||
| # Python thread itself. | ||
| request_cancel() | ||
| raise _budget_exceeded( | ||
| f"[E-BUDGET-001] budget exceeded: wall-clock limit {budget.wall_ms} ms elapsed", | ||
| remediation=( | ||
| "raise Budget(wall_ms=...), or accept a heuristic/numeric result for this " | ||
| "candidate instead of an exact one" | ||
| ), | ||
| ) from exc |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
The timeout path does not return within wall_ms, and it leaves the cancel flag set.
Two defects in this block:
with concurrent.futures.ThreadPoolExecutor(...)callsshutdown(wait=True)on exit. The raisedBudgetExceededErrorpropagates through that__exit__, so the call blocks until the abandoned worker finishes.run_with_wall_fallback(slow_fn, budget=Budget(wall_ms=10))returns control only afterslow_fncompletes. The documented hard deadline is not enforced.tests/test_budget.py::test_run_with_wall_fallback_raises_on_timeoutstill passes because it only asserts the exception, not the elapsed time.request_cancel()sets a process-wide flag and nothing clears it. After a single timeout, every other thread's next cooperative checkpoint fails withE-BUDGET-003, including unrelated calls. The autouse_clear_cancel_before_and_afterfixture intests/test_budget.pymasks this in the test suite. Neither the function docstring nor the module docstring instructs the caller to callclear_cancel().
Fix 1 by managing the executor manually and shutting it down without waiting. For 2, either document that the caller must call clear_cancel(), or do not set the global flag here.
🐛 Proposed fix for the blocking shutdown
- with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
- future = pool.submit(fn, *args, **kwargs)
- try:
- return future.result(timeout=budget.wall_ms / 1000.0)
- except concurrent.futures.TimeoutError as exc:
- # Best-effort: ask any cooperative Rust checkpoint the call has
- # reached (or will reach) to stop, since we can't stop the
- # Python thread itself.
- request_cancel()
- raise _budget_exceeded(
- f"[E-BUDGET-001] budget exceeded: wall-clock limit {budget.wall_ms} ms elapsed",
- remediation=(
- "raise Budget(wall_ms=...), or accept a heuristic/numeric result for this "
- "candidate instead of an exact one"
- ),
- ) from exc
+ pool = concurrent.futures.ThreadPoolExecutor(max_workers=1)
+ future = pool.submit(fn, *args, **kwargs)
+ try:
+ result = future.result(timeout=budget.wall_ms / 1000.0)
+ except concurrent.futures.TimeoutError as exc:
+ # Best-effort: ask any cooperative Rust checkpoint the call has
+ # reached (or will reach) to stop, since we can't stop the
+ # Python thread itself. Do not wait for the worker — waiting would
+ # defeat the deadline this function exists to enforce.
+ request_cancel()
+ pool.shutdown(wait=False)
+ raise _budget_exceeded(
+ f"[E-BUDGET-001] budget exceeded: wall-clock limit {budget.wall_ms} ms elapsed",
+ remediation=(
+ "raise Budget(wall_ms=...), or accept a heuristic/numeric result for this "
+ "candidate instead of an exact one; call alkahest.clear_cancel() before the "
+ "next candidate, because the timeout set the process-wide cancel flag"
+ ),
+ ) from exc
+ else:
+ pool.shutdown(wait=False)
+ return resultNote that the worker thread is non-daemon, so interpreter shutdown still joins it.
🤖 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 `@python/alkahest/_budget.py` around lines 185 - 203, Update the timeout
handling in run_with_wall_fallback to manage ThreadPoolExecutor manually and
call shutdown(wait=False) before propagating _budget_exceeded, so the timeout
path returns by the wall-clock deadline instead of waiting for the worker. Also
prevent request_cancel from leaving unrelated calls affected by either clearing
the flag after the worker stops or documenting the required clear_cancel()
contract; preserve cooperative cancellation for the timed-out call.
| _state.stack.append(ctx) | ||
| budget_pushed = False | ||
| if budget is not None: | ||
| from . import alkahest as _native | ||
|
|
||
| _native.push_budget(wall_ms=budget.wall_ms, max_steps=budget.max_steps, seed=budget.seed) | ||
| budget_pushed = True | ||
| try: | ||
| yield | ||
| finally: | ||
| if budget_pushed: | ||
| from . import alkahest as _native | ||
|
|
||
| _native.pop_budget() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Pop the context frame if push_budget fails.
Line 176 appends ctx to the thread-local stack. Line 181 then calls into the extension. If push_budget raises, the generator terminates before the try, so the finally block never runs and ctx stays on _state.stack for the lifetime of the thread. Every later active_pool() / active_domain() call on that thread then reads the stale frame. Move the native push inside the protected region, or unwind the append on failure.
🐛 Proposed fix
_state.stack.append(ctx)
budget_pushed = False
- if budget is not None:
- from . import alkahest as _native
-
- _native.push_budget(wall_ms=budget.wall_ms, max_steps=budget.max_steps, seed=budget.seed)
- budget_pushed = True
try:
+ if budget is not None:
+ from . import alkahest as _native
+
+ _native.push_budget(
+ wall_ms=budget.wall_ms, max_steps=budget.max_steps, seed=budget.seed
+ )
+ budget_pushed = True
yield
finally:
if budget_pushed:
from . import alkahest as _native
_native.pop_budget()
_state.stack.pop()📝 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.
| _state.stack.append(ctx) | |
| budget_pushed = False | |
| if budget is not None: | |
| from . import alkahest as _native | |
| _native.push_budget(wall_ms=budget.wall_ms, max_steps=budget.max_steps, seed=budget.seed) | |
| budget_pushed = True | |
| try: | |
| yield | |
| finally: | |
| if budget_pushed: | |
| from . import alkahest as _native | |
| _native.pop_budget() | |
| _state.stack.append(ctx) | |
| budget_pushed = False | |
| try: | |
| if budget is not None: | |
| from . import alkahest as _native | |
| _native.push_budget( | |
| wall_ms=budget.wall_ms, max_steps=budget.max_steps, seed=budget.seed | |
| ) | |
| budget_pushed = True | |
| yield | |
| finally: | |
| if budget_pushed: | |
| from . import alkahest as _native | |
| _native.pop_budget() | |
| _state.stack.pop() |
🤖 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 `@python/alkahest/_context.py` around lines 176 - 189, Update the context
manager around the stack append and native budget setup so a failure from
_native.push_budget does not leave ctx on _state.stack. Move push_budget into
the existing try/finally and ensure cleanup pops both the native budget when
successfully pushed and the context frame on any setup failure or exit.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Budget+context(budget=…)with cooperative Rust checkpoints;BudgetExceededError(E-BUDGET-001..003);request_cancel/budget_seed.batch_map/*_manythat never raise on one bad element; optionalparallel=True; preservesE-*codes (E-BATCH-001fallback).DerivedResult.to_dict/to_jsonwith honestmode="compact"(keepsverification.status, omits Lean source).Implements P1 search-plumbing items 4–6 from the autoresearch planning note. Leaves CAD/
decidealone.Test plan
cargo test -p alkahest-cas budget --lib(9 tests)pytest tests/test_budget.py tests/test_batch_workload.py tests/test_derived_result_schema.py(76 tests)docs/mdbook/src/budgets.md,batch.md,derivations.mdMade with Cursor
Summary by CodeRabbit
New Features
DerivedResultserialization in full and compact dictionary or JSON formats.Documentation