Skip to content

Reuse cached checksums in the capped full_sync pass - #2165

Merged
kickster97 merged 7 commits into
mainfrom
capped-sync-checksum-cache
Aug 7, 2026
Merged

Reuse cached checksums in the capped full_sync pass#2165
kickster97 merged 7 commits into
mainfrom
capped-sync-checksum-cache

Conversation

@kickster97

@kickster97 kickster97 commented Aug 3, 2026

Copy link
Copy Markdown
Member

Fixes #2162.

Builds on @lukas8219's 80cd199, which spotted the capped pass never using the checksum cache. That commit is cherry-picked here with a corrected reuse condition on top: it compared the digest's byte length against the caps hash, so it could never be true and the capped pass still recomputed everything.

During the final full_sync pass the leader hashes every file up to a snapshotted cut while holding the lock that all replicated operations need, so publishing stalls until the pass is done. The checksum cache was bypassed because entries did not record how many bytes a hash covers, so a cached hash could not be trusted for a capped size.

Every checksum write now records the covered byte count, on the leader as well as on the follower, so a size may only be recorded by code that just hashed exactly that many bytes. Lookups hand out the whole entry, hash and size together, and the caller decides whether the recorded coverage fits its use. The capped pass reuses a cached hash exactly when it covers the requested cut, which is every file not written to between the two passes, so it no longer re-reads tens of thousands of unchanged files under the lock. Comparing the cap against the file's current size instead would race local writes that have not invalidated the cache yet, which is why the covered size is stored rather than inferred.

The size is persisted in checksums.sha1 as "hash size *path". Files written by older versions parse as entries without a size; the uncapped pass recomputes those and records sizes, so an old cache heals in one pass and the capped pass never trusts a sizeless entry.

Specs: the reuse spec deletes the file from disk before the capped pass, so the hash can only come from the cache, and it fails without this change. Further specs cover recompute on cap mismatch, healing of old-format entries, size persistence across append/store/restore, and that follower-written checksums match both content and size on disk.

@claude

claude Bot commented Aug 3, 2026

Copy link
Copy Markdown

Reviewed the changes. No issues found.

The size-tracking implementation is correct: Entry serialization round-trips cleanly (sized/sizeless lines parse unambiguously since paths never contain *), hash_for? correctly gates on exact size match so sizeless and mismatched entries force a recompute, and the capped-reuse path in server.cr compares against the stored hashed size rather than the file's current size, avoiding a race with local writes. Follower-side append calls store sizeless entries that safely degrade to recompute. Specs cover persistence, store rewrite, sizeless restore, capped reuse, and capped recompute.

@kickster97
kickster97 force-pushed the capped-sync-checksum-cache branch from ce4b6b7 to eac45f6 Compare August 3, 2026 11:47
Comment thread src/lavinmq/clustering/checksums.cr
@kickster97
kickster97 marked this pull request as ready for review August 4, 2026 06:43
@kickster97
kickster97 requested a review from a team as a code owner August 4, 2026 06:43
@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown

Re-reviewed the current diff (capped-sync checksum-cache with covered sizes).

No issues found.

  • Checksums::Entry tracks hash + covered size; line/restore round-trip cleanly, and old-format sizeless lines are dropped unambiguously (paths never contain *).
  • Server#cached_hash? reuses a cached hash only on exact entry.size == cap for capped passes and any coverage otherwise — correct for append-only files and avoids racing local writes against the file's current size.
  • Capped passes never store into the cache (unless caps), so only full-size hashes are cached; the capped path can still reuse a full-size entry when the cap equals the whole file.
  • All callers updated to append/3 / set/3; []? now returns Entry? and is dereferenced correctly (.try &.hash).
  • Specs cover persistence, store rewrite, sizeless restore drop + heal, capped reuse, and capped recompute.

@kickster97
kickster97 requested a review from spuun August 5, 2026 07:49
@viktorerlingsson

Copy link
Copy Markdown
Member

Here's some thoughts on an improvement from me and Claude 🤖

At server.cr:209 the uncapped pass reuses a cached hash without checking whether the entry records a covered size, and it
doesn't call set on the hit, so a sizeless entry gets served as-is and never upgraded. The capped pass only reuses entries
that have a size, so those files keep getting re-hashed under @lock on every follower join.

