Skip to content

Commit 7a9f82a

Browse files
committed
fix(sandbox): /wait detects policy reload by content, not the schema version
The previous attempt at the /wait-after-approve race fix compared `SandboxPolicy.version` between /wait start and the policy reload — but that field is the *schema* version (constant 1), not a revision counter. Every comparison was `current(1) > baseline(1) == false`, so the wait blocked until the agent's 300s timeout regardless of whether the supervisor had actually reloaded. The demo SSH connection then timed out around the 240s mark. Diagnosed from a live run's OCSF trace: supervisor pulled v2 at +8.5s after approval (CONFIG:LOADED), but the sandbox-side CONFIG:APPROVED that my /wait emits didn't fire until +304s — exactly at the 300s deadline. Fix: compare the whole policy via prost's derived PartialEq. Any field change (network_policies map being the only one that actually mutates today) flips equality. A clone-per-200ms-tick on a few-KB proto is cheap inside the bounded wait window. Tests rewritten to match the new contract: the supervisor-reload fixture now keeps `version: 1` constant and changes `network_policies` contents, mirroring the exact failure mode from the live run. Signed-off-by: Alexander Watson <zredlined@gmail.com>
1 parent e44f1a0 commit 7a9f82a

1 file changed

Lines changed: 69 additions & 37 deletions

File tree

crates/openshell-sandbox/src/policy_local.rs

