feat(db): make SQLite durability configurable, defaulting to today's behaviour - #173
Merged
Merged
Conversation
…behaviour SqliteDatabase sets journal_mode=WAL, foreign_keys and busy_timeout, but never sets `synchronous` -- so it stays at SQLite's default of FULL, which fsyncs on every commit. Measured on a copy of a real library, 300 single-row inserts each committed on its own: median 0.427ms at FULL against 0.035ms at NORMAL, p95 0.811ms against 0.121ms. Roughly 12x shorter commits on this machine. That matters more than the raw numbers suggest, because all database work is serialised through one lock: a shorter write is also a shorter time during which every read is blocked. The default stays FULL. NORMAL cannot corrupt the database -- WAL guarantees consistency either way -- but a power loss or OS crash can lose the most recent transactions: a just-written image record or queue status, not the image file itself, which the orphan scan can recover. That is a durability decision for whoever runs the server, not one to make for them on upgrade. `off` and `extra` are deliberately not offered: `off` can corrupt the database on an OS crash, and `extra` costs more than `full` for a guarantee this application does not need. The Literal is closed, which is also why the PRAGMA can interpolate the value -- PRAGMA takes no bind parameters. The wiring is covered by its own test. A config field and a working PRAGMA can both be correct while nothing connects them, and that failure is silent: the app boots, every other test passes, and the setting does nothing. It happened once while writing this.
…pt-in # Conflicts: # invokeai/frontend/web/openapi.json
- Regenerate `docs/src/generated/settings.json`. Adding a field to `InvokeAIAppConfig` without it fails `check-docs-data`, which is `generate-docs-data && git diff --exit-code -- src/generated` behind the `invokeai/app/**` path filter — it never ran here because the PR is a draft, and would have fired on the first push after that. The two `models/.…` path defaults are left with forward slashes: regenerating on Windows rewrites them to backslashes, which is itself a diff on the Linux runner. - The durability page claimed a lost image row "can be recovered by the orphan scan". It cannot: `clean_orphaned_disk_files` archives record-less files out of `outputs/`, and re-importing them takes `import_images.py`. The claim was the argument for `normal` being cheap, so it mattered. - The PRAGMA comment still quoted the superseded 6x figure while the field description, the docs page and the API schema all say 12x. - The eight user-management CLI entry points opened the database without the setting, so it silently did not apply there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The wiring test stubbed `SqliteDatabase`, `SqliteMigrator` and `build_migrations` and asserted the keyword arrived at the constructor. That pins the argument's name, not the outcome: an implementation that accepts `synchronous` and then applies it conditionally passes it unchanged. Now it calls the real `init_db` against an in-memory database, migrations and all, and reads `PRAGMA synchronous` off the connection the app ends up with. Mutation-tested both ways — dropping the wiring fails it, and so does accepting the keyword while applying it only for on-disk databases, which the stub version did not catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pfannkuchensack
marked this pull request as ready for review
September 10, 2026 19:53
Pfannkuchensack
requested review from
JPPhoto,
blessedcoolant and
lstein
as code owners
September 10, 2026 19:53
Pfannkuchensack
enabled auto-merge
September 10, 2026 19:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Feature, opt-in, default unchanged.
SqliteDatabase.__init__setsjournal_mode=WAL,foreign_keysandbusy_timeout, but never setssynchronous— so it stays at SQLite's own default ofFULL, which fsyncs on every commit. This adds adb_synchronousconfig setting so an operator can choosenormalinstead.Measured on a copy of a real library (939 images), 300 single-row inserts each committed on its own:
fullnormalRoughly 12× shorter commits at the median on this machine (NVMe SSD; the effect is storage-dependent). That matters more than the raw numbers suggest:
SqliteDatabaseholds one connection and oneRLockthrough which all database work is serialised, so a shorter write is also a shorter window during which every read is blocked.The default stays
full.normalcannot corrupt the database — WAL guarantees a consistent database either way — but a power loss or OS crash can lose the most recent transactions: a just-written image record or a queue status, not the image file itself, which the orphan scan can recover. That is a durability decision for whoever runs the server, and making it for them on upgrade would be wrong.offandextraare deliberately not offered.offcan corrupt the database on an OS crash, andextracosts more thanfullfor a guarantee this application does not need. TheLiteralis closed, which is also why the PRAGMA interpolates its value directly —PRAGMAtakes no bind parameters, and no user-supplied string can reach that statement.The setting is documented in
configuration/invokeai-yaml.mdxalongside the other operational settings, with the trade-off stated rather than buried.Related Issues / Discussions
From the local
.ideas/sql-gallery-indizes-und-db-tuning.md§3. The numbers here were re-measured on this machine rather than taken from that document, which recorded ~6.4× on different hardware.QA Instructions
What I ran:
tests/app/services/shared/sqlite/test_sqlite_synchronous.py— 15 tests: the config default,normalaccepted, the other real SQLite values (off,extra) and malformed input rejected, the PRAGMA reaching the connection for each setting, in-memory databases behaving the same, WAL still on (without it the trade would be a different one), and the wiring.tests/test_config.py::test_db_synchronous_defaults_to_full_and_loads_from_yaml— default plus a YAML round trip, matching the pattern of the neighbouring settings.normal(2 failures), deleting the PRAGMA statement (2), widening theLiteralto includeoff(1), dropping the argument ininit_db(2).tests/app/services/shared+tests/test_config.py— 180 passed.pytest --collect-onlyclean.ruff check/format --checkclean.openapi.jsonandschema.tsregenerated the way CI does. The diff is the new field plus its line in theInvokeAIAppConfigdocstring — additive, with a default, so nothing existing changes shape.To verify by hand: set
db_synchronous: normalininvokeai.yaml, start the server, and check the connection agrees:PRAGMA synchronous; -- 2 = full (default), 1 = normalOne finding worth naming, because it is the kind that does not announce itself: at one point the config field and the PRAGMA were both correct while nothing connected them. The app booted, every test passed, and the setting silently did nothing.
TestTheSettingReachesTheDatabaseexists specifically for that — it stubsSqliteDatabaseand assertsinit_dbpasses the configured value through.Merge Plan
Nothing special. No DB schema, no migration, no redux slice, no dependency change. The
openapi.jsonchange is additive.Independent of #171 and #172, which are in flight in parallel — disjoint files, any merge order.
Checklist
invokeai-yaml.mdxWhat's Newcopy (if doing a release after this PR) — optional; the setting is opt-in and changes nothing by default🤖 Generated with Claude Code