fix: content-hash echo cache + heartbeat + directory rename#10
Merged
Conversation
Core changes across the sync stack: * client.py — re-enable websockets lib's own ping with generous timeout (ping_interval=45, ping_timeout=90). Auto-pongs to the server's 25s pings keep its 60s deadline refreshed; the long timeout tolerates Pong delay behind large binary chunk writes that was tripping keepalive at 30s and 60s during initial sync. * note_sync.py / file_sync.py — replace the 0.6s/2s time-based ignore with a content-hash echo cache. The sleep-in-handler serialized the whole WS receive loop: 178 modifies × 2s ate the 300s sync window. _echo_hashes[path] now tracks the last known synced state and is updated on BOTH inbound (server write) and outbound (push) so a legitimate revert to a previous value still propagates. _DELETED sentinel for deletes, with the same outbound update so recreate + re-delete works. push_modify accepts force=True and NoteSyncNeedPush uses it to bypass the echo check. * watcher.py — directory rename now enumerates the new tree and schedules per-file transitions. _schedule_move_transition handles the excluded-path edge cases: move into excluded → emit delete of old, move out of excluded → emit modify of new. * doc/server-protocol.md — new reference distilled from the Go source: heartbeat protocol, broadcast mechanics, sync flow, and the specific client pitfalls we hit. * tests — new test_echo_cache.py covers the revert, tombstone-reuse, NeedPush-force, and failed-write-no-poison cases; test_file_sync adds the excluded-path rename transitions. 23 unit tests pass. Bidirectional integration tests (not committed) confirm notes, attachments, and folders sync correctly in both directions for create / modify / delete / rename.
Reviewer's GuideRefactors websocket heartbeat handling to rely on the websockets library ping/pong, replaces the sleep-based ignore window with a content-hash based echo cache for notes and files, and makes watcher directory moves schedule per-file transitions with proper ignored/excluded handling, backed by new regression tests and protocol documentation. Sequence diagram for WebSocket heartbeat and dead-connection detectionsequenceDiagram
participant Client
participant WebsocketsLib
participant Server
Client->>WebsocketsLib: connect(url, ping_interval=45, ping_timeout=90)
WebsocketsLib->>Server: WebSocket handshake
Server-->>WebsocketsLib: Authorization response
WebsocketsLib-->>Client: connection established
loop Server ping loop (every 25s)
Server-->>WebsocketsLib: Ping
WebsocketsLib-->>Server: Pong (auto)
end
loop Client ping loop (every 45s)
WebsocketsLib-->>Server: Ping (auto)
Server-->>WebsocketsLib: Pong
end
alt Server stops responding or path is half-open
WebsocketsLib-->>Client: ping timeout after 90s
Client->>WebsocketsLib: close()
end
Sequence diagram for note echo-cache on modify/delete and NeedPushsequenceDiagram
participant Watcher
participant NoteSync
participant Server
rect rgb(235,235,255)
Note over Watcher,NoteSync: Outbound modify with echo suppression
Watcher->>NoteSync: push_modify(rel_path, force=False)
NoteSync->>NoteSync: read file, compute hash_ = content_hash(text)
alt hash_ equals NoteSync._echo_hashes[rel_path]
NoteSync-->>Watcher: return (skip echo)
else hash_ differs or missing
NoteSync->>Server: ACTION_NOTE_MODIFY(contentHash=hash_)
NoteSync->>NoteSync: _echo_hashes[rel_path] = hash_
end
end
rect rgb(235,255,235)
Note over Server,NoteSync: Inbound modify broadcast from server
Server-->>NoteSync: NoteSyncModify(path, content)
NoteSync->>NoteSync: write file to disk
NoteSync->>NoteSync: _echo_hashes[path] = content_hash(content)
NoteSync-->>Watcher: filesystem write triggers local modify event
Watcher->>NoteSync: push_modify(path, force=False)
NoteSync->>NoteSync: hash_ matches _echo_hashes[path] -> skip
end
rect rgb(255,235,235)
Note over Watcher,NoteSync: Delete and tombstone reuse
Watcher->>NoteSync: push_delete(path)
alt NoteSync._echo_hashes[path] == _DELETED
NoteSync-->>Watcher: return (skip echo)
else
NoteSync->>Server: ACTION_NOTE_DELETE
NoteSync->>NoteSync: _echo_hashes[path] = _DELETED
end
Note over Watcher,NoteSync: Later user recreates same path
Watcher->>NoteSync: push_modify(path, force=False)
NoteSync->>NoteSync: hash_ != _DELETED -> send modify and update cache
end
rect rgb(255,255,210)
Note over Server,NoteSync: NeedPush forcing a resend
Server-->>NoteSync: NoteSyncNeedPush(path)
NoteSync->>NoteSync: log request
NoteSync->>NoteSync: call push_modify(path, force=True)
NoteSync->>NoteSync: compute hash_ (no echo check)
NoteSync->>Server: ACTION_NOTE_MODIFY(contentHash=hash_)
NoteSync->>NoteSync: _echo_hashes[path] = hash_
end
Sequence diagram for file echo-cache with chunked download and renamesequenceDiagram
participant Watcher
participant FileSync
participant Server
rect rgb(235,235,255)
Note over Watcher,FileSync: Outbound file upload with echo suppression
Watcher->>FileSync: push_upload(path)
FileSync->>FileSync: hash_ = file_content_hash_binary(full)
alt hash_ equals FileSync._echo_hashes[path]
FileSync-->>Watcher: return (skip upload)
else
FileSync->>Server: FileUploadCheck(contentHash=hash_)
FileSync->>FileSync: _echo_hashes[path] = hash_
end
end
rect rgb(235,255,235)
Note over Server,FileSync: Chunked download finalization
Server-->>FileSync: binary chunks for session_id
FileSync->>FileSync: buffer chunks in _download_sessions
Server-->>FileSync: final chunk
FileSync->>FileSync: _finalize_download(session_id)
FileSync->>FileSync: write all chunks to disk
FileSync->>FileSync: _echo_hashes[path] = file_content_hash_binary(full)
FileSync-->>Watcher: filesystem write triggers local event
Watcher->>FileSync: push_upload(path)
FileSync->>FileSync: hash_ matches cache → skip
end
rect rgb(255,235,235)
Note over Watcher,FileSync: Delete and rename handling in cache
Watcher->>FileSync: push_delete(old_path)
alt FileSync._echo_hashes[old_path] == _DELETED
FileSync-->>Watcher: return (skip delete)
else
FileSync->>Server: FileDelete
FileSync->>FileSync: _echo_hashes[old_path] = _DELETED
end
Server-->>FileSync: FileSyncRename(old_path, new_path)
FileSync->>FileSync: _echo_hashes[old_path] = _DELETED
FileSync->>FileSync: rename on disk old_full -> new_full
FileSync->>FileSync: _echo_hashes[new_path] = file_content_hash_binary(new_full)
end
Sequence diagram for directory rename handling with excluded pathssequenceDiagram
participant Watchdog
participant Watcher
participant SyncEngine
Note over Watchdog,Watcher: Directory rename observed by watchdog
Watchdog-->>Watcher: on_moved(event is_directory=True)
Watcher->>Watcher: _handle_directory_move(event)
Watcher->>Watcher: old_dir_rel = _rel(event.src_path)
Watcher->>Watcher: new_dir_rel = _rel(event.dest_path)
Watcher->>Watcher: iterate new_dir.rglob("*")
loop For each child file under renamed directory
Watcher->>Watcher: new_rel = _rel(child)
Watcher->>Watcher: compute tail and old_rel from prefixes
Watcher->>Watcher: _schedule_move_transition(old_rel, new_rel)
alt both old_rel and new_rel ignored
Watcher-->>Watcher: return
else old_rel or new_rel ignored
Watcher-->>Watcher: return
else both excluded
Watcher-->>Watcher: return
else moved from included to excluded
Watcher->>SyncEngine: on_local_delete(old_rel) (scheduled)
else moved from excluded to included
Watcher->>SyncEngine: on_local_change(new_rel) (scheduled)
else normal rename (both included)
Watcher->>SyncEngine: on_local_rename(new_rel, old_rel) (scheduled)
end
end
Updated class diagram for sync client, echo cache, and directory watcherclassDiagram
class Client {
-config: AppConfig
-ws: WebSocket
-_on_reconnect: Callable
-_msg_queue: list
-_ready_event: asyncio.Event
+on_reconnect(handler: Callable) void
+connect() Coroutine
-_connect() Coroutine
-_listen() Coroutine
-_handle_text(raw: str) Coroutine
-_handle_binary(raw: bytes) Coroutine
}
class NoteSync {
-engine: SyncEngine
-config: AppConfig
-vault_path: Path
-_received_modify: int
-_received_delete: int
-_got_end: bool
-_echo_hashes: dict~str, str~
+is_sync_complete: bool
+request_full_sync() Coroutine
+push_modify(rel_path: str, force: bool=False) Coroutine
+push_delete(rel_path: str) Coroutine
+push_rename(new_rel: str, old_rel: str) Coroutine
-_on_sync_modify(msg: WSMessage) Coroutine
-_on_sync_delete(msg: WSMessage) Coroutine
-_on_sync_need_push(msg: WSMessage) Coroutine
-_on_sync_end(msg: WSMessage) Coroutine
}
class FileSync {
-engine: SyncEngine
-config: AppConfig
-vault_path: Path
-_received_modify: int
-_received_delete: int
-_got_end: bool
-_echo_hashes: dict~str, str~
-_download_sessions: dict
+is_sync_complete: bool
+push_upload(rel_path: str) Coroutine
+push_delete(rel_path: str) Coroutine
-_on_sync_update(msg: WSMessage) Coroutine
-_on_sync_delete(msg: WSMessage) Coroutine
-_on_sync_rename(msg: WSMessage) Coroutine
-_on_binary_chunk(session_id: str, chunk_index: int, data: bytes) Coroutine
-_finalize_download(session_id: str, session: _DownloadSession) Coroutine
-_on_sync_end(msg: WSMessage) Coroutine
}
class Watcher {
-engine: SyncEngine
-_schedule(label: str, fn: Callable) void
-_rel(path: str) str
+on_modified(event: FileSystemEvent) void
+on_deleted(event: FileSystemEvent) void
+on_moved(event: FileSystemEvent) void
-_handle_directory_move(event: FileSystemEvent) void
-_schedule_move_transition(old_rel: str, new_rel: str) void
}
class SyncEngine {
+ws_client: Client
+note_sync: NoteSync
+file_sync: FileSync
+is_ignored(rel_path: str) bool
+is_excluded(rel_path: str) bool
+on_local_change(rel_path: str) Coroutine
+on_local_delete(rel_path: str) Coroutine
+on_local_rename(new_rel: str, old_rel: str) Coroutine
}
Client --> NoteSync : used via ws_client
Client --> FileSync : used via ws_client
SyncEngine o-- Client
SyncEngine o-- NoteSync
SyncEngine o-- FileSync
Watcher --> SyncEngine
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
FileSync._on_sync_deletethe_echo_hashes[rel_path] = _DELETEDassignment happens before the filesystem delete; this differs fromNoteSync._on_sync_deleteand can poison the cache if the unlink fails—consider moving the assignment into the successful path of thetryblock to keep the semantics consistent with notes and the regression tests’ intent. - The
test_failed_inbound_delete_does_not_poison_cachetest intest_echo_cache.pycompares against the literal string"_DELETED"rather than the actual_DELETEDsentinel value, so it won’t fail if the cache is mistakenly set to"__deleted__"; importing and asserting against the real sentinel (or checking that the path is absent from_echo_hashes) would make this regression test robust.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `FileSync._on_sync_delete` the `_echo_hashes[rel_path] = _DELETED` assignment happens before the filesystem delete; this differs from `NoteSync._on_sync_delete` and can poison the cache if the unlink fails—consider moving the assignment into the successful path of the `try` block to keep the semantics consistent with notes and the regression tests’ intent.
- The `test_failed_inbound_delete_does_not_poison_cache` test in `test_echo_cache.py` compares against the literal string `"_DELETED"` rather than the actual `_DELETED` sentinel value, so it won’t fail if the cache is mistakenly set to `"__deleted__"`; importing and asserting against the real sentinel (or checking that the path is absent from `_echo_hashes`) would make this regression test robust.
## Individual Comments
### Comment 1
<location path="fns_cli/note_sync.py" line_range="112-115" />
<code_context>
await self.engine.ws_client.send(msg)
- async def push_modify(self, rel_path: str) -> None:
+ async def push_modify(self, rel_path: str, *, force: bool = False) -> None:
full = self.vault_path / rel_path
if not full.exists():
</code_context>
<issue_to_address>
**issue (bug_risk):** Echo suppression in `push_modify` can suppress the "new" side of a rename if the target path previously existed with identical content.
Since `push_rename` calls `push_modify(new_rel)` then `push_delete(old_rel)`, a rename into a path already present in `_echo_hashes` with the same hash (e.g. reusing an old filename with identical contents) will skip the modify and only send a delete for `old_rel`. The server never learns to create `new_rel`. Consider skipping echo suppression for the new side of a rename (similar to `force=True` handling), or adding an explicit rename operation to preserve correct semantics.
</issue_to_address>
### Comment 2
<location path="tests/test_echo_cache.py" line_range="174-183" />
<code_context>
+ f"expected local push after failed inbound write, got {actions}",
+ )
+
+ async def test_failed_inbound_delete_does_not_poison_cache(self):
+ """A failed server delete must not suppress a later real local delete."""
+ rel = "note.md"
+ target = self.vault / rel
+ target.write_text("A", encoding="utf-8")
+
+ with patch.object(Path, "unlink", side_effect=OSError("boom")):
+ await self._inbound_delete(rel)
+
+ self.assertNotEqual(self.ns._echo_hashes.get(rel), "_DELETED")
+
+ target.unlink()
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid string-literal comparison to the deletion sentinel; import and assert against the actual _DELETED constant instead.
This test currently checks `self.ns._echo_hashes.get(rel) != "_DELETED"` while the implementation uses a `_DELETED` constant. Comparing to a string literal makes the test brittle if the sentinel changes. Please import `_DELETED` from `fns_cli.note_sync` and assert against that constant (or assert that the key is absent / not equal to the sentinel object). Apply the same approach to any similar checks for the file sync echo cache.
Suggested implementation:
```python
self.assertNotEqual(self.ns._echo_hashes.get(rel), _DELETED)
```
1. At the top of `tests/test_echo_cache.py`, add an import for the sentinel constant:
- `from fns_cli.note_sync import _DELETED`
Place this alongside the other `fns_cli` imports to match existing style.
2. Search the rest of `tests/test_echo_cache.py` for any other string-literal comparisons to the deletion sentinel (e.g. `"_DELETED"`), and update them to use the imported `_DELETED` constant in the same way:
- Replace `"_DELETED"` with `_DELETED` in those assertions or checks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+112
to
115
| async def push_modify(self, rel_path: str, *, force: bool = False) -> None: | ||
| full = self.vault_path / rel_path | ||
| if not full.exists(): | ||
| return |
There was a problem hiding this comment.
issue (bug_risk): Echo suppression in push_modify can suppress the "new" side of a rename if the target path previously existed with identical content.
Since push_rename calls push_modify(new_rel) then push_delete(old_rel), a rename into a path already present in _echo_hashes with the same hash (e.g. reusing an old filename with identical contents) will skip the modify and only send a delete for old_rel. The server never learns to create new_rel. Consider skipping echo suppression for the new side of a rename (similar to force=True handling), or adding an explicit rename operation to preserve correct semantics.
Comment on lines
+174
to
+183
| async def test_failed_inbound_delete_does_not_poison_cache(self): | ||
| """A failed server delete must not suppress a later real local delete.""" | ||
| rel = "note.md" | ||
| target = self.vault / rel | ||
| target.write_text("A", encoding="utf-8") | ||
|
|
||
| with patch.object(Path, "unlink", side_effect=OSError("boom")): | ||
| await self._inbound_delete(rel) | ||
|
|
||
| self.assertNotEqual(self.ns._echo_hashes.get(rel), "_DELETED") |
There was a problem hiding this comment.
suggestion (testing): Avoid string-literal comparison to the deletion sentinel; import and assert against the actual _DELETED constant instead.
This test currently checks self.ns._echo_hashes.get(rel) != "_DELETED" while the implementation uses a _DELETED constant. Comparing to a string literal makes the test brittle if the sentinel changes. Please import _DELETED from fns_cli.note_sync and assert against that constant (or assert that the key is absent / not equal to the sentinel object). Apply the same approach to any similar checks for the file sync echo cache.
Suggested implementation:
self.assertNotEqual(self.ns._echo_hashes.get(rel), _DELETED)- At the top of
tests/test_echo_cache.py, add an import for the sentinel constant:from fns_cli.note_sync import _DELETED
Place this alongside the otherfns_cliimports to match existing style.
- Search the rest of
tests/test_echo_cache.pyfor any other string-literal comparisons to the deletion sentinel (e.g."_DELETED"), and update them to use the imported_DELETEDconstant in the same way:- Replace
"_DELETED"with_DELETEDin those assertions or checks.
- Replace
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ping_interval=45, ping_timeout=90) so a genuine dead connection is still detected without false-positives during heavy chunk downloads. Fixes issue [Bug] 30秒没有更新之后，会自动断开连接 #9 (30s auto-disconnect).ignore_filewindow with a content-hash echo cache, and move the 2sawait asyncio.sleep(...)out of the WS receive handler (it serialized the receive loop and blew the 300s NoteSync timeout on initial sync). Cache is updated on both inbound and outbound, so a revert to an earlier value — and a delete after recreate — still propagate.watcher.on_movednow enumerates the renamed tree and schedules per-file transitions, including the edge cases of moving into or out of an excluded path.Context / new doc
doc/server-protocol.mdis a new reference distilled from the fast-note-sync-service Go source — heartbeat protocol, broadcast mechanics, sync flow, and the specific client pitfalls we hit (including the ones this PR fixes).Test plan
pytest -q— 23 passed (4 new regression tests intests/test_echo_cache.py: revert-after-push, delete-after-recreate, NeedPush-force, failed-inbound-no-cache-poison; plus 2 watcher tests for excluded-path rename transitions intests/test_file_sync.py).Notes
client.heartbeat_intervalconfig field is now unused; left in place to avoid breaking existing user configs._echo_hasheshas no eviction — fine for typical vault sizes, but worth bounding if someone runs this on a million-file vault.Summary by Sourcery
Improve WebSocket heartbeat handling and file/note sync echo suppression while fixing directory move handling across ignored/excluded paths.
Bug Fixes:
Enhancements:
Documentation:
Tests: