Skip to content

Commit 4931e83

Browse files
feat(persistence): implement optimistic concurrency control with CAS
Adds Compare-And-Swap (CAS) based optimistic concurrency control to prevent lost updates in concurrent modification scenarios. This implements client-driven CAS for update operations and proper atomic create protection. Database Changes: - Add resource_version field to ObjectMeta proto message - Add resource_version column to objects table (migration 005) - Implement update_message_cas for single-attempt versioned updates - Add WriteCondition::MatchResourceVersion for CAS enforcement - Add PersistenceError::Conflict for version mismatch detection Protected Operations: - AttachSandboxProvider: uses client-driven CAS with expected_resource_version parameter - DetachSandboxProvider: uses client-driven CAS with expected_resource_version parameter - UpdateProvider: extracts resource_version from Provider.metadata, validates against current version - UpdateConfig: uses client-driven CAS for policy backfill path - CreateProvider: uses WriteCondition::MustCreate for atomic creation - SSH session operations: proper CAS protection CAS Modes: - Client-driven (expected_version > 0): Client fetches resource, uses its version for update. Conflict returns ABORTED status. - Server-driven (expected_version = 0): Server uses current DB version. Used for internal operations. CLI Changes: - Update attach/detach operations to fetch sandbox first and use its resource_version for CAS protection - Add clear error messages for ABORTED status on CAS conflicts - Add expected_resource_version: 0 to all UpdateConfig requests Testing: - 12 integration tests for concurrent modification scenarios - Tests verify ABORTED status on version conflicts - Coverage for all protected operations Documentation: - Update architecture/gateway.md with CAS design and semantics - Document expected_version parameter modes - List all client-driven CAS operations Signed-off-by: Derek Carr <decarr@redhat.com>
1 parent 52c7757 commit 4931e83

29 files changed

Lines changed: 3750 additions & 286 deletions

architecture/gateway.md

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ The storage schema is intentionally narrow:
8383
| `version` | Optional monotonically increasing version for scoped records. |
8484
| `status` | Optional workflow state for records such as policy revisions or draft policy chunks. |
8585
| `dedup_key` and `hit_count` | Optional policy-advisor fields for coalescing repeated observations. |
86+
| `resource_version` | Monotonically increasing counter for optimistic concurrency control. Incremented atomically on each update. |
8687
| `payload` | Prost-encoded protobuf payload for the full domain object. |
8788
| `created_at_ms` and `updated_at_ms` | Gateway timestamps used for ordering and list output. |
8889
| `labels` | JSON object carrying Kubernetes-style object labels for filtering and organization. |
@@ -113,6 +114,57 @@ default WAL journal mode), which mirror the same sensitive contents.
113114
Persisted state includes sandboxes, providers, SSH sessions, policy revisions,
114115
settings, inference configuration, and deployment records.
115116

117+
### Optimistic Concurrency (CAS)
118+
119+
Every object row carries a `resource_version` that the database increments
120+
atomically on each write. Concurrent mutations use compare-and-swap (CAS): the
121+
writer reads the current version, applies changes, and writes back with a
122+
`WHERE resource_version = <expected>` guard. If another writer updated the row
123+
in between, the guard fails and the caller receives a `Conflict` error.
124+
125+
This matters for HA deployments where multiple gateway replicas share the same
126+
Postgres database, and for single-node deployments where concurrent gRPC
127+
handlers or the reconciler mutate the same sandbox.
128+
129+
**When to use CAS** -- any mutation that merges caller-supplied fields into an
130+
existing object:
131+
132+
- Provider credential and config updates (merge maps).
133+
- Sandbox provider attach/detach (append/remove from a list).
134+
- Policy version bumps and draft operations.
135+
- Compute status updates (sandbox phase transitions and reconciliation).
136+
137+
**When CAS is not needed** -- create operations that generate a unique ID
138+
(conflicts are caught by the primary key constraint), unconditional deletes,
139+
and idempotent overwrites where the full payload is self-contained.
140+
141+
The `update_message_cas` helper makes a single CAS attempt: it fetches the
142+
current object, applies a mutation closure, and writes with a
143+
`MatchResourceVersion` condition. On conflict the persistence layer returns a
144+
`Conflict` error, which gRPC handlers map to `ABORTED` status so clients can
145+
read fresh state and retry.
146+
147+
The helper accepts an `expected_version` parameter that selects between two
148+
modes:
149+
150+
- **Server-driven** (`expected_version = 0`): the helper uses the version it
151+
just read from the database. Internal operations (reconciler, policy status
152+
reports, compute phase transitions) use this mode because the caller does
153+
not track versions.
154+
- **Client-driven** (`expected_version != 0`): the helper validates that the
155+
caller's version matches the current database version before applying the
156+
mutation. If they diverge it returns `Conflict` without attempting the
157+
write. Client-facing operations that carry an `expected_resource_version`
158+
field use this mode: `AttachSandboxProvider`, `DetachSandboxProvider`,
159+
`UpdateProvider`, and `UpdateConfig` (policy backfill path).
160+
161+
Settings updates are an exception: they use a Tokio `Mutex` instead of CAS
162+
because settings operations require multi-step validation that is simpler under
163+
an exclusive lock than within a CAS write.
164+
165+
The `resource_version` is surfaced to clients through `ObjectMeta` in proto
166+
responses. Database migrations backfill existing rows with version 1.
167+
116168
Policy and runtime settings are delivered together through the effective sandbox
117169
config path. A gateway-global policy can override sandbox-scoped policy. The
118170
sandbox supervisor polls for config revisions and hot-reloads dynamic policy

