feat(replay-vision): add ScannerCandidateQuery for Phase 2 sweep#60617
Merged
Conversation
TueHaulund
requested review from
a team,
arnohillen,
fasyy612,
ksvat and
nicowaltz
and removed request for
a team
Contributor
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
products/replay_vision/backend/queries/scanner_candidate_query.py:76-77
`candidate_limit` is validated as positive, but `max_execution_time_seconds` has no equivalent check. Passing `0` is silently forwarded to ClickHouse, which interprets `max_execution_time=0` as *no limit* — the opposite of the protection the comment describes. A negative value would also pass through without error.
```suggestion
if candidate_limit <= 0:
raise ValueError(f"candidate_limit must be positive, got {candidate_limit}")
if max_execution_time_seconds <= 0:
raise ValueError(f"max_execution_time_seconds must be positive, got {max_execution_time_seconds}")
```
Reviews (1): Last reviewed commit: "fix(replay-vision): address ScannerCandi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
2 tasks
Lays the foundation for per-scanner Temporal schedules. Wraps SessionRecordingListFromQuery with a session_end-based filter: a session is eligible when it's had no activity in the last 35 minutes and its end time is past the scanner's watermark. The wrap delegates all RecordingsQuery filter compilation to the recordings list, so a scanner's saved filters resolve identically to the UI.
- Bump _PARTITION_LOOKBACK from 6h to 26h, anchored to posthog-js's 24h session_id rotation cap + 2h skew/lag headroom. Adds regression test for long-running sessions whose start is older than 6h. - Add keyset pagination via last_seen_session_id kwarg + lexicographic tuple comparison. Lets the schedule resume past a saturated batch without skipping sessions tied at the boundary microsecond. - Drop the now kwarg; use datetime.now(dt.UTC) directly so inner and outer clocks always agree under @freeze_time. - Push sampling into the inner HAVING via extra_having_predicates so un-sampled sessions are dropped before being aggregated by the outer. - Validate max_execution_time_seconds > 0. - Comment on inner.order_by mutation noting get_query() re-parses each call.
Drop multiline blocks, keep at most one sentence per comment, remove plan/phase prose.
TueHaulund
force-pushed
the
tue/replay-vision-scanner-candidate-query
branch
from
93698c0 to
f7c0345
Compare
Closes a DoS vector flagged in review: a client sending events with session_ids longer than 128 chars (the MAX_SESSION_ID_LENGTH used by the ApplyScannerWorkflow wire payload) would wedge the sweep on Pydantic validation. Filtering at the query layer keeps over-length sessions invisible to the scanner so the watermark always advances.
arnohillen
approved these changes
Jun 1, 2026
MattPua
pushed a commit
that referenced
this pull request
Jun 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
2 participants
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.
Problem
Phase 2 of Replay Vision needs per-scanner Temporal schedules that find sessions to apply scanners to. This PR adds the query that does the finding. No Temporal yet — that's the next PR.
Changes
ScannerCandidateQuerywrapsSessionRecordingListFromQuerywith asession_end-based filter: a session is eligible when it's had no activity in the last 35 minutes and its end time is past the scanner's watermark. Filter compilation is delegated to the recordings list so a scanner's savedRecordingsQueryresolves identically to the UI. ORDER BYsession_end ASClets the schedule advance its watermark monotonically.Also adds
Product.REPLAY_VISIONtoquery_tagging.pyforsystem.query_logattribution.How did you test this code?
I'm an agent. 36 automated tests pass — construction-time sanitization, sampling AST, and a ClickHouse integration suite covering watermark/settle bounds, sampling determinism,
$librouting, recording-metric HAVING, test-account exclusion, retention expiry, and two regression tests for sessions straddling the watermark / still settling. No manual testing.Automatic notifications
🤖 Agent context
Agent: Claude (Claude Code). Worth flagging: the 6-hour
_PARTITION_LOOKBACKon the innerdate_from. It's a perf-only bound, sized above Vision's 1-hour active-seconds cap — a session with >6h wall-clock idle gap whose truesession_endlands just past the watermark would be missed. Acceptable for v1 in my read.