fix 508 #517
fix 508 #517
Conversation
WalkthroughHardened cache loading in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant DI as DirectoryIndexer
participant FS as Filesystem
participant JS as JSON
C->>DI: _get_index_map(path)
DI->>FS: check if cache file exists
alt cache exists
DI->>FS: open cache file
DI->>JS: json.load(file)
alt load succeeds
JS-->>DI: index_map (dict)
DI-->>C: return index_map
else OSError / JSONDecodeError
JS--x DI: error
DI->>FS: delete corrupted cache file
DI-->>C: return {}
end
else no cache
DI-->>C: return {}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #517 +/- ##
==========================================
+ Coverage 99.85% 99.87% +0.02%
==========================================
Files 118 118
Lines 9724 9732 +8
==========================================
+ Hits 9710 9720 +10
+ Misses 14 12 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_io/test_indexer.py (2)
78-91: Nit: fix docstring typos and use write_text for clarity.Minor cleanups: “bd” → “bad”, add apostrophe in “doesn’t”, and prefer Path.write_text with explicit encoding.
- def directory_indexer_bad_cache(self, tmp_path_factory): - """Create a subclass of indexer which has a bd index_map file.""" + def directory_indexer_bad_cache(self, tmp_path_factory): + """Create a subclass of indexer with a bad index_map file.""" path = tmp_path_factory.mktemp("corrupt_cache_test") cache_path = path / "corrupt_cache.json" - - with cache_path.open("wt") as fi: - fi.write("{'bad': 'json'") + cache_path.write_text("{'bad': 'json'", encoding="utf-8") class SubIndexer(DirectoryIndexer): index_map_path = cache_path return SubIndexer
123-134: Great targeted test; consider asserting functional fallback as well.This validates the corrupted cache is removed. As an optional enhancement, also assert that a subsequent update writes a new, valid cache (non-empty dict) to fully exercise the recovery path.
assert directory_indexer_bad_cache.index_map_path.exists() - directory_indexer_bad_cache(path) + inst = directory_indexer_bad_cache(path) assert not directory_indexer_bad_cache.index_map_path.exists() + # Optional: trigger a write and ensure a valid cache is created. + inst.update() + assert directory_indexer_bad_cache.index_map_path.exists()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
dascore/io/indexer.py(1 hunks)tests/test_io/test_indexer.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_io/test_indexer.py (1)
dascore/io/indexer.py (1)
DirectoryIndexer(94-342)
🔇 Additional comments (1)
tests/test_io/test_indexer.py (1)
272-278: Rename fix looks good.The corrected test name improves readability and discoverability.
| out = {} | ||
| if path.exists(): | ||
| with path.open("r") as fi: | ||
| out = json.load(fi) | ||
| else: | ||
| out = {} | ||
| try: | ||
| with path.open("r") as fi: | ||
| out = json.load(fi) | ||
| # On rare occasions, the file can become corrupt; just delete so it | ||
| # gets regenerated. See #508. | ||
| except (OSError, json.JSONDecodeError): | ||
| os.remove(path) | ||
| return out |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make cache-file recovery more robust (race-safe delete, type check, and explicit encoding).
Good defensive change. Two small hardening tweaks:
- Wrap the delete in a suppress block (or use Path.unlink with missing_ok=True); concurrent readers/writers can race and raise FileNotFoundError/PermissionError.
- Validate that the decoded JSON is a dict; if it’s a different valid JSON type (e.g., a list), downstream code assumes dict and may break.
- Use an explicit UTF-8 encoding for portability.
Apply this diff:
- if path.exists():
- try:
- with path.open("r") as fi:
- out = json.load(fi)
- # On rare occasions, the file can become corrupt; just delete so it
- # gets regenerated. See #508.
- except (OSError, json.JSONDecodeError):
- os.remove(path)
+ if path.exists():
+ try:
+ with path.open("r", encoding="utf-8") as fi:
+ loaded = json.load(fi)
+ # Ensure expected structure.
+ if isinstance(loaded, dict):
+ out = loaded
+ else:
+ # Unexpected but valid JSON; clear so it can be regenerated.
+ from contextlib import suppress
+ with suppress(FileNotFoundError, PermissionError):
+ path.unlink(missing_ok=True)
+ # On rare occasions, the file can become corrupt; just delete so it
+ # gets regenerated. See #508.
+ except (OSError, json.JSONDecodeError, UnicodeDecodeError):
+ from contextlib import suppress
+ with suppress(FileNotFoundError, PermissionError):
+ path.unlink(missing_ok=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out = {} | |
| if path.exists(): | |
| with path.open("r") as fi: | |
| out = json.load(fi) | |
| else: | |
| out = {} | |
| try: | |
| with path.open("r") as fi: | |
| out = json.load(fi) | |
| # On rare occasions, the file can become corrupt; just delete so it | |
| # gets regenerated. See #508. | |
| except (OSError, json.JSONDecodeError): | |
| os.remove(path) | |
| return out | |
| out = {} | |
| if path.exists(): | |
| try: | |
| with path.open("r", encoding="utf-8") as fi: | |
| loaded = json.load(fi) | |
| # Ensure expected structure. | |
| if isinstance(loaded, dict): | |
| out = loaded | |
| else: | |
| # Unexpected but valid JSON; clear so it can be regenerated. | |
| from contextlib import suppress | |
| with suppress(FileNotFoundError, PermissionError): | |
| path.unlink(missing_ok=True) | |
| # On rare occasions, the file can become corrupt; just delete so it | |
| # gets regenerated. See #508. | |
| except (OSError, json.JSONDecodeError, UnicodeDecodeError): | |
| from contextlib import suppress | |
| with suppress(FileNotFoundError, PermissionError): | |
| path.unlink(missing_ok=True) | |
| return out |
🤖 Prompt for AI Agents
In dascore/io/indexer.py around lines 40 to 49, make cache-file recovery
race-safe and more defensive: when opening the file use path.open("r",
encoding="utf-8") to explicitly decode as UTF-8; after json.load(fi) validate
that the result is a dict and if not replace with an empty dict; and when
removing the corrupt file wrap the remove/unlink in a suppress block (or use
path.unlink(missing_ok=True)) to ignore FileNotFoundError and PermissionError
from concurrent access so deletion won’t raise.
Description
This PR fixes issue #508 by making the logic for reading the index_map file more robust to corrupt files.
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Bug Fixes
Tests