refactor(storage): consolidate common store behavior through Bun by mariusvniekerk · Pull Request #1344 · kenn-io/agentsview

mariusvniekerk · 2026-08-05T02:07:03Z

The concrete stores separately implemented sessions, project identity, curation, Recall, usage, and analytics, so fixes had to be repeated and snapshot semantics could drift. This layer makes the guarded BunStore the owner of those common behaviors while leaving adapters responsible for lifecycle and genuine engine capabilities.

Composite session pages, sidebars, message hydration, and timing reads now use replay-safe consistent views. Signal and trend analytics stream transcript content in bounded batches, and the dead PostgreSQL usage renderer is removed. Shared contracts preserve filtering, ordering, hydration, reduction, and supported mutations across real SQLite, PostgreSQL, and DuckDB stores. Stack 2 of 6.

^{generated by a clanker}

roborev-ci · 2026-08-05T02:25:47Z

roborev: Combined Review (`1b69f35`)

The storage refactor has three medium-severity consistency and performance issues; no security vulnerabilities were identified.

Medium

Inconsistent snapshots in multi-query reads — internal/db/bun_sessions.go:175, internal/db/bun_messages.go:23, internal/db/bun_data.go:144
These composite or chunked reads use View instead of ConsistentView, allowing Quack mirror replacement to split related queries across generations; SQLite and PostgreSQL may likewise observe different snapshots. Use consistentView, stage results per attempt because Quack may replay callbacks, and add replay-generation tests.
Activity report fetches usage outside candidate sessions — internal/db/bun_activity_report.go:145
The query loads usage for every session in the date range and filters candidate session IDs only after materialization in Go, potentially causing excessive memory and network usage. Apply candidate IDs through chunked SQL queries before scanning rows.
Session count materializes and sorts all matching usage data — internal/db/usage.go:2937
GetUsageMatchingSessionCount loads and sorts every matching assistant message and usage event to count distinct sessions, creating a substantial performance and memory regression on large archives. Restore a database-side COUNT/COUNT(DISTINCT session_id) query using the existing activity predicates.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 24m8s

roborev-ci · 2026-08-07T04:09:40Z

roborev: Combined Review (`9fe49a4`)

Medium-severity correctness and consistency issues remain in the shared Bun storage layer.

Medium

internal/db/bun_messages.go:500 — Timing data uses the wrong message identifier. GetSessionTiming exposes the message ordinal as MessageID, while SQLite messages normally use the physical messages.id. Because the frontend joins timing data using this ID, timing badges and tool-call durations can fail to associate when the physical ID differs from the ordinal. Select the nullable stored message ID, map ordinals to physical IDs when available, and fall back to the ordinal only for backends without stored IDs. Apply the mapping to turns and calls, with a regression test where the message ID differs from its ordinal.
internal/db/bun_messages.go:23 — Composite reads are not snapshot-consistent. Multi-statement reads such as message hydration use View instead of ConsistentView. In Quack, a mirror replacement between message and tool-call/event queries can mix data from different generations. This also affects composite reads such as ListSessions count/page operations and timing queries. Run each multi-statement logical read through consistentView; keep results and counters local to each callback attempt, publishing them only after a successful attempt so retries cannot leak or duplicate state.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 31m26s

roborev-ci · 2026-08-07T14:27:01Z

roborev: Combined Review (`191fc71`)

Medium

internal/db/bun_sessions.go:175, internal/db/bun_messages.go:23: Composite reads use s.view without a transactional snapshot or DuckDB generation check. Concurrent updates may produce mismatched counts/pages or combine messages and tool data from different snapshots. Use consistentView for multi-query and chunked reads, staging attempt-local results so retries cannot duplicate appended data.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 18m8s

roborev-ci · 2026-08-07T16:52:17Z

roborev: Combined Review (`30ad076`)

Medium-severity consistency and performance issues remain in the shared Bun-backed storage implementation.

Medium

