📝 CodeRabbit Chat: Implement requested code changes#131
Open
coderabbitai[bot] wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
github-actions
Bot
added
the
scope: agent
Contributor
Reviewer's GuideRefactors test-only synchronization helpers for RoutineEngine by moving them into a dedicated, feature-gated module, adjusts RoutineEngine’s test API to expose the underlying atomic counter, and wires the new helpers into the e2e traces test suite while keeping production code and non-libsql tests unaffected. Sequence diagram for e2e trace test synchronisation using RoutineEngine.running_countsequenceDiagram
actor Tester
participant E2ETracesTest as E2ETracesTest
participant RoutineEngine as RoutineEngine
participant RoutineSync as RoutineSyncModule
Tester->>E2ETracesTest: run_test_case()
E2ETracesTest->>RoutineEngine: start_routine()
RoutineEngine-->>RoutineEngine: increment running_count AtomicUsize
E2ETracesTest->>RoutineSync: wait_until_idle(engine)
RoutineSync->>RoutineEngine: running_count()
RoutineEngine-->>RoutineSync: Arc<AtomicUsize>
loop poll until zero
RoutineSync-->>RoutineSync: read atomic value
alt running_count > 0
RoutineSync-->>RoutineSync: sleep/backoff
else running_count == 0
RoutineSync-->>E2ETracesTest: routines_idle()
end
end
E2ETracesTest-->>Tester: assertions_passed
Class diagram for RoutineEngine test-only API and sync helpersclassDiagram
class RoutineEngine {
+running_count() Arc~AtomicUsize~
}
class AtomicUsize {
+load(ordering)
+store(value, ordering)
+fetch_add(value, ordering)
+fetch_sub(value, ordering)
}
class RoutineSyncModule {
<<module>>
+wait_for_running_count(engine, expected_count)
+wait_until_idle(engine)
}
class E2ETracesTestsModule {
<<module>>
+routine_cooldown_tests()
+routine_cron_tests()
+routine_event_tests()
+routine_system_event_tests()
}
RoutineEngine --> AtomicUsize : uses
RoutineSyncModule --> RoutineEngine : test_synchronisation
E2ETracesTestsModule --> RoutineSyncModule : uses
E2ETracesTestsModule --> RoutineEngine : drives_routines
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
size: M
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
wait_for_persisted_run, you now have two termination conditions (timeout andmax_attempts), which can conflict with thetimeoutparameter’s intent; consider either derivingmax_attemptsfromtimeout/poll_intervalor relying on a single condition so callers’ timeouts behave predictably. - Changing
RoutineEngine::running_countto returnArc<AtomicUsize>and gating it behindcfg(feature = "libsql")alters its public surface even though it’s test-oriented; consider either returning&AtomicUsize(avoiding an extra clone) and/or keeping the method available behind a test-only cfg to reduce surprises for non-libsql consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `wait_for_persisted_run`, you now have two termination conditions (timeout and `max_attempts`), which can conflict with the `timeout` parameter’s intent; consider either deriving `max_attempts` from `timeout`/`poll_interval` or relying on a single condition so callers’ timeouts behave predictably.
- Changing `RoutineEngine::running_count` to return `Arc<AtomicUsize>` and gating it behind `cfg(feature = "libsql")` alters its public surface even though it’s test-oriented; consider either returning `&AtomicUsize` (avoiding an extra clone) and/or keeping the method available behind a test-only cfg to reduce surprises for non-libsql consumers.
## Individual Comments
### Comment 1
<location path="tests/support/routine_sync.rs" line_range="65-50" />
<code_context>
-/// * `routine_id` - The ID of the routine to check for runs
-/// * `timeout` - Maximum duration to wait for persistence
-#[allow(dead_code)]
-pub async fn wait_for_persisted_run(db: &Arc<dyn Database>, routine_id: Uuid, timeout: Duration) {
- let start = std::time::Instant::now();
- let poll_interval = Duration::from_millis(10);
-
- loop {
- let runs = db
- .list_routine_runs(routine_id, 10)
- .await
- .expect("list_routine_runs should not fail");
-
- if !runs.is_empty() {
- return;
- }
-
- if start.elapsed() >= timeout {
- panic!(
- "Timeout waiting for routine run to be persisted (routine_id: {}, elapsed: {:?})",
- routine_id,
- start.elapsed()
- );
- }
-
- tokio::time::sleep(poll_interval).await;
- }
-}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid having both `timeout` and a hard-coded `max_attempts` to prevent surprising behaviour in long-running tests
Because `max_attempts` is tied to a fixed 5s window while `timeout` is configurable, callers using a longer `timeout` (e.g. 15s in slow CI) would still hit the `max_attempts` condition and panic after ~5s. To keep behavior aligned with the requested `timeout`, either derive `max_attempts` from `timeout` and `poll_interval` (e.g. `timeout / poll_interval`) or remove `max_attempts` and rely solely on `timeout`.
Suggested implementation:
```rust
// Derive max_attempts from the configured timeout so the loop's upper bound
// is consistent with the requested maximum wait duration.
let max_attempts = (timeout.as_millis() / poll_interval.as_millis())
.max(1) as u32;
```
Depending on the exact current implementation, you may also need to:
1. Ensure `timeout` and `poll_interval` are both `Duration` values already in scope at the point where `max_attempts` is defined.
2. If `max_attempts` is typed as something other than `u32` (e.g. `usize`), adjust the cast to match:
- For `usize`: `as usize` instead of `as u32`.
3. If the doc comment or tests mention a fixed 5s window / 500 attempts, update them to reflect that the limit is now derived from `timeout` (e.g. "waits up to `timeout` for persistence").
4. If there are multiple helpers in `tests/support/routine_sync.rs` that define a hard-coded `max_attempts` (for idle or persistence helpers), apply the same pattern to each one so all of them respect the configured `timeout`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ); | ||
| } | ||
|
|
||
| tokio::time::sleep(poll_interval).await; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Avoid having both timeout and a hard-coded max_attempts to prevent surprising behaviour in long-running tests
Because max_attempts is tied to a fixed 5s window while timeout is configurable, callers using a longer timeout (e.g. 15s in slow CI) would still hit the max_attempts condition and panic after ~5s. To keep behavior aligned with the requested timeout, either derive max_attempts from timeout and poll_interval (e.g. timeout / poll_interval) or remove max_attempts and rely solely on timeout.
Suggested implementation:
// Derive max_attempts from the configured timeout so the loop's upper bound
// is consistent with the requested maximum wait duration.
let max_attempts = (timeout.as_millis() / poll_interval.as_millis())
.max(1) as u32;Depending on the exact current implementation, you may also need to:
- Ensure
timeoutandpoll_intervalare bothDurationvalues already in scope at the point wheremax_attemptsis defined. - If
max_attemptsis typed as something other thanu32(e.g.usize), adjust the cast to match:- For
usize:as usizeinstead ofas u32.
- For
- If the doc comment or tests mention a fixed 5s window / 500 attempts, update them to reflect that the limit is now derived from
timeout(e.g. "waits up totimeoutfor persistence"). - If there are multiple helpers in
tests/support/routine_sync.rsthat define a hard-codedmax_attempts(for idle or persistence helpers), apply the same pattern to each one so all of them respect the configuredtimeout.
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.
Code changes was requested by @leynos.
The following files were modified:
src/agent/routine_engine.rstests/e2e_traces.rstests/e2e_traces/routine_cooldown.rstests/e2e_traces/routine_cron.rstests/e2e_traces/routine_event.rstests/e2e_traces/routine_system_event.rstests/support/routine_sync.rstests/support/routines.rsSummary by Sourcery
Introduce libsql-gated test-only synchronization utilities for RoutineEngine and adjust routine engine API accordingly.
New Features:
Enhancements: