[sqlite store fixes 8] Hold the engine lock until a cancelled engine call's thread finishes - #1843

…emMachine#1588)

* Atomically swap vector search engine index files on save

SQLiteVectorStore persists each collection's index by calling the search
engine's save(), which wrote directly to the final path. A crash mid-write
left a truncated/corrupt file. Because index_saved=True makes the on-disk
index a durable contract (missing/corrupt is a hard IndexLoadError, not a
silent empty rebuild), an interrupted save could render a collection
unrecoverable.

Write the index to a sibling temp file and swap it into place with
os.replace (atomic on POSIX and Windows on the same filesystem), so a reader
sees either the old or new index, never a partial write; a failed save leaves
the previous index intact. Leftover temp files are cleared on load so a crash
does not leak them across restarts.

Implemented in the engines (shared index_persistence helper) rather than in
SQLiteVectorStore/SQLiteVectorStoreCollection, since the index save location
and number of files written differ across engine implementations.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Make the index swap durable, not only atomic

The swap protects a reader from a torn index, but the vector store also
trims its pending-operation log once `save` returns -- and that log is the
only other copy of those vectors, since the records table stores no vector
column. So the swap reaching disk is load-bearing rather than a bonus:

- fsync the parent directory after the replace, since POSIX `rename(2)`
  leaves the new directory entry in the page cache. Best-effort and ignored
  on failure, matching SQLite's `unixSync`; a no-op on Windows, which has no
  equivalent operation.
- stop swallowing a failed fsync of the temp file. SQLite draws the same
  line -- a file fsync failure raises SQLITE_IOERR_FSYNC while a directory
  fsync failure is ignored -- and `EIO` means the writeback already failed
  and the dirty pages were dropped, which is exactly when the save must not
  be reported as committed. The existing cleanup then leaves the previous
  index in place with the log untrimmed, so the next save retries.
- use F_FULLFSYNC on macOS, where plain `fsync` leaves the data in the
  drive's volatile write cache, falling back when a filesystem refuses it.

State the resulting obligation on `VectorSearchEngine.save` itself, since
that is what the store now relies on: replace atomically, then make the
replacement as durable as the platform allows. An engine whose backend
already implements the whole protocol can delegate to it and skip these
helpers; the rest use `atomic_index_write`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Let the engine own index durability, not the vector store

The pending log holds the only durable copy of a vector between
checkpoints -- the records table has no vector column -- so trimming it is
safe only at an instant when the index provably holds those vectors. The
temp-write + rename protocol this PR shipped could not provide that
instant. A rename changes a directory entry, and Windows exposes no way
to flush one: os.fsync is _commit, which is FlushFileBuffers, which is
for file data, and you cannot open a directory to fsync it. The decisive
evidence is SQLite's own -- it threads a directory-sync flag through
every commit-relevant directory operation, honors it in unixDelete, and
declares it /* Not used on win32 */ in winDelete. So os.replace could
return, _save_collection_index could commit its trim durably behind it,
and a power cut could still roll the rename back: records forward, index
back, no copy of the difference left. MOVEFILE_WRITE_THROUGH is not a
fix; its documented guarantee covers copy-and-delete (cross-volume)
moves, not same-volume renames.

Take SQLite's answer, which was not to harden the directory operation
but to stop using one as a commit point (PERSIST commits by zeroing a
header, TRUNCATE by truncating, WAL by appending frames).

A base path now expands into two index slots plus a generation record
each, created once and thereafter only overwritten. A checkpoint writes
the index over the inactive slot and flushes it, then writes that slot's
generation record and flushes that. The record is the commit, and it is
a write into a file that already exists. It holds the generation and its
bitwise complement, so a torn write reads as absent rather than as some
other generation -- all or nothing without needing single-sector
atomicity from the hardware. load takes the highest believable
generation, and deliberately does not fall back to the older slot when
the published index will not parse: the log was trimmed against the
newer one, so the older is stale by exactly the ops that can no longer
be replayed.

Both backends already write straight to the path they are given, which
is what this protocol wants -- verified that repeated saves preserve the
inode and leave no stray files -- so no engine gains a temp file, a
buffer, or a rename.

Durability is entirely the engine's, including which artifact is live.
The store keeps no slot pointer, manifest, or generation, so no schema
change and no migration: what remains is one rule, never trim past what
save says is durable, and _save_collection_index already had that order.
index_path becomes index_base_path since it no longer names a file, and
discarding a collection asks the engine layer which files that covers.

BREAKING CHANGE: an index written by the previous protocol is not
published under the new one, so a collection with index_saved=True
raises IndexLoadError until its index directory is cleared and the
records re-ingested.

Anomaly tests walk every crash point in the publish sequence by
constructing the on-disk state each would leave, plus one that pins the
ordering itself (a failed index write must publish nothing) since
state-based tests cannot observe it. Verified against three deliberate
breaks -- dropping the complement check, writing the record first, and
reusing one slot instead of alternating -- each caught.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Publish the index atomically, and stop promising durability

The two-slot generation-record protocol bought a guarantee we have
decided not to make: that a save survives a power failure. Every engine
would have to implement and maintain that protocol, and the failure it
buys out is bounded -- search recall for the records applied since the
last checkpoint, repaired by re-ingesting them. The direction that
actually costs, a published index that will not parse, is closed by the
atomic swap on its own.

So this returns to the temp-file-plus-rename publication and spends the
difference on stating the contract instead of strengthening it: `save`
publishes atomically, never durably; the store trims the pending log
behind a publication a power failure can revert; a record whose vector
is lost that way still resolves by uuid, is absent from search until it
is upserted again, and nothing here detects the gap for the caller.

Reverts the durability and engine-owned-publication commits, keeps the
atomic swap, and adds a store-level test that reconstructs a reverted
publication deterministically -- restore the previous index bytes after
the trim -- to pin the direction it fails in.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Report a lost embedding as lost, not as a missing feature

`update_feature` reads the stored embedding back when a caller updates a
feature without supplying one, and that is the only place in the server
that depends on the index still holding a vector. With publication now
atomic rather than durable, a power failure can leave a feature whose
row is intact and whose vector is not -- a state this path reported as
"Vector record not found", which points the caller at the wrong thing
and hides the repair.

Split the two cases. A record that is genuinely absent keeps the old
message; a record whose embedding the index no longer holds says so and
names the fix, which is to pass a fresh embedding.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Let a failed fsync fail the save, and cut the essay around it

`_flush_to_disk` wrapped its fsync in `contextlib.suppress(OSError)` and
called itself best-effort. A failed fsync is exactly the evidence that the
bytes are not safe to publish -- on Linux an EIO from fsync means writeback
failed, reported once and then cleared -- so swallowing it and renaming
anyway published a file we had positive evidence was bad. The safeguard
cost something and, in the one case it existed for, guaranteed nothing.
Nothing tested it either.

Let it propagate. `atomic_index_write` already unlinks the temp and
re-raises, so a failed flush now leaves the previously published index
standing, which is the correct outcome. A test pins that.

The fsync is not best-effort, and the docstring should not have said so: it
rules out a class rather than narrowing a window. Because the flush
completes before the rename is issued, and a durable write does not
un-happen, the new name can never appear over incomplete bytes. What the
missing directory fsync costs is the other direction -- the rename may not
survive, so the publish reverts -- and that is the benign one this store
already accepts.

The module docstring was 76 lines against 49 of everything else, most of it
argument rather than documentation: a walk through SQLite's `unixDelete` /
`winDelete` sync-flag handling, and a rejected two-slot commit protocol.
That is the PR's case for the design, not something to re-read every time
someone opens a 20-line module, and the PR body carries it. What a reader
here needs is the guarantee, the non-guarantee, and the cost.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Say why the swap is a rename, and what the reopened fd cannot see

Two things the module was silent on.

Why a rename at all. The stronger answer is to put the commit point inside
the file, where an fsync reaches it portably -- SQLite never renames, and
commits by truncating or zeroing its rollback journal, or in WAL mode by
appending frames whose checksums make a torn tail self-identifying. Both
need the writer to own the file format. A search engine owns its own and
exposes `save(path)`, so above that call a rename is the only atomicity
primitive left, and an engine whose format already commits that way needs
none of this. Worth saying, because "why not do the better thing" is the
first question the module invites.

What the reopened descriptor cannot see. Flushing is fine on a fresh fd --
dirty pages belong to the file, not to the descriptor that dirtied them --
but error reporting is not: Linux hands a writeback error to descriptors
open when it was recorded, so one recorded between the engine's close and
this open is never reported and the save proceeds on bytes already known
bad. Same shape as the 2018 PostgreSQL fsync report. It cannot be closed
from here: the engine writes through its own descriptor and closes it
before returning, and closing the window needs an engine that writes
through a handle the caller supplies.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Fsync the index on a descriptor that predates the write

The fsync was on a descriptor opened after the engine had written and
closed its own, which flushes correctly -- dirty pages belong to the file,
not to the descriptor that dirtied them -- but reports nothing useful.
Linux samples the writeback error sequence when a file is opened, so a
descriptor opened after an error was recorded never learns of it: the fsync
returns success and the save publishes bytes already known bad. Same shape
as the 2018 PostgreSQL fsync report.