internal/db/bun_usage.go:556 — Filtering bounds fall back to s.created_at, while emitted timestamps only fall back to started_at. Rows lacking usage and start timestamps can produce empty-date buckets for To-only filters or disappear with From. Add SessionCreatedAt to usageProjectionTimestamp, or align the SQL fallback with bucketing.
internal/db/bun_activity_report.go:21 — Activity reports load all matching sessions and messages before applying the requested range, causing substantial latency and memory regressions for narrow reports. Apply the session-window overlap predicate in SQL, then hydrate messages only for candidate session IDs.
internal/db/bun_data.go:144 — BuildProjectIdentityMap uses view for a composite read, so observations and source-archive scope may come from different commits or DuckDB mirror generations, producing invalid response-scoped project keys. Use consistentView and publish only the accepted attempt’s map.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 19m45s

roborev-ci · 2026-08-09T04:18:50Z

roborev: Combined Review (`e21d19f`)

Review identified three medium-severity regressions involving message timing identity, nullable legacy data, and activity-report query performance.

Medium

internal/db/bun_messages.go:527 — Session timing uses message ordinals as message_id, but SQLite and DuckDB messages have distinct database IDs that the frontend uses for joins. Timings may not attach when IDs differ from ordinals.
- Fix: Propagate the actual message ID for backends that provide one, falling back to the ordinal only for PostgreSQL.
internal/db/bunmodel/message.go:54 — tool_use_id is scanned into a non-nullable Go string even though SQLite and its legacy migration permit NULL. Existing archives may fail message, timing, analytics, and recent-edit queries with NULL-to-string scan errors.
- Fix: Model the column as nullable and normalize NULL to an empty string, or add an idempotent migration that backfills nulls and enforces non-null values.
internal/db/bun_activity_report.go:21 — Activity reports load all sessions and messages before applying the requested date range in Go. Usage loading at line 145 also reads all projections in the window rather than restricting them to candidate sessions, potentially causing archive-wide reads and excessive PostgreSQL traffic.
- Fix: Apply overlap predicates in the session query, load messages only for candidate sessions, and restrict usage loading to candidate IDs plus required deduplication peers.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 36m41s

roborev-ci · 2026-08-10T16:13:40Z

roborev: Combined Review (`bea681c`)

Review found two medium-severity regressions in message timing identity and activity-report query scope.

Medium

internal/db/bun_messages.go:527 — GetSessionTiming emits message ordinals as message_id for turns and calls (also line 552), while SQLite and DuckDB message APIs expose persisted IDs. Because the frontend joins timing data to message.id, duration and running-state UI can fail when IDs differ from ordinals.
- Fix: Select messages.id and tool_calls.message_id, using ordinals only when backend IDs are null. Add a behavioral test confirming timing IDs match GetMessages IDs.
internal/db/bun_activity_report.go:21 — The activity-report path loads every message for generally eligible sessions before applying the requested overlap filter at line 46. The usage query at line 145 also ignores candidate session IDs and materializes every usage row in the padded date range. Narrow reports may therefore consume time and memory proportional to unrelated archive history.
- Fix: Apply the overlap predicate when selecting candidate sessions, load messages only for those sessions, and restrict usage loading to candidate IDs plus the cross-session Claude snapshot peers needed for deduplication.

Reviewers: 2 done | Synthesis: codex, 13s | Total: 35m44s

roborev-ci · 2026-08-13T13:48:45Z

roborev: Combined Review (`b1948da`)

The storage refactor has two medium-severity performance regressions; no security issues were found.

Medium

internal/db/bun_activity_report.go:21 — Activity reports load every message from all sessions matching non-date filters, then discard sessions outside the requested window. Short reports over large archives may consume time and memory proportional to the entire archive.
- Fix: Apply the activity-overlap predicate when selecting candidate sessions, then fetch messages and usage only for those IDs and any required Claude snapshot peers.
internal/db/usage.go:3219 — GetUsageMatchingSessionCount materializes all matching messages and usage events and deduplicates session IDs in memory, regressing count queries to O(total usage rows) memory and transfer.
- Fix: Use a portable Bun database-side COUNT, COUNT(DISTINCT ...), or EXISTS query while preserving bounded activity semantics.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 25m36s

