fix: bulk-submit(sqlite): import aborts with 'database is locked' at high file concurrency with the full search-parameter registry - #946
Open
eterceros wants to merge 2 commits into
Conversation
At HFS_BULK_SUBMIT_FILE_CONCURRENCY=8 on SQLite with the full search-parameter registry, fanned-out ingest batches queue behind the single write lock long enough to outlast busy_timeout, and the manifest bookkeeping UPDATE aborts the whole import with "database is locked". Two combinable guards: - Clamp the effective file concurrency to SQLITE_MAX_FILE_CONCURRENCY (2) when the primary backend is SQLite, with a startup log stating configured vs effective; concurrent-writer backends keep the configured value. - Retry the SQLite bookkeeping writes (manifest counts, submission updated_at, progress, bytes) on SQLITE_BUSY/LOCKED with bounded backoff instead of aborting the manifest: a busy attempt never acquired the write lock so reissuing is safe, and the lease's worker_id/fencing_token still fences stale writers. Busy now classifies via classify_sqlite_error (Unavailable/503) instead of collapsing to Internal. Closes #942 Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
angela-helios
left a comment
angela-helios
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full diff and ran the suite locally on the branch: the 49 sqlite bulk_submit tests (including the three new retry tests) and the clamp tests in config.rs all pass.
The design looks right to me, and it matches Steve's point about SQLite being a single-user engine: rather than fighting the single-writer model, the clamp respects it, and the retry only hardens the one bookkeeping write that today aborts a whole manifest over a transient lock. The two care points from the issue are handled — a busy UPDATE in autocommit applied nothing so reissuing the increment is safe, and the affected == 0 fencing check still runs after the retry, so a stale lease still loses.
Two minor notes, neither blocking:
- The per-file status writes (
SubmitFileRecordupdates) are not wrapped inretry_bookkeeping_on_busy. With the clamp at 2 the exposure drops a lot, but if your extra testing shows that site failing under contention, it deserves the same treatment. - Once #944 lands (partial indexes shorten the write-lock holds ~40% measured), it may be worth re-evaluating whether
SQLITE_MAX_FILE_CONCURRENCYcan go to 3–4. Not for this PR — just leaving the breadcrumb.
LGTM once you finish your testing and mark it ready.
The bulk-submit smoke flow only existed inline in the CI workflow, so there was no reusable way to reproduce the SQLite fan-out behaviour by hand. These three scripts stand up a throwaway HFS plus a static Data Provider on free ports above 18000, all under `timeout`, so nothing has to be killed afterwards. They live under crates/hfs/tests/bulk_submit/ to match the existing per-area harnesses (audit, bulk_export, cluster, subscriptions). Unlike those, they start their own server rather than driving an external one, hence `_check` instead of `run_external_*_smoke`. Free ports are found by attempting a real bind instead of parsing netstat: netstat prints LISTEN on Linux and LISTENING on Windows, and is frequently absent altogether, so a pattern miss would silently report every port as free and only fail later at startup. The SQLite database is a file rather than `:memory:`. An in-memory database cannot honour `PRAGMA journal_mode = WAL`, and without WAL the contention surfaces as table-level SQLITE_LOCKED, which does not represent a real deployment. Co-Authored-By: Claude <noreply@anthropic.com>
eterceros
force-pushed
the
fix/942-bulk-submit-sqlite-import
branch
from
September 5, 2026 01:51
6b3dcde to
92508f5
Compare
eterceros
marked this pull request as ready for review
September 5, 2026 01:57
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.
Closes #942
Fix scoped to two localized changes in the bulk-submit worker: clamp effective file concurrency when the primary backend is SQLite, and add a bounded SQLITE_BUSY retry around the idempotent update_manifest_progress bookkeeping write so a contended counter update no longer aborts the whole manifest. BackendKind and StorageBackendMode::primary_backend_kind() already exist and only need plumbing through spawn_submit_workers, so no new abstractions, schema changes, or migrations are required. Main care points are emitting a startup log line explaining the clamp and ensuring the retry does not swallow LeaseError::LeaseLost fencing failures.
PR auto-generated by claude-agent; fix implemented with Claude Code and validated with the Playwright e2e suite.