fix(session-ingest): shrink cli_sessions_v2 status row lock#4322
Open
RSO wants to merge 3 commits into
Open
Conversation
Reduce per-session row lock hold time on cli_sessions_v2 by removing the multi-step JS transaction around status metadata. Status transitions now run as a single SELECT ... FOR UPDATE + UPDATE ... RETURNING CTE, no-op status writes no longer stamp timestamps or lock the row, and parent and non-status metadata updates no longer execute while the status lock is held.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe rewrite consolidates the metadata, parent, and status writes into a single atomic Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 830a27d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 830a27d)Status: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Previous review (commit 2a81e84)Status: 1 Issue Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
Files Reviewed (3 files)
Reviewed by claude-sonnet-5 · Input: 36 · Output: 15.9K · Cached: 948.7K Review guidance: REVIEW.md from base branch |
…ction applyMetadataChanges split persistence into three independently-committed statements. If a later statement (e.g. the status CTE) failed after an earlier one committed, Postgres was left permanently out of sync: the queue consumer's DO only re-emits metadata changes that differ from its last-sent value, so a retry after partial failure could arrive with an empty/partial changes list. Wrap the three writes in a single db.transaction so they commit or roll back together, while keeping each write as one raw-SQL statement - updateSessionStatus still uses a single WITH locked AS (SELECT ... FOR UPDATE) ... UPDATE ... RETURNING statement, just run via tx.execute instead of db.execute. This preserves the PR's goal of only holding the cli_sessions_v2 row lock for that single statement, not across multiple round trips.
…tement Replaces the db.transaction wrapping the metadata/parent/status writes with a single UPDATE ... RETURNING guarded by a SELECT ... FOR UPDATE CTE. One statement is inherently atomic (no torn subset can commit if a later part fails) yet holds the row lock for exactly one round trip, never across a parent lookup, a second write, or a final read -- resolving both review findings without reintroducing cross-statement lock holding. Parent FK validation is now a plain unlocked read before the write, so it never extends the locked critical section. No-op status re-sends still take no lock and bump no timestamps, and the notification semantics (previous status from the locked row) are unchanged.
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
Reduces per-session row-lock hold time on
cli_sessions_v2, the second major lock contributor identified in production Supabase lock-wait logs (theselect "status" from "cli_sessions_v2" ... for updatequery). Over a representative 7-day window this exact query accounted for 1,440 acquired lock waits, dominated byShareLock-on-transaction waits (max 110s) that the earlierExclusiveLock-only investigation missed.The previous
applyMetadataChangesflow opened a JS transaction, tookSELECT ... FOR UPDATEearly, then held that row lock across a metadataUPDATE, a parent-session lookup + update, and a final full-rowSELECTbefore commit. This change shrinks the locked critical section:WITH locked AS (SELECT ... FOR UPDATE)CTE plusUPDATE ... RETURNING, returning both the locked previous status and the updated row. No multi-round-trip JS transaction holds the lock.status_updated_at/updated_at, no longer hold the lock through a write, and emit no status notification.UPDATE ... RETURNINGstatements.RETURNINGinstead of a final in-transactionSELECT.Locked-status notification semantics are preserved:
session.status.updatedstill reports the persisted previous status captured under the row lock, not the queued intake snapshot.Verification
Visual Changes
N/A
Reviewer Notes
db.execute(sql\...`)) because Drizzle's query builder can't express "return the locked previous value plus the updated row" in one statement.previous_statusis read via a CTE subquery inRETURNINGrather than relying onFROM`-alias visibility, for cross-version safety.updated_at/status_updated_atand emits no notification. This is intended (it was previously incidental lock-holding work), but flagging in case any consumer relied on the timestamp bump.