From cac4a4d65a410112e978f9228a367d6a2dbcece6 Mon Sep 17 00:00:00 2001
From: atlas <atlas@pr1ma.darkest.space>
Date: Sun, 21 Jun 2026 21:10:00 +0200
Subject: [PATCH] hive-c0re: polish subvol-upgrade error paths (post-merge
 review follow-up)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Two non-blocking points from the subvol-upgrade review:

- The start request was `?`-propagated before the migration result was
  surfaced, so a restart-side failure (incl. the IPC call itself
  erroring) could shadow whether the migration succeeded or failed.
  Capture the start result instead and surface the migration outcome
  first; the restart-failure messages now point at `hivectl start
  --agent <name>` for manual recovery.

- The crash-window case (host dies between the two swap renames, leaving
  the agent root missing but the original data under `.<name>.old`) now
  detects the leftover and tells the operator to `mv` it back, instead
  of a bare "no state dir to upgrade — nothing to do".
---
 hive-c0re/src/bin/hivectl.rs | 29 ++++++++++++++++++++++-------
 hive-priv/src/main.rs        | 15 +++++++++++++++
 2 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/hive-c0re/src/bin/hivectl.rs b/hive-c0re/src/bin/hivectl.rs
index 2f278c60..17d328c7 100644
--- a/hive-c0re/src/bin/hivectl.rs
+++ b/hive-c0re/src/bin/hivectl.rs
@@ -1339,23 +1339,38 @@ async fn subvol_upgrade(socket: &Path, name: &str, yes: bool) -> Result<()> {
     println!("migrating {name} state dir to a btrfs subvolume…");
     let upgrade = hive_c0re::priv_client::upgrade_agent_subvolume(name).await;
 
-    // Always restart, even if the migration failed — don't leave the agent down.
+    // Always attempt the restart, even if the migration failed — don't leave
+    // the agent down. Capture the result rather than `?`-ing it so a
+    // start-side failure (incl. the IPC call itself erroring) can't mask the
+    // migration outcome below.
     println!("starting {name}…");
-    let start_resp = hive_c0re::client::request(
+    let start_result = hive_c0re::client::request(
         socket,
         hive_sh4re::HostRequest::Start {
             scope: single_agent_scope(name),
         },
     )
-    .await
-    .with_context(|| format!("connect to daemon socket {}", socket.display()))?;
+    .await;
 
-    // Surface the migration error first — it's the meaningful one — but only
-    // after the restart attempt above.
+    // Surface the migration outcome FIRST — it's the meaningful result and
+    // must not be shadowed by a restart-side failure. On migration failure the
+    // original state dir is untouched (the priv op rolls back before the swap).
     upgrade.with_context(|| format!("upgrade {name} state subvolume"))?;
+
+    // Migration succeeded; now surface any restart problem — either the IPC
+    // call erroring, or the daemon reporting a failed start. The migration is
+    // done regardless, so point at the manual recovery.
+    let start_resp = start_result.with_context(|| {
+        format!(
+            "{name} migrated to a btrfs subvolume, but the restart request to the daemon \
+             socket {} failed — run `hivectl start --agent {name}` to bring it back up",
+            socket.display()
+        )
+    })?;
     if !start_resp.ok {
         bail!(
-            "migration succeeded but restarting {name} failed: {}",
+            "{name} migrated to a btrfs subvolume, but restarting it failed: {} — run \
+             `hivectl start --agent {name}` to retry",
             start_resp.error.as_deref().unwrap_or("unknown error")
         );
     }
diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs
index 53403394..b23085f9 100644
--- a/hive-priv/src/main.rs
+++ b/hive-priv/src/main.rs
@@ -797,6 +797,21 @@ async fn upgrade_agent_subvolume(agent_name: &str) -> Result<(String, String)> {
     let agent_root = root.join(agent_name);
 
     if !agent_root.exists() {
+        // A crash between the two swap renames (original → `.<name>.old`
+        // succeeded, `.<name>.migrating` → agent_root did not) leaves the
+        // agent root missing but the original data intact under `.<name>.old`.
+        // Point at the recovery rather than a bare "nothing to do" so the
+        // operator isn't left guessing where the data went.
+        let old = root.join(format!(".{agent_name}.old"));
+        if old.exists() {
+            bail!(
+                "no state dir at {agent} — but {old} holds the original data from an \
+                 interrupted upgrade (host crashed mid-swap). Restore it with \
+                 `mv {old} {agent}`, then re-run the upgrade.",
+                agent = agent_root.display(),
+                old = old.display(),
+            );
+        }
         bail!(
             "no state dir to upgrade at {} — nothing to do",
             agent_root.display()