crates/openshell-cli/src/run.rs

Lines changed: 75 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2279,6 +2279,11 @@ pub async fn sandbox_get(
22792279
println!(" {} {}", "Id:".dimmed(), id);
22802280
println!(" {} {}", "Name:".dimmed(), name);
22812281
println!(" {} {}", "Phase:".dimmed(), phase_name(sandbox.phase));
2282+
println!(
2283+
" {} {}",
2284+
"Resource version:".dimmed(),
2285+
sandbox.metadata.as_ref().map_or(0, |m| m.resource_version)
2286+
);
22822287

22832288
// Display labels if present
22842289
if let Some(metadata) = &sandbox.metadata
@@ -2888,14 +2893,38 @@ pub async fn sandbox_provider_attach(
28882893
tls: &TlsOptions,
28892894
) -> Result<()> {
28902895
let mut client = grpc_client(server, tls).await?;
2891-
let response = client
2896+
2897+
// Fetch current sandbox to get resource_version for CAS
2898+
let sandbox = client
2899+
.get_sandbox(GetSandboxRequest {
2900+
name: name.to_string(),
2901+
})
2902+
.await
2903+
.into_diagnostic()?
2904+
.into_inner()
2905+
.sandbox
2906+
.ok_or_else(|| miette::miette!("sandbox not found"))?;
2907+
2908+
let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
2909+
2910+
let response = match client
28922911
.attach_sandbox_provider(AttachSandboxProviderRequest {
28932912
sandbox_name: name.to_string(),
28942913
provider_name: provider.to_string(),
2914+
expected_resource_version: resource_version,
28952915
})
28962916
.await
2897-
.into_diagnostic()?
2898-
.into_inner();
2917+
{
2918+
Ok(response) => response.into_inner(),
2919+
Err(status) if status.code() == Code::Aborted => {
2920+
return Err(miette::miette!(
2921+
"Failed to attach provider: sandbox was modified by another operation.\n\
2922+
Please retry the command."
2923+
)
2924+
.with_source_code(status.message().to_string()));
2925+
}
2926+
Err(e) => return Err(e).into_diagnostic(),
2927+
};
28992928

29002929
if response.attached {
29012930
println!(
@@ -2917,14 +2946,38 @@ pub async fn sandbox_provider_detach(
29172946
tls: &TlsOptions,
29182947
) -> Result<()> {
29192948
let mut client = grpc_client(server, tls).await?;
2920-
let response = client
2949+
2950+
// Fetch current sandbox to get resource_version for CAS
2951+
let sandbox = client
2952+
.get_sandbox(GetSandboxRequest {
2953+
name: name.to_string(),
2954+
})
2955+
.await
2956+
.into_diagnostic()?
2957+
.into_inner()
2958+
.sandbox
2959+
.ok_or_else(|| miette::miette!("sandbox not found"))?;
2960+
2961+
let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
2962+
2963+
let response = match client
29212964
.detach_sandbox_provider(DetachSandboxProviderRequest {
29222965
sandbox_name: name.to_string(),
29232966
provider_name: provider.to_string(),
2967+
expected_resource_version: resource_version,
29242968
})
29252969
.await
2926-
.into_diagnostic()?
2927-
.into_inner();
2970+
{
2971+
Ok(response) => response.into_inner(),
2972+
Err(status) if status.code() == Code::Aborted => {
2973+
return Err(miette::miette!(
2974+
"Failed to detach provider: sandbox was modified by another operation.\n\
2975+
Please retry the command."
2976+
)
2977+
.with_source_code(status.message().to_string()));
2978+
}
2979+
Err(e) => return Err(e).into_diagnostic(),
2980+
};
29282981

29292982
if response.detached {
29302983
println!(
@@ -3259,6 +3312,7 @@ async fn auto_create_provider(
32593312
name: exact_name.to_string(),
32603313
created_at_ms: 0,
32613314
labels: HashMap::new(),
3315+
resource_version: 0,
32623316
}),
32633317
r#type: provider_type.to_string(),
32643318
credentials: discovered.credentials.clone(),
@@ -3299,6 +3353,7 @@ async fn auto_create_provider(
32993353
name: name.clone(),
33003354
created_at_ms: 0,
33013355
labels: HashMap::new(),
3356+
resource_version: 0,
33023357
}),
33033358
r#type: provider_type.to_string(),
33043359
credentials: discovered.credentials.clone(),
@@ -3711,6 +3766,7 @@ pub async fn provider_create(
37113766
name: name.to_string(),
37123767
created_at_ms: 0,
37133768
labels: HashMap::new(),
3769+
resource_version: 0,
37143770
}),
37153771
r#type: provider_type.clone(),
37163772
credentials: credential_map,
@@ -3755,6 +3811,11 @@ pub async fn provider_get(server: &str, name: &str, tls: &TlsOptions) -> Result<
37553811
println!(" {} {}", "Id:".dimmed(), provider.object_id());
37563812
println!(" {} {}", "Name:".dimmed(), provider.object_name());
37573813
println!(" {} {}", "Type:".dimmed(), provider.r#type);
3814+
println!(
3815+
" {} {}",
3816+
"Resource version:".dimmed(),
3817+
provider.metadata.as_ref().map_or(0, |m| m.resource_version)
3818+
);
37583819
println!(
37593820
" {} {}",
37603821
"Credential keys:".dimmed(),
@@ -4211,6 +4272,7 @@ pub async fn provider_update(
42114272
name: name.to_string(),
42124273
created_at_ms: 0,
42134274
labels: HashMap::new(),
4275+
resource_version: 0,
42144276
}),
42154277
r#type: String::new(),
42164278
credentials: credential_map,
@@ -4765,6 +4827,7 @@ pub async fn sandbox_policy_set_global(
47654827
delete_setting: false,
47664828
global: true,
47674829
merge_operations: vec![],
4830+
expected_resource_version: 0,
47684831
})
47694832
.await
47704833
.into_diagnostic()?
@@ -4963,6 +5026,7 @@ pub async fn gateway_setting_set(
49635026
delete_setting: false,
49645027
global: true,
49655028
merge_operations: vec![],
5029+
expected_resource_version: 0,
49665030
})
49675031
.await
49685032
.into_diagnostic()?
@@ -4997,6 +5061,7 @@ pub async fn sandbox_setting_set(
49975061
delete_setting: false,
49985062
global: false,
49995063
merge_operations: vec![],
5064+
expected_resource_version: 0,
50005065
})
50015066
.await
50025067
.into_diagnostic()?
@@ -5031,6 +5096,7 @@ pub async fn gateway_setting_delete(
50315096
delete_setting: true,
50325097
global: true,
50335098
merge_operations: vec![],
5099+
expected_resource_version: 0,
50345100
})
50355101
.await
50365102
.into_diagnostic()?
@@ -5065,6 +5131,7 @@ pub async fn sandbox_setting_delete(
50655131
delete_setting: true,
50665132
global: false,
50675133
merge_operations: vec![],
5134+
expected_resource_version: 0,
50685135
})
50695136
.await
50705137
.into_diagnostic()?
@@ -5123,6 +5190,7 @@ pub async fn sandbox_policy_set(
51235190
delete_setting: false,
51245191
global: false,
51255192
merge_operations: vec![],
5193+
expected_resource_version: 0,
51265194
})
51275195
.await
51285196
.into_diagnostic()?;
@@ -5297,6 +5365,7 @@ pub async fn sandbox_policy_update(
52975365
delete_setting: false,
52985366
global: false,
52995367
merge_operations: plan.merge_operations,
5368+
expected_resource_version: 0,
53005369
})
53015370
.await
53025371
.into_diagnostic()?

