Repository navigation
fix(vector): take the writer lock before delete_field removes a vector field, so a cancelled call removes nothing - #1563
Merged
Conversation
…r field, so a cancelled call removes nothing VectorStore::delete_field removed the field from the index and then awaited the writer lock. A delete_field dropped there, as when a laurus-server client disconnects while a commit holds the writer, left the field in the engine's schema but not in the index, and the next put of the field failed after its WAL append, which stopped writes. - delete_field takes the writer lock before the index changes and holds it to the end, as add_field (since #1545) and rebuild_field do. A failed commit still keeps the writer and returns Ok (#1535). - Engine tests drop delete_field and add_field of a vector field while they wait for the writer, and check that the field did not change and that a later put of it is applied without stopping writes. - persistence.md (English and Japanese) and the rustdoc of add_field and delete_field say that a cancelled call changes no field. Closes #1530
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
Closes #1530. Refs #1545, #1535, #1517.
VectorStore::delete_fieldnow takes the vector writer lock before it removes the field from the index, asadd_fieldhas done since #1545. Adelete_fieldwhose future is dropped while a commit holds the writer, as when a laurus-server client disconnects, therefore removes nothing, and a later put of the field is applied instead of stopping writes.Problem
VectorStore::delete_fieldremoved the field from the index (vector/store.rs:1743on3e7249d6) and then awaited the writer lock (vector/store.rs:1754).Engine::delete_fieldremoves the field from its schema only after the store returns. A scratch engine test on3e7249d6(a Flat fieldvecwith one committed document; the test holds the writer, as a running commit does, and dropsdelete_field("vec")while it waits) showed:vec; the index no longer did.vecfailed after its WAL append withunknown vector field 'vec': no index configured for it, which stopped writes: a put without a vector then failed withWritesStopped. The same happened when a commit had replaced the cached writer first.vec, replayed the put.The Issue's other two cases, a cancelled
add_fieldand a failed commit inadd_field, were fixed by #1545, which madeVectorStore::add_fieldtake the lock before it changes the index and return the commit's error first. No test cancelled anadd_field, so this PR adds one.Every other
VectorStoremethod that takes the writer lock (commit_with_checkpoint,optimize,rebuild_field,close) takes it before it changes anything.Changes
VectorStore::delete_fieldtakes the writer lock first and holds it while it removes the field from the index, unregisters the field's base weight and embedder, and commits and drops the cached writer. A failed removal returns before anything changes. A failed commit still keeps the writer and returnsOk, as fix(engine): re-adding a removed vector field brings back documents deleted before or while it was removed #1535 decided;add_fieldcommits that writer before it adds the field again. The cached searcher is now dropped after the writer lock is released on that path too, as on the others.laurus/persistence.md, under "Failed Writes", says thatadd_fieldanddelete_fieldtake everything they wait for before they change either store, so a cancelled call adds or removes nothing.VectorStore::delete_field,Engine::add_fieldandEngine::delete_field.Tests
Engine tests in
laurus/src/engine.rs, built on the fairness of the writer lock as the #1517 cancellation tests are:a_delete_field_cancelled_while_it_waits_for_the_vector_writer_removes_nothing: the test holds the writer, anddelete_field("vec")is dropped while it waits. The field stays in the schema, a later put of it is applied without stopping writes, both documents are found after a commit, anddelete_fieldthen succeeds.an_add_field_cancelled_while_it_waits_for_the_vector_writer_adds_nothing: the test holds the writer,add_field("vec2")queues behind it in its commit, and a second request queues behind that, so after the commitadd_fieldwaits for the writer in the store, where it is dropped. A search checks that its commit ran. The field is in neither the schema nor the index, adding it again succeeds, and a put of it is found.Verification
All on Linux x86_64 (evo-x2, rustc 1.99.1), on this branch's commit (run on
3e7249d6with the same change uncommitted, before the commit;origin/mainhas not moved since).cargo +stable fmt --all -- --check: passes.cargo +stable clippy --all-targets --features embeddings-all -- -D warnings(CI's command, every workspace member including the bindings, withPHPENV_VERSION=8.5.4 RBENV_VERSION=4.0.2for this host's toolchains): passes.cargo +stable test -p laurus --features embeddings-all,pq-fastscan --no-fail-fast: 142 test runs (lib, integration and doc tests), 3380 passed, 0 failed, 29 ignored.cargo +stable test --features embeddings-all --no-fail-fastforlaurus-server,laurus-mcpandlaurus-cli: 14 test runs, 332 passed, 0 failed.python3 scripts/check-binding-snippets.py: no violations (682 call sites in 799 files).laurus-wasm, as in the Test laurus-wasm job:cargo buildandcargo clippy --tests -- -D warningsforwasm32-unknown-unknownpass, andwasm-pack test --node laurus-wasmpasses 61 tests.RED: each order put back in turn, then the two tests above and three neighbours (
a_put_cancelled_while_it_waits_for_the_vector_writer_writes_nothing, and the fix(engine)!: re-adding a removed vector field starts it empty, so a deleted document never comes back #1545 store testsre_add_commits_a_kept_writer_before_it_empties_the_fieldandre_add_fails_while_the_kept_writer_cannot_commit) run:delete_fieldremoves the field, then takes the lock (as before this PR)a_delete_field_cancelled_..._removes_nothing(unknown vector field 'vec': no index configured for itfrom the later put)add_fieldadds the field, then takes the lock (as before fix(engine)!: re-adding a removed vector field starts it empty, so a deleted document never comes back #1545)an_add_field_cancelled_..._adds_nothing(vector field 'vec2' already existsfrom the secondadd_field), and both fix(engine)!: re-adding a removed vector field starts it empty, so a deleted document never comes back #1545 store testsThe other tests pass in each run. After the source was restored, all five pass.
Not covered
update_field.delete_fieldleaves the field's name inpending_reindex.