Skip to content

fix: audit findings — index the polled panel query, stabilise its trigger, unify the terminal rule, report every live fan-out - #272

Merged
saucam merged 1 commit into
mainfrom
feat/panel-state-ui
Jul 30, 2026
Merged

fix: audit findings — index the polled panel query, stabilise its trigger, unify the terminal rule, report every live fan-out#272
saucam merged 1 commit into
mainfrom
feat/panel-state-ui

Conversation

@saucam

@saucam saucam commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

A second full audit of the collaborative-sessions feature, after the panel UI landed in #271. Four findings, all in code #271 introduced. I'd intended to fold them into that PR, but it was squash-merged before this push landed, so they arrive as their own.

Net delta vs main: 8 files, +247/−65.


HIGH — the panel poll scanned the tenant's entire dispatch history

dispatchRecentGroups filters on created_by, and no index covered it. Measured over 20k tasks rather than guessed:

before  SEARCH dispatch_tasks USING INDEX idx_dispatch_tenant (account_id=? AND project_id=?)
        17.84 ms per call
after   SEARCH dispatch_tasks USING INDEX idx_dispatch_panels (account_id=? AND project_id=? AND created_by=?)
         0.091 ms per call   — 196×

That query runs on a 3-second timer for as long as a collaboration is focused, and its cost scaled with the tenant's total task history rather than with the number of panels — so it got worse the longer a daemon lived.

Fixed with a partial index on (account_id, project_id, created_by, created_at DESC) WHERE group_id IS NOT NULL. Partial so it stays small: only grouped rows are ever read this way.

Pinned by a test that asserts the query plan, deliberately not a duration — a timing assertion in CI is a flake generator.

HIGH — the poll re-fired on every session.info_update, not every 3 s

The effect derived its goal id by reading focusedSession()?.collaboration, which subscribed it to that store property. mergeSession reassigns every field on each info_update with no equality guard (unlike ingestSessionList, which has jsonEqual), so collaboration got a fresh object reference on every status flip. The effect then tore down and rebuilt its interval, and each rebuild fired an immediate poll.

The two findings multiplied: during a tool-heavy turn this produced a burst of the 17.8 ms query above on the daemon's event loop.

Now a createMemo returning a plain string — a memo only notifies when the value actually changes, so a fresh collaboration object with the same id is inert.

MEDIUM — the terminal-status rule was defined twice

TERMINAL in dispatch.ts drove the barrier; a second literal TERMINAL_TASK_STATUS in session-manager.ts drove the panel UI's settled. Correctness-critical duplication: add a status and the sidebar would quietly disagree with the barrier about whether a fan-out had finished. One exported definition now, imported by both.

MEDIUM — only one live fan-out was reported

Concurrent panels are legal — an orchestrator can start a second while the first resolves, which is precisely why the dispatcher watches a set of tasks per session. livePanel() returned the first unjoined one, so a child participating only in the other live panel showed no badge, and the progress bar described one of two.

livePanels() returns all of them, liveProgress() sums across them (one panel's "1/3" misstates the outstanding work when two are running), and livePanelMember searches every live fan-out. The chip reads ⇉ panels ×2 when there's more than one.


Verification

suite before after
daemon 2200 2202
web 382 387

biome clean; both typechecks clean; web eslint 30 warnings / 0 errors (unchanged); bun run build OK. Re-verified after rebasing onto main at e49cf5f, not only where it was authored.

All four mutation-checked — index removed, terminal rule diverged, member lookup narrowed to the newest panel, progress reporting one panel instead of the sum. Each fails exactly the test written for it.

Two things the tests themselves taught me

  • The new index makes group_id undroppable, which broke the migration tests' artificial downgrade() helper. The real migration only ever adds columns, so the fix belonged in the helper — the drop ordering is an artifact of simulating a downgrade.
  • My index-coverage block used it( in a file that imports test(. It threw ReferenceError: it is not defined and the entire describe was skipped while the suite still reported green. Caught only because the pass count didn't move: 21 → 21 → 23 once fixed.

🤖 Generated with Claude Code

…gger, unify the terminal rule, report every live fan-out

A second full audit of the feature, after the panel UI landed. Four findings,
all in code this PR introduced, so they belong in it rather than in a
follow-up that ships a known-slow poll.

## HIGH — the panel poll scanned the tenant's entire dispatch history

`dispatchRecentGroups` filters on `created_by`, and no index covered it. The
plan used `idx_dispatch_tenant` for (account, project) and then filtered
every remaining row, plus three temp B-trees. Measured over 20k tasks:

    EXPLAIN: SEARCH ... USING INDEX idx_dispatch_tenant (account_id=? AND project_id=?)
    17.84 ms per call

That is on a 3-second timer for as long as a collaboration is focused, and
it grows with the tenant's total task history rather than with the number of
panels — so it gets worse the longer a daemon lives.

A partial index on (account_id, project_id, created_by, created_at DESC)
WHERE group_id IS NOT NULL. Partial so it stays small: only grouped rows are
ever read this way.

    EXPLAIN: SEARCH ... USING INDEX idx_dispatch_panels (account_id=? AND project_id=? AND created_by=?)
    0.091 ms per call — 196x

Pinned by a test that asserts the PLAN, not a duration: a timing assertion in
CI is a flake generator.

## HIGH — the poll re-fired on every session.info_update, not every 3s

The effect read `focusedSession()?.collaboration` to derive the goal id,
which subscribed it to that store property — and `mergeSession` reassigns
every field on each `info_update` with no equality guard (unlike
`ingestSessionList`, which has one). So `collaboration` got a fresh object
reference on every status flip, the effect tore down and rebuilt its
interval, and each rebuild fired an immediate poll.

During a tool-heavy turn that turned a 3s cadence into a burst — and each
one of those was the 17.8ms query above. The two findings multiplied.

Now a `createMemo` returning a plain string: a memo only notifies when the
value actually changes, so a fresh `collaboration` object with the same id is
inert.

## MEDIUM — the terminal-status rule was defined twice

`TERMINAL` in dispatch.ts drove the barrier; a second literal
`TERMINAL_TASK_STATUS` in session-manager.ts drove the panel UI's `settled`.
Correctness-critical duplication: add a status and the sidebar would quietly
disagree with the barrier about whether a fan-out had finished. One exported
definition now, imported by both.

## MEDIUM — only one live fan-out was reported

Concurrent panels are legal — an orchestrator can start a second while the
first resolves, which is precisely why the dispatcher watches a SET of tasks
per session. `livePanel()` returned the first unjoined one, so a child
participating only in the other live panel showed no badge, and the progress
bar described one of two.

`livePanels()` now returns all of them, `liveProgress()` sums across them
(one panel's "1/3" misstates the outstanding work when two are running), and
`livePanelMember` searches every live fan-out. The chip reads
"⇉ panels x2" when there is more than one.

## Verification

Daemon 2200 to 2202, web 382 to 387. All four mutation-checked: index
removed, terminal rule diverged, member lookup narrowed to the newest panel,
and progress reporting one panel instead of the sum.

Two things the work surfaced about the tests themselves:

- The new index makes `group_id` undroppable, which broke the migration
  tests' artificial `downgrade()` helper. The real migration only ever ADDS
  columns, so the fix is in the helper; the ordering is an artifact of
  simulating a downgrade.
- The index-coverage block used `it(` in a file that imports `test(`, so it
  threw `ReferenceError: it is not defined` and the whole describe was
  skipped while the suite still reported green. Caught because the pass count
  did not move: 21 before, 21 after, 23 once fixed.
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@saucam
saucam merged commit 9d82ef6 into main Jul 30, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants