fix(sqlite): serialize all writers through the engine write lock - #321
Open
yesyayen wants to merge 4 commits into
Open
fix(sqlite): serialize all writers through the engine write lock#321yesyayen wants to merge 4 commits into
yesyayen wants to merge 4 commits into
Conversation
yesyayen
marked this pull request as ready for review
August 27, 2026 19:35
yesyayen
requested review from
LeeroyHannigan,
amrith,
c33howard,
jcshepherd,
pdf-amzn and
robinnsc
as code owners
August 27, 2026 19:35
| //! writer holding the file lock fails a plain `PutItem` with exactly the | ||
| //! `InternalServerError` seen in the `run-integration-sqlite` CI flake. | ||
| //! | ||
| //! Deliberate exclusions from the lock, so the invariant stays auditable: |
Collaborator
There was a problem hiding this comment.
Should-fix set_backfilling and mark_active in data/vector_index.rs still run UPDATE vector_indexes on the engine pool with no write lock (their siblings backfill_batch and reset_data_table on the same struct take it), so a slow locked commit can exhaust their busy_timeout and fail a build step. Take the lock in both, or add them to this exclusion list so the invariant stays auditable.
Collaborator
Author
There was a problem hiding this comment.
good call! Confirmed and fixed: both flips now take the write lock, with mark_active holding it for the whole method.
This finding is also a fair point about the pattern itself: the lock is a per-site convention today. Planning a follow-up that packages lock + BEGIN IMMEDIATE behind a single write_tx() helper so an unserialized write transaction cannot be expressed; kept out of this PR to avoid scope creep.
Design decision D1 says the engine serializes all writers through write_lock, so SQLITE_BUSY cannot arise from competing writers. Five writers ran outside the lock: CreateTable's two transactions, the TTL metadata and index DDL, tagging, the table-size refresh, and the stream-record and idempotency-token cleanup sweeps. Any of them can hold the SQLite write lock past a locked writer's 5s busy_timeout when a commit is slow, and the locked writer then fails an unrelated request with 'database is locked', which the engine maps to a 500. Measured on 2026-08-27 against a local server: an uncoordinated writer holding the file lock fails a plain PutItem with the exact InternalServerError seen in the run-integration-sqlite flake on main (commit e41f370), including the server-side 'database is locked' line. Also add a syslog dump step to the integration jobs: devtools/run-tests restarts the server daemonized, so its logs go to syslog and a failing job otherwise records no server-side evidence.
…lock Each test holds the engine write lock, spawns one of the previously unserialized writers, asserts it makes no progress while the lock is held, and asserts it completes after release. Failing-first verified: with the lock removed from create_table_impl and the idempotency cleanup, the matching tests fail with 'completed while the engine write lock was held'.
Review round 1 findings on the D1 serialization fix: - delete_backup ran two unlocked writes on the engine pool, and the backup_items delete is a bulk statement (one row per backed-up item), which is exactly the slow-commit shape the parent commit eliminates. Wire-reachable through DeleteBackup and automatically after every point-in-time restore. - update_continuous_backups ran an unlocked upsert. Same class, one small row; the lock is scoped so the trailing read-only describe runs after release. - The store.rs module doc now names the deliberate exclusions (init-time bootstrap, the management stores on the separate catalog pool) and the residual same-file contention risk that pool carries. - The test helper comment claimed a 2-connection pool; in-memory engines are pinned to a single connection, so the comment now describes the actual mechanism. Two new D1 tests cover the backup writers. Failing-first verified: with the locks removed, both fail with 'completed while the engine write lock was held'.
PR review finding: set_backfilling and mark_active ran UPDATE vector_indexes on the engine pool with no write lock, while their siblings backfill_batch and reset_data_table on the same struct take it. A slow locked commit could exhaust their busy_timeout and fail a build step. mark_active takes the lock for the whole method, so its empty-flip branch no longer re-acquires (that would deadlock). One new test pins both flips, failing-first verified for each independently.
yesyayen
force-pushed
the
fix/sqlite-writer-serialization
branch
from
August 28, 2026 18:33
7da76e5 to
e7846e2
Compare
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
2 participants
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.
What
The SQLite backend's concurrency design (D1,
crates/storage-sqlite/src/store.rs) serializes every writer through the enginewrite_lockand states thatSQLITE_BUSYcannot arise from competing writers. Seven writers ran outside the lock:create_table_impl(catalog + data-table DDL, two transactions)create_ttl_index(CREATE INDEXover a populated data table) + TTL metadatatag_resource/untag_resourcerefresh_table_size(UPDATE)delete_backup(bulk DELETE, one row per backed-up item) +update_continuous_backupsAn unserialized writer with a slow commit holds the SQLite file lock past a locked writer's 5s
busy_timeout; the locked writer then fails an unrelated request withdatabase is locked, which the engine maps to a 500. This is therun-integration-sqlitefailure on main (run 33078534390): a plainPutItemon a fresh table returnedInternalServerErrorand took 7s where neighbors take under 1s (the busy-timeout signature).All seven writers now take the lock. Contention queues on the in-process mutex, which waits without failing.
Also: the integration jobs now dump the server syslog on failure (
journalctl -t extenddb).devtools/run-testsrestarts the server daemonized, so its logs go to syslog and the original failure left zero server-side evidence in CI.Why
Reproduced the mechanism locally at
e41f370: an uncoordinated writer holding the file lock fails a plainPutItemwith the byte-identical client error, and the server logsstorage internal error internal_error=error returned from database: (code: 5) database is locked.Closes # n/a
Testing done
write_lock, spawn the writer, assert no progress until release. Failing-first verified: with the lock removed fromcreate_table_impl, the idempotency cleanup,delete_backup, andupdate_continuous_backups, the matching tests fail withcompleted while the engine write lock was held (D1 violation).create_tablereturns; the PITR path acquires and releases per step).Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-disk format, or public CLI surface, an RFC has been accepted or is linked below. Otherwise, an ADR captures the decision (link below).ADR / RFC: n/a (enforces the existing D1 design decision; no wire, trait, or on-disk change)
Breaking changes
n/a
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache License 2.0 and I agree to the Developer Certificate of Origin (DCO). See CONTRIBUTING.md for details.