diff --git a/docs/architecture.md b/docs/architecture.md index 2d3d8cb..59114fb 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -148,9 +148,9 @@ Index Bilan (9) — 7 ajoutés en v9 : - **Verrou strict, non réentrant (#408).** Le rappel de `withTransaction` reçoit un **handle de transaction** (typé `Database`) qui exécute directement sur la connexion. Toute autre requête attend derrière la transaction, y compris un `getDb()` appelé depuis le rappel ou depuis une fonction qu'il appelle : celui-ci attend la transaction qui l'attend, un interblocage. Règle : dans un rappel, n'utiliser **que** le handle reçu et le passer aux fonctions appelées (par exemple `exec`, obligatoire sur `findOrCreateSecurity`). Avant #408, un drapeau global laissait passer hors verrou toute requête lancée pendant un rappel, d'où qu'elle vienne. - **Handle révoqué.** À la fin du rappel (succès ou erreur), le handle est révoqué : un appel tardif depuis une promesse orpheline lève `TransactionHandleRevokedError` au lieu de passer hors verrou. `close()` sur le handle lève toujours `TransactionHandleCloseError`. -- **Changement de profil dans une seule section.** `connectToProfile`, `initializeNewProfileDb` et `closeDb` sont chacun **un seul** `runExclusive` : fermeture de l'ancien pool, puis `repair_migrations` et `prepare_profile_db` (pour `connectToProfile`, appelé seulement après la fin du `close()`), puis `Database.load` et la bascule. Ils attendent donc les opérations déjà en file et une transaction en cours, et rien ne s'exécute entre la fermeture et l'ouverture. Les helpers internes (`closeUnlocked`, `loadUnlocked`) ne prennent jamais le verrou. Pendant un rattrapage long, toute la file attend : rien d'utile ne peut tourner pendant la bascule. -- **Génération de connexion.** Chaque ouverture et chaque fermeture incrémentent un numéro de génération. Le proxy de `getDb()` et le handle de transaction le vérifient au moment où l'opération sort de la file : une opération mise en file pour une connexion fermée entre-temps lève `DbConnectionClosedError` (« connection closed by profile switch »), au lieu de tourner sur la base du profil suivant ou sur un pool fermé. Un handle obtenu par `getDb()` avant un changement de profil ne sert donc plus après : le reprendre par `getDb()`. -- **Alerte de blocage.** Une opération qui attend le verrou depuis plus de `BLOCKED_ALERT_MS` (15 s) écrit **un** `logWarn` au Journal, qui nomme ce qui tient le verrou (`transaction`, `query`, `profile opening`, `profile initialization`, `connection close`) et depuis quand. Cause typique : un `getDb()` dans un rappel. Un rattrapage de migrations de plus de 15 s produit une alerte « profile opening », attendue. `logService` n'écrit qu'en mémoire et en `sessionStorage`, jamais en base : l'appel est sûr pendant que le verrou est tenu. +- **Changement de profil dans une seule section.** `connectToProfile`, `initializeNewProfileDb` et `closeDb` sont chacun **un seul** `runExclusive` : fermeture de l'ancien pool, puis `repair_migrations` et `prepare_profile_db` (pour `connectToProfile`, appelé seulement après la fin du `close()`), puis `Database.load` et la bascule. Ils attendent donc les opérations déjà en file et une transaction en cours, et rien ne s'exécute entre la fermeture et l'ouverture. Les helpers internes (`closeUnlocked`, `loadUnlocked`) ne prennent jamais le verrou. Pendant un rattrapage long, toute la file attend : rien d'utile ne peut tourner pendant la bascule. Un `close()` de l'ancien pool qui échoue ne bloque pas la bascule : l'état est vidé et la génération incrémentée avant le `close()`, l'échec part au Journal (`logWarn`) et le profil suivant s'ouvre quand même. `closeDb()` seul se résout aussi dans ce cas, sans connexion, et tout ancien handle refuse de tourner. +- **Génération de connexion.** Chaque ouverture et chaque fermeture incrémentent un numéro de génération. Le proxy de `getDb()` et le handle de transaction le vérifient au moment où l'opération sort de la file : une opération mise en file pour une connexion fermée entre-temps lève `DbConnectionClosedError` (« connection closed by profile switch »), au lieu de tourner sur la base du profil suivant ou sur un pool fermé. Un handle obtenu par `getDb()` avant un changement de profil ne sert donc plus après : le reprendre par `getDb()`. Son `close()` est vérifié de la même façon et lève `DbConnectionClosedError`, au lieu de fermer la connexion du profil courant. Dans le handle de transaction, la vérification est une défense, inatteignable aujourd'hui : rien ne ferme la connexion pendant qu'une transaction tient le verrou. +- **Alerte de blocage.** Une opération qui attend le verrou depuis plus de `BLOCKED_ALERT_MS` (15 s) écrit **un** `logWarn` au Journal, qui nomme ce qui tient le verrou (`transaction`, `query`, `profile opening`, `profile initialization`, `connection close`) et depuis quand. L'alerte cite un `getDb()` dans un rappel comme cause possible, pas comme diagnostic : une transaction longue (grosse restauration, migration de catégories sur un gros profil) dépasse aussi 15 s. Un rattrapage de migrations de plus de 15 s produit une alerte « profile opening », attendue. `logService` n'écrit qu'en mémoire et en `sessionStorage`, jamais en base : l'appel est sûr pendant que le verrou est tenu. - **Tests.** Seul `src/services/db.test.ts` voit le verrou (un `Database` factice qui journalise le début et la fin de chaque opération). Les tests de services mockent `db.ts` (`withTransaction: fn => fn(await getDb())`) et ne le voient pas. ## Système de migrations diff --git a/src/services/db.test.ts b/src/services/db.test.ts index 50fd13e..dabc4f6 100644 --- a/src/services/db.test.ts +++ b/src/services/db.test.ts @@ -322,6 +322,65 @@ describe("profile switch goes through the lock (#408)", () => { await expect(db.execute("AFTER")).rejects.toBeInstanceOf(DbConnectionClosedError); }); + it("close() on a getDb() handle kept from the previous profile rejects and keeps the current one open", async () => { + const before = await getDb(); + const next = makeFakeDb("next"); + h.fakeDb = next; + await connectToProfile("next.db"); + events.length = 0; + await expect(before.close()).rejects.toBeInstanceOf(DbConnectionClosedError); + expect(next.close).not.toHaveBeenCalled(); + await (await getDb()).execute("STILL OPEN"); + expect(next.execute).toHaveBeenCalledWith("STILL OPEN", undefined); + }); + + it("close() on the current getDb() handle closes the connection", async () => { + const current = h.fakeDb; + await (await getDb()).close(); + expect(current.close).toHaveBeenCalledTimes(1); + await expect(getDb()).rejects.toThrow(/No database connection/); + }); + + it("a failing close of the old pool does not prevent the switch, and goes to the Journal", async () => { + vi.mocked(logWarn).mockClear(); + const old = h.fakeDb; + const before = await getDb(); + vi.mocked(old.close).mockImplementation(async () => { + throw new Error("close refused for /home/someone/.config/x.db"); + }); + const next = makeFakeDb("next"); + h.fakeDb = next; + await connectToProfile("next.db"); + expect(old.close).toHaveBeenCalledTimes(1); + expect(logWarn).toHaveBeenCalledWith( + "DB: closing the previous connection failed, continuing without it: close refused for x.db" + ); + await (await getDb()).execute("ON NEXT"); + expect(next.execute).toHaveBeenCalledWith("ON NEXT", undefined); + await expect(before.execute("ON OLD")).rejects.toBeInstanceOf(DbConnectionClosedError); + expect(old.execute).not.toHaveBeenCalled(); + }); + + it("closeDb with a failing close resolves, warns, and leaves a coherent state without connection", async () => { + vi.mocked(logWarn).mockClear(); + const old = h.fakeDb; + const before = await getDb(); + vi.mocked(old.close).mockImplementation(async () => { + throw new Error("close refused"); + }); + await expect(closeDb()).resolves.toBeUndefined(); + expect(logWarn).toHaveBeenCalledWith("DB: closing the previous connection failed, continuing without it: close refused"); + // No connection, and the generation moved: the old handle refuses to run. + await expect(getDb()).rejects.toThrow(/No database connection/); + await expect(withTransaction(async () => undefined)).rejects.toThrow(/No database connection/); + await expect(before.execute("ON OLD")).rejects.toBeInstanceOf(DbConnectionClosedError); + expect(old.execute).not.toHaveBeenCalled(); + // The old pool is no longer referenced: the next open does not close it again. + h.fakeDb = makeFakeDb("next"); + await connectToProfile("next.db"); + expect(old.close).toHaveBeenCalledTimes(1); + }); + it("initializeNewProfileDb runs its SQL inside the section, before a queued query", async () => { const next = makeFakeDb("new"); h.fakeDb = next; @@ -371,6 +430,8 @@ describe("blocking alert (#408)", () => { const message = String(vi.mocked(logWarn).mock.calls[0][0]); expect(message).toContain("a query has waited 15 s"); expect(message).toContain(`held by a transaction since ${startedAt}`); + // A cause the reader may check, not a diagnosis: a long restore also waits 15 s. + expect(message).toContain("(possible cause: a getDb() call inside a withTransaction callback"); // Once per operation: still one warning much later. await vi.advanceTimersByTimeAsync(BLOCKED_ALERT_MS * 3); expect(logWarn).toHaveBeenCalledTimes(1); @@ -382,6 +443,32 @@ describe("blocking alert (#408)", () => { } }); + it("says the lock is free, not who held it last, when the check lands between two operations", async () => { + // In the app only microtasks separate the end of one operation from the + // start of the next, so a timer cannot fire there; the test puts the check + // there to pin that the holder is cleared when an operation ends. + const db = await getDb(); + let release: () => void = () => {}; + const gate = new Promise((resolve) => { + release = resolve; + }); + vi.mocked(h.fakeDb.execute).mockImplementationOnce(async () => { + await gate; + return { rowsAffected: 0 }; + }); + const first = db.execute("LONG"); + const waiting = db.select("WAITING"); + // Registered after the queue's own reaction: runs once the first operation + // ended (holder cleared), before the waiting one starts. + const checked = first.then(() => vi.advanceTimersByTime(BLOCKED_ALERT_MS)); + release(); + await checked; + await vi.advanceTimersByTimeAsync(10); + await waiting; + expect(logWarn).toHaveBeenCalledTimes(1); + expect(String(vi.mocked(logWarn).mock.calls[0][0])).toBe("DB lock: a query has waited 15 s behind the queue"); + }); + it("says nothing for an operation that got the lock in time", async () => { const db = await getDb(); const run = db.execute("QUICK"); diff --git a/src/services/db.ts b/src/services/db.ts index 54a92ff..48b25d2 100644 --- a/src/services/db.ts +++ b/src/services/db.ts @@ -41,6 +41,11 @@ let serializedInstance: Database | null = null; // generation when its operation leaves the queue: an operation queued for a // connection that was closed meanwhile throws `DbConnectionClosedError` // rather than running on the next profile's database or on a closed pool. +// `close()` on a `getDb()` handle is checked the same way, so a handle kept +// from a previous profile cannot close the current one. +// - A failing `close()` of the old pool never blocks a switch: the state is +// already cleared and the generation bumped, the failure goes to the Journal +// and the next profile still opens (see `closeUnlocked`). // --------------------------------------------------------------------------- /** An operation queued for a connection closed before it could run. */ @@ -77,10 +82,13 @@ type LockKind = /** * An operation still waiting for the lock after this delay writes ONE warning - * to the Journal, naming what holds the lock and since when. The typical cause - * is a `getDb()` call from inside a `withTransaction` callback (deadlock). A - * profile catch-up (`prepare_profile_db`) that takes longer also triggers it, - * naming a "profile opening": expected, nothing can run during a switch. + * to the Journal, naming what holds the lock and since when. One possible + * cause is a `getDb()` call from inside a `withTransaction` callback + * (deadlock), but a long transaction (a large restore, a category migration on + * a big profile) also exceeds the delay, so the warning names it as a possible + * cause only. A profile catch-up (`prepare_profile_db`) that takes longer also + * triggers it, naming a "profile opening": expected, nothing can run during a + * switch. * `logWarn` only writes to memory and sessionStorage, never to the DB, so it * is safe to call while the lock is held. */ @@ -102,7 +110,7 @@ function runExclusive(kind: LockKind, fn: () => Promise): Promise { holder ? `DB lock: a ${kind} has waited ${waited} s, the lock is held by a ${holder.kind} since ${new Date(holder.since).toISOString()}` + (holder.kind === "transaction" - ? " (a getDb() call inside a withTransaction callback deadlocks: use the callback's handle)" + ? " (possible cause: a getDb() call inside a withTransaction callback, which deadlocks; the callback must use its own handle)" : "") : `DB lock: a ${kind} has waited ${waited} s behind the queue` ); @@ -157,7 +165,13 @@ function serialize(db: Database, handleGeneration: number): Database { }); } if (prop === "close") { - return () => closeDb(); + // Same check as execute/select: a handle kept from a previous profile + // must not close the current profile's connection (#408 review). + return () => + runExclusive("connection close", async () => { + assertGeneration(handleGeneration); + await closeUnlocked(); + }); } const value = Reflect.get(target, prop, receiver); return typeof value === "function" ? value.bind(target) : value; @@ -169,6 +183,11 @@ function serialize(db: Database, handleGeneration: number): Database { * The handle a `withTransaction` callback receives: runs directly on the * connection (the transaction already holds the lock), refuses `close()`, and * throws once `revoke()` ran at the end of the callback. + * + * Its generation check is a defence, not reachable today: only the exclusive + * profile sections change the generation, and none of them can run while a + * transaction holds the lock, so nothing closes the connection under a + * running callback. It stays in case a future path closes without the lock. */ function transactionHandle( db: Database, @@ -247,13 +266,29 @@ export async function withTransaction( // Internal helpers for the profile sections below. They NEVER take the lock: // only call them from inside a `runExclusive` section. +/** + * Drop the current connection, then close its pool. Never rejects: the state + * is cleared and the generation bumped BEFORE `close()`, so a failing close + * leaves a coherent "no connection" state (every older handle refuses to run) + * and only writes a warning to the Journal. A switch therefore still opens the + * next profile, and `closeDb()` resolves: the app no longer references the old + * pool, and keeping it would leave the app without any usable connection. + */ async function closeUnlocked(): Promise { if (rawInstance) { const closing = rawInstance; rawInstance = null; serializedInstance = null; generation += 1; - await closing.close(); + try { + await closing.close(); + } catch (e) { + logWarn( + withoutFolders( + `DB: closing the previous connection failed, continuing without it: ${e instanceof Error ? e.message : String(e)}` + ) + ); + } } }