fix: optimize evaluation overview reads#168
Merged
Merged
Conversation
Replace per-session evaluation overview lookups with bulk storage reads and targeted evaluation-result/shadow verdict helpers. This keeps the public response shape unchanged while avoiding N+1 request and citation scans for self-hosted deployments.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces ChangesBulk Storage API and Service Refactor
Sequence Diagram(s)sequenceDiagram
participant APIEndpoint as API / Service Caller
participant EvalOverviewService as EvaluationOverviewService
participant Storage
participant Braintrust
APIEndpoint->>EvalOverviewService: run(request)
EvalOverviewService->>Storage: get_agent_success_evaluation_results_in_window(extended_from, to_ts)
Storage-->>EvalOverviewService: bulk AgentSuccessEvaluationResult list
EvalOverviewService->>Storage: get_first_requests_by_session_ids(session_ids)
Storage-->>EvalOverviewService: dict[session_id, SessionFirstRequest]
EvalOverviewService->>Storage: get_citations_by_session_ids(session_ids)
Storage-->>EvalOverviewService: list[SessionCitation]
EvalOverviewService->>Braintrust: get_imported_scores(current 7d window)
EvalOverviewService->>Braintrust: get_imported_scores(prior 7d window)
alt include_shadow = True
EvalOverviewService->>Storage: get_recent_shadow_comparison_verdicts(from_ts, to_ts, judge_prompt_version, limit)
Storage-->>EvalOverviewService: list[ShadowComparisonVerdict]
end
EvalOverviewService-->>APIEndpoint: GetEvaluationOverviewResponse
sequenceDiagram
participant RegenJob as _build_sample_candidates
participant Storage
RegenJob->>Storage: get_first_requests_by_session_ids(all_session_ids)
alt bulk fetch succeeds
Storage-->>RegenJob: dict[session_id, SessionFirstRequest]
RegenJob->>RegenJob: populate SampleCandidate.created_at / first_request_source per session
else bulk fetch fails
Storage-->>RegenJob: exception
RegenJob->>RegenJob: warn + fallback to created_at=0, source=""
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 (1)
tests/server/services/storage/test_session_window_contract.py (1)
296-300: ⚡ Quick winUse
next(...)instead of building a list then indexing[0].This avoids an unnecessary list allocation and matches the project’s static-analysis recommendation.
Proposed change
- row = [ - r - for r in storage.get_agent_success_evaluation_results(limit=100) - if r.result_id == ids[0] - ][0] + row = next( + r + for r in storage.get_agent_success_evaluation_results(limit=100) + if r.result_id == ids[0] + )🤖 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/server/services/storage/test_session_window_contract.py` around lines 296 - 300, Replace the list comprehension with indexing `[0]` in the row variable assignment with the `next()` function. The current code builds a full list from the filtered results of storage.get_agent_success_evaluation_results and then takes the first element. Instead, wrap the generator expression (without the list brackets) in a next() call to efficiently retrieve just the first matching item where r.result_id equals ids[0], avoiding unnecessary list allocation.Source: Linters/SAST tools
🤖 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 `@reflexio/server/services/storage/sqlite_storage/_extras.py`:
- Around line 500-507: The SQL query in the method filters out NULL and empty
array citations using the conditions `AND i.citations IS NOT NULL AND
i.citations != '[]'`, but it does not filter out empty string citations which
can break the _json_loads parsing call. Add an additional WHERE clause condition
`AND i.citations != ''` to exclude empty string values from the query results,
ensuring that only valid non-empty citation strings are processed by the
_json_loads function in the subsequent loop.
In `@reflexio/server/services/storage/sqlite_storage/_playbook.py`:
- Around line 1359-1364: The SQL SELECT query on the
agent_success_evaluation_result table orders results only by created_at DESC,
which produces non-deterministic ordering when multiple rows share the same
timestamp. Fix this by adding result_id DESC as a secondary tie-breaker in the
ORDER BY clause after created_at DESC, so that when timestamps are equal, the
results are ordered consistently by result_id in descending order.
In `@reflexio/server/services/storage/sqlite_storage/_shadow_verdicts.py`:
- Around line 184-195: The limit parameter is passed directly to the SQLite
LIMIT clause without defensive validation, and negative values in SQLite LIMIT
can remove the cap entirely and trigger large reads. Before using limit in the
SQL query parameters tuple passed to _fetchall, clamp it to a safe minimum value
(such as 0 or a reasonable maximum) using similar max and min logic already
applied to from_ts and to_ts with _MAX_SAFE_EPOCH_TS, ensuring the storage layer
enforces safe boundaries regardless of what callers provide.
In `@reflexio/server/services/storage/storage_base/_playbook.py`:
- Around line 650-666: The methods in the base fallback implementation are using
hard-coded limits of 10_000 rows when calling
get_agent_success_evaluation_results, which silently truncates results and
violates the storage contract. In both the method filtering by time range and
get_agent_success_evaluation_result_ids, remove or modify the hard-coded limit
parameter to either paginate exhaustively through all available rows until no
more results are returned, or explicitly raise NotImplementedError to indicate
that the default fallback implementation cannot guarantee complete results for
backends that do not override these methods.
In `@reflexio/server/services/storage/storage_base/_requests.py`:
- Around line 168-174: The hardcoded top_k=1000 parameter in the get_sessions
method call can miss the true earliest request when a session contains more than
1,000 rows, causing SessionFirstRequest to return incorrect data. Replace the
fixed top_k=1000 with either a sufficiently large limit that captures all
possible rows in a session, or remove the top_k constraint entirely if the
get_sessions method supports retrieving all rows. Alternatively, if the method
supports sorting, request results sorted by created_at in ascending order and
take only the first row instead of filtering locally.
In `@reflexio/server/services/storage/storage_base/_shadow_verdicts.py`:
- Around line 116-124: The fetch_newest_shadow_comparison_verdicts method does
not validate the limit parameter before slicing. When limit is zero or negative,
Python's slicing behavior produces unexpected results instead of returning an
empty list. Add a guard clause at the beginning of the return statement that
checks if limit is less than or equal to zero and returns an empty list in that
case, otherwise proceed with the current slicing logic on the reversed verdicts.
In `@tests/server/services/evaluation_overview/test_service_integration.py`:
- Around line 371-375: Remove the strict wall-clock time assertion `assert
elapsed < 2.0` from the integration test as it is environment-sensitive and can
cause flakiness in CI environments. The existing assertions like `assert
response.hero.regular_success_rate_pp == 50.0` and
`storage.get_agent_success_evaluation_results_in_window.assert_called_once()`
are sufficient for verifying correct behavior in integration tests. Optionally
remove the `elapsed` variable calculation if it is no longer used elsewhere in
the test.
- Around line 42-60: The helper function _storage_with_results is mocking the
outdated method name get_shadow_comparison_verdicts, but the new storage
contract uses get_recent_shadow_comparison_verdicts. Update the mocked method
name in the storage mock within _storage_with_results from
get_shadow_comparison_verdicts to get_recent_shadow_comparison_verdicts to align
with the new storage contract. Additionally, apply the same fix to the
assertions at lines 328-345 that also reference the old method name to prevent
call-shape regressions.
---
Nitpick comments:
In `@tests/server/services/storage/test_session_window_contract.py`:
- Around line 296-300: Replace the list comprehension with indexing `[0]` in the
row variable assignment with the `next()` function. The current code builds a
full list from the filtered results of
storage.get_agent_success_evaluation_results and then takes the first element.
Instead, wrap the generator expression (without the list brackets) in a next()
call to efficiently retrieve just the first matching item where r.result_id
equals ids[0], avoiding unnecessary list allocation.
🪄 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: CHILL
Plan: Pro Plus
Run ID: dca05c92-3e96-408d-973a-331f218a7405
📒 Files selected for processing (19)
reflexio/models/api_schema/internal_schema.pyreflexio/server/api.pyreflexio/server/services/agent_success_evaluation/group_evaluation_runner.pyreflexio/server/services/agent_success_evaluation/regen_jobs.pyreflexio/server/services/evaluation_overview/service.pyreflexio/server/services/storage/sqlite_storage/_base.pyreflexio/server/services/storage/sqlite_storage/_extras.pyreflexio/server/services/storage/sqlite_storage/_playbook.pyreflexio/server/services/storage/sqlite_storage/_requests.pyreflexio/server/services/storage/sqlite_storage/_shadow_verdicts.pyreflexio/server/services/storage/storage_base/_extras.pyreflexio/server/services/storage/storage_base/_playbook.pyreflexio/server/services/storage/storage_base/_requests.pyreflexio/server/services/storage/storage_base/_shadow_verdicts.pytests/server/services/agent_success_evaluation/test_regen_jobs_sampling_integration.pytests/server/services/evaluation_overview/test_service_integration.pytests/server/services/storage/test_session_window_contract.pytests/server/services/storage/test_shadow_verdicts_contract_integration.pytests/server/services/storage/test_sqlite_storage_bc_extras.py
Comment on lines
+1359
to
+1364
| """SELECT result_id FROM agent_success_evaluation_result | ||
| WHERE session_id = ? | ||
| AND evaluation_name = ? | ||
| AND agent_version = ? | ||
| ORDER BY created_at DESC""", | ||
| (session_id, evaluation_name, agent_version), |
There was a problem hiding this comment.
Make targeted result-id ordering deterministic on timestamp ties.
Ordering only by created_at DESC is unstable when multiple rows share the same timestamp. Add result_id DESC as a tie-breaker so “latest” resolution is deterministic.
Proposed fix
- ORDER BY created_at DESC""",
+ ORDER BY created_at DESC, result_id DESC""",🤖 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 `@reflexio/server/services/storage/sqlite_storage/_playbook.py` around lines
1359 - 1364, The SQL SELECT query on the agent_success_evaluation_result table
orders results only by created_at DESC, which produces non-deterministic
ordering when multiple rows share the same timestamp. Fix this by adding
result_id DESC as a secondary tie-breaker in the ORDER BY clause after
created_at DESC, so that when timestamps are equal, the results are ordered
consistently by result_id in descending order.
Comment on lines
+650
to
+666
| rows = self.get_agent_success_evaluation_results( | ||
| limit=limit or 10_000, | ||
| agent_version=agent_version, | ||
| ) | ||
| return [r for r in rows if from_ts <= r.created_at <= to_ts] | ||
|
|
||
| def get_agent_success_evaluation_result_ids( | ||
| self, | ||
| session_id: str, | ||
| evaluation_name: str, | ||
| agent_version: str, | ||
| ) -> list[int]: | ||
| """Return result ids for one eval identity tuple.""" | ||
| rows = self.get_agent_success_evaluation_results( | ||
| limit=10_000, | ||
| agent_version=agent_version, | ||
| ) |
There was a problem hiding this comment.
Avoid silent truncation in base fallback bulk reads.
The new default implementations hard-cap reads at 10_000, which can silently miss valid rows for both window queries and targeted ID lookups. That breaks the storage contract semantics on non-overridden backends and can produce incorrect overview/regeneration behavior.
Suggested direction
- rows = self.get_agent_success_evaluation_results(
- limit=limit or 10_000,
- agent_version=agent_version,
- )
+ # Fallback must be complete, not capped; backends should override
+ # for efficient server-side filtering.
+ rows = self.get_agent_success_evaluation_results(
+ limit=limit if limit is not None else 1_000_000_000,
+ agent_version=agent_version,
+ )Consider either paginating exhaustively in the fallback or explicitly raising NotImplementedError for backends that cannot guarantee completeness.
📝 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.
Suggested change
| rows = self.get_agent_success_evaluation_results( | |
| limit=limit or 10_000, | |
| agent_version=agent_version, | |
| ) | |
| return [r for r in rows if from_ts <= r.created_at <= to_ts] | |
| def get_agent_success_evaluation_result_ids( | |
| self, | |
| session_id: str, | |
| evaluation_name: str, | |
| agent_version: str, | |
| ) -> list[int]: | |
| """Return result ids for one eval identity tuple.""" | |
| rows = self.get_agent_success_evaluation_results( | |
| limit=10_000, | |
| agent_version=agent_version, | |
| ) | |
| # Fallback must be complete, not capped; backends should override | |
| # for efficient server-side filtering. | |
| rows = self.get_agent_success_evaluation_results( | |
| limit=limit if limit is not None else 1_000_000_000, | |
| agent_version=agent_version, | |
| ) | |
| return [r for r in rows if from_ts <= r.created_at <= to_ts] | |
| def get_agent_success_evaluation_result_ids( | |
| self, | |
| session_id: str, | |
| evaluation_name: str, | |
| agent_version: str, | |
| ) -> list[int]: | |
| """Return result ids for one eval identity tuple.""" | |
| rows = self.get_agent_success_evaluation_results( | |
| limit=10_000, | |
| agent_version=agent_version, | |
| ) |
🤖 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 `@reflexio/server/services/storage/storage_base/_playbook.py` around lines 650
- 666, The methods in the base fallback implementation are using hard-coded
limits of 10_000 rows when calling get_agent_success_evaluation_results, which
silently truncates results and violates the storage contract. In both the method
filtering by time range and get_agent_success_evaluation_result_ids, remove or
modify the hard-coded limit parameter to either paginate exhaustively through
all available rows until no more results are returned, or explicitly raise
NotImplementedError to indicate that the default fallback implementation cannot
guarantee complete results for backends that do not override these methods.
Comment on lines
+42
to
+60
| def _storage_with_results( | ||
| results: list[AgentSuccessEvaluationResult], | ||
| ) -> MagicMock: | ||
| storage = MagicMock() | ||
| storage.get_agent_success_evaluation_results.return_value = [ | ||
| _eval_result(result_id=1, session_id="s1", is_success=True, corrections=0), | ||
| _eval_result(result_id=2, session_id="s2", is_success=True, corrections=1), | ||
| _eval_result(result_id=3, session_id="s3", is_success=False, corrections=3), | ||
| ] | ||
| storage.get_playbook_application_stats.return_value = [] | ||
| storage.get_interactions_by_session.return_value = [] | ||
| storage.org_id = "" | ||
| storage.get_agent_success_evaluation_results_in_window.return_value = results | ||
| storage.get_first_requests_by_session_ids.return_value = { | ||
| r.session_id: SessionFirstRequest( | ||
| session_id=r.session_id, | ||
| user_id="u1", | ||
| source="api", | ||
| created_at=r.created_at, | ||
| ) | ||
| for r in results | ||
| } | ||
| storage.get_citations_by_session_ids.return_value = [] | ||
| storage.get_imported_scores.return_value = [] | ||
| storage.get_shadow_comparison_verdicts.return_value = [] | ||
| return storage |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align mocked/asserted shadow API with the new targeted storage contract.
The helper and skip-shadow assertion still reference get_shadow_comparison_verdicts, but this PR’s storage contract introduces get_recent_shadow_comparison_verdicts. Keeping the old method in these assertions can let call-shape regressions slip through.
Proposed change
def _storage_with_results(
results: list[AgentSuccessEvaluationResult],
) -> MagicMock:
storage = MagicMock()
@@
- storage.get_shadow_comparison_verdicts.return_value = []
+ storage.get_recent_shadow_comparison_verdicts.return_value = []
return storage
@@
def test_service_skips_shadow_storage_when_shadow_excluded() -> None:
@@
- storage.get_shadow_comparison_verdicts.assert_not_called()
+ storage.get_recent_shadow_comparison_verdicts.assert_not_called()Also applies to: 328-345
🤖 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/server/services/evaluation_overview/test_service_integration.py` around
lines 42 - 60, The helper function _storage_with_results is mocking the outdated
method name get_shadow_comparison_verdicts, but the new storage contract uses
get_recent_shadow_comparison_verdicts. Update the mocked method name in the
storage mock within _storage_with_results from get_shadow_comparison_verdicts to
get_recent_shadow_comparison_verdicts to align with the new storage contract.
Additionally, apply the same fix to the assertions at lines 328-345 that also
reference the old method name to prevent call-shape regressions.
Paginate the base first-request fallback so sessions with more than 1000 requests still find the true earliest request. Clamp negative SQLite recent-verdict limits to avoid uncapped reads.
Ignore blank SQLite citation payloads before JSON parsing, guard base recent-shadow reads for non-positive limits, and remove an environment-sensitive wall-clock assertion from the overview integration test.
yilu331
added a commit
that referenced
this pull request
Jun 18, 2026
…172) ## Summary Found by `/check-and-test` (nightly CI). Two related fixes surfaced by the new evaluation-overview query added in #168. **Application fix — clamp `_epoch_to_iso` to a representable range:** - `_epoch_to_iso` raised `ValueError` when callers passed sentinel "open" window bounds (e.g. `to_ts=10**12`, used by the evaluation-overview API as a far-future upper bound) because `datetime.fromtimestamp` cannot represent year > 9999. - The new `get_agent_success_evaluation_results_in_window` query (#168) hit this and **500'd the evaluation overview endpoint on a fresh app**. - Clamp the input to `[0, 253402300799]` centrally in `_epoch_to_iso` so all current and future callers are protected. `_shadow_verdicts.py` had already worked around this at two call sites with a local `_MAX_SAFE_EPOCH_TS` clamp; that constant now lives in `_base.py` and the redundant per-call clamps are removed. **Test fix — stale drift from #168:** - The `force_regenerate` runner now captures prior result ids via the targeted `get_agent_success_evaluation_result_ids` lookup instead of reading full result rows. `test_group_evaluation_runner_regen.py` still mocked the old method, so the by-id delete asserted the wrong argument (MagicMock tests) or bound a `MagicMock` into SQL (real-sqlite tests). Seed the new lookup and patch `get_extractor_name` so the capture query's `evaluation_name` matches the seeded rows. ## Changes - `reflexio/services/storage/sqlite_storage/_base.py` — central clamp in `_epoch_to_iso`; host `_MAX_SAFE_EPOCH_TS`. - `reflexio/services/storage/sqlite_storage/_shadow_verdicts.py` — drop the now-redundant local clamps. - `tests/.../test_group_evaluation_runner_regen.py` — align mocks with the by-id lookup path. ## Test Plan - `test_group_evaluation_runner_regen.py` passes under both MagicMock and real-sqlite paths. - Evaluation-overview endpoint no longer 500s with an open far-future `to_ts` on a fresh app. ## Provenance Generated by the nightly AI-driven CI run on 2026-06-18. The enterprise-side submodule bump + companion test changes are in ReflexioAI/reflexio-enterprise#263 — that PR's `open_source/reflexio` pointer should be repointed to this PR's **merged** `main` commit once it lands. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Bug Fixes** * Fixed query failures when requesting results with extreme or boundary timestamp values by safely handling out-of-range epochs in SQLite-backed storage. * **Tests** * Improved coverage for agent success evaluation “force regenerate” flows, including correct identity matching and deletion/preservation behavior across both mocked and real SQLite scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jul 1, 2026
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
Changes
Test Plan
uv run ruff checkon touched OSS filesuv run pyrighton touched OSS filesuv run pytest --no-cov tests/server/services/evaluation_overview/test_service_integration.py tests/server/services/evaluation_overview/test_service_grouping_integration.py tests/server/services/storage/test_session_window_contract.py tests/server/services/storage/test_sqlite_storage_bc_extras.py tests/server/services/storage/test_shadow_verdicts_contract_integration.py tests/server/services/agent_success_evaluation/test_regen_jobs_sampling_integration.py/api/get_evaluation_overviewreturned 200 in ~0.28-0.33s after optimization.Summary by CodeRabbit