Skip to content

Commit d65c359

Browse files
feat(persistence): implement optimistic concurrency control with CAS
Add resource_version-based optimistic concurrency control to the persistence layer. Every write now requires an explicit WriteCondition (MustCreate or MatchResourceVersion), enforced at compile time by gating unconditional put/put_message behind #[cfg(test)]. - Add WriteCondition enum and put_if for conditional writes - Add update_message_cas for atomic read-modify-write operations - Add list_messages/list_messages_with_selector helpers that hydrate resource_version from authoritative DB rows - Convert all production write paths to CAS-aware methods - Gate put/put_message behind #[cfg(test)] to prevent non-CAS writes - Use structured PersistenceError::UniqueViolation matching instead of string matching for duplicate detection - Hydrate resource_version from WriteResult directly on creates, eliminating unnecessary read-after-write round trips Signed-off-by: Derek Carr <decarr@redhat.com>
1 parent f819f7d commit d65c359

29 files changed

Lines changed: 3904 additions & 395 deletions

architecture/gateway.md

Lines changed: 87 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,92 @@ 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+
**Compile-time enforcement.** The unconditional write methods `put` and
130+
`put_message` are gated behind `#[cfg(test)]`. Production code must use
131+
`put_if` with an explicit `WriteCondition` or `update_message_cas`. The
132+
compiler rejects any other write path, making non-CAS writes structurally
133+
impossible outside of tests.
134+
135+
Every write goes through one of three conditions:
136+
137+
- `MustCreate` -- insert-only. The database rejects the write with a
138+
`UniqueViolation` error if a row with that ID already exists. Handlers match
139+
on the structured `PersistenceError::UniqueViolation { .. }` variant to
140+
distinguish creation conflicts from other failures.
141+
- `MatchResourceVersion(v)` -- update-only. The database rejects the write
142+
with a `Conflict` error if the current version differs from `v`.
143+
- `Unconditional` -- test-only; not reachable in production builds.
144+
145+
**Creates.** All create paths use `MustCreate` and hydrate the response
146+
directly from the `WriteResult` returned by `put_if`, which carries the
147+
assigned `resource_version`, `created_at_ms`, and `updated_at_ms`. This
148+
eliminates a read-after-write round trip and the race window that would come
149+
with it.
150+
151+
**Updates.** The `update_message_cas` helper makes a single CAS attempt: it
152+
fetches the current object, applies a mutation closure, and writes with a
153+
`MatchResourceVersion` condition. On conflict the persistence layer returns a
154+
`Conflict` error, which gRPC handlers map to `ABORTED` status so the client
155+
(or the next watch/reconcile event) can retry with fresh state. There is no
156+
automatic retry loop.
157+
158+
The helper accepts an `expected_version` parameter that selects between two
159+
modes:
160+
161+
- **Server-driven** (`expected_version = 0`): the helper uses the version it
162+
just read from the database. Internal operations (reconciler, policy status
163+
reports, compute phase transitions) use this mode because the caller does
164+
not track versions.
165+
- **Client-driven** (`expected_version != 0`): the helper validates that the
166+
caller's version matches the current database version before applying the
167+
mutation. If they diverge it returns `Conflict` without attempting the
168+
write. Client-facing operations that carry an `expected_resource_version`
169+
field use this mode: `AttachSandboxProvider`, `DetachSandboxProvider`,
170+
`UpdateProvider`, and `UpdateConfig` (policy backfill path).
171+
172+
**Lists.** The `list_messages` and `list_messages_with_selector` helpers decode
173+
protobuf payloads from list results and hydrate `resource_version` from the
174+
authoritative database column into each decoded message, mirroring the
175+
`get_message` pattern. This ensures list responses carry correct versions
176+
without requiring callers to manually hydrate each record.
177+
178+
**Deletes.** Delete operations are not yet CAS-protected -- the delete request
179+
protos do not carry `expected_resource_version`. A `delete_if` primitive exists
180+
in the persistence layer but is not wired into gRPC handlers.
181+
182+
**Coverage.** All `ObjectMeta`-bearing message types have write-condition
183+
coverage:
184+
185+
| Type | Create | Update | List |
186+
|---|---|---|---|
187+
| Sandbox | `MustCreate` | `update_message_cas` | `list_messages` |
188+
| Provider | `MustCreate` | `update_message_cas` | `list_messages` |
189+
| ProviderProfile | `MustCreate` | (immutable) | `list_messages` |
190+
| InferenceRoute | `MustCreate` | `update_message_cas` | `list_messages` |
191+
| SandboxPolicy | scoped versioning | scoped versioning | scoped query |
192+
| Settings | `Mutex`-guarded | `Mutex`-guarded | single-row |
193+
194+
Global settings updates use a Tokio `Mutex` to serialize multi-step
195+
validation within a single gateway process, with CAS on the underlying
196+
persistence write as defense in depth. In an HA deployment with multiple
197+
gateways, the Mutex alone would be insufficient. Sandbox-scoped settings
198+
rely entirely on CAS without a Mutex.
199+
200+
The `resource_version` is surfaced to clients through `ObjectMeta` in proto
201+
responses. Database migrations backfill existing rows with version 1.
202+
116203
Policy and runtime settings are delivered together through the effective sandbox
117204
config path. A gateway-global policy can override sandbox-scoped policy. The
118205
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
@@ -2389,6 +2389,11 @@ pub async fn sandbox_get(
23892389
println!(" {} {}", "Id:".dimmed(), id);
23902390
println!(" {} {}", "Name:".dimmed(), name);
23912391
println!(" {} {}", "Phase:".dimmed(), phase_name(sandbox.phase));
2392+
println!(
2393+
" {} {}",
2394+
"Resource version:".dimmed(),
2395+
sandbox.metadata.as_ref().map_or(0, |m| m.resource_version)
2396+
);
23922397

23932398
// Display labels if present
23942399
if let Some(metadata) = &sandbox.metadata
@@ -3152,14 +3157,38 @@ pub async fn sandbox_provider_attach(
31523157
tls: &TlsOptions,
31533158
) -> Result<()> {
31543159
let mut client = grpc_client(server, tls).await?;
3155-
let response = client
3160+
3161+
// Fetch current sandbox to get resource_version for CAS
3162+
let sandbox = client
3163+
.get_sandbox(GetSandboxRequest {
3164+
name: name.to_string(),
3165+
})
3166+
.await
3167+
.into_diagnostic()?
3168+
.into_inner()
3169+
.sandbox
3170+
.ok_or_else(|| miette::miette!("sandbox not found"))?;
3171+
3172+
let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
3173+
3174+
let response = match client
31563175
.attach_sandbox_provider(AttachSandboxProviderRequest {
31573176
sandbox_name: name.to_string(),
31583177
provider_name: provider.to_string(),
3178+
expected_resource_version: resource_version,
31593179
})
31603180
.await
3161-
.into_diagnostic()?
3162-
.into_inner();
3181+
{
3182+
Ok(response) => response.into_inner(),
3183+
Err(status) if status.code() == Code::Aborted => {
3184+
return Err(miette::miette!(
3185+
"Failed to attach provider: sandbox was modified by another operation.\n\
3186+
Please retry the command."
3187+
)
3188+
.with_source_code(status.message().to_string()));
3189+
}
3190+
Err(e) => return Err(e).into_diagnostic(),
3191+
};
31633192

31643193
if response.attached {
31653194
println!(
@@ -3181,14 +3210,38 @@ pub async fn sandbox_provider_detach(
31813210
tls: &TlsOptions,
31823211
) -> Result<()> {
31833212
let mut client = grpc_client(server, tls).await?;
3184-
let response = client
3213+
3214+
// Fetch current sandbox to get resource_version for CAS
3215+
let sandbox = client
3216+
.get_sandbox(GetSandboxRequest {
3217+
name: name.to_string(),
3218+
})
3219+
.await
3220+
.into_diagnostic()?
3221+
.into_inner()
3222+
.sandbox
3223+
.ok_or_else(|| miette::miette!("sandbox not found"))?;
3224+
3225+
let resource_version = sandbox.metadata.as_ref().map_or(0, |m| m.resource_version);
3226+
3227+
let response = match client
31853228
.detach_sandbox_provider(DetachSandboxProviderRequest {
31863229
sandbox_name: name.to_string(),
31873230
provider_name: provider.to_string(),
3231+
expected_resource_version: resource_version,
31883232
})
31893233
.await
3190-
.into_diagnostic()?
3191-
.into_inner();
3234+
{
3235+
Ok(response) => response.into_inner(),
3236+
Err(status) if status.code() == Code::Aborted => {
3237+
return Err(miette::miette!(
3238+
"Failed to detach provider: sandbox was modified by another operation.\n\
3239+
Please retry the command."
3240+
)
3241+
.with_source_code(status.message().to_string()));
3242+
}
3243+
Err(e) => return Err(e).into_diagnostic(),
3244+
};
31923245

31933246
if response.detached {
31943247
println!(
@@ -3523,6 +3576,7 @@ async fn auto_create_provider(
35233576
name: exact_name.to_string(),
35243577
created_at_ms: 0,
35253578
labels: HashMap::new(),
3579+
resource_version: 0,
35263580
}),
35273581
r#type: provider_type.to_string(),
35283582
credentials: discovered.credentials.clone(),
@@ -3563,6 +3617,7 @@ async fn auto_create_provider(
35633617
name: name.clone(),
35643618
created_at_ms: 0,
35653619
labels: HashMap::new(),
3620+
resource_version: 0,
35663621
}),
35673622
r#type: provider_type.to_string(),
35683623
credentials: discovered.credentials.clone(),
@@ -3975,6 +4030,7 @@ pub async fn provider_create(
39754030
name: name.to_string(),
39764031
created_at_ms: 0,
39774032
labels: HashMap::new(),
4033+
resource_version: 0,
39784034
}),
39794035
r#type: provider_type.clone(),
39804036
credentials: credential_map,
@@ -4019,6 +4075,11 @@ pub async fn provider_get(server: &str, name: &str, tls: &TlsOptions) -> Result<
40194075
println!(" {} {}", "Id:".dimmed(), provider.object_id());
40204076
println!(" {} {}", "Name:".dimmed(), provider.object_name());
40214077
println!(" {} {}", "Type:".dimmed(), provider.r#type);
4078+
println!(
4079+
" {} {}",
4080+
"Resource version:".dimmed(),
4081+
provider.metadata.as_ref().map_or(0, |m| m.resource_version)
4082+
);
40224083
println!(
40234084
" {} {}",
40244085
"Credential keys:".dimmed(),
@@ -4475,6 +4536,7 @@ pub async fn provider_update(
44754536
name: name.to_string(),
44764537
created_at_ms: 0,
44774538
labels: HashMap::new(),
4539+
resource_version: 0,
44784540
}),
44794541
r#type: String::new(),
44804542
credentials: credential_map,
@@ -5029,6 +5091,7 @@ pub async fn sandbox_policy_set_global(
50295091
delete_setting: false,
50305092
global: true,
50315093
merge_operations: vec![],
5094+
expected_resource_version: 0,
50325095
})
50335096
.await
50345097
.into_diagnostic()?
@@ -5227,6 +5290,7 @@ pub async fn gateway_setting_set(
52275290
delete_setting: false,
52285291
global: true,
52295292
merge_operations: vec![],
5293+
expected_resource_version: 0,
52305294
})
52315295
.await
52325296
.into_diagnostic()?
@@ -5261,6 +5325,7 @@ pub async fn sandbox_setting_set(
52615325
delete_setting: false,
52625326
global: false,
52635327
merge_operations: vec![],
5328+
expected_resource_version: 0,
52645329
})
52655330
.await
52665331
.into_diagnostic()?
@@ -5295,6 +5360,7 @@ pub async fn gateway_setting_delete(
52955360
delete_setting: true,
52965361
global: true,
52975362
merge_operations: vec![],
5363+
expected_resource_version: 0,
52985364
})
52995365
.await
53005366
.into_diagnostic()?
@@ -5329,6 +5395,7 @@ pub async fn sandbox_setting_delete(
53295395
delete_setting: true,
53305396
global: false,
53315397
merge_operations: vec![],
5398+
expected_resource_version: 0,
53325399
})
53335400
.await
53345401
.into_diagnostic()?
@@ -5387,6 +5454,7 @@ pub async fn sandbox_policy_set(
53875454
delete_setting: false,
53885455
global: false,
53895456
merge_operations: vec![],
5457+
expected_resource_version: 0,
53905458
})
53915459
.await
53925460
.into_diagnostic()?;
@@ -5561,6 +5629,7 @@ pub async fn sandbox_policy_update(
55615629
delete_setting: false,
55625630
global: false,
55635631
merge_operations: plan.merge_operations,
5632+
expected_resource_version: 0,
55645633
})
55655634
.await
55665635
.into_diagnostic()?

crates/openshell-cli/tests/ensure_providers_integration.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ impl TestOpenShell {
6262
name: name.to_string(),
6363
created_at_ms: 0,
6464
labels: HashMap::new(),
65+
resource_version: 0,
6566
}),
6667
r#type: provider_type.to_string(),
6768
credentials: HashMap::new(),
@@ -326,6 +327,7 @@ impl OpenShell for TestOpenShell {
326327
name: provider_metadata.name,
327328
created_at_ms: existing_metadata.created_at_ms,
328329
labels: existing_metadata.labels,
330+
resource_version: 0,
329331
}),
330332
r#type: existing.r#type,
331333
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
@@ -20,7 +20,7 @@ use openshell_core::proto::{
2020
GetSandboxProviderEnvironmentResponse, GetSandboxRequest, HealthRequest, HealthResponse,
2121
ListProvidersRequest, ListProvidersResponse, ListSandboxProvidersRequest,
2222
ListSandboxProvidersResponse, ListSandboxesRequest, ListSandboxesResponse, Provider,
23-
ProviderProfile, ProviderResponse, RevokeSshSessionRequest, RevokeSshSessionResponse,
23+
ProviderProfile, ProviderResponse, RevokeSshSessionRequest, RevokeSshSessionResponse, Sandbox,
2424
SandboxResponse, SandboxStreamEvent, ServiceStatus, SupervisorMessage, UpdateProviderRequest,
2525
WatchSandboxRequest,
2626
};
@@ -83,9 +83,25 @@ impl OpenShell for TestOpenShell {
8383

8484
async fn get_sandbox(
8585
&self,
86-
_request: tonic::Request<GetSandboxRequest>,
86+
request: tonic::Request<GetSandboxRequest>,
8787
) -> Result<Response<SandboxResponse>, Status> {
88-
Ok(Response::new(SandboxResponse::default()))
88+
let name = request.into_inner().name;
89+
// Return a minimal sandbox with metadata for CAS operations
90+
Ok(Response::new(SandboxResponse {
91+
sandbox: Some(Sandbox {
92+
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
93+
id: format!("sb-{name}"),
94+
name,
95+
created_at_ms: 0,
96+
labels: HashMap::new(),
97+
resource_version: 1,
98+
}),
99+
spec: None,
100+
status: None,
101+
phase: 0,
102+
current_policy_version: 0,
103+
}),
104+
}))
89105
}
90106

