Skip to content

fix(sync): include needUpload/needSyncMtime in initial pull-ack decision - #22

Closed
lyston11 wants to merge 1 commit into
Go1c:mainfrom
lyston11:fix/sync-initial-ack-needupload
Closed

fix(sync): include needUpload/needSyncMtime in initial pull-ack decision#22
lyston11 wants to merge 1 commit into
Go1c:mainfrom
lyston11:fix/sync-initial-ack-needupload

Conversation

@lyston11

@lyston11 lyston11 commented Aug 28, 2026

Copy link
Copy Markdown

The server (fast-note-sync-service >= 3.5.0) delivers download detail frames (NoteSyncNeedPush / NoteSyncModify / NoteSyncMtime / NoteSyncDelete and their file/setting counterparts) through a paged download channel. It only sends a page after the client sends the initial pull ack (XxxSyncPageAck with pageIndex=-1).

All four sync modules computed total_expected = needModify + needDelete to decide whether to send that initial ack. This silently dropped uploads and mtime-only syncs: when the server reported needUploadCount > 0 but needModify/needDelete == 0 (the common case of local-new-notes-only), total_expected was 0, the initial ack was never sent, the download pump never started, and NoteSyncNeedPush never reached the client — so the new note was never uploaded. The CLI logged "Sync complete" and committed lastTime anyway.

Fix: add _expected_upload / _expected_mtime counters in note_sync, file_sync, and setting_sync (folder_sync has no upload/mtime in its End message), include them in total_expected for both the initial-ack decision and the completion check, and credit the corresponding detail handlers (NoteSyncNeedPush / NoteSyncMtime / NoteSyncRename; FileUpload / FileSyncMtime; SettingSyncNeedUpload / SettingSyncMtime) with the received count so _check_all_received/_check_complete can finish.

Verified end-to-end against fast-note-sync-service 3.6.1: a vault with only a local-new note now triggers the initial ack, receives NoteSyncNeedPush, uploads via NoteModify, and the note lands on the server.

Also ignore local test artifacts (.venv/, config-test.yaml, vault/).

Summary by Sourcery

Prevent sync operations from silently completing without applying upload and mtime-only changes.

Bug Fixes:

  • Ensure sync modules request and process paged server detail frames when uploads or mtime-only changes are pending, preventing missed uploads and premature sync completion.

Enhancements:

  • Track upload and mtime expectations across note, file, and setting synchronization flows and include them in completion accounting.

Chores:

  • Ignore local test artifacts and configuration files.

The server (fast-note-sync-service >= 3.5.0) delivers download detail
frames (NoteSyncNeedPush / NoteSyncModify / NoteSyncMtime / NoteSyncDelete
and their file/setting counterparts) through a paged download channel.
It only sends a page after the client sends the initial pull ack
(XxxSyncPageAck with pageIndex=-1).

All four sync modules computed `total_expected = needModify + needDelete`
to decide whether to send that initial ack. This silently dropped uploads
and mtime-only syncs: when the server reported needUploadCount > 0 but
needModify/needDelete == 0 (the common case of local-new-notes-only),
total_expected was 0, the initial ack was never sent, the download pump
never started, and NoteSyncNeedPush never reached the client — so the
new note was never uploaded. The CLI logged "Sync complete" and committed
lastTime anyway.

Fix: add _expected_upload / _expected_mtime counters in note_sync,
file_sync, and setting_sync (folder_sync has no upload/mtime in its
End message), include them in total_expected for both the initial-ack
decision and the completion check, and credit the corresponding
detail handlers (NoteSyncNeedPush / NoteSyncMtime / NoteSyncRename;
FileUpload / FileSyncMtime; SettingSyncNeedUpload / SettingSyncMtime)
with the received count so _check_all_received/_check_complete can finish.

Verified end-to-end against fast-note-sync-service 3.6.1: a vault with
only a local-new note now triggers the initial ack, receives
NoteSyncNeedPush, uploads via NoteModify, and the note lands on the
server.

Also ignore local test artifacts (.venv/, config-test.yaml, vault/).
@sourcery-ai

sourcery-ai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Reviewer's Guide

Fixes paged sync deadlocks by incorporating upload and mtime counts into initial pull-ack and completion logic across note, file, and setting synchronization, and credits their corresponding detail handlers so upload-only or mtime-only syncs complete correctly.

Sequence diagram for upload-only paged sync

sequenceDiagram
    participant Client
    participant SyncModule
    participant Server

    Server->>SyncModule: SyncEnd(needUploadCount > 0)
    SyncModule->>SyncModule: _on_sync_end()
    SyncModule->>Server: XxxSyncPageAck(pageIndex=-1)
    Server-->>SyncModule: NoteSyncNeedPush / FileUpload / SettingSyncNeedUpload
    SyncModule->>Server: NoteModify via push_modify()
    SyncModule->>SyncModule: _check_all_received()
Loading

Flow diagram for upload and mtime completion accounting

flowchart TD
    A[SyncEnd counts] --> B{total_expected > 0?}
    B -->|No| C[Commit last time]
    B -->|Yes| D[Send initial page ack]
    D --> E[Paged detail handlers]
    E --> F[Credit received count]
    F --> G{total_received >= total_expected?}
    G -->|No| E
    G -->|Yes| H[Complete sync]

    A --> A1[needModify + needDelete + needUpload + needSyncMtime]
    E --> E1[Upload, modify, delete, rename, or mtime detail]
Loading

File-Level Changes

Change Details Files
Include upload and mtime requirements when initiating and completing paged sync downloads.
  • Track needUploadCount and needSyncMtimeCount alongside modify/delete counts.
  • Send the pageIndex=-1 pull acknowledgment when any detail type is expected.
  • Count upload, rename, and mtime detail handlers toward completion.
fns_cli/note_sync.py
fns_cli/file_sync.py
fns_cli/setting_sync.py
Ignore local development and test artifacts.
  • Add virtual-environment, test configuration, and vault paths to Git ignore rules.
.gitignore

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@lyston11 lyston11 closed this Aug 28, 2026

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!

Sourcery assessment

Needs a human reviewer. This changes synchronization behavior so the client begins requesting and applying upload, modification, deletion, and mtime pages that were previously skipped. If the counts or received-item accounting are wrong, the client could apply incomplete or incorrect file updates, and upload stale content to the server; reverting would not undo those already-applied or uploaded changes, though they are generally bounded and repairable by resynchronizing or restoring files.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant