fix(minimald): persist session composition across daemon restarts - #979
Merged
Conversation
📝 WalkthroughWalkthroughChangesComposition persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionActor
participant SessionRecordHandle
participant StoreActor
participant DiskLoader
SessionActor->>SessionRecordHandle: store composition before Materializing
SessionRecordHandle->>StoreActor: send composition request
StoreActor->>DiskLoader: write composition.json atomically
DiskLoader-->>StoreActor: storage result
StoreActor-->>SessionRecordHandle: return result
SessionActor->>SessionRecordHandle: load composition for Active record
SessionRecordHandle->>StoreActor: send load request
StoreActor->>DiskLoader: read composition.json
DiskLoader-->>StoreActor: composition or missing result
StoreActor-->>SessionRecordHandle: return restored composition or None
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
0chroma
force-pushed
the
fix/persist-session-composition-849
branch
3 times, most recently
from
July 27, 2026 19:34
5c7b615 to
fda5378
Compare
0chroma
marked this pull request as ready for review
July 27, 2026 20:42
The daemon held session composition (loadout + project + package contributions) only in memory. After a restart, the launcher silently fell back to the baseline package/var set, dropping all loadout contributions for existing sessions. Persist the finalized Composition as a composition.json sidecar alongside record.json at composition-assembly time. On spawn-from-disk, read the sidecar back so the launcher re-applies the exact composition approved at `min activate` time, preserving the bake-in guarantee across daemon lifecycle events. A missing or corrupt sidecar logs a warning and falls back to baseline (loud fallback) so attach still works, but the operator sees the problem instead of a silent drop. Fixes #849.
0chroma
force-pushed
the
fix/persist-session-composition-849
branch
from
July 27, 2026 20:46
fda5378 to
8522f91
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/sessions/src/core/compose.rs (2)
1244-1265: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
versionis deserialized but never validated.
WireComposition::versionis documented as gating the on-disk shape ("a future format can evolve without ambiguity"), buttry_fromignoreswire.versionentirely. If a futureCOMPOSITION_SNAPSHOT_VERSIONbump changes field semantics (not just adds new optional fields), a mismatched version would deserialize silently rather than being rejected, defeating the field's stated purpose.Consider matching on
wire.versionhere (or inDiskLoader::load_composition) and returningComposeError::InvalidWireItemfor unrecognized versions.♻️ Proposed version gate
fn try_from(wire: crate::wire::request::WireComposition) -> Result<Self, Self::Error> { + if wire.version != crate::wire::request::COMPOSITION_SNAPSHOT_VERSION { + return Err(ComposeError::InvalidWireItem { + what: "unsupported composition snapshot version", + context: format!( + "found {}, expected {}", + wire.version, + crate::wire::request::COMPOSITION_SNAPSHOT_VERSION + ), + }); + } let hooks: Vec<ProvenancedHook> = wire🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sessions/src/core/compose.rs` around lines 1244 - 1265, Validate wire.version in Composition::try_from before converting the composition fields, accepting only the current COMPOSITION_SNAPSHOT_VERSION and returning ComposeError::InvalidWireItem for unrecognized versions. Preserve the existing conversion behavior for supported versions.
1247-1257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate hook-conversion error mapping.
This
map_errclosure ("lifecycle hook with no callbacks") is identical to the one inextend_from_wire(lines 1139-1150). Extracting a shared helper (e.g.fn convert_wire_hooks(hooks: Vec<WireProvenancedHook>) -> Result<Vec<ProvenancedHook>, ComposeError>) would remove the duplication now that there are two call sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sessions/src/core/compose.rs` around lines 1247 - 1257, The WireComposition conversion in try_from duplicates the lifecycle-hook error mapping already used by extend_from_wire. Extract a shared helper for converting Vec<WireProvenancedHook> to Result<Vec<ProvenancedHook>, ComposeError>, preserving the existing "lifecycle hook with no callbacks" context, then reuse it from both try_from and extend_from_wire.crates/minimald/src/session.rs (1)
2040-2133: 🧹 Nitpick | 🔵 TrivialRun the daemon integration harness for this restart/lifecycle change.
These new tests are solid in-crate coverage for the sidecar-restore and fallback paths, but they're unit/integration-style
#[tokio::test]s, not the repository's dedicated daemon/VM harnesses.As per coding guidelines, "When changing VM or daemon paths, run the relevant integration coverage:
just e2eand/orjust test-vm" and "Do not rely only on unit tests for VM/networking behavior; preserve and run the applicable integration and root-integration harnesses."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/session.rs` around lines 2040 - 2133, Run the repository’s applicable daemon/VM integration coverage for the composition restart changes, including just e2e and/or just test-vm as appropriate, in addition to the in-crate tests composition_survives_actor_restart and missing_sidecar_falls_back_to_baseline. Record or address any failures before completing the change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/minimald/src/session.rs`:
- Around line 2040-2133: Run the repository’s applicable daemon/VM integration
coverage for the composition restart changes, including just e2e and/or just
test-vm as appropriate, in addition to the in-crate tests
composition_survives_actor_restart and missing_sidecar_falls_back_to_baseline.
Record or address any failures before completing the change.
In `@crates/sessions/src/core/compose.rs`:
- Around line 1244-1265: Validate wire.version in Composition::try_from before
converting the composition fields, accepting only the current
COMPOSITION_SNAPSHOT_VERSION and returning ComposeError::InvalidWireItem for
unrecognized versions. Preserve the existing conversion behavior for supported
versions.
- Around line 1247-1257: The WireComposition conversion in try_from duplicates
the lifecycle-hook error mapping already used by extend_from_wire. Extract a
shared helper for converting Vec<WireProvenancedHook> to
Result<Vec<ProvenancedHook>, ComposeError>, preserving the existing "lifecycle
hook with no callbacks" context, then reuse it from both try_from and
extend_from_wire.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bf65520-5e55-4d01-8756-aae232763d89
📒 Files selected for processing (6)
crates/minimald/src/session.rscrates/minimald/src/store.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/src/core/compose.rscrates/sessions/src/store.rscrates/sessions/src/wire/request.rs
twitchyliquid64
approved these changes
Jul 27, 2026
0chroma
enabled auto-merge (squash)
July 27, 2026 20:52
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sessions/src/core/compose.rs`:
- Around line 1247-1263: Update the WireComposition conversion in try_from to
validate wire.version before converting any fields, accepting only the currently
supported snapshot version and returning the appropriate ComposeError for
unsupported versions so callers can fall back instead of interpreting them as
v1.
In `@crates/sessions/src/store.rs`:
- Around line 161-174: The store_composition implementation must sync the
temporary composition sidecar file to durable storage before renaming it into
place, matching the crash-safety behavior of write_record. Update the temp-file
write flow behind the store_composition trait method to flush and sync the file,
then perform the existing atomic rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1198d1ae-89f1-4a2e-9ae2-7564ab57c844
📒 Files selected for processing (6)
crates/minimald/src/session.rscrates/minimald/src/store.rscrates/sessions/docs/COMPOSITION.mdcrates/sessions/src/core/compose.rscrates/sessions/src/store.rscrates/sessions/src/wire/request.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/minimald/src/store.rs
- crates/sessions/docs/COMPOSITION.md
- crates/sessions/src/wire/request.rs
- crates/minimald/src/session.rs
Comment on lines
+1247
to
+1263
| fn try_from(wire: crate::wire::request::WireComposition) -> Result<Self, Self::Error> { | ||
| let hooks: Vec<ProvenancedHook> = wire | ||
| .lifecycle_hooks | ||
| .into_iter() | ||
| .map(|h| { | ||
| h.try_into().map_err(|e| ComposeError::InvalidWireItem { | ||
| what: "lifecycle hook with no callbacks", | ||
| context: format!("{e}"), | ||
| }) | ||
| }) | ||
| .collect::<Result<_, _>>()?; | ||
| Ok(Self { | ||
| vars: wire.vars.into_iter().map(Into::into).collect(), | ||
| patches: wire.patches.into_iter().map(Into::into).collect(), | ||
| packages: wire.packages.into_iter().map(Into::into).collect(), | ||
| lifecycle_hooks: hooks, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsupported snapshot versions.
WireComposition.version is never checked, so an older daemon will silently interpret a future snapshot using v1 semantics rather than treating it as corrupt and falling back. That can restore the wrong loadout after restart.
Proposed fix
fn try_from(wire: crate::wire::request::WireComposition) -> Result<Self, Self::Error> {
+ if wire.version != crate::wire::request::COMPOSITION_SNAPSHOT_VERSION {
+ return Err(ComposeError::InvalidWireItem {
+ what: "unsupported composition snapshot version",
+ context: wire.version.to_string(),
+ });
+ }
+
let hooks: Vec<ProvenancedHook> = wire📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn try_from(wire: crate::wire::request::WireComposition) -> Result<Self, Self::Error> { | |
| let hooks: Vec<ProvenancedHook> = wire | |
| .lifecycle_hooks | |
| .into_iter() | |
| .map(|h| { | |
| h.try_into().map_err(|e| ComposeError::InvalidWireItem { | |
| what: "lifecycle hook with no callbacks", | |
| context: format!("{e}"), | |
| }) | |
| }) | |
| .collect::<Result<_, _>>()?; | |
| Ok(Self { | |
| vars: wire.vars.into_iter().map(Into::into).collect(), | |
| patches: wire.patches.into_iter().map(Into::into).collect(), | |
| packages: wire.packages.into_iter().map(Into::into).collect(), | |
| lifecycle_hooks: hooks, | |
| }) | |
| fn try_from(wire: crate::wire::request::WireComposition) -> Result<Self, Self::Error> { | |
| if wire.version != crate::wire::request::COMPOSITION_SNAPSHOT_VERSION { | |
| return Err(ComposeError::InvalidWireItem { | |
| what: "unsupported composition snapshot version", | |
| context: wire.version.to_string(), | |
| }); | |
| } | |
| let hooks: Vec<ProvenancedHook> = wire | |
| .lifecycle_hooks | |
| .into_iter() | |
| .map(|h| { | |
| h.try_into().map_err(|e| ComposeError::InvalidWireItem { | |
| what: "lifecycle hook with no callbacks", | |
| context: format!("{e}"), | |
| }) | |
| }) | |
| .collect::<Result<_, _>>()?; | |
| Ok(Self { | |
| vars: wire.vars.into_iter().map(Into::into).collect(), | |
| patches: wire.patches.into_iter().map(Into::into).collect(), | |
| packages: wire.packages.into_iter().map(Into::into).collect(), | |
| lifecycle_hooks: hooks, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sessions/src/core/compose.rs` around lines 1247 - 1263, Update the
WireComposition conversion in try_from to validate wire.version before
converting any fields, accepting only the currently supported snapshot version
and returning the appropriate ComposeError for unsupported versions so callers
can fall back instead of interpreting them as v1.
Comment on lines
+161
to
+174
| /// Atomically persists the composition snapshot for the session | ||
| /// at `key` (tmp + rename, same crash-safety as [`Self::save`]). | ||
| /// Called at composition-assembly time so a restart can re-apply | ||
| /// the exact composition that was approved at `min activate` time. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// - `NotFound` if `key` is stale (same semantics as [`Self::save`]). | ||
| /// - I/O or serialization errors if the sidecar cannot be written. | ||
| fn store_composition( | ||
| &self, | ||
| key: &Self::Key, | ||
| composition: &crate::core::compose::Composition, | ||
| ) -> Result<(), std::io::Error>; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Sync the sidecar before publishing it.
Unlike write_record, this path renames an unsynced temp file. A power loss can durably retain the Active record while losing composition.json, causing the restart path to fall back to baseline and lose approved contributions.
Proposed fix
let json = serde_json::to_vec_pretty(&wire)
.map_err(|e| std::io::Error::other(format!("serializing composition snapshot: {e}")))?;
std::fs::write(&tmp, &json)?;
+std::fs::OpenOptions::new()
+ .write(true)
+ .open(&tmp)?
+ .sync_all()?;
#[cfg(target_os = "linux")]
common::renameat2::renameat2_cwd(tmp.as_std_path(), dest.as_std_path(), 0)?;Also applies to: 956-964
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sessions/src/store.rs` around lines 161 - 174, The store_composition
implementation must sync the temporary composition sidecar file to durable
storage before renaming it into place, matching the crash-safety behavior of
write_record. Update the temp-file write flow behind the store_composition trait
method to flush and sync the file, then perform the existing atomic rename.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Compositionas acomposition.jsonsidecar alongsiderecord.jsonat composition-assembly time. On spawn-from-disk, the daemon reads the sidecar back. The launcher re-applies the exact composition approved atmin activatetime. This preserves the bake-in guarantee across daemon lifecycle events.SessionRecordHandle, mirroring how the record itself is read and written. TheLoadertrait gainsload_compositionandstore_composition, so the store owns the serialization, not the session actor.Fixes #849.
Approach
Investigated two strategies (see #849 for the full analysis):
Compositionto a sidecar via the existing wire types, which already have serde and bidirectional conversions. It preserves the bake-in guarantee exactly: client-sideInheritvars keep their already-resolved values, so no re-resolution is needed. No re-gating of user policy. No patch or home drift.attachintoactivate. It creates patch and home drift.Approach 3 (loud baseline fallback) ships alongside. The
load_compositionhelper on the session actor logs awarn!on missing or corrupt files, instead of silently falling back.Changes
sessions/src/store.rsload_compositionandstore_compositionon theLoadertrait.DiskLoaderimplements them with the same atomic tmp and rename pattern aswrite_record.sessions/src/wire/request.rsWireCompositionenvelope (serde, versioned) plusFrom<&Composition>and 2 round-trip testssessions/src/core/compose.rsTryFrom<WireComposition> for Compositionreconstructs from the persisted snapshotminimald/src/store.rsCompositionLoadandCompositionStoremessages on the Store actor.load_compositionandstore_compositiononSessionRecordHandle.minimald/src/session.rsload_compositionhelper at spawn-from-disk (loud fallback).store_compositioncalls at both assembly sites. 2 restart tests.sessions/docs/COMPOSITION.mdVerification
cargo test -p sessions -p minimald: all tests pass (4 new)cargo clippy -p sessions -p minimald --all-targets -- -D warnings: cleancargo fmt --all -- --check: cleancargo check -p minvmd --tests: compiles (no broken downstream)New tests
composition_survives_actor_restart: stops a session actor, evicts it, then re-resolves (spawning from disk). Asserts the sidecar restores the composition.missing_sidecar_falls_back_to_baseline: deletes the sidecar, restarts the actor. Asserts the composition isNone(baseline fallback).composition_snapshot_round_tripsandcomposition_snapshot_defaults_version_to_one: wire-level serde round-trip and version default.Notes
record.json,index.json).Materializing. A crash leaves either a reaped session or an Active session with its sidecar intact. The daemon reaps Pending or Materializing records at startup.WireCompositioncarries aversionfield (defaulting to 1) so the on-disk shape can evolve without ambiguity.Note
Persist session composition across daemon restarts in
minimaldWireCompositionwire format with versioning to serialize/deserializeCompositionsnapshots to/fromcomposition.jsonper session.Loadertrait andDiskLoaderwithload_composition/store_composition, writing atomically via temp-file-and-rename.minimaldsession actor now stores the composition at assembly time and restores it on restart; missing or corrupt files log a warning and fall back toNone(baseline).load_composition/store_compositiontoSessionRecordHandle, wired through newStoreMessagevariants.store_compositionpropagate asio::Errorand will surface at composition assembly time.Macroscope summarized 8522f91.
Summary by CodeRabbit