Skip to content

fix(api,web,db): optimistic-locking for dashboards + service-map nested aggregate - #36

Merged
Makisuo merged 4 commits into
mainfrom
claude/flamboyant-lamarr-b598e5
May 9, 2026
Merged

Makisuo merged 4 commits into
mainfrom
claude/flamboyant-lamarr-b598e5

Conversation

@Makisuo

@Makisuo Makisuo commented May 9, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Closes Section 4 (dashboard concurrency / lost updates) and Section 6 (service-map nested-aggregate SQL error) of the DeepSec triage in .claude/plans/we-created-an-depsec-cuddly-tarjan.md.

  • Dashboards now use optimistic concurrency via a new dashboards.version column + CAS upsert. Two concurrent MCP calls or browser edits can no longer silently lose one update; the loser surfaces a typed DashboardConcurrencyError (HTTP 409) and the web hook auto-refetches.
  • Service map ClickHouse query no longer trips the UNION-ALL+GROUP-BY optimizer into producing sum(sum(...)); aliases are renamed and the average is now properly weighted across MV + raw branches.

What changed

Schema (held until human review)

  • packages/db/drizzle/0009_dashboard_versioning.sql — adds dashboards.version INTEGER NOT NULL DEFAULT 0 and a unique index on dashboard_versions(org_id, dashboard_id, version_number).
  • packages/db/src/schema/dashboards.ts, packages/db/drizzle/meta/0009_snapshot.json, _journal.json updated to match.

Domain

  • DashboardConcurrencyError (HTTP 409) added to @maple/domain/http. Wired into the upsert, restoreVersion, create, and instantiateTemplate HTTP endpoints' error unions.

Persistence

  • DashboardPersistenceService.upsert now reads current version and runs UPDATE ... WHERE id = ? AND version = ?. Zero rows affected → conflict.
  • New DashboardPersistenceService.mutate(orgId, userId, dashboardId, transform) runs the read-CAS-write loop with up to 5 retries before surfacing the typed conflict. The MCP withDashboardMutation helper now delegates to it, so every widget tool inherits retry-on-conflict.

MCP tools

  • update-dashboard: preserves existing.variables in both the metadata-only and full-replacement branches (the full-replacement path was silently wiping it).
  • reorder-dashboard-widgets: server-side geometry validation (x>=0, y>=0, 1<=w<=12, h>=1, x+w<=12).

Web client

  • useDashboardStore now serializes mutations per dashboard with a FIFO queue (Map<id, Promise>), so back-to-back edits can't race against each other.
  • On DashboardConcurrencyError the optimistic update rolls back, a banner surfaces the conflict, and useAtomRefresh triggers a refetch — the existing list-result effect clears the banner once fresh state lands.

Service map (separate root-cause, same PR)

  • Inner branches in serviceDependenciesSQL and serviceDbEdgesSQL expose distinct bucket* aliases so the outer sum(...) AS callCount has no inner name to fold against.
  • Outer average switched from avg(avgDurationMs) (averaging averages) to sum(bucketDurationSumMs) / nullIf(sum(bucketCallCount), 0) (properly weighted across branches).
  • MV branch reads Hour < toStartOfHour(endTime) only; raw branch reads Timestamp >= toStartOfHour(endTime) to endTime. Previously both overlapped on the trailing hour and double-counted spans.

Tests

  • apps/api/src/mcp/tools/__tests__/dashboard-concurrency.test.ts — 3 vitest cases:
    1. Two concurrent mutate calls both land via retry — no lost update.
    2. Two concurrent upsert calls — loser surfaces DashboardConcurrencyError instead of silently overwriting.
    3. After a conflict, a refetch+retry resolves cleanly.

Whole api test suite (279 tests across 20 files) green.

