Skip to content

feat(upload): skip already-uploaded photos via the shared upload ledger - #93

Merged
AnnatarHe merged 1 commit into
mainfrom
claude/ecstatic-hopper-mhy12r
Oct 17, 2026
Merged

AnnatarHe merged 1 commit into
mainfrom
claude/ecstatic-hopper-mhy12r

Conversation

@AnnatarHe

@AnnatarHe AnnatarHe commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Why

A successful SFTP close doesn't say whether Rawback stored the photo, found it to be a duplicate, or failed to record it. Until now, every photos upload re-read EXIF for every file and asked the server again. Photos with no capture time, which the server can't deduplicate, were uploaded again on every run.

rawback-app/sdk#57 (released in 0.3.6) added a shared upload ledger, ~/.rawback/upload-ledger.jsonl, and a batched server check. This PR wires both into the CLI's own transfer loop. Desktop gets the same through the SDK's UploadManager in a separate PR.

What

  • Fast skip before any metadata read. Files the ledger settles are skipped at the cost of one stat:

    • a server-confirmed photo
    • a photo with no capture time that was transferred before

    A photo whose confirmation request failed still goes to the server's check. Its capture time comes from the ledger, so it skips the EXIF read. This is the same rule the SDK's UploadManager applies.

  • Moved or copied photos. A photo confirmed at another path is skipped by filename + capture time, and recorded at its new path.

  • Acknowledgement. After the transfer, the uploaded photos are checked with the server through matchUploadIdentities.

    • Matches are recorded as server with their image id.
    • Misses are forgotten, so the next run uploads them again.

    The summary now shows Confirmed, and Unconfirmed when there are any. Each confirmed photo is one ledger line.

  • Remote matches found at review are recorded with their image ids.

  • Legacy resume record. The upload-state.json resume record now only skips photos with no capture time. Other photos are checked with the server once and remembered in the ledger after that. So a photo the server never recorded is no longer skipped forever.

  • --recheck ignores the ledger and the resume record and asks the server about every file. What it learns is still recorded.

  • Remote check. It now uses the SDK's batched helper (500 per request) instead of its own 200-per-request loop. It still fails open.

  • Ledger location. The ledger sits beside the upload state, so tests that point statePath at a temp dir never touch ~/.rawback.

  • Output. The dry-run's "Completed" field is now "Already uploaded". A dry run never writes the ledger.

  • SDK bump. @rawback/sdk goes to 0.3.6. This also brings bun.lock back in line with package.json (it was pinned at 0.3.6 but locked at 0.3.5).

  • Docs. docs/commands.md and docs/configuration.md gain a new "Upload acknowledgements" section, the README file table and AGENTS.md list the new file, and I fixed a garbled paragraph in the upload behaviour section.

Testing

  • bun run check passes: typecheck, 741 tests, lint, format, and the compiled binary build.

  • New tests in test/upload.test.ts:

    • A second run reads no metadata and asks the server nothing.
    • An unconfirmed photo uploads again.
    • A failed confirmation is rechecked without an EXIF read.
    • A moved photo is matched by identity and its new path recorded.
    • --recheck checks every file.
    • A photo with no capture time is remembered with a fresh upload state.
    • An unreadable ledger fails open.
    • A dry run writes no ledger.

    The help test covers --recheck.

  • End to end. I ran the real runUpload against an ssh2 SFTP server, through the CLI's real SftpClient and real exiftool. The server records photos on CLOSE and fails CLOSE for one file.

    • Run 1 sent 4 files: 2 were confirmed with image ids, the PNG with no EXIF was kept as transfer, and the refused file failed.
    • Run 2 used a fresh upload state with the same ledger. It sent and checked only the refused file.

Generated by Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

`photos upload` now uses the SDK 0.3.6 upload ledger
(~/.rawback/upload-ledger.jsonl, shared with Desktop) and confirms each
upload with the server.

- Before reading any metadata, files the ledger settles are skipped: a
  server-confirmed photo, or a photo without a capture time that was
  transferred before. A photo whose confirmation failed keeps going to
  the server's check, with its capture time taken from the ledger.
