fix: skip re-uploading unchanged attachments to avoid Confluence rollback error

src/confluence/client.ts

@@ -150,49 +150,6 @@ export class ConfluenceClient {
     }
     return result
   }
-
-  async updateAttachment(
-    pageId: string,
-    attachmentId: string,
-    filename: string,
-    data: Buffer,
-    contentType: string,
-  ): Promise<Attachment> {
-    const formData = new FormData()
-    const blob = new Blob([data], { type: contentType })
-    formData.append('file', blob, filename)
-
-    const url = `${this.baseUrl}/content/${pageId}/child/attachment/${attachmentId}/data`
-    const response = await fetch(url, {
-      method: 'POST',
-      headers: {
-        Authorization: this.authHeader,
-        'X-Atlassian-Token': 'nocheck',
-      },
-      body: formData,
-    })
-
-    if (!response.ok) {
-      let errorMessage: string
-      try {
-        const errorBody = (await response.json()) as { message?: string }
-        errorMessage = errorBody.message || response.statusText
-      } catch {
-        errorMessage = response.statusText
-      }
-      throw new ConfluenceApiError({
-        statusCode: response.status,
-        message: errorMessage,
-      })
-    }
-
-    // API may return attachment directly or wrapped in results array
-    const result = (await response.json()) as Attachment | AttachmentSearchResult
-    if ('results' in result) {
-      return result.results[0]
-    }
-    return result
-  }
 }
 
 export class ConfluenceApiError extends Error {

src/sync.ts

@@ -148,25 +148,23 @@ async function uploadAttachments(
   client: ConfluenceClient,
   config: Config,
 ): Promise<void> {
-  // Get existing attachments to avoid duplicates
   const existing = await client.getAttachments(pageId)
-  const existingMap = new Map(existing.map((a) => [a.title, a]))
+  const existingNames = new Set(existing.map((a) => a.title))
 
   for (const [filename, info] of attachments) {
-    const existingAttachment = existingMap.get(filename)
-
-    if (existingAttachment) {
-      // Update existing attachment
-      await client.updateAttachment(pageId, existingAttachment.id, filename, info.data, info.contentType)
-      if (config.verbose) {
-        console.log(`  Updated attachment: ${filename}`)
-      }
-    } else {
-      // Upload new attachment
-      await client.uploadAttachment(pageId, filename, info.data, info.contentType)
+    // Attachment names embed an md5 of their content, so a name match means the bytes are
+    // already identical. Re-uploading is a no-op that Confluence Cloud rejects with
+    // "UnexpectedRollbackException: marked as rollback-only" — skip instead of re-uploading.
+    if (existingNames.has(filename)) {
       if (config.verbose) {
-        console.log(`  Uploaded attachment: ${filename}`)
+        console.log(`  Skipped attachment (unchanged): ${filename}`)
       }
+      continue
+    }
+
+    await client.uploadAttachment(pageId, filename, info.data, info.contentType)
+    if (config.verbose) {
+      console.log(`  Uploaded attachment: ${filename}`)
     }
   }
 }

test/sync.test.ts

@@ -180,6 +180,90 @@ describe('syncFiles', () => {
     })
   })
 
+  describe('attachment upload', () => {
+    // Writes a markdown file referencing a local image, returns the content-hashed
+    // attachment name Confluence would store it under (basename-<md5>.ext).
+    async function withLocalImage(): Promise<{ file: string; attachmentName: string; cleanup: () => void }> {
+      const fs = await import('node:fs')
+      const path = await import('node:path')
+      const os = await import('node:os')
+      const { createHash } = await import('node:crypto')
+
+      const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'markfluence-attach-'))
+      const data = Buffer.from('fake-png-bytes')
+      fs.writeFileSync(path.join(dir, 'pic.png'), data)
+      const file = path.join(dir, 'page.md')
+      fs.writeFileSync(file, '# Title\n\n![pic](./pic.png)\n')
+
+      const hash = createHash('md5').update(data).digest('hex').slice(0, 12)
+      return {
+        file,
+        attachmentName: `pic-${hash}.png`,
+        cleanup: () => fs.rmSync(dir, { recursive: true, force: true }),
+      }
+    }
+
+    const existingPage = {
+      id: '123',
+      title: 'Title',
+      version: { number: 1 },
+      body: { storage: { value: '<p>old</p>' } },
+      _links: { webui: '/pages/123' },
+    }
+    const updatedPage = { id: '123', title: 'Title', version: { number: 2 }, _links: { webui: '/pages/123' } }
+
+    it('skips re-uploading an attachment that already exists (idempotent re-sync)', async () => {
+      const { file, attachmentName, cleanup } = await withLocalImage()
+      try {
+        fetchMock
+          // getPageByTitle
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve({ results: [existingPage] }) })
+          // updatePage
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve(updatedPage) })
+          // getAttachments — the image is already there under the same content-hashed name
+          .mockResolvedValueOnce({
+            ok: true,
+            status: 200,
+            json: () => Promise.resolve({ results: [{ id: 'att-1', title: attachmentName }] }),
+          })
+
+        const results = await syncFiles([file], mockConfig)
+
+        // The file must still be reported as updated, not silently dropped on an attachment error.
+        expect(results).toHaveLength(1)
+        expect(results[0].action).toBe('updated')
+        // getPageByTitle + updatePage + getAttachments, and crucially NO re-upload POST.
+        expect(fetchMock).toHaveBeenCalledTimes(3)
+        expect(fetchMock.mock.calls.some((c) => c[1]?.method === 'POST')).toBe(false)
+      } finally {
+        cleanup()
+      }
+    })
+
+    it('uploads an attachment that is not yet present', async () => {
+      const { file, cleanup } = await withLocalImage()
+      try {
+        fetchMock
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve({ results: [existingPage] }) })
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve(updatedPage) })
+          // getAttachments — nothing there yet
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve({ results: [] }) })
+          // uploadAttachment
+          .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve({ id: 'att-2' }) })
+
+        const results = await syncFiles([file], mockConfig)
+
+        expect(results).toHaveLength(1)
+        expect(results[0].action).toBe('updated')
+        expect(fetchMock).toHaveBeenCalledTimes(4)
+        const postCall = fetchMock.mock.calls.find((c) => c[1]?.method === 'POST')
+        expect(postCall?.[0]).toContain('/child/attachment')
+      } finally {
+        cleanup()
+      }
+    })
+  })
+
   describe('per-file space override', () => {
     it('uses space from frontmatter when available', async () => {
       const fs = await import('node:fs')