Skip to content

fix(server): drop the V1 tables V2 never reads - #20946

Closed
derektrimm wants to merge 1 commit into
pingdotgg:mainfrom
derektrimm:fix/drop-unread-v1-tables
Closed

derektrimm wants to merge 1 commit into
pingdotgg:mainfrom
derektrimm:fix/drop-unread-v1-tables

Conversation

@derektrimm

Copy link
Copy Markdown
Contributor

Fixes #20922

The V2 database starts as a full backup of the V1 state.sqlite, so every V1 table comes along. The legacy importer reads only V1 threads, messages and pull request links, and nothing else on main reads projection_thread_activities, projection_turns, projection_thread_proposed_plans, projection_pending_approvals, projection_state or checkpoint_diff_blobs. They stayed in statev2.sqlite for good, and t3 doctor --fix copied them into the rebuilt file.

Migration 063 drops those six tables. Their pages join the free list, which the Storage status already counts as reclaimable and t3 doctor --fix returns to disk. scripts/mobile-showcase-environment.ts stops seeding them; per its own comment, V2 clients never read those rows. The screenshot doc no longer lists activities among the seeded data.

Checked on a VACUUM INTO copy of a real 0.0.45 database (14.1 GiB, 351 threads, 1.63M V1 events, about 1.0M activities). Each arm booted its own copy until the transcript import finished (importedThreadCount: 351, importedMessageCount: 48255), then ran t3 doctor --fix:

main (133f57a) this branch
statev2.sqlite after t3 doctor --fix 6.31 GiB 409.4 MiB
largest table after doctor projection_thread_activities, 5.7 GiB projection_thread_messages, 109 MiB
t3 doctor --fix duration 1,228 s 935 s
runMigrations on first boot 44.4 s 88.8 s

The cost is on first boot. Dropping a 5.9 GiB table means SQLite reads every overflow page to free it. In the server that added 44 s to migrations on this machine (SATA SSD, NTFS). On its own, the drop took 52 to 54 s on an NVMe btrfs box, 31 s of it kernel time. mmap_size did not change that, and a 256 MiB page cache brought it to 38 s. It runs once and scales with the size of the activities table. The upgrade it joins already backs up the whole V1 database first.

The new 063_DropUnreadV1Projections test fails when the migration does nothing. The migration list tests, the legacy importer, cutover and initializeV2Database tests, DatabaseMaintenance and mobile-showcase tests pass. Typecheck, lint and format pass for apps/server and scripts.

Not checked: running the mobile showcase end to end, since it needs the mobile build. A V2 preview database migrated by reconcileV2PreviewMigration takes the same migration, and its test passes.

Made with Claude Opus 5.5 in Claude Code, running in T3 Code.

The V2 database starts as a backup of the V1 one, so every V1 table came
along. The legacy importer reads only V1 threads, messages and pull
request links; the activity, turn, proposed plan, pending approval,
projection state and checkpoint diff tables stayed for good, 5.9 GiB of
activities on one long-lived install.

Fixes pingdotgg#20922
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 17, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 17, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR adds a production migration that irreversibly removes six persisted tables and may substantially increase first-boot migration time for large databases. Although tests support the unused-table rationale and the other changes are limited to fixtures and documentation, the destructive schema change warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 245a94dd-1cd8-45ff-ae37-54b56098a43c
📥 Commits

Reviewing files that changed from the base of the PR and between 4b1024d and b9209f2.

📒 Files selected for processing: 7
  • apps/server/src/persistence/Migrations.ts
  • apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts
  • apps/server/src/persistence/Migrations/063_DropUnreadV1Projections.test.ts
  • apps/server/src/persistence/Migrations/063_DropUnreadV1Projections.ts
  • apps/server/src/persistence/reconcileV2PreviewMigration.test.ts
  • docs/operations/mobile-app-store-screenshots.md
  • scripts/mobile-showcase-environment.ts
💤 Files with no reviewable changes: 1
  • scripts/mobile-showcase-environment.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Migration 63 drops six V1 projection tables. Migration tests and migration-result expectations include the new migration. The mobile showcase seed no longer creates or clears several removed V1 projection tables, and its documentation now describes persisted mobile-outbox tasks.

