cache: flush pack writer before periodic chunk-index write (#9900)

[mr-raj12]

Closes #9900.

The periodic chunk-index write runs on a 600 second timer, independently of the pack writer. When it fires, any chunks added since the last pack flush are still buffered in the pack writer. Their pack location isn't known yet, so in the chunk index they carry the UNKNOWN_BYTES32 placeholder pack_id (b"\xff" * 32).

The periodic write serialized those buffered chunks with the placeholder pack_id, then clear_new() stripped their F_NEW flag. With the flag gone, the corrected pack location that arrives at the next flush was never written incrementally. A fresh process that rebuilt the index from the stored fragments would then find a live chunk pointing at pack 0xff..ff, which shows up as ObjectNotFound.

The fix flushes the pack writer before the periodic write, so every buffered chunk already has its real pack location by the time it gets serialized. The flush is a no-op on an empty buffer, so it's cheap on the common path. Cache.close and Repository.close already flush or assert an empty buffer before writing, so this just brings the timer path in line with them.

A follow-up commit adds an assertion in write_chunkindex_to_repo, which is the one function every index fragment write goes through. It refuses to serialize an entry that still has F_PENDING set, i.e. one whose pack location is unresolved. Checking the flag keeps the sentinel pack_id value out of the guard, and it covers every caller, including the compact paths that pass an index other than repository.chunks. If some future caller reaches this code with a buffered chunk, it now fails loudly during development instead of quietly writing a broken location. The assert compiles out under python -O.

There's a regression test that drives the same write the timer triggers while a chunk is still buffered, rebuilds the index from the stored fragments, and checks the chunk's persisted pack_id is real rather than the placeholder.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.13%. Comparing base (52d7ca0) to head (7da670c).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9903      +/-   ##
==========================================
- Coverage   85.24%   85.13%   -0.12%     
==========================================
  Files          93       93              
  Lines       15898    15899       +1     
  Branches     2429     2428       -1     
==========================================
- Hits        13552    13535      -17     
- Misses       1638     1654      +16     
- Partials      708      710       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[ThomasWaldmann]

can we do it without in-band signalling?

[ThomasWaldmann]

review by claude fable 5:

Correctness of the fix

The core fix is sound. I verified the load-bearing details against master:

• Shared index object: ChunksMixin.chunks is literally repository.chunks (the cache adopts the repository's index), so Repository.flush() → PackWriter.flush() → update_pack_info() updates exactly the index that _maybe_write_chunks_index() then serializes. No stale-copy problem.
• The flush sits inside the if self._chunks is not None: guard and is a no-op on an empty buffer, so the common path stays cheap. Repository.flush() also does _lock_refresh(), a small bonus on the timer path.
• Ordering is right: add_chunk() runs the timer check before putting the current chunk, so the flush covers everything buffered so far.
• Flushing is the right approach vs. the alternative (skipping still-pending entries during serialization): that would also require teaching clear_new() to spare pending entries, otherwise their F_NEW would still be lost — more complexity for no gain.
• Performance: worst case is one small, non-full pack per 600 s window; negligible, and correctness requires it.

Empirical check: I applied the diff locally — all 53 cache tests pass; with only the flush() line removed, the new regression test fails (it trips the guard in write_chunkindex_to_repo). So the test really covers the bug.

The in-band signalling point (supporting the requested changes)

To answer the open question from the review discussion: the flag is still available at that point. ChunkIndex.__getitem__ runs hide_system_flags, but that only masks M_SYSTEM (0xff000000, i.e. F_NEW); F_PENDING = 2**2 is in the user-flag range and is only cleared by update_pack_info() after a real flush. Flags are stripped (F_NONE) solely on the serialized copy in the batch. So the assert can simply be:

assert not (entry.flags & ChunkIndex.F_PENDING), f"chunk {bin_to_hex(key)} has no pack location yet"

— exact, no collision probability with a legitimate pack ID, no magic value.

Comparing the two suggested alternatives:

• Per-entry flag check: one bitmask test on an entry the loop already fetched — essentially free, and it guards every caller, including calls where the passed index is not repository.chunks (compact's save_chunk_index, repack_chunkindex).
• Pack-writer emptiness peek: a single O(1) check (like the existing assert not self._pack_writer._pieces in Repository.close), but it only sees the repository's own writer, so it wouldn't cover the explicit-chunks cases. The flag assert in the central function seems strictly better for the same cost.

For the test, the in-band-free version is also stronger: after the periodic write, compare against the real location instead of the placeholder —

assert index[H(7)].pack_id == repository.chunks[H(7)].pack_id

which checks the persisted entry is correct, not merely "not the placeholder". With both changes, the UNKNOWN_BYTES32 imports in cache.py and cache_test.py become unnecessary — a nice sign the in-band constant has fully left the picture.

Minor nits (non-blocking)

• Cache.close() already calls self.repository.flush() right before _maybe_write_chunks_index(..., force=True); with the flush now inside _maybe_write_chunks_index, the close-path call is redundant (harmless) — could be removed to keep one canonical place.
• The test constructs AdHocWithFilesCache(manifest) by hand although the class already has a cache fixture doing exactly that, and the key parameter is unused (manifest already depends on key).

[ThomasWaldmann]

user-contributed reproducer by @mirko:

https://paste.nanl.de/?1212557912c850cc#HB5zEhLysz5dd3UhTAVCptM5PyQTHc9GnvCmevwSHyLK

claude:

"""
Test: src/borg/testsuite/h1_pending_index_test.py — a single test, test_periodic_index_write_persists_pending_chunk_with_bogus_location. It reuses borg's own H and fchunk helpers, so it follows the conventions of the existing pending-chunk tests in repository_test.py.

What it proves. It puts one chunk (buffered, F_PENDING), does the mid-backup incremental index write the way _maybe_write_chunks_index does (no flush first), flushes, closes, then reopens as a fresh process. On current master it fails because the rebuilt index entry for the chunk is:

ChunkIndexEntry(flags=0, size=0,
pack_id=b'\xff'*32, obj_offset=4294967295, obj_size=4294967295)
— the UNKNOWN placeholder location persisted verbatim, even though the chunk's bytes are in a real pack. That is finding H1, reproduced concretely.
"""

[mirko]

user-contributed reproducer by @mirko:

https://paste.nanl.de/?1212557912c850cc#HB5zEhLysz5dd3UhTAVCptM5PyQTHc9GnvCmevwSHyLK

The paste link is eventually going to expire, so I'll copy&paste the reproducer into the ticket directly:

    """Regression test for finding H1 (see borg2-integrity-findings.md).
     
    The periodic mid-backup chunk-index write persists chunks that are still buffered in the
    pack writer (F_PENDING) using their placeholder pack location, and clear_new() then strips
    F_NEW so the corrected location is never written. The chunk's bytes do reach a real pack,
    but the persisted index maps the chunk to a non-existent pack, so a fresh process cannot
    read it (and would deduplicate against it).
     
    This test FAILS on the current code. It should PASS once _maybe_write_chunks_index flushes
    the pack writer before persisting the index (mirroring Cache.close()).
    """
     
    from ..cache import write_chunkindex_to_repo
    from ..constants import UNKNOWN_BYTES32
    from ..repository import Repository
    from .hashindex_test import H
    from .repository_test import fchunk
     
     
    def test_periodic_index_write_persists_pending_chunk_with_bogus_location(tmp_path):
        repo_path = str(tmp_path / "repo")
        data = fchunk(b"BUFFERED", chunk_id=H(1))
        with Repository(repo_path, exclusive=True, create=True) as repository:
            repository.put(H(1), data)  # buffered in the pack writer: F_NEW | F_PENDING, no flush yet
            assert repository.chunks.is_pending(H(1))
            # The periodic mid-backup index write (cache.ChunksMixin._maybe_write_chunks_index).
            # Unlike Cache.close(), it is NOT preceded by repository.flush(), so it serializes the
            # still-pending entry with its placeholder location.
            write_chunkindex_to_repo(repository, repository.chunks, incremental=True)
            repository.flush()  # the pack becomes durable; the real location is set in memory
            # write_chunkindex_to_repo() cleared F_NEW on H(1) above, so close()'s incremental
            # write will not re-persist the now-correct location.
     
        # A fresh process rebuilds the in-memory index from the persisted index/ fragments.
        with Repository(repo_path, exclusive=True) as repository:
            entry = repository.chunks.get(H(1))
            assert entry is not None, "chunk vanished from the rebuilt index"
            assert entry.pack_id != UNKNOWN_BYTES32, (
                "the persisted index maps a real chunk to a non-existent (UNKNOWN) pack: a still-"
                "buffered chunk was serialized with its placeholder pack location"
            )
            assert repository.get(H(1)) == data, "chunk unreadable from a fresh process"

[ThomasWaldmann]

review by claude fable 5:

Re-review at 1b0fd66 — all points from the previous review round are properly addressed.

What changed

• In-band signalling removed from the guard: the assert in write_chunkindex_to_repo now tests entry.flags & ChunkIndex.F_PENDING instead of comparing pack_id against the UNKNOWN_BYTES32 sentinel — exact, no collision possibility with a legitimate pack ID. The flag is indeed still visible at that point (hide_system_flags only masks M_SYSTEM/F_NEW; flags are stripped only on the serialized batch copy).
• In-band signalling removed from the test: it now asserts the persisted entry equals the real in-memory location (index[H(7)].pack_id == repository.chunks[H(7)].pack_id), which is also a stronger check than "not the placeholder".
• Both UNKNOWN_BYTES32 imports are gone from cache.py and cache_test.py.
• The redundant flush() in Cache.close was removed — the flush now lives only in _maybe_write_chunks_index, one canonical place. The resulting reordering in close() (flush now happens after the files-cache write instead of before) is harmless: the files cache is local and independent of the pack writer, and the flush still precedes the index write on the same path.
• Test style fixed: it uses the existing cache fixture; the unused key/manifest parameters are gone.

Empirical verification (repeated against the new head)

• With the diff applied locally: all 53 tests in cache_test.py pass.
• With only the flush() line removed: the regression test fails at the new F_PENDING assert with a clear message (AssertionError: chunk 3030…3037 has no pack location yet), so the guard both catches the bug and diagnoses it well.

Remaining observations

Nothing blocking. One pre-existing quirk noticed in passing, not introduced by this PR: Repository.close asserts the pack writer is empty unconditionally, so an exception that unwinds past a buffered writer could mask the original error with that assert — independent of this change and only reachable if Cache.close itself fails mid-way.

Verdict: looks ready to merge.