also looking at this
fix(app): an installed database gains the columns a new build needs
#43
4 files
+440
-37
Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.
| @@ -477,19 +477,6 @@ export async function startDesktopAgent(options: { version: string }): Promise<D | ||
| 477 | 477 | : automation?.detail ?? 'Uplink asks for this once the steps above are done.'), |
| 478 | 478 | informational: false, |
| 479 | 479 | }, |
| 480 | // Only when there is something to say. An intact database is not a step | |
| 481 | // anyone performs, and a broken one is not a reason to call the app | |
| 482 | // unready: it is still reading texts and still answering them, it just | |
| 483 | // will not remember any of it. See `SqliteStore.schemaError`. | |
| 484 | ...(store.schemaError | |
| 485 | ? [{ | |
| 486 | id: 'database', | |
| 487 | name: 'History', | |
| 488 | ok: false, | |
| 489 | detail: store.schemaError, | |
| 490 | informational: true, | |
| 491 | }] | |
| 492 | : []), | |
| 493 | 480 | ] |
| 494 | 481 | // An engine the person has not selected must not hold the app in setup. |
| 495 | 482 | const ready = checks.every(check => check.ok || check.informational) |
| @@ -513,6 +500,11 @@ export async function startDesktopAgent(options: { version: string }): Promise<D | ||
| 513 | 500 | }, |
| 514 | 501 | engines: allEngines(config).map(engine => ({ id: engine.id, label: engine.label })), |
| 515 | 502 | lastError: uplink?.lastError ?? null, |
| 503 | // Not a check: a broken database is not a setup step anyone performs, | |
| 504 | // and it must not hold the app in `setup` when it is still answering | |
| 505 | // texts. The popover shows this in every state, which a check row cannot | |
| 506 | // do. See `SqliteStore.schemaError`. | |
| 507 | notice: store.schemaError, | |
| 516 | 508 | signIn: signIn.state, |
| 517 | 509 | } |
| 518 | 510 | } |
| @@ -1,6 +1,7 @@ | ||
| 1 | 1 | import type { ConversationRecord, RunRecord, Store } from './store' |
| 2 | 2 | import { Database } from 'bun:sqlite' |
| 3 | 3 | import { mkdirSync } from 'node:fs' |
| 4 | import { homedir } from 'node:os' | |
| 4 | 5 | import { dirname } from 'node:path' |
| 5 | 6 | |
| 6 | 7 | /** |
| @@ -17,8 +18,13 @@ import { dirname } from 'node:path' | ||
| 17 | 18 | * copy: the first text after the update would fail on the INSERT, for every |
| 18 | 19 | * existing user, on every message. That is why the tables are described as |
| 19 | 20 | * data below rather than as one block of SQL. The same description creates a |
| 20 | * new database and tells an old one which columns it is missing, so adding a | |
| 21 | * column means editing `TABLES`, and there is no second place to forget. | |
| 21 | * new database and tells an old one which columns it is missing. | |
| 22 | * | |
| 23 | * `TABLES` is not the only place a `runs` column is named: `RUN_COLUMNS` maps | |
| 24 | * it to its field and `recentRuns` reads it back. TypeScript ties | |
| 25 | * `RUN_COLUMNS` to `RunRecord` but cannot tie either to `TABLES`, so a test | |
| 26 | * does it instead. Adding a column means editing both, and the suite says so | |
| 27 | * rather than a customer's Mac. | |
| 22 | 28 | */ |
| 23 | 29 | |
| 24 | 30 | export interface RunRow extends RunRecord { |
| @@ -91,7 +97,7 @@ export const TABLES: TableSpec[] = [ | ||
| 91 | 97 | }, |
| 92 | 98 | ] |
| 93 | 99 | |
| 94 | const RUN_COLUMNS: Record<keyof RunRecord, string> = { | |
| 100 | export const RUN_COLUMNS: Record<keyof RunRecord, string> = { | |
| 95 | 101 | chatGuid: 'chat_guid', |
| 96 | 102 | messageGuid: 'message_guid', |
| 97 | 103 | prompt: 'prompt', |
| @@ -111,19 +117,24 @@ const RUN_COLUMNS: Record<keyof RunRecord, string> = { | ||
| 111 | 117 | * Bring a database up to the shape `TABLES` describes: create the tables it |
| 112 | 118 | * does not have, and add to the ones it does any column it is missing. |
| 113 | 119 | * |
| 114 | * Forward only, and idempotent, so it runs on every start and costs one | |
| 115 | * `PRAGMA table_info` per table once a database is current. The whole thing is | |
| 116 | * one transaction, which in SQLite covers the `ALTER TABLE`s and the version | |
| 117 | * stamp together: a database is either fully updated or untouched, never left | |
| 118 | * halfway by a Mac that went to sleep. | |
| 120 | * Forward only, and idempotent, so it runs on every start. The work is decided | |
| 121 | * by reading first, and a database that is already current returns before the | |
| 122 | * transaction opens. That matters because opening one takes a write lock, and | |
| 123 | * during an update two Uplinks hold the same file for a moment: a start that | |
| 124 | * wrote on every launch would lose that race, and the caller answers a lost | |
| 125 | * race by abandoning the file. Almost every launch is the no-work case. | |
| 126 | * | |
| 127 | * When there is work it is one transaction, which in SQLite covers the | |
| 128 | * `ALTER TABLE`s and the version stamp together: a database is either fully | |
| 129 | * updated or untouched, never left halfway by a Mac that went to sleep. | |
| 119 | 130 | */ |
| 120 | 131 | export function migrate(db: Database, tables: TableSpec[] = TABLES): void { |
| 132 | if (isCurrent(db, tables)) | |
| 133 | return | |
| 134 | ||
| 121 | 135 | db.transaction(() => { |
| 122 | 136 | for (const table of tables) { |
| 123 | // Empty means the table does not exist, which is also how a brand new | |
| 124 | // file answers, so one query covers both cases. | |
| 125 | const present = new Set((db.query(`PRAGMA table_info(${table.name})`).all() as Array<{ name: string }>) | |
| 126 | .map(column => column.name)) | |
| 137 | const present = columnsOf(db, table.name) | |
| 127 | 138 | const columns = Object.entries(table.columns) |
| 128 | 139 | |
| 129 | 140 | if (present.size === 0) { |
| @@ -140,6 +151,26 @@ export function migrate(db: Database, tables: TableSpec[] = TABLES): void { | ||
| 140 | 151 | })() |
| 141 | 152 | } |
| 142 | 153 | |
| 154 | /** | |
| 155 | * Whether the database already has every table and column `tables` names, and | |
| 156 | * carries this build's version. Reads only, so it is safe to ask while another | |
| 157 | * process is writing. | |
| 158 | */ | |
| 159 | function isCurrent(db: Database, tables: TableSpec[]): boolean { | |
| 160 | if (schemaVersion(db) !== SCHEMA_VERSION) | |
| 161 | return false | |
| 162 | return tables.every((table) => { | |
| 163 | const present = columnsOf(db, table.name) | |
| 164 | return present.size > 0 && Object.keys(table.columns).every(name => present.has(name)) | |
| 165 | }) | |
| 166 | } | |
| 167 | ||
| 168 | /** Empty means the table does not exist, which is also how a new file answers. */ | |
| 169 | function columnsOf(db: Database, table: string): Set<string> { | |
| 170 | return new Set((db.query(`PRAGMA table_info(${table})`).all() as Array<{ name: string }>) | |
| 171 | .map(column => column.name)) | |
| 172 | } | |
| 173 | ||
| 143 | 174 | export function schemaVersion(db: Database): number { |
| 144 | 175 | return (db.query('PRAGMA user_version').get() as { user_version: number }).user_version |
| 145 | 176 | } |
| @@ -165,7 +196,13 @@ export class SqliteStore implements Store { | ||
| 165 | 196 | this.db = open(path) |
| 166 | 197 | } |
| 167 | 198 | catch (error) { |
| 168 | this.schemaError = `${message(error)}. Uplink is answering texts but not saving them.` | |
| 199 | // Written for whoever reads it in the popover, not for whoever wrote | |
| 200 | // this: what is happening, and the one thing they can do about it. | |
| 201 | this.schemaError = [ | |
| 202 | 'Uplink is answering texts but not saving them.', | |
| 203 | `Its history file could not be opened (${message(error)}).`, | |
| 204 | `Quit and reopen Uplink to try again, or delete ${tilde(path)} to start a fresh history.`, | |
| 205 | ].join(' ') | |
| 169 | 206 | this.db = open(':memory:') |
| 170 | 207 | } |
| 171 | 208 | } |
| @@ -249,9 +286,13 @@ export class SqliteStore implements Store { | ||
| 249 | 286 | function open(path: string): Database { |
| 250 | 287 | const db = new Database(path, { create: true }) |
| 251 | 288 | try { |
| 252 | // A second Uplink reading the same file is the normal case during an | |
| 253 | // update, and WAL is what lets it read while this one writes. | |
| 289 | // A second Uplink on the same file is the normal case during an update. | |
| 290 | // WAL is what lets it read while this one writes, and the timeout is what | |
| 291 | // stops the one real upgrade from failing because the other one had the | |
| 292 | // lock for a moment. Without it SQLite gives up at once and the caller | |
| 293 | // reads that as a broken file. | |
| 254 | 294 | db.exec('PRAGMA journal_mode = WAL;') |
| 295 | db.exec('PRAGMA busy_timeout = 5000;') | |
| 255 | 296 | migrate(db) |
| 256 | 297 | return db |
| 257 | 298 | } |
| @@ -264,3 +305,8 @@ function open(path: string): Database { | ||
| 264 | 305 | function message(error: unknown): string { |
| 265 | 306 | return error instanceof Error ? error.message : String(error) |
| 266 | 307 | } |
| 308 | ||
| 309 | function tilde(path: string): string { | |
| 310 | const home = homedir() | |
| 311 | return path.startsWith(`${home}/`) ? `~${path.slice(home.length)}` : path | |
| 312 | } | |
| @@ -7,7 +7,7 @@ import { join } from 'node:path' | ||
| 7 | 7 | import { loadConfig } from '../../../app/Uplink/config' |
| 8 | 8 | import { MessagesDb } from '../../../app/Uplink/messages-db' |
| 9 | 9 | import { cleanToken, readSettings, settingsEnv, tokenLooksValid, writeSettings } from '../../../app/Uplink/settings' |
| 10 | import { migrate, schemaVersion, SqliteStore, TABLES } from '../../../app/Uplink/sqlite-store' | |
| 10 | import { migrate, RUN_COLUMNS, schemaVersion, SqliteStore, TABLES } from '../../../app/Uplink/sqlite-store' | |
| 11 | 11 | import { Uplink } from '../../../app/Uplink/uplink' |
| 12 | 12 | import { FakeChatDb, FakeEngine, FakeSender, ME, settle } from './fixtures' |
| 13 | 13 | |
| @@ -90,6 +90,23 @@ function columnsOf(path: string, table: string): string[] { | ||
| 90 | 90 | return names |
| 91 | 91 | } |
| 92 | 92 | |
| 93 | /** | |
| 94 | * Compared as a set, not a list. `ALTER TABLE ADD COLUMN` appends, so a column | |
| 95 | * written into the middle of `TABLES` is last on an upgraded database and | |
| 96 | * mid-list on a new one. Both are correct, and an order-sensitive assertion | |
| 97 | * would fail the upgrade that worked. | |
| 98 | */ | |
| 99 | function sameColumns(path: string, table: { name: string, columns: Record<string, string> }): void { | |
| 100 | expect([...columnsOf(path, table.name)].sort()).toEqual(Object.keys(table.columns).sort()) | |
| 101 | } | |
| 102 | ||
| 103 | function versionOf(path: string): number { | |
| 104 | const db = new Database(path) | |
| 105 | const version = schemaVersion(db) | |
| 106 | db.close() | |
| 107 | return version | |
| 108 | } | |
| 109 | ||
| 93 | 110 | describe('SqliteStore migrations', () => { |
| 94 | 111 | it('creates a database that is already current', () => { |
| 95 | 112 | const path = join(tempDir(), 'uplink.sqlite') |
| @@ -100,9 +117,7 @@ describe('SqliteStore migrations', () => { | ||
| 100 | 117 | for (const table of TABLES) |
| 101 | 118 | expect(columnsOf(path, table.name)).toEqual(Object.keys(table.columns)) |
| 102 | 119 | |
| 103 | const db = new Database(path) | |
| 104 | expect(schemaVersion(db)).toBeGreaterThan(0) | |
| 105 | db.close() | |
| 120 | expect(versionOf(path)).toBeGreaterThan(0) | |
| 106 | 121 | }) |
| 107 | 122 | |
| 108 | 123 | /** |
| @@ -164,12 +179,63 @@ describe('SqliteStore migrations', () => { | ||
| 164 | 179 | used.run('INSERT INTO runs (chat_guid, message_guid, prompt) VALUES (?, ?, ?)', ['g', 'm', 'p']) |
| 165 | 180 | used.close() |
| 166 | 181 | |
| 182 | expect(versionOf(path)).toBe(0) | |
| 183 | ||
| 167 | 184 | const store = new SqliteStore(path) |
| 168 | 185 | expect(store.schemaError).toBeNull() |
| 169 | 186 | store.close() |
| 170 | 187 | |
| 171 | 188 | for (const table of TABLES) |
| 172 | expect(columnsOf(path, table.name)).toEqual(Object.keys(table.columns)) | |
| 189 | sameColumns(path, table) | |
| 190 | // Stamped on an upgraded database, not only on a new one. Without this a | |
| 191 | // migration could leave the version at 0 and every other test stay green. | |
| 192 | const fresh = join(tempDir(), 'fresh.sqlite') | |
| 193 | new SqliteStore(fresh).close() | |
| 194 | expect(versionOf(path)).toBe(versionOf(fresh)) | |
| 195 | }) | |
| 196 | ||
| 197 | /** | |
| 198 | * The claim in this file's header is that a column is added in one place. | |
| 199 | * TypeScript only half enforces it: `RUN_COLUMNS` is a `Record` over | |
| 200 | * `RunRecord`, so those two cannot drift, but nothing ties either to | |
| 201 | * `TABLES`. A column named in one and not the other is exactly #9 again, so | |
| 202 | * it is pinned here instead of in the type system. | |
| 203 | */ | |
| 204 | it('names the same runs columns in TABLES and in RUN_COLUMNS', () => { | |
| 205 | const declared = Object.keys(TABLES.find(table => table.name === 'runs')!.columns) | |
| 206 | // `id` is the key SQLite assigns; nothing writes it. | |
| 207 | expect(Object.values(RUN_COLUMNS).sort()).toEqual(declared.filter(name => name !== 'id').sort()) | |
| 208 | }) | |
| 209 | ||
| 210 | /** | |
| 211 | * During an update two Uplinks hold this file for a moment, and the loser of | |
| 212 | * a write lock abandons it for an in-memory database and loses the history. | |
| 213 | * A launch that changes nothing must therefore not ask for a write lock, and | |
| 214 | * almost every launch changes nothing. | |
| 215 | * | |
| 216 | * Not covered here: the launch that does have work to do and meets a lock. | |
| 217 | * That one waits on `PRAGMA busy_timeout`, and proving it needs a second | |
| 218 | * process, because SQLite's wait blocks this one's event loop. | |
| 219 | */ | |
| 220 | it('opens a current database while another process is writing to it', async () => { | |
| 221 | const path = join(tempDir(), 'uplink.sqlite') | |
| 222 | new SqliteStore(path).close() | |
| 223 | ||
| 224 | const other = new Database(path) | |
| 225 | other.exec('BEGIN IMMEDIATE') | |
| 226 | other.run('INSERT INTO state (key, value) VALUES (?, ?)', ['cursor', '1']) | |
| 227 | ||
| 228 | // Opening is the assertion: it neither threw nor fell back to memory. | |
| 229 | const store = new SqliteStore(path) | |
| 230 | expect(store.schemaError).toBeNull() | |
| 231 | ||
| 232 | other.exec('ROLLBACK') | |
| 233 | other.close() | |
| 234 | ||
| 235 | // And it is the real file, not a replacement that forgets on quit. | |
| 236 | expect(await store.createRun({ chatGuid: 'g', messageGuid: 'm', prompt: 'p', cwd: '/tmp', status: 'queued' })).toBeGreaterThan(0) | |
| 237 | store.close() | |
| 238 | expect(new SqliteStore(path).recentRuns()[0]).toMatchObject({ prompt: 'p' }) | |
| 173 | 239 | }) |
| 174 | 240 | |
| 175 | 241 | it('runs twice over the same database without changing it', () => { |