Skip to content

Commit 320d4ef

Browse files
authored
perf(supervisor-network): cache proposal coverage by policy snapshot (#3201)
Evaluate proposal coverage once per observed installed policy snapshot while preserving replacement and deadline behavior. Signed-off-by: Shiju <shiju@nvidia.com>
1 parent b9c7d5c commit 320d4ef

1 file changed

Lines changed: 114 additions & 13 deletions

File tree

crates/openshell-supervisor-network/src/policy_local.rs

Lines changed: 114 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ const MAX_DENIAL_LINE_BYTES: usize = 4096;
8585

8686
#[derive(Debug)]
8787
pub struct PolicyLocalContext {
88-
current_policy: Arc<RwLock<Option<ProtoSandboxPolicy>>>,
88+
current_policy: Arc<RwLock<Option<Arc<ProtoSandboxPolicy>>>>,
8989
agent_proposals: AgentProposals,
9090
gateway_endpoint: Option<String>,
9191
sandbox_name: Option<String>,
@@ -120,7 +120,7 @@ impl PolicyLocalContext {
120120
workspace_rx: tokio::sync::watch::Receiver<String>,
121121
) -> Self {
122122
Self {
123-
current_policy: Arc::new(RwLock::new(current_policy)),
123+
current_policy: Arc::new(RwLock::new(current_policy.map(Arc::new))),
124124
agent_proposals,
125125
gateway_endpoint,
126126
sandbox_name,
@@ -130,7 +130,10 @@ impl PolicyLocalContext {
130130
}
131131

132132
pub async fn set_current_policy(&self, policy: ProtoSandboxPolicy) {
133-
*self.current_policy.write().await = Some(policy);
133+
// Every successful reload receives a distinct Arc, including an
134+
// identical policy installed again. Waiters use pointer identity to
135+
// decide whether the installed snapshot needs another coverage scan.
136+
*self.current_policy.write().await = Some(Arc::new(policy));
134137
}
135138

136139
pub fn workspace(&self) -> String {
@@ -887,18 +890,41 @@ async fn wait_for_local_policy_to_cover(
887890
proposed_rule: &NetworkPolicyRule,
888891
deadline: tokio::time::Instant,
889892
) -> bool {
893+
wait_for_local_policy_to_cover_with(
894+
ctx,
895+
proposed_rule,
896+
deadline,
897+
openshell_policy::policy_covers_rule,
898+
)
899+
.await
900+
}
901+
902+
async fn wait_for_local_policy_to_cover_with<F>(
903+
ctx: &PolicyLocalContext,
904+
proposed_rule: &NetworkPolicyRule,
905+
deadline: tokio::time::Instant,
906+
covers_rule: F,
907+
) -> bool
908+
where
909+
F: Fn(&ProtoSandboxPolicy, &NetworkPolicyRule) -> bool,
910+
{
890911
const TICK: std::time::Duration = std::time::Duration::from_millis(200);
912+
let mut checked_snapshot: Option<Arc<ProtoSandboxPolicy>> = None;
891913
loop {
892-
// Clone the snapshot out of the RwLock before running coverage —
893-
// otherwise the read guard is held across `policy_covers_rule`'s
894-
// iteration of `network_policies`, serializing a writer (supervisor
895-
// reload) on the very thing we're waiting for. Clone-per-tick on
896-
// a few-KB struct is cheap for the bounded wait window here.
914+
// Clone only the Arc while holding the lock. Coverage runs once for
915+
// each installed snapshot and never holds the read guard, so reloads
916+
// cannot block behind a full network-policy scan.
897917
let snapshot = ctx.current_policy.read().await.clone();
898918
if let Some(policy) = snapshot.as_ref()
899-
&& openshell_policy::policy_covers_rule(policy, proposed_rule)
919+
&& checked_snapshot
920+
.as_ref()
921+
.is_none_or(|checked| !Arc::ptr_eq(checked, policy))
900922
{
901-
return true;
923+
let covered = covers_rule(policy, proposed_rule);
924+
checked_snapshot = Some(Arc::clone(policy));
925+
if covered {
926+
return true;
927+
}
902928
}
903929
let remaining = deadline.saturating_duration_since(tokio::time::Instant::now());
904930
if remaining.is_zero() {
@@ -2083,7 +2109,7 @@ mod tests {
20832109
let policy = ctx.current_policy.clone();
20842110
tokio::spawn(async move {
20852111
tokio::time::sleep(std::time::Duration::from_millis(50)).await;
2086-
*policy.write().await = Some(policy_with_rule(NetworkPolicyRule {
2112+
*policy.write().await = Some(Arc::new(policy_with_rule(NetworkPolicyRule {
20872113
name: "unrelated".to_string(),
20882114
endpoints: vec![NetworkEndpoint {
20892115
host: "api.example.com".to_string(),
@@ -2095,7 +2121,7 @@ mod tests {
20952121
path: "/usr/bin/curl".to_string(),
20962122
..Default::default()
20972123
}],
2098-
}));
2124+
})));
20992125
})
21002126
};
21012127

@@ -2136,7 +2162,7 @@ mod tests {
21362162
let target = proposed.clone();
21372163
tokio::spawn(async move {
21382164
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
2139-
*policy.write().await = Some(policy_with_rule(target));
2165+
*policy.write().await = Some(Arc::new(policy_with_rule(target)));
21402166
})
21412167
};
21422168

@@ -2185,6 +2211,81 @@ mod tests {
21852211
);
21862212
}
21872213

2214+
#[tokio::test(start_paused = true)]
2215+
async fn wait_checks_unchanged_policy_snapshot_once() {
2216+
let proposed = proposed_curl_rule_for_github();
2217+
let ctx = PolicyLocalContext::new(
2218+
Some(ProtoSandboxPolicy {
2219+
version: 1,
2220+
..Default::default()
2221+
}),
2222+
None,
2223+
None,
2224+
AgentProposals::new(false),
2225+
test_workspace_rx(),
2226+
);
2227+
let checks = std::cell::Cell::new(0_u32);
2228+
let deadline = tokio::time::Instant::now() + std::time::Duration::from_millis(650);
2229+
2230+
let reloaded = wait_for_local_policy_to_cover_with(&ctx, &proposed, deadline, |_, _| {
2231+
checks.set(checks.get() + 1);
2232+
false
2233+
})
2234+
.await;
2235+
2236+
assert!(!reloaded);
2237+
assert_eq!(
2238+
checks.get(),
2239+
1,
2240+
"an unchanged snapshot must be checked once"
2241+
);
2242+
}
2243+
2244+
#[tokio::test(start_paused = true)]
2245+
async fn wait_checks_each_installed_snapshot_once() {
2246+
let proposed = proposed_curl_rule_for_github();
2247+
let empty_policy = ProtoSandboxPolicy {
2248+
version: 1,
2249+
..Default::default()
2250+
};
2251+
let ctx = Arc::new(PolicyLocalContext::new(
2252+
Some(empty_policy.clone()),
2253+
None,
2254+
None,
2255+
AgentProposals::new(false),
2256+
test_workspace_rx(),
2257+
));
2258+
let reloads = Arc::clone(&ctx);
2259+
let covering_rule = proposed.clone();
2260+
let installations = tokio::spawn(async move {
2261+
tokio::time::sleep(std::time::Duration::from_millis(250)).await;
2262+
reloads.set_current_policy(empty_policy).await;
2263+
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
2264+
reloads
2265+
.set_current_policy(policy_with_rule(covering_rule))
2266+
.await;
2267+
});
2268+
let checks = std::cell::Cell::new(0_u32);
2269+
let deadline = tokio::time::Instant::now() + std::time::Duration::from_secs(2);
2270+
2271+
let reloaded =
2272+
wait_for_local_policy_to_cover_with(&ctx, &proposed, deadline, |policy, rule| {
2273+
checks.set(checks.get() + 1);
2274+
openshell_policy::policy_covers_rule(policy, rule)
2275+
})
2276+
.await;
2277+
installations
2278+
.await
2279+
.expect("policy installation task must complete");
2280+
2281+
assert!(reloaded, "the covering snapshot must complete the wait");
2282+
assert_eq!(
2283+
checks.get(),
2284+
3,
2285+
"initial, identical replacement, and covering snapshots need one check each"
2286+
);
2287+
}
2288+
21882289
#[test]
21892290
fn sanitize_reason_for_audit_strips_control_chars_and_caps_length() {
21902291
// Tabs and newlines are stripped; ordinary printable chars survive;

0 commit comments

Comments
 (0)