From 140445dda8b22becd0e701e954d986eb0750f593 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Wed, 23 Sep 2026 20:01:54 +0200 Subject: [PATCH] Let the publisher carry its own custody choice, so the lease-retry tests run on Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aca5686's fix was right but its two new tests always called acquire_recent (custody enforced), and a test lease necessarily lives under a tempdir — never root-controlled. On the pinned Linux image that failed with "process write-lease ancestor contains a non-root-controlled directory" instead of exercising the retry logic; on Windows it passed only because require_process_write_lease_custody is a deliberate no-op there, which is exactly the platform gap this whole fix is about. The deploy platform is Linux, so the tests have to run there for real, not slip through on a Windows-only no-op. RuntimePresencePublisher now carries the same enforce_custody choice ProcessWriteLeaseGuard::acquire_inner already had: true from from_environment (every production construction path), false from the test fixture constructor. wait_for_write_lease calls acquire_recent or acquire_recent_fixture to match. acquire_recent_fixture is no longer #[cfg(test)]-gated, since production's own wait_for_write_lease source must reference it (unreachable there, since production publishers always carry enforce_custody: true, but the symbol has to exist in a release build). No trait, no env flag — a plain bool threaded through the existing constructor. Added production_custody_choice_rejects_a_lease_under_a_non_root_tempdir: builds a publisher the way from_environment does (enforce_custody: true) via a new fixture_with_state_contract_and_custody helper (the existing fixture() stays enforce_custody: false, unchanged), points it at the same empty-revoked-store shape the other two tests use, and asserts wait_for_write_lease still refuses it with a custody-rejection error rather than acquiring it. This one is #[cfg(target_os = "linux")]-gated — correctly, not as a workaround: custody enforcement is architecturally a root-ownership concept that idunn_health.rs's own code makes a no-op on every other OS, so there is nothing for the assertion to observe on Windows regardless of this fix. The two tests from aca5686 (an_empty_revoked_lease_store_times_out_not_not_admitted, a_lease_written_after_an_empty_revoked_store_still_wins) needed no changes beyond inheriting enforce_custody: false through fixture() as before — they were already testing the retry logic, not custody, and now do so honestly on both platforms since custody enforcement is out of their path entirely. cargo test -p ghostlight-dungeon --bin ghostlight-dungeon (Windows): 192 passed, 0 failed, 4 ignored, unchanged from aca5686 — the new Linux-gated test does not compile into this binary here, so it does not appear even as ignored. Awaiting the Linux run for its count and the custody-rejection assertion. Co-Authored-By: Claude Sonnet 5 --- crates/ghostlight-dungeon/src/idunn_health.rs | 93 +++++++++++++++++-- 1 file changed, 86 insertions(+), 7 deletions(-) diff --git a/crates/ghostlight-dungeon/src/idunn_health.rs b/crates/ghostlight-dungeon/src/idunn_health.rs index a881366..66770b0 100644 --- a/crates/ghostlight-dungeon/src/idunn_health.rs +++ b/crates/ghostlight-dungeon/src/idunn_health.rs @@ -111,6 +111,16 @@ pub(crate) struct RuntimePresencePublisher { dependency_bindings: RuntimeDependencyBindings, publisher_sequence: u64, active_write_lease_sha256: Option, + /// Whether `wait_for_write_lease` requires the lease store to sit under + /// a root-controlled directory chain (`acquire_recent`) or accepts a + /// test's own tempdir (`acquire_recent_fixture`). Every production + /// construction path (`from_environment`) sets this `true`; only the + /// test fixture constructor sets it `false`. This is the same choice + /// `ProcessWriteLeaseGuard::acquire_inner`'s own `enforce_custody` + /// parameter already makes — the publisher just carries which one its + /// caller needs instead of `wait_for_write_lease` always picking the + /// production path. + enforce_custody: bool, } impl RuntimePresencePublisher { @@ -149,6 +159,7 @@ impl RuntimePresencePublisher { provider_signer, activation_signer, write_lease_path, + true, )?)) } @@ -161,6 +172,7 @@ impl RuntimePresencePublisher { provider_signer: ServiceIdentitySigner, activation_signer: IdunnRuntimeActivationSigner, write_lease_path: PathBuf, + enforce_custody: bool, ) -> Result { expected.validate()?; activation.validate()?; @@ -243,6 +255,7 @@ impl RuntimePresencePublisher { dependency_bindings, publisher_sequence: 0, active_write_lease_sha256: None, + enforce_custody, }) } @@ -286,12 +299,22 @@ impl RuntimePresencePublisher { // them by size or by matching the error text. if self.write_lease_path.exists() && sibling_lock_path(&self.write_lease_path).exists() { - match ProcessWriteLeaseGuard::acquire_recent( - self.write_lease_path.clone(), - self.expected.clone(), - self.activation.clone(), - recent_warming_proofs.iter().cloned().collect(), - ) { + let acquired = if self.enforce_custody { + ProcessWriteLeaseGuard::acquire_recent( + self.write_lease_path.clone(), + self.expected.clone(), + self.activation.clone(), + recent_warming_proofs.iter().cloned().collect(), + ) + } else { + ProcessWriteLeaseGuard::acquire_recent_fixture( + self.write_lease_path.clone(), + self.expected.clone(), + self.activation.clone(), + recent_warming_proofs.iter().cloned().collect(), + ) + }; + match acquired { Ok(guard) => return Ok(guard), Err(error) => last_acquisition_error = Some(error), } @@ -900,7 +923,13 @@ impl ProcessWriteLeaseGuard { ) } - #[cfg(test)] + /// Not test-gated: `wait_for_write_lease` calls this directly whenever + /// its publisher's own `enforce_custody` is `false` (the test fixture + /// constructor), the same way it calls `acquire_recent` when `true` (the + /// production constructor). A test lease deliberately lives under a + /// tempdir, which is never root-controlled, so custody enforcement must + /// be off for it — not skipped by `#[cfg(test)]`, which a Linux release + /// build does not carry. fn acquire_recent_fixture( path: PathBuf, expected: IdunnExpectedIncarnationRecord, @@ -1294,6 +1323,14 @@ pub(crate) mod tests { } fn fixture_with_state_contract(root: &Path, state_contract_sha256: &str) -> Result { + fixture_with_state_contract_and_custody(root, state_contract_sha256, false) + } + + fn fixture_with_state_contract_and_custody( + root: &Path, + state_contract_sha256: &str, + enforce_custody: bool, + ) -> Result { let provider_path = root.join("provider.cc"); let provider = enroll_service_identity_at::(&provider_path)?; @@ -1376,6 +1413,7 @@ pub(crate) mod tests { provider_signer, activation_signer, root.join("write-lease.cc"), + enforce_custody, )?; Ok(Fixture { publisher, @@ -1994,6 +2032,47 @@ pub(crate) mod tests { Ok(()) } + /// Pins the choice `wait_for_write_lease` now makes between + /// `acquire_recent` and `acquire_recent_fixture`: a publisher built the + /// way `from_environment` (production) builds it — `enforce_custody: + /// true` — must still refuse a lease store that sits under a plain + /// tempdir, never root-controlled. Proven behaviorally through the real + /// `wait_for_write_lease` path, not by reading the private field, so a + /// future change that keeps the field but stops honoring it still fails + /// this test. Linux-only: custody enforcement is architecturally a + /// root-ownership concept (`require_process_write_lease_custody` is a + /// deliberate no-op on every other OS, `idunn_health.rs`'s own + /// `#[cfg(not(target_os = "linux"))]` fallback), so there is nothing for + /// this assertion to observe elsewhere. + #[cfg(target_os = "linux")] + #[tokio::test] + async fn production_custody_choice_rejects_a_lease_under_a_non_root_tempdir() -> Result<()> { + let root = tempfile::tempdir()?; + let mut fixture = + fixture_with_state_contract_and_custody(root.path(), STATE_CONTRACT_SHA256, true)?; + let path = root.path().join("write-lease.cc"); + leave_an_empty_revoked_lease_store(&path)?; + let warming = PublishedRuntimePresence { + canonical_sha256: digest('9'), + }; + let error = match fixture + .publisher + .wait_for_write_lease(&warming, Duration::from_millis(600)) + .await + { + Ok(_) => panic!( + "wait_for_write_lease acquired a lease under a tempdir with custody enforced" + ), + Err(error) => error, + }; + let chain = format!("{error:#}"); + assert!( + chain.contains("non-root-controlled") || chain.contains("root-owned"), + "expected a custody rejection, got `{chain}` — production custody enforcement regressed" + ); + Ok(()) + } + /// Pure over (uid, gid, mode), so it runs on this Windows workstation too /// rather than sitting unexercised behind `#[cfg(target_os = "linux")]` /// the way `require_runtime_bundle`'s real-filesystem check has always