fsck: Add read-only diagnostic report for composefs state by cgwalters-bot · Pull Request #2497 · bootc-dev/bootc

cgwalters-bot · 2026-09-24T17:35:03Z

Why

Issue #2422 shows that missing composefs OCI metadata can make status, upgrade, and switch fail even after booting an older deployment. A read-only report makes it possible to collect boot, deployment, repository, and image evidence without mutating a broken system or stopping at the first missing input.

The collector bounds its reads. Fatal errors (no sysroot) fail the command; everything else is recorded as a typed finding and collection continues. This is a large patch; please review the report schema and overall approach before treating it as ready to merge.

The content of Colin's commit (fsck: Add read-only diagnostic report) changed in the review rework, which also took in Johan's comments, so his Signed-off-by was dropped pending his re-review.

Verification

On a 16-core devspace with BOOTC_base=quay.io/fedora/fedora-bootc:44, at 64e3608:

just validate: passed
just unit-tests: passed (bootc-lib: 299 passed)
just test-tmt --composefs-backend --bootloader=grub --filesystem=ext4 --boot-type=bls readonly (incl. 015-test-fsck: 0 findings): passed

Related: #2422

Assisted-by: AI

Generated-by: https://github.com/cgwalters/#llms

cgwalters · 2026-09-24T18:51:37Z

