Skip to content

fix: close local lock file on failure - #1241

Open
fengjikui wants to merge 1 commit into
qdrant:masterfrom
fengjikui:codex/lock-cleanup
Open

fix: close local lock file on failure#1241
fengjikui wants to merge 1 commit into
qdrant:masterfrom
fengjikui:codex/lock-cleanup

Conversation

@fengjikui

Copy link
Copy Markdown

Closes #1234.

Summary

  • close the local .lock file when portalocker.lock() fails in QdrantLocal
  • apply the same cleanup in AsyncQdrantLocal
  • add a regression test for both local client implementations

Root cause

The local client opens the .lock file before acquiring the portalocker lock. If lock acquisition raises, the constructor raises without closing the opened file handle.

Validation

  • Base reproduction on origin/master: monkeypatched portalocker.lock() to raise LockException; captured .lock file remained open (closed: False)
  • uv run --with numpy --with pydantic --with portalocker --with grpcio --with protobuf --with urllib3 --with 'httpx[http2]' --with pytest pytest tests/test_qdrant_client.py::test_local_lock_failure_closes_lock_file -q -> 2 passed\n- uv run --with 'ruff==0.4.3' ruff check qdrant_client/local/qdrant_local.py qdrant_client/local/async_qdrant_local.py -> All checks passed\n\nNote: running ruff across the full touched test file also reports an existing unrelated F841 in tests/test_qdrant_client.py on origin/master (except Exception as ex), so I kept the lint check scoped to the changed local implementation files.

@netlify

netlify Bot commented Jun 23, 2026

Copy link
Copy Markdown

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit f0e6e9b
🔍 Latest deploy log https://app.netlify.com/projects/poetic-froyo-8baba7/deploys/6a3af72ee32511000893a227
😎 Deploy Preview https://deploy-preview-1241--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 63e85bc3-e80e-451b-bc6b-27960ebb3bda

📥 Commits

Reviewing files that changed from the base of the PR and between 326adef and f0e6e9b.

📒 Files selected for processing (3)
  • qdrant_client/local/async_qdrant_local.py
  • qdrant_client/local/qdrant_local.py
  • tests/test_qdrant_client.py

📝 Walkthrough

Walkthrough

Both QdrantLocal._load and AsyncQdrantLocal._load had a file descriptor leak: when portalocker.lock() raised LockException, the already-opened _flock_file handle was never closed before the RuntimeError was raised. The fix adds self._flock_file.close() and self._flock_file = None inside the LockException handler and a new catch-all except Exception handler in both the sync and async implementations. A new parametrized test covers both code paths by monkeypatching builtins.open and portalocker.lock.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix: close local lock file on failure' directly and clearly describes the main change—ensuring proper cleanup of the lock file when failure occurs.
Description check ✅ Passed The PR description provides relevant context about the file handle leak issue, root cause analysis, validation steps, and references the linked issue #1234.
Linked Issues check ✅ Passed The PR fully addresses issue #1234 by closing the file handle in both sync and async implementations when portalocker.lock() fails, and includes regression tests.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the file handle leak in QdrantLocal and AsyncQdrantLocal, with an appropriate regression test; no extraneous changes detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@eeshsaxena eeshsaxena left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed this is a genuine handle leak: in _load, self._flock_file = open(lock_file_path, "r+") runs before portalocker.lock(...), and the old except portalocker.exceptions.LockException raised RuntimeError without ever closing that handle - so the lock file object leaks until GC (relevant on Windows especially, since this is exactly the "folder already locked by another instance" path). Closing it and nulling self._flock_file before raising is correct, and adding the broad except Exception: ... raise to clean up on any other failure is a nice touch.

One small suggestion: #1264 is an almost identical fix for the same lines, and it additionally does raise RuntimeError(...) from None, which suppresses the "During handling of the above exception" chaining and reads a bit cleaner for this user-facing message. Might be worth adding here (or the maintainers may want to consolidate the two PRs).

Solid fix otherwise. Deferring to the maintainers.

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.

fix: file handle leak when portalocker.lock() raises exception in local mode

2 participants