perf: materialize AI Bridge sessions for list query - #27996
perf: materialize AI Bridge sessions for list query#27996evgeniy-scherbina wants to merge 5 commits into
Conversation
e16d462 to
5325657
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 9 findings (1 P0, 3 P1, 1 P2, 2 P3, 2 Note), COMMENT. Review Finding inventoryFinding inventory: PR #27996Findings
Law analysis
Round logRound 1Netero + Law (mandatory split). No panel. 1 P0, 3 P1, 1 P2, 2 P3, 2 Note. Reviewed against 1c993c7..a90109e. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This is a well-motivated perf change: materializing sessions into a trigger-maintained table with GIN indexes is the right shape for the list query's cost problem, the migration comments are unusually thorough, and the backfill correctly avoids the prompt fan-out distortion the old lateral existed to prevent.
This is a first-pass review only: mechanical findings from Netero plus a PR-decomposition analysis from Law. The full review panel has not yet reviewed this PR and will review after these findings are addressed.
Severity count: 1 P0, 3 P1, 1 P2, 2 P3, 2 Notes.
Split required (Law, mandatory). scripts/seed/ and scripts/sqldebug/ are +834 LOC, 78% of the effective diff, independent of the feature, and you have already stated they will not merge. Remove them from this branch now rather than "later": reviews, CI, and the merge decision run against what is in the branch, and the remaining ~230-LOC change (migration + query rewrite + codegen) deserves an undiluted diff. As Law put it: "'Will be removed later before merging' is a promise, not a state of the diff."
The P0 explains the sqlc-vet CI failure: migration 000562 collides with 000562_oauth2_public_client_tokens already on main. The three P1s are semantic: the prompt trigger silently drops timestamps for the normal production flow (prompt recorded before the interception ends), retention purge orphans session rows and then breaks the list endpoint, and CountAIBridgeSessions was left on the old table so list and count disagree under filters.
coderd/database/queries/aibridge.sql:301
P1 [CRF-4] CountAIBridgeSessions was not migrated to aibridge_sessions; list and count disagree whenever filters are set, and count keeps the full-scan cost this PR exists to remove. (Netero)
Reproduced: one session with interceptions (anthropic, claude) and (openai, gpt-4); filters provider=anthropic, model=gpt-4 yield list=1, count=0. The same divergence exists for the time-window filters (span overlap vs per-interception
started_at) and the client filter (stored scalar vs any interception).
The API returns both Sessions and Count from the same request, so pagination totals will contradict the visible rows. Count also still does COUNT(DISTINCT ...) over every interception on every request. Fix: count from aibridge_sessions with the identical filter block.
🤖
🤖 This review was automatically generated with Coder Agents.
| @@ -0,0 +1,165 @@ | |||
| -- Materializes AI Bridge sessions so the sessions list can order and filter | |||
There was a problem hiding this comment.
P0 [CRF-1] Migration number 000562 collides with 000562_oauth2_public_client_tokens already on origin/main; the merged tree cannot migrate. (Netero)
Verified by copying main's 000562 files into the worktree and running
migrations.Up: panics withduplicate migration file: 000562_oauth2_public_client_tokens.down.sql. This is why thesqlc-vetCI job fails (CI runs against the PR merged with main;sqlc veton the PR tree alone passes locally, verified).
Main is now at 000566. Renumber via coderd/database/migrations/fix_migration_numbers.sh and rerun make gen.
🤖
| SET last_active_at = GREATEST(s.last_active_at, NEW.created_at) | ||
| FROM aibridge_interceptions ai | ||
| WHERE ai.id = NEW.interception_id | ||
| AND s.session_id = ai.session_id |
There was a problem hiding this comment.
P1 [CRF-2] Prompts recorded before their interception ends never advance last_active_at, so the sort key is wrong for the normal production flow. (Netero)
Production order is
RecordPromptUsage(inserts the prompt) during the interception, thenRecordInterceptionEnded(setsended_at):coderd/aibridgedserver/aibridgedserver.go:481and:311. For a session's first interception, the prompt trigger's UPDATE matches noaibridge_sessionsrow (the row is only created when an interception completes), so the prompt timestamp is silently dropped. The interception trigger then seedslast_active_atfromstarted_at; itsGREATESTonly ever comparesstarted_atvalues and never sees the prompt'screated_at.
Reproduced empirically by Netero: interception in-flight, prompt at 00:05, interception ended at 00:06 yields last_active_at = 00:00 (started_at), where the old query returned 00:05. The trigger comment claiming the interception trigger's GREATEST "keeps whichever timestamp is later" is false. Every single-interception session, and the first interception of every session, sorts by start time instead of prompt time, diverging from the column's own documented semantics and from pre-PR behavior.
🤖
| @@ -488,13 +470,21 @@ FROM | |||
| JOIN | |||
There was a problem hiding this comment.
P1 [CRF-3] Retention purge orphans aibridge_sessions rows, and the list query then fails to scan NULL aggregates into non-nullable Go fields. (Netero)
DeleteOldAIBridgeRecordsdeletes interceptions but nothing deletes the correspondingaibridge_sessionsrow: no DELETE trigger exists and the purge CTE does not touch the table. Reproduced: after deleting a session's interceptions, the session row survives and the page lateral'sMIN(ai.started_at)/MAX(ai.ended_at)return NULL.
ListAIBridgeSessionsRow.StartedAt/EndedAt are non-nullable time.Time, so scanning NULL fails and the entire ListAIBridgeSessions call errors; the sessions list endpoint breaks once retention purges any session still on a page. Fix: delete session rows whose interceptions are purged (a DELETE trigger or an extra CTE in DeleteOldAIBridgeRecords).
🤖
| @@ -0,0 +1,165 @@ | |||
| -- Materializes AI Bridge sessions so the sessions list can order and filter | |||
There was a problem hiding this comment.
P2 [CRF-5] 165 lines of trigger, upsert, and backfill logic plus rewritten list-query semantics ship with zero new tests (diff test density 0.0%). (Netero)
The two proven bugs above (prompt ordering, purge orphans) are exactly the paths no test exercises:
TestAIBridgeListSessionsinserts data in an order that avoids the prompt race, and no test covers purge interaction or the backfill.
At minimum: a test inserting a prompt before UpdateAIBridgeInterceptionEnded, a test running DeleteOldAIBridgeRecords then listing, and a backfill assertion in the migration fixtures.
🤖
| -- or, for interceptions recorded after the fact, on insert. Restricting the | ||
| -- update case to ended_at keeps later updates such as credential_hint from | ||
| -- re-running the upsert. | ||
| CREATE TRIGGER aibridge_interceptions_track_session |
There was a problem hiding this comment.
P3 [CRF-6] The trigger, the backfill, and the display query disagree on which client a session has. (Netero)
The trigger keeps the first non-NULL client (
COALESCE(existing, EXCLUDED)); the backfill and the outer display lateral keep the first client ordered bystarted_at, id, which can be NULL even when later interceptions have one.
A session whose first interception has NULL client filters as client=X (trigger-maintained) but displays as empty, and the same session backfilled stores NULL and does not match the filter. The old filter matched any interception's client. Pick one rule and use it in all three places.
🤖
| @@ -0,0 +1,390 @@ | |||
| // Seed aibridge interceptions to stress-test ListAIBridgeSessions. | |||
There was a problem hiding this comment.
P3 [CRF-7] 834 LOC of debug tooling (scripts/seed, scripts/sqldebug) are in the diff. (Netero P3, Law mandatory split)
They compile (
go buildverified) and are mechanically clean, but a removal promise is not a removal.
Law's split verdict (mandatory, see review body) requires removing these from the branch now, not at merge time. They are 78% of the effective diff and independent of the feature; git holds the files if a follow-up tooling PR is wanted.
🤖
| -- Filter by time frame | ||
| AND CASE | ||
| WHEN @started_after::timestamptz != '0001-01-01 00:00:00+00'::timestamptz THEN ai.started_at >= @started_after::timestamptz | ||
| -- Filter by time frame. A session matches when its interceptions |
There was a problem hiding this comment.
Note [CRF-8] The time-window filter semantics changed from "some interception started inside the window" to "the session's [first, last] start range overlaps the window". (Netero)
The migration comment documents this as intentional, but it is a user-visible behavior change (a session spanning the window now matches even if no interception started inside it) and the PR description only claims a perf change.
Worth an explicit callout in the PR description for API consumers.
🤖
|
|
||
| -- Backfills existing sessions. Uses a plain LEFT JOIN rather than the lateral | ||
| -- the old query needed: there is no COUNT here, so fan-out from multiple | ||
| -- prompts per interception cannot distort the MIN/MAX aggregates. |
There was a problem hiding this comment.
Note [CRF-9] The backfill runs inside the migration transaction as a full GROUP BY over aibridge_interceptions. (Netero)
The PR's own seed target is 2M rows; deployments of that size hold the migration (and startup) for the duration of the scan.
One-time and probably acceptable, but the operator should know the upgrade cost scales with interception count.
🤖
I need initial review on this PR. NOTE: scripts folder is only for debugging, will be removed later before merging this PR.