Skip to content

Commit 04cd79c

Browse files
fix(validate): judge the socket bound against the runtime catalog
#442 added a socket-path guard that read its bound off whatever tree was being validated. Publication does not validate the tree that will run: `st2 agent publish` admits a candidate through a disposable projection nested in the catalog's control directory, and validation canonicalizes that root, so every identity was charged for the projection's own depth. Declarations whose real socket is bindable were refused, and CI showed it on an 11-byte task id: agents/host/worker/agent.kdl [socket-path-too-long]: task 'host.worker' would bind a 108-byte session socket at .../catalog/.st2/catalog-admission-okX01u/pty/host.worker.sock On a host whose catalog root is 49 bytes the projection leaves a 15-byte identity budget, so once authoring resumed this would have failed closed on essentially every managed declaration. A fail-closed guard on the wrong measurement is worse than the defect it replaced. The bound is a host-local runtime fact, so it now travels with the caller that knows where the supervisor will bind sockets. `RuntimeRoot` makes that explicit in the validator's contract: `Catalog(root)` names the catalog that will run, and `Unknown` is the deliberately context-free mode that omits the host-local guard. Publication supplies the real catalog while validating the projection; catalog transactions supply it while validating captures, stages and admission projections; direct validation of a live catalog supplies itself. Two publish-path regression tests cover the boundary the original tests missed. Both pin `PTY_ROOT` off, because an ambient pty root wins over the catalog-relative default and makes the guard unobservable — which is why this regression was visible in CI and invisible locally. Refs #432
1 parent 5f9d9f0 commit 04cd79c

8 files changed

Lines changed: 247 additions & 34 deletions

File tree

INVARIANTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,6 @@ materialization, messaging, DING, or presence must preserve them.
4242
| **Atomic resource proposal publication** | Every changed resource publication is one host-owned compare-and-swap fenced by binding generation, state revision, and prior carrier digest. A persistent cross-process lock admits at most one proposal from the same prior. The content-derived proposal ID binds the accepted carrier digest and semantic outbox envelope; the durable intent becomes eligible only with the exact canonical carrier, then folds into one authoritative catch-up state. A pre-carrier crash exposes old state, a post-carrier crash catches up on restart, and retry after a lost acknowledgement returns the durable receipt without another transition. Ordinary reconciliation fails closed on out-of-band divergence after a committed intent; only an explicit generation-advance recovery may re-adopt the canonical carrier or its absence while invalidating the old intent and fence. | `src/resource_profile.rs::tests::atomic_publication_fences_races_and_survives_crash_restarts`; `src/resource_profile.rs::tests::generation_advance_explicitly_recovers_diverged_or_missing_carrier` |
4343
| **Pass progress is independent of publication** | A reconcile pass completes whether or not resync publication is making progress. Publication runs on its own thread, so a pass's per-seat watch-set handshakes never wait for a catalog lock, a stream lock, or a refused publication. One publication is outstanding per subscription at a time, and a queued publication is dropped when its subscription is deactivated or removed by a refresh, so nothing is published to a seat the pass has already decided receives no events. | `src/run/tests.rs::reconcile_pass_completes_while_a_resync_publication_is_blocked`; `src/resync/tests.rs::a_flush_never_hands_off_a_subscription_whose_publication_is_outstanding`; `src/resync/tests.rs::deactivation_drops_only_that_recipients_queued_publication`; `src/resync/tests.rs::a_refresh_drops_a_queued_publication_for_a_subscription_it_removed` |
4444
| **Terminal refusals are classified, not retried** | A refused resync publication is classified by what could admit it later. A recipient that is declared but not running parks its reservation: it is attempted once, captures and schedules nothing while parked, and re-arms with its exact reserved bytes when a refresh carries that recipient again. An ambiguous recipient, a foreign owning host, and an undeclared stream are permanently refused: the reservation is dropped and the carrier baseline advances so the same transition is not re-captured. Everything else, including an absent declaration, stays retryable. | `src/resync/tests.rs::a_not_running_recipient_parks_its_reservation_and_is_attempted_once`; `src/resync/tests.rs::a_parked_reservation_re_arms_and_replays_when_its_recipient_runs_again`; `src/resync/tests.rs::a_permanently_refused_reservation_is_dropped_and_not_recaptured`; `src/resync/tests.rs::refusals_are_classified_by_what_could_admit_them_later` |
45-
| **Unbindable session sockets fail at admission** | Host-scoped validation rejects a pty task whose session socket path would exceed the portable 104-byte `sun_path` bound, because `pty` refuses that bind and the task can then never spawn. The bound is derived from the pty root resolved for the selected host rather than a fixed identity length, only that host's pty tasks are judged against it, and the diagnostic names the resolved path and the byte overage. A park whose cause is that same unbindable path does not advise `st2 unpark`, which would relaunch into the identical failure. | `tests/validate.rs::an_unbindable_session_socket_path_is_rejected_at_admission`; `tests/validate.rs::another_hosts_long_identity_is_not_judged_against_this_hosts_pty_root`; `tests/validate.rs::a_long_exec_task_id_is_not_a_socket_path_issue`; `src/run/tests.rs::session_socket_overage_is_derived_from_the_resolved_root`; `tests/run.rs::a_structurally_unrecoverable_park_does_not_advise_unpark` |
45+
| **Unbindable session sockets fail at admission** | Host-scoped validation rejects a pty task whose session socket path would exceed the portable 104-byte `sun_path` bound, because `pty` refuses that bind and the task can then never spawn. The bound comes from the pty root resolved for the RUNTIME catalog the caller names, never from the tree under inspection: publication admits a candidate through a disposable projection nested in the catalog, and measuring that tree charges every identity for the projection's own depth and refuses declarations whose real socket is bindable. Only a deliberately context-free validation omits the guard. It is not a fixed identity length either, only that host's pty tasks are judged, and the diagnostic names the resolved path and the byte overage. A park whose cause is that same unbindable path does not advise `st2 unpark`, which would relaunch into the identical failure. | `tests/agent_publish.rs::publication_judges_the_socket_bound_against_the_runtime_catalog_not_the_projection`; `tests/agent_publish.rs::publication_still_refuses_an_unbindable_canonical_socket_path`; `tests/validate.rs::an_unbindable_session_socket_path_is_rejected_at_admission`; `tests/validate.rs::another_hosts_long_identity_is_not_judged_against_this_hosts_pty_root`; `tests/validate.rs::a_long_exec_task_id_is_not_a_socket_path_issue`; `src/run/tests.rs::session_socket_overage_is_derived_from_the_resolved_root`; `tests/run.rs::a_structurally_unrecoverable_park_does_not_advise_unpark` |
4646
| **Marker-matched declaration authority** | Every st2 write path that rewrites a declaration carrying `meta { managed-by "nix" }` admits it only from a caller asserting exactly the marker that declaration carries: the lifecycle verb and the compare-and-swap `agent publish` both refuse a mismatched marker, an unmarked subject, an unresolvable multi-marker declaration, and an empty or padded assertion before any write, while presentation, address, stream, and Resource authoring refuse a Nix-owned declaration outright. Publication reads that authority from the incumbent, so creating a declaration and republishing byte-identical bytes stay open — neither replaces bytes another writer owns. A marker-matched edit rewrites nothing but the lifecycle line and is admitted through the same full-catalog gate as a compare-and-swap publication of the same bytes, so it cannot commit a declaration the catalog would reject. | `src/agent_author.rs::marker_matched_lifecycle_authority_is_exact_and_source_preserving`; `src/agent_author.rs::marker_matched_retirement_refuses_a_candidate_admission_would_reject`; `tests/agent_desired_state.rs::cli_managed_by_authority_retires_a_projected_seat_and_refuses_every_inexact_claim`; `tests/agent_publish.rs::publication_honours_the_incumbent_ownership_marker`; `tests/agent_publish.rs::publication_refuses_an_unresolvable_owner_and_repairs_unreadable_bytes` |
4747
| **Proof references resolve** | Every qualified test named in this table exists in its named source file, so stale invariant claims fail the suite instead of silently surviving a refactor. | `tests/invariants.rs::qualified_proof_references_resolve` |

