Skip to content

Commit 343c349

Browse files
committed
fix(down): teardown names its catalog instead of inferring one
`st2 down` with no target resolved the shared catalog selection — `$CATALOG`, else `${XDG_STATE_HOME:-$HOME/.local/state}/st2/default/catalog` — then discovered every spec under it and killed every live task. Inheriting a catalog is right for the read-only verbs, and for `up`: it is additive, and it refuses to start at all while another supervisor holds the host lock. `down` is the only thing that ends tasks, which is what makes explicit teardown a lifecycle guarantee (R11) rather than a habit, and it holds no lock, has no dry-run, and cannot be undone. Neither inherited source is a choice made at the call site. `PtyCli` exports `CATALOG` into every task st2 launches, so inside a seat the ambient value names that seat's own fleet; and the standard default is derived from `$HOME`, so it can never be absent. Between them, a forgotten argument always resolves to some live fleet instead of failing — the same defect class as a `PTY_ROOT` that has to be exported into every reader. So teardown now takes its target from argv alone and otherwise refuses, printing the catalog it would have used and the explicit command to run. The refusal is in argv rather than a prompt because the callers that most need it — managed tasks and scripts — have no tty to confirm on. Every invocation in the README and the test suite already names its target, so none of them change.
1 parent 15e8527 commit 343c349

4 files changed

Lines changed: 118 additions & 6 deletions

File tree

README.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,8 @@ ${XDG_STATE_HOME:-$HOME/.local/state}/st2/default/catalog
4040
```
4141

4242
Every catalog-aware command accepts `--catalog`; otherwise st2 uses `$CATALOG`, then that standard
43-
location.
43+
location. `st2 down` is the exception: teardown is the only thing that ends tasks, so it takes its
44+
target from `--catalog` or a path argument and refuses to infer one.
4445

4546
### A catalog may declare its session registry
4647

@@ -283,12 +284,18 @@ the correctness fallback.
283284

284285
## Cleanup
285286

286-
Explicit teardown is the only operation that ends declared tasks:
287+
Explicit teardown is the only operation that ends declared tasks, and it never infers which catalog
288+
to end:
287289

288290
```sh
289291
st2 down --catalog "$CATALOG" --host <host>
290292
```
291293

294+
Without `--catalog` or a path argument, `st2 down` refuses and prints the catalog it would have used.
295+
`$CATALOG` is set inside every managed task, and the standard default is derived from `$HOME`, so an
296+
inferred target would always exist — a forgotten argument would read as "tear down this host's
297+
fleet".
298+
292299
On Linux, remove the supervisor service after teardown when it is no longer wanted:
293300

294301
```sh

docs/vrs/spec.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,12 @@ validate ──► materialize ──► host-local st2 scheduler/reconciler
5959
binary, starts the control plane again, and proves adoption with the same
6060
agent PID/creation identity and no duplicate process.
6161

62+
Teardown, the one action that ends tasks, resolves its catalog from the
63+
command line alone. Every other verb may inherit `$CATALOG` or fall back to
64+
the standard catalog; both are ambient — st2 exports `CATALOG` into every task
65+
it launches, and the standard default is derived from `$HOME` — so an inferred
66+
teardown target always exists and a missing argument would end a live fleet.
67+
6268
- **Session registry:** A catalog owns the `pty` registry holding its tasks.
6369
`<catalog>/pty` is the default; a catalog may declare another so that one host
6470
can share a single registry across catalogs. Resolution is an exported

