handle_batch: destroy correctness + provision seam + DNS write lock

docs/content/docs/inventory/_index.md

@@ -38,6 +38,33 @@ So a secrets inventory loaded second can add (or override) variables without los
 > [!NOTE]
 > Each `-i` directory must contain a `groups/` subdirectory (it may be empty), or Jetpack will refuse to load it.
 
+## Provision overlays
+
+A `provision:` block in `group_vars/<group>` deep-merges onto each member host's
+`host_vars` provision config — so you can set or override provision fields for a
+whole fleet from one file. Host-specific fields win on conflict, exactly like
+every other variable (more-specific wins). The same works ad-hoc from the CLI
+with `-e` / `--extra-vars` (highest precedence): `-e provision.state=destroyed`
+overrides every host's lifecycle state for that run, no inventory file touched.
+(Values take the convenient `key=value` form, with dotted keys for nesting; JSON
+and `@file` are still accepted for typed/structured values.)
+
+The common use is toggling a fleet's lifecycle state. Put each host's real
+provision spec in its `host_vars` (no `state`, so it defaults to `present`), then
+drive the whole group from one `group_vars` file:
+
+```yaml
+# group_vars/test-k8s
+provision:
+  state: destroyed     # tear every member down (remove this block, or use
+                       # `present`, to create them again)
+```
+
+Because the `host_vars` provision fields take precedence, only `state` is
+overridden — `type`, `cluster`, `ip`, and the rest still come from each host.
+This is how a repeatable test harness can reset and recreate a cluster by
+flipping a single file between `destroyed` and `present`.
+
 ## Inspecting inventory
 
 See exactly what an inventory resolves to before running anything:

src/api.rs

@@ -75,8 +75,12 @@ impl PlaybookRunner {
         } else {
             let inv = Arc::new(RwLock::new(Inventory::new()));
             if !self.config.inventory_paths.read().unwrap().is_empty() {
-                load_inventory(&inv, Arc::clone(&self.config.inventory_paths))
-                    .map_err(JetpackError::Inventory)?;
+                load_inventory(
+                    &inv,
+                    Arc::clone(&self.config.inventory_paths),
+                    self.config.extra_vars.clone(),
+                )
+                .map_err(JetpackError::Inventory)?;
             }
             inv
         };

src/cli/confirm.rs

@@ -9,11 +9,16 @@
 //! is skipped and the run proceeds as before.
 
 use crate::cli::parser::CliParser;
+use crate::connection::no::NoFactory;
 use crate::inventory::hosts::Host;
 use crate::inventory::inventory::Inventory;
+use crate::playbooks::context::PlaybookContext;
+use crate::playbooks::traversal::{RunState, resolve_playbook_targets};
+use crate::playbooks::visitor::{CheckMode, PlaybookVisitor};
 use crate::provisioners::ProvisionState;
+use std::collections::{HashMap, HashSet};
 use std::io::{IsTerminal, Write};
-use std::sync::{Arc, RwLock};
+use std::sync::{Arc, Mutex, RwLock};
 
 /// Mutating execution modes that can actually destroy resources. The dry-run /
 /// validation modes (plan/check-*/syntax/simulate/show/pull) never mutate, so
@@ -60,28 +65,34 @@ fn host_in_limit_scope(host: &Host, parser: &CliParser) -> bool {
 }
 
 /// Sorted list of in-scope hosts a run would destroy — those whose
-/// `provision.state` is `absent` or `destroyed` and that pass the CLI
-/// `--limit-hosts` / `--limit-groups` filters. Pure (no IO) so it is
-/// unit-testable.
+/// `provision.state` is `absent`/`destroyed`, passing the `--limit` filters, and
+/// (when known) actually targeted by the specified playbook's plays.
 ///
-/// Note: this cannot account for which groups a playbook's plays target (known
-/// only after playbook parsing, post-dispatch), so without `--limit` it may
-/// name destroy-declared hosts no play actually touches. That is a
-/// safe-direction over-approximation — it never misses a real destroy.
+/// `playbook_targets` scopes the list to the hosts the `-p` playbook resolves to,
+/// so `apply -p repro.yml` only prompts for what repro.yml touches — not every
+/// destroy-declared host in the inventory. It is `None` when no playbook was
+/// given or its groups couldn't be resolved; in that case every destroy-bearing
+/// host in `--limit` scope is listed, preserving the safe-direction
+/// over-approximation (it never misses a real destroy).
 pub fn destroy_bearing_hosts(
     inventory: &Arc<RwLock<Inventory>>,
     parser: &CliParser,
+    playbook_targets: Option<&std::collections::HashSet<String>>,
 ) -> Vec<String> {
     let inv = inventory.read().unwrap();
     let mut names: Vec<String> = inv
         .hosts
         .values()
         .filter_map(|h| {
             let h = h.read().unwrap();
-            if host_is_destroy_bearing(&h) && host_in_limit_scope(&h, parser) {
-                Some(h.name.clone())
-            } else {
-                None
+            if !host_is_destroy_bearing(&h) || !host_in_limit_scope(&h, parser) {
+                return None;
+            }
+            // Scope to the playbook's resolved targets when known; None = unknown,
+            // so keep the host (over-approximation — never under-prompt).
+            match playbook_targets {
+                Some(targets) if !targets.contains(&h.name) => None,
+                _ => Some(h.name.clone()),
             }
         })
         .collect();
@@ -103,7 +114,11 @@ pub fn confirm_destroy_if_tty(
     if !std::io::stdin().is_terminal() {
         return Ok(());
     }
-    let targets = destroy_bearing_hosts(inventory, parser);
+    // Scope the prompt to what the -p playbook actually targets (resolved play
+    // groups), not the whole inventory. None when there's no playbook or its
+    // groups can't be resolved → destroy_bearing_hosts falls back to inventory-wide.
+    let playbook_targets = resolve_playbook_targets_for_confirm(inventory, parser);
+    let targets = destroy_bearing_hosts(inventory, parser, playbook_targets.as_ref());
     if targets.is_empty() {
         return Ok(());
     }
@@ -130,6 +145,54 @@ pub fn confirm_destroy_if_tty(
     }
 }
 
+/// Resolve the `-p` playbook's target host set for the destroy prompt. Returns
+/// `None` (→ inventory-wide over-approximation) when there's no playbook or
+/// `resolve_playbook_targets` couldn't resolve every play's groups — so the
+/// prompt never silently under-counts.
+fn resolve_playbook_targets_for_confirm(
+    inventory: &Arc<RwLock<Inventory>>,
+    parser: &CliParser,
+) -> Option<HashSet<String>> {
+    if parser.playbook_paths.read().unwrap().is_empty() {
+        return None;
+    }
+    let run_state = resolution_run_state(inventory, parser);
+    match resolve_playbook_targets(&run_state) {
+        Ok(Some(set)) => Some(set),
+        Ok(None) | Err(_) => None,
+    }
+}
+
+/// A `RunState` just capable of group resolution (no real connection factory) —
+/// mirrors `playbook_syntax_check`'s build. Used only to drive
+/// `resolve_target_groups` / `get_play_hosts` for the destroy prompt.
+fn resolution_run_state(inventory: &Arc<RwLock<Inventory>>, parser: &CliParser) -> Arc<RunState> {
+    Arc::new(RunState {
+        inventory: Arc::clone(inventory),
+        playbook_paths: Arc::clone(&parser.playbook_paths),
+        role_paths: Arc::clone(&parser.role_paths),
+        module_paths: Arc::clone(&parser.module_paths),
+        limit_hosts: parser.limit_hosts.clone(),
+        limit_groups: parser.limit_groups.clone(),
+        batch_size: parser.batch_size,
+        context: Arc::new(RwLock::new(PlaybookContext::new(parser))),
+        visitor: Arc::new(RwLock::new(PlaybookVisitor::new(CheckMode::No))),
+        connection_factory: Arc::new(RwLock::new(NoFactory::new())),
+        tags: parser.tags.clone(),
+        allow_localhost_delegation: parser.allow_localhost_delegation,
+        is_pull_mode: false,
+        syntax_mode: false,
+        play_groups: parser.play_groups.clone(),
+        output_handler: None,
+        async_mode: parser.async_mode,
+        playbook_contents: Vec::new(),
+        processed_role_tasks: Arc::new(RwLock::new(std::collections::HashSet::new())),
+        processed_role_handlers: Arc::new(RwLock::new(std::collections::HashSet::new())),
+        role_processing_stack: Arc::new(RwLock::new(Vec::new())),
+        fetched_files: Arc::new(Mutex::new(HashMap::new())),
+    })
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -184,7 +247,7 @@ mod tests {
             ("db-bare", None, &["dbservers"]),
         ]);
         let parser = parser_with(&[], &[]);
-        let got = destroy_bearing_hosts(&inv, &parser);
+        let got = destroy_bearing_hosts(&inv, &parser, None);
         assert_eq!(
             got,
             vec!["web-absent".to_string(), "web-destroyed".to_string()]
@@ -198,7 +261,7 @@ mod tests {
             ("db", None, &["dbservers"]),
         ]);
         let parser = parser_with(&[], &[]);
-        assert!(destroy_bearing_hosts(&inv, &parser).is_empty());
+        assert!(destroy_bearing_hosts(&inv, &parser, None).is_empty());
     }
 
     #[test]
@@ -210,7 +273,7 @@ mod tests {
         // --limit web-destroyed only
         let parser = parser_with(&["web-destroyed"], &[]);
         assert_eq!(
-            destroy_bearing_hosts(&inv, &parser),
+            destroy_bearing_hosts(&inv, &parser, None),
             vec!["web-destroyed".to_string()]
         );
     }
@@ -224,8 +287,37 @@ mod tests {
         // --limit webservers only
         let parser = parser_with(&[], &["webservers"]);
         assert_eq!(
-            destroy_bearing_hosts(&inv, &parser),
+            destroy_bearing_hosts(&inv, &parser, None),
             vec!["web-destroyed".to_string()]
         );
     }
+
+    #[test]
+    fn playbook_targets_scope_the_destroy_prompt() {
+        let inv = inventory_with(&[
+            ("web-destroyed", Some("destroyed"), &["webservers"]),
+            ("db-destroyed", Some("destroyed"), &["dbservers"]),
+            ("web-present", Some("present"), &["webservers"]),
+        ]);
+        let parser = parser_with(&[], &[]);
+        // A -p playbook resolving to only the webservers' hosts.
+        let targets: HashSet<String> = ["web-destroyed", "web-present"]
+            .iter()
+            .map(|s| s.to_string())
+            .collect();
+        // Only the destroy-bearing host the playbook targets is prompted; the
+        // db-destroyed host (destroy-bearing, but not targeted) is excluded.
+        assert_eq!(
+            destroy_bearing_hosts(&inv, &parser, Some(&targets)),
+            vec!["web-destroyed".to_string()]
+        );
+        // None (no playbook, or unresolvable groups) → inventory-wide
+        // over-approximation: every destroy-bearing host in scope is listed.
+        let mut all = destroy_bearing_hosts(&inv, &parser, None);
+        all.sort();
+        assert_eq!(
+            all,
+            vec!["db-destroyed".to_string(), "web-destroyed".to_string()]
+        );
+    }
 }