docs/vrs/spec.md

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -418,12 +418,20 @@ Host-scoped validation rejects a pty task whose session socket path would exceed
418418
portable `sun_path` bound. `pty` binds `<PTY_ROOT>/<session-id>.sock` and refuses a
419419
bind over the limit, so such a task can never spawn and fails identically on every
420420
reconcile pass, which also makes the pass result useless as a health signal for that
421-
host. The bound is derived from the pty root resolved for the selected host, never a
422-
fixed maximum identity length: the usable identity length is what remains of the limit
423-
after that root. The portable 104-byte bound applies so a declaration admitted on Linux
424-
does not fail on Darwin, and the diagnostic states the resolved path and the byte
425-
overage so the author can shorten the identity rather than discover the failure as a
426-
spawn error later.
421+
host. The bound is derived from the pty root resolved for the RUNTIME catalog — the
422+
one the supervisor will bind sockets from — and never from the tree under inspection,
423+
which is frequently a different tree: publication admits a candidate through a
424+
disposable projection nested in the catalog, transactions validate captures and stages,
425+
and a retained catalog is addressed through a file-descriptor path that canonicalizes
426+
back to the real one. A caller that knows where sockets will be bound supplies that
427+
root; only a deliberately context-free validation omits the guard. Measuring the
428+
inspected tree instead charges every identity for the depth of whichever temporary tree
429+
is being validated and refuses declarations whose real socket is bindable. The bound is
430+
never a fixed maximum identity length either: the usable identity length is what
431+
remains of the limit after the runtime root. The portable 104-byte bound applies so a
432+
declaration admitted on Linux does not fail on Darwin, and the diagnostic states the
433+
resolved path and the byte overage so the author can shorten the identity rather than
434+
discover the failure as a spawn error later.
427435

428436
A park notice whose cause is structurally unrecoverable says so instead of offering
429437
`st2 unpark`, which would relaunch into the identical failure. The test is the same

src/agent_author/declared_field.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,14 @@ fn refuse_address_collision(
246246
spec.address = requested.map(str::to_owned);
247247
}
248248
}
249-
let report = crate::validate::validate_discovered(catalog_root, Some(this_host), &prospective);
249+
// The prospective catalog is an in-memory edit of the live one, which is where sockets
250+
// will be bound, so the live root is the runtime root.
251+
let report = crate::validate::validate_discovered(
252+
catalog_root,
253+
Some(this_host),
254+
crate::validate::RuntimeRoot::Catalog(catalog_root),
255+
&prospective,
256+
);
250257
if !report
251258
.issues
252259
.iter()

src/agent_publish.rs

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -494,8 +494,12 @@ fn verify_published_spec(
494494
observed_sha256 == expected_sha256 && observed == expected_bytes,
495495
"published Agent Spec readback mismatch: expected sha256 {expected_sha256}, found {observed_sha256}"
496496
);
497-
crate::catalog_transaction::validate_full_catalog(catalog)
498-
.context("published catalog fails locked core/catalog re-admission")?;
497+
// The live catalog is validated in place, so it is its own runtime root.
498+
crate::catalog_transaction::validate_full_catalog(
499+
catalog,
500+
crate::validate::RuntimeRoot::Catalog(catalog),
501+
)
502+
.context("published catalog fails locked core/catalog re-admission")?;
499503
Ok(observed_sha256)
500504
}
501505

