Skip to content

Commit 2132856

Browse files
committed
fix(api): scope workspace request IDs by target
Include the requested workspace name in create/delete admission keys while leaving workspace UUID guards unset. Cover cross-target UUID reuse, replay, and missing targets with server and live gateway regressions. Merge the latest phase-two SDK fixes and preserve the approved interceptor replay changes. Refs #3051. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
2 parents 59a6077 + b1e41c5 commit 2132856

13 files changed

Lines changed: 698 additions & 32 deletions

File tree

‎architecture/gateway.md‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,8 @@ write conflicts attach `google.rpc.ErrorInfo` with a stable reason and current
5959
version when available. `google.rpc.RetryInfo` expresses a minimum retry delay;
6060
it does not establish that a mutation is safe to repeat. SDKs retain the original
6161
transport status, metadata, and unknown details alongside decoded fields.
62+
SDK deletion waits recognize missing-resource status through typed error wrappers
63+
without suppressing other failures.
6264

6365
Ordinary user-callable unary mutations explicitly opt into durable request
6466
admission when the client supplies a UUID. Typed adapters
@@ -412,8 +414,10 @@ Public delete, membership-removal, and SSH-revocation responses use
412414
logical gateway deletion or revocation; it does not guarantee that downstream
413415
platform garbage collection has finished. Sandbox deletion returns `ACCEPTED`
414416
while its captured object ID remains in the store, and returns that ID so callers
415-
can distinguish the original sandbox from a same-name replacement. The existing
416-
owned deletion worker continues after request cancellation.
417+
can distinguish the original sandbox from a same-name replacement. Identity-aware
418+
SDK deletion waits complete on absence or a different observed ID; name-only waits
419+
continue until the name is absent. The existing owned deletion worker continues
420+
after request cancellation.
417421

418422
Missing targets return `NOT_FOUND` unless `allow_missing` explicitly requests
419423
`ALREADY_ABSENT`. Authorization, parent resolution, preconditions, and backend
@@ -476,6 +480,9 @@ Mutation admission uses a private, version-tagged JSON envelope in the same
476480
object store. Its identity namespace stays stable across format changes, and an
477481
unknown format fails closed. It contains explicit typed receipts, not arbitrary
478482
public response payloads, and is not part of the protobuf storage closure.
483+
Workspace create/delete admissions include the requested workspace name in the
484+
key, but omit a workspace UUID guard. Different names have independent request-ID
485+
namespaces; deletion receipts remain replayable after the target disappears.
479486

480487
Each sandbox policy revision stores the complete provenance annotation map
481488
supplied with that update. The revision payload is the authoritative immutable

‎crates/openshell-sdk/README.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,22 @@ Curated calls without a workspace argument explicitly select the `default`
5959
workspace. Cross-workspace listing uses the separate `*_all_workspaces`
6060
methods and requires Platform Admin access.
6161

62+
For an accepted sandbox deletion, pass its original ID to `wait_deleted` so a
63+
same-name replacement does not extend the wait. Both the default and
64+
workspace-scoped clients accept the optional third argument; pass `None` to wait
65+
for name absence instead.
66+
67+
```rust
68+
let deletion = client.delete_sandbox(name, openshell_sdk::DeleteOptions::default()).await?;
69+
if deletion.outcome == openshell_sdk::DeletionOutcome::Accepted {
70+
client.wait_deleted(
71+
name,
72+
std::time::Duration::from_secs(60),
73+
deletion.sandbox_id.as_deref(),
74+
).await?;
75+
}
76+
```
77+
6278
Curated `list_*` methods return a lazy `Pager<T>`. Each `next_page()` call
6379
issues at most one RPC and returns a `Page<T>` with its opaque continuation
6480
token. The explicit `list_all_*` conveniences exhaust that pager; `page_size`

‎crates/openshell-sdk/src/client.rs‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -422,16 +422,26 @@ impl OpenShellClient {
422422
.await
423423
}
424424

