attachments sync: attach offline, upload when linked, removals stick (#5168)

Desktop could not create an attachment at all, and a removed attachment or
dismissed preview came back on the next pull. Now:

- core: add_attachment keeps the bytes in the blob store and queues the row
  (schema v10: attachments.uploaded / upload_error). Push uploads it once its
  note has landed. A refusal that retrying won't fix (too large, id clash, hash
  mismatch) is recorded on the file and not re-sent every cycle; the editor
  shows it.
- core: removing a synced attachment or dismissing a preview leaves a tombstone
  in pending_deletes; push sends it as an `attachment`/`preview` delete, and a
  pull while it waits doesn't put the row back. A pull also keeps files still
  waiting to upload instead of replacing them wholesale.
- server: PUT /api/sync/attachments/<id> (raw body, sha256-checked, idempotent,
  size-capped) and child deletes in push, which apply regardless of LWW and
  answer noop for rows the caller can't see. One store_attachment helper for
  the upload route, the importer and sync. Protocol 5, feature attachment_sync;
  the client sends neither to a server without it.
- server: migration 0031 makes a link preview's insert/delete bump its note, so
  background-fetched previews and web dismissals reach linked devices.
- desktop: Attach and paste-image work offline (raw-bytes IPC command).
- SVG is served as a download by the desktop blob scheme too (as #1981 did for
  the web), and drawn as a file chip on both.
- autosync: drop the catch_unwind; release builds abort on panic, so it only
  ever worked in debug builds.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

core/src/local/models.rs

@@ -55,6 +55,10 @@ pub struct Attachment {
     pub mime: String,
     pub size: Option<i64>,
     pub sha256: Option<String>,
+    /// Why the server refused a file attached on this device, in words to show on it.
+    /// Absent for everything else, including a file still waiting to upload.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub upload_error: Option<String>,
 }
 
 #[derive(Serialize)]

core/src/local/schema.rs

@@ -274,6 +274,23 @@ UPDATE saved_filters
    AND params LIKE '%"color"%';
 "#;
 
+// v10 (#5168): an attachment can be created on THIS device.
+//
+// Until now every attachment row arrived on the delta feed, so every one was already on
+// the server. A file attached here, offline, is not, and `uploaded = 0` is the queue
+// push drains once the note has landed. The rows already here all came from the
+// server, which is why the default is 1.
+//
+// `upload_error` holds a refusal that retrying won't fix — over the size limit, an id
+// already in use, bytes that don't match their hash. A row carrying one leaves the
+// queue: a file refused for its size would otherwise be re-sent in full on every
+// cycle, every five minutes, for as long as the app was open. The message is what the
+// editor shows on the file instead.
+const SCHEMA_V10: &str = r#"
+ALTER TABLE attachments ADD COLUMN uploaded INTEGER NOT NULL DEFAULT 1;
+ALTER TABLE attachments ADD COLUMN upload_error TEXT;
+"#;
+
 pub fn migrate(conn: &Connection) -> rusqlite::Result<()> {
     conn.execute_batch("PRAGMA foreign_keys = ON;")?;
     let version: i64 = conn.query_row("PRAGMA user_version", [], |r| r.get(0))?;
@@ -313,6 +330,10 @@ pub fn migrate(conn: &Connection) -> rusqlite::Result<()> {
         conn.execute_batch(SCHEMA_V9)?;
         conn.execute_batch("PRAGMA user_version = 9;")?;
     }
+    if version < 10 {
+        conn.execute_batch(SCHEMA_V10)?;
+        conn.execute_batch("PRAGMA user_version = 10;")?;
+    }
     Ok(())
 }
 
@@ -427,7 +448,34 @@ mod tests {
         let version: i64 = conn
             .query_row("PRAGMA user_version", [], |r| r.get(0))
             .expect("version");
-        assert_eq!(version, 9);
+        assert_eq!(version, 10);
+    }
+
+    /// Every attachment that predates v10 came down the feed, so it is already on the
+    /// server. Defaulting it to "waiting to upload" would re-send every file once.
+    #[test]
+    fn v10_counts_existing_attachments_as_already_on_the_server() {
+        let conn = v7_db();
+        migrate_v8(&conn).expect("v8");
+        conn.execute_batch(SCHEMA_V9).expect("v9");
+        conn.execute_batch("PRAGMA user_version = 9;").expect("v9");
+        add_note(&conn, "n", "a note");
+        conn.execute(
+            "INSERT INTO attachments (id, note_id, url) VALUES ('a', 'n', '/x')",
+            [],
+        )
+        .expect("seed");
+
+        migrate(&conn).expect("migrate");
+        let (uploaded, error): (bool, Option<String>) = conn
+            .query_row(
+                "SELECT uploaded, upload_error FROM attachments WHERE id = 'a'",
+                [],
+                |r| Ok((r.get(0)?, r.get(1)?)),
+            )
+            .expect("row");
+        assert!(uploaded);
+        assert_eq!(error, None);
     }
 
     /// The column is gone, not merely unread. Asserted by asking SQLite rather than by

core/src/local/store.rs

@@ -92,7 +92,7 @@ fn items_of(body: &str) -> Vec<ChecklistItem> {
 
 fn load_attachments(conn: &Connection, note_id: &str) -> rusqlite::Result<Vec<Attachment>> {
     let mut stmt = conn.prepare(
-        "SELECT id, url, filename, mime, size, sha256 FROM attachments WHERE note_id = ?1 ORDER BY position ASC",
+        "SELECT id, url, filename, mime, size, sha256, upload_error FROM attachments WHERE note_id = ?1 ORDER BY position ASC",
     )?;
     let rows = stmt.query_map([note_id], |r| {
         let server_url: String = r.get(1)?;
@@ -117,6 +117,7 @@ fn load_attachments(conn: &Connection, note_id: &str) -> rusqlite::Result<Vec<At
             mime,
             size: r.get(4)?,
             sha256,
+            upload_error: r.get(6)?,
         })
     })?;
     rows.collect()
@@ -660,20 +661,110 @@ pub fn delete_item(conn: &Connection, id: &str, item_id: &str) -> rusqlite::Resu
     set_body(conn, id, derive::remove_item(&body, index))
 }
 
+/// The stored form of a declared content type: the bare media type, lowercase.
+/// Matches the server's `normalize_mime`, so both sides file a type the same way.
+fn normalize_mime(raw: &str) -> String {
+    let bare = raw
+        .split(';')
+        .next()
+        .unwrap_or("")
+        .trim()
+        .to_ascii_lowercase();
+    if bare.is_empty() {
+        "application/octet-stream".to_string()
+    } else {
+        bare
+    }
+}
+
+/// A file's name reduced to its last path component, as the server's
+/// `_safe_filename` does — the name is for display and download, never a path.
+fn safe_filename(raw: &str) -> String {
+    let base = raw.trim().replace('\\', "/");
+    let base = base.rsplit('/').next().unwrap_or("").trim();
+    let capped: String = base.chars().take(255).collect();
+    if capped.is_empty() {
+        "file".to_string()
+    } else {
+        capped
+    }
+}
+
+/// Attach a file on this device, linked or not.
+///
+/// The bytes go into the blob store under their hash, and the row waits with
+/// `uploaded = 0` until push sends it — after the note itself has landed, since the
+/// server files an attachment under its note. The note is touched, so it is dirty
+/// too: that is what a background sync keys on to send it promptly.
+pub fn add_attachment(
+    conn: &Connection,
+    blobs: &crate::sync::blobs::BlobStore,
+    note_id: &str,
+    filename: &str,
+    mime: &str,
+    bytes: &[u8],
+) -> Result<Note, String> {
+    let exists: Option<i64> = conn
+        .query_row("SELECT 1 FROM notes WHERE id = ?1", [note_id], |r| r.get(0))
+        .optional()
+        .map_err(|e| e.to_string())?;
+    if exists.is_none() {
+        return Err("That note no longer exists.".to_string());
+    }
+    let sha256 = blobs.put(bytes)?;
+    let id = new_id();
+    conn.execute(
+        "INSERT INTO attachments (id, note_id, url, filename, mime, size, sha256, position, uploaded)
+              VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7,
+                      (SELECT COALESCE(MAX(position) + 1, 0) FROM attachments WHERE note_id = ?2),
+                      0)",
+        params![
+            id,
+            note_id,
+            // The server's route for it, which is what the row holds once it has
+            // synced too. Nothing renders it: `load_attachments` serves the local bytes.
+            format!("/api/notes/{note_id}/attachments/{id}"),
+            safe_filename(filename),
+            normalize_mime(mime),
+            bytes.len() as i64,
+            sha256,
+        ],
+    )
+    .map_err(|e| e.to_string())?;
+    touch(conn, note_id).map_err(|e| e.to_string())?;
+    load_note(conn, note_id).map_err(|e| e.to_string())
+}
+
 pub fn delete_attachment(conn: &Connection, id: &str, att_id: &str) -> rusqlite::Result<Note> {
+    let uploaded: Option<bool> = conn
+        .query_row(
+            "SELECT uploaded FROM attachments WHERE id = ?1 AND note_id = ?2",
+            params![att_id, id],
+            |r| r.get(0),
+        )
+        .optional()?;
     conn.execute(
         "DELETE FROM attachments WHERE id = ?1 AND note_id = ?2",
         params![att_id, id],
     )?;
+    // Only a file the server holds needs a tombstone; without one the next pull would
+    // bring it straight back. One still waiting to upload leaves the queue with its row.
+    if uploaded == Some(true) {
+        record_pending_delete(conn, "attachment", att_id)?;
+    }
     touch(conn, id)?;
     load_note(conn, id)
 }
 
 pub fn delete_preview(conn: &Connection, id: &str, preview_id: &str) -> rusqlite::Result<Note> {
-    conn.execute(
+    let removed = conn.execute(
         "DELETE FROM link_previews WHERE id = ?1 AND note_id = ?2",
         params![preview_id, id],
     )?;
+    // Previews are made by the server, so every one it has is one it would send back.
+    if removed > 0 {
+        record_pending_delete(conn, "preview", preview_id)?;
+    }
     touch(conn, id)?;
     load_note(conn, id)
 }
@@ -1460,4 +1551,126 @@ mod tests {
         trash(&conn, &binned.id).expect("trash");
         assert_eq!(list_labels(&conn).expect("labels")[0].count, Some(1));
     }
+
+    fn blobs(tag: &str) -> crate::sync::blobs::BlobStore {
+        let dir = std::env::temp_dir().join(format!("ts-store-blobs-{}-{tag}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&dir);
+        crate::sync::blobs::BlobStore::new(dir).expect("blobs")
+    }
+
+    fn tombstones(conn: &Connection) -> Vec<(String, String)> {
+        let mut stmt = conn
+            .prepare("SELECT entity, id FROM pending_deletes ORDER BY entity, id")
+            .expect("prepare");
+        let rows = stmt
+            .query_map([], |r| Ok((r.get(0)?, r.get(1)?)))
+            .expect("query");
+        rows.collect::<rusqlite::Result<_>>().expect("rows")
+    }
+
+    #[test]
+    fn an_attached_file_is_kept_here_and_queued_to_upload() {
+        let conn = db();
+        let blobs = blobs("attach");
+        let n = note(&conn, "with a receipt");
+        conn.execute("UPDATE notes SET dirty = 0", [])
+            .expect("clean");
+
+        let got = add_attachment(
+            &conn,
+            &blobs,
+            &n.id,
+            "C:\\scans\\receipt.PDF",
+            "Application/PDF; name=x",
+            b"%PDF",
+        )
+        .expect("attach");
+
+        let att = &got.attachments[0];
+        assert_eq!(
+            att.filename.as_deref(),
+            Some("receipt.PDF"),
+            "a name, not a path"
+        );
+        assert_eq!(att.mime, "application/pdf");
+        assert_eq!(att.size, Some(4));
+        let hash = att.sha256.clone().expect("hashed");
+        assert!(blobs.has(&hash), "the bytes are on this device");
+        assert!(att.url.contains(&hash), "and served from here: {}", att.url);
+        assert_eq!(att.upload_error, None);
+        let (uploaded, dirty): (bool, bool) = conn
+            .query_row(
+                "SELECT a.uploaded, n.dirty FROM attachments a JOIN notes n ON n.id = a.note_id",
+                [],
+                |r| Ok((r.get(0)?, r.get(1)?)),
+            )
+            .expect("row");
+        assert!(!uploaded, "it waits for push");
+        assert!(
+            dirty,
+            "the note is touched, which is what starts a background sync"
+        );
+    }
+
+    #[test]
+    fn attaching_to_a_note_that_is_gone_writes_nothing() {
+        let conn = db();
+        let blobs = blobs("gone");
+        assert!(add_attachment(&conn, &blobs, "missing", "a.txt", "text/plain", b"hi").is_err());
+        let rows: i64 = conn
+            .query_row("SELECT COUNT(*) FROM attachments", [], |r| r.get(0))
+            .expect("count");
+        assert_eq!(rows, 0);
+        let files = std::fs::read_dir(blobs.root()).expect("dir").count();
+        assert_eq!(files, 0, "and no bytes were filed for it");
+    }
+
+    #[test]
+    fn only_a_file_the_server_holds_leaves_a_tombstone_when_removed() {
+        let conn = db();
+        let blobs = blobs("remove");
+        let n = note(&conn, "two files");
+        conn.execute(
+            "INSERT INTO attachments (id, note_id, url) VALUES ('synced', ?1, '/x')",
+            [&n.id],
+        )
+        .expect("synced row");
+        let with_local =
+            add_attachment(&conn, &blobs, &n.id, "new.txt", "text/plain", b"hi").expect("attach");
+        let local = with_local
+            .attachments
+            .iter()
+            .find(|a| a.id != "synced")
+            .expect("local")
+            .id
+            .clone();
+
+        delete_attachment(&conn, &n.id, "synced").expect("remove synced");
+        delete_attachment(&conn, &n.id, &local).expect("remove local");
+
+        // Without the tombstone the next pull would put the synced file straight back.
+        // The local one never reached the server, so there is nothing to tell it.
+        assert_eq!(
+            tombstones(&conn),
+            vec![("attachment".to_string(), "synced".to_string())]
+        );
+    }
+
+    #[test]
+    fn dismissing_a_preview_leaves_a_tombstone() {
+        let conn = db();
+        let n = note(&conn, "https://example.com");
+        conn.execute(
+            "INSERT INTO link_previews (id, note_id, url) VALUES ('p1', ?1, 'https://example.com')",
+            [&n.id],
+        )
+        .expect("preview");
+
+        delete_preview(&conn, &n.id, "p1").expect("dismiss");
+        delete_preview(&conn, &n.id, "not-there").expect("a miss is fine");
+        assert_eq!(
+            tombstones(&conn),
+            vec![("preview".to_string(), "p1".to_string())]
+        );
+    }
 }