fastboop-session API

[samcday]

No description provided.

[samcday]

@claude review this PR, if you can ;)

[claude]

Claude finished @samcday's task in 3m 20s —— View job

---

PR Review: fastboop-session API

• Gather context and read key files
• Review new fastboop-session crate
• Review fastboop-environment-std crate
• Review fastboop-environment-web crate
• Review CLI and package changes
• Post final review

---

This is a substantial and well-executed architectural refactor. The plan.md describes the goals well, and the PR delivers cleanly on the core objectives: introducing a shared fastboop-session orchestration layer while deleting a large amount of duplicated code from the CLI and GUI packages.

What's good

Crate boundaries are clean. fastboop-session is no_std with clear trait contracts (SessionEnvironment, BootSessionEnvironment, Stage0RootfsFactory), fastboop-environment-std owns all the tokio/native bindings, and fastboop-environment-web owns the WASM bindings. The Stage0RootfsFactory abstraction is elegant.

Resume semantics are correct. is_post_handoff() correctly identifies the semantic point of no return — once BootHandoffStarted is persisted, the session skips re-issuing fastboot boot and goes straight to smoo reconnect. This matches the plan and is the right call.

Stage0CoalescingFilesystem is a nice primitive. Union of multiple rootfs providers with fallthrough is clean.

The codec is straightforward. Magic-header-prefixed postcard for session snapshots; encode/decode pair is testable and tested. Good round-trip test at lib.rs:2103.

Net deletion. cli/src/commands/mod.rs loses 2636 lines; device_boot.rs in both desktop and web packages shrink dramatically. That's the right direction.

---

Issues

CoalescingFilesystem::read_dir only uses the first provider (lib.rs:1177)

All other Filesystem methods coalesce across providers; read_dir silently delegates only to the first. If a path exists only in a secondary provider, ostree auto-detection will fail with a misleading error. Worth a comment explaining the invariant (e.g. "primary rootfs is always first and must own directory structure"), or make it coalesce too.

async fn read_dir(&self, path: &str) -> Result<Vec<String>, Self::Error> {
    let Some(first) = self.providers.first() else {
        return Err(Stage0RootfsProviderError::EmptyProviderList);
    };
    first.read_dir(path).await  // ← only first provider
}

Fix this →

SESSION_STATE_VERSION stored but never validated on decode (lib.rs:2092)

decode_session_snapshot tries to postcard-decode without checking the version field. If the schema changes in a future release, old persisted sessions will fail with a raw postcard decode error rather than a meaningful "incompatible session version" error. Checking snapshot.version != SESSION_STATE_VERSION after decoding and returning a new error variant would make this much easier to diagnose.

reader_has_fat_magic reads offset 54 twice (lib.rs:435)

let fat12 = read_magic_bytes(reader, 54, 5).await?;
if fat12.as_deref() == Some(b"FAT12") { return Ok(true); }

let fat16 = read_magic_bytes(reader, 54, 5).await?;  // same offset again
if fat16.as_deref() == Some(b"FAT16") { return Ok(true); }

The fat16 read is a copy-paste artefact. Read once, match against both: if bytes.as_deref() == Some(b"FAT12") || bytes.as_deref() == Some(b"FAT16").

Fix this →

OffsetBlockReader::read_blocks constructs AlignedByteReader on every call (lib.rs:568)

let byte_reader = AlignedByteReader::new(self.inner.clone()).await?;

This runs on every read_blocks invocation. If AlignedByteReader::new does any I/O setup (reads metadata, etc.), this is a per-read overhead. If it's purely a wrapper construction, it's fine — but worth confirming or caching.

Desktop and web boot_selected_device bypass session.run() (packages/desktop/src/views/device_boot.rs:25, packages/web/src/views/device_boot.rs:34)

Both desktop and web boot_selected_device call session.prepare() then manually drive fastboot download/boot, rather than using session.run(). This means session state transitions for Downloading, BootHandoffStarted, BootIssued, and Serving aren't persisted for these callers. The plan calls this "incremental", which is reasonable — but it means the desktop/web paths don't yet benefit from the resume semantics introduced in this PR.

Async methods that don't await anything (lib.rs:1695)

request(), status(), phase(), and snapshot() are all async fn that just call self.snapshot_sync() (no actual async work). The coexistence of the _sync suffix variant with identical functionality named differently is confusing for callers. This is possibly forward-looking API shaping — if so, a comment on the async accessors would help; otherwise the _sync variant should go.

DEFAULT_SMOO_MAX_IO_BYTES duplicated

Defined identically in native.rs:34 and boot.rs:80. Could live as a public constant in fastboop-session.

plan.md at repo root

Planning docs typically live in docs/ or as PR body. Having it at the root is a bit unusual — if it's intended as living architecture docs, a docs/session-design.md home would be cleaner. If it's just a scratchpad, it can be dropped.

---

Minor

• FastboopSession is !Send due to Rc<RefCell<...>>. This is the forced consequence of no_std (no std::sync::Mutex), but callers need to keep the session on one thread. Not documented anywhere.
• NativeBootStage0Config::from_raw_ostree hardcodes impersonate_fastboot: true — if a caller wants false, they need a post-construction override.
• fastboop-environment-std::lib re-exports several types/functions from fastboop-session (e.g. BootProfileSourceOverrides, OstreeArg, format_probe_error). This is convenient but blurs crate ownership at the public API boundary.

---

Overall this is solid work — the abstractions are well-chosen, the plan is clearly followed, and the net deletion is real. The issues above are all fixable incrementally.