fix(cos,engine): saving leaves out what was added and removed again

An incremental save wrote every object added during the session, whether
anything still referred to it or not. A redaction mark added and then
cleared (or a comment added and deleted) was therefore written into the
file as an unreferenced object, with its QuadPoints, overlay text and
author.

`SaveOptions::drop_unlinked` writes such additions as free entries instead:
objects whose numbers the file never used and that nothing the update
writes refers to, walked from the trailer and the file's changed objects
through additions only. `/Size` and the numbering stay as before, so undo
past the save can bring an object back under its number. The option is off
by default because the standalone document timestamp is found by scanning,
not by reference; `Session::save_bytes` (Save, and the automation save)
turns it on. Full saves already left these objects out.

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

crates/cos/src/writer.rs

@@ -2,17 +2,20 @@
 //!
 //! * `write_incremental` appends the edited objects, a cross-reference section in the same style
 //!   as the file's last one (table or stream) and a trailer with `/Prev`. The original bytes are
-//!   never modified, which keeps digital signatures over earlier revisions valid.
+//!   never modified, which keeps digital signatures over earlier revisions valid. With
+//!   `SaveOptions::drop_unlinked`, objects added since the file was read that nothing refers to
+//!   any more are left out (written as free).
 //! * `write_full` writes only objects reachable from the trailer, renumbered densely, with a
 //!   cross-reference table or stream — the "Save As" / garbage-collecting path. Unknown trailer
 //!   entries are copied, including objects reachable only from them.
 
 use std::borrow::Cow;
-use std::collections::{BTreeMap, HashMap, VecDeque};
+use std::collections::{BTreeMap, HashMap, HashSet, VecDeque};
 use std::io::Write;
+use std::sync::Arc;
 
 use crate::CosError;
-use crate::document::{Document, ObjectReader};
+use crate::document::{Document, ObjectReader, XrefEntry};
 use crate::object::{Dict, ObjRef, Object, PdfString, Stream};
 
 /// Serialize one object. Deterministic: same object → same bytes.
@@ -156,11 +159,16 @@ pub struct SaveOptions {
     /// Full saves: pack non-stream objects into compressed object streams with a cross-reference
     /// stream (PDF 1.5+, §7.5.7–7.5.8). Off writes a classic table that any reader accepts.
     pub object_streams: bool,
+    /// Incremental saves: leave out what was added since the file was read and nothing refers to
+    /// any more, such as a comment added and deleted again or a redaction mark added and cleared
+    /// (written as free). Off by default, because a writer may add an object only it finds again
+    /// by scanning, as a standalone document timestamp is. Full saves always leave these out.
+    pub drop_unlinked: bool,
 }
 
 impl Default for SaveOptions {
     fn default() -> Self {
-        Self { mod_date: None, id_seed: 0x5052_494E_5443_5241, object_streams: true }
+        Self { mod_date: None, id_seed: 0x5052_494E_5443_5241, object_streams: true, drop_unlinked: false }
     }
 }
 
@@ -209,8 +217,14 @@ pub fn write_incremental(doc: &Document, opts: &SaveOptions) -> Result<Vec<u8>,
     let base = (original.len() + usize::from(newline)).saturating_sub(shift) as u64;
     let mut out = Vec::new();
     let mut offsets: BTreeMap<u32, (u64, u16, bool)> = BTreeMap::new(); // num → (offset, gen, in use)