crates/openshell-cli/tests/ensure_providers_integration.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,7 @@ impl TestOpenShell {
113113
name: name.to_string(),
114114
created_at_ms: 0,
115115
labels: HashMap::new(),
116+
resource_version: 0,
116117
}),
117118
r#type: provider_type.to_string(),
118119
credentials: HashMap::new(),
@@ -377,6 +378,7 @@ impl OpenShell for TestOpenShell {
377378
name: provider_metadata.name,
378379
created_at_ms: existing_metadata.created_at_ms,
379380
labels: existing_metadata.labels,
381+
resource_version: 0,
380382
}),
381383
r#type: existing.r#type,
382384
credentials: merge(existing.credentials, provider.credentials),

crates/openshell-cli/tests/provider_commands_integration.rs

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ use openshell_core::proto::{
1515
HealthRequest, HealthResponse, ListProvidersRequest, ListProvidersResponse,
1616
ListSandboxProvidersRequest, ListSandboxProvidersResponse, ListSandboxesRequest,
1717
ListSandboxesResponse, Provider, ProviderProfile, ProviderResponse, RevokeSshSessionRequest,
18-
RevokeSshSessionResponse, SandboxResponse, SandboxStreamEvent, ServiceStatus,
18+
RevokeSshSessionResponse, Sandbox, SandboxResponse, SandboxStreamEvent, ServiceStatus,
1919
SupervisorMessage, UpdateProviderRequest, WatchSandboxRequest,
2020
};
2121
use openshell_core::{ObjectId, ObjectName};
@@ -111,9 +111,25 @@ impl OpenShell for TestOpenShell {
111111

112112
async fn get_sandbox(
113113
&self,
114-
_request: tonic::Request<GetSandboxRequest>,
114+
request: tonic::Request<GetSandboxRequest>,
115115
) -> Result<Response<SandboxResponse>, Status> {
116-
Ok(Response::new(SandboxResponse::default()))
116+
let name = request.into_inner().name;
117+
// Return a minimal sandbox with metadata for CAS operations
118+
Ok(Response::new(SandboxResponse {
119+
sandbox: Some(Sandbox {
120+
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
121+
id: format!("sb-{name}"),
122+
name,
123+
created_at_ms: 0,
124+
labels: HashMap::new(),
125+
resource_version: 1,
126+
}),
127+
spec: None,
128+
status: None,
129+
phase: 0,
130+
current_policy_version: 0,
131+
}),
132+
}))
117133
}
118134

