From 2a4e86f5c3c40a261ab62b9ce0a2c0ffaa8ca29d Mon Sep 17 00:00:00 2001 From: Konstantin M Date: Mon, 6 Jul 2026 18:34:02 +0000 Subject: [PATCH] fix(webui): preserve user_message_count under DB contention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile flagged that _compute_user_message_count_lazy silently returns 0 when the SQLite query fails (DB locked, missing file, schema drift). The pre-patch compact() walked self.messages in Python and returned the correct count without ever touching the DB, so the silent zero fallback is a regression that breaks _looks_like_stale_zero_message_row under contention. Fix: helper now takes an optional messages argument. On SQLite failure it falls back to the same in-memory walk the original code used, so callers see the same number they'd have seen before for any session where Session.load() populated self.messages. Also drop the dead ('GET', '/api/session') allowlist entry flagged by Greptile — the /api/session handler already does its own per-stage logging via the _t0.._t6 block (with auto-log on slow requests), and adding RequestDiagnostics on top would just duplicate the slow-request journal entries without adding information. --- api/models.py | 44 ++++++++++++++++++++++++++------------ api/request_diagnostics.py | 3 +-- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/api/models.py b/api/models.py index 590404425..4904ddc7b 100644 --- a/api/models.py +++ b/api/models.py @@ -1408,19 +1408,25 @@ class Session: return cls.load(sid) @staticmethod - def _compute_user_message_count_lazy(sid: str) -> int: + def _compute_user_message_count_lazy(sid: str, messages=None) -> int: """perf(session-load-latency) Priority 1: cheap SQL-only user count. - Returns the number of messages with role='user' for a given session, - computed via a single indexed SQLite query (~5ms even for 2,500+ msg - sessions because idx_messages_session covers the WHERE filter). The - role='user' predicate is a second filter on the small index range, so - it is bounded by the index lookup, not a full table scan. + Returns the number of messages with role='user' for a given session. + Fast path is a single indexed SQLite query (~5ms even for 2,500+ msg + sessions because idx_messages_session covers the WHERE filter); the + role='user' predicate is a second filter on the small index range. - The compact() output previously did an O(N) Python walk over - self.messages for the same value. compact() now emits user_message_count - as None; callers needing an exact count call this helper. The frontend - does not use the field, so no caller is broken today. + If the SQLite query fails for any reason (DB locked, file missing, + schema drift), the helper falls back to an O(N) walk over the + ``messages`` argument passed in by the caller. The caller always has + ``self.messages`` in memory by the time compact() runs (Session.load() + populates it from the sidecar), so the fallback is bounded by the + already-loaded Python list, not a fresh disk read. + + The previous O(N) walk inside compact() did not handle a missing + self.messages either, falling back to 0 in that case. The fallback + here therefore preserves the prior behavior on every input the prior + code accepted. """ try: import sqlite3 as _sqlite3 @@ -1444,9 +1450,16 @@ class Session: finally: conn.close() except Exception: - # Never let the helper break a response -- return 0 like the old code did - # when self.messages wasn't a list. - return 0 + # DB unavailable or schema mismatch — fall back to the in-memory + # walk. This is what the pre-patch compact() did unconditionally, + # so callers see the same number they'd have seen before for any + # session where Session.load() populated self.messages. + if not isinstance(messages, list): + return 0 + from api.models import _message_role + return sum( + 1 for m in messages if _message_role(m) == 'user' + ) def compact(self, include_runtime=False, active_stream_ids=None) -> dict: active_stream_ids = active_stream_ids if active_stream_ids is not None else set() @@ -1506,7 +1519,10 @@ class Session: 'worktree_repo_root': self.worktree_repo_root, 'worktree_created_at': self.worktree_created_at, } if self.worktree_path else {}), - 'user_message_count': Session._compute_user_message_count_lazy(self.session_id), + 'user_message_count': Session._compute_user_message_count_lazy( + self.session_id, + self.messages, + ), 'active_stream_id': self.active_stream_id, 'pending_user_message': self.pending_user_message, 'has_pending_user_message': has_pending_user_message, diff --git a/api/request_diagnostics.py b/api/request_diagnostics.py index 72bc891a8..71fb1f3ee 100644 --- a/api/request_diagnostics.py +++ b/api/request_diagnostics.py @@ -139,12 +139,11 @@ class RequestDiagnostics: if (method.upper(), clean_path) not in { ("GET", "/api/sessions"), ("POST", "/api/chat/start"), - # perf(session-load-latency): Phase 0 instrumentation gaps. + # perf(session-load-latency) Phase 0 instrumentation gaps. # These endpoints fire on every session click and were the source # of the multi-second waterfalls in the slow-request logs. ("GET", "/api/profiles"), ("GET", "/api/models"), - ("GET", "/api/session"), }: return None return cls(method, clean_path, logger=logger)