Changes: V1 projection cleanup

Layer / File(s) Summary
Register and verify migration 63
apps/server/src/persistence/Migrations.ts, apps/server/src/persistence/Migrations/063_DropUnreadV1Projections.ts, apps/server/src/persistence/Migrations/063_DropUnreadV1Projections.test.ts, apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts, apps/server/src/persistence/reconcileV2PreviewMigration.test.ts
Migration 63 is registered and drops six tables if they exist. Tests check the dropped and retained tables, preserved message data, and migration results.
Align mobile showcase seed
scripts/mobile-showcase-environment.ts, docs/operations/mobile-app-store-screenshots.md
The seed no longer creates turns, activities, or projector state, and reseeding only clears three importer-source tables. The screenshot instructions now describe persisted mobile-outbox tasks.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: maria-rcks, realhasanshoaib, juliusmarminge

Merge Risk: ⚪ Minimal · up to b9209

The checked upgrade and screenshot workflows remain consistent with the changes. No issue identified here needs resolution before merge.

Security architecture risk: 🔵 Low · up to b9209

The cleanup is narrowly scoped and preserves the tables used to import threads and messages. No new attacker-controlled entrypoint was identified. Remaining uncertainty concerns interrupted or concurrent migration recovery and compatibility with older operational tooling.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated destructive scope is six tables in the configured database undergoing migration. In the normal V1-to-V2 initialization path, the original state.sqlite is read-only and the cleanup targets its V2 copy; this is not evidence of a cross-service or tenant-wide operation.

Trust Boundaries and Controls

  • observed — The existing migration guard rejects histories that would skip required V2 schema migrations. Preview reconciliation also rejects unexpected later migrations before modifying their history. Neither control is weakened by this PR.

Resilience and Maintainability Implications

  • observed — The revised showcase seed still wraps retained-table deletion and insertion in BEGIN IMMEDIATE, commits on success, attempts rollback on failure without replacing the original error, and closes the connection in finally.

Hardening Proposals

  • proposed — Validate migration 063 with a file-backed interruption and concurrent-startup scenario that checks the six table states together with the migration marker, then retries. This would resolve the remaining recovery uncertainty without implying that a failure has been observed.
Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: removing unused V1 tables from the V2 database.
Description check Passed The description provides the problem, implementation details, linked issue, scope, detailed verification results, limitations, and tool attribution. It does not use the template headings, but the requ…
Linked Issues check Passed Issue #20922 requires V2 migration to remove the six unread V1 tables while retaining importer data. Migration 063 drops projection_thread_activities, projection_turns, `projection_thread_proposed…
Out of Scope Changes check Passed The migration, migration tests, manifest updates, and expectation updates directly implement issue #20922. The mobile showcase seed stops creating unread V1 rows, and the screenshot documentation upda…
Full details: Description check

Explanation

The description provides the problem, implementation details, linked issue, scope, detailed verification results, limitations, and tool attribution. It does not use the template headings, but the required information is substantially present.

Full details: Linked Issues check

Explanation

Issue #20922 requires V2 migration to remove the six unread V1 tables while retaining importer data. Migration 063 drops projection_thread_activities, projection_turns, projection_thread_proposed_plans, projection_pending_approvals, projection_state, and checkpoint_diff_blobs. Its test verifies that these tables are absent after migration and that importer source tables and message data remain. The migration is registered as version 63. The changes do not remove the original V1 state.sqlite, which the issue excludes.

Full details: Out of Scope Changes check

Explanation

The migration, migration tests, manifest updates, and expectation updates directly implement issue #20922. The mobile showcase seed stops creating unread V1 rows, and the screenshot documentation updates the related seeded-data description. These changes support the same storage and fixture behavior. No unrelated change is demonstrated.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Thanks for this, and for the before/after numbers. #20947 just landed on main with the same change (migration 063 dropping those six V1 projection tables) and closed #20922, so I'm closing this one as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Upgrading to V2 keeps the unread V1 activities table, so statev2.sqlite stays at the V1 size

2 participants