119135
async fn list_sandboxes(
@@ -183,7 +199,7 @@ impl OpenShell for TestOpenShell {
183199
providers.push(request.provider_name.clone());
184200
true
185201
};
186-
let sandbox = openshell_core::proto::Sandbox {
202+
let sandbox = Sandbox {
187203
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
188204
name: request.sandbox_name,
189205
..Default::default()
@@ -220,7 +236,7 @@ impl OpenShell for TestOpenShell {
220236
let before_len = providers.len();
221237
providers.retain(|name| name != &request.provider_name);
222238
let detached = providers.len() != before_len;
223-
let sandbox = openshell_core::proto::Sandbox {
239+
let sandbox = Sandbox {
224240
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
225241
name: request.sandbox_name,
226242
..Default::default()
@@ -475,6 +491,7 @@ impl OpenShell for TestOpenShell {
475491
name: provider_metadata.name,
476492
created_at_ms: existing_metadata.created_at_ms,
477493
labels: existing_metadata.labels,
494+
resource_version: 0,
478495
}),
479496
r#type: existing.r#type,
480497
credentials: merge(existing.credentials, provider.credentials),

crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ impl OpenShell for TestOpenShell {
121121
name: sandbox_name,
122122
created_at_ms: 0,
123123
labels: HashMap::new(),
124+
resource_version: 0,
124125
}),
125126
phase: SandboxPhase::Provisioning as i32,
126127
..Sandbox::default()
@@ -140,6 +141,7 @@ impl OpenShell for TestOpenShell {
140141
name,
141142
created_at_ms: 0,
142143
labels: HashMap::new(),
144+
resource_version: 0,
143145
}),
144146
phase: SandboxPhase::Ready as i32,
145147
..Sandbox::default()
@@ -354,6 +356,7 @@ impl OpenShell for TestOpenShell {
354356
name: sandbox_id.trim_start_matches("id-").to_string(),
355357
created_at_ms: 0,
356358
labels: HashMap::new(),
359+
resource_version: 0,
357360
}),
358361
phase: SandboxPhase::Provisioning as i32,
359362
..Sandbox::default()

crates/openshell-cli/tests/sandbox_name_fallback_integration.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ impl OpenShell for TestOpenShell {
119119
name,
120120
created_at_ms: 0,
121121
labels: std::collections::HashMap::new(),
122+
resource_version: 0,
122123
}),
123124
..Default::default()
124125
}),

crates/openshell-core/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ pub mod settings;
2323

2424
pub use config::{ComputeDriverKind, Config, OidcConfig, TlsConfig};
2525
pub use error::{ComputeDriverError, Error, Result};
26-
pub use metadata::{ObjectId, ObjectLabels, ObjectName};
26+
pub use metadata::{GetResourceVersion, ObjectId, ObjectLabels, ObjectName, SetResourceVersion};
2727

2828
/// Build version string derived from git metadata.
2929
///

0 commit comments

Comments
 (0)