file kvstore: after a crashed write, the next shorter write returns the new value plus stale bytes (regression in 0.1.65)

Inizia dal percorso di scrittura e dal test del lock obsoleto in tensorstore/kvstore/file/file_key_value_store.cc e tensorstore/kvstore/file/file_key_value_store_test.cc. Leggi il commento di progettazione del percorso di scrittura al passaggio 6a, poi esegui i test di file kvstore. Estendi il test del lock obsoleto con un file di lock più lungo del nuovo valore; il lavoro è completato quando una scrittura successiva rilegge esattamente i byte scritti.

Summary. In the default os locking mode, the file kvstore writes a value to K.__lock and renames it to K, but it reopens an existing K.__lock without truncating it. If a writer is killed after writing to K.__lock and before the rename, the next write of a shorter value to K stores the new value followed by the tail of the killed writer's bytes, and reads return that with status OK. This is a regression: the truncate was removed in 2a44dc7d (2024-08-20), first released in 0.1.65; 0.1.64 is not affected.

Affected versions

• 0.1.65 through 0.1.86 (the latest release on PyPI). Reproduced on 2026-11-12 with 0.1.86 (all three modes below) and 0.1.65 (stale and kill); 0.1.64 passes both. Python 3.12, Linux arm64, local file system.
• Checked by reading master at f524c932 (2026-11-09): the code paths below are unchanged, and no commit since 0.1.86 changes the behavior of the lock or write path. master was not built.

Impact

• Silent for plain keys: the read succeeds and returns more bytes than were written.
• Loud for OCDBT databases on the file kvstore: the manifest is written the same way, so after the same sequence the database no longer opens (DATA_LOSS: ... Error decoding manifest: Length in header (182) does not match actual length (278), from the script below; the lengths vary between runs). Every later open fails until someone repairs or deletes the database.
• No operator action is needed to trigger it, and it happens in the default configuration (file_io_locking.mode = "os"). The trigger is a process crash (SIGKILL, OOM kill, preemption, node loss) during a write, followed later by a write of a shorter value to the same key. Checkpointing workloads do this when a restarted job writes into the directory of a save that crashed. We first saw it through Levanter, which stores checkpoints as TensorStore arrays in an OCDBT database on the file kvstore: the restarted job re-saved into the crashed save's directory and failed with DATA_LOSS: ... Length in header (193) does not match actual length (194).
• The documentation promises the opposite. schema.yml lines 151-153: "Stale lock files with the suffix .__lock may remain if a failure occurs while a write is in progress, but these files will be cleaned up automatically by any subsequent write to the same key."

Reproduction

Standalone script, no extra tools (in the collapsible block below). The stale mode needs no crash: it leaves a 200-byte k.__lock, writes 100 bytes to k and reads them back. The kill mode starts a real writer of a 512 MiB value in a child process, waits until k.__lock holds more than 1 MiB, kills the child with SIGKILL, then writes 100 bytes to k and reads them back. The ocdbt mode plants a larger manifest, taken from a different database that it then deletes, as manifest.ocdbt.__lock. That is the shape of what a manifest writer killed before its rename leaves behind, though in a real crash the stale manifest belongs to the same database. It then writes one more key.

"""A write to a file kvstore key after a killed writer returns the new value plus stale bytes.

  python tensorstore_stale_lock.py stale   a 200-byte k.__lock left over, then a 100-byte write
  python tensorstore_stale_lock.py kill    SIGKILL a real writer while it writes k.__lock
  python tensorstore_stale_lock.py ocdbt   the same effect on an OCDBT manifest

Exits 1 if the store returned corrupted data, 0 otherwise.
"""
import importlib.metadata
import os
import shutil
import signal
import subprocess
import sys
import tempfile
import time

import tensorstore as ts

BIG = 512 << 20
SHORT = b"B" * 100


def kv(root):
    return ts.KvStore.open(f"file://{root}/").result()


def stale(root):
    # The state a killed writer leaves, without a crash: k.__lock larger than the next value.
    with open(os.path.join(root, "k.__lock"), "wb") as f:
        f.write(b"A" * 200)
    kv(root).write("k", SHORT).result()
    got = kv(root).read("k").result().value
    print(f"200-byte k.__lock left over; wrote {len(SHORT)} bytes to k, read back {len(got)} bytes")
    return 0 if got == SHORT else 1


def kill(root):
    # A writer of a large value, killed before it renames k.__lock to k.
    child = subprocess.Popen([sys.executable, "-c",
                              "import sys, tensorstore as ts; "
                              "kv = ts.KvStore.open(f'file://{sys.argv[1]}/').result(); "
                              f"kv.write('k', b'A' * {BIG}).result()", root])
    lock = os.path.join(root, "k.__lock")
    while not (os.path.exists(lock) and os.path.getsize(lock) > 1 << 20):
        if child.poll() is not None:
            sys.exit("writer finished before it could be killed; rerun or raise BIG")
        time.sleep(0.001)
    os.kill(child.pid, signal.SIGKILL)
    child.wait()
    print(f"writer killed; k exists: {os.path.exists(os.path.join(root, 'k'))}, "
          f"k.__lock holds {os.path.getsize(lock)} bytes")

    kv(root).write("k", SHORT).result()
    got = kv(root).read("k").result().value
    print(f"wrote {len(SHORT)} bytes to k, read back {len(got)} bytes")
    return 0 if got == SHORT else 1