Test plan

  • bun typecheck clean on @maple/api, @maple/web, @maple/db, @maple/domain, @maple/query-engine (only pre-existing unrelated worker.ts import error remains)
  • bun test from apps/api — 279 pass, 0 fail
  • D1 migration 0009_dashboard_versioning.sql applies cleanly via wrangler d1 migrations apply --local
  • Browser preview verified: created a dashboard (version=1, audit row created), added a Bar Chart widget (version=2, audit row widget_added), fired two parallel PUT /api/dashboards/:id requests (both committed against fresh state, version advanced 2→4 — local D1 serializes so neither write actually overlapped, but both succeeded which is the correct CAS behavior; the actual race-window assertion is covered by the unit tests)
  • Browser preview verified: service-map page renders cleanly — previously failed with the nested-aggregate SQL error, now returns 200 OK with proper content
  • Generated SQL inspected: zero sum(sum(...)) patterns, single AS callCount (outer only) in both queries

Reviewer notes

  • Schema migration apply is held per the plan — please review 0009_dashboard_versioning.sql before running it through the release flow. SQLite-friendly (ADD COLUMN ... DEFAULT 0 NOT NULL), no backfill required.
  • HTTP upsert does single-attempt CAS (no retry) so the client gets a 409 to refetch from. MCP mutate retries internally because the transform is re-applied on top of fresh state — that's safe; HTTP retry would silently overwrite.
  • recordVersion is still wrapped in Effect.ignore (best-effort audit). The new unique index protects against duplicate version_number rows under racy recordVersion calls; the worst case is a missed audit row, which is acceptable given the dashboard-table CAS guarantees the actual state is consistent.

🤖 Generated with Claude Code


View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

…ed aggregate

DeepSec section 4 (dashboard concurrency) + section 6 (service-map SQL).

Dashboards previously used read-modify-write with no compare-and-swap, so two
concurrent MCP calls or browser edits would silently lose one update.
`recordVersion` also computed `latest+1` without a unique index, allowing two
concurrent saves to stamp the same `versionNumber`.

- Add `dashboards.version` column + unique index on
  `dashboard_versions(org_id, dashboard_id, version_number)` via migration
  0009_dashboard_versioning.
- `DashboardPersistenceService.upsert` now reads current version and runs
  `UPDATE ... WHERE id = ? AND version = ?`. Zero rows affected → typed
  `DashboardConcurrencyError` (HTTP 409). New `mutate(transform)` API runs
  the read-CAS-write loop with up to 5 retries before surfacing the conflict;
  used by all 5 widget MCP tools via `withDashboardMutation`.
- `update-dashboard` MCP tool: preserve `existing.variables` in both the
  metadata-only and full-replacement branches (was wiping it).
- `reorder-dashboard-widgets` MCP tool: validate layout geometry server-side
  (`x>=0, y>=0, 1<=w<=12, h>=1, x+w<=12`).
- `useDashboardStore`: per-dashboard FIFO mutation queue so back-to-back
  edits can't race; on `DashboardConcurrencyError` roll back the optimistic
  update, surface a refetch banner, and trigger `useAtomRefresh`.
- 3 vitest cases in `apps/api/src/mcp/tools/__tests__/dashboard-concurrency.test.ts`
  exercise concurrent `mutate` (both succeed via retry), concurrent `upsert`
  (loser surfaces typed conflict), and the refetch+retry recovery flow.

Service map (`serviceDependenciesSQL` + `serviceDbEdgesSQL`) was returning
"Aggregate function sum(callCount) AS callCount is found inside another
aggregate function in query." ClickHouse's UNION-ALL+GROUP-BY optimizer was
pushing the outer `sum(callCount)` into each branch where the inner already
aliased its aggregate as `callCount`, producing the rejected `sum(sum(...))`.

- Inner branches now expose distinct `bucket*` aliases so the outer
  aggregate has no name to fold against.
- Outer average is now properly weighted:
  `sum(bucketDurationSumMs) / nullIf(sum(bucketCallCount), 0)` instead of
  `avg(avgDurationMs)` (which averaged averages, ignoring relative call
  counts per branch).
- MV branch reads `Hour < toStartOfHour(endTime)` (complete buckets only),
  raw branch reads `Timestamp >= toStartOfHour(endTime)` to `endTime` (the
  in-progress hour). Previously both branches overlapped on the trailing
  hour and double-counted spans.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pullfrog

pullfrog Bot commented May 9, 2026 •

Copy link
Copy Markdown
Contributor

This run croaked 😵

The workflow encountered an error before any progress could be reported. Please check the link below for details.

Pullfrog  | Rerun failed job ➔ | View workflow run | via Pullfrog | 𝕏

…eanup filter

The `cleanupStaleTinybirdDeployments` source already restricts deletion to
terminal `failed`/`error` statuses (this is the plan section 3 fix that
landed on main earlier), but the test still asserted the old behaviour
("delete deploying + data_ready"). Align the test fixture with the actual
filter so CI on this branch passes.

This matches the uncommitted fix already present on the user's main worktree.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`AlertDestinationCreateRequest` is a `Schema.Union` of `Schema.Class`
constructors, so `Schema.encodeUnknownSync` rejects plain objects on the
input side — they aren't instances of the union members. The test was
passing plain objects directly, which broke when the configs were migrated
to `Schema.Class`.

Construct the inputs with `new SlackAlertDestinationConfig({...})` etc. so
encoding has a class instance to project to the plain wire shape. Same
assertions, same wire-format expectations — only the input side changes.

Pre-existing breakage on main, not introduced by this PR; fixing here so
the branch's CI is green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`packages/clickhouse-cli` has no test files but the `test` script ran
`bun test`, which exits 1 when no files match the test glob. Turbo then
fails the whole pipeline on this package even though there's nothing to
test. Remove the script so turbo skips this package's `test` task instead.

Pre-existing breakage on main, surfaced once the unrelated domain test
failures stopped masking it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Makisuo
Makisuo merged commit e91d126 into main May 9, 2026
2 of 3 checks passed
@Makisuo
Makisuo deleted the claude/flamboyant-lamarr-b598e5 branch May 9, 2026 21:57
JeremyFunk added a commit that referenced this pull request Sep 28, 2026
)

Seeds a reused Strands agent over an event-loop span (two turns) and an
OpenRouter request whose generation and provider attempt both failed, under
their own org, and runs the real compiled page query: 595 tokens and 2 calls
for the Strands session, 1 call for the failed request. The unit tests only
compare the netting SQL's text. The suite runs in CI once the ClickHouse E2E
job lists this file.
JeremyFunk added a commit that referenced this pull request Sep 29, 2026
* fix(agent-sessions): stop zero-usage spans from absorbing their children's tokens

Symptom: Vercel AI SDK sessions showed exactly twice their tokens on the
session page (docs-verify-vercel-ai-sdk a2: 680 for two chat calls totalling
340; a1 3930 for 1965).

Cause: with `usage: true` the SDK stamps
`ai.usage.outputTokenDetails.reasoningTokens="0"` on the `step` span between
`invoke_agent` and `chat`. That decodes to a reasoning bucket of 0, so
`spanTokenBuckets` returned a zero total and `countableUsageSpans` treated the
step as a reporter. `chargeToNearestReporter` then charged each chat to its
step (netting to nothing) and the agent span kept its whole roll-up.

Fix: a span whose buckets total zero is not a reporter, which is already the
list SQL's rule (`usageReportersExpr` admits `Tokens > 0`).

Seen in: Vercel AI SDK 7 capture docs_vercel-ai-sdk_a.

* fix(agent-sessions): net list usage through spans that report nothing

Symptom: the sessions list showed exactly twice the tokens of the session
page for Strands (docs-verify-strands a1 4712/262 vs 2356/131, b 6146/1246
vs 3073/623) and for the Vercel AI SDK (a2 680 vs 340).

Cause: the list SQL charged each reporter to its direct parent
(`childClaimsExpr` keyed on ParentSpanId, ai-span-columns.ts:121). Strands
puts `execute_event_loop_cycle` between `invoke_agent` and `chat`, the
Vercel AI SDK a `step` span, smolagents a `Step N` chain; the claims were
keyed on those spans, no reporter picked them up, and the agent span kept
its full roll-up. The session page walks the whole ancestry.

