Always end the transaction, even when a commit or discard raises

[youdie006]

The problem

Transaction.complete() drains the file deque with no try/finally:

while self.files:
    f = self.files.popleft()
    if commit:
        f.commit()
    else:
        f.discard()
self.fs._intrans = False
self.fs._transaction = None
self.fs = None

If any commit() or discard() raises, the loop aborts. Files still queued are neither committed nor discarded, so their temporary files leak, and the three reset lines never run. __exit__ cannot compensate — its if self.fs: reset block sits after the complete() call that raised.

Leaving _intrans set is the damaging part. Filesystem instances are cached, so the poisoned object is handed back for the rest of the process and every later write silently disappears:

fs = fsspec.filesystem("file")
try:
    with fs.transaction:
        ...            # one commit fails, e.g. an unwritable target directory
except PermissionError:
    pass

fs2 = fsspec.filesystem("file")   # same cached object
with fs2.open(somewhere_else, "wb") as f:
    f.write(b"important")         # no error raised
os.path.exists(somewhere_else)    # False

Verbatim, before this change:

transaction raised: PermissionError
-- leaked temp files: 2
   fs._intrans     = True
   fs._transaction = True
   fsspec.filesystem('file') is the same object: True
   wrote written_much_later.txt with no error raised
   ...but exists = False

and after:

-- leaked temp files: 0
   fs._intrans     = False
   fs._transaction = False
   wrote written_much_later.txt with no error raised
   ...but exists = True

The contract

If the context finishes due to an (uncaught) exception, then the files are discarded and the file target locations untouched.
— docs/source/features.rst:137

That sentence is unqualified. The partial commit itself is arguably covered by "semi-atomic" a few lines above, so this change does not attempt to roll back files that already committed — but leaked temporaries and a filesystem stuck in transaction mode are not covered by anything.

The change

Drain in a try, and in the finally discard whatever is still queued — including the file that was in flight when the error was raised, since it is already off the deque — then always reset _intrans / _transaction / fs. Cleanup failures are logged rather than raised, so a failing os.remove cannot mask the user's original exception (which is what happens today on the discard path: a FileNotFoundError from reaping /tmp replaces the caller's own error).

DaskTransaction.complete had the same hole and additionally never cleared _transaction; it gets the same finally.

Tests

test_transaction_ends_when_a_commit_fails in fsspec/implementations/tests/test_local.py patches LocalFileOpener.commit to fail on one of three files, then asserts the transaction state is cleared, no temporaries are left in the temp directory, and a subsequent write on the same instance actually lands.

Nothing existing covered this: grep -rn "_intrans" fsspec/tests/ fsspec/implementations/tests/ only finds two SMB/SFTP happy-path assertions.

Verified by reverting only fsspec/transaction.py and keeping the test — it fails on assert fs._intrans is False. Full suite: 867 passed, 117 skipped, 2 xfailed.

---

Disclosure: this patch was prepared with AI assistance. The reproduction, the red/green check and the suite run above were executed against this branch; happy to adjust anything on request.