Skip to content

Store packfile size metadata.#377

Open
mathieu-plak wants to merge 2 commits into
mainfrom
mm/store_size_metadata
Open

Store packfile size metadata.#377
mathieu-plak wants to merge 2 commits into
mainfrom
mm/store_size_metadata

Conversation

@mathieu-plak

Copy link
Copy Markdown
Contributor

No description provided.

@mathieu-plak
mathieu-plak marked this pull request as draft May 13, 2026 15:08
* Add a new field inside the ET_PACKFILE type to store the packfile
  size.

* This needs a format version bump.
* Now that we store each packfile sizes in the state, let's use that to
  compute the size of the repository.

* If any of the packfile entry has a 0 size fallback to the old
  codepath. Let's see if it's worth implementing a self healing solution
  (it might be a bit heavy as we need to regen a full state...)

* Note that this misses the state size, which will be tackled in a
  subsequent branch as they do not need a state file change.
@mathieu-plak
mathieu-plak force-pushed the mm/store_size_metadata branch from d9f938c to f24fa76 Compare May 15, 2026 14:54
@mathieu-plak
mathieu-plak marked this pull request as ready for review May 15, 2026 15:09
Comment thread repository/state/state.go
Comment on lines +262 to 266
if ver.Equals(versioning.FromString("1.0.0")) {
err = ls.deserializeFromStreamv100(rd)
} else {
err = ls.deserializeFromStream(rd)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The VERSION bump to 1.2.0 in this PR updates the version dispatch in MergeState (state.go:262-266) to treat 1.0.0 as legacy and everything else as the new format, but two sibling dispatch sites in the same file were missed and still gate on ver.Equals("1.1.0"). The more serious one is ReadHeader (state.go:234) — it is called from RebuildState/RebuildStateWithCache as the TopoSort getParent callback (repository.go:367, :492), so for every new v1.2.0 state it skips the 32-byte parent header and returns hdr.Parent = NilMac, causing TopoSort to classify every new state as a root and breaking the partial topological ordering state merging relies on. FromStream (state.go:167-175) has the symmetric defect (latent — only test-called today). Fix: mirror the MergeState dispatch at both sites — treat 1.0.0 as legacy and everything else as the current format.

Extended reasoning...

Bug

This PR bumps VERSION from 1.1.0 to 1.2.0 (state.go:35) and inverts the polarity of the version dispatch in MergeState (state.go:262-266) so that 1.0.0 routes to the legacy deserializeFromStreamv100 and everything else (including the new 1.2.0) routes to deserializeFromStream. There are two sibling dispatch sites in the same file that should have been updated in lock-step but were missed:

// state.go:234 — ReadHeader (NOT in the diff)
if ver.Equals(versioning.FromString("1.1.0")) {
    n, err := rd.Read(hdr.Parent[:])
    ...
}

// state.go:167-175 — FromStream (NOT in the diff)
if ver.Equals(versioning.FromString("1.1.0")) {
    err = st.deserializeFromStream(rd)
} else {
    err = st.deserializeFromStreamv100(rd)
}

Version.Equals does an exact semver compare, so 1.2.0 != 1.1.0 and these checks now fall to the wrong branch for any state written by this PR.

Why the existing code does not prevent this

SerializeToStream (state.go:~374) writes ls.Metadata.Parent[:] (32 bytes) at the start of every stream regardless of version, and the matching deserializeFromStream consumes them. So the on-disk format for 1.2.0 still has the parent header — but the dispatch that decides whether to read it was never updated for the new version.

Impact

ReadHeader (hot path regression): ReadHeader is called from RebuildState (repository.go:367) and RebuildStateWithCache (repository.go:492) inside the TopoSort getParent callback. With ver=1.2.0, the if ver.Equals("1.1.0") branch is skipped, no parent bytes are consumed, and the returned hdr.Parent is the zero value NilMac. TopoSort then takes the if p == objects.NilMac { S = append(S, stateID) } branch for every new state and classifies all of them as roots. The partial topological ordering that subsequent MergeState calls rely on (parents must be merged before children so the implicit parent-setting comment at lines 257-260 holds) degenerates into reverse-insertion order. This affects any deployment that writes more than one v1.2.0 state and then rebuilds — i.e. every repository upgraded past this PR.

FromStream (latent): With ver=1.2.0, the else branch routes to deserializeFromStreamv100, which (a) does not consume the 32-byte parent header that the new format writes, and (b) rejects any ET_PACKFILE entry whose length is not PackfileEntrySerializedSizeV1 (state.go:907-909). A v1.2.0 stream pushed through FromStream would either mis-frame from the 32-byte offset or hard-error on the first V2 packfile entry. The only current caller is TestFromStream (state_test.go:327) which hardcodes "1.1.0", so the runtime impact today is zero — but it is an exported API on which the just-updated MergeState is the obvious reference point, and the divergence is the same half-applied-refactor pattern as the ReadHeader issue.

Step-by-step proof for ReadHeader

  1. After this PR, PutState calls storage.Serialize(..., versioning.GetCurrentVersion(RT_STATE), ...), which tags the state file with version 1.2.0 (because VERSION = "1.2.0" is the registered current version).
  2. SerializeToStream writes ls.Metadata.Parent[:] (32 bytes) at offset 0 — see state.go:374.
  3. Later, RebuildState calls TopoSort(missingStates, getParent) (repository.go:367) where getParent opens the state file, gets back ver = 1.2.0 from GetState, and calls state.ReadHeader(rd, ver).
  4. In ReadHeader: ver.Equals(versioning.FromString("1.1.0")) evaluates to false (semver compare, 1.2.0 != 1.1.0). The body is skipped. hdr.Parent remains {0,0,...,0} == objects.NilMac.
  5. Back in TopoSort: p == objects.NilMac is true, so stateID is appended to S (root set). This happens for every v1.2.0 state, even those whose stream actually starts with a non-Nil parent.
  6. The pop-based main loop produces orderedStates in reverse insertion order of missingStates rather than parent-before-child order. MergeState is then called in arbitrary order; the comment at state.go:257-260 ("This implicitely sets the parent ... will set the correct parent") depends on parents being processed first, so the resulting aggregated parent chain is wrong.

Addressing the bug_002 refutation

One verifier refuted bug_002 as a duplicate of an internal bug_005 (same root cause and lines). The duplicate observation is correct — both describe the same FromStream defect — and the underlying claim is itself confirmed by that same verifier. It is not a refutation of the bug's validity, only of double-counting. The synthesis-merged report folds both ReadHeader (hot path) and FromStream (latent) into a single review item, which is the right framing: they are two faces of the same incomplete refactor and the fix is identical.

Fix

Mirror the updated MergeState dispatch at both sites — treat 1.0.0 as the legacy case (no parent header / v100 deserializer) and everything else as the current format:

// ReadHeader
if !ver.Equals(versioning.FromString("1.0.0")) {
    n, err := rd.Read(hdr.Parent[:])
    ...
}

// FromStream
if ver.Equals(versioning.FromString("1.0.0")) {
    err = st.deserializeFromStreamv100(rd)
} else {
    err = st.deserializeFromStream(rd)
}

Adding a regression test that round-trips a v1.2.0 state with a non-Nil parent through ReadHeader (and ideally feeds two such states with a parent/child relation into TopoSort) would lock the invariant down.

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.

2 participants