Repository navigation
feat(server): follow-ups to scoped mutation locks (#4147) #4307
Description
User Story
I run multi-replica OpenShell gateways on Kubernetes with an external PostgreSQL database. #4147 scopes the gateway's mutation locks by workspace and sandbox and bounds every lock wait. A few paths next to it still stall sandbox operations or block a restart, and I need those fixed too.
Problem Statement
Follow-ups to #4147 (merged), part of #3528. None of them is a regression from #4147:
- Provisioning-deadline expiry waits for its locks with no deadline.
reconcile_provisioning_deadlines(crates/openshell-server/src/compute/provisioning_deadline.rs) takes local S(global) S(workspace) X(sandbox) throughlock_sandbox_local_in_workspace. Keys are taken in key order, so when a sandbox's key sorts before its workspace's, the scanner holds X(sandbox) while it waits behind a provider or profile change on that workspace. That change holds the workspace key through its PostgreSQL wait (up to 10 seconds) and its guarded work, and start, stop and delete for the sandbox wait the whole time. feat(kubernetes): add template-driven warm pooling #3508 (warm pools) adds two more callers of the same deadline refresh. - The first settings write for a sandbox can outlive it. Sandbox settings are stored by sandbox name.
UpdateConfigchecksensure_sandbox_keeps_nameafter loading them, andsave_settings_recordrejects a row whose ID changed since the load. When no row exists yet, though, the insert isn't tied to the sandbox: a delete on another replica can remove the sandbox and its settings (cleanup_sandbox_owned_records) between the check and the insert, and a later sandbox with the same name inherits the new row. Main has the same race, since lifecycle deletes take no database lock. - Dangling provider references crashloop the gateway. Before feat(server): scope mutation locks and bound lock waits #4147,
DeleteProvidertook no mutation guard, so a delete racing an attach could leave a sandbox that references a missing provider.validate_provider_composition_startup_preflight(which runs when no global policy is set) has been fatal since feat(providers): make profiles authoritative #2962, so every gateway restart, on any release, then fails with "provider 'X' not found". feat(server): scope mutation locks and bound lock waits #4147 stops new dangling references but doesn't repair the ones a database already has. - The SSH host identity lock from fix(ssh): persist sandbox host identities #4094 is still fleet-wide: one advisory key (
SSH_IDENTITY_LOCK_KEY) plus a process-wide mutex held across credential-store calls. Sandbox create waits for it while holding its sandbox mutation guard, and start, restart and delete wait for it under their local sandbox lock, so a burst of creates on one replica can tie up its lock pool (4 connections by default,database_lock_max_connections). - The PostgreSQL data pool floor of 2 only counts the SSH identity lock. The provider refresh lock (
acquire_refresh_guard) also keeps a data connection for its advisory lock whileget_refresh_statetakes another from the same pool. Withdatabase_max_connectionsset to 2 to 4, a few concurrent refreshes (up to 4 per replica throughREFRESH_EXECUTORS, and the pending-deletion cleanup path takes no permit), or one refresh next to an SSH identity holder, can hold every data connection and wait for a second one until acquire times out, stalling every database request on that replica. The default of 10 isn't affected. - Provider attach validates a stale copy of the sandbox.
handle_attach_sandbox_provider(crates/openshell-server/src/grpc/sandbox.rs) resolves the sandbox before it takes the sandbox mutation guard and validates the new provider set (profiles, env keys, composed policy, credential policy) against that copy, while its CAS only appends the provider. Two attaches that send noexpected_resource_version(the Go and TypeScript SDKs allow it; the CLI pins one) both commit, and nothing validates the combined set. Detach and sandbox policy updates throughUpdateConfigvalidate the same way. Two providers that pass alone but are ambiguous together then fail the startup preflight, like a dangling reference. DeleteWorkspacetakes no mutation lock. It marks the workspaceTerminating, checks for sandboxes and deletes it, while create paths check that the workspace is active before they take their guard, so a create that passed that check can land in a workspace being deleted.- Sandbox create holds its mutation guard across middleware
validate_policyand the driver'svalidate_sandbox_create, so a few slow driver or middleware calls on one replica can use up its lock connections. feat(kubernetes): add template-driven warm pooling #3508 adds warm-pair selection, another driver call, under the same guards. feat(server): scope mutation locks and bound lock waits #4147 makes the lock pool size configurable (database_lock_max_connections), which only moves the ceiling. - Guarded work has no hold bound, and a holder that is cancelled or loses its lock session can still commit writes after its lock is released: a dropped request future releases the guard while sqlx still flushes buffered statements, and the lock session can end (failover,
pg_terminate_backend) while the holder keeps writing over data-pool connections. Main has the same property. feat(server): scope mutation locks and bound lock waits #4147 adds a hold-time histogram and warning, but no bound. - Peers serve endpoint evidence from a dead supervisor session. Get, List and Watch return the stored
endpoint_statusesas is, and only StartSandbox masks them through the session owner (project_endpoint_status). After a replica crashes, its sandboxes keep showing the dead session's last results on the other replicas until the supervisor reconnects or a replica restarts and its startup sweep resets them. The sweep skips sandboxes whose owner record is under 45 seconds old without checking that the owner is alive, so after a quick restart the evidence stays, and nothing resets it if that supervisor never comes back. Since the sweep is the only reset, startup still fails when it gives up. No server decision reads these fields; the CLI shows them. - Lock waits and timeouts during that startup sweep aren't counted. It runs before the metrics recorder is installed, so those samples go to the no-op recorder. The warnings still get logged.
- Open PRs written against the fleet-wide guard need a scope for each guarded path when they rebase, since feat(server): scope mutation locks and bound lock waits #4147 removed
sandbox_sync_guard()andruntime.sync_lock. feat(config): deliver gateway configuration over the supervisor session #4320, refactor(runtime): extract portable driver and isolation foundations #4106 and feat(policy): add managed maximum permission modes #2168 also call them. Two need a real choice:- feat(kubernetes): add template-driven warm pooling #3508 (warm pools): create can keep
sandbox_create_guards, because theallocation_claimsprimary key already fences a target across replicas. The deadline refresh paths need the workspace key (lock_sandbox_local_in_workspace), and warm-pair selection should run before the create guards. - feat(sandbox): add delegated identity for token exchange #2772 (delegated identity): extend takes
MutationScope::sandbox. Delete-credential checks references across every workspace, where onlyGlobalfits, so it should rely on its CAS fence or a per-credential lock key, and keep secret deletion outside any guard. Same-user creates now run in parallel and conflict on the credential touch, so they need a bounded retry.
- feat(kubernetes): add template-driven warm pooling #3508 (warm pools): create can keep
Impact / Why This Matters
The provisioning-deadline, SSH identity, lock pool and data pool items delay sandbox operations under contention. The settings race is rare but leaks one sandbox's settings into a newer one with the same name. A dangling provider reference blocks every gateway restart until someone edits the database by hand, which also blocks the upgrade that would prevent new ones. An unvalidated provider set can block restarts the same way, and a create that races a workspace delete leaves a sandbox in a deleted workspace. Stale endpoint evidence makes a sandbox look healthy, or failing, on peers after its session died. An open PR that rebases with Global on a hot path brings back the fleet-wide serialization #4147 removed.
Proposed Design
- Provisioning-deadline expiry gives up on a sandbox whose locks aren't free within a short deadline and picks it up on the next pass.
- Sandbox settings record the ID of the sandbox that owns them, and a row owned by a different sandbox reads as absent and gets replaced.
- The startup preflight reports sandboxes with missing provider references and lets the gateway start (those sandboxes already fail closed), or a repair step removes the missing references.
- The SSH host identity lock uses a per-sandbox key, like the mutation locks.
- Data pool advisory-lock holders (provider refresh and SSH identity) share a semaphore capped below the pool size, so a connection always stays free for their queries. Raising the PostgreSQL floor above the number of concurrent holders is the simpler alternative.
- Attach, Detach and sandbox policy updates through
UpdateConfigre-read the sandbox under its guard and validate that copy. DeleteWorkspacetakes the workspace's mutation key, and create paths check again under their guard that the workspace is active.- Sandbox create runs middleware and driver validation before it takes its guards, re-checking under them only what needs the lock.
- Once guarded sections are DB-only, guarded writes run in a transaction on the lock session with
idle_in_transaction_session_timeout(ortransaction_timeouton PostgreSQL 17+) as the hold bound, so PostgreSQL rolls back the writes and drops the locks together. - Get, List and Watch apply the StartSandbox projection, batching owner reads for List, so evidence from a session that is no longer live reads as absent. The startup sweep then only cleans up and can log and continue. The projection covers any evidence field added later, such as the one in feat(providers): report bounded token-grant failures #4427.
- The metrics recorder is installed before startup reconciliation.
- For open PRs: a sandbox scope for records a sandbox owns, a database CAS or unique key for fences across sandboxes, and no lock held across driver or secret-backend calls.
Acceptance Criteria
One PR per line.
- Lock waits and timeouts in the startup sweep show up in the lock metrics (fix(server): count startup-sweep lock waits in the lock metrics #4523).
- Provisioning-deadline expiry bounds its lock wait, with a test using a sandbox whose key sorts before its workspace's key while a provider change holds the workspace (fix(server): bound the provisioning-deadline scan's lock wait #4547).
- A settings write that races a cross-replica delete can't leave a row a later same-name sandbox inherits, with a test for the first insert.
- A database holding a dangling provider reference no longer stops the gateway from starting.
- The SSH host identity lock is scoped per sandbox, and creates for different sandboxes don't wait on each other.
- At the smallest accepted
database_max_connections, concurrent provider refreshes and SSH identity holders can't exhaust the data pool, with a PostgreSQL test. - Attach, Detach and sandbox policy updates through
UpdateConfigvalidate a copy of the sandbox read under its guard, with a test where two unpinned attaches race and only a valid combined set commits. -
DeleteWorkspaceand sandbox create can't interleave: a create either finishes before the workspace is markedTerminatingor fails, with a test. - Sandbox create runs middleware and driver validation outside its mutation guards where the invariants allow, so a slow driver or middleware call doesn't hold a lock connection.
- A guarded section can't commit after its lock is released, with a PostgreSQL test where a stalled holder's session ends at the bound, a waiter on another replica acquires, none of the holder's writes are visible, and its later commit fails.
- Get, List and Watch don't show endpoint evidence from a session that is no longer live, with a test where a replica dies and a peer serves its sandbox, and a startup sweep that gives up no longer stops the gateway.
- feat(kubernetes): add template-driven warm pooling #3508 and feat(sandbox): add delegated identity for token exchange #2772 rebase with a scope for each path that took the fleet-wide guard, and no
Globalon a hot path (done in those PRs).
Checklist
- I've reviewed existing issues and the published docs
- This is a design proposal, not a "please build this" request
Activity