Make file→CMS upload sync idempotent

[elemdos]

Problem

Every primo dev push re-uploaded unchanged local images instead of no-op'ing. On each push an already-canonicalized upload got another hash suffix appended and a new site_uploads record created server-side, orphaning the previous one. Over many pushes one image became many byte-identical duplicates (favicon_x_y_..._z.svg with stacked _xxxxxxxxxx segments); in a real site 13 images grew into 187 physical files / manifest entries, resending ~18MB per push until it hit This operation was aborted.

Root cause

Upload identity was keyed on the mutable suffixed filename, not content:

• reconcileSiteUploads indexed existing records by file (the suffixed name). When the CLI re-sent a symbolic name (favicon.svg), it missed the record stored as favicon_ab12cd34.svg → fell into the create branch → new record, and PocketBase's NewFileFromBytes re-suffixed unconditionally.
• upsertSiteUpload's no-op hash-read built the storage path from the caller-supplied filename instead of the record's stored name, and swallowed the read error — so a name mismatch silently fell through to the re-suffixing update branch.
• The bootstrap endpoint ran the full import but dropped created_ids from its response, so the CLI's first push never wrote back canonical names (kept re-sending symbolic names forever).

Fix

• reconcileSiteUploads matches incoming files by content hash first (stable identity), filename second — an unchanged image is a no-op regardless of the name sent.
• upsertSiteUpload reads existing bytes by the record's stored filename and returns that stored name as canonical.
• bootstrap response now includes created_ids.

Paired CLI fix (makes the first push write back canonical names): primocms/primo-cli.

Tests

internal/upload_idempotent_test.go — repeated pushes of a byte-identical image (both symbolic and canonical names) stay at exactly 1 record with one stable suffix. Previously the symbolic-name case stacked suffixes and created duplicates on every push.

Full internal suite passes (TestUpdatedTimestampStableOnNoOpReimport fails identically on main — pre-existing, unrelated to uploads).

Follow-up (not in this PR)

The orphaned site_uploads records this bug already created still need a --prune-uploads --dedupe GC pass (group by content hash, keep the referenced/oldest record, repoint refs, delete the rest).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Bootstrap upload responses now include a created_ids field, improving how the CLI tracks newly created records and updates upload references.

• Bug Fixes

  • Re-uploads are now reliably detected as no-ops when content is unchanged, even if the provided filename differs from the stored canonical name.
  • Prevents duplicate site_uploads records and repeated filename suffixing on subsequent pushes.

• Tests

  • Added end-to-end idempotency tests covering both canonical and symbolic filename repeat uploads.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8df5648e-adbb-4d77-b94e-15071fdd1948

📥 Commits

Reviewing files that changed from the base of the PR and between 9bcef84 and c4bce57.

📒 Files selected for processing (1)

• internal/import.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/import.go

---

📝 Walkthrough

Walkthrough

The bootstrap ZIP-import response now returns created_ids. Upload reconciliation uses stored bytes and content hashes to recognize repeated uploads, with tests covering canonical and symbolic filename repushes.

Changes

Upload idempotency

Layer / File(s)	Summary
Expose created upload metadata internal/bootstrap.go, internal/upload_idempotent_test.go	ZIP import success responses include created_ids, and test helpers extract canonical upload filenames from returned manifest metadata.
Reconcile uploads by content hash internal/import.go, internal/upload_idempotent_test.go	Existing uploads are read using stored filenames and matched by content hash before filename fallback; repeated canonical and symbolic pushes verify single-record, stable canonical results.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant handleBootstrap
  participant reconcileSiteUploads
  participant site_uploads
  CLI->>handleBootstrap: ZIP upload with symbolic or canonical filename
  handleBootstrap->>reconcileSiteUploads: reconcile uploaded files
  reconcileSiteUploads->>site_uploads: hash stored bytes and match existing upload
  site_uploads-->>reconcileSiteUploads: existing record or no match
  reconcileSiteUploads-->>handleBootstrap: canonical upload metadata
  handleBootstrap-->>CLI: success response with created_ids

Loading

Possibly related PRs

• primocms/primo#1148: Both changes update upload reconciliation in internal/import.go to use content hashes and avoid duplicate filename suffixing.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: making file-to-CMS upload sync idempotent.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/upload-dedup-idempotent

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

internal/import.go (1)

3500-3500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replace deprecated fsys.GetFile with GetReader.

Static analysis flags fsys.GetFile as deprecated (SA1019); the recommended replacement is GetReader(fileKey). Both changed segments use the deprecated method — line 3500 in upsertSiteUpload and line 3629 in reconcileSiteUploads. Verify that GetReader returns an io.ReadCloser (or equivalent) so the existing reader.Close() calls remain valid.

♻️ Proposed refactor for both call sites

 // Line 3500 in upsertSiteUpload
-reader, readErr := fsys.GetFile(sourceKey)
+reader, readErr := fsys.GetReader(sourceKey)

 // Line 3629 in reconcileSiteUploads
-reader, rerr := fsys.GetFile(uploadsColl.Id + "/" + rec.Id + "/" + fn)
+reader, rerr := fsys.GetReader(uploadsColl.Id + "/" + rec.Id + "/" + fn)

Also applies to: 3629-3629

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/import.go` at line 3500, Replace deprecated fsys.GetFile calls with
fsys.GetReader in upsertSiteUpload and reconcileSiteUploads, passing the same
source key. Confirm the returned reader supports Close so the existing
reader.Close calls remain valid, and preserve current error handling.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/import.go`:
- Line 3500: Replace deprecated fsys.GetFile calls with fsys.GetReader in
upsertSiteUpload and reconcileSiteUploads, passing the same source key. Confirm
the returned reader supports Close so the existing reader.Close calls remain
valid, and preserve current error handling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7f9b761d-71de-4a64-ac44-ad097859ef2b

📥 Commits

Reviewing files that changed from the base of the PR and between 1690022 and 9bcef84.

📒 Files selected for processing (3)

• internal/bootstrap.go
• internal/import.go
• internal/upload_idempotent_test.go

[elemdos]

Good catch — replaced both fsys.GetFile calls with GetReader in c4bce579. GetReader has the identical signature (*blob.Reader, still Close-able), so error handling and reader.Close() are unchanged.