src/main.rs

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -148,10 +148,11 @@ enum Command {
148148
extra_arg: Vec<String>,
149149
},
150150
/// Explicit teardown: kill every live task of this host's catalog agents. The ONLY thing that ends
151-
/// tasks (stopping/crashing st2 never does). Idempotent.
151+
/// tasks (stopping/crashing st2 never does). Idempotent. Its target must be named — unlike every
152+
/// other verb, it does not fall back to $CATALOG or the standard catalog.
152153
Down {
153-
/// Optional positional catalog/spec path. Prefer --catalog; defaults to $CATALOG, then the
154-
/// standard st2 catalog.
154+
/// Positional catalog/spec path to tear down. Required unless --catalog is passed; teardown
155+
/// never infers its target from $CATALOG or the standard st2 catalog.
155156
#[arg(conflicts_with = "catalog_path")]
156157
root: Option<PathBuf>,
157158
/// Host to tear down. Defaults to the local hostname.
@@ -562,7 +563,7 @@ fn main() -> Result<()> {
562563
)
563564
}
564565
Command::Down { root, host } => {
565-
let root = catalog_arg(root)?;
566+
let root = down_target(root, catalog_path.as_deref())?;
566567
down_cmd(&root, host)
567568
}
568569
Command::Env { root } => {
@@ -828,6 +829,32 @@ fn initialize_catalog_env(explicit: Option<&Path>) -> Result<()> {
828829
Ok(())
829830
}
830831

832+
/// Teardown resolves its target from argv alone — `--catalog <path>` or the positional path — and
833+
/// never from `$CATALOG` or the standard default.
834+
///
835+
/// Inheriting a catalog is right for the read-only verbs, and for `up`, which is additive and refuses
836+
/// to start while another supervisor holds the host lock. `st2 down` is the only thing that ends
837+
/// tasks, which is what makes "explicit teardown" a lifecycle guarantee (R11) rather than a habit —
838+
/// and neither inherited source is a choice the operator made *here*: st2 exports `CATALOG` into
839+
/// every task it launches, and the standard default is derived from `$HOME`, so it can never be
840+
/// absent. Inferring one would make a forgotten argument indistinguishable from "tear down this
841+
/// host's fleet", with no dry-run, no confirmation, and nothing to undo it.
842+
fn down_target(positional: Option<PathBuf>, flag: Option<&Path>) -> Result<PathBuf> {
843+
// clap makes these mutually exclusive.
844+
if let Some(path) = positional.as_deref().or(flag) {
845+
return absolute_catalog_path(path);
846+
}
847+
let inferred = catalog_root_for_env()
848+
.map(|root| root.display().to_string())
849+
.unwrap_or_else(|_| "<none>".to_string());
850+
anyhow::bail!(
851+
"refusing to tear down an inferred catalog.\n\
852+
`st2 down` is the only thing that ends tasks, so its target is named, never inherited.\n\
853+
It would have used {inferred} (from $CATALOG, else the standard default catalog).\n\
854+
Name it if that is what you mean: st2 down --catalog {inferred}"
855+
)
856+
}
857+
831858
/// Resolve an optional legacy positional path, otherwise use the shared catalog selection.
832859
fn catalog_arg(explicit: Option<PathBuf>) -> Result<PathBuf> {
833860
match explicit {

tests/catalog_selection.rs

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
//! Shared catalog selection across the CLI: explicit global `--catalog`, inherited `$CATALOG`, then
22
//! `${XDG_STATE_HOME:-$HOME/.local/state}/st2/default/catalog`. Legacy positional/`--root` forms
33
//! remain covered by the command-specific integration suites.
4+
//!
5+
//! `down` is the one deliberate exception: teardown names its target or refuses.
46
57
use std::fs;
68
use std::path::Path;
@@ -107,6 +109,76 @@ fn global_catalog_flag_overrides_the_environment_from_a_subcommand() {
107109
assert_eq!(rows[0]["identity"], "h.selected-seat");
108110
}
109111

112+
/// Teardown is the only verb that ends tasks, so it does not inherit its target from the shared
113+
/// selection: both fallbacks are ambient (st2 exports `CATALOG` into every task it launches, and the
114+
/// standard default is derived from `$HOME`), which would make a forgotten argument resolve to a live
115+
/// fleet instead of failing. Hermetic: `HOME`/`XDG_STATE_HOME` are temp dirs and the bus env vars are
116+
/// removed, so a regression here kills a temp catalog's (empty) session set, never a real one.
117+
#[test]
118+
fn down_refuses_an_inferred_target_but_takes_a_named_one() {
119+
let home = tempfile::tempdir().unwrap();
120+
let state = tempfile::tempdir().unwrap();
121+
let ambient = tempfile::tempdir().unwrap();
122+
write_agent(&state.path().join("st2/default/catalog"), "h", "default-seat");
123+
write_agent(ambient.path(), "h", "ambient-seat");
124+
125+
let down = |catalog_env: Option<&Path>, extra: &[&str]| -> Output {
126+
let mut cmd = Command::new(env!("CARGO_BIN_EXE_st2"));
127+
cmd.args(["down", "--host", "h"])
128+
.args(extra)
129+
.env("HOME", home.path())
130+
.env("XDG_STATE_HOME", state.path())
131+
.env_remove("ST_ROOT")
132+
.env_remove("PTY_ROOT");
133+
match catalog_env {
134+
Some(path) => cmd.env("CATALOG", path),
135+
None => cmd.env_remove("CATALOG"),
136+
};
137+
cmd.output().unwrap()
138+
};
139+
140+
// The standard default catalog is not a teardown target…
141+
let out = down(None, &[]);
142+
let stderr = String::from_utf8_lossy(&out.stderr);
143+
assert!(
144+
!out.status.success(),
145+
"down tore down an inferred catalog:\n{}",
146+
String::from_utf8_lossy(&out.stdout)
147+
);
148+
assert!(
149+
stderr.contains("refusing to tear down an inferred catalog"),
150+
"no refusal:\n{stderr}"
151+
);
152+
assert!(
153+
stderr.contains(
154+
state
155+
.path()
156+
.join("st2/default/catalog")
157+
.to_str()
158+
.unwrap()
159+
),
160+
"the refusal must name what it would have torn down:\n{stderr}"
161+
);
162+
163+
// …and neither is an inherited `$CATALOG`, which st2 itself exports into every managed task.
164+
let out = down(Some(ambient.path()), &[]);
165+
let stderr = String::from_utf8_lossy(&out.stderr);
166+
assert!(!out.status.success(), "down tore down an inherited $CATALOG");
167+
assert!(
168+
stderr.contains(ambient.path().to_str().unwrap()),
169+
"the refusal must name the inherited catalog:\n{stderr}"
170+
);
171+
172+
// A named target is accepted — from argv, over an inherited $CATALOG pointing elsewhere.
173+
let named = tempfile::tempdir().unwrap();
174+
let out = down(Some(ambient.path()), &["--catalog", named.path().to_str().unwrap()]);
175+
let stderr = String::from_utf8_lossy(&out.stderr);
176+
assert!(
177+
!stderr.contains("refusing to tear down"),
178+
"a named target was refused:\n{stderr}"
179+
);
180+
}
181+
110182
#[test]
111183
fn compile_agent_can_target_only_the_global_catalog_flag() {
112184
let tmp = tempfile::tempdir().unwrap();

0 commit comments

Comments
 (0)