Skip to content

Store packfile size metadata. #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mathieu-plak wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Store packfile size metadata. #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8a4989
state: Store the packfile size.
mathieu-plak May 13, 2026
f24fa76
repository: Change the way we compute repo's size.
mathieu-plak May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 30 additions & 5 deletions repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,14 +549,39 @@ func (r *Repository) Store() storage.Store {

func (r *Repository) StorageSize() (int64, error) {
if r.storageSizeDirty {
size, err := r.store.Size(r.appContext)
if err != nil {
return 0, err
var totSize uint64
var old bool

if r.state != nil {
for p, err := range r.state.ListPackfileEntries() {
if err != nil {
return 0, err
}

// We have an old repo, fallback to slow method
if p.Size == 0 {
old = true
break
}

totSize += p.Size
}
}

r.storageSize = size
r.storageSizeDirty = false
if old {
size, err := r.store.Size(r.appContext)
if err != nil {
return 0, err
}

r.storageSize = size
r.storageSizeDirty = false
} else {
r.storageSize = int64(totSize)
r.storageSizeDirty = false
}
}

return r.storageSize, nil
}

Expand Down
4 changes: 2 additions & 2 deletions repository/repositorywriter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ func (r *RepositoryWriter) PutPackfile(pfile packfile.Packfile) error {

db.Commit()

return r.currentDeltaState().PutPackfile(r.currentStateID, mac)
return r.currentDeltaState().PutPackfile(r.currentStateID, mac, uint64(nbytes))
}

func (r *RepositoryWriter) PutPtarPackfile(packfile *packer.PackWriter) error {
Expand Down Expand Up @@ -342,5 +342,5 @@ func (r *RepositoryWriter) PutPtarPackfile(packfile *packer.PackWriter) error {
}
}

return r.currentDeltaState().PutPackfile(r.currentStateID, mac)
return r.currentDeltaState().PutPackfile(r.currentStateID, mac, uint64(nbytes))
}
45 changes: 29 additions & 16 deletions repository/state/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"github.com/vmihailenco/msgpack/v5"
)

const VERSION = "1.1.0"
const VERSION = "1.2.0"

func init() {
versioning.Register(resources.RT_STATE, versioning.FromString(VERSION))
Expand Down Expand Up @@ -92,9 +92,11 @@
Packfile objects.MAC
StateID objects.MAC
Timestamp time.Time
Size uint64
}

const PackfileEntrySerializedSize = 32 + 32 + 8
const PackfileEntrySerializedSizeV1 = 32 + 32 + 8
const PackfileEntrySerializedSizeV2 = 32 + 32 + 8 + 8

type ConfigurationEntry struct {
Key string
Expand Down Expand Up @@ -257,11 +259,11 @@
// This implicitely sets the parent, see the note about refactoring, and
// since we Derive() to construct Delta streams this will set the correct
// parent. This is all way too intricated and will be fixed by a refacto.
if ver.Equals(versioning.FromString("1.1.0")) {
err = ls.deserializeFromStream(rd)
} else {
if ver.Equals(versioning.FromString("1.0.0")) {
err = ls.deserializeFromStreamv100(rd)
} else {
err = ls.deserializeFromStream(rd)
}

Check failure on line 266 in repository/state/state.go

View check run for this annotation

Claude / Claude Code Review

Version dispatch for v1.2.0 not propagated to ReadHeader/FromStream 

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 pare
Comment on lines +262 to 266

@claude claude Bot May 21, 2026

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.

if err != nil {
return err
}
Expand Down Expand Up @@ -379,7 +381,7 @@
return fmt.Errorf("failed to write packfile entry type: %w", err)
}

if err := writeUint32(PackfileEntrySerializedSize); err != nil {
if err := writeUint32(PackfileEntrySerializedSizeV2); err != nil {
return fmt.Errorf("failed to write packfile entry length: %w", err)
}

Expand Down Expand Up @@ -551,6 +553,10 @@
timestamp := binary.LittleEndian.Uint64(bbuf.Next(8))
pe.Timestamp = time.Unix(0, int64(timestamp))

if len(buf) == PackfileEntrySerializedSizeV2 {
pe.Size = binary.LittleEndian.Uint64(bbuf.Next(8))
}

return
}

Expand All @@ -559,10 +565,12 @@
pos += copy(buf[pos:], pe.Packfile[:])
pos += copy(buf[pos:], pe.StateID[:])
binary.LittleEndian.PutUint64(buf[pos:], uint64(pe.Timestamp.UnixNano()))
pos += 8
binary.LittleEndian.PutUint64(buf[pos:], uint64(pe.Size))
}

func (pe *PackfileEntry) ToBytes() (ret []byte) {
ret = make([]byte, PackfileEntrySerializedSize)
ret = make([]byte, PackfileEntrySerializedSizeV2)
pe._toBytes(ret)
return
}
Expand Down Expand Up @@ -674,7 +682,6 @@
de_buf := make([]byte, DeltaEntrySerializedSize)
del_buf := make([]byte, DeleteEntrySerializedSize)
coloured_buf := make([]byte, ColouredEntrySerializedSize)
pe_buf := make([]byte, PackfileEntrySerializedSize)
for {
n, err := r.Read(et_buf)
if err != nil || n != len(et_buf) {
Expand Down Expand Up @@ -755,10 +762,11 @@
return err
}
case ET_PACKFILE:
if length != PackfileEntrySerializedSize {
return fmt.Errorf("failed to read packfile entry wrong length got(%d)/expected(%d)", length, PackfileEntrySerializedSize)
if length != PackfileEntrySerializedSizeV1 && length != PackfileEntrySerializedSizeV2 {
return fmt.Errorf("failed to read packfile entry wrong length got(%d)/expected(%d or %d)", length, PackfileEntrySerializedSizeV1, PackfileEntrySerializedSizeV2)
}

pe_buf := make([]byte, length)
if n, err := io.ReadFull(r, pe_buf); err != nil {
return fmt.Errorf("failed to read packfile entry %w, read(%d)/expected(%d)", err, n, length)
}
Expand All @@ -768,7 +776,9 @@
return fmt.Errorf("failed to deserialize packfile entry %w", err)
}

if err := ls.cache.PutPackfile(pe.Packfile, pe_buf); err != nil {
// We have to re-encode here to force normalize in-db to have the V2
// version.
if err := ls.cache.PutPackfile(pe.Packfile, pe.ToBytes()); err != nil {
return err
}

Expand Down Expand Up @@ -837,7 +847,7 @@
et_buf := make([]byte, 1)
de_buf := make([]byte, DeltaEntrySerializedSize)
coloured_buf := make([]byte, ColouredEntrySerializedSize)
pe_buf := make([]byte, PackfileEntrySerializedSize)
pe_buf := make([]byte, PackfileEntrySerializedSizeV1)
for {
n, err := r.Read(et_buf)
if err != nil || n != len(et_buf) {
Expand Down Expand Up @@ -894,8 +904,8 @@
return err
}
case ET_PACKFILE:
if length != PackfileEntrySerializedSize {
return fmt.Errorf("failed to read packfile entry wrong length got(%d)/expected(%d)", length, PackfileEntrySerializedSize)
if length != PackfileEntrySerializedSizeV1 {
return fmt.Errorf("failed to read packfile entry wrong length got(%d)/expected(%d)", length, PackfileEntrySerializedSizeV1)
}

if n, err := io.ReadFull(r, pe_buf); err != nil {
Expand All @@ -907,7 +917,9 @@
return fmt.Errorf("failed to deserialize packfile entry %w", err)
}

if err := ls.cache.PutPackfile(pe.Packfile, pe_buf); err != nil {
// We have to re-encode here to force normalize in-db to have the V2
// version.
if err := ls.cache.PutPackfile(pe.Packfile, pe.ToBytes()); err != nil {
return err
}

Expand Down Expand Up @@ -1080,11 +1092,12 @@
}
}

func (ls *LocalState) PutPackfile(stateId, packfile objects.MAC) error {
func (ls *LocalState) PutPackfile(stateId, packfile objects.MAC, size uint64) error {
pe := PackfileEntry{
StateID: stateId,
Packfile: packfile,
Timestamp: time.Now(),
Size: size,
}

return ls.cache.PutPackfile(pe.Packfile, pe.ToBytes())
Expand Down
4 changes: 2 additions & 2 deletions repository/state/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ func TestSerializeToStream(t *testing.T) {
StateID: objects.MAC{17, 18, 19, 20},
Timestamp: time.Now(),
}
state.PutPackfile(packfileEntry.StateID, packfileEntry.Packfile)
state.PutPackfile(packfileEntry.StateID, packfileEntry.Packfile, 0)

configEntry := &ConfigurationEntry{
Key: "test_key",
Expand Down Expand Up @@ -531,7 +531,7 @@ func TestPackfileEntrySerialization(t *testing.T) {

// Serialize
data := original.ToBytes()
require.Len(t, data, PackfileEntrySerializedSize)
require.Len(t, data, PackfileEntrySerializedSizeV2)

// Deserialize
deserialized, err := PackfileEntryFromBytes(data)
Expand Down
Loading