+    let unlinked = if opts.drop_unlinked { unlinked_additions(&doc) } else { HashSet::new() };
     for (num, generation, obj) in doc.overlay_entries() {
         match obj {
+            // Written as free, so `/Size` still covers every number this document handed out. The
+            // generation stays: the number was never in use, and an undo can bring it back as is.
+            Some(_) if unlinked.contains(&num) => {
+                offsets.insert(num, (0, generation, false));
+            }
             Some(o) => {
                 offsets.insert(num, (base + out.len() as u64, generation, true));
                 write_indirect(num, generation, &prepared(&doc, num, num, generation, &o), &mut out);
@@ -387,7 +401,7 @@ pub fn write_full(doc: &Document, opts: &SaveOptions) -> Result<Vec<u8>, CosErro
 /// Object `r` read again for writing. The traversal read it once and only kept its number, so
 /// a second read that fails, finds nothing or finds a different kind of object must fail the
 /// save rather than write `null` in its place.
-fn reread(reader: &mut ObjectReader, r: ObjRef, is_stream: bool) -> Result<std::sync::Arc<Object>, CosError> {
+fn reread(reader: &mut ObjectReader, r: ObjRef, is_stream: bool) -> Result<Arc<Object>, CosError> {
     let o = reader.try_get(r)?;
     if matches!(*o, Object::Null) || matches!(*o, Object::Stream(_)) != is_stream {
         return Err(CosError::Syntax { offset: 0, detail: format!("object {} read differently while saving; nothing was written", r.num) });
@@ -439,6 +453,37 @@ fn output_number(index: usize) -> Result<u32, CosError> {
     u32::try_from(index).ok().and_then(|n| n.checked_add(1)).ok_or_else(|| CosError::Syntax { offset: 0, detail: "too many objects to save".into() })
 }
 
+/// Objects added since the file was read (their numbers are not in use in it) that nothing an
+/// incremental update writes refers to: what one edit added and a later one unlinked, such as a
+/// deleted comment or a cleared redaction mark. The file never held them, so leaving them out
+/// loses nothing, while writing them would keep what the user removed. Only the update is walked:
+/// from the trailer and the file's changed objects through additions. An unchanged object of the
+/// file is not followed: a reference it holds to such a number already reads as null in the file
+/// (a damaged file's dangling reference), and it still does.
+fn unlinked_additions(doc: &Document) -> HashSet<u32> {
+    let added = |num: u32| matches!(doc.xref_entry(num), None | Some(XrefEntry::Free { .. }));
+    let mut pending: HashMap<u32, Arc<Object>> =
+        doc.overlay_entries().filter_map(|(num, _, o)| Some((num, o?))).filter(|(num, _)| added(*num)).collect();
+    if pending.is_empty() {
+        return HashSet::new();
+    }
+    let mut stack: Vec<ObjRef> = Vec::new();
+    doc.trailer().iter().for_each(|(_, v)| collect_refs(v, &mut |r| stack.push(r)));
+    for (num, _, o) in doc.overlay_entries() {
+        if let Some(o) = o
+            && !added(num)
+        {
+            collect_refs(&o, &mut |r| stack.push(r));
+        }
+    }
+    while let Some(r) = stack.pop() {
+        if let Some(o) = pending.remove(&r.num) {
+            collect_refs(&o, &mut |r| stack.push(r));
+        }
+    }
+    pending.into_keys().collect()
+}
+
 fn collect_refs(o: &Object, f: &mut impl FnMut(ObjRef)) {
     match o {
         Object::Ref(r) => f(*r),

crates/cos/tests/it/writer.rs

@@ -228,6 +228,54 @@ fn incremental_save_keeps_a_trailer_only_edit() {
     assert_qpdf_clean(&saved);
 }
 
+/// What one edit added and a later one unlinked (a deleted comment, a cleared redaction mark) is
+/// not written: the file never held it, and writing it would keep what was removed. Additions the
+/// update still refers to, from the trailer or a changed object and through other additions, are.
+#[test]
+fn incremental_save_leaves_out_additions_nothing_refers_to() {
+    let mut doc = Document::open(Arc::new(classic_table())).unwrap();
+    let text = |s: &str| Object::String(pdfcraft_cos::PdfString::literal(s.as_bytes().to_vec()));
+    let dict = |entries: &[(&str, Object)]| {
+        let mut d = Dict::new();
+        entries.iter().for_each(|(k, v)| d.set(k.as_bytes().to_vec(), v.clone()));
+        Object::Dict(d)
+    };
+    let page_obj = ObjRef::new(3, 0);
+    let leaf = doc.add(text("kept leaf"));
+    let kept = doc.add(dict(&[("Leaf", Object::Ref(leaf))]));
+    doc.update_dict(page_obj, |d| d.set(b"PieceInfo".to_vec(), Object::Ref(kept))).unwrap();
+    let by_trailer = doc.add(text("kept by the trailer"));
+    doc.trailer_mut().set(b"Custom".to_vec(), Object::Ref(by_trailer));
+    // Linked from the page and unlinked again, as clearing a mark does.
+    let dropped_leaf = doc.add(text("dropped leaf"));
+    let dropped = doc.add(dict(&[("Leaf", Object::Ref(dropped_leaf))]));
+    doc.update_dict(page_obj, |d| d.set(b"Annots".to_vec(), Object::Array(vec![Object::Ref(dropped)]))).unwrap();
+    doc.update_dict(page_obj, |d| {
+        d.remove(b"Annots");
+    })
+    .unwrap();
+    // Unlinked additions that refer to each other.
+    let first = doc.add(Object::Null);
+    let second = doc.add(dict(&[("Next", Object::Ref(first)), ("Note", text("dropped cycle"))]));
+    doc.set(first, dict(&[("Next", Object::Ref(second))]));
+
+    // Off by default: a writer may add what only it finds again, by scanning.
+    let kept_all = write_incremental(&doc, &SaveOptions::default()).unwrap();
+    assert!(kept_all.windows(12).any(|w| w == b"dropped leaf"));
+    let saved = write_incremental(&doc, &SaveOptions { drop_unlinked: true, ..SaveOptions::default() }).unwrap();
+    let holds = |s: &str| saved.windows(s.len()).any(|w| w == s.as_bytes());
+    assert!(holds("kept leaf") && holds("kept by the trailer"));
+    assert!(!holds("dropped leaf") && !holds("dropped cycle"), "unlinked additions are written");
+    let back = Document::open(Arc::new(saved.clone())).unwrap();
+    assert!(back.repair_log().is_empty(), "{:?}", back.repair_log());
+    assert_eq!(back.get(leaf).as_string().map(|s| s.bytes.as_slice()), Some(b"kept leaf".as_slice()));
+    for gone in [dropped, dropped_leaf, first, second] {
+        assert_eq!(*back.get(gone), Object::Null);
+    }
+    assert!(!page(&back).contains(b"Annots"));
+    assert_qpdf_clean(&saved);
+}
+
 #[test]
 fn qpdf_accepts_object_stream_output() {
     let qpdf = pdfcraft_oracle::or_skip!(pdfcraft_oracle::qpdf());

crates/engine/src/lib.rs

@@ -4310,7 +4310,8 @@ impl Session {
         if !editor.cos.is_modified() && on_saved && (!editor.cos.reconstructed() || doc.is_signed()) {
             return Ok(editor.saved.clone());
         }
-        let opts = SaveOptions { mod_date: self.now().map(pdfcraft_cos::pdf_date), ..SaveOptions::default() };
+        // What the user removed again before saving (a cleared redaction mark) stays out of the file.
+        let opts = SaveOptions { mod_date: self.now().map(pdfcraft_cos::pdf_date), drop_unlinked: true, ..SaveOptions::default() };
         // A restored snapshot can be based on an earlier revision. Append to the last saved
         // file so that revision stays in the chain. A full rewrite (redaction, new encryption,
         // a reconstructed file) still writes the current graph on its own.

crates/engine/src/tests.rs

@@ -2490,6 +2490,55 @@ fn redaction_marks_apply_for_good_and_undo() {
     assert!(matches!(s.apply(id, Edit::ApplyRedactions { pages: None }), Err(EditError::Redact(_))));
 }
 
+/// Marks removed before they are applied, all at once or one by one, are not saved, nor are
+/// replies to them: the file would otherwise record where they were, their overlay text and who
+/// made them, and the page is left as it was.
+#[test]
+fn removed_redaction_marks_are_not_saved() {
+    for clear in [true, false] {
+        let (mut s, id) = session_with(1);
+        let shape = Shape::Redact {
+            quads: vec![pdfcraft_annot::rect_quad([80.0, 140.0, 100.0, 180.0])],
+            overlay: "MARKED OVERLAY".into(),
+            look: Default::default(),
+        };
+        let mark = Edit::AddAnnotation(NewAnnotation {
+            page: 0,
+            style: Style::default_for(&shape),
+            shape,
+            contents: String::new(),
+            author: "Mark Author".into(),
+        });
+        s.apply(id, mark).unwrap();
+        let reply = Edit::ReplyToAnnotation { page: 0, index: 0, text: "REPLY TO THE MARK".into(), author: "Replier".into() };
+        s.apply(id, reply).unwrap();
+        let removal = if clear { Edit::ClearRedactions } else { Edit::DeleteAnnotation { page: 0, index: 0 } };
+        s.apply(id, removal).unwrap();
+        assert_eq!(s.get(id).unwrap().redaction_marks(), 0);
+        let saved = s.save_bytes(id).unwrap();
+        let holds = |s: &[u8]| saved.windows(s.len()).any(|w| w == s);
+        assert!(!holds(b"/Redact") && !holds(b"MARKED OVERLAY") && !holds(b"Mark Author"), "clear: {clear}");
+        assert!(!holds(b"REPLY TO THE MARK") && !holds(b"Replier"), "clear: {clear}");
+        let mut reopened = Session::new();
+        let back = reopened.open("saved.pdf", None, saved.clone(), None).unwrap();
+        assert_eq!(page_texts(&reopened, back), ["Page 1"]);
+        let page = pdfcraft_model::pages(&reopened.get(back).unwrap().editor.as_ref().unwrap().cos).swap_remove(0);
+        assert!(!page.dict.contains(b"Annots"), "clear: {clear}");
+
+        // Undo past that save brings the mark back, under the number the saved file left free.
+        s.mark_saved(id, saved, None).unwrap();
+        s.undo(id).unwrap();
+        let again = s.save_bytes(id).unwrap();
+        let mut reopened = Session::new();
+        let back = reopened.open("again.pdf", None, again.clone(), None).unwrap();
+        let d = reopened.get(back).unwrap();
+        assert!(d.info.warnings.is_empty() && d.editor.as_ref().unwrap().cos.repair_log().is_empty(), "{:?}", d.info.warnings);
+        assert_eq!(d.redaction_marks(), 1, "clear: {clear}");
+        let marks = pdfcraft_redact::marks(&d.editor.as_ref().unwrap().cos);
+        assert_eq!(marks.first().map(|m| m.overlay.as_str()), Some("MARKED OVERLAY"));
+    }
+}
+
 #[test]
 fn oversized_redaction_overlay_preserves_the_document_and_undo() {
     let (mut s, id) = session_with(1);