Repository navigation
fix(queuefs): serve list_unacked in id order so a large queue cannot fail startup - #6151
Tong-bit-art wants to merge 1 commit into
Conversation
…fail startup
list_unacked filters status IN ('pending','processing') and orders by id, but
idx_queue_status is (queue_name, status, id). With two status values that index
cannot deliver rows in id order, so SQLite inserts a sorter:
SEARCH queue_messages USING INDEX idx_queue_status (queue_name=? AND status=?)
USE TEMP B-TREE FOR ORDER BY
Once the unacknowledged set is large enough the sorter spills to a temporary
file, and with no writable temp directory list_unacked fails with 'unable to
open database file' even though the database is intact. Task recovery snapshots
every queue before any consumer starts, so that aborts server startup, and the
queue only drains while the server runs, so it never heals on its own.
Add idx_queue_unacked on (queue_name, id) so that query is served in id order
without a sorter. The single-status queries keep using idx_queue_status, so no
existing plan changes.
The index needs its own name: CREATE INDEX IF NOT EXISTS cannot redefine an
existing index, so changing the column order of idx_queue_status would silently
not apply to databases that already have it, which is exactly the population
hitting this failure. Verified against SQLite via EXPLAIN QUERY PLAN for a fresh
database and for one migrated from the old schema.
Fixes volcengine#6127
|
Heads-up on the It is the OpenCode state-retention flake, not this change: That is a Node test under It is also already diagnosed. #5408 documents the identical TypeError:
and has the fix open ( On the Rust buildWorth recording that |
|
Correction to my earlier note: after re-triggering, Effective check status now (newest run per check name): The single remaining red is the fork-PR credential issue, same as on #5683: repository Nothing else on this PR is red. |
This branch has not been deployed
2 participants
Description
list_unackedfiltersstatus IN ('pending','processing')and orders byid, butidx_queue_statusis(queue_name, status, id). With two status values that indexcannot deliver rows in
idorder, so SQLite inserts a sorter:Once the unacknowledged set is large enough the sorter spills to a temporary file, and
with no writable temp directory
list_unackedfails withunable to open database fileeven though the database is intact. Task recovery snapshots every queue before any
consumer starts, so that aborts startup — and since a queue only drains while the server
runs, the state never heals on its own.
Add
idx_queue_unackedon(queue_name, id)so that query is served inidorderwithout a sorter.
Refs #6127
Human Involvement
An AI agent located the defect, produced the analysis and authored the change. The root
cause, the fix shape and the choice to verify at the SQL level before submitting were
directed and reviewed by the human author. See Additional Notes for what could not be
verified locally.
Related Issue
Fixes #6127
Type of Change
Changes Made
crates/ragfs/src/plugins/queuefs/backend.rs: addidx_queue_unackedon(queue_name, id)to the idempotent migration list, with a comment recording why theexisting index cannot serve this query and why the new index needs its own name.
crates/ragfs/src/plugins/queuefs/backend.rs: extend the existing migration test toalso assert the new index exists.
No query is changed.
idx_queue_statusis kept — it is still the best plan for the foursingle-status queries (
dequeue next,peek,count pending,last enqueue).Testing
Why the fix needs a new index name
This is the part worth reviewing carefully.
run_migrationsuses idempotent statementsthat tolerate
already exists. Changing the column order ofidx_queue_statuswouldtherefore be a no-op on any database that already has it — which is precisely the
population hitting this failure. The new index must have its own name so
CREATE INDEX IF NOT EXISTSactually creates it on an existing install.The same reasoning rules out dropping and recreating the old index on every startup:
that rebuilds an index over the whole queue table on each launch.
Validation actually run
I could not compile Rust here (see the limitation below), so I verified the change at the
SQL level instead, driving real SQLite with the migration statements parsed out of the
modified Rust file so the strings under test are the ones in the diff:
Reproduction of the sorter matches the
EXPLAIN QUERY PLANoutput in #6127.Limitation, stated plainly
I could not compile or run the Rust test suite in my environment — no
cargo, and noC++/CMake toolchain for this workspace. So:
string literals in the existing assertion, which I reviewed by hand but did not compile;
cargo test -p ragfsandcargo fmt --checkhave not been run by me.check-depstreatscrates/as a dependency change, so thebuildjob should compilethis. I would rather say this up front than imply a green run I did not perform.
Checklist
Additional Notes
Owner module per the CONTRIBUTING routing table: Storage —
crates/ragfs*→ @baojun-zhang.Scope is one defect. I did not touch the error text:
unable to open database fileisSQLite's own message surfacing through
sqlite list unacked row error, and renaming itwould change behaviour beyond this fix. It may be worth a follow-up so the message names
the sorter rather than the database, but that is a separate change.
I also deliberately did not drop
idx_queue_status. Reordering it looked like thesmaller diff until the migration path was checked;
/explainshows the four single-statusqueries still prefer it.
Diff is +14/-2 in one file.