src/cli/parser.rs

@@ -19,7 +19,7 @@
 
 include!(concat!(env!("OUT_DIR"), "/version.rs"));
 use crate::cli::config_file::{self, JetpackFileConfig};
-use crate::inventory::loading::convert_json_vars;
+use crate::inventory::loading::{convert_json_vars, extra_vars_from_key_value};
 use crate::util::io::directory_as_string;
 use crate::util::io::jet_file_open;
 use crate::util::io::read_local_file;
@@ -1574,16 +1574,15 @@ impl CliParser {
                 }
             };
             blend_variables(&mut self.extra_vars, serde_yaml::Value::Mapping(parsed));
-        } else {
+        } else if let Ok(actual) = serde_json::from_str::<serde_json::Value>(value) {
             // input is inline JSON (as YAML wouldn't make sense with the newlines)
-
-            let parsed: Result<serde_json::Value, serde_json::Error> = serde_json::from_str(value);
-            let actual = match parsed {
-                Ok(x) => x,
-                Err(y) => return Err(format!("inline json is not valid: {}", y)),
-            };
             let serde_map = convert_json_vars(&actual);
             blend_variables(&mut self.extra_vars, serde_yaml::Value::Mapping(serde_map));
+        } else {
+            // key=value, dotted key → nested: the convenient form, e.g.
+            // `-e provision.state=destroyed` (use JSON for typed values).
+            let mapping = extra_vars_from_key_value(value)?;
+            blend_variables(&mut self.extra_vars, serde_yaml::Value::Mapping(mapping));
         }
 
         Ok(())

src/cli/playbooks.rs

