fix(db): keep the lock-free path for the transaction handle only (#408) #433

Merged

maximus merged 3 commits from issue-408-db-strict-lock into main 2026-10-14 00:28:08 +00:00

Conversation 4 Commits 3 Files changed 6 +556 -113

maximus commented 2026-10-12 23:32:42 +00:00

Owner

Copy link

Generated autonomously by /autopilot run of 2026-10-12

First link of the stack #408 → #409 → #389 → #395 (milestone planned-2026-10-10-fiabilite-tests). Issue #408.

What changes

src/services/db.ts:

• The global inTransaction flag is gone. A withTransaction callback receives its own transaction handle (typed Database) that runs directly on the connection; every other call, getDb() from a callback included, waits behind the transaction (strict, non-reentrant lock).
• The handle is revoked in the finally: a later call rejects with TransactionHandleRevokedError; close() on it always rejects with TransactionHandleCloseError.
• connectToProfile, initializeNewProfileDb and closeDb are each one runExclusive section, chaining closeUnlocked() → (repair_migrations, prepare_profile_db) → loadUnlocked(); prepare_profile_db runs only once the old close() resolved. The SQL of initializeNewProfileDb runs inside its section.
• Connection generation: every open and every close bumps it; the getDb() proxy and the transaction handle check it when the operation leaves the queue and reject with DbConnectionClosedError ("connection closed by profile switch").
• Blocking alert: an operation waiting more than BLOCKED_ALERT_MS (15 s) writes ONE logWarn, naming the holder (transaction, query, profile opening, profile initialization, connection close) and since when (ISO time). The code comment says a catch-up over 15 s yields an expected "profile opening" alert.
• close() on the getDb() proxy is routed to closeDb() (through the lock); no caller uses it today.

Other files:

• src/services/balance.service.ts: findOrCreateSecurity takes exec as a mandatory argument (no more exec ?? getDb()); its only production caller already passes it. Doc comment updated.
• src/services/onboardingService.ts: the now-wrong parenthesis about queries bypassing the lock is removed (the issue's L128-131, still there on 8ceb3de).
• docs/architecture.md: new ### Accès DB et transactions section at the end of « Base de données », and the db.ts row of the services table points to it.
• Tests: src/services/db.test.ts (the old « nested getDb() runs directly » test replaced), src/services/balance.service.test.ts (callers pass mockDb; the executor test asserts getDb is never called).

No CHANGELOG entry (internal robustness, nothing visible), no ADR, no migration (v1→v17).

Call sites re-checked on 8ceb3de

None of the 8 withTransaction callbacks reaches getDb(): balance.service x3 (proposeStarterAccountsInTransaction, the two snapshot saves through insertSnapshotLineWithHoldings → findOrCreateSecurity(…, exec)), onboardingService.initializeGuidedPath, categoryMigrationService.applyMigrationInTransaction, categorizationService.applyKeywordInTransaction, dataExportService.runRestore (all restore* helpers and attachTransactions take db; the file's getDb() calls are in the getExport* readers only). Service tests mock db.ts and do not see the lock: only db.test.ts does.

Tests added in db.test.ts

• a getDb() call from inside a callback waits until after the COMMIT (replaces the old nested-getDb test);
• a read launched from elsewhere after BEGIN ends after the COMMIT;
• the callback's own handle runs its statements without waiting;
• handle used after the callback (success and failure) → TransactionHandleRevokedError;
• close() on the handle → TransactionHandleCloseError, the pool is not closed;
• connectToProfile launched during a transaction closes only after the COMMIT;
• two concurrent connectToProfile → one pool open, the last file asked for;
• prepare_profile_db invoked after the old close() resolved;
• withTransaction queued behind connectToProfile → DbConnectionClosedError, callback never called, nothing run on either database;
• a query queued on an old getDb() handle behind a switch → DbConnectionClosedError;
• closeDb waits for the queue, then the old handle refuses to run;
• initializeNewProfileDb runs its SQL inside the section, before a queued read;
• alert (vitest fake timers): nothing at 14.999 s, one warning at 15 s naming the transaction and its start time, still one at 60 s; nothing for an operation that got the lock in time; a long catch-up is named "profile opening".

Gated tests release their transaction in finally, so a red assertion leaves no held lock (no cascade into later tests).

Mutation pass (one at a time, db.ts restored by git checkout after each, vitest run src/services/db.test.ts)

Mutation	rc	Red tests
Global inTransaction flag back (proxy runs directly during a callback)	1	3: getDb() from a callback waits; outside read after COMMIT; alert names the transaction
Revocation removed (active = false deleted)	1	2: handle revoked after success; after failure
Generation check removed	1	4: transaction queued behind a switch; query on old handle; closeDb then old handle; long catch-up (queued txn ran)
closeDb outside the section	1	1: closeDb waits for the queue
connectToProfile closes before entering the section	1	4: close after COMMIT; two concurrent switches; queued transaction; long catch-up
close() not awaited before prepare_profile_db	1	2: prepare after close resolved; closeDb ordering
Alert removed (logWarn call dropped)	1	2: alert naming the transaction; alert naming the profile opening
Handle close() passes through to the pool	1	1: refuses close() on the handle

The « red before » criterion (outside read during a transaction) is the first row: with the pre-#408 flag rule, the read runs between BEGIN and COMMIT.

Proofs

• npm run build green; npm test 115 files / 1978 tests green (worktree after npm ci).
• No Rust touched.

Decisions (autopilot, logged in the run's decisions log)

• [HIGH] Three named error classes, exported; handle methods reject (async) rather than throw synchronously.
• [HIGH] Generation bumped on close as well as open.
• [MEDIUM] close() on the getDb() proxy routed to closeDb() (through the lock) rather than straight to the pool or throwing; no caller today.
• [MEDIUM] Alert text in English, like the other Journal lines of db.ts; adds the getDb()-in-callback hint when a transaction holds the lock.
• [MEDIUM] findOrCreateSecurity tests pass mockDb explicitly.

For the reviewer

• The getDb() proxy now creates one setTimeout per queued operation (cleared as soon as it starts): the cost of the alert.
• A getDb() handle kept across a profile switch now rejects instead of silently hitting the new profile. ProfileContext reloads the page data after a switch through fresh getDb() calls; a component holding an old handle in a closure across the switch would now see DbConnectionClosedError in its catch.

🤖 Generated with Claude Code

Generated autonomously by /autopilot run of 2026-10-12 First link of the stack #408 → #409 → #389 → #395 (milestone `planned-2026-10-10-fiabilite-tests`). Issue #408. ## What changes `src/services/db.ts`: - The global `inTransaction` flag is gone. A `withTransaction` callback receives its own **transaction handle** (typed `Database`) that runs directly on the connection; every other call, `getDb()` from a callback included, waits behind the transaction (strict, non-reentrant lock). - The handle is revoked in the `finally`: a later call rejects with `TransactionHandleRevokedError`; `close()` on it always rejects with `TransactionHandleCloseError`. - `connectToProfile`, `initializeNewProfileDb` and `closeDb` are each **one** `runExclusive` section, chaining `closeUnlocked()` → (`repair_migrations`, `prepare_profile_db`) → `loadUnlocked()`; `prepare_profile_db` runs only once the old `close()` resolved. The SQL of `initializeNewProfileDb` runs inside its section. - Connection generation: every open and every close bumps it; the `getDb()` proxy and the transaction handle check it when the operation leaves the queue and reject with `DbConnectionClosedError` ("connection closed by profile switch"). - Blocking alert: an operation waiting more than `BLOCKED_ALERT_MS` (15 s) writes ONE `logWarn`, naming the holder (`transaction`, `query`, `profile opening`, `profile initialization`, `connection close`) and since when (ISO time). The code comment says a catch-up over 15 s yields an expected "profile opening" alert. - `close()` on the `getDb()` proxy is routed to `closeDb()` (through the lock); no caller uses it today. Other files: - `src/services/balance.service.ts`: `findOrCreateSecurity` takes `exec` as a mandatory argument (no more `exec ?? getDb()`); its only production caller already passes it. Doc comment updated. - `src/services/onboardingService.ts`: the now-wrong parenthesis about queries bypassing the lock is removed (the issue's L128-131, still there on 8ceb3de). - `docs/architecture.md`: new `### Accès DB et transactions` section at the end of « Base de données », and the `db.ts` row of the services table points to it. - Tests: `src/services/db.test.ts` (the old « nested getDb() runs directly » test replaced), `src/services/balance.service.test.ts` (callers pass `mockDb`; the executor test asserts `getDb` is never called). No CHANGELOG entry (internal robustness, nothing visible), no ADR, no migration (v1→v17). ## Call sites re-checked on 8ceb3de None of the 8 `withTransaction` callbacks reaches `getDb()`: `balance.service` x3 (`proposeStarterAccountsInTransaction`, the two snapshot saves through `insertSnapshotLineWithHoldings` → `findOrCreateSecurity(…, exec)`), `onboardingService.initializeGuidedPath`, `categoryMigrationService.applyMigrationInTransaction`, `categorizationService.applyKeywordInTransaction`, `dataExportService.runRestore` (all `restore*` helpers and `attachTransactions` take `db`; the file's `getDb()` calls are in the `getExport*` readers only). Service tests mock `db.ts` and do not see the lock: only `db.test.ts` does. ## Tests added in `db.test.ts` - a `getDb()` call from inside a callback waits until after the COMMIT (replaces the old nested-getDb test); - a read launched from elsewhere after BEGIN ends after the COMMIT; - the callback's own handle runs its statements without waiting; - handle used after the callback (success and failure) → `TransactionHandleRevokedError`; - `close()` on the handle → `TransactionHandleCloseError`, the pool is not closed; - `connectToProfile` launched during a transaction closes only after the COMMIT; - two concurrent `connectToProfile` → one pool open, the last file asked for; - `prepare_profile_db` invoked after the old `close()` resolved; - `withTransaction` queued behind `connectToProfile` → `DbConnectionClosedError`, callback never called, nothing run on either database; - a query queued on an old `getDb()` handle behind a switch → `DbConnectionClosedError`; - `closeDb` waits for the queue, then the old handle refuses to run; - `initializeNewProfileDb` runs its SQL inside the section, before a queued read; - alert (vitest fake timers): nothing at 14.999 s, one warning at 15 s naming the transaction and its start time, still one at 60 s; nothing for an operation that got the lock in time; a long catch-up is named "profile opening". Gated tests release their transaction in `finally`, so a red assertion leaves no held lock (no cascade into later tests). ## Mutation pass (one at a time, `db.ts` restored by `git checkout` after each, `vitest run src/services/db.test.ts`) | Mutation | rc | Red tests | |---|---|---| | Global `inTransaction` flag back (proxy runs directly during a callback) | 1 | 3: getDb() from a callback waits; outside read after COMMIT; alert names the transaction | | Revocation removed (`active = false` deleted) | 1 | 2: handle revoked after success; after failure | | Generation check removed | 1 | 4: transaction queued behind a switch; query on old handle; closeDb then old handle; long catch-up (queued txn ran) | | `closeDb` outside the section | 1 | 1: closeDb waits for the queue | | `connectToProfile` closes before entering the section | 1 | 4: close after COMMIT; two concurrent switches; queued transaction; long catch-up | | `close()` not awaited before `prepare_profile_db` | 1 | 2: prepare after close resolved; closeDb ordering | | Alert removed (`logWarn` call dropped) | 1 | 2: alert naming the transaction; alert naming the profile opening | | Handle `close()` passes through to the pool | 1 | 1: refuses close() on the handle | The « red before » criterion (outside read during a transaction) is the first row: with the pre-#408 flag rule, the read runs between BEGIN and COMMIT. ## Proofs - `npm run build` green; `npm test` 115 files / 1978 tests green (worktree after `npm ci`). - No Rust touched. ## Decisions (autopilot, logged in the run's decisions log) - [HIGH] Three named error classes, exported; handle methods reject (async) rather than throw synchronously. - [HIGH] Generation bumped on close as well as open. - [MEDIUM] `close()` on the `getDb()` proxy routed to `closeDb()` (through the lock) rather than straight to the pool or throwing; no caller today. - [MEDIUM] Alert text in English, like the other Journal lines of `db.ts`; adds the getDb()-in-callback hint when a transaction holds the lock. - [MEDIUM] `findOrCreateSecurity` tests pass `mockDb` explicitly. ## For the reviewer - The getDb() proxy now creates one `setTimeout` per queued operation (cleared as soon as it starts): the cost of the alert. - A `getDb()` handle kept across a profile switch now rejects instead of silently hitting the new profile. `ProfileContext` reloads the page data after a switch through fresh `getDb()` calls; a component holding an old handle in a closure across the switch would now see `DbConnectionClosedError` in its catch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

maximus added 3 commits 2026-10-12 23:32:43 +00:00

fix(db): keep the lock-free path for the transaction handle only ... 08946e6d08

The global inTransaction flag let every execute/select bypass the FIFO
lock while a withTransaction callback ran, wherever it came from. The
callback now receives its own handle, which runs directly, refuses
close() and throws once the callback settled; any other call, getDb()
from a callback included, queues behind the transaction.

connectToProfile, initializeNewProfileDb and closeDb each run as one
exclusive section (close the old pool, prepare, load), and every open
or close bumps a connection generation: an operation queued for a
closed connection throws "connection closed by profile switch". An
operation waiting more than 15 s for the lock writes one Journal
warning naming the holder and since when.

findOrCreateSecurity now requires its executor: its getDb() fallback
would deadlock if called from a transaction callback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

test(db): release gated transactions in finally so a red assertion holds no lock ... 8efe99aa20

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs(architecture): add the DB access and transactions section ... b161d54e3a

Strict lock, transaction handle, single section for a profile switch,
connection generation and blocking alert.

Resolves #408

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

maximus added the

autopilot:pending-human

label 2026-10-12 23:32:55 +00:00

maximus commented 2026-10-12 23:33:21 +00:00

Author

Owner

Copy link

Self-review (autopilot worker, not an independent review) — verdict: APPROVE

Diff read in full against main (6 files). Planning acceptance criteria of #408:

• Transaction handle typed Database, runs directly; close() → TransactionHandleCloseError; any call after the callback → TransactionHandleRevokedError (success and failure paths tested).
• connectToProfile, initializeNewProfileDb, closeDb: one runExclusive each, through closeUnlocked / loadUnlocked (never take the lock); prepare_profile_db after the old close() resolved (tested with a delayed close).
• Connection generation bumped on open and close, checked when the operation leaves the queue, for the getDb() proxy and the transaction handle; DbConnectionClosedError "connection closed by profile switch".
• Alert: one logWarn per waiting operation after 15 s, naming the holder and its start time; long catch-up case said in the code comment and tested.
• Tests listed by the issue present; old nested-getDb test replaced; fake timers for the alert.
• Mutation pass: 8 mutations (the 5 required plus 3), each red, table in the PR body.
• findOrCreateSecurity exec mandatory; wrong comment of onboardingService.ts removed; architecture section created.

Checks: security (no secrets, Journal lines name no paths), correctness vs the 8 withTransaction sites (none reaches getDb(), re-verified on 8ceb3de), no SQL migration touched, no UI text (no i18n impact), no CHANGELOG entry (internal, as the issue says), docs rule applied. npm run build green, npm test 115 files / 1978 tests green.

Points for the independent reviewer: one setTimeout per queued operation (cleared at start); a getDb() handle kept across a profile switch now rejects instead of hitting the new profile; close() on the getDb() proxy now goes through closeDb() (MEDIUM decision, no caller today).

**Self-review (autopilot worker, not an independent review) — verdict: APPROVE** Diff read in full against `main` (6 files). Planning acceptance criteria of #408: - [x] Transaction handle typed `Database`, runs directly; `close()` → `TransactionHandleCloseError`; any call after the callback → `TransactionHandleRevokedError` (success and failure paths tested). - [x] `connectToProfile`, `initializeNewProfileDb`, `closeDb`: one `runExclusive` each, through `closeUnlocked` / `loadUnlocked` (never take the lock); `prepare_profile_db` after the old `close()` resolved (tested with a delayed close). - [x] Connection generation bumped on open and close, checked when the operation leaves the queue, for the `getDb()` proxy and the transaction handle; `DbConnectionClosedError` "connection closed by profile switch". - [x] Alert: one `logWarn` per waiting operation after 15 s, naming the holder and its start time; long catch-up case said in the code comment and tested. - [x] Tests listed by the issue present; old nested-getDb test replaced; fake timers for the alert. - [x] Mutation pass: 8 mutations (the 5 required plus 3), each red, table in the PR body. - [x] `findOrCreateSecurity` `exec` mandatory; wrong comment of `onboardingService.ts` removed; architecture section created. Checks: security (no secrets, Journal lines name no paths), correctness vs the 8 `withTransaction` sites (none reaches `getDb()`, re-verified on 8ceb3de), no SQL migration touched, no UI text (no i18n impact), no CHANGELOG entry (internal, as the issue says), docs rule applied. `npm run build` green, `npm test` 115 files / 1978 tests green. Points for the independent reviewer: one `setTimeout` per queued operation (cleared at start); a `getDb()` handle kept across a profile switch now rejects instead of hitting the new profile; `close()` on the `getDb()` proxy now goes through `closeDb()` (MEDIUM decision, no caller today).

maximus commented 2026-10-12 23:35:02 +00:00

Author

Owner

Copy link

CI lue par l’orchestrateur de l’autopilot (2026-10-12) sur la tête b161d54 : PR Check — Frontend run 529 vert (Node v24.21.0, build vite, Test Files 115 passed (115)). Pas de run check-rust attendu (aucun fichier sous src-tauri/).

CI lue par l’orchestrateur de l’autopilot (2026-10-12) sur la tête `b161d54` : `PR Check — Frontend` run 529 vert (Node `v24.21.0`, build vite, `Test Files 115 passed (115)`). Pas de run `check-rust` attendu (aucun fichier sous `src-tauri/`).

maximus referenced this pull request 2026-10-12 23:45:21 +00:00

test: shared React hook harness for the jsdom-less tests (#409) #434

maximus referenced this pull request 2026-10-12 23:59:23 +00:00

test(i18n): one marked fake t for the suite, and a guard (#389) #435

maximus referenced this pull request 2026-10-13 00:26:09 +00:00

feat(profiles): name the copy of a stopped catch-up, one copy per profile, startup error cleared (#395) #436

maximus commented 2026-10-13 00:56:43 +00:00

Author

Owner

Copy link

/pr-review — PR #433 (#408) — Verdict : APPROVE

Résumé. Le verrou devient strict et non réentrant : le rappel reçoit son propre handle, révoqué au finally, et les trois fonctions de profil sont chacune une seule section exclusive, avec un contrôle de génération. Le code tient ce que dit la description. J'ai rejoué la suite et mes propres mutations sur la tête b161d54, dans une copie hors dépôt.

Ce que j'ai vérifié

• Suite : copie de la tête par git archive, puis npm ci. npm run build est vert et vitest run passe : 115 fichiers, 1978 tests. La CI de la tête est verte (PR Check — Frontend). Rust n'est pas touché, donc pas de check-rust.
• Aucun rappel n'atteint getDb() : j'ai tracé les appels de façon statique, par transitivité, à travers tout src/services/*.ts. Les racines sont les 5 fonctions *InTransaction et runRestore, et l'inline de initializeGuidedPath a été lu à la main. Aucune n'atteint getDb(). Les restore* et attachTransactions n'appellent que db.execute, toSqlFlag et notifyDataChanged. tsc confirme que tous les appelants de findOrCreateSecurity passent exec.
• Mutations du relecteur (sur une copie de db.ts, une à la fois, fichier restauré et vérifié par cmp) :

Mutation	Tests rouges
Alerte sans garde started ni clearTimeout	3
initializeNewProfileDb passe son SQL par le proxy (interblocage dans la section)	13 (timeouts)
Le rappel reçoit le proxy sérialisé au lieu du handle	28
closeUnlocked n'incrémente plus la génération	1
withTransaction relit rawInstance et la génération à l'exécution (plus de capture à l'appel)	2
BLOCKED_ALERT_MS à 16 s	2
lockHolder jamais remis à null	0 (survit)
loadUnlocked n'incrémente plus la génération	0 (survit, redondant avec la fermeture)
Le handle de transaction ne vérifie plus la génération	0 (survit, inatteignable)

Ces trois survivants ne cachent pas de défaut de comportement (voir suggestions 1 et 2).

• Fermeture : Resolves #408 figure dans le message du commit 08946e6, pas dans le corps de la PR. La timeline de #408 porte seulement des pull_ref en ref_action: none (#433, #434, #435). Un merge en avance rapide de la pile fermera donc #408 par le message de commit, ce qui est le comportement voulu.

Suggestions (non bloquantes)

1. lockHolder n'est épinglé par aucun test du chemin « behind the queue ». Si on ne le remet jamais à null, l'alerte nomme un détenteur périmé, et aucun test ne rougit. Un test suffit : une opération longue se termine, puis une attente sans détenteur. Ce serait une épingle à ajouter en suivi, pas un tour de plus.
2. Contrôle de génération dans transactionHandle : inatteignable aujourd'hui. Seules les sections exclusives touchent à la génération, et aucune ne peut tourner pendant que la transaction tient le verrou. Le garder comme défense est raisonnable, mais il ne faut pas le compter comme couvert. L'incrément dans loadUnlocked est de même redondant avec celui de closeUnlocked.
3. close() sur un ancien proxy de getDb() ferme la connexion courante. serialize route close vers closeDb() sans vérifier handleGeneration. Un handle gardé avant un changement de profil fermerait donc la base du profil suivant. Aucun appelant aujourd'hui. Il vaudrait mieux rejeter avec DbConnectionClosedError quand la génération ne correspond plus, par cohérence avec execute et select.
4. closeUnlocked met l'état à null avant await closing.close(). Si close() rejette, connectToProfile rejette sans rouvrir, et l'app reste sans connexion (SET_ERROR dans ProfileContext). Avant, l'ancienne instance restait référencée. C'est un cas marginal, mais un try/finally autour du close() dans connectToProfile (ouvrir quand même) suivrait la logique « échec ouvert » du reste de la fonction.
5. Texte de l'alerte. L'indice « a getDb() call inside a withTransaction callback deadlocks » s'affiche pour toute transaction de plus de 15 s, par exemple une restauration volumineuse ou une migration de catégories sur un gros profil. Formuler en cause possible (« possible cause: … ») éviterait de lancer une fausse piste en lisant le Journal.

Actions faites

• Revue postée (ce commentaire). Label de #408 : status:review remplacé par status:approved.
• Copie hors dépôt (scratchpad/pr433, sauvegarde db.ts.orig, scripts calls433.py et mut433.py) supprimée en fin de revue.

## /pr-review — PR #433 (#408) — Verdict : **APPROVE** **Résumé.** Le verrou devient strict et non réentrant : le rappel reçoit son propre handle, révoqué au `finally`, et les trois fonctions de profil sont chacune une seule section exclusive, avec un contrôle de génération. Le code tient ce que dit la description. J'ai rejoué la suite et mes propres mutations sur la tête `b161d54`, dans une copie hors dépôt. ### Ce que j'ai vérifié - **Suite** : copie de la tête par `git archive`, puis `npm ci`. `npm run build` est vert et `vitest run` passe : 115 fichiers, 1978 tests. La CI de la tête est verte (`PR Check — Frontend`). Rust n'est pas touché, donc pas de `check-rust`. - **Aucun rappel n'atteint `getDb()`** : j'ai tracé les appels de façon statique, par transitivité, à travers tout `src/services/*.ts`. Les racines sont les 5 fonctions `*InTransaction` et `runRestore`, et l'inline de `initializeGuidedPath` a été lu à la main. Aucune n'atteint `getDb()`. Les `restore*` et `attachTransactions` n'appellent que `db.execute`, `toSqlFlag` et `notifyDataChanged`. `tsc` confirme que tous les appelants de `findOrCreateSecurity` passent `exec`. - **Mutations du relecteur** (sur une copie de `db.ts`, une à la fois, fichier restauré et vérifié par `cmp`) : | Mutation | Tests rouges | |---|---| | Alerte sans garde `started` ni `clearTimeout` | 3 | | `initializeNewProfileDb` passe son SQL par le proxy (interblocage dans la section) | 13 (timeouts) | | Le rappel reçoit le proxy sérialisé au lieu du handle | 28 | | `closeUnlocked` n'incrémente plus la génération | 1 | | `withTransaction` relit `rawInstance` et la génération à l'exécution (plus de capture à l'appel) | 2 | | `BLOCKED_ALERT_MS` à 16 s | 2 | | `lockHolder` jamais remis à `null` | **0** (survit) | | `loadUnlocked` n'incrémente plus la génération | **0** (survit, redondant avec la fermeture) | | Le handle de transaction ne vérifie plus la génération | **0** (survit, inatteignable) | Ces trois survivants ne cachent pas de défaut de comportement (voir suggestions 1 et 2). - **Fermeture** : `Resolves #408` figure dans le message du commit `08946e6`, pas dans le corps de la PR. La timeline de #408 porte seulement des `pull_ref` en `ref_action: none` (#433, #434, #435). Un merge en avance rapide de la pile fermera donc #408 par le message de commit, ce qui est le comportement voulu. ### Suggestions (non bloquantes) 1. **`lockHolder` n'est épinglé par aucun test du chemin « behind the queue ».** Si on ne le remet jamais à `null`, l'alerte nomme un détenteur périmé, et aucun test ne rougit. Un test suffit : une opération longue se termine, puis une attente sans détenteur. Ce serait une épingle à ajouter en suivi, pas un tour de plus. 2. **Contrôle de génération dans `transactionHandle` : inatteignable aujourd'hui.** Seules les sections exclusives touchent à la génération, et aucune ne peut tourner pendant que la transaction tient le verrou. Le garder comme défense est raisonnable, mais il ne faut pas le compter comme couvert. L'incrément dans `loadUnlocked` est de même redondant avec celui de `closeUnlocked`. 3. **`close()` sur un ancien proxy de `getDb()` ferme la connexion courante.** `serialize` route `close` vers `closeDb()` sans vérifier `handleGeneration`. Un handle gardé avant un changement de profil fermerait donc la base du profil suivant. Aucun appelant aujourd'hui. Il vaudrait mieux rejeter avec `DbConnectionClosedError` quand la génération ne correspond plus, par cohérence avec `execute` et `select`. 4. **`closeUnlocked` met l'état à `null` avant `await closing.close()`.** Si `close()` rejette, `connectToProfile` rejette sans rouvrir, et l'app reste sans connexion (`SET_ERROR` dans `ProfileContext`). Avant, l'ancienne instance restait référencée. C'est un cas marginal, mais un `try/finally` autour du `close()` dans `connectToProfile` (ouvrir quand même) suivrait la logique « échec ouvert » du reste de la fonction. 5. **Texte de l'alerte.** L'indice « a getDb() call inside a withTransaction callback deadlocks » s'affiche pour toute transaction de plus de 15 s, par exemple une restauration volumineuse ou une migration de catégories sur un gros profil. Formuler en cause possible (« possible cause: … ») éviterait de lancer une fausse piste en lisant le Journal. ### Actions faites - Revue postée (ce commentaire). Label de #408 : `status:review` remplacé par `status:approved`. - Copie hors dépôt (`scratchpad/pr433`, sauvegarde `db.ts.orig`, scripts `calls433.py` et `mut433.py`) supprimée en fin de revue.

maximus referenced this pull request from a commit 2026-10-13 23:58:13 +00:00

fix(db): follow-ups of the review of PR #433

maximus referenced this pull request 2026-10-13 23:58:57 +00:00

feat(profiles): name the copy of a stopped catch-up, one copy per profile, startup error cleared (#395) #436

maximus commented 2026-10-13 23:59:03 +00:00

Author

Owner

Copy link

Suivis de cette revue appliqués en tête de pile, PR #436, commit 74fae14 (détail dans le commentaire 23535 de la PR #436, non relu).

Suivis de cette revue appliqués en tête de pile, PR #436, commit `74fae14` (détail dans le commentaire 23535 de la PR #436, non relu).

maximus merged commit b161d54e3a into main 2026-10-14 00:28:08 +00:00

maximus referenced this pull request 2026-10-14 00:29:49 +00:00

Suivis des revues de la pile #433 à #436 (fiabilité des tests) : relecture des commits de suivi, épingles manquantes #437

maximus referenced this pull request 2026-10-14 00:29:58 +00:00

feat(profiles): name the copy of a stopped catch-up, one copy per profile, startup error cleared (#395) #436

maximus referenced this pull request 2026-10-18 12:22:26 +00:00

docs(review): REVIEW.md, calibrated risk levels and review rules per zone (#456) #460

maximus referenced this pull request 2026-10-18 12:43:22 +00:00

docs(review): REVIEW.md, calibrated risk levels and review rules per zone (#456) #460

maximus referenced this pull request 2026-10-19 00:02:08 +00:00

test: pin the surviving marked-t keys and document the known test limits (#437) #473

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

autopilot:pending-human

PR produced by /autopilot — awaits human review/merge

  

source:analyste

Créée par le Super-Analyste

  

source:defenseur

Créée par l Escouade Défenseur

  

source:human

Créée par un humain

  

source:medic

Créée par le Medic (auto-détection)

  

status:approved

PR approuvée, prête à merger

  

status:blocked

Bloquée (question, dépendance, clarification)

  

status:in-progress

Un agent (Medic/Dev) travaille dessus

  

status:needs-clarification

Issue blocked by ambiguity — Max needs to clarify before re-running autopilot

  

status:needs-fix

Reviewer demande des corrections

  

status:ready

Analysée et documentée, prête à prendre

  

status:review

PR créée, en attente du Reviewer

  

status:triage

Nouvelle issue, pas encore analysée

  

type:bug

Correction de bug → Medic

  

type:feature

Nouvelle fonctionnalité → Dev

  

type:infra

Config, CI/CD, déploiement → Dev

  

type:refactor

Refactoring / amélioration technique → Dev

  

type:schema

Modèle de données / migrations → Dev

  

type:security

Issue de sécurité → Medic ou Dev

No labels

autopilot:pending-human

source:analyste

source:defenseur

source:human

source:medic

status:approved

status:blocked

status:in-progress

status:needs-clarification

status:needs-fix

status:ready

status:review

status:triage

type:bug

type:feature

type:infra

type:refactor

type:schema

type:security

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: maximus/Simpl-Resultat#433

No description provided.