- A photo confirmed at another path (moved folder, second copy) is
  skipped by filename + capture time and recorded at its new path.
- The server's existing photos found at review are recorded with their
  image ids.
- After the transfer, uploaded photos are confirmed with
  `matchUploadIdentities`: matches are recorded as `server`; photos the
  server does not have are forgotten and uploaded again next time. The
  summary reports Confirmed and Unconfirmed.
- The upload-state resume record now only skips photos without a capture
  time; the rest are checked with the server once and remembered.
- `--recheck` ignores local history and asks the server about every file.
- The remote check uses the SDK's batched helper instead of its own loop.
- Bumps @rawback/sdk to 0.3.6, which also brings bun.lock back in line
  with package.json.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013P4VoitjQTCoqBXwDacJQi
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-10-17T03:25:38.074885Z 25984c3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@AnnatarHe

Copy link
Copy Markdown
Contributor Author

/claude review

@claude

claude Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Claude finished @AnnatarHe's task in 25s —— View job


Review of PR #93: skip already-uploaded photos via the shared upload ledger

I read the src/ diff against origin/main. I didn't run bun run check or the tests, and I didn't read the test and docs changes. Your PR description reports the check passing.

The design holds together. Fast skip comes before the EXIF read, the ledger and resume record keep distinct trust rules, and every ledger failure degrades to a warning. I found no blocking bugs, only the points below.

  • Read the diff
  • Check the skip, acknowledge and flush paths

Worth considering

  1. Server lag could cause re-uploads (src/upload-ledger.ts, acknowledge).

    • matchUploadIdentities runs once, right after the last SFTP close.
    • If the server finishes recording a photo slightly after its CLOSE, a photo that did upload is reported as a miss. It is then forget-ed, counted as Unconfirmed, and uploaded again next run.
    • Your e2e server records on CLOSE, so it wouldn't show this.
    • Is there a latency guarantee from the gateway?
    • If not, a short retry for the misses only (once, after a brief delay) would avoid a re-upload. The cost is a few seconds at the end of a run.
    • The warning says "will be uploaded again", so the behaviour is at least honest.
  2. A crash or exception before acknowledge loses the acknowledgements (src/upload.ts, around await acks.acknowledge(...)).

    • It only runs after the retry loop and progress.finish().
    • A thrown error between those, such as a SIGKILL, leaves uploaded photos covered only by the legacy resume record.
    • The resume record no longer skips photos that have a capture time. The next run therefore asks the server once, which is correct and cheap, so this is only a note.
    • Wrapping the acknowledge call in a finally would also record photos from a cancelled or partly failed run, if you want that.
  3. skipByIdentity copies entry.source onto the new path (src/upload-ledger.ts).

    • A moved or copied photo gets a source: 'upload' entry. That entry wasn't produced by an upload at that path.
    • It's harmless if the SDK treats source as informational. If UploadManager keys any behaviour on it, a distinct value would be cleaner.
  4. Dry run depends on a flush that never happens (src/upload.ts).

    • Dry runs write no ledger only because the early return precedes await acks.flush(). acks.record(...) has already queued entries by then.
    • That is correct today, but a later reorder would silently start writing in dry runs.
    • A guard in UploadAcknowledgements, such as a readonly flag that turns record and flush into no-ops, would make it structural.
    • Your dry-run test covers the current behaviour.

Minor

  • review.recheck uses entry.capturedAt! (src/upload-ledger.ts). It is safe because isSettled returns false only when capturedAt is truthy. A destructure or an explicit guard would avoid the non-null assertion, which the repo guidance steers away from.
  • remoteIdentities still returns an empty map on failure, so --recheck plus an outage uploads everything and relies on the gateway to deduplicate. This is the existing fail-open behaviour, and the warning covers it.
  • The AGENTS.md and README.md updates look consistent with the CLAUDE.md "Documentation changes" rule. I didn't verify that docs/commands.md lists --recheck in the options table.

@AnnatarHe
AnnatarHe merged commit 0279514 into main Oct 17, 2026
6 of 7 checks passed
@AnnatarHe
AnnatarHe deleted the claude/ecstatic-hopper-mhy12r branch October 17, 2026 06:37
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.

2 participants