fix(realtime): delete files by absolute path - #1153
Conversation
|
Thanks for this — the root cause and fix are correct. `CYPHER_DELETE_FILE` matches on `absolute_path` ( Approved CI to run. One optional nit (your call, not a blocker): the following debug line still formats logger.debug(logs.DELETION_QUERY.format(path=relative_path_str)) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe realtime watcher now passes resolved absolute paths to ChangesRealtime file deletion
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
|
@coderabbitai review |
|
@greptileai review |
✅ Action performedReview finished.
|
Greptile SummaryRealtime deletion now uses the resolved absolute path that identifies stored File nodes, while module deletion continues to use its project-scoped relative identity. A focused regular-file deletion check passed for the updated handler. Confidence Score: 5/5No blocking failure remains. The focused deletion check confirmed that regular files are deleted using their stored absolute-path identity and that module deletion retains its project scope.
 T-Rex LogsWhat T-Rex did
Reviews (2): Last reviewed commit: "fix(realtime): delete files by absolute ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Delete File node (for all files including non-code like .md, .json) | ||
| ingestor.execute_write(CYPHER_DELETE_FILE, {KEY_PATH: relative_path_str}) | ||
| ingestor.execute_write( | ||
| CYPHER_DELETE_FILE, {KEY_PATH: path.resolve().as_posix()} |
There was a problem hiding this comment.
A deletion event is handled after a symlink has been removed, so path.resolve() now returns the missing symlink pathname rather than the target’s canonical path that was stored in File.absolute_path while the link existed. CYPHER_DELETE_FILE consequently receives an identity that cannot match the indexed File node, leaving stale graph data after a symlinked file is deleted. Preserve the indexed resolved path for watched files, or maintain a lexical-path-to-indexed-path mapping for DELETE events.
Artifacts
Direct pathlib symlink resolve harness source
- This exact authored self-contained Python harness creates a symlink, resolves it before and after unlinking it, and prints both identities, with the takeaway that unlinking changes resolve output from target identity to missing-link identity.
Pathlib symlink resolution before unlink
- This command capture runs the direct harness while the symlink exists and shows resolve equals the canonical target identity, with the takeaway that indexing a live symlink records its target path.
Pathlib symlink resolution after unlink
- This command capture runs the direct harness after unlinking the symlink and shows resolve returns the missing symlink pathname instead of the target path, with the takeaway that the deletion identity does not match the indexed identity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: realtime_updater.py
Line: 270
Comment:
**Deleted symlink identity is lost**
A deletion event is handled after a symlink has been removed, so `path.resolve()` now returns the missing symlink pathname rather than the target’s canonical path that was stored in `File.absolute_path` while the link existed. `CYPHER_DELETE_FILE` consequently receives an identity that cannot match the indexed File node, leaving stale graph data after a symlinked file is deleted. Preserve the indexed resolved path for watched files, or maintain a lexical-path-to-indexed-path mapping for DELETE events.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Good find, and the mechanism is correct: cached_resolve_posix() calls Path.resolve() at ingest (so a symlink is stored under its target's canonical path), and once the link is deleted resolve() can no longer follow it, so the delete key misses.
However this is pre-existing and not introduced by this PR — the batch-delete path already does the same thing at graph_updater.py:1860 (deleted_path.resolve().as_posix(), issue #897). This PR brings the realtime path into parity with it and correctly fixes the common regular-file case, which was the actual bug (relative key vs absolute_path match). The symlink edge case needs a separate identity strategy for both paths, so Iʼve split it out as #1154 rather than block this fix.
@aoright — nothing for you to change here on account of this; the symlink handling is tracked in #1154 and is out of scope for this PR.
There was a problem hiding this comment.
Fair point — the symlink gap predates this PR and is consistently present in the batch path at graph_updater.py:1860, so this change correctly achieves parity rather than introducing a regression. Splitting it to #1154 is the right call.
Acknowledged, no action needed here on that front.
|
@greptileai the symlink-deletion case is a valid but pre-existing gap (shared with the batch path at |
2 participants
Summary
Filenodes with their resolved absolute path so the query matches theFile.absolute_pathidentity introduced by fix: key Folder and File nodes on absolute path to stop cross-project merging #898Moduledeletion on its existing relative-path contractType of Change
Related Issues
Fixes #1141
Test Plan
make test-paralleloruv run pytest -n auto -m "not integration")make test-integration, requires Docker)Regression verification:
Checklist
make pre-commit)# type: ignore,cast(),Any, orobjecttype hintsAI assistance
AI assistance was used to investigate the issue, implement the focused fix, and update regression assertions. The final diff was manually reviewed and validated with the full test and pre-commit suites above.
Summary by CodeRabbit