diff --git a/src/borgstore/backends/sftp.py b/src/borgstore/backends/sftp.py index 9d46cb0..926e9a5 100644 --- a/src/borgstore/backends/sftp.py +++ b/src/borgstore/backends/sftp.py @@ -447,20 +447,28 @@ def _write_to_tmpfile(): # so the store never sees partially written data. tmp_name = str(tmp_dir / ("".join(random.choices("abcdefghijklmnopqrstuvwxyz", k=8)) + TMP_SUFFIX)) try: - # try to do it quickly, not doing the mkdir. each sftp op might be slow due to latency. - # this will frequently succeed, because the dir is already there. - _write_to_tmpfile() - except FileNotFoundError: - # retry, create potentially missing dirs first. this covers these cases: - # - either the dirs were not precreated - # - a previously existing directory was "lost" in the filesystem - self._mkdir(str(tmp_dir), parents=True, exist_ok=True) - _write_to_tmpfile() - # rename it to the final name: - try: + try: + # try to do it quickly, not doing the mkdir. each sftp op might be slow due to latency. + # this will frequently succeed, because the dir is already there. + _write_to_tmpfile() + except FileNotFoundError: + # retry, create potentially missing dirs first. this covers these cases: + # - either the dirs were not precreated + # - a previously existing directory was "lost" in the filesystem + self._mkdir(str(tmp_dir), parents=True, exist_ok=True) + _write_to_tmpfile() + # rename it to the final name: self.client.posix_rename(tmp_name, name) - except OSError: - self.client.unlink(tmp_name) + except BaseException: + # writing the temp file or the rename failed (e.g. disk full, I/O error): remove the + # (partially written) temp file so it does not linger on the server. It would be invisible + # to .list (TMP_SUFFIX), but it would occupy space. Best effort: if the cleanup itself + # fails (e.g. the file was never created, or the connection is gone), keep the original + # exception, which is the more interesting one (and lets with_reconnect do its job). + try: + self.client.unlink(tmp_name) + except Exception: + pass raise @with_reconnect(swallow_not_found=True) diff --git a/tests/test_backends.py b/tests/test_backends.py index e2322f0..85b026f 100644 --- a/tests/test_backends.py +++ b/tests/test_backends.py @@ -624,6 +624,122 @@ def posix_rename(self, curr_name, new_name): assert type(written[0]) is bytes +@pytest.mark.parametrize("fail_in", ["write", "close"], ids=["fails-in-write", "fails-at-close"]) +def test_sftp_store_failed_write_removes_tmpfile(fail_in): + # if writing the temp file fails (e.g. server disk full), the partial temp file must be + # removed so it does not linger on the server (invisible to .list, but occupying space). + opened_names = [] + unlinked = [] + + class FakeFile: + def __enter__(self): + return self + + def __exit__(self, *args): + if fail_in == "close": # a pipelined write error surfaces when the file is closed + raise OSError(errno.ENOSPC, "No space left on device") + return False + + def set_pipelined(self, pipelined): + pass + + def write(self, data): + if fail_in == "write": + raise OSError(errno.ENOSPC, "No space left on device") + + class FakeClient: + def open(self, name, mode): + opened_names.append(name) + return FakeFile() + + def posix_rename(self, curr_name, new_name): + raise AssertionError("rename must not happen when the write failed") + + def unlink(self, name): + unlinked.append(name) + + backend = Sftp(hostname="localhost", path="/some/path") + backend.opened = True + backend.client = FakeClient() + with pytest.raises(OSError) as exc_info: + backend.store("dir/key", b"x" * 100) + assert exc_info.value.errno == errno.ENOSPC + # the (partial) temp file the store opened must have been unlinked: + assert len(opened_names) == 1 + assert unlinked == opened_names + + +def test_sftp_store_failed_rename_removes_tmpfile(): + # if the rename to the final name fails, the temp file must be removed, too. + unlinked = [] + + class FakeFile: + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def set_pipelined(self, pipelined): + pass + + def write(self, data): + pass + + class FakeClient: + def open(self, name, mode): + self.tmp_name = name + return FakeFile() + + def posix_rename(self, curr_name, new_name): + raise OSError(errno.EIO, "I/O error") + + def unlink(self, name): + unlinked.append(name) + + backend = Sftp(hostname="localhost", path="/some/path") + backend.opened = True + backend.client = FakeClient() + with pytest.raises(OSError) as exc_info: + backend.store("key", b"data") + assert exc_info.value.errno == errno.EIO + assert unlinked == [backend.client.tmp_name] + + +def test_sftp_store_failed_write_keeps_original_error_if_cleanup_fails(): + # cleanup of the temp file is best effort: if the unlink itself fails, the original write + # error must still propagate (not be masked by the cleanup error). + class FakeFile: + def __enter__(self): + return self + + def __exit__(self, *args): + return False + + def set_pipelined(self, pipelined): + pass + + def write(self, data): + raise OSError(errno.ENOSPC, "No space left on device") + + class FakeClient: + def open(self, name, mode): + return FakeFile() + + def posix_rename(self, curr_name, new_name): + pass + + def unlink(self, name): + raise OSError(errno.EIO, "unlink also fails") + + backend = Sftp(hostname="localhost", path="/some/path") + backend.opened = True + backend.client = FakeClient() + with pytest.raises(OSError) as exc_info: + backend.store("key", b"data") + assert exc_info.value.errno == errno.ENOSPC # the original error, not the cleanup's EIO + + @pytest.mark.skipif(boto3 is None, reason="boto3 is not installed") def test_s3_store_memoryview_gives_bytes_to_boto3(): # boto3 rejects a memoryview Body (parameter validation), so the s3 backend must convert it.