Repository navigation
put_file is not atomic: a failed write leaves a truncated file that is then served as complete #130
Description
🤖 helped me writing this summary
do_put_file writes straight to the destination path. If the write dies partway through, the partial file stays at the final path and every later get returns it as though it were complete. Readers can't tell a truncated object from a whole one — local reads aren't checksummed.
This bites hardest with a disk cache under a process pool: an OOM-killed worker leaves a short (or 0-byte) cache entry that is then trusted indefinitely.
Repro
The content object needs a real fd because cloudfiles.py:685 calls os.fstat(content.fileno()); a pure io.RawIOBase fake dies with UnsupportedOperation: fileno before it ever reaches put_file.
import os, tempfile
from cloudfiles import CloudFiles
class FailingFile:
"""Real fd, but read() dies after handing over `stop_after` bytes."""
def __init__(self, real, stop_after):
self._f, self._stop_after, self._n = real, stop_after, 0
def fileno(self): return self._f.fileno()
def seek(self, *a, **k): return self._f.seek(*a, **k)
def read(self, n=-1):
remaining = self._stop_after - self._n
if remaining <= 0:
raise IOError("connection died mid-body")
chunk = self._f.read(remaining)
self._n += len(chunk)
return chunk
def test_failed_put_leaves_no_partial_object():
d = tempfile.mkdtemp()
src = os.path.join(d, "src.bin")
with open(src, "wb") as f:
f.write(b"x" * 1_000_000)
cf = CloudFiles("file://" + tempfile.mkdtemp())
with open(src, "rb") as fh:
try:
cf.put("chunk", FailingFile(fh, stop_after=3), compress=None)
except IOError:
pass
assert cf.get("chunk") is NoneExpected: None — the write never completed.
Actual: b'xxx', a 3-byte object standing in for a 1,000,000-byte one, served as complete.
Suggested fix
Write to a sibling temp name and os.replace. Atomic within a filesystem (hence a sibling, not /tmp), and unlike os.rename it also works on Windows. threading is already imported, so the temp name can be made unique per writer with no new dependency.
This changes durability semantics not at all — no fsync — which is fine for a cache but shouldn't be read as a stronger guarantee than it is.
def do_put_file():
tmp = f"{path}.{os.getpid()}.{threading.get_ident()}.tmp"
try:
try:
handle = open(tmp, 'wb')
except IOError:
mkdir(os.path.dirname(path))
handle = open(tmp, 'wb')
with handle as f:
if hasattr(content, "read") and hasattr(content, "seek"):
shutil.copyfileobj(content, f)
else:
f.write(content)
os.replace(tmp, path)
except BaseException:
try:
os.unlink(tmp)
except FileNotFoundError:
pass
raiseWith that applied the test above passes and the destination directory is left empty. automated_test.py -k "file or local or lock" is unchanged: 114 passed before (145.25s) and after (147.49s).
Verified against cloud-files 6.4.1 (1dafe2d2).
Activity
william-silversmith commentedon Aug 12, 2026
This is a good point. OOM or kill -9 will cause file truncation without easy detection (and presumably other rare issues can too). The suggested solution however will increase IOPS across the board. Is that viable?
nkemnitz commentedon Aug 12, 2026
I don't see another safe way, yet... For compressed files the decoders will likely complain at decode time. Similarly, truncated uncompressed files (>0 byte) should fail loudly in CloudVolume because the expected chunk size is known...
But for erroneous cache entries with 0 length, CloudVolume will just accept them as is and either permanently throw an
EmptyVolumeException, or (fill_missing=True) confidently build an empty chunk.The suggestion is not going to work well on NFS or gcsfuse or things like that... and if the process crashes, it also leaves
.tmpfiles cluttering your cache directorywilliam-silversmith commentedon Aug 12, 2026