feat(space): maintain sessions.visible_message_count counter [#2336] - #2358
feat(space): maintain sessions.visible_message_count counter [#2336]#2358lsm wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Replace the correlated COUNT(*) over sdk_messages that spaceSessions.bySpace ran for every session on every 150ms-debounced re-evaluation (~92ms warm for dev-neokai) with a maintained sessions.visible_message_count column read directly. - schema: add visible_message_count INTEGER NOT NULL DEFAULT 0 to sessions - migration 170: ALTER + one-time backfill using the badge predicate (renumbered from 169 because dev shipped 169 for message_subtype_norm in #2346) - SDKMessageRepository: increment on visible inserts (saveSDKMessage, saveUserMessage, saveHyperNeoActionMessage); recompute affected sessions on send_status transitions, rewind deletes, and pending-message removal - spaceSessions.bySpace: select s.visible_message_count instead of the per-session subquery Counter maintenance no-ops gracefully when the column/table is absent (unit test harnesses), keeping the existing suite green. The visibility predicate (top-level rows, non-deferred user rows, non-hidden subtypes) is centralized in isVisibleBadgeRow so the maintained counter cannot drift from the predicate it replaces; a cross-check test asserts it equals a fresh COUNT(*) across a mixed sequence of inserts, status flips, and rewinds.
3afafb3 to
721c5cf
Compare
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (GLM / Zhipu)
Model: glm-5.1[1m] | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: REQUEST_CHANGES (one P3 nit; everything else is solid).
What I verified
- The perf fix lands correctly.
spaceSessions.bySpacenow readss.visible_message_countdirectly — the correlated per-sessionCOUNT(*)is gone. The new "messageCount is decoupled from sdk_messages" test proves adding message rows can no longer move the badge; only an explicit update to the maintained column does. - Predicate parity is exact across all three sites — INSERT (
isVisibleBadgeRow), RECOMPUTE (recomputeVisibleMessageCount), and the migration-170 backfill. I checked every dimension:parent_tool_use_id IS NULL, user rows gated onsend_status IN (consumed, failed)with NULL→consumed, subtype not inHIDDEN_SYSTEM_SUBTYPES + thinking_tokens, and non-user rows always visible. NULL/empty-string handling formessage_subtypeandsend_statusis identical in all three.messages.bySessionmatches on every badge-relevant dimension (its two extra filters are transcript-rendering only, intentionally excluded from the badge). - Every runtime
sdk_messagesmutation is covered. All writes funnel throughSDKMessageRepository; the 6 structural methods all maintain the counter (3 inserts bump, 2 rewinds + pending-delete + status-flip recompute). Session archive preserves messages (counter stays correct); hard-delete FK-cascades and the session row — and the counter — go together, so no drift. No clear/reset/import/fork bulk-insert path exists. - Transactions on the hot paths.
saveSDKMessage/saveUserMessagewrap insert + bump inthis.db.transaction(...)— atomic. ✓ - Migration is robust: idempotent (recompute-on-rerun), missing-table guards, correct backfill. Tested.
- No-op guard (
supportsVisibleMessageCount) makes maintenance a safe no-op for partial-schema test harnesses — good design. - Tests are comprehensive: visible/invisible inserts, status flips, action messages, rewind recompute, an anti-drift cross-check (counter == fresh
COUNT(*)) across a mixed sequence, plus migration backfill/idempotency/guards. Relocating the visibility-predicate coverage from the LQH test to the repo test is the right call.
Verification run locally
check:db-schema-parity ✅ · 4-space-storage shard 1893/1893 ✅ · live-query-handlers test 108/108 ✅ · new repo + migration-170 tests 128/128 ✅.
Findings
- P3 —
saveHyperNeoActionMessagecounter bump is outside a transaction (see anchored comment onsdk-message-repository.ts:1786). The other two save paths wrap insert+bump in a transaction; this one doesn't. Trivial one-line fix for consistency. The only thing keeping this from an approve.
Optional observation (not a finding, no change requested)
BADGE_HIDDEN_SUBTYPES (Set, insert path) and EXCLUDED_FROM_PAGINATION_SQL_LIST (SQL, recompute path) both encode HIDDEN_SYSTEM_SUBTYPES + 'thinking_tokens' ~20 lines apart. They're consistent today and the anti-drift test guards against divergence; this also matches the file's existing multi-list style, so I'm not asking for a change — just noting the two representations must stay in sync.
Summary
Clean, well-scoped implementation of exactly what the issue asked for, with thorough tests and correct predicate parity. Only the one P3 transaction-consistency nit on the action-message path — fix that and this is good to go.
…ction Match saveSDKMessage / saveUserMessage: wrap the INSERT and the visible_message_count bump in a single transaction so a failure between the two autocommit statements can't leave the counter under-counted. Addresses review P3 on #2358.
There was a problem hiding this comment.
lsm has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (GLM / Zhipu)
Model: glm-5.1[1m] | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: APPROVE — zero findings remain.
The P3 from the previous round is addressed correctly in dfa38a0: saveHyperNeoActionMessage now wraps the INSERT + bumpVisibleMessageCount in this.db.transaction(...), with the prepared statement hoisted and upsertMessageSearchRow left outside — exactly matching saveSDKMessage / saveUserMessage. Surgical change, no scope creep.
Re-verified this round:
- Fix is well-formed and consistent with the other two save paths.
- Repo suite 121/121 (incl. the action-message increment test) ✅.
4-space-storageshard 1893/1893 ✅.- PR is OPEN / MERGEABLE; the single review thread is resolved.
Everything from the prior review stands: the perf fix lands (spaceSessions.bySpace reads the maintained column, correlated per-session COUNT(*) removed), predicate parity is exact across insert/recompute/backfill (incl. NULL handling), every runtime sdk_messages mutation maintains the counter, and tests are comprehensive. Approving.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa38a031f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| -- 150ms debounce). SDKMessageRepository keeps this in sync with the same | ||
| -- visibility predicate (top-level rows, non-deferred user rows, non-hidden | ||
| -- subtypes) on every sdk_messages mutation. | ||
| s.visible_message_count as messageCount, |
There was a problem hiding this comment.
When normal message writes go through reactiveDb.db.saveSDKMessage / saveUserMessage / updateMessageStatus, createReactiveDatabase still emits only a sdk_messages table change for those facade methods, and LiveQueryEngine reevaluates subscriptions only for tables extracted from the SQL. After this change spaceSessions.bySpace is indexed under sessions/spaces and no longer under sdk_messages, while the counter update is performed inside SDKMessageRepository against the raw SQLite connection, so no sessions change event is emitted. In a running space, the sidebar badge will therefore stay stale after messages arrive or statuses flip until some unrelated session/space write happens; please also notify/invalidate sessions for visible-message-count maintenance or keep an explicit sdk_messages dependency.
Useful? React with 👍 / 👎.
| -- 150ms debounce). SDKMessageRepository keeps this in sync with the same | ||
| -- visibility predicate (top-level rows, non-deferred user rows, non-hidden | ||
| -- subtypes) on every sdk_messages mutation. | ||
| s.visible_message_count as messageCount, |
There was a problem hiding this comment.
P1 — reactivity regression: the badge query lost its sdk_messages dependency, and the counter write emits no sessions event.
This SELECT is what LiveQueryEngine uses to index the query. Old SQL had a correlated FROM sdk_messages subquery, so extractTables() yielded ["sdk_messages","sessions","spaces"] — every saveSDKMessage (which emits a sdk_messages change per METHOD_TABLE_MAP) re-evaluated the badge live. Verified: the new SQL extracts only ["sessions","spaces"] — the sdk_messages dependency is gone.
Meanwhile the counter is maintained by raw UPDATE sessions SET visible_message_count=… inside SDKMessageRepository.bumpVisibleMessageCount/recomputeVisibleMessageCount. The reactive proxy emits table events from the method name (METHOD_TABLE_MAP), not from the SQL, and prepare().run() is bound to the raw target — so that raw sessions write emits no sessions change.
Net: a counter-changing event no longer causes spaceSessions.bySpace to re-evaluate. The badge now updates only incidentally when some unrelated reactive updateSession (e.g. a processing_state transition) happens to fire — which is unreliable and misses action-message inserts and post-hoc status flips entirely. This breaks the live unread badge, which is the whole point of the change.
Fix (matches the codebase pattern — every other repo calls notifyChange after raw writes; see state-projection-service.ts:285/308 for notifyChange(sessions) specifically): thread reactiveDb into SDKMessageRepository (like task/skill/provider repos) and call this.reactiveDb.notifyChange(sessions) after bumpVisibleMessageCount / recomputeVisibleMessageCount. This restores live updates without giving up the perf win — the query stays a cheap column read; only the re-eval frequency is restored (still 150ms-debounced).
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1[1m] (GLM / Zhipu)
Model: glm-5.1[1m] | Client: NeoKai | Provider: GLM (Zhipu)
Recommendation: REQUEST_CHANGES — supersedes my earlier APPROVE. A reactivity regression surfaced (raised by @chatgpt-codex-connector and confirmed on re-investigation) that breaks the live unread badge — the PR's central purpose. The earlier P3 (transaction wrap) is fixed and fine; this is a separate, more serious issue I missed in both prior rounds.
P1 — badge query lost its sdk_messages dependency, and the counter write emits no sessions event
How the badge stayed live before. LiveQueryEngine derives each query's table dependencies from extractTables(sql) and re-evaluates only on a change to one of those tables (live-query.ts:306, 402). The old spaceSessions.bySpace SQL had a correlated FROM sdk_messages subquery, so its deps were ["sdk_messages","sessions","spaces"]. Every saveSDKMessage emits a sdk_messages change (reactive-database.ts METHOD_TABLE_MAP), which re-evaluated the badge — that was the live-update path.
What changed. I ran extractTables on both SQL strings:
- OLD deps:
["sdk_messages","sessions","spaces"] - NEW deps:
["sessions","spaces"]←sdk_messagesis gone.
So a sdk_messages change no longer re-evaluates spaceSessions.bySpace. And the maintained counter is updated by raw UPDATE sessions SET visible_message_count=… inside SDKMessageRepository.bumpVisibleMessageCount / recomputeVisibleMessageCount. The reactive proxy emits table events from the method name (METHOD_TABLE_MAP), not from the SQL — prepare().run() binds to the raw target and emits nothing. So that sessions write emits no sessions change either.
Net effect. A counter-changing event no longer triggers badge re-evaluation. The badge now refreshes only incidentally when an unrelated reactive updateSession (e.g. a processing_state transition around a turn) happens to fire. That is unreliable and demonstrably misses cases the old code handled:
saveHyperNeoActionMessage— action-message inserts with no coincident session-row write.updateMessageStatusflips (deferred→consumed) after a turn ends.- the tail of a turn once
processing_statehas gone idle.
This violates the codebase's own reactivity pattern. Every other repository that writes raw SQL calls reactiveDb.notifyChange('<table>') afterwards (task, skill, provider, mcp-enablement, app-mcp-server, space-task, goal, …). state-projection-service.ts:285/308 even calls notifyChange('sessions') for its raw UPDATE sessions SET last_error. The counter maintenance is the one raw sessions write that skips it.
Suggested fix (preserves the perf win)
Thread reactiveDb into SDKMessageRepository (as task/skill/provider repos already do) and call this.reactiveDb.notifyChange('sessions') after bumpVisibleMessageCount / recomputeVisibleMessageCount. This restores the live re-eval without re-introducing the correlated COUNT(*): the query stays a cheap column read, and re-evaluation frequency returns to what it was (still 150ms-debounced via DEBOUNCE_SPACE_SESSIONS_MS).
A test for this should assert that a saveSDKMessage on a space session causes spaceSessions.bySpace to re-evaluate (e.g. via the LiveQueryEngine change event / a subscriber delta), not just that the column is updated — the repo-level counter tests pass today but don't cover the reactivity gap.
Standing
Everything else from prior rounds holds (predicate parity across insert/recompute/backfill; all mutation paths covered; migration idempotent; the P3 transaction wrap is now in). Only this P1 blocks.
|
To use Codex here, create an environment for this repo. |
1 participant
Replaces the correlated
COUNT(*)oversdk_messagesthatspaceSessions.bySpaceran for every session on every 150ms-debounced re-evaluation (~92ms warm for dev-neokai) with a maintainedsessions.visible_message_countcolumn that the query reads directly.What changed
visible_message_count INTEGER NOT NULL DEFAULT 0onsessions.ALTER TABLE+ one-time backfill using the badge visibility predicate. (Renumbered from 169 — dev shipped 169 for themessage_subtype_normgenerated column in perf(daemon): make sdk_messages subtype filters sargable (#2330) #2346, which this branch rebased onto.)SDKMessageRepository: increments on visible inserts (saveSDKMessage/saveUserMessage/saveHyperNeoActionMessage); recomputes affected sessions onsend_statustransitions, rewind deletes (deleteMessagesAt/After), and pending-message removal. The visibility predicate (top-level rows, non-deferred user rows, non-hidden subtypes) is centralized inisVisibleBadgeRowso the counter cannot drift from the predicate it replaces.spaceSessions.bySpace: selectss.visible_message_countinstead of the per-session subquery.Maintenance no-ops gracefully when the column/table is absent, so unit-test harnesses with a partial schema are unaffected.
Verification
spaceSessions.bySpaceno longer runs a per-sessionCOUNT(*)(test asserts the badge is decoupled fromsdk_messages).send_statusflips, action messages, rewind recompute, and an anti-drift cross-check (counter == freshCOUNT(*)) across a mixed sequence.bun run check(lint/typecheck/knip/db-schema-parity/session-guards) green; storage/agent/session tests pass.Note:
check:test-qualityflags one pre-existing issue inprovider-registry.test.ts(unmodified by this PR) — not introduced here. One unrelated flaky test (model-service-provider-routing) times out intermittently regardless of this change.