Open the temp before yielding it and hold it across the caller's write, so
the descriptor predates the bytes and any error from writing them is
reported here, where it fails the save.

That assumes the caller writes in place. Both engines do -- verified: the
inode is unchanged across `save_index` and `save`, and the held descriptor
sees the written size -- but it is their behaviour, not their contract. An
engine that built a file of its own and renamed it over the temp would
leave this descriptor on an orphaned inode, and the fsync would report on a
file nobody is about to publish. So it is checked before the fsync, and a
mismatch fails the save rather than passing it quietly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* State the in-place rule where the caller reads it

The descriptor held across the body only flushes what the body wrote if the
body writes the yielded path in place, and that requirement was recorded in
`_flush_to_disk` -- a private function nobody writing an engine opens. It
belongs on `atomic_index_write`, which is the API they use, alongside what
happens when it is broken: an `OSError` and no publication, so the mistake
surfaces at the first save rather than at a power cut.

`_flush_to_disk` keeps the mechanism -- why the descriptor has to predate
the write -- and now points at the rule instead of restating it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Ask the drive to flush on macOS, where fsync does not

`os.fsync` is not the same guarantee on all three platforms this ships to.
On Linux it flushes to the device, and on Windows `FlushFileBuffers` does
the same. Darwin's `fsync` explicitly does not: it returns once the data
reaches the drive, which may hold it in a volatile write cache. So on macOS
the ordering this module is built on -- data durable before the rename is
issued -- did not hold at the device, which is exactly the case it claims
to rule out.

`F_FULLFSYNC` asks the drive to flush that cache. Filesystems that cannot
refuse it, and there `fsync` is the most that can be asked, so a refusal
falls back; any other error is a write failure and propagates, as before.

The flush-failure test patched `os.fsync`, which Darwin no longer reaches.
It patches the module's own `_fsync` instead, which every platform does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Stop guessing which errnos mean "this drive cannot do that"

The fallback from `F_FULLFSYNC` to `fsync` was gated on an errno allowlist
-- ENOTSUP, EOPNOTSUPP, EINVAL -- so that a genuine write failure would
propagate rather than be quietly downgraded. Probing the actual returns on
Darwin shows the list is both incomplete and partly invented:

  ENOTSUP=45  EOPNOTSUPP=102   distinct here, so both are needed
  /dev/null   F_FULLFSYNC -> ENODEV(19), while fsync succeeds
  pipe/socket F_FULLFSYNC -> EBADF(9)
  EINVAL      never came from F_FULLFSYNC at all; it came from fsync

So ENODEV -- a real refusal, on a path anyone can reproduce -- would have
raised instead of falling back, and EINVAL was in the list by analogy
rather than evidence. What a network mount answers is not knowable from
here, which makes the whole list a guess that fails closed on whatever it
missed. This codebase does not classify driver errors by guessing, and
this was that.

Fall through on any failure instead. It is not a suppression: `fsync` runs
on the same descriptor and raises in its turn, so a flush that cannot
happen still fails the save. What the fallback gives up is the drive-cache
flush -- the guarantee this had before `F_FULLFSYNC` was asked for at all.
That is also what SQLite does with this same call, for the same reason.

The test drives it through `/dev/null`, which refuses with ENODEV and
accepts `fsync`; it fails against the allowlist and passes without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

* Fail the save when the drive cannot be told to flush

`F_FULLFSYNC` failing fell through to `fsync`, on the reasoning that a
refusal is a statement about the filesystem rather than a write failure.
That reasoning does not survive asking what `fsync` alone actually buys on
Darwin.

Against a process or kernel crash it is enough: the data has left the OS
for the drive before the rename is issued, so the rename dies in the page
cache and the old index stands. Against power loss it is not. The data
sits in the drive's volatile cache, the rename's metadata joins it moments
later, and nothing orders them -- and the rename is a few bytes against an
index of megabytes, so a drive flushing as it pleases can easily put the
new name on media while the bytes behind it are still queued. That is the
torn publication this module exists to prevent, in precisely the scenario
its docstring is about.

So the fallback answered a request for ordering with a flush that does not
provide it, and said nothing. A filesystem that cannot order data ahead of
a rename is not one to publish an index onto; raise, and let the operator
point `index_directory` at storage that can.

This is also the simpler code. Refusal and failure now take the same path,
so no errno is inspected -- there is no line to draw and no list to get
wrong, which is what the previous two revisions kept getting wrong in
opposite directions. The `/dev/null` test went with the fallback it pinned.

The earlier defence of falling back rested on network and FUSE mounts
being a realistic home for an index directory. They are not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NKmF9xNph9QH3ozNw3ZnJL

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 21105d6)