@@ -576,8 +580,13 @@ fn read_regular_optional(path: &Path) -> Result<Option<Vec<u8>>> {
576580

577581
fn validate_overlay(catalog: &Path, control: &Path, candidate: &Candidate) -> Result<()> {
578582
let shadow = build_overlay(catalog, control, candidate)?;
579-
crate::catalog_transaction::validate_full_catalog(shadow.path())
580-
.context("candidate fails full-catalog validation")
583+
// The shadow is a disposable projection nested under the catalog; sockets will be bound from
584+
// the real catalog, so that is the runtime root the candidate must satisfy.
585+
crate::catalog_transaction::validate_full_catalog(
586+
shadow.path(),
587+
crate::validate::RuntimeRoot::Catalog(catalog),
588+
)
589+
.context("candidate fails full-catalog validation")
581590
}
582591

583592
fn build_overlay(
@@ -653,8 +662,12 @@ pub(crate) fn admit_declaration_rewrite(
653662
.with_context(|| format!("create validation overlay {}", parent.display()))?;
654663
}
655664
fs::write(&target, bytes).context("write candidate into validation shadow")?;
656-
crate::catalog_transaction::validate_full_catalog(shadow.path())
657-
.context("candidate fails full-catalog validation")
665+
// Another disposable projection: judge the candidate against the catalog that will run.
666+
crate::catalog_transaction::validate_full_catalog(
667+
shadow.path(),
668+
crate::validate::RuntimeRoot::Catalog(&catalog),
669+
)
670+
.context("candidate fails full-catalog validation")
658671
}
659672

660673
fn copy_filtered_catalog(

src/catalog_graph.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,13 @@ pub fn snapshot(root: &Path, this_host: &str) -> Result<CatalogGraph> {
149149
let _lock = crate::CatalogLock::shared(&root)
150150
.context("acquire shared catalog-authoring lock for catalog graph")?;
151151
let found = crate::discover_strict(&root);
152-
let report = crate::validate::validate_discovered(&root, Some(this_host), &found);
152+
// The graph reads the live catalog in place, so the tree under inspection is the runtime root.
153+
let report = crate::validate::validate_discovered(
154+
&root,
155+
Some(this_host),
156+
crate::validate::RuntimeRoot::Catalog(&root),
157+
&found,
158+
);
153159

154160
let mut runtime_by_path: BTreeMap<PathBuf, Vec<crate::agents::AgentRow>> = BTreeMap::new();
155161
for row in crate::agents::roster_from_discovered(&found, &root, this_host) {

src/catalog_transaction.rs

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,8 @@ pub fn diff(request: DiffRequest) -> Result<DiffResult> {
398398
let before = project(&retained_catalog, ProjectionSource::Current, &catalog)?;
399399
validate_projection_link_counts(&retained_catalog, &before, "live catalog")?;
400400
validate_live_workspace_facts(&catalog, &before.workspace_dirs)?;
401-
validate_full_catalog(&retained_catalog).context("validate live catalog for diff")?;
401+
validate_full_catalog(&retained_catalog, crate::validate::RuntimeRoot::Catalog(&catalog))
402+
.context("validate live catalog for diff")?;
402403
anyhow::ensure!(
403404
before.root_sha256 == request.expect_sha256,
404405
"catalog diff precondition failed: expected root sha256 {}, found {}",
@@ -409,7 +410,8 @@ pub fn diff(request: DiffRequest) -> Result<DiffResult> {
409410
let captured = tempfile::tempdir().context("create prepared diff capture root")?;
410411
capture_prepared_catalog(&prepared, captured.path())?;
411412
let after = project(captured.path(), ProjectionSource::Prepared, &catalog)?;
412-
validate_full_catalog(captured.path()).context("validate prepared catalog for diff")?;
413+
validate_full_catalog(captured.path(), crate::validate::RuntimeRoot::Catalog(&catalog))
414+
.context("validate prepared catalog for diff")?;
413415

414416
let before_specs = canonical_semantic_specs(&retained_catalog)?;
415417
let after_specs = canonical_semantic_specs(captured.path())?;
@@ -1297,7 +1299,7 @@ pub fn bootstrap(request: BootstrapRequest) -> Result<BootstrapResult> {
12971299

12981300
let admission = tempfile::tempdir().context("create prepared-catalog admission root")?;
12991301
materialize_projection(&desired, admission.path())?;
1300-
validate_full_catalog(admission.path())?;
1302+
validate_full_catalog(admission.path(), crate::validate::RuntimeRoot::Catalog(&catalog))?;
13011303
let desired_config = crate::catalog::load(admission.path())?;
13021304
validate_external_pty_root(
13031305
&catalog,
@@ -1337,7 +1339,7 @@ pub fn bootstrap(request: BootstrapRequest) -> Result<BootstrapResult> {
13371339
desired.root_sha256,
13381340
staged.root_sha256
13391341
);
1340-
validate_full_catalog(&stage)?;
1342+
validate_full_catalog(&stage, crate::validate::RuntimeRoot::Catalog(&catalog))?;
13411343
let lock = initialize_bootstrap_control(&stage)?;
13421344
sync_tree_dirs(&stage)?;
13431345
Ok(lock)
@@ -1405,7 +1407,7 @@ fn inspect_existing_bootstrap(
14051407
&catalog,
14061408
&desired.workspace_dirs,
14071409
)?;
1408-
validate_full_catalog(&retained_catalog)?;
1410+
validate_full_catalog(&retained_catalog, crate::validate::RuntimeRoot::Catalog(&catalog))?;
14091411
anyhow::ensure!(
14101412
current.root_sha256 == desired.root_sha256,
14111413
"catalog bootstrap target already exists with root sha256 {}, expected {}",
@@ -1616,7 +1618,7 @@ pub fn apply(request: ApplyRequest) -> Result<ApplyResult> {
16161618
// are mirrored as empty directories; their live content is never copied or hashed.
16171619
let admission = tempfile::tempdir().context("create prepared-catalog admission root")?;
16181620
materialize_projection(&desired, admission.path())?;
1619-
validate_full_catalog(admission.path())?;
1621+
validate_full_catalog(admission.path(), crate::validate::RuntimeRoot::Catalog(&catalog))?;
16201622
let desired_config = crate::catalog::load(admission.path())?;
16211623
validate_external_pty_root(
16221624
&catalog,
@@ -1728,7 +1730,8 @@ pub fn apply(request: ApplyRequest) -> Result<ApplyResult> {
17281730
staged.root_sha256,
17291731
verified.root_sha256
17301732
);
1731-
validate_full_catalog(&catalog).context("validate applied live catalog")?;
1733+
validate_full_catalog(&catalog, crate::validate::RuntimeRoot::Catalog(&catalog))
1734+
.context("validate applied live catalog")?;
17321735
sync_dir(&catalog)?;
17331736
generation.commit()?;
17341737
test_checkpoint("before-clear");
@@ -1755,7 +1758,13 @@ pub fn apply(request: ApplyRequest) -> Result<ApplyResult> {
17551758
}
17561759

17571760
/// Full structural and host-scoped validation for a complete prospective catalog.
1758-
pub(crate) fn validate_full_catalog(root: &Path) -> Result<()> {
1761+
///
1762+
/// `runtime` names the catalog whose resolved pty root bounds session sockets. It is a separate
1763+
/// argument because `root` is frequently NOT that catalog: admission validates a projection, diff
1764+
/// validates a capture, bootstrap validates a stage, and a retained live catalog is addressed
1765+
/// through a file-descriptor path. Reading the bound off `root` charged declarations for the depth
1766+
/// of whichever temporary tree happened to be under inspection.
1767+
pub(crate) fn validate_full_catalog(root: &Path, runtime: crate::validate::RuntimeRoot<'_>) -> Result<()> {
17591768
let found = crate::discover(root);
17601769
let mut hosts = BTreeSet::new();
17611770
for spec in &found.specs {
@@ -1775,7 +1784,7 @@ pub(crate) fn validate_full_catalog(root: &Path) -> Result<()> {
17751784
.map(format_issue),
17761785
);
17771786
for host in hosts {
1778-
let report = crate::validate::validate_for_host(root, &host);
1787+
let report = crate::validate::validate_for_host_at(root, &host, runtime);
17791788
errors.extend(
17801789
report
17811790
.issues

0 commit comments

Comments
 (0)