That bites in two routine cases. Any checksums.sha1 written before this PR is entirely in the old format, so every entry
restored at startup is sizeless. And every entry a follower writes (client.cr:225, :335, :569) is sizeless too, and
Clustering::Server#listen restores that same file at server.cr:300, so a node promoted from follower to leader starts
with nothing but sizeless entries.

A file only picks up a size once a write invalidates its entry and the uncapped pass recomputes it, so actively written files
heal on their own while untouched files keep their sizeless entry indefinitely. Untouched files are exactly what the capped
pass spends its time on, which means on a cluster upgraded to this version that's most files at the first follower join, and
after a failover it's all of them. I think that makes the improvement hard to see on a real cluster as it stands.

Treating a sizeless entry as a miss fixes it. Beside hash_for?:

# The hash for `path` only if the entry records how many bytes it covers.
# A sizeless entry is deliberately a miss: recomputing records the size,
# which is what makes the entry reusable by a later capped pass.
def sized_hash?(path) : Bytes?
  if entry = @checksums[path]?
    entry.hash if entry.size
  end
end

and then at server.cr:209:

cached_hash = @file_index.shared do |_files, checksums|
  if caps
    checksums.hash_for?(path, caps[path]? || 0i64)
  else
    checksums.sized_hash?(path)
  end
end

Nothing else needs to change since set at server.cr:226 already records the size, and because every sized entry is a
full-file hash (only the uncapped pass calls set, and the capped pass never writes) this is really just the uncapped
counterpart to hash_for?. The extra recompute lands in the uncapped pass, which runs outside @lock (sync_and_serve only
holds it around the capped pass), so it costs sync time rather than publish latency. Worth a spec as well: seed a sizeless
entry the way a follower would, run the uncapped pass, then assert the capped pass reuses the hash, deleting the file first
so it can only have come from the cache.

Passing sizes at the client.cr write sites would help too, but I'd leave that out of here: once the above lands it only
saves one re-hash on the first sync after a promotion, and it breaks the three assertions that hardcode the sizeless line
format (client_sync_spec.cr:465, :496, clustering_spec.cr:630).

@kickster97
kickster97 force-pushed the capped-sync-checksum-cache branch 2 times, most recently from 49749f2 to e8991ce Compare August 6, 2026 13:05
lukas8219 and others added 6 commits August 6, 2026 15:48
The capped full_sync pass bypassed the checksum cache entirely and
re-hashed every file from disk while holding @lock, stalling all
replicated operations for the duration. Checksums entries now record
how many bytes each hash covers, so the capped pass reuses a cached
hash exactly when it covers the requested cut, which is every file not
written to between the two passes. Comparing against the file's current
size instead would race local writes that haven't invalidated the cache
yet.

The persisted format gains an optional size field; old checksums.sha1
files still parse, their entries just never match a sized lookup.

Fixes #2162

Co-authored-by: Lucas Polesello <lucas.c4d@gmail.com>
Keeps the on-disk line format of a serialized Entry in one place
instead of splitting it between the writer and the parser in restore.
Written bytes are unchanged, so existing checksums.sha1 files stay
readable.
The compare loop, file_from_socket and the close-time digest
finalization now store how many bytes their hashes cover, so a node
promoted to leader can reuse them in the capped sync pass instead of
re-hashing every file under the replication lock.
append and set now always take the covered size; only entries restored
from an old-format checksums.sha1 lack one. Lookups return the whole
Entry (hash plus covered size) and each caller decides what coverage it
accepts: the capped sync pass demands an exact match, the uncapped pass
takes any recorded coverage and recomputes sizeless entries outside the
lock so they gain a size the capped pass can trust. Without that
healing, every entry on an upgraded or promoted node stays sizeless
indefinitely and the capped pass keeps re-hashing all untouched files
under the replication lock on each follower join.
@kickster97
kickster97 force-pushed the capped-sync-checksum-cache branch from e8991ce to f8b4ed9 Compare August 6, 2026 13:57
@kickster97
kickster97 merged commit f3ccef5 into main Aug 7, 2026
20 of 21 checks passed
@kickster97
kickster97 deleted the capped-sync-checksum-cache branch August 7, 2026 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Leader re-hashes every file while holding the replication lock during follower sync

4 participants