425-
/// Poll until the sandbox is gone (gRPC `NotFound`) or the `timeout`
426-
/// elapses.
427-
pub async fn wait_deleted(&self, name: &str, timeout: Duration) -> Result<()> {
425+
/// Poll until the sandbox is gone (gRPC `NotFound`) or the timeout elapses.
426+
///
427+
/// Pass the deletion result's `sandbox_id` as `expected_sandbox_id` to also
428+
/// complete when the name resolves to a different sandbox. With `None`,
429+
/// waits for the name to be absent, including any same-name replacement.
430+
pub async fn wait_deleted(
431+
&self,
432+
name: &str,
433+
timeout: Duration,
434+
expected_sandbox_id: Option<&str>,
435+
) -> Result<()> {
428436
let deadline = Instant::now() + timeout;
429437
let mut delay = Duration::from_millis(250);
430438
loop {
431439
match self.get_sandbox(name).await {
432440
Err(SdkError::NotFound { .. }) => return Ok(()),
433441
Err(other) => return Err(other),
434-
Ok(snapshot) if snapshot.phase == SandboxPhase::Deleting => {}
442+
Ok(snapshot) if expected_sandbox_id.is_some_and(|id| snapshot.id != id) => {
443+
return Ok(());
444+
}
435445
Ok(_) => {}
436446
}
437447
if Instant::now() >= deadline {
@@ -1021,13 +1031,25 @@ impl WorkspaceScopedClient {
10211031
}
10221032

10231033
/// Poll until the sandbox is gone (`NotFound`) or the timeout elapses.
1024-
pub async fn wait_deleted(&self, name: &str, timeout: Duration) -> Result<()> {
1034+
///
1035+
/// Pass the deletion result's `sandbox_id` as `expected_sandbox_id` to also
1036+
/// complete when the name resolves to a different sandbox. With `None`,
1037+
/// waits for the name to be absent, including any same-name replacement.
1038+
pub async fn wait_deleted(
1039+
&self,
1040+
name: &str,
1041+
timeout: Duration,
1042+
expected_sandbox_id: Option<&str>,
1043+
) -> Result<()> {
10251044
let deadline = Instant::now() + timeout;
10261045
let mut delay = Duration::from_millis(250);
10271046
loop {
10281047
match self.get_sandbox(name).await {
10291048
Err(SdkError::NotFound { .. }) => return Ok(()),
10301049
Err(other) => return Err(other),
1050+
Ok(snapshot) if expected_sandbox_id.is_some_and(|id| snapshot.id != id) => {
1051+
return Ok(());
1052+
}
10311053
Ok(_) => {}
10321054
}
10331055
if Instant::now() >= deadline {

‎crates/openshell-sdk/tests/client_mock.rs‎

Lines changed: 160 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@ struct MockState {
6060
last_workspace_request: Mutex<Option<String>>,
6161
get_calls: AtomicU32,
6262
phase_sequence: Vec<proto::SandboxPhase>,
63+
get_sandbox_id: Option<String>,
64+
get_error: Option<Status>,
65+
delete_response: Option<proto::DeleteSandboxResponse>,
6366
get_returns_not_found: bool,
6467
not_found_after: Option<u32>,
6568
paginate_list: bool,
@@ -337,6 +340,9 @@ impl OpenShell for TestOpenShell {
337340
selected_workspace(&req.workspace_scope).map(str::to_string);
338341
let count = self.state.get_calls.fetch_add(1, Ordering::SeqCst);
339342

343+
if let Some(error) = &self.state.get_error {
344+
return Err(error.clone());
345+
}
340346
if self.state.get_returns_not_found {
341347
return Err(Status::not_found(format!("sandbox '{name}' not found")));
342348
}
@@ -354,8 +360,12 @@ impl OpenShell for TestOpenShell {
354360
.or_else(|| self.state.phase_sequence.last().copied())
355361
.unwrap_or(proto::SandboxPhase::Ready);
356362

363+
let mut sandbox = sandbox_with_phase(&name, phase);
364+
if let Some(id) = &self.state.get_sandbox_id {
365+
sandbox.metadata.as_mut().unwrap().id.clone_from(id);
366+
}
357367
Ok(Response::new(proto::SandboxResponse {
358-
sandbox: Some(sandbox_with_phase(&name, phase)),
368+
sandbox: Some(sandbox),
359369
}))
360370
}
361371

@@ -429,6 +439,9 @@ impl OpenShell for TestOpenShell {
429439
*self.state.last_delete_name.lock().await = Some(req.name);
430440
*self.state.last_delete_workspace.lock().await =
431441
selected_workspace(&req.workspace_scope).map(str::to_string);
442+
if let Some(response) = &self.state.delete_response {
443+
return Ok(Response::new(response.clone()));
444+
}
432445
Ok(Response::new(proto::DeleteSandboxResponse {
433446
sandbox_id: String::new(),
434447
outcome: proto::DeletionOutcome::Completed.into(),
@@ -1282,19 +1295,154 @@ async fn wait_ready_surfaces_error_phase() {
12821295

12831296
#[tokio::test]
12841297
async fn wait_deleted_returns_when_get_reports_not_found() {
1285-
let state = Arc::new(MockState {
1286-
phase_sequence: vec![proto::SandboxPhase::Deleting],
1287-
not_found_after: Some(2),
1288-
..Default::default()
1289-
});
1290-
let endpoint = start_mock(state.clone()).await;
1291-
let client = connect(&endpoint).await;
1298+
for scoped in [false, true] {
1299+
for expected_id in [None, Some("id-my-box")] {
1300+
let state = Arc::new(MockState {
1301+
phase_sequence: vec![proto::SandboxPhase::Deleting],
1302+
not_found_after: Some(1),
1303+
..Default::default()
1304+
});
1305+
let endpoint = start_mock(state.clone()).await;
1306+
let client = connect(&endpoint).await;
1307+
let timeout = std::time::Duration::from_secs(5);
1308+
if scoped {
1309+
client
1310+
.workspace("team")
1311+
.wait_deleted("my-box", timeout, expected_id)
1312+
.await
1313+
} else {
1314+
client.wait_deleted("my-box", timeout, expected_id).await
1315+
}
1316+
.unwrap();
1317+
assert_eq!(state.get_calls.load(Ordering::SeqCst), 2);
1318+
}
1319+
}
1320+
}
12921321

1293-
client
1294-
.wait_deleted("my-box", std::time::Duration::from_secs(5))
1295-
.await
1322+
#[tokio::test]
1323+
async fn wait_deleted_completes_on_replacement_after_accepted_deletion() {
1324+
for scoped in [false, true] {
1325+
let state = Arc::new(MockState {
1326+
get_sandbox_id: Some("replacement-id".to_string()),
1327+
delete_response: Some(proto::DeleteSandboxResponse {
1328+
outcome: proto::DeletionOutcome::Accepted.into(),
1329+
sandbox_id: "old-id".to_string(),
1330+
}),
1331+
..Default::default()
1332+
});
1333+
let endpoint = start_mock(state.clone()).await;
1334+
let client = connect(&endpoint).await;
1335+
let timeout = std::time::Duration::from_millis(50);
1336+
let result = if scoped {
1337+
client
1338+
.workspace("team")
1339+
.delete_sandbox("my-box", openshell_sdk::DeleteOptions::default())
1340+
.await
1341+
} else {
1342+
client
1343+
.delete_sandbox("my-box", openshell_sdk::DeleteOptions::default())
1344+
.await
1345+
}
12961346
.unwrap();
1297-
assert!(state.get_calls.load(Ordering::SeqCst) >= 3);
1347+
assert_eq!(result.outcome, openshell_sdk::DeletionOutcome::Accepted);
1348+
assert_eq!(result.sandbox_id.as_deref(), Some("old-id"));
1349+
if scoped {
1350+
client
1351+
.workspace("team")
1352+
.wait_deleted("my-box", timeout, result.sandbox_id.as_deref())
1353+
.await
1354+
} else {
1355+
client
1356+
.wait_deleted("my-box", timeout, result.sandbox_id.as_deref())
1357+
.await
1358+
}
1359+
.unwrap();
1360+
assert_eq!(state.get_calls.load(Ordering::SeqCst), 1);
1361+
let workspace = if scoped { "team" } else { "default" };
1362+
assert_eq!(
1363+
state.last_get_workspace.lock().await.as_deref(),
1364+
Some(workspace)
1365+
);
1366+
assert_eq!(
1367+
state.last_delete_workspace.lock().await.as_deref(),
1368+
Some(workspace)
1369+
);
1370+
}
1371+
}
1372+
1373+
#[tokio::test]
1374+
async fn wait_deleted_does_not_complete_while_expected_identity_remains() {
1375+
for scoped in [false, true] {
1376+
let state = Arc::new(MockState {
1377+
get_sandbox_id: Some("old-id".to_string()),
1378+
..Default::default()
1379+
});
1380+
let endpoint = start_mock(state.clone()).await;
1381+
let client = connect(&endpoint).await;
1382+
let timeout = std::time::Duration::ZERO;
1383+
let error = if scoped {
1384+
client
1385+
.workspace("team")
1386+
.wait_deleted("my-box", timeout, Some("old-id"))
1387+
.await
1388+
} else {
1389+
client.wait_deleted("my-box", timeout, Some("old-id")).await
1390+
}
1391+
.unwrap_err();
1392+
assert_eq!(error.code(), "connect");
1393+
assert!(error.to_string().contains("timed out waiting"));
1394+
assert_eq!(state.get_calls.load(Ordering::SeqCst), 1);
1395+
}
1396+
}
1397+
1398+
#[tokio::test]
1399+
async fn wait_deleted_without_identity_waits_for_replacement_to_disappear() {
1400+
for scoped in [false, true] {
1401+
let state = Arc::new(MockState {
1402+
get_sandbox_id: Some("replacement-id".to_string()),
1403+
not_found_after: Some(1),
1404+
..Default::default()
1405+
});
1406+
let endpoint = start_mock(state.clone()).await;
1407+
let client = connect(&endpoint).await;
1408+
let timeout = std::time::Duration::from_secs(5);
1409+
if scoped {
1410+
client
1411+
.workspace("team")
1412+
.wait_deleted("my-box", timeout, None)
1413+
.await
1414+
} else {
1415+
client.wait_deleted("my-box", timeout, None).await
1416+
}
1417+
.unwrap();
1418+
assert_eq!(state.get_calls.load(Ordering::SeqCst), 2);
1419+
}
1420+
}
1421+
1422+
#[tokio::test]
1423+
async fn wait_deleted_propagates_errors_other_than_not_found() {
1424+
for scoped in [false, true] {
1425+
for code in [tonic::Code::PermissionDenied, tonic::Code::Unavailable] {
1426+
let state = Arc::new(MockState {
1427+
get_error: Some(Status::new(code, "lookup failed")),
1428+
..Default::default()
1429+
});
1430+
let endpoint = start_mock(state.clone()).await;
1431+
let client = connect(&endpoint).await;
1432+
let timeout = std::time::Duration::from_secs(5);
1433+
let error = if scoped {
1434+
client
1435+
.workspace("team")
1436+
.wait_deleted("my-box", timeout, Some("old-id"))
1437+
.await
1438+
} else {
1439+
client.wait_deleted("my-box", timeout, Some("old-id")).await
1440+
}
1441+
.unwrap_err();
1442+
assert_eq!(error.grpc_status().unwrap().code(), code);
1443+
assert_eq!(state.get_calls.load(Ordering::SeqCst), 1);
1444+
}
1445+
}
12981446
}
12991447

13001448
#[tokio::test]

‎crates/openshell-server/src/grpc/mutation_replay.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -646,17 +646,21 @@ resource_mutation!(
646646
"CreateWorkspace",
647647
workspace::handle_create_workspace,
648648
workspace,
649-
async |_req: &CreateWorkspaceRequest, state: &ServerState, principal: &Principal| {
650-
global_scope(state, principal)
649+
async |req: &CreateWorkspaceRequest, state: &ServerState, principal: &Principal| {
650+
let mut scope = global_scope(state, principal)?;
651+
scope.name.clone_from(&req.name);
652+
Ok(scope)
651653
}
652654
);
653655
deletion_mutation!(
654656
DeleteWorkspaceRequest,
655657
DeleteWorkspaceResponse,
656658
"DeleteWorkspace",
657659
workspace::handle_delete_workspace,
658-
async |_req: &DeleteWorkspaceRequest, state: &ServerState, principal: &Principal| {
659-
global_scope(state, principal)
660+
async |req: &DeleteWorkspaceRequest, state: &ServerState, principal: &Principal| {
661+
let mut scope = global_scope(state, principal)?;
662+
scope.name.clone_from(&req.name);
663+
Ok(scope)
660664
}
661665
);
662666
resource_mutation!(

0 commit comments

Comments
 (0)