From 5b5afdad8c17094dc9a3eedf944057bdb7c2b7e8 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 7 Aug 2025 19:25:08 +0000 Subject: [PATCH 1/3] fix: handle vim atomic write DELETE events without ADD Vim''s atomic write operation generates DELETE_SELF inotify events but no corresponding ADD events since the filename never changes. This caused Basic Memory to lose track of files after editing with vim. - Check if file exists on disk before processing DELETE events - Treat DELETE + file exists as modification (atomic write) - Add comprehensive tests covering atomic write scenarios - Maintain backward compatibility for true deletions - Log atomic write detection for better debugging Fixes #248 Co-authored-by: Paul Hernandez --- src/basic_memory/sync/watch_service.py | 27 ++- tests/sync/test_watch_service_edge_cases.py | 202 ++++++++++++++++++++ 2 files changed, 222 insertions(+), 7 deletions(-) diff --git a/src/basic_memory/sync/watch_service.py b/src/basic_memory/sync/watch_service.py index 022ef3ac..699e400b 100644 --- a/src/basic_memory/sync/watch_service.py +++ b/src/basic_memory/sync/watch_service.py @@ -284,13 +284,26 @@ async def handle_changes(self, project: Project, changes: Set[FileChange]) -> No # Process deletes for path in deletes: if path not in processed: - logger.debug("Processing deleted file", path=path) - await sync_service.handle_delete(path) - self.state.add_event(path=path, action="deleted", status="success") - self.console.print(f"[red]✕[/red] {path}") - logger.info(f"deleted: {path}") - processed.add(path) - delete_count += 1 + # Check if file still exists on disk (vim atomic write edge case) + full_path = directory / path + if full_path.exists() and full_path.is_file(): + # File still exists despite DELETE event - treat as modification + logger.debug("File exists despite DELETE event, treating as modification", path=path) + entity, checksum = await sync_service.sync_file(path, new=False) + self.state.add_event(path=path, action="modified", status="success", checksum=checksum) + self.console.print(f"[yellow]✎[/yellow] {path} (atomic write)") + logger.info(f"atomic write detected: {path}") + processed.add(path) + modify_count += 1 + else: + # File truly deleted + logger.debug("Processing deleted file", path=path) + await sync_service.handle_delete(path) + self.state.add_event(path=path, action="deleted", status="success") + self.console.print(f"[red]✕[/red] {path}") + logger.info(f"deleted: {path}") + processed.add(path) + delete_count += 1 # Process adds for path in adds: diff --git a/tests/sync/test_watch_service_edge_cases.py b/tests/sync/test_watch_service_edge_cases.py index b104ee73..50e18c0c 100644 --- a/tests/sync/test_watch_service_edge_cases.py +++ b/tests/sync/test_watch_service_edge_cases.py @@ -1,5 +1,6 @@ """Test edge cases in the WatchService.""" +from pathlib import Path from unittest.mock import patch import pytest @@ -66,3 +67,204 @@ async def test_handle_changes_empty_set(watch_service, project_config, test_proj # Verify synced_files wasn't changed assert watch_service.state.synced_files == 0 + + +@pytest.mark.asyncio +async def test_handle_vim_atomic_write_delete_still_exists(watch_service, project_config, test_project, sync_service): + """Test vim atomic write scenario: DELETE event but file still exists on disk.""" + project_dir = project_config.home + + # Create initial file and sync it + test_file = project_dir / "vim_test.md" + initial_content = """--- +type: note +title: vim test +--- +# Vim Test +Initial content for atomic write test +""" + test_file.write_text(initial_content) + await sync_service.sync(project_dir) + + # Get initial entity state + initial_entity = await sync_service.entity_repository.get_by_file_path("vim_test.md") + assert initial_entity is not None + initial_checksum = initial_entity.checksum + + # Simulate vim's atomic write: modify content but send DELETE event + # (vim moves original file, creates new content, then deletes old inode) + modified_content = """--- +type: note +title: vim test +--- +# Vim Test +Modified content after atomic write +""" + test_file.write_text(modified_content) + + # Setup DELETE event even though file still exists (vim's atomic write behavior) + changes = {(Change.deleted, str(test_file))} + + # Handle the change + await watch_service.handle_changes(test_project, changes) + + # Verify the entity still exists and was updated (not deleted) + entity = await sync_service.entity_repository.get_by_file_path("vim_test.md") + assert entity is not None + assert entity.id == initial_entity.id # Same entity + assert entity.checksum != initial_checksum # Checksum should be updated + + # Verify the file content was properly synced + actual_content = test_file.read_text() + assert "Modified content after atomic write" in actual_content + + # Check that correct event was recorded (should be "modified", not "deleted") + events = [e for e in watch_service.state.recent_events if e.path == "vim_test.md"] + assert len(events) == 1 + assert events[0].action == "modified" + assert events[0].status == "success" + + +@pytest.mark.asyncio +async def test_handle_true_deletion_vs_vim_atomic(watch_service, project_config, test_project, sync_service): + """Test that true deletions are still handled correctly vs vim atomic writes.""" + project_dir = project_config.home + + # Create and sync two files + atomic_file = project_dir / "atomic_test.md" + delete_file = project_dir / "delete_test.md" + + content = """--- +type: note +--- +# Test File +Content for testing +""" + + atomic_file.write_text(content) + delete_file.write_text(content) + await sync_service.sync(project_dir) + + # For atomic_file: modify content but keep file (vim atomic write scenario) + modified_content = content.replace("Content for testing", "Modified content") + atomic_file.write_text(modified_content) + + # For delete_file: actually delete it (true deletion) + delete_file.unlink() + + # Setup DELETE events for both files + changes = { + (Change.deleted, str(atomic_file)), # File still exists - atomic write + (Change.deleted, str(delete_file)), # File deleted - true deletion + } + + # Handle the changes + await watch_service.handle_changes(test_project, changes) + + # Verify atomic_file was treated as modification (still exists in DB) + atomic_entity = await sync_service.entity_repository.get_by_file_path("atomic_test.md") + assert atomic_entity is not None + + # Verify delete_file was truly deleted (no longer exists in DB) + delete_entity = await sync_service.entity_repository.get_by_file_path("delete_test.md") + assert delete_entity is None + + # Check events were recorded correctly + events = watch_service.state.recent_events + atomic_events = [e for e in events if e.path == "atomic_test.md"] + delete_events = [e for e in events if e.path == "delete_test.md"] + + assert len(atomic_events) == 1 + assert atomic_events[0].action == "modified" + + assert len(delete_events) == 1 + assert delete_events[0].action == "deleted" + + +@pytest.mark.asyncio +async def test_handle_vim_atomic_write_markdown_with_relations(watch_service, project_config, test_project, sync_service): + """Test vim atomic write with markdown files that contain relations.""" + project_dir = project_config.home + + # Create target file for relations + target_file = project_dir / "target.md" + target_content = """--- +type: note +title: Target Note +--- +# Target Note +This is the target of relations. +""" + target_file.write_text(target_content) + + # Create main file with relations + main_file = project_dir / "main.md" + initial_content = """--- +type: note +title: Main Note +--- +# Main Note +This note links to [[Target Note]]. + +- relates_to [[Target Note]] +""" + main_file.write_text(initial_content) + await sync_service.sync(project_dir) + + # Get initial state + main_entity = await sync_service.entity_repository.get_by_file_path("main.md") + assert main_entity is not None + initial_relations = len(main_entity.relations) + + # Simulate vim atomic write with content change that adds more relations + modified_content = """--- +type: note +title: Main Note +--- +# Main Note +This note links to [[Target Note]] multiple times. + +- relates_to [[Target Note]] +- references [[Target Note]] +""" + main_file.write_text(modified_content) + + # Setup DELETE event (vim atomic write) + changes = {(Change.deleted, str(main_file))} + + # Handle the change + await watch_service.handle_changes(test_project, changes) + + # Verify entity still exists and relations were updated + updated_entity = await sync_service.entity_repository.get_by_file_path("main.md") + assert updated_entity is not None + assert updated_entity.id == main_entity.id + + # Verify relations were processed correctly + updated_relations = len(updated_entity.relations) + assert updated_relations >= initial_relations # Should have at least as many relations + + # Check event was recorded as modification + events = [e for e in watch_service.state.recent_events if e.path == "main.md"] + assert len(events) == 1 + assert events[0].action == "modified" + + +@pytest.mark.asyncio +async def test_handle_vim_atomic_write_directory_path_ignored(watch_service, project_config, test_project): + """Test that directories are properly ignored even in atomic write detection.""" + project_dir = project_config.home + + # Create directory + test_dir = project_dir / "test_directory" + test_dir.mkdir() + + # Setup DELETE event for directory (should be ignored) + changes = {(Change.deleted, str(test_dir))} + + # Handle the change - should not cause errors + await watch_service.handle_changes(test_project, changes) + + # Verify no events were recorded for the directory + events = [e for e in watch_service.state.recent_events if "test_directory" in e.path] + assert len(events) == 0 From ef3b6792383d33c1f380b4e488a787e733f3fee0 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 7 Aug 2025 21:31:49 +0000 Subject: [PATCH 2/3] fix: correct path handling in vim atomic write tests The vim atomic write tests were failing because they passed absolute paths to handle_changes(), but the watch service expects relative paths (it converts absolute paths from the file watcher to relative paths internally). Fixed by converting absolute paths to relative paths in test setup to match the actual watch service behavior. Co-authored-by: Paul Hernandez --- tests/sync/test_watch_service_edge_cases.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/sync/test_watch_service_edge_cases.py b/tests/sync/test_watch_service_edge_cases.py index 50e18c0c..cb1dac8a 100644 --- a/tests/sync/test_watch_service_edge_cases.py +++ b/tests/sync/test_watch_service_edge_cases.py @@ -103,7 +103,9 @@ async def test_handle_vim_atomic_write_delete_still_exists(watch_service, projec test_file.write_text(modified_content) # Setup DELETE event even though file still exists (vim's atomic write behavior) - changes = {(Change.deleted, str(test_file))} + # Use relative path like the real watch service would + relative_path = str(test_file.relative_to(project_dir)) + changes = {(Change.deleted, relative_path)} # Handle the change await watch_service.handle_changes(test_project, changes) @@ -153,9 +155,12 @@ async def test_handle_true_deletion_vs_vim_atomic(watch_service, project_config, delete_file.unlink() # Setup DELETE events for both files + # Use relative paths like the real watch service would + atomic_relative = str(atomic_file.relative_to(project_dir)) + delete_relative = str(delete_file.relative_to(project_dir)) changes = { - (Change.deleted, str(atomic_file)), # File still exists - atomic write - (Change.deleted, str(delete_file)), # File deleted - true deletion + (Change.deleted, atomic_relative), # File still exists - atomic write + (Change.deleted, delete_relative), # File deleted - true deletion } # Handle the changes @@ -230,7 +235,9 @@ async def test_handle_vim_atomic_write_markdown_with_relations(watch_service, pr main_file.write_text(modified_content) # Setup DELETE event (vim atomic write) - changes = {(Change.deleted, str(main_file))} + # Use relative path like the real watch service would + relative_path = str(main_file.relative_to(project_dir)) + changes = {(Change.deleted, relative_path)} # Handle the change await watch_service.handle_changes(test_project, changes) @@ -260,7 +267,9 @@ async def test_handle_vim_atomic_write_directory_path_ignored(watch_service, pro test_dir.mkdir() # Setup DELETE event for directory (should be ignored) - changes = {(Change.deleted, str(test_dir))} + # Use relative path like the real watch service would + relative_path = str(test_dir.relative_to(project_dir)) + changes = {(Change.deleted, relative_path)} # Handle the change - should not cause errors await watch_service.handle_changes(test_project, changes) From e01820fbb6ac1db515d3d28bd91a3892176606f9 Mon Sep 17 00:00:00 2001 From: Joe P Date: Wed, 20 Aug 2025 11:52:44 -0600 Subject: [PATCH 3/3] fix: Fix failing watch service edge case tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix tests to pass absolute paths to handle_changes instead of relative paths - Add directory filtering to deletion processing to skip directories without entities - All watch service edge case tests now pass 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude Signed-off-by: Joe P --- src/basic_memory/sync/watch_service.py | 10 +++++++++ tests/sync/test_watch_service_edge_cases.py | 24 ++++++++------------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/basic_memory/sync/watch_service.py b/src/basic_memory/sync/watch_service.py index 699e400b..b8756f05 100644 --- a/src/basic_memory/sync/watch_service.py +++ b/src/basic_memory/sync/watch_service.py @@ -296,6 +296,16 @@ async def handle_changes(self, project: Project, changes: Set[FileChange]) -> No processed.add(path) modify_count += 1 else: + # Check if this was a directory - skip if so + # (we can't tell if the deleted path was a directory since it no longer exists, + # so we check if there's an entity in the database for it) + entity = await sync_service.entity_repository.get_by_file_path(path) + if entity is None: + # No entity means this was likely a directory - skip it + logger.debug(f"Skipping deleted path with no entity (likely directory), path={path}") + processed.add(path) + continue + # File truly deleted logger.debug("Processing deleted file", path=path) await sync_service.handle_delete(path) diff --git a/tests/sync/test_watch_service_edge_cases.py b/tests/sync/test_watch_service_edge_cases.py index cb1dac8a..dac33ac8 100644 --- a/tests/sync/test_watch_service_edge_cases.py +++ b/tests/sync/test_watch_service_edge_cases.py @@ -1,6 +1,5 @@ """Test edge cases in the WatchService.""" -from pathlib import Path from unittest.mock import patch import pytest @@ -103,9 +102,8 @@ async def test_handle_vim_atomic_write_delete_still_exists(watch_service, projec test_file.write_text(modified_content) # Setup DELETE event even though file still exists (vim's atomic write behavior) - # Use relative path like the real watch service would - relative_path = str(test_file.relative_to(project_dir)) - changes = {(Change.deleted, relative_path)} + # Use absolute path like the real watch service would + changes = {(Change.deleted, str(test_file))} # Handle the change await watch_service.handle_changes(test_project, changes) @@ -155,12 +153,10 @@ async def test_handle_true_deletion_vs_vim_atomic(watch_service, project_config, delete_file.unlink() # Setup DELETE events for both files - # Use relative paths like the real watch service would - atomic_relative = str(atomic_file.relative_to(project_dir)) - delete_relative = str(delete_file.relative_to(project_dir)) + # Use absolute paths like the real watch service would changes = { - (Change.deleted, atomic_relative), # File still exists - atomic write - (Change.deleted, delete_relative), # File deleted - true deletion + (Change.deleted, str(atomic_file)), # File still exists - atomic write + (Change.deleted, str(delete_file)), # File deleted - true deletion } # Handle the changes @@ -235,9 +231,8 @@ async def test_handle_vim_atomic_write_markdown_with_relations(watch_service, pr main_file.write_text(modified_content) # Setup DELETE event (vim atomic write) - # Use relative path like the real watch service would - relative_path = str(main_file.relative_to(project_dir)) - changes = {(Change.deleted, relative_path)} + # Use absolute path like the real watch service would + changes = {(Change.deleted, str(main_file))} # Handle the change await watch_service.handle_changes(test_project, changes) @@ -267,9 +262,8 @@ async def test_handle_vim_atomic_write_directory_path_ignored(watch_service, pro test_dir.mkdir() # Setup DELETE event for directory (should be ignored) - # Use relative path like the real watch service would - relative_path = str(test_dir.relative_to(project_dir)) - changes = {(Change.deleted, relative_path)} + # Use absolute path like the real watch service would + changes = {(Change.deleted, str(test_dir))} # Handle the change - should not cause errors await watch_service.handle_changes(test_project, changes)