Lines changed: 69 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -680,17 +680,17 @@ async fn proposal_wait_response(
680680
};
681681
let timeout_secs = parse_timeout_query(query);
682682
let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(timeout_secs);
683-
// Baseline local policy version at /wait start. When a chunk is
683+
// Baseline local policy fingerprint at /wait start. When a chunk is
684684
// approved upstream the gateway merges it immediately, but the local
685685
// supervisor reloads policy on its own poll cycle — without this
686686
// baseline we can return "approved" before the local rule is active,
687687
// and the agent's single retry races the reload.
688-
let baseline_policy_version: u32 = ctx
689-
.current_policy
690-
.read()
691-
.await
692-
.as_ref()
693-
.map_or(0, |p| p.version);
688+
//
689+
// We compare encoded policy bytes rather than `SandboxPolicy.version`,
690+
// which is a schema-version constant (always 1) and never bumps on a
691+
// revision change. A naive version compare loops forever; a byte
692+
// compare picks up any rule mutation, which is what reload means.
693+
let baseline_fingerprint = local_policy_fingerprint(ctx).await;
694694
loop {
695695
match fetch_chunk(&session, chunk_id).await {
696696
Ok(Some(chunk)) if is_terminal_status(&chunk.status) => {
@@ -700,7 +700,7 @@ async fn proposal_wait_response(
700700
// policy. Bounded by the same deadline so a stuck
701701
// supervisor cannot extend the wait beyond what the
702702
// caller asked for.
703-
wait_for_local_policy_bump(ctx, baseline_policy_version, deadline).await;
703+
wait_for_local_policy_bump(ctx, &baseline_fingerprint, deadline).await;
704704
}
705705
// Audit beat: emit at the moment this sandbox observes the
706706
// decision so the trace correlates with the proxy events
@@ -772,29 +772,39 @@ fn is_terminal_status(status: &str) -> bool {
772772
matches!(status, "approved" | "rejected")
773773
}
774774

775+
/// Snapshot of the local policy at a moment in time, used as a revision
776+
/// fingerprint by `wait_for_local_policy_bump`. Compares with `==` against
777+
/// a later snapshot to detect a reload — prost-generated types derive
778+
/// `PartialEq`, so any field change (including the `network_policies` map)
779+
/// flips equality.
780+
async fn local_policy_fingerprint(ctx: &PolicyLocalContext) -> Option<ProtoSandboxPolicy> {
781+
ctx.current_policy.read().await.clone()
782+
}
783+
775784
/// After a chunk is approved upstream, wait until the local supervisor has
776-
/// loaded a policy version strictly newer than the baseline captured at the
777-
/// start of `/wait`. Bounded by the caller-supplied deadline; returns early
778-
/// (best-effort) if the deadline passes without the version bumping.
785+
/// loaded a policy that differs from the baseline captured at the start of
786+
/// `/wait`. Bounded by the caller-supplied deadline; returns early
787+
/// (best-effort) if the deadline passes without a change.
788+
///
789+
/// We compare the whole policy proto rather than any single field because
790+
/// `SandboxPolicy.version` is a schema-version constant (always 1) and does
791+
/// not track revisions — only the policy *content* changes when a reload
792+
/// lands. Equality is via prost's derived `PartialEq`; a clone-per-tick on
793+
/// a few-KB struct is cheap for the bounded wait window this lives in.
779794
///
780795
/// The polling cadence here is faster than `PROPOSAL_WAIT_POLL_INTERVAL`
781796
/// (which paces upstream gateway calls). This loop only reads in-memory
782797
/// state, so 200ms gives a responsive handoff to the agent's retry once
783798
/// the supervisor's own policy poll catches up.
784799
async fn wait_for_local_policy_bump(
785800
ctx: &PolicyLocalContext,
786-
baseline_version: u32,
801+
baseline: &Option<ProtoSandboxPolicy>,
787802
deadline: tokio::time::Instant,
788803
) {
789804
const TICK: std::time::Duration = std::time::Duration::from_millis(200);
790805
loop {
791-
let current: u32 = ctx
792-
.current_policy
793-
.read()
794-
.await
795-
.as_ref()
796-
.map_or(0, |p| p.version);
797-
if current > baseline_version {
806+
let current = local_policy_fingerprint(ctx).await;
807+
if current.as_ref() != baseline.as_ref() {
798808
return;
799809
}
800810
let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
@@ -1721,50 +1731,72 @@ mod tests {
17211731
}
17221732

17231733
#[tokio::test]
1724-
async fn wait_for_local_policy_bump_returns_when_version_advances() {
1725-
let ctx = PolicyLocalContext::new(
1726-
Some(ProtoSandboxPolicy {
1727-
version: 5,
1728-
..Default::default()
1729-
}),
1730-
None,
1731-
None,
1732-
);
1734+
async fn wait_for_local_policy_bump_returns_when_policy_content_changes() {
1735+
// Baseline: a policy with one network policy keyed "initial".
1736+
// SandboxPolicy.version stays 1 across reloads (it's a schema
1737+
// marker, not a revision id) — this test deliberately changes
1738+
// *content* without bumping the version field, which is the
1739+
// failure mode that surfaced in the demo (supervisor reloaded
1740+
// the policy but kept version=1, so the prior version-compare
1741+
// implementation hung until deadline).
1742+
let initial = ProtoSandboxPolicy {
1743+
version: 1,
1744+
network_policies: HashMap::from([(
1745+
"initial".to_string(),
1746+
NetworkPolicyRule {
1747+
name: "initial".to_string(),
1748+
..Default::default()
1749+
},
1750+
)]),
1751+
..Default::default()
1752+
};
1753+
let ctx = PolicyLocalContext::new(Some(initial), None, None);
1754+
let baseline = local_policy_fingerprint(&ctx).await;
1755+
17331756
let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(2);
1734-
let bumped = {
1757+
let changed = {
17351758
let policy = ctx.current_policy.clone();
17361759
tokio::spawn(async move {
17371760
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
1761+
// Same schema version (1), different content — exactly
1762+
// what the supervisor does on a real reload.
17381763
*policy.write().await = Some(ProtoSandboxPolicy {
1739-
version: 6,
1764+
version: 1,
1765+
network_policies: HashMap::from([(
1766+
"reloaded".to_string(),
1767+
NetworkPolicyRule {
1768+
name: "reloaded".to_string(),
1769+
..Default::default()
1770+
},
1771+
)]),
17401772
..Default::default()
17411773
});
17421774
})
17431775
};
17441776
let start = tokio::time::Instant::now();
1745-
wait_for_local_policy_bump(&ctx, 5, deadline).await;
1746-
bumped.await.unwrap();
1747-
// Returned promptly after the bump, well before the 2s deadline.
1777+
wait_for_local_policy_bump(&ctx, &baseline, deadline).await;
1778+
changed.await.unwrap();
17481779
let elapsed = start.elapsed();
17491780
assert!(
17501781
elapsed < std::time::Duration::from_millis(800),
1751-
"should return shortly after version bumps; took {elapsed:?}"
1782+
"should return shortly after content changes; took {elapsed:?}"
17521783
);
17531784
}
17541785

17551786
#[tokio::test]
1756-
async fn wait_for_local_policy_bump_returns_at_deadline_if_no_advance() {
1787+
async fn wait_for_local_policy_bump_returns_at_deadline_if_no_change() {
17571788
let ctx = PolicyLocalContext::new(
17581789
Some(ProtoSandboxPolicy {
1759-
version: 5,
1790+
version: 1,
17601791
..Default::default()
17611792
}),
17621793
None,
17631794
None,
17641795
);
1796+
let baseline = local_policy_fingerprint(&ctx).await;
17651797
let deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(300);
17661798
let start = tokio::time::Instant::now();
1767-
wait_for_local_policy_bump(&ctx, 5, deadline).await;
1799+
wait_for_local_policy_bump(&ctx, &baseline, deadline).await;
17681800
let elapsed = start.elapsed();
17691801
// Best effort: returns at or shortly after the deadline rather
17701802
// than extending past it.

0 commit comments

Comments
 (0)