91107
async fn list_sandboxes(
@@ -155,7 +171,7 @@ impl OpenShell for TestOpenShell {
155171
providers.push(request.provider_name.clone());
156172
true
157173
};
158-
let sandbox = openshell_core::proto::Sandbox {
174+
let sandbox = Sandbox {
159175
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
160176
name: request.sandbox_name,
161177
..Default::default()
@@ -192,7 +208,7 @@ impl OpenShell for TestOpenShell {
192208
let before_len = providers.len();
193209
providers.retain(|name| name != &request.provider_name);
194210
let detached = providers.len() != before_len;
195-
let sandbox = openshell_core::proto::Sandbox {
211+
let sandbox = Sandbox {
196212
metadata: Some(openshell_core::proto::datamodel::v1::ObjectMeta {
197213
name: request.sandbox_name,
198214
..Default::default()
@@ -447,6 +463,7 @@ impl OpenShell for TestOpenShell {
447463
name: provider_metadata.name,
448464
created_at_ms: existing_metadata.created_at_ms,
449465
labels: existing_metadata.labels,
466+
resource_version: 0,
450467
}),
451468
r#type: existing.r#type,
452469
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
@@ -85,6 +85,7 @@ impl OpenShell for TestOpenShell {
8585
name: sandbox_name,
8686
created_at_ms: 0,
8787
labels: HashMap::new(),
88+
resource_version: 0,
8889
}),
8990
phase: SandboxPhase::Provisioning as i32,
9091
..Sandbox::default()
@@ -104,6 +105,7 @@ impl OpenShell for TestOpenShell {
104105
name,
105106
created_at_ms: 0,
106107
labels: HashMap::new(),
108+
resource_version: 0,
107109
}),
108110
phase: SandboxPhase::Ready as i32,
109111
..Sandbox::default()
@@ -324,6 +326,7 @@ impl OpenShell for TestOpenShell {
324326
name: sandbox_id.trim_start_matches("id-").to_string(),
325327
created_at_ms: 0,
326328
labels: HashMap::new(),
329+
resource_version: 0,
327330
}),
328331
phase: SandboxPhase::Provisioning as i32,
329332
..Sandbox::default()

0 commit comments

Comments
 (0)