refactor(engine): unify manager deps, logging and filemeta loading - #426
Merged
Conversation
LsSnapshotManager.metaCache was allocated and read but never written, so it could never return a hit. It could not have helped even if populated: a HAMT key is derived from meta.FileID, which is itself a FileMeta field, so no two keys share a filemeta ref and a single-root walk reaches each ref exactly once. DiffManager and PruneManager keep their caches, which do hit because they walk several snapshots and unchanged files share a ref across them. The collected-entries line also sat inside the counting loop, turning a one-line summary into one line per file and burying every other diagnostic in the debug log on a real snapshot.
store and keyCache held the same pointer: NewBackupManager assigned the KeyCacheStore to both, one at store.ObjectStore and one at its concrete type, and the concrete field existed only so the single PreloadKeys call could reach it. Keeping the concrete type on the one remaining field loses nothing — every other use passes it where a store.ObjectStore is wanted, and the capability assertions in store.GetConcurrencyHint see the same dynamic value either way.
The catalog was three free functions that each re-took a store and a *logger.Logger, so every manager touching it had to carry a second, snapshot-prefixed logger purely to hand back down. That is where the two loggers on BackupManager and CopyManager came from. Binding the pair once in a snapshotCatalog value leaves every manager with a single logger of its own. It also makes copy's two sides explicit: it reads the source catalog and writes the destination's, which was previously one snapLog field shared across calls against two different stores. ListManager and ForgetManager used their logger only for catalog calls, so they now hold a catalog and no logger at all. DiffManager and LsSnapshotManager never touched the catalog despite their log field being documented as the snapshot-catalog sink; they get their own [diff] and [ls] prefixes, as does FindManager, whose logger is genuinely its own — it feeds the scanner's progress reporting. The four catalog symbols had no callers outside this package and are now unexported, along with SnapshotLogger.
loadMeta was written six times — backup, diff, prune, ls, restore and the find scanner — each pairing getVerified with a json.Unmarshal, and the copies had drifted apart: backup guarded its cache with an RWMutex, diff lazily allocated one, prune wrapped its error while the rest returned it bare, and ls and restore had no cache at all. metaLoader is that function once, with the cache as a constructor choice rather than an accident. Whether to memoize is a property of the traversal: an operation crossing several snapshots meets the same ref repeatedly, because an unchanged file keeps its filemeta from one snapshot to the next, while a single-root walk reaches every ref exactly once. backup, diff and prune memoize; ls, restore and the find scanner read through — the scanner deliberately, since a full scan crosses every snapshot and cannot hold what it decodes. It keeps its own progress counter, which feeds FindResult.MetaFetched. Errors are now wrapped uniformly. No caller inspects them, and %w leaves errors.Is unaffected.
The same four dependencies — store, reporter, log sink, dedup key — appeared in five different positional orders across eleven constructors, and Reporter and LogSink are both "somewhere output goes", a transposition no compiler catches. Three managers took no log writer at all: restore, check and prune could not emit a debug line, which read as an omission rather than a decision, and each now gets a sink for free. Client.engineDeps() supplies the set once. Backup is the only caller that overrides it, swapping in its own meter so the run's raw byte count stays separable from the client-wide stored total. Forget also stopped wrapping the store in a MeteredStore before handing it to NewPruneManager, which wraps one itself. The inner meter was created inline, never referenced, and counted the same writes as the outer one that PruneResult actually reads. engine.Deps is exported because the root package constructs it; it appears in no exported signature there, so it needs no alias.
Both carried the same defect already fixed in ls. diff's change summary sat inside the counting loop, so it printed once per change with running totals instead of once with the final ones. list's per-snapshot line sat inside the branch that formats the source suffix, so a snapshot with no source info was silently absent from a listing the caller had just been handed.
rmanibus
added
bug
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Cleanup pass over
internal/engine, landed as six atomic commits — each verified green before the next, and each purely structural or purely behavioural, never both.The duplicate store was literal.
NewBackupManagerassigned the sameKeyCacheStorepointer to bothstoreandkeyCache; the second field existed only so onePreloadKeyscall could reach the concrete type. One field now, held concrete — the capability assertions instore.GetConcurrencyHintsee the same dynamic value either way.The two loggers were a symptom. The snapshot catalog was three free functions that each re-took a store and a
*logger.Logger, so every manager touching it carried a second, snapshot-prefixed logger purely to hand back down. Binding the pair once in asnapshotCatalogvalue leaves each manager with one logger. That also surfaced thatDiffManagerandLsSnapshotManagerhad alogfield documented as "the snapshot-catalog sink" while never calling a catalog function — they were borrowing the[snapshots]prefix, and now have their own, as doesfind. Copy's singlesnapLogwas used against two different stores; it is now an explicitsrcCatalog/dstCatalog. The four catalog symbols had no callers outside the package and are unexported.loadMetawas written six times — backup, diff, prune, ls, restore, find scanner — and the copies had drifted: backup guarded its cache with an RWMutex, diff lazily allocated one, prune wrapped its error while others returned it bare, ls and restore had none. OnemetaLoadernow, with memoization as a constructor choice rather than an accident: it is a property of the traversal, not the call site.Eleven constructors took the same four dependencies in five different orders, and
Reporter/LogSinkare both "somewhere output goes" — a transposition the compiler cannot catch.engine.Depsfixes the order and gives restore, check and prune a debug sink they had simply never been passed.Client.engineDeps()supplies the set; backup is the only caller that overrides it, swapping in its own meter. Forget also stopped double-wrapping the store in aMeteredStorebefore handing it toNewPruneManager, which wraps one itself — the inner meter was created inline, never referenced, and counted the same writes.Defects found along the way
Each fixed in its own commit with a regression test, kept separate from the structural work:
lsallocated a filemeta cache it never wrote to. Note: this was not costing re-fetches — a HAMT key derives frommeta.FileID, itself aFileMetafield, so no two keys share a filemeta ref and a single-root walk reaches each exactly once. The cache could never hit, so it is deleted rather than populated. The caches indiffandprunedo earn their keep; those walk several snapshots, where unchanged files share a ref.diff's change summary sat inside the counting loop, printing once per change with running totals instead of once with the final ones.list's per-snapshot line sat inside the branch formatting the source suffix, so a snapshot with no source info was silently absent from a listing the caller had just been handed.Reviewer notes
engine.Depsis exported because the root package constructs it, but it appears in no exported signature there, so it needs no alias —internal/apicheckpasses.docs/compatibility.mdneeds no new baseline.%wleaveserrors.Isunaffected.diff,lsandfindnow carry their own component prefixes instead of[snapshots].<type>/<kebab-slug>convention; the PR title does, and the squash subject is what lands.Verification
env GOCACHE=/tmp/cloudstic-gocache go test -race -count=1 ./...passes (full module, includinginternal/apicheck)env GOCACHE=/tmp/cloudstic-gocache GOLANGCI_LINT_CACHE=/tmp/cloudstic-golangci-lint golangci-lint run ./...— 0 issuesDocker-backed e2e tests (MinIO store, SFTP source/store) skipped locally for want of
/var/run/docker.sock, so those paths were not exercised here and rely on CI.