@@ -103,7 +103,11 @@ pub fn inventory_check(inventory: &Arc<RwLock<Inventory>>, parser: &CliParser) -
         println!("inventory-check requires --inventory");
         return 1;
     }
-    match load_inventory(inventory, Arc::clone(&parser.inventory_paths)) {
+    match load_inventory(
+        inventory,
+        Arc::clone(&parser.inventory_paths),
+        parser.extra_vars.clone(),
+    ) {
         Ok(_) => {
             let count = inventory.read().expect("inventory read").hosts.len();
             println!("inventory OK ({} hosts)", count);

src/cli/secrets_diagnostic.rs

@@ -49,6 +49,8 @@ use crate::playbooks::ref_collector::collect_per_play;
 const RENDER_BUILTINS: &[&str] = &[
     "jet_hostname",
     "jet_hostname_short",
+    "inventory_hostname",
+    "inventory_hostname_short",
     "jet_play_hosts",
     "jet_sudo_user",
     "jet_command",

src/dns/mod.rs

@@ -20,6 +20,26 @@ pub mod zone;
 use serde::Deserialize;
 use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::sync::{LazyLock, Mutex};
+
+/// Serializes DNS zone mutations across threads.
+///
+/// Parallel host provisioning calls `add_host_record` / `remove_host_record`
+/// concurrently; without this guard the zone-file read-modify-write in
+/// `zone.rs` (and the octodns/gravity sync that follows it) would race and lose
+/// updates. The record-mutating entry points below hold this lock for their
+/// entire body — zone edit plus provider sync — so a host's DNS change is
+/// atomic. Internal helpers (`add_ptr_record`, `sync`) deliberately take no
+/// lock: they are only reached through these entry points, and
+/// `std::sync::Mutex` is not reentrant.
+static DNS_WRITE_LOCK: LazyLock<Mutex<()>> = LazyLock::new(|| Mutex::new(()));
+
+/// Acquire the process-wide DNS write lock, propagating poison as a `String`.
+fn dns_write_lock() -> Result<std::sync::MutexGuard<'static, ()>, String> {
+    DNS_WRITE_LOCK
+        .lock()
+        .map_err(|e| format!("DNS write lock poisoned: {}", e))
+}
 
 /// Source of truth for IP addresses
 #[derive(Debug, Clone, Deserialize, PartialEq, Default)]
@@ -160,6 +180,7 @@ pub fn dns_config_from_vars(
 /// Add a DNS A record for a host, plus PTR if configured, and optionally sync
 /// inventory_name is the FQDN from inventory (e.g., "gravity01.island.lagun.co")
 pub fn add_host_record(config: &DnsConfig, inventory_name: &str, ip: &str) -> Result<bool, String> {
+    let _guard = dns_write_lock()?;
     let zone = config
         .zone
         .clone()
@@ -211,6 +232,7 @@ pub fn add_host_record(config: &DnsConfig, inventory_name: &str, ip: &str) -> Re
 /// Remove a DNS A record for a host and optionally sync
 /// inventory_name is the FQDN from inventory (e.g., "gravity01.island.lagun.co")
 pub fn remove_host_record(config: &DnsConfig, inventory_name: &str) -> Result<bool, String> {
+    let _guard = dns_write_lock()?;
     let zone = config
         .zone
         .clone()
@@ -252,6 +274,7 @@ pub fn set_service_records(
     service_name: &str,
     ips: &[String],
 ) -> Result<bool, String> {
+    let _guard = dns_write_lock()?;
     let changed = zone::set_a_records(&config.zones_path(), zone, service_name, ips)?;
 
     if changed && config.auto_sync {
@@ -278,6 +301,7 @@ pub fn add_cname_alias(
     alias: &str,
     target: &str,
 ) -> Result<bool, String> {
+    let _guard = dns_write_lock()?;
     let changed = zone::add_cname_record(&config.zones_path(), zone, alias, target)?;
 
     if changed && config.auto_sync {
@@ -315,6 +339,7 @@ pub fn add_ptr_record(config: &DnsConfig, ip: &str, fqdn: &str) -> Result<bool,
 
 /// Remove a record (A, CNAME, or PTR)
 pub fn remove_record(config: &DnsConfig, zone: &str, name: &str) -> Result<bool, String> {
+    let _guard = dns_write_lock()?;
     let changed = zone::remove_a_record(&config.zones_path(), zone, name)?;
 
     if changed && config.auto_sync {