Conversation
Expose per-replica supervisor sessions, pending relay capacity, relay rejections and claim latency, and outbound peer request outcomes and latency. Use bounded labels and Prometheus histograms for the new latency metrics while preserving existing summary metrics. Add an optional Helm HPA with external-database and resource validation, conservative scale-down defaults, and support for custom metrics. Keep certificate hook pods outside gateway workload selectors. Document per-pod scraping, scaling limits, upgrade behavior, and PostgreSQL connection sizing. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Combine local relay setup and outbound peer requests in one counter, labeled by operation, route, target, outcome, and gRPC status. Preserve peer latency metrics. Rename the rejection reason from global_capacity to replica_capacity to reflect the per-replica relay budget. Keep capacity limits unchanged. Update tests and documentation. Signed-off-by: divesh <dgude@nvidia.com>
Count a local relay as successful only when its supervisor claims it, the same event that answers a peer relay on the owner, and rename the outcomes to local_error and remote_error so they say where an attempt failed. Label the peer latency histogram by operation, like the routed request counter, and rename the target label to relay_kind so it does not read as the Prometheus scrape target. Rename RelayCapacity.global to per_replica, drop the per-sandbox relay capacity gauge, which no per-sandbox series can pair with, describe the latency histogram as peer-only, and restore the note that unavailable spikes are expected during rollouts. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
The openshell-server tests that need a real PostgreSQL server are ignored by default and had no shared way to run. The mutation-replay test reads its own OPENSHELL_REPLAY_TEST_DATABASE_URL, so every contributor had to provision a database by hand, and the advisory-lock tests that follow in this series need the same setup. Add mise run test:rust:postgres. It runs every ignored postgres_* test in openshell-server against OPENSHELL_TEST_POSTGRES_URL, or starts a disposable PostgreSQL container with Docker or Podman (CONTAINER_ENGINE selects one) on the image pinned by the Kubernetes e2e fixture and removes it on exit. Tests run one at a time because advisory locks are database-wide. The runner always points the legacy replay variable at the selected database, so an inherited URL cannot send that test to a different server. test:postgres-runner checks that selection with a fake cargo and needs no database or container engine. CI does not run the PostgreSQL tests; TESTING.md documents the task. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
DeleteProvider checked that no sandbox referenced the provider and then deleted the record without holding the sandbox mutation guard. Sandbox create and provider attach take that guard while they write provider references, so one of them could add a reference after the attached sandbox check passed, and the delete then removed a provider that a sandbox spec still named. Take the guard before the attached sandbox check, as provider create and update already do, so the check and the delete run against a stable set of sandbox references. A new test holds the guard, attaches the provider while the delete waits, and asserts that the delete fails with FailedPrecondition and leaves the provider in place. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
A provider credential refresh stages the minted values under new credential handles before it commits the provider update, and deletes those handles when validation or persistence fails. Two earlier returns skipped that cleanup. The minted expiry was converted to a protobuf timestamp only after staging, so an expiry outside the timestamp range failed the refresh and left the staged values in the credential driver. A failure to acquire the sandbox mutation guard returned the same way. Convert the expiry before staging anything, so a bad value fails before any handle exists, and delete the staged handles when the guard cannot be acquired. A new test refreshes a stored credential with an expiry of i64::MAX and asserts that the call fails, the provider keeps its original handles, and the credential driver holds the same number of values as before. The guard failure path has no test here, because the SQLite guard used by unit tests cannot fail. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
The cross-object mutation guard took its advisory-lock session from the 10-connection data pool and closed it on every release, so each guarded mutation opened a new database connection, and lock holders competed with their own critical sections for data connections. A lock wait that hit the 10 second lock_timeout surfaced as INTERNAL. Add a lazy, dedicated 4-connection lock pool and a new persistence::mutation_lock module for the lock key, mode and set types. Store::acquire_distributed_mutation_guard now takes a lock set and a deadline, and each lock statement sets lock_timeout to the time left before that deadline, so a cancelled acquisition never waits past it on the server. Returned connections are scrubbed with pg_advisory_unlock_all() and reused, and each return is bounded by 5 seconds so a stalled session cannot keep a pool permit. A lock statement that PostgreSQL ends with SQLSTATE 55P03 fails with a new LockTimeout error, and so does a wait for a lock connection during which every one was checked out at some point, or that started with less than a second left, too little to open one. The API returns it as UNAVAILABLE with reason MUTATION_LOCK_TIMEOUT and a 1 second retry delay, because the mutation lock was not acquired, so the request's guarded writes did not run. Only the guard acquisition classifies 55P03; any other 55P03 stays a database error. A lock connection that PostgreSQL does not open by the deadline despite at least a second to do so (refused, out of connection slots, starting up, or not answering, all of which SQLx retries silently) is a database error, not a lock timeout. Mutations still take the legacy global key exclusively, so old and new replicas keep excluding each other during a rolling upgrade. Each PostgreSQL-backed gateway pod can now open up to 14 database connections (10 data plus 4 lock) instead of 10, and the chart's autoscaling.maxReplicas comment says what its default of 4 needs. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Every cross-object mutation, and every lifecycle and supervisor update, took one process-wide mutex and, on PostgreSQL, one fleet-wide advisory lock. Unrelated sandboxes and providers therefore serialized across all replicas, and one slow critical section delayed every other mutation in the fleet. Replace that lock with hierarchical intention locks on a global key, one key per workspace and one key per sandbox, each held shared or exclusive. Global settings and policy writers and platform-scope profile writers hold the global key exclusively. Provider and workspace-scoped profile writers, including DeleteProvider and credential refresh, hold their workspace key exclusively. Sandbox writers and supervisor reports hold their sandbox key exclusively under shared global and workspace keys. Lifecycle, driver-watch and reconcile paths take only process-local keys and keep relying on compare-and-swap across replicas. Workspace and sandbox keys are domain-separated SHA-256 prefixes. Each guard takes its local keys and then its advisory locks in ascending order under one 10 second deadline, and no path nests guards. The settings mutex is removed, since the global and sandbox keys cover it. The global key keeps its legacy value, so older replicas, which hold it exclusively for every mutation, still exclude new ones during a rolling upgrade. Guards now also give up after 10 seconds on SQLite, and provider refresh reports a guard timeout as UNAVAILABLE instead of INTERNAL. A supervisor report for a missing sandbox returns NOT_FOUND. Startup endpoint-status reconciliation still holds the global key for its whole scan. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Startup endpoint-status reconciliation held the global mutation key exclusively for its whole scan. On PostgreSQL that blocked every guarded mutation on every replica until the starting gateway had walked all sandboxes, and the scan first waited behind every in-flight sandbox mutation. It also paged with an offset, so sandboxes deleted mid-scan shifted rows between pages, and a write that raced the scan (a Conflict from a lifecycle update on another replica, or a sandbox deleted after it was listed) failed gateway startup. Walk the sandboxes with keyset paging instead, and reset each candidate under its own sandbox-scoped guard, up to four at a time to match the lock pool. A candidate with a fresh shared owner is skipped before any guard is taken. Under the guard the reconciliation re-checks the owner, re-reads the sandbox, skips it when it is gone or no longer carries endpoint status, and writes against the version it just read. A Conflict or a database error at write time, which is how a row deleted after the re-read surfaces, is retried with a fresh read, at most five attempts per sandbox. Store::list_all_messages loses its only caller and is removed. Reconciliation stays fail-closed: a lock timeout, an owner lookup failure, or a sandbox that keeps changing still fails startup, so stale endpoint success is never served. User-visible change: a restarting gateway no longer stalls mutations of unrelated sandboxes and providers on its peers while it reconciles. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
When a supervisor session ends, the gateway resets that sandbox's tool server endpoint evidence unless a replacement session already owns endpoint observation. The replacement check ran only after the reset had read the sandbox and taken its sandbox mutation guard, so a reset whose supervisor had already reconnected still queued behind other mutations of that sandbox, and on PostgreSQL it used a lock-pool connection, only to find it had nothing to do. Check for a replacement first: a live local session, or a fresh owner record on a peer. When one exists, return without reading the sandbox or taking the guard. Otherwise take the guard and repeat the same check under it, because a replacement's pre-acknowledgement reset runs under the same sandbox key and resetting after it would wipe the replacement's fresh evidence. Both checks share one helper, so they cannot drift apart. When no replacement exists, the reset costs one extra owner-row read. This helps most when a replica notices a dead stream late, after the supervisor has already reconnected elsewhere (a network partition or a keepalive expiry). Deferring to a replacement leaves the old session's evidence to the replacement's pre-acknowledgement reset, which can time out on its bounded mutation lock wait. That reset's failure path removed the replacement and released its owner record but scheduled no invalidation, so the old session's success evidence could outlive both sessions. An old session that ended after its replacement registered already deferred this way, and the unguarded check now makes every disconnect that finds the replacement defer too. When the reset fails while the replacement is still current, also schedule the retried invalidation that follows a session end. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
A provider credential refresh deletes its staged handles when it cannot acquire the provider mutation guard, but that path had no test: the SQLite guard used by unit tests could not fail. Mutation locks now acquire under a deadline that tests can shorten, so the timeout path can be exercised directly. The new test shortens the lock deadline to 50 ms, holds the workspace mutation guard, and refreshes a provider that already stores an AWS access key. The refresh returns UNAVAILABLE with the MUTATION_LOCK_TIMEOUT reason, the provider keeps its original handle, which still resolves to the old key, and the credential driver holds the same number of values as before the refresh. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
A handler mutation guard can now wait up to 10 seconds for its keys and then fail with UNAVAILABLE, but nothing showed operators how long guards waited or how often they gave up, short of reading warning logs on each replica. Export openshell_server_mutation_lock_wait_seconds, a histogram of the time one guard took to acquire all of its keys (local registry plus PostgreSQL advisory locks), recorded on success, and openshell_server_mutation_lock_timeouts_total, a counter of guard acquisitions that timed out. Both carry a bounded scope label: global, workspace or sandbox. The platform scope, whose workspace is empty, counts as global. The histogram uses the same 1 ms to 15 s buckets as the relay and peer histograms, and the timeout counter starts at 0 for every scope, so rate() works on an idle replica. The timeout warning now also logs the scope. A guard that fails for another reason, such as a lock connection PostgreSQL did not open, is not counted as a timeout and logs a "mutation lock acquisition failed" warning instead. Lifecycle, driver-watch and reconcile paths take only process-local locks and never time out, so they are not recorded. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
The scoped mutation locks were only unit-tested on SQLite, where the database layer is a no-op. Add ignored postgres_* tests that run the advisory-lock layer against a real PostgreSQL through mise run test:rust:postgres, each in its own disposable schema (TestSchema). persistence/mutation_lock_pg_tests.rs uses two stores on one database as two replicas. It covers disjoint sandbox scopes holding at once, a workspace key blocking only sandboxes in that workspace, the global key blocking every scope, and exclusion in both directions against a raw holder of the legacy global key, whose 55P03 stays a database error outside a guard acquisition. It checks that timed-out and cancelled acquisitions release the keys they already took, that a released lock connection returns to the pool without residual locks, that a release stalled on the network closes its session and the pool recovers, and that the lock pool never grows past its size. It also checks that a full lock pool times out as lock contention, while a lock connection that PostgreSQL never answers fails as a database error. compute/mutation_guard.rs runs the random scope mix across two PostgreSQL runtimes, checks that a by-id guard waits for a workspace writer on the other replica, and measures a paced burst of 500 reconnects 12 ms apart into one replica, failing when the p99 lock wait reaches a fifth of the lock timeout. The tests add test-only helpers: the lock pool's size and idle count and the backend pid of a held lock session. Nothing changes outside tests. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
In the High Availability guide, size PostgreSQL for 14 connections per gateway pod, 10 for data and 4 for the new lock pool, so three or more replicas now exceed what the PostgreSQL default leaves, and keep headroom for lock sessions that cancelled requests leave behind. Explain which changes lock a sandbox, a workspace, or the whole fleet, and that a lock wait over 10 seconds returns a retryable UNAVAILABLE with reason MUTATION_LOCK_TIMEOUT. A new upgrade subsection says to raise max_connections to the 14-per-pod size before upgrading, because the rollout already runs new pods at that count, and warns that older replicas keep serializing every mutation until the rollout finishes. Monitor Capacity gains the lock metrics and their queries, and names slow driver, credential, middleware, or profile source calls as another cause of lock waits. List the lock wait histogram and timeout counter, with their scope label, on the Gateway Metrics page. The counter also counts background work and leaves out lock connections that PostgreSQL does not open although at least a second of the wait remained. In the cluster debugging skill, replace the single advisory-lock paragraph with the lock levels and pool, what each timeout detail means, how a lock connection that PostgreSQL does not open shows up, when terminating a lock backend is safe, a pg_locks query that shows holders and waiters, and how to spot the global key. The per-pod greps also read the timeout counter, and new rows cover lock timeouts and lock connections that PostgreSQL does not open. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Scope gateway mutation locks by workspace and sandbox and bound every lock wait, so a burst of supervisor reconnects (rollout, scale-down, redirects) no longer queues behind one fleet-wide lock.
Related Issue
Part of #3528. Stacked on #3978: the first three commits are #3978's and drop out once it merges, so review starts at
test(server): add a local PostgreSQL test runner.Changes
sync_lockand the single fleet-wide advisory key with global, workspace and sandbox lock scopes (shared/exclusive) held on a dedicated 4-connection PostgreSQL lock pool. Replicas still on the old release keep taking the global key, so a mixed-version rollout stays serialized.UNAVAILABLEwith reasonMUTATION_LOCK_TIMEOUTand retry info. A lock connection PostgreSQL never opens returnsINTERNAL, so it doesn't read as contention.DeleteProviderholds the mutation guard while it deletes, and staged provider credentials are released when a refresh fails early. On kind, the delete race left a dangling provider reference that crashlooped every gateway at startup; cleaning up references that already exist is follow-up work.mise run test:rust:postgresrunner (not wired into CI), and document lock-pool sizing. Each pod now opens up to 14 PostgreSQL connections instead of 10, so raisemax_connectionsbefore upgrading.Testing
mise run pre-commit, the fullmise run test, andmise run docs:build:strictpass. Every commit passes clippy and the server tests.mise run test:rust:postgres: 16 tests covering scope exclusion, interop with the old global key, cancellation, pool bounds, connection failures and a reconnect burst.Checklist
🤖 Generated with Claude Code