+    collect_readonly_with_roots(&roots)
+}
+
+fn collect_readonly_with_roots(roots: &ReadonlyRoots<'_>) -> ReadonlyReport {


This is way too big of a function, needs to be split up

Split up: collect_readonly_with_roots now just sequences composefs_report, collect_deployments/collect_deployment, resolve_manifest_stream and collect_host. The ReadonlyRoots struct is gone; four of its six dirs were never used. All of this is squashed into your commit, now a030d83 (fsck: Add read-only diagnostic report), with your sign-off kept.

cgwalters · 2026-09-24T18:52:43Z

+        detail: sanitize(detail),
+    }
+}
+fn read_composefs_cmdline(


Shouldn't reimplement this

Now uses ComposefsCmdline::find_in_cmdline. The old parser only accepted 64 hex digits, so it redacted every real (sha512) composefs= value.

cgwalters · 2026-09-24T18:54:54Z

+    truncated: &mut bool,
+    total: &mut u64,
+) -> RuntimeStaged {
+    let Ok(dir) = open_dir_beneath(sysroot, Path::new("run/composefs")) else {


While fsck needs to run to completion, we also should not just completely swallow errors.

This can also use cap-std-ext open_dir_optional.

I think our fsck infrastructure should be like the linting code - we have a Result<Result<>> or really Result<Vec<Result<>> or so, the outer is really truly fatal/unexpected runtime errors, the inner Vec<Result> is any errors we encoutered while running fsck.

Opens now use open_dir_optional: a missing dir is None, and any other error becomes a finding (open_dir_or_record). That covers the staged marker, the deploy dir, the repository and the BLS/GRUB dirs. The report now has the two levels you described: collect_readonly_host returns anyhow::Result for fatal errors (e.g. no sysroot), and everything else goes into the findings list while collection continues. I left the existing FsckResult/FSCK_CHECKS alone; whether to convert those too is asked in cgwalters-forge/tracker#161.

cgwalters · 2026-09-24T18:55:05Z

+        Err(e) => {
+            findings.push(finding(
+                "STAGED_MARKER_UNREADABLE",
+                "run/composefs/staged-deployment",


Reuse existing const

Now uses STATE_DIR_RELATIVE, COMPOSEFS, COMPOSEFS_TRANSIENT_STATE_DIR, COMPOSEFS_STAGED_DEPLOYMENT_FNAME and ORIGIN_KEY_IMAGE/ORIGIN_KEY_MANIFEST_DIGEST.

cgwalters · 2026-09-24T18:55:30Z

+    Ok(dir)
+}
+
+fn open_from(dir: &Dir, relative: &Path) -> std::io::Result<File> {


Don't reimplement stuff like this use cap-std or have a strong rationale why it's not needed

Dropped open_from/open_beneath/open_dir_from/metadata_beneath and the raw openat walks in favor of cap-std, which already confines every lookup to the dir. The only helper left is open_nofollow, which checks symlink_metadata first so that a symlinked final component is reported instead of being read.

cgwalters · 2026-09-24T18:56:39Z

+        Ok(dir) => dir,
+        Err(e) => {
+            findings.push(finding(
+                "REPOSITORY_DIRECTORY_UNREADABLE",


The SCREAMING_SNAKE_CASE is ugly, let's use PascalCase basically each of these is like an error enum per above

Codes are now a FindingCode enum that serializes as PascalCase (e.g. DeploymentOriginMissing). is_collection_error() replaces the string suffix matching.

cgwalters · 2026-09-24T18:57:03Z

+        })
+        .collect()
+}
+impl ImageReport {


Btw doesn't cargo fmt want a space here? We should ensure it does...

rustfmt doesn't add blank lines between items. Its only knob for that, blank_lines_lower_bound, is nightly-only and applies inside blocks too, so we can't enforce it there. I added the blank lines across fsck.rs by hand.

cgwalters · 2026-09-24T18:57:28Z

+    }
+    edges
+}
+fn valid_sha256(v: &str) -> bool {


There's sstuff for this in oci-spec, avoid reimplmeenting your own validators

Now parses with oci-spec's Digest::from_str, which accepts any valid algorithm.

Johan-Liebert1

Not a full review because Github is unable to load a ~3k diff...

Johan-Liebert1 · 2026-09-25T08:39:44Z

+const MAX_ARTIFACTS: usize = 256;
+const MAX_LAYERS: usize = 256;
+const MAX_REPORT_STRING: usize = 256;
+const EFI_LOADER_INFO: &str = "LoaderInfo-4a67b082-0a4c-41cf-b6c7-440b29bb8c4f";


We have this constant elsewhere

Fixed: it now uses crate::install::EFI_LOADER_INFO.

Johan-Liebert1 · 2026-09-25T08:43:42Z

+                repo: open_dir_from(&sysroot, Path::new("composefs")).ok(),
+                deploy: open_dir_from(&sysroot, Path::new("state/deploy")).ok(),
+                run: open_dir_from(&sysroot, Path::new("run")).ok(),
+                boot: open_dir_from(&sysroot, Path::new("boot")).ok(),


For grub classic systems we have entries in /sysroot/boot

Those are covered: the BLS roots are relative to the sysroot, so boot/loader/entries (and .staged) is /sysroot/boot/loader/entries, and GRUB user.cfg is read from boot/grub2 and boot/grub. The ReadonlyRoots.boot field this was on was never used and has been removed.

Johan-Liebert1 · 2026-09-25T08:45:07Z

+                deploy: open_dir_from(&sysroot, Path::new("state/deploy")).ok(),
+                run: open_dir_from(&sysroot, Path::new("run")).ok(),
+                boot: open_dir_from(&sysroot, Path::new("boot")).ok(),
+                esp: open_dir_from(&sysroot, Path::new("boot/efi")).ok(),


If the image has /efi with XBOOTLDR then ESP is mounted at /efi

Fixed: efi/loader/entries (and .staged) are now BLS roots too, and boot artifacts are also looked up under efi/. The ESP mount check already accepted /sysroot/efi.

Johan-Liebert1 · 2026-09-25T08:46:19Z

+    let mut findings = Vec::new();
+    let mut truncated = false;
+    let mut read_bytes = 0;
+    let composefs_dir = open_dir_beneath(sysroot, Path::new("composefs"));


Don't we already have this in ReadonlyRoots.repo?

Yes. ReadonlyRoots is gone and the repository is opened once in composefs_report.

Johan-Liebert1 · 2026-09-25T08:50:18Z

+        .and_then(|dir| metadata_at(dir, "meta.json".as_ref()).ok());
+    let meta_contents = meta_fs
+        .as_ref()
+        .is_some_and(|m| m.file_type == EntryType::File)


If it's a symlink (which we're not treating as an error) we won't read it.

Right. A symlinked composefs/meta.json now gets a RepositoryMetadataSymlink finding. A symlink there, or any error other than not-found, used to look the same as the file being absent.

Johan-Liebert1 · 2026-09-25T09:00:16Z

+                Ok(ini) => {
+                    deployment.image = ini
+                        .get::<String>("origin", "container")
+                        .map(|v| redact_ref(&v));


Wouldn't redacting be very unhelpful here as we won't be able to determine if the origin points to the correct oci image or not?

Agreed. The origin image ref is now reported as-is, digest-pinned @sha256: refs included. Only URL-style user:password@ credentials are redacted.

Johan-Liebert1 · 2026-09-25T09:05:22Z

+    truncated: &mut bool,
+    total: &mut u64,
+) -> RuntimeStaged {
+    let Ok(dir) = open_dir_beneath(sysroot, Path::new("run/composefs")) else {


this is incorrect. run/composefs is relative to / and not /sysroot

Good catch, fixed: the staged marker is now read from the host root's run/composefs, and there's a unit test with separate host and sysroot dirs.

When the head is his own commit, he acted on his review himself, as on bootc-dev/bootc#2497, where he force-pushed after his line comments and the PR was then listed as outstanding every sweep. Pushes by anyone else, like GitHub's "Update branch" (web-flow), still answer nothing. Generated-by: AI

cgwalters-bot · 2026-09-26T04:07:23Z

Rebased onto main; 1 commit, no content change.

Generated-by: https://github.com/cgwalters/#llms

cgwalters-bot · 2026-09-26T06:05:09Z

Fixed the CI failures, squashed into the one commit (now 94f6532). The failing baseconfigs, test-upgrade and sealed UKI legs all failed in 015-test-fsck.nu on false findings, not on the transient/volatile layouts:

OCI_MANIFEST_DIGEST_INVALID: manifests and configs are stored as splitstreams, so the report now reads the JSON through SplitStreamReader instead of hashing the stream file.
BOOT_ARTIFACT_MISSING: GRUB BLS paths written as /boot/... (no separate /boot partition) are now also resolved against the physical root.
The test asserted describe starts with list, but nushell reports a non-empty list of records as table<...>.

Tested on a 16-core devspace: cargo test -p bootc-lib --lib fsck (19 passed), just test-tmt-baseconfig var-volatile and just test-upgrade (composefs, grub, ext4, centos-10), both passing plan-01-readonly. Your review comments aren't addressed yet.

Generated-by: https://github.com/cgwalters/#llms

cgwalters · 2026-09-29T00:40:39Z

+/// A problem found while collecting the report.  These are the "inner"
+/// errors of fsck: collection continues past them, and each is reported.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize)]
+enum FindingCode {


I love the idea of structured errors but I think the real problem here is that ultimately we need to drive some of this down into the relevant code and stop using anyhow::Error which is gonna be painful. But let's track that as a TODO.

How about breaking this into at least separate enums for each subsystem? e.g. composefs vs ostree, /boot, grub-related things etc.

Split FindingCode into per-subsystem enums (collection, host, composefs, deployment, /boot, GRUB, EFI) wrapped by an untagged outer enum, so the JSON codes are unchanged. Added a TODO on the enum about moving typed errors into the inspected code, tracked in cgwalters-forge/tracker#248. (No ostree group: the readonly report has no ostree findings yet.)

Generated-by: https://github.com/cgwalters/#llms

cgwalters · 2026-09-29T13:52:32Z

+
+impl FindingCode {
+    /// Whether this finding means some input could not be collected at all.
+    fn is_collection_error(self) -> bool {


How about may_mask_further_errors

Renamed to may_mask_further_errors.

Generated-by: https://github.com/cgwalters/#llms

cgwalters · 2026-09-29T13:52:48Z

+        serde_json::to_writer_pretty(&mut output, &result)?;
+        writeln!(output)?;


This is an inefficient pattern lock stdout write directly

I read this as the locked stdout being line-buffered, so the pretty JSON went out a line per write; the output is now wrapped in a BufWriter and flushed once. Let me know if you meant something else.

Generated-by: https://github.com/cgwalters/#llms

cgwalters-bot · 2026-09-30T23:36:12Z

@cgwalters Rebased onto main; 1 commit (now 7b10116), no content change, so nothing new to re-approve. It still awaits your review.

Generated-by: https://github.com/cgwalters/#llms

The checks returned at most one free-form error string, so the fsverity check pre-formatted its own truncated list of objects, and nothing could tell the kinds of problems apart. The readonly report being added in bootc-dev#2497 settled on a two-level shape instead: fatal errors in the outer anyhow::Result, every problem found collected as a typed finding with a subject. Use the same shape here, so the two can share a finding type once that lands. The output stays bounded: the runner prints at most five findings per check and counts the rest, as the fsverity check did itself before. The messages become one "code: subject: detail" line per finding; the tmt tests only look at the exit status. Generated-by: AI

The checks returned at most one free-form error string, so the fsverity check pre-formatted its own truncated list of objects, and nothing could tell the kinds of problems apart. The readonly report being added in bootc-dev#2497 settled on a two-level shape instead: fatal errors in the outer anyhow::Result, every problem found collected as a typed finding with a subject. Use the same shape here, so the two can share a finding type once that lands. The output stays bounded: the runner prints at most five findings per check and counts the rest, as the fsverity check did itself before. The messages become one "code: subject: detail" line per finding; the tmt tests only look at the exit status. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>

The checks returned at most one free-form error string, so the fsverity check pre-formatted its own truncated list of objects, and nothing could tell the kinds of problems apart. The readonly report being added in #2497 settled on a two-level shape instead: fatal errors in the outer anyhow::Result, every problem found collected as a typed finding with a subject. Use the same shape here, so the two can share a finding type once that lands. The output stays bounded: the runner prints at most five findings per check and counts the rest, as the fsverity check did itself before. The messages become one "code: subject: detail" line per finding; the tmt tests only look at the exit status. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>

Provide the diagnostic evidence requested by bootc-dev#2422 without changing the booted system. Collection is capability-confined and non-mutating, includes composefs and boot evidence, and reports bounded partial failures rather than aborting on the first unavailable input. Assisted-by: AI Related: bootc-dev#2422

cgwalters-bot · 2026-10-02T22:32:24Z

@cgwalters Rebased onto main (now 6c38116, still 1 commit) after #2528 landed, and unified the two finding types as planned. Of your open review (changes requested at f074a15), the three comments (per-subsystem enums, may_mask_further_errors, buffered stdout) were already answered and pushed in the previous head, so nothing new is outstanding from it; it still needs your re-review.

What changed since 7b10116 (git range-diff, conflict-only plus the unification):

One Finding struct (code, subject, detail) now serves both the readonly report and the checks; fsck: Return typed findings from the checks #2528's CheckFinding struct is gone, and its CheckFindingCode enum became CheckFinding, one more group of FindingCode next to the composefs, deployment, boot, GRUB and EFI ones. FsckResult is anyhow::Result<Vec<Finding>>.
Behaviour is unchanged: the check output lines and the report JSON codes are the same (the check codes are untagged like the others).

Tested on a 16-core devspace (cgwalters-devspace-37068905608) at 6c38116: just validate, just unit-tests (fsck tests included) and the readonly tmt plan (plan-01-readonly) on both the ostree/grub and composefs/systemd/bls images, all passing.

Generated-by: https://github.com/cgwalters/#llms

bootc-bot Bot requested a review from cgwalters September 24, 2026 17:35

github-actions Bot added the area/documentation Updates to the documentation label Sep 24, 2026

cgwalters reviewed Sep 24, 2026

View reviewed changes

cgwalters-bot added this to Workstream Sep 24, 2026

cgwalters-bot moved this to In Review in Workstream Sep 24, 2026

cgwalters force-pushed the issue-2422-fsck-report branch from 411768d to 95399e3 Compare September 24, 2026 21:55

Johan-Liebert1 reviewed Sep 25, 2026

View reviewed changes

cgwalters-bot moved this from In Review to Needs human in Workstream Sep 25, 2026

cgwalters-bot added this to Composefs Stable Sep 25, 2026

cgwalters-bot moved this to Blocked in Composefs Stable Sep 25, 2026

cgwalters-bot force-pushed the issue-2422-fsck-report branch 2 times, most recently from f06e364 to 94f6532 Compare September 26, 2026 06:05

cgwalters-bot force-pushed the issue-2422-fsck-report branch 3 times, most recently from adda0ae to f074a15 Compare September 26, 2026 22:32

cgwalters-bot mentioned this pull request Sep 29, 2026

bootc fsck: convert FSCK_CHECKS/FsckResult to the Result<Vec<Finding>> shape cgwalters-forge/tracker#173

Open

cgwalters requested changes Sep 29, 2026

View reviewed changes

cgwalters-bot added this to Workstream Sep 30, 2026

cgwalters-bot mentioned this pull request Sep 30, 2026

bootc fsck: drive typed errors down into the storage and boot code instead of anyhow::Error cgwalters-forge/tracker#248

Open

cgwalters-bot force-pushed the issue-2422-fsck-report branch from f074a15 to 64e3608 Compare September 30, 2026 12:43

cgwalters-bot force-pushed the issue-2422-fsck-report branch from 64e3608 to 7b10116 Compare September 30, 2026 23:36

This was referenced Oct 1, 2026

Morning brief 2026-10-01 cgwalters-bot/cgwalters-bot#18

Open

fsck: Return typed findings from the checks cgwalters-forge/bootc#35

Closed

cgwalters-bot mentioned this pull request Oct 2, 2026

fsck: Return typed findings from the checks #2528

Merged

cgwalters-bot force-pushed the issue-2422-fsck-report branch from 7b10116 to 6c38116 Compare October 2, 2026 22:32

		serde_json::to_writer_pretty(&mut output, &result)?;
		writeln!(output)?;

Conversation

cgwalters-bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters-bot Sep 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters-bot commented Sep 26, 2026

Uh oh!

cgwalters-bot commented Sep 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cgwalters-bot commented Sep 24, 2026 •

edited

Loading

cgwalters-bot Sep 26, 2026 •

edited

Loading

cgwalters-bot commented Sep 30, 2026 •

edited

Loading