Fix: each trace also collects its links (span id -> itself when it
reported usage, else its parent). The session level climbs up to 8 links
from each reporter's parent to the nearest ancestor that reported and
carries it as a twelfth tuple element, which the claims and the "any
ancestor reported" test for unreported calls key on. Verified against the
EU replay: list totals now equal the session page for every Strands, Vercel
AI SDK and smolagents session. A week of the production org nets to the
same totals as before at the same read time (~370ms either way).

Limit: a parent outside the index (a span without a vendor stamp, e.g. the
Strands TypeScript loop span under a custom service name) still stops the
climb.

Seen in: captures docs_strands_{a,b,g}, docs_vercel-ai-sdk_{a,b}.

* fix(agent-sessions): count a failed gateway request once, not once per attempt

Symptom: an OpenRouter Broadcast request whose every provider attempt failed
counted as 2 LLM calls on the list and the session page
(trace:83d675eb59e436078591a758e28cb09a: `LLM Generation` plus
`provider attempt 1: OpenAI`, both op `chat`, both Error, no usage).

Cause: a model call that reported no usage counted unless an ancestor
reported usage (session-summary.ts `countedLlmCalls`, ai-span-columns.ts
`nettedReportersExpr`). The attempt netting only worked when the generation
above it reported usage, which a failed request never does.

Fix: a model call that reported no usage does not count when its parent is
a model call. On the list the session's reporter ids now carry every
reporter (usage and model calls), so the check is one `has` on the direct
parent next to the existing one on the charged ancestor. Verified on the
EU replay (the trace now counts 1; other sessions unchanged) and on a week
of the production org (378 unreported calls counted before and after).

Seen in: OpenRouter Broadcast capture (openrouter guide, failed request).

* fix(agent-sessions): net cost the same way on the list and the session page

Symptom: nested agents that each stamp `gen_ai.usage.cost` summed to
different session costs on the list and the session page (LiteLLM guide,
orchestrator delegating to workers through tool spans): the page subtracted
the sub-agents' cost from the orchestrator's roll-up, the list did not.

Cause: two rules differed. The list charged a claim to its direct parent,
which for a sub-agent is its tool span, so nothing was netted; that half is
fixed by the full-ancestry netting in the list commit before this one. The
page also took a span stamped with a zero cost as a cost reporter
(`costBySpan`, `cost >= 0`, session-summary.ts), where the list only
charges `Cost > 0`: a zero-cost wrapper absorbed its calls' cost and left
the agent above it keeping its whole roll-up, the #28 shape for cost.

Fix: a zero cost is recorded (so the session still reads "free", not
"unmeasured") but is not a reporter claims are charged to.

Seen in: LiteLLM capture docs_litellm_b (nested agents), reconstructed as a
fixture; the replayed session already agrees because the guide now prices
only the outermost agent.

* fix(agent-sessions): count a tool call paused for a human once

Symptom: a Strands session with one human-approved `delete_file` call
showed it twice in the session page's tool ledger and tool-call count
(docs-verify-strands a1: 4 tool calls, `delete_file` x2, for 3 calls).

Cause: the interrupted call ends its `execute_tool` span Ok with no result,
and the resumed turn's trace opens a second span under the same
`gen_ai.tool.call.id` (call_ltoPrLQIg3ZHWkyFnBIOE65u, traces df191678… and
18645746…). `buildSessionSummary` counted every tool span
(session-summary.ts `work.toolCalls`, `toolUsage`).

Fix: `countedToolCalls` takes the session's tool spans with the ones
sharing a call id collapsed to the last to start (the resumed copy, which
carries the result); spans without an id count as before. Both the count
and the ledger read it.

The list's tool-call count is not changed: `ai_trace_index` carries no
call id, so deduping it there needs a new index column and a recreated
materialized view. Across the EU replay (every framework) this is the only
shared call id among tool spans, and a week of the production org has
none.

Seen in: capture docs_strands_a (HITL resume).

* fix(agent-sessions): climb list claims per measure, as the session page charges them

Review follow-up to the #7 list netting. The list climbed one chain for
both measures (nearest ancestor with tokens or cost), while the session page
charges tokens to the nearest token reporter and cost to the nearest cost
reporter. A wrapper that priced but did not count (or the reverse) between
an agent and its call stopped the other measure's claim, and the agent kept
its roll-up of it on the list only.

Each trace now carries two links maps (`tokenLinks`, `costLinks`); each
reporter carries both ancestors (elements 12 and 13), and the child-claims
sumMap enters each reporter twice, its tokens under one and its cost under
the other. The climb is 4 links (past up to three non-reporting spans; the
deepest shape seen is two) to keep the added map lookups down: a
production week netted in full reads ~360-440ms against ~220-370ms before
the netting change, same totals; EU replay totals unchanged.

* fix(agent-sessions): merge only a paused tool call into its resumed copy

Review follow-up to #21. Keying the tool calls on `gen_ai.tool.call.id`
alone merged any two calls sharing an id, and ids are not unique across a
session for every emitter (parallel lanes, providers that number calls per
turn). Only a span that recorded no result and did not fail is now dropped,
and only when a later span with the same id carries a result — the shape a
human-approval pause leaves. A session captured without payloads keeps
every span.

* fix(agent-sessions): count nothing of an agent span whose calls reported (cumulative reporters)

#7, cumulative reporters. Symptom: agents that live across turns report the
conversation so far on their agent span, and both pages counted that
excess over the agent's own calls again.
- smolagents `run(reset=False)` (capture cap_a_v1, session a1): run spans
  report 1067, 2243, 4857, 7811 cumulatively over calls summing to 7811;
  both pages showed 15978 (2.05x).
- Strands with one Agent reused across requests (capture strands_user,
  scenario a): 18044 shown against 4541 billed (~4x).

Cause: a wrapper kept whatever it reported above the reporters beneath it
(`countableUsageSpans` / `costBySpan`, `nettedReportersExpr`), which is
right for a model-call wrapper whose child call reported nothing and wrong
for an agent whose excess is earlier turns.

Fix: a reporter that is not a model call (an agent, a workflow, any wrapper
the index does not flag `IsLlmCall`) claims nothing of a measure once a
reporter of that measure is charged to it; model-call wrappers (an SDK's
`generateText` over `doGenerate`, a gateway generation over its attempts)
keep their excess. Same rule on the session page (`keptClaim`) and the
list (`if(r.6 = 0 AND charged > 0, 0, …)` per measure). Verified with the
compiled page query over literal index rows on ClickHouse 25.8 (smolagents
7811, Strands 1031 for three turns); the EU replay and a production week
have no agent span with an excess, so their totals are unchanged.

* fix(agent-sessions): say which frameworks the paused tool-call merge covers (#21)

OpenAI Agents stamps no gen_ai.tool.call.id on its tool spans, so its
human-approval copies still count twice; the comment no longer lists it.

* test(agent-sessions): prove the list netting on real index rows (#7, #36)

Seeds a reused Strands agent over an event-loop span (two turns) and an
OpenRouter request whose generation and provider attempt both failed, under
their own org, and runs the real compiled page query: 595 tokens and 2 calls
for the Strands session, 1 call for the failed request. The unit tests only
compare the netting SQL's text. The suite runs in CI once the ClickHouse E2E
job lists this file.

* fix(agent-sessions): state which paused tool-call copies the #21 merge covers

Covered: Strands, which records the resumed call's result on span
attributes. Not covered, still counted twice: Google ADK (outcome only in
gcp.vertex.agent.tool_response, and the paused copy's confirmation request
reads as a result), Strands versions that record results in span events,
and OpenAI Agents (no gen_ai.tool.call.id).

This branch was previously deployed

1 inactive deployment
pr-preview — b58905d5 Deployed May 9, 2026 by Makisuo via deploy-pr-preview #113
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.

1 participant