def ocdbt(root):
    db = lambda: ts.KvStore.open({"driver": "ocdbt", "base": f"file://{root}/"}).result()
    for i in range(20):
        db().write(f"key{i:02d}", b"x").result()
    # A larger manifest, taken from a different database that is then deleted.
    larger_manifest = open(os.path.join(root, "manifest.ocdbt"), "rb").read()
    shutil.rmtree(root)
    os.makedirs(root)
    db().write("key00", b"x").result()
    # Planted as a stale manifest.ocdbt.__lock: a larger file than the next manifest, which is
    # what a manifest writer killed before its rename leaves behind (there, of the same database).
    with open(os.path.join(root, "manifest.ocdbt.__lock"), "wb") as f:
        f.write(larger_manifest)
    db().write("key01", b"y").result()
    try:
        print(f"database lists {len(db().list().result())} keys")
        return 0
    except Exception as e:
        print(f"database no longer opens: {str(e).splitlines()[0][:200]}")
        return 1


if __name__ == "__main__":
    print(f"tensorstore {importlib.metadata.version('tensorstore')}")
    root = tempfile.mkdtemp()
    sys.exit({"stale": stale, "kill": kill, "ocdbt": ocdbt}[sys.argv[1]](root))

Output with 0.1.86 (byte counts and manifest lengths vary between runs; they depend on when the kill lands):

$ python tensorstore_stale_lock.py stale
tensorstore 0.1.86
200-byte k.__lock left over; wrote 100 bytes to k, read back 200 bytes

$ python tensorstore_stale_lock.py kill
tensorstore 0.1.86
writer killed; k exists: False, k.__lock holds 1937408 bytes
wrote 100 bytes to k, read back 1937408 bytes

$ python tensorstore_stale_lock.py ocdbt
tensorstore 0.1.86
database no longer opens: DATA_LOSS: Error decoding local file "/tmp/tmp1czdak6l/manifest.ocdbt": Error decoding manifest: Length in header (182) does not match actual length (278); at byte 12

The same script with 0.1.64 and 0.1.65:

$ python tensorstore_stale_lock.py stale
tensorstore 0.1.64
200-byte k.__lock left over; wrote 100 bytes to k, read back 100 bytes

$ python tensorstore_stale_lock.py kill
tensorstore 0.1.64
writer killed; k exists: False, k.__lock holds 3305472 bytes
wrote 100 bytes to k, read back 100 bytes

$ python tensorstore_stale_lock.py stale
tensorstore 0.1.65
200-byte k.__lock left over; wrote 100 bytes to k, read back 200 bytes

$ python tensorstore_stale_lock.py kill
tensorstore 0.1.65
writer killed; k exists: False, k.__lock holds 1654784 bytes
wrote 100 bytes to k, read back 1654784 bytes

Expected vs actual

• Expected: after a write of v to K succeeds, a read of K returns exactly v, as the documentation and the design comment describe.
• Actual: the read returns v followed by the bytes beyond len(v) that the killed writer had written to K.__lock.

Root cause

• Until 0.1.64, the write path truncated the lock file once it held the lock. 2a44dc7d ("Rework "file" kvstore write paths for better network file support") removed these lines from the write task:

  if (internal_os::GetSize(lock_helper.info) > value.size()) {
    // Only truncate when the file is larger. In the common path, the lock
    // file is newly created, so truncate is useless.
    TENSORSTORE_RETURN_IF_ERROR(internal_os::TruncateFile(fd));
  }
  

• The design comment in file_key_value_store.cc lines 70-72 (step 6a) says: "Truncate the lock file (it may contain garbage data from a previous write attempt that terminated unexpectedly)."

• In os mode the write path takes the lock with AcquireFileLock(full_path + kLockSuffix) (lines 689-711). AcquireFileLock opens the file with OpenFlags::DefaultWrite, which is O_CREAT | O_WRONLY | O_CLOEXEC (open_flags.h line 50), without O_TRUNC, and does not truncate it.

• WriteWithSync writes the new value from offset 0 and does not truncate either, so the file keeps any older bytes beyond the new length. The rename then publishes that file as K.

• TruncateAndOverwrite exists but is used only by the non_atomic mode. The lockfile mode is not affected because it creates the lock file with O_EXCL (AcquireExclusiveFile); there, a stale lock file instead blocks every write to the key until acquire_timeout expires, as documented.

• The existing test for stale lock files, file_key_value_store_test.cc lines 277-300, creates the stale lock file empty, so it cannot observe leftover bytes.

Suggested fix

Restore the truncate that 2a44dc7d removed: once the lock is held (and after any if_equal check), truncate the lock file before the value is written, as step 6a describes. Truncating after the lock is acquired is safe because the lock serializes writers of the key. Truncating when the file is opened (O_TRUNC) would not be: AcquireFileLock opens the file before it holds the lock, so a second writer would truncate a live writer's file. An equivalent alternative is to truncate to the written length after WriteWithSync and before the fsync and rename. We are happy to send a PR with the fix and the test below.

A regression test: extend the stale-lock test to write a lock file longer than the new value (for example 200 bytes), write a 100-byte value, and check that the read returns exactly 100 bytes.

Prior reports and related issues

We found no prior report. Related but different: #183 (lock acquisition fails on some network file systems) and #160 (lock file errors on Windows and WSL network file systems; 2a44dc7d, which removed the truncate, was the rework for it).

How this was found

We found this with a crash-injection tool that kills a process with SIGKILL just before or just after each file operation of a checkpoint save, then restarts the job and checks what it resumed. The standalone script above reproduces it without that tool.

AI assistance

This report, its reproduction script and the analysis were prepared with an AI coding agent. I ran the reproduction on the versions listed above and checked the analysis and the cited code.