Skip to content

Fix CI failures by updating OSError handling in test finalizers#477

Open
Harishmcw wants to merge 1 commit intobastibe:masterfrom
Harishmcw:win_arm64
Open

Fix CI failures by updating OSError handling in test finalizers#477
Harishmcw wants to merge 1 commit intobastibe:masterfrom
Harishmcw:win_arm64

Conversation

@Harishmcw
Copy link
Copy Markdown

This PR fixes the CI failures on Windows builds.

The Problem

  • The test suite was passing on windows-2022 but failing on windows-latest because GitHub updated the latter to Windows Server 2025. On this newer image, closing an invalid file descriptor now fails silently instead of raising an OSError. Since the test strictly expected a crash via pytest.raises(OSError), the lack of an exception caused a test failure.

The Soultion

  • Updated the finalizer in tests/test_soundfile.py to use a try/except block. This ensures the file is closed if still open, but allows the test to pass if the OS handles the closure silently or if the library has already disposed of the resource.

@bastibe
Copy link
Copy Markdown
Owner

bastibe commented Apr 13, 2026

Can you point me to documentation of this behavior? This seems awfully strange.

I'm also a bit saddened that we're no longer checking for the OSError. Perhaps we should explicitly disable the check only on Windows?

@Harishmcw
Copy link
Copy Markdown
Author

Thanks for the question, I investigated this further.

I verified that SoundFile correctly closes the file descriptor. After f.close(), the FD is invalid and operations like os.lseek() fail with EBADF, which confirms the expected behavior.

However, on the newer Windows CI environment (windows-latest), calling os.close(fd) on this already-closed descriptor does not raise OSError, which is what causes the test failure.

Minimal check I used:

import os
import numpy as np
import soundfile as sf

sf.write("test.wav", np.zeros((1000, 2)), 44100)

fd = os.open("test.wav", os.O_RDONLY)
f = sf.SoundFile(fd)
f.close()

try:
    os.lseek(fd, 0, os.SEEK_SET)
    print("fd is STILL OPEN")
except OSError as e:
    print(f"fd is closed — got {e}")

Output:

fd is closed — got [Errno 9] Bad file descriptor

So the FD is definitely closed, but os.close(fd) doesn’t raise on this environment.

If you prefer, we could keep the check and disable it only on Windows.

@bastibe
Copy link
Copy Markdown
Owner

bastibe commented Apr 17, 2026

This whole situation is very silly. Since soundfile does close the file on its own, there really is no point in explicitly closing it in the finalizer. I suppose we did close it just to make sure we're not leaking a resource, and it's good practice to check that it actually was closed.

So I'd say we do as you propose in your reproduction, and assert explicitly that lseek fails, so as to be sure the file descriptor is indeed closed, and then indiscriminately close it if it isn't.

def finalizer():
    try:
        os.lseek(fd, 0, os.SEEK_SET)
    except OSError as exc:
        assert exc.errno == 9 # the file was closed correctly
    else:
        os.close(fd)
        pytest.fail("file descriptor not closed correctly")

@Harishmcw
Copy link
Copy Markdown
Author

I tried this approach, but I’m seeing different behavior when running the full test suite.

In the minimal example I shared earlier, os.lseek() fails with EBADF, which indicates the FD is closed.

However, after updating the finalizer as suggested and running the tests, there are cases where os.lseek(fd, ...) succeeds, leading to the "file descriptor not closed correctly" failure.

CI run with this change: https://github.com/Harishmcw/python-soundfile/actions/runs/24632959347/job/72023393030

@bastibe
Copy link
Copy Markdown
Owner

bastibe commented Apr 20, 2026

Thank you for testing. That's very annoying. I agree with your original assessment, then, that we can't do any better than try/ignore os.close. Could you add a comment to the code in this pull request that explains the situation?

After that, I'd be happy to merge it. Thank you for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants