erofs: restore multi-layer splitting, with the bugs review found fixed

[smoser]

Section 2 of #2408: brings back the EROFS layer split, descoped in
e5778f1 so #2249 could land as a single-layer writer plus ls, with the
five bugs review found fixed on top.

#2415 has since merged, and this is rebased on it. The code is still
disjoint — that PR is pkg/erofsmount and the CLI, this one is
pkg/build — they meet only in hack/test-erofs.sh.

1. Restore, unchanged

pkg/build/erofs_layers.go and its test come back exactly as they were,
and the layering + format: erofs rejection leaves
ImageConfiguration.Validate and buildLayers. This commit deliberately
restores the known-broken behavior; the four after it are the fix, and
that delta is the point of the PR.

2. Package routing — the bug that inverted the whole thing

The type assertion asked info.Sys() for a Package() method. tarfs,
which is what a real build walks, returns a fresh *tar.Header from
Sys() and hangs Package() off the FileInfo itself — the receiver
splitLayers asserts on. So it never succeeded outside a fixture: every
file landed in the top layer, each group layer held only ancestor
directories plus a partial installed db (that case keys on the path
string, so it still fired), and per-layer scanners read a db naming
packages whose files were not there.

Assert on info, and turn a packageToWriter miss into an error rather
than a silent fall back to top.

TestSplitErofsLayers could not catch this: it drove the split through
apkfs.NewMemFS(), which implements Package() nowhere, so the fixture
had zero package-owned files by construction and passed identically with
and without routing. It is replaced by erofs_layers_test.go, which
builds its fixture with pkg/tarfs and installs files through
WriteHeader with an *apk.Package, the way an apk install does. Its
routing assertions fail against the previous code.

3. Directory-only subtrees

Directories were recorded during the walk and materialized only by
emitAncestors, which runs for non-directory entries. A directory whose
subtree holds no file was therefore never written anywhere: /tmp,
/run, /var/empty, every empty dir and every mount point were absent
from the merged view. Each directory is now emitted into its owning
writer as it is walked, which is what both siblings already do.

4. Temp files and fds

No error return out of splitErofsLayers closed or removed the per-layer
temp files, and each go-erofs Writer also holds an unlinked spool fd
that only Close releases. The CLI happened to be bounded by its
MkdirTemp/RemoveAll wrapper; a library caller accumulated both until
process exit. The per-writer state moves into an erofsGroupWriter type
with finish/discard, and a deferred sweep discards every writer
unless the function reaches its return. Still no upstream abort API, so
discard writes an image it then removes.

5. Build time

newErofsGroupWriter passed WithBuildTime only for a non-zero build
time — the exact case erofsBuildTime's comment describes, where
go-erofs stamps time.Now() from Close and a library caller gets
different layer digests every build. It now uses erofsBuildTime, same
as writeErofs. apko's own CLI was never affected.

6. Drive it from hack/test-erofs.sh

The privileged job from #2414, extended by #2415, only built and mounted
a single layer, because that was all apko could produce. The script now
builds the same config again with layering and adds, after the existing
comparisons:

• layer roles and mediaTypes across the manifest, and digest == the
  sha256 of each blob;
• fsck.erofs on every layer, and a kernel mount of every layer stacked
  as read-only overlayfs lowerdirs;
• apko erofs ls on the layered OCI directory diffed against that
  overlay mount — Stack's merge only becomes reachable for apko's own
  output now that splitting is back, and this is the first thing to
  compare it with what the kernel assembles;
• every non-final layer must hold a regular file other than the partial
  installed db, which is exactly the shape the routing bug produced;
• the merged tree diffed against a tar build of the same config
  unpacked in layer order. The tar split is the reference for what
  splitting must preserve, and a directory that reaches no layer shows up
  here and nowhere else. Both builds resolve from one apko lock output
  so a package published between them cannot make them differ. The unpack
  uses --numeric-owner; without it GNU tar resolves each header's
  uname/gname against the runner's /etc/passwd and invents ids for
  lp, mail, news and uucp.

The layered section runs last, after #2415's apko erofs mount /
apko erofs umount checks, and reuses their helpers — fail,
assert_mounted, normalize_ls, tree_listing. Their cleanup already
unmounts everything under the workdir deepest-first, so the layer and
overlay mounts need no separate bookkeeping. The one existing line this
touches is the whitespace-in-paths guard, lifted into a
check_ls_whitespace function so the layered listing gets it too.

Not in this PR

Whiteout support in Stack — the last item in section 2 — is really the
layer-horizon fix from section 5, and is reader-side and independently
testable. Left for its own PR.

Verification

gofmt -l clean, golangci-lint run -n reports 0 issues, go build ./... and GOOS=darwin go build ./... both succeed, and
SOURCE_DATE_EPOCH=0 go test ./... passes. shellcheck is clean on the
script.

Each fix was checked to fail without its change: the four new tests are
red against the restore commit and green after their own commit.

Locally, a layered wolfi-base build produces five layers whose merged
apko erofs ls listing is identical to the single-layer build's, and
whose non-top layers are 0.7–7.4 MB rather than the directories-only
skeletons the routing bug produced.

Refs #2408

🤖 Generated with Claude Code

[mattmoor]

No verdict yet — comments inline. Verification notes so the threads don't re-litigate what already checks out: the restore commit is byte-identical to e5778f17^; 5 of the 6 new test functions fail against the restore commit as advertised (only the carried-over LayersAreValidImages passes); the grouping semantics match the tar path (same Package() interface on the same receiver, unowned→top, miss→loud); and the cleanup machinery probed clean — no double-close, no leak on partial construction, discard-on-success correct, layer order deterministic under -count=2. The inline comments are parity/efficiency items plus one cross-PR follow-up with #2422.

[mattmoor]

Once #2422 lands, this walk still materializes every hardlink as a full copy — including when link and target land in the same writer — because nothing here goes through hardlinkTarget/the deferred-Link pass; only writeErofs does. Probed on a merged tree of the two PRs: a group layer holding a 64K file plus two extra names comes out 204800 bytes with three nlink=1 inodes, vs 73728 with one nlink=3 inode from the single-layer build. The tar split doesn't pay this (links ride as zero-size TypeLink headers), and hack/test-erofs.sh's erofs-vs-tar diff can't catch it, since copied content compares equal. Fine as a follow-up — collect an erofsHardlink slice per owning writer during the walk and run emitErofsHardlinks per writer before finish() — but can we get it filed before layering+erofs is user-reachable, so a layered coreutils doesn't silently lose #2422's benefit?

[smoser]

Filed as #2451, with your measurement and the same shape you describe: a per-writer erofsHardlink slice collected during the split walk, emitErofsHardlinks before each finish(), and cross-layer targets staying materialized since Writer.Link can only bind within one image (§3.7).

Agreed it belongs before the combination is user-reachable — this PR is what removes the rejection, so #2451 is the gate on the release that ships both, not on this merge.

[mattmoor]

splitLayers deliberately normalizes ancestor-dir ModTime in group layers (layers.go, "we can improve deduplication without having any real effect on the image…") because merged-view dir mtimes are last-installer-wins — without that, a group layer's bytes depend on packages outside the group. This path re-emits ancestors with faithful mtimes, so an identical package group can hash to different layer digests across images. Note SOURCE_DATE_EPOCH doesn't clamp fs entry mtimes (it feeds the superblock, scripts.tar, installed db, and config), so this bites at the default epoch too — probed: varying only a shared dir's mtime changes the erofs group-layer digest while the tar group's diffid stays stable. Worth mirroring the normalization when replicating ancestors into non-owner writers, or a comment acknowledging the divergence.

[smoser]

Fixed in ccabf99. emitAncestors now takes the triggering entry's mtime and stamps it on every ancestor it creates, via a FileInfo wrapper that forwards Sys() so uid/gid still come from the source header — the same thing alignStacks does for the tar path, for the same reason.

The owning writer is unaffected: a directory is emitted into its owner when the walk reaches it, with its real mtime, and that is the copy that wins on merge.

TestSplitErofsLayers_AncestorModTimeIsNormalized builds the same group twice varying only a shared directory's mtime and compares layers[0].Digest(). Red against the parent commit.

Good catch on SOURCE_DATE_EPOCH not covering it — I'd assumed it did, and the test would have passed for the wrong reason if it had.

[mattmoor]

go-erofs also takes WithTempDir: the spool for regular-file data goes to os.CreateTemp(tempDir, …) — unlinked, held until Close — and with the default it lands in the system temp dir. A layered build holds len(groups)+1 writers open at once, so that's roughly a rootfs of scratch on a volume the caller can't choose, while tmpdir is right here: erofs.Create(f, erofs.WithBuildTime(sec, nsec), erofs.WithTempDir(tmpdir)). Same nit applies to writeErofs, pre-existing.

[smoser]

Fixed in 4072a40, both call sites. writeErofs takes a tmpdir and the CLI passes bc.o.TempDir() — the same directory it already writes the layer files into — so the spool follows the build's temp dir rather than the system one.

[mattmoor]

Latent edge, unreachable today: if the installed db ever carried a non-nil Package() (routing it to a group), this special case writes the partial db into that group's writer and marks it emitted, and then the normal path below emits the full db into the same writer at the same path — go-erofs errors on duplicate paths, where the tar sibling tolerates the identical shape (last-wins). Regular files have no owner.emitted guard the way directories do at L202. A guard before the emit at L245 — or a comment — would future-proof it.

[smoser]

Guarded in e3a34d2: an owner.emitted[absPath] check before the regular-file emit, mirroring the directory guard at L202.

It keeps the partial db in that hypothetical, which is also what tar's ordering picks — splitLayers writes f first and the per-group partial db after, so last-wins lands on the partial there too.

[mattmoor]

The fixtures are regular files + dirs only, so symlink routing is pinned only by the CI script — and tarfs does attach Package() to symlinks, so a package-owned symlink exercises ownerOf in a way nothing here does. A symlink entry in tarfsFixture would be cheap. (Package-owned directories are impossible through tarfs — WriteHeader TypeDir goes through MkdirAll with no entry — so that branch genuinely can't be driven harder.)

[smoser]

Added in 9f21b6c. entry gained a link field, tarfsFixture sends it through the same WriteHeader path with the owning package attached, and the routing test now carries a package-owned usr/bin/one-compat.

Checked it isn't vacuous: forcing ownerOf to return top for symlinks makes the assertion fire.

Left package-owned directories alone for the reason you gave — WriteHeader TypeDir goes through MkdirAll and records no entry, so there's nothing to attach a package to.

[mattmoor]

This assumes every group contributes at least one regular file besides the partial db — true for wolfi-base, but a symlink-only package group would false-fail if someone points the script at another yaml (the usage line invites it). Fine as-is if that's understood; a comment on the assumption would help the next person.

[smoser]

Understood, and documented in 842083b — the comment now names the assumption and the -type f -o -type l relaxation for anyone pointing the script at another yaml.

[mattmoor]

This bullet is where #2422 conflicts, and neither side's wording survives the pair: post-merge reality is "inodes are shared in single-layer builds; a layered build currently materializes every hardlink as a copy" (see the thread on the routing walk). Also its rationale is already stale — the pinned go-erofs has had Writer.Link with shared-inode semantics since the #2412 bump. Whichever PR rebases second owns the merged wording.

[smoser]

842083b drops the stale half now, independent of #2422: the bullet blamed go-erofs for having no API, which stopped being true at the #2412 bump. It now says apko's writer materializes each link, which is a statement about apko rather than the library.

On the merge: agreed, whoever rebases second owns it. If that ends up being this PR I'll take the bullet to "inodes are shared in single-layer builds; a layered build still materializes every hardlink as a copy" and point it at #2451.

[mattmoor]

The response commits all verify. The normalization picks the same triggering-entry mtime the tar path does, and the wrapper preserves everything else — probed a setgid+sticky, xattr'd, chowned ancestor replicated into a group writer: only the mtime differs. The digest-stability test fails at the previous head; the once-per-writer guard lands on the partial db exactly as tar's write ordering does; WithTempDir covers both call sites with the right value; and the symlink fixture fires under the ownerOf-to-top mutation.

Second-pass sweeps came back clean where it matters: all 12 commits build and pass tests individually on darwin and GOOS=linux, so the series bisects; layer assembly is right end to end (diffID == digest for the uncompressed media type, one history entry per layer same as the tar path, role annotations ride the ggcr Addendum, os.features set); empty/metapackage groups produce a valid partial-db-only layer with counts intact; and the one genuinely dangerous merge case — the partial installed db in every group layer vs the full db on top — resolves correctly: Stack's lookup is strictly topmost-wins over manifest order (probed with a 3-layer partial/partial/full fixture), agreeing with the kernel. Remaining inline comments are docs/script nits plus one theoretical note; none blocking. Thanks for filing #2451 for the split-path hardlink port.

Found while probing, pre-existing and format-neutral (separate-issue material): tarfs WriteHeader(TypeDir) drops setgid/sticky and uid/gid from tar dir headers — both the tar and erofs paths serialize the degraded node state equally.

[mattmoor]

Nit: the final layer carries the full installed db, not a partial one — the partial-db loop covers only groups, and the top layer contributes no packages. Something like "each package-group layer carries a partial usr/lib/apk/db/installed …; the final layer carries the full db" would match what splitErofsLayers writes.

[mattmoor]

Nit: plain jq pretty-prints multi-line; jq -c produces the one-object-per-line output the Expected block shows.

[mattmoor]

The note could also mention metapackage groups (zero files at all) — they false-fail the same way as symlink-only ones. Probed that the split itself handles them fine: the layer holds exactly the partial db plus ancestors, stays a valid mountable EROFS image, and the layer count stays len(groups)+1.

[mattmoor]

Optional hardening, not for this PR necessarily: normalize_ls keeps mode/owner/path and tree_listing has no size either, so a wrong-layer pick of usr/lib/apk/db/installed — the one path present in every layer with different content but identical name/mode/owner — would pass every diff in this script. I verified Stack is strictly topmost-wins (and TestStack_Override_TopWins pins it at unit level), so this is belt-and-suspenders: carrying the size column for regular-file lines on both sides would let the end-to-end check see it.

[mattmoor]

Root is now the one replicated path that keeps its faithful mtime in every writer. tarfs's root mtime is the zero time today, so Chtimes is skipped and every writer gets the uniform build time — but that's an assumption, not an invariant: a package data tar carrying a ./ directory header appears to reach WriteHeader (the install loop only skips bare dot names without a /) and would touch the root's mtime, recoupling every group digest to whichever package last did so. Worth either normalizing "/" like the other replicated ancestors or a comment pinning the assumption.