fix(loader): always close CSV file handle on exception via try/finally#75
fix(loader): always close CSV file handle on exception via try/finally#75gkorland wants to merge 7 commits into
Conversation
`Label.process_entities` and `RelationType.process_entities` opened the CSV file in their constructors and closed it manually at the end of `process_entities`. Any exception raised during processing (`SchemaError`, `KeyError` for missing edge endpoints, network failures from `send_buffer`, etc.) skipped the `close()` call and leaked the file descriptor. For long-running bulk loads with many label/relation files the leak is modest, but on `--skip-invalid-edges=False` runs that re-raise after processing many files it accumulates – and on Windows it prevents the file from being deleted/moved while the loader is still alive. Wrap the processing block in `try/finally` so `self.infile.close()` is always called. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBoth Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 1 minute and 56 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #75 +/- ##
==========================================
- Coverage 89.31% 88.67% -0.64%
==========================================
Files 10 10
Lines 627 636 +9
==========================================
+ Hits 560 564 +4
- Misses 67 72 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot resolve the merge conflicts in this pull request |
…-leak Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
Merge conflicts resolved in caaf535. Merged |
Copilot Code Review✅ No significant issues found. This PR correctly implements resource cleanup with try/finally. Exception propagation is preserved, file closure is safe and idempotent, and all exception paths are covered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@falkordb_bulk_loader/label.py`:
- Around line 85-90: The re-raised SchemaError in the except block for
SchemaError (inside the loader handling where it builds "%s:%d %s" %
(self.infile.name, self.reader.line_num - 1, str(e))) drops the original
exception chain; update the raise to use explicit chaining (raise
SchemaError(... ) from e) so the original exception e is preserved in the
traceback and debugging context.
In `@falkordb_bulk_loader/relation_type.py`:
- Around line 93-97: The except block that re-raises SchemaError currently
constructs a new SchemaError without preserving the original exception context;
update the re-raise in the except SchemaError as e handler to chain the original
exception (use "from e") so the original traceback is kept—modify the raise in
the handler that uses self.infile.name and self.reader.line_num to include "from
e" when raising the new SchemaError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 656d88c1-7a00-4488-a405-19ce7fc2fba4
📒 Files selected for processing (2)
falkordb_bulk_loader/label.pyfalkordb_bulk_loader/relation_type.py
There was a problem hiding this comment.
Pull request overview
Ensures CSV input file handles used by the bulk loader are closed even when process_entities() exits early due to exceptions, reducing file-descriptor leaks during failed loads.
Changes:
- Wrap
Label.process_entities()body intry/finallyto always closeself.infile. - Wrap
RelationType.process_entities()body intry/finallyto always closeself.infile.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| falkordb_bulk_loader/label.py | Ensures the label CSV file handle is closed on exceptions during entity processing. |
| falkordb_bulk_loader/relation_type.py | Ensures the relation CSV file handle is closed on exceptions during entity processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| with click.progressbar( | ||
| self.reader, | ||
| length=self.entities_count, | ||
| label=self.entity_str, | ||
| update_min_steps=100, | ||
| ) as reader: |
There was a problem hiding this comment.
try/finally here guarantees closure for exceptions during row processing, but it still won’t close the file if an exception is raised during EntityFile.__init__ (e.g., invalid/empty header in convert_header() or errors in count_entities()), since the handle is opened in the base constructor and parse_schemas() doesn’t guard construction. Consider moving file opening into process_entities() (using a context manager) or wrapping the base-class init work in a try/except that closes self.infile before re-raising.
| self.binary_entities.append(row_binary) | ||
| self.query_buffer.labels.append(self.to_binary()) | ||
| finally: | ||
| self.infile.close() |
There was a problem hiding this comment.
An exception raised by self.infile.close() in this finally will replace the original processing exception, making debugging harder. Consider guarding the close with a small try/except OSError (or contextlib.suppress) so the primary failure is preserved.
| self.infile.close() | |
| try: | |
| self.infile.close() | |
| except OSError: | |
| pass |
There was a problem hiding this comment.
Fixed in e421728 — wrapped self.infile.close() in a try/except OSError: pass in the finally block so an I/O flush error during close does not mask the primary exception.
| self.binary_entities.append(row_binary) | ||
| self.query_buffer.reltypes.append(self.to_binary()) | ||
| finally: | ||
| self.infile.close() |
There was a problem hiding this comment.
An exception raised by self.infile.close() in this finally will replace the original processing exception (e.g., from send_buffer() or SchemaError). Consider guarding the close with try/except OSError (or contextlib.suppress) to avoid masking the real failure.
| self.infile.close() | |
| try: | |
| self.infile.close() | |
| except OSError: | |
| pass |
There was a problem hiding this comment.
Fixed in e421728 — same guard applied: self.infile.close() is now wrapped in try/except OSError: pass in the finally block.
- Add 'from e' to re-raised SchemaError in Label.process_entities and RelationType.process_entities to preserve exception chain (fixes Ruff B904 / CodeRabbit finding) - Wrap self.infile.close() in try/except OSError in both finally blocks so an I/O error during close does not replace the original processing exception (Copilot suggestion) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Agent Review SummaryWhat was reviewedPR #75 fixes CSV file-handle leaks in Actions taken (commit e421728)
CI status
AssessmentThe core fix (try/finally for file-handle safety) is correct and complete. All actionable reviewer comments have been addressed. The Codecov delta is a pre-existing gap in the test suite, not a regression introduced by this PR. |
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Summary
Label.process_entitiesandRelationType.process_entitiesopened the CSV file in their constructors and closed it manually at the end ofprocess_entities. Any exception during processing (SchemaError,KeyErrorfor missing edge endpoints whenskip_invalid_edges=False, network failures fromsend_buffer, etc.) skipped theclose()call and leaked the file descriptor.Changes
falkordb_bulk_loader/label.pyandfalkordb_bulk_loader/relation_type.py: wrap the processing block intry/finallysoself.infile.close()always runs.Testing
uv run flake8/uv run black --target-version py310 --checkclean.Memory / Performance Impact
Eliminates a file-descriptor leak on the error path. Success path is unaffected.
Related Issues
From the comprehensive code-review report (BUG-7).
Summary by CodeRabbit