roborev-ci · 2026-08-14T21:51:23Z

roborev: Combined Review (`9fcd600`)

The refactor has one medium-severity correctness issue affecting timing badges across SQLite and DuckDB.

Medium

internal/db/bun_messages.go:527 — Timing turns and calls now expose message ordinals as message_id, but SQLite and DuckDB messages retain distinct source row IDs. Because the frontend joins timing data against those source IDs, duration and running-state badges can attach to the wrong messages or not appear.
- Fix: Select optional message/tool-call IDs and use them when available, falling back to ordinals only for backends without IDs. Add a cross-backend assertion that timing IDs match those returned by GetMessages.

Reviewers: 2 done | Synthesis: codex, 10s | Total: 36m54s

roborev-ci · 2026-08-15T02:56:45Z

roborev: Combined Review (`3d56eec`)

Medium-severity consistency issues remain in composite database reads.

Medium

internal/db/bun_data.go:28,144 — Project identity reads execute multiple queries through View, which provides neither a database snapshot nor Quack generation retries. Concurrent updates or mirror replacement can mix observation chunks from different generations or derive project keys from observations and archive scope from different snapshots. Use ConsistentView, publishing only attempt-local results after a successful callback.
internal/db/bun_sessions.go:266,317 — GetSessionFull and batched partial-ID lookup also perform multiple queries through View. SQLite can combine ordinary and operational fields from different row versions, while a Quack replacement between keyset batches can skip or duplicate partial-ID results. Use ConsistentView and build partial-ID results in an attempt-local slice so retries cannot retain rejected results.

Reviewers: 2 done | Synthesis: codex, 11s | Total: 30m14s

Core storage reads had drifted across SQLite, PostgreSQL, and DuckDB, making lifecycle and schema parity difficult to preserve. Centralize canonical session, message, timing, cursor, and metadata reads in BunStore so adapters only own guarded handles and engine lifecycle. Keep result hydration bounded, preserve case-sensitive partial-ID behavior portably, and expose source-file metadata only through full-session reads.

The shared-store cutover cannot trust a compatibility stamp unless canonical parents, indexes, triggers, and backend column contracts still match. Fail closed on stamped drift and advance the archive data version so older binaries cannot write through the one-time identity cutover. Keep core reads bounded and chronological across large transcripts and mixed SQLite timestamps, while preserving PostgreSQL's native update marker and serialized convergence semantics.

Project identity must resolve from the same source-scoped rows on every serving backend before inventory and governance can move into the common store. Centralize observation selection and archive-scope aggregation in BunStore so SQLite, PostgreSQL, and DuckDB derive identical project identities without a backend-specific scanner.

The shared-store cutover must preserve archive chronology, parser bookkeeping, and source provenance while retiring backend-specific identity readers. Without these invariants, an upgraded archive could be reopened by an older binary or leave a required resync incomplete across restarts.\n\nMake the cutover version floor and pending-resync state atomic, keep canonical identity replacement and artifact writes provenance-complete, and restrict adapter-specific hydration to SQLite operational fields.

Inventory, governance rules, and worktree candidates must observe the same canonical session provenance and source-scoped mappings on every serving backend. Separate scanners made cross-archive isolation and snapshot selection depend on the adapter in use.\n\nRun these reads once through BunStore, preserve the existing pure identity and governance reducers, and prove the literal contract against SQLite, DuckDB, and PostgreSQL before deleting the concrete implementations.

The long-lived canonical cutover must stay ahead of main-line data versions and treat an installed replacement as final even when its caller is canceled after the atomic swap. Otherwise a successful resync can be reported stale or reopened by an older writer. Keep composite inventory, governance, and candidate reads on one backend snapshot with bounded hydration, and compare SQLite activity as instants so mixed-offset archives preserve the cross-backend contract.

