Add failing tests for #403: File Handle Resource Leak in SyncLock.acquire()

[Serhan-Asad]

Summary

Fixes the file handle resource leak bug in SyncLock.acquire() reported in #403.

Changes

Code Fix

File: pdd/sync_determine_operation.py (lines 185-202)

Added nested try-except block to ensure file descriptors are properly closed on ANY exception:

self.fd = open(self.lock_file, 'w')
try:
    # Critical section - must close file if anything fails
    if HAS_FCNTL:
        fcntl.flock(self.fd.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
    # ... write PID ...
except:
    # Close file on ANY exception (not just IOError/OSError)
    if self.fd:
        self.fd.close()
        self.fd = None
    raise

Tests Added

File: tests/test_sync_determine_operation.py

• test_file_handle_cleanup_on_keyboard_interrupt - Verifies fd cleanup on Ctrl+C
• test_file_handle_cleanup_on_runtime_error - Verifies fd cleanup on unexpected errors
• test_file_handle_cleanup_on_exception_during_write - Verifies fd cleanup during write failures

Testing

✅ All 11 SyncLock unit tests pass
✅ All 88 tests in test_sync_determine_operation.py pass
✅ 5/5 manual comprehensive tests pass

Manual Testing Performed

1. Normal lock acquisition and release - ✅ No leaks
2. KeyboardInterrupt during acquisition - ✅ FD properly closed
3. RuntimeError during acquisition - ✅ FD properly closed
4. Exception during PID write - ✅ FD properly closed
5. Context manager usage - ✅ FD properly closed

Impact

• Prevents file descriptor exhaustion in long-running processes
• Fixes crashes with OSError: [Errno 24] Too many open files
• Improves reliability for PDD server, CI/CD pipelines, and automated scripts

Root Cause Fixed

The original code only caught (IOError, OSError) for cleanup, allowing other exceptions like KeyboardInterrupt, RuntimeError, or ValueError to leak file descriptors. The fix uses a bare except: clause to catch ALL exceptions and ensure cleanup.

Fixes #403

[Copilot]

Pull request overview

This PR adds comprehensive test coverage to detect a file descriptor resource leak in SyncLock.acquire() when interrupted by exceptions other than IOError or OSError. The tests verify that the current exception handling is insufficient and demonstrate that the proposed fix (nested try-finally block) will resolve the issue.

Changes:

• Added 6 unit tests covering KeyboardInterrupt, RuntimeError, write failures, and regression scenarios
• Added 4 E2E tests that simulate real-world interrupt scenarios using subprocess isolation
• Included the fix implementation showing nested try-finally pattern for guaranteed cleanup

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tests/test_sync_determine_operation.py	Unit tests for file handle cleanup under various exception conditions
tests/test_e2e_issue_403_file_handle_leak.py	E2E tests simulating real-world KeyboardInterrupt and RuntimeError scenarios
pdd/sync_determine_operation.py	Implementation of nested try-finally fix for guaranteed file descriptor cleanup

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.