Stars and pins are dashboard curation regardless of the serving engine, but concrete implementations encoded different message identities and read-only behavior. Keeping them separate made PostgreSQL and DuckDB semantics drift from the canonical session/ordinal model. Route the family through operation-scoped Bun writes, preserve target-generated pin identities during replicated upserts, and keep DuckDB rejection ahead of SQL while allowing PostgreSQL curation despite its public remote-mode status.

Insight persistence and reads are dashboard behavior regardless of serving engine, but concrete implementations duplicated filtering, timestamp normalization, and write policy. That drift made future schema work depend on three method families instead of the canonical Bun model.\n\nRoute insights through the operation-scoped common store, keep DuckDB read-only before SQL, and require PostgreSQL's existing insight capability probe before permitting writes.

Composite reads and curation writes must remain coherent across engine lifecycle changes, canonical ID transitions, and restricted PostgreSQL roles. Without explicit adapter guarantees, a unified method could still mix DuckDB generations, hydrate an entire archive, collide replicated pin IDs, or lose the established read-only sentinel.\n\nMake snapshot semantics mandatory, keep the one-time cutover fence fixed, bound candidate hydration by selected projects, and separate generated versus mirrored curation identities. Probe PostgreSQL insight insertion and deletion independently while preserving permission failures as ErrReadOnly.

Session rename and trash behavior must preserve the same user-tombstone, alias-exclusion, and atomicity rules regardless of the serving engine. Separate implementations made PostgreSQL ownership timestamps, SQLite watcher baselines, and DuckDB read-only policy easy to drift. Route the family through operation-scoped Bun transactions, retain only adapter-owned operational touches, and reject unsupported writes before opening a backend write guard.

Unified Bun mutations must retain each adapter’s operational guarantees and expose result counts only after atomic success. Quack also needs a replacement identity that cannot collide when descriptive metadata repeats. Keep SQLite FTS cleanup and PostgreSQL database-clock revisions behind adapter hooks, and stamp disposable DuckDB mirrors with a schema-v11 opaque generation.

Recall should follow the same guarded handle and capability policy as the other common Store families. Concrete remote stubs split method ownership and made read-only rejection depend on each wrapper. Route canonical entry reads, inserts, and query events through BunStore while retaining SQLite-only FTS, vector, import, and eval behavior behind an explicit capability.

Shared storage callbacks and mirror replication must remain correct under replay, read-only operation, stale replicated identities, and partial metadata failures. These cases sit at adapter boundaries where a nominally unified query can otherwise publish rejected state or make a valid mirror unreadable. Preserve read-only Recall validation, make composite results attempt-local, reconcile mirror-owned pin IDs, and publish DuckDB generation metadata atomically with an explicit compatibility requirement. Keep the implementation plan aligned so later Bun cutover work retains those constraints.

Pricing and usage are common storage behavior, but three independent implementations made exact money arithmetic, catalog fallback, windowing, and transaction semantics vulnerable to backend drift.\n\nRoute all usage reads and pricing state through BunStore, preserve engine-neutral cost reducers, and make pricing-row plus band replacement atomic so every adapter observes the same catalog snapshot.

Unified reads and writes must stay inside one backend snapshot and one atomic publication boundary, including retry, optional-schema, pricing, and mirror identity edge cases. Without those guarantees the common Bun path could combine generations, leak replay state, or partially publish adapter metadata. Keep filtering and time windows in SQL, use database time for portable insight ordering, and make the remaining adapter-specific capabilities explicit so the next cutover slices can delete concrete paths safely.

Analytics, trends, activity reports, and recent edits are common serving behavior, but independent backend implementations made filters, timestamp handling, and aggregation semantics drift across engines.\n\nRoute the full family through one replay-safe Bun snapshot and shared reducers, leaving backend wrappers with only their operational usage helpers. Remove the private SQL-builder tests and concrete scanners so future analytics changes have one owner and one literal cross-backend contract.

The shared analytics and usage paths must retain backend ordering, optional-schema, replay, and activity-window contracts while keeping remote reads bounded. Review exposed edge cases where the first unified implementation could mix rejected snapshots, drift across dialects, or substitute the wrong timestamps. Keep only genuinely dialect-specific timestamp expressions at the adapter boundary and preserve the established empty-result and nil-safety contracts so the next search/vector cutover starts from a verified common store.

Remote snapshot adapters can replay a read callback after the underlying generation changes. Composite session and message reads must therefore discard the first attempt and publish only the accepted retry, including count, hydration, and timing data.\n\nThe PostgreSQL usage cutover also left an unreachable query renderer behind and dropped a required schema-validation return. Remove only renderer paths with no production caller, retain the activity-report row and pricing contracts, and restore the schema prerequisite.

Signal and trend requests can span an entire archive, so retaining every matching transcript body makes peak memory grow with archive size. Reduce narrow content projections as database rows arrive and bound each query to a small session batch without limiting the final totals.\n\nSignal drill-down still ranks the complete candidate set, then loads full message content only for the requested examples. Trend session filtering keeps date bounds at message time so timestamp fallback behavior does not change.

Foundation now gives Bun-owned identity triggers non-colliding names so legacy startup DDL cannot overwrite them. Keep the later stamped-drift regression pointed at that canonical trigger while preserving its fail-without-repair assertion.

Bun SelectQuery.Clone drops a transaction connection, which deadlocks composite reads on a single-connection DuckDB store. Rebind cloned session queries to the guarded view handle. Bun also renders zero-valued default-tagged fields as DEFAULT, but existing PostgreSQL pricing-band columns have no physical defaults. Bind every canonical numeric price explicitly while retaining bounded shared batches across all adapters.

Stamped convergence now validates the metadata value rather than only key existence. Make the schema probe return the stored compatibility value so fail-closed and DDL-skip tests continue to exercise the stamped path.

The usage cutover must select complete Claude snapshots before applying model and session filters, then credit the surviving tokens and fees to the original session. Otherwise delegated transcripts change dashboard totals, session counts, activity reports, and web-search costs when the shared Bun path replaces backend-specific SQL.\n\nKeep operational DuckDB costing aligned with that contract and remove renderer-only tests whose implementation no longer has a production caller.

Current main requires incremental session lookup to include the agent because one path can belong to multiple provider namespaces. Preserve the existing test intent by selecting the seeded Claude and Codex sessions explicitly.

roborev-ci · 2026-08-17T19:23:26Z

roborev: Combined Review (`1f81c0f`)

The storage refactor has two medium-severity issues; no security regressions were identified.

Medium

internal/duckdb/push.go:1038 — DuckDB synchronization omits parser_parent_session_id from inserts, updates, arguments, and fingerprinting. Mirrored sessions lose the value, and changes to it alone do not trigger synchronization. Mirror the column throughout these paths and add a round-trip test for parser-parent-only changes.
internal/db/bun_activity_report.go:40 — Activity reports load every matching session and all messages before applying the requested time range. Usage loading at line 219 also materializes the entire date window before filtering, risking archive-wide memory and latency regressions. Restore SQL-level overlap filtering and limit message and usage queries to candidate sessions and required attribution peers.

Reviewers: 2 done | Synthesis: codex, 5s | Total: 19m10s

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 1b69f35 to 9fe49a4 Compare August 7, 2026 03:46

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 9fe49a4 to 191fc71 Compare August 7, 2026 14:10

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 191fc71 to 30ad076 Compare August 7, 2026 16:34

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 30ad076 to e21d19f Compare August 9, 2026 03:46

mariusvniekerk force-pushed the t3code/bun-store-reads branch from e21d19f to bea681c Compare August 10, 2026 15:39

mariusvniekerk force-pushed the t3code/bun-store-reads branch from bea681c to b1948da Compare August 13, 2026 13:25

mariusvniekerk force-pushed the t3code/bun-store-reads branch from b1948da to 9fcd600 Compare August 14, 2026 21:18

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 9fcd600 to 3d56eec Compare August 15, 2026 02:28

mariusvniekerk added 6 commits August 17, 2026 14:14

mariusvniekerk added 18 commits August 17, 2026 14:14

test(storage): model stamped PostgreSQL metadata values

42010ca

Stamped convergence now validates the metadata value rather than only key existence. Make the schema probe return the stored compatibility value so fail-closed and DDL-skip tests continue to exercise the stamped path.

test(storage): scope incremental lookups after rebase

1f81c0f

Current main requires incremental session lookup to include the agent because one path can belong to multiple provider namespaces. Preserve the existing test intent by selecting the seeded Claude and Codex sessions explicitly.

mariusvniekerk force-pushed the t3code/bun-store-reads branch from 3d56eec to 1f81c0f Compare August 17, 2026 19:06

Navigation Menu

Uh oh!

refactor(storage): consolidate common store behavior through Bun - #1344

refactor(storage): consolidate common store behavior through Bun#1344
mariusvniekerk wants to merge 24 commits into
t3code/bun-storage-foundationfrom
t3code/bun-store-reads

mariusvniekerk commented Aug 5, 2026 •

edited

Loading

Uh oh!

roborev-ci Bot commented Aug 5, 2026

Uh oh!

roborev-ci Bot commented Aug 7, 2026

Uh oh!

roborev-ci Bot commented Aug 7, 2026

Uh oh!

roborev-ci Bot commented Aug 7, 2026

Uh oh!

roborev-ci Bot commented Aug 9, 2026

Uh oh!

roborev-ci Bot commented Aug 10, 2026

Uh oh!

roborev-ci Bot commented Aug 13, 2026

Uh oh!

roborev-ci Bot commented Aug 14, 2026

Uh oh!

roborev-ci Bot commented Aug 15, 2026

Uh oh!

roborev-ci Bot commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mariusvniekerk commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roborev-ci Bot commented Aug 5, 2026

roborev: Combined Review (1b69f35)

Medium

Uh oh!

roborev-ci Bot commented Aug 7, 2026

roborev: Combined Review (9fe49a4)

Medium

Uh oh!

roborev-ci Bot commented Aug 7, 2026

roborev: Combined Review (191fc71)

Uh oh!

roborev-ci Bot commented Aug 7, 2026

roborev: Combined Review (30ad076)

Medium

Uh oh!

roborev-ci Bot commented Aug 9, 2026

roborev: Combined Review (e21d19f)

Medium

Uh oh!

roborev-ci Bot commented Aug 10, 2026

roborev: Combined Review (bea681c)

Medium

Uh oh!

roborev-ci Bot commented Aug 13, 2026

roborev: Combined Review (b1948da)

Medium

Uh oh!

roborev-ci Bot commented Aug 14, 2026

roborev: Combined Review (9fcd600)

Medium

Uh oh!

roborev-ci Bot commented Aug 15, 2026

roborev: Combined Review (3d56eec)

Medium

Uh oh!

roborev-ci Bot commented Aug 17, 2026

roborev: Combined Review (1f81c0f)

Medium

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

1 participant

mariusvniekerk commented Aug 5, 2026 •

edited

Loading

roborev: Combined Review (`1b69f35`)

roborev: Combined Review (`9fe49a4`)

roborev: Combined Review (`191fc71`)

roborev: Combined Review (`30ad076`)

roborev: Combined Review (`e21d19f`)

roborev: Combined Review (`bea681c`)

roborev: Combined Review (`b1948da`)

roborev: Combined Review (`9fcd600`)

roborev: Combined Review (`3d56eec`)

roborev: Combined Review (`1f81c0f`)