feat(server): scope mutation locks and bound lock waits by EmilienM · Pull Request #4147 · NVIDIA/OpenShell

EmilienM · 2026-10-02T23:38:00Z

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

Closes #4501. Part of #3528. Follows #3978, now merged. Carries #2828 by @bjw123. Follow-ups are tracked in #4307.

Changes

Replace the process-wide sync_lock and the single fleet-wide advisory key with global, workspace and sandbox lock scopes (shared/exclusive) held on a dedicated PostgreSQL lock pool. Replicas still on the old release keep taking the global key, so a mixed-version rollout stays serialized. Debug builds check the lock ordering rules before every wait.
Bound lock waits at 10 seconds. A timeout returns UNAVAILABLE with reason MUTATION_LOCK_TIMEOUT and retry info. A lock connection PostgreSQL never opens returns INTERNAL, so it doesn't read as contention.
Reconcile endpoint status at startup one sandbox at a time, retrying lock timeouts for up to about 40 seconds so one long holder can't crash a starting replica, and skip the endpoint-status lock for a superseded supervisor session.
Sandbox settings are stored by sandbox name, which a per-sandbox lock doesn't protect once another sandbox reuses the name. A settings update now fails with NOT_FOUND or ABORTED instead of writing into the newer sandbox's settings.
Fix two races that exist on main, each in its own commit: DeleteProvider holds 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.
Make both pool sizes configurable: feat(server): make the database pool ceiling configurable #2828's commit for the data pool (database_max_connections) and a matching database_lock_max_connections, with flag and env forms. Helm sets both through gatewayConfig. Defaults stay 10 and 4 (PostgreSQL needs at least 2 data connections), so each pod opens up to 14 PostgreSQL connections instead of 10; raise max_connections before upgrading.
Export lock wait, timeout, error and hold-time metrics plus lock connections in use, add PostgreSQL integration tests run by mise run test:rust:postgres and a Branch Checks job, and document pool sizing. The reconnect-burst capacity check runs separately with mise run test:rust:postgres:bench, outside CI.

Testing

On the tip (rebased on main), mise run pre-commit and mise run docs:build:strict pass, mise run rust:lint, the Helm tests and the openshell-server tests pass, and every commit builds with its tests.
mise run test:rust:postgres: 18 tests covering scope exclusion, interop with the old global key, cancellation, pool bounds and gauges, connection failures and a stalled holder. mise run test:rust:postgres:bench passes on an idle machine.
kind with external PostgreSQL: the HA suite passed 3/3, the lock metrics showed up in Prometheus with zero timeouts, and a mixed fleet (old and new replicas serving together through upgrade, rollback and upgrade) ran about 11,000 mutations with no inconsistencies and no lock timeouts. These runs predate the rebases onto main; CI's HA E2E covers the current tip.

Checklist

Follows Conventional Commits
Commits are signed off (DCO)
Unit and PostgreSQL integration tests added
Operator documentation and the cluster debug skill updated

copy-pr-bot · 2026-10-02T23:38:05Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

github-actions · 2026-10-06T18:43:49Z

Label test:e2e applied, but pull-request/4147 does not exist yet. A maintainer needs to comment /ok to test 71b9c5294ec18ced30b7179b177adeac54b3588f to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

github-actions · 2026-10-06T18:43:50Z

Label test:e2e-kubernetes applied, but pull-request/4147 does not exist yet. A maintainer needs to comment /ok to test 71b9c5294ec18ced30b7179b177adeac54b3588f to mirror this PR. Once the mirror exists, re-apply the label or re-run Branch E2E Checks from the Actions tab.

johntmyers · 2026-10-06T18:43:55Z

/ok to test 71b9c52

johntmyers · 2026-10-06T18:45:49Z

gator-agent

PR Review Status

The independent review of scoped mutation locks, bounded waits, and provider cleanup found no blocking findings. Current-head Branch Checks, Helm Lint, Trivy Changes, and E2E workflows are queued or running; Gator will monitor their results.

Blocking findings: None.
Carried findings: None.
Non-blocking suggestions: None.

Gator metadata

Validation: Implements the scoped mutation-lock work in feat(ha): add production scaling signals and graceful gateway redistribution #3528 and follows merged feat(server): add gateway capacity metrics and optional HPA #3978; no active duplicate found.
Docs: Fern HA connection sizing, mixed-version upgrades, lock metrics, and API error guidance updated; cluster debug skill updated.
Checks: DCO passes; no merge conflicts. Branch Checks (37513563884), Helm Lint (37513564067), and Trivy Changes (37512710449) are dispatched for the current head.
E2E: test:e2e and test:e2e-kubernetes applied. Branch E2E Checks (37513564920) is running after /ok to test refreshed the mirror; labels were present before dispatch. No rerun needed.
Local validation: PostgreSQL test-runner database selection test and diff whitespace check passed. Full Rust/PostgreSQL tests were not run locally because this sandbox lacks the toolchain and container engine.
Head SHA: 71b9c5294ec18ced30b7179b177adeac54b3588f
Base SHA: 8760396975f4b0f13cd6fd88ac1a46342ee84343
Merge base SHA: 8760396975f4b0f13cd6fd88ac1a46342ee84343
Patch ID: 10a548689cc4281ba946a4f7a503950e90537704
Gator payload: 10
Review mode: initial
Previous reviewed SHA: none
Review budget exhausted: no
Maintainer decision required: no
Next state: gator:watch-pipeline

johntmyers · 2026-10-06T22:15:05Z

/ok to test 51822ba

johntmyers · 2026-10-06T22:16:41Z

gator-agent

PR Review Status

The independent follow-up review found no blocking findings in the author’s changes across the rebase, including settings row identity checks, provider-name validation, SSH identity lock-pool separation, and lifecycle lock adaptations. The stale test mirror has been refreshed, and current-head Branch Checks, Helm Lint, Trivy Changes, and E2E workflows are running; Gator will monitor their results.

Blocking findings: None.
Carried findings: None.

Gator metadata

Validation: Continues the scoped mutation-lock work in feat(ha): add production scaling signals and graceful gateway redistribution #3528 and follows merged feat(server): add gateway capacity metrics and optional HPA #3978.
Docs: Fern HA guidance and the cluster debug skill cover the SSH identity lock behavior and connection sizing.
Checks: DCO passes; no merge conflicts. Branch Checks (37539382397), Helm Lint (37539382408), and Trivy Changes (37539113467) are running for the current head. Trivy workflow execution was approved after a narrowly scoped sandbox policy update.
E2E: test:e2e and test:e2e-kubernetes were present before mirror refresh. Branch E2E Checks (37539383455) is running; no label-help rerun request was posted.
Local validation: PostgreSQL test-runner database selection test and diff whitespace check passed. Full Rust/PostgreSQL tests could not run locally because this sandbox lacks the Rust toolchain and container engine.
Head SHA: 51822ba9c96ae3fad3cf94761e3f9f9a24dd5a90
Base SHA: 9a6148fc988a5e7a20a43805a52fdfaad766f8ae
Merge base SHA: 360c5a02cba5b9ee62f24e2528ab1ea2aead9b6f
Patch ID: c19aea2c1d6810b73eaed1fc917ee38092fff512
Gator payload: 10
Review mode: follow_up
Previous reviewed SHA: 71b9c5294ec18ced30b7179b177adeac54b3588f
Review budget exhausted: no
Maintainer decision required: no
Next state: gator:watch-pipeline

johntmyers · 2026-10-07T16:40:48Z

/ok to test c00b632

The mutation lock ordering rules were prose only, so a nested guard on a different key passed every test and could only deadlock under contention. Debug builds now register each held guard with the Tokio task (or the thread, outside a task) that acquired it, and panic before waiting when a local key, a blocking lifecycle gate or the SSH identity lock would break rules 1, 4 or 5. Startup endpoint-status reconciliation and the tests that hold a guard while driving guarded code run under lock_order::branch(), which marks deliberate concurrency within one task. Release builds compile the check out. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

When a replacement supervisor session failed its endpoint-status reset, the spawned cleanup task kept the session lifetime guard through retry_endpoint_status_after_supervisor_disconnect. That retry only ends once the reset is durable, so a gateway shutdown during a database or lock outage waited the full 10 s and reported ownership cleanup as incomplete. The guard now covers only the lifecycle demotion, as in finish_supervisor_session, and a SQLite test checks that shutdown completes while the retry is still blocked. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

Platform-scope provider profile writers (import, update, delete) pass an empty workspace on purpose and need X(global), because they validate sandboxes in every workspace. MutationScope::profiles() now names that escalation and picks Global for the platform scope, instead of relying on MutationScope::Workspace("") to mean it. An empty workspace name now trips a debug assertion, and release builds keep X(global) as a fail-safe that over-locks rather than under-locks. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

LocalMutationLocks swept its whole table on every key lookup, so a burst of N concurrent acquisitions on one replica cost O(N^2) under a blocking mutex. The table is now swept only when a new key grows it past the larger of 64 and twice the live entries left by the last sweep, which makes acquisition amortized O(1). Released entries, including those of cancelled waiters, stay bounded at about twice the peak live set. The lifecycle gate registry keeps its per-call sweep. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

Both stores hardcoded their pool ceiling: 10 connections for Postgres, 5 for on-disk SQLite. One pool is shared by every database-backed RPC, so that number is also the gateway's ceiling on concurrent database work. Once every connection is checked out, callers queue on acquire and sqlx logs "time to acquire exceeded slow threshold"; sandbox creates then time out and are retried, which adds load rather than shedding it. The right ceiling is deployment-specific — it depends on how many gateway replicas share the database and what max_connections the server itself allows — so it cannot be a single number baked into the binary. Expose it on the same config surface as the rest of the gateway's settings: --db-max-connections, OPENSHELL_DB_MAX_CONNECTIONS, and the TOML key database_max_connections, resolved in that precedence order. Helm deployments set the key through gatewayConfig. Omitting it keeps each backend's previous value, so existing deployments do not move. Values below 1 are rejected rather than silently replaced by the default: a zero pool would block every acquire, and quietly ignoring a typo would reproduce the ceiling the operator is trying to lift. An in-memory SQLite database stays pinned to one connection, since the database lives inside that connection. Refs: NVIDIA#2561 Signed-off-by: Bryce Wilkinson <22760097+bjw123@users.noreply.github.com> (cherry picked from commit 33517b3) [emacchi: dropped the server.dbMaxConnections Helm value; operators set the key through gatewayConfig] Signed-off-by: Emilien Macchi <emacchi@redhat.com>

The mutation lock pool was fixed at 4 connections per replica. That number rests on one reconnect-burst bench rather than scale testing, so a gateway that serves many more sandboxes needs a way to raise it without a rebuild. Add --db-lock-max-connections, OPENSHELL_DB_LOCK_MAX_CONNECTIONS and the database_lock_max_connections gateway.toml key next to the data pool knob, resolved in the same order; Helm deployments set it through gatewayConfig. The default stays 4, zero is rejected, SQLite ignores it with a warning, and the gateway logs both pool sizes at startup. PostgreSQL now needs at least 2 data connections, because the SSH identity lock keeps one while it queries the pool. The connection sizing docs, the HA guide formula and the debug skill now count data plus lock connections. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

A mutation lock acquisition that failed without timing out, such as a lock connection PostgreSQL never opened or a failed lock statement, only logged a warning, and readiness stays healthy because it pings the data pool. Count those failures in openshell_server_mutation_lock_errors_total{scope}, kept apart from timeouts because they point at PostgreSQL capacity or connectivity rather than contention. Export openshell_server_mutation_lock_connections_in_use as a gauge slot held by each checked-out lock connection, next to openshell_server_mutation_lock_connections_capacity, which reads the configured lock pool size; both exist only with PostgreSQL. The metrics catalog, the HA guide's signals and PromQL examples, and the debug skill cover the new series. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

Nothing bounds how long a mutation guard is held, so a holder stuck in a compute driver, credential backend or middleware call keeps its scope locked while waiters time out. Each guard now records its hold in openshell_server_mutation_lock_hold_seconds{scope} when it is released, and logs a warning when the hold outlasted the lock wait timeout, so operators can find the replica and scope behind those timeouts. A PostgreSQL test with a stalled holder pins the current contract: waiters on its sandbox, workspace and global scopes fail by their deadline, other scopes proceed, the holder keeps its key, and a full lock pool on one replica leaves the other working. The metrics catalog, the HA guide and the debug skill describe the new signals, which observe holds without bounding them. Part of NVIDIA#3528 Signed-off-by: Emilien Macchi <emacchi@redhat.com>

EmilienM · 2026-10-13T21:23:04Z

Thanks @pimlock, good catch on the startup lock timeout. Pushed 9941f73:

Startup reconciliation now retries a lock timeout instead of exiting. It stops retrying 30 seconds into the scan, so a replica stuck behind a long holder still gives up within about 40 seconds, inside the 60-second startup probe, and logs which sandbox it was waiting on. Two new tests cover a holder that lets go after the first timeout and one that never does.
I opened feat(server): scope mutation locks and bound lock waits #4501 for this PR's scope and the body now says Closes #4501, which should satisfy the issue acceptance check once a maintainer adds state:accepted.

@FrostGod, another force push, sorry. If the four earlier threads look done to you, could you resolve them? My account can't.

Could a maintainer take another look and /ok to test 9941f739cd82e297eaa5c4f839efed012b74a8b1? Thanks!

pimlock · 2026-10-13T21:31:31Z

/ok to test 9941f73

johntmyers · 2026-10-13T21:41:57Z

gator-agent

PR Review Status

@EmilienM, I checked your startup-timeout update: lock timeouts now retry within the scan's shared deadline, with tests for a holder that releases and one that outlasts the retry window. The independent Critical-only review of the author-only changes found no new Critical defects. No carried Gator code findings remain.

@pimlock, I verified that you accepted #4501, the PR's sole closing issue. Its accepted scope explicitly includes both database pool settings, which supplies the requested scope confirmation. Your earlier Helm feedback is reflected in the existing gatewayConfig.openshell.gateway configuration surface.

@johntmyers, your request to close out addressed feedback still needs human confirmation. @EmilienM has asked for help resolving the earlier threads because his account cannot do so. Gator cannot resolve human-owned review threads.

Action required: a maintainer should confirm and resolve the addressed PostgreSQL CI, benchmark, first-settings-insert follow-up, nested-wait follow-up, and startup retry threads, or identify any remaining change. The stale PR Issue Acceptance failure also needs a rerun of run 38470182027, which failed before #4501 was accepted. Gator submitted an exact-run sandbox policy proposal for that rerun; it is approved but not yet loaded, so no rerun has been queued.

Blocking findings: No new Critical findings.
Carried findings: None.

The test mirror matches the current head after @pimlock's /ok to test. Helm Lint, Trivy Changes, DCO, and the focused PostgreSQL CI job pass. Branch Checks, E2E, and both Windows lanes are dispatched and still running; no E2E label-help rerun instruction was posted. Gator remains blocked on the human feedback confirmation and acceptance-check refresh, and will inspect CI next cycle.

Gator metadata

Validation: Continues feat(ha): add production scaling signals and graceful gateway redistribution #3528 and merged feat(server): add gateway capacity metrics and optional HPA #3978; accepted feat(server): scope mutation locks and bound lock waits #4501 explicitly covers scoped locks, retry errors, configurable pools and metrics. Pool configuration scope is confirmed through that accepted issue; feat(server): make the database pool ceiling configurable #2828 remains related carried work for maintainer bookkeeping.
Docs: Fern gateway configuration, HA sizing, lock metrics, API errors, and cluster debug skill cover the changes and bounded startup retry.
Checks: Branch Checks 38471277143 and Windows MSVC 38471277127 are running. Helm Lint 38471277177, Trivy Changes 38470172262, DCO and Rust PostgreSQL tests pass. PR Issue Acceptance 38470182027 has a stale failure; rerun policy is approved pending reload.
E2E: test:e2e and test:e2e-kubernetes are applied; current-head Branch E2E Checks 38471277689 is dispatched. test:windows is applied; x64 and ARM64 lanes are running.
Local validation: PostgreSQL runner database/prefix selection test and diff whitespace check pass. No full local Rust or PostgreSQL suite was run.
Head SHA: 9941f739cd82e297eaa5c4f839efed012b74a8b1
Base SHA: 82977e27a438c8a6a1072498432f9f7c75a33a85
Merge base SHA: b9dc1f64660942a168b06f53b9dcbdfb3dd63de7
Patch ID: 8087a2368be4a268184811d552b5fd3fd530a02d
Gator payload: 12
Review mode: critical_only
Previous reviewed SHA: 7663cc225a538f1bbfd925f8cba367d8e51e10d0
Review budget exhausted: yes
Maintainer decision required: yes — confirm addressed human-owned feedback; no new Warning or additional configuration scope decision is requested.
Next state: gator:blocked
Blocked reason: maintainer_feedback_resolution_required

johntmyers · 2026-10-13T23:58:39Z

gator-agent

Monitoring Complete

Monitoring is complete because PR #4147 was merged by @pimlock.

The last Gator review found no new Critical defects and no carried Gator code findings remained. Existing review history is preserved.

I am removing the active gator:* label because there is nothing left for Gator to monitor on this PR.

Gator metadata

Head SHA: 9941f739cd82e297eaa5c4f839efed012b74a8b1
Gator payload: 12
Terminal state: merged

Resolve conflicts with main's scoped mutation locks (#4147) and configurable database pool size: - record_policy_apply_result takes the sandbox mutation guard by id in place of the removed sandbox_sync_guard, and returns NOT_FOUND when the sandbox is gone. The running-workload result test holds the same guard. - accept_supervisor_session takes main's endpoint-status reset failure path: when the session was current, demote the sandbox and retry the invalidation off the setup path. - backfill_legacy_policy_history pages sandboxes by keyset cursor, since list_all_messages was removed. - Keep both the config push and the mutation lock histograms. - Keep Store::max_connections for config delivery sizing; SQLite's test-only max_connections_for_test folds into it. - Main's new endpoint-status tests set the new SupervisorHello fields. Signed-off-by: Piotr Mlocek <pmlocek@nvidia.com>

EmilienM mentioned this pull request Oct 5, 2026

feat(server)!: harden gateway peer transport and expand HA conformance #3825

Draft

7 of 8 tasks

EmilienM force-pushed the feat/3528-scoped-locks/EmilienM branch from 26c5f0d to 71b9c52 Compare October 6, 2026 18:37

EmilienM marked this pull request as ready for review October 6, 2026 18:37

EmilienM requested review from a team, derekwaynecarr, mrunalp and sjenning as code owners October 6, 2026 18:37

johntmyers added test:e2e Requires end-to-end coverage test:e2e-kubernetes Requires Kubernetes end-to-end coverage labels Oct 6, 2026

EmilienM force-pushed the feat/3528-scoped-locks/EmilienM branch from 71b9c52 to 51822ba Compare October 6, 2026 22:12

johntmyers added gator:watch-pipeline Gator is monitoring PR CI/CD status gator:approval-needed Gator completed review; maintainer approval needed and removed gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 6, 2026

FrostGod reviewed Oct 7, 2026

View reviewed changes

Comment thread TESTING.md Outdated

Comment thread crates/openshell-server/src/compute/mutation_guard.rs

Comment thread crates/openshell-server/src/grpc/policy.rs

Comment thread crates/openshell-server/src/compute/mutation_guard.rs

EmilienM force-pushed the feat/3528-scoped-locks/EmilienM branch from 51822ba to c00b632 Compare October 7, 2026 16:35

EmilienM mentioned this pull request Oct 7, 2026

feat(server): follow-ups to scoped mutation locks (#4147) #4307

Open

2 of 14 tasks

EmilienM and others added 8 commits October 13, 2026 17:20

EmilienM force-pushed the feat/3528-scoped-locks/EmilienM branch from 189a96a to 9941f73 Compare October 13, 2026 21:22

johntmyers added gator:in-review Gator is reviewing or awaiting PR review feedback and removed gator:blocked Gator is blocked by process or repository gates labels Oct 13, 2026

pimlock approved these changes Oct 13, 2026

View reviewed changes

EmilienM mentioned this pull request Oct 13, 2026

bug: managed-workspace E2E tests intermittently fail with SQLite lock contention #3679

Open

4 tasks

pimlock enabled auto-merge October 13, 2026 23:21

pimlock added this pull request to the merge queue Oct 13, 2026

Merged via the queue into NVIDIA:main with commit e30c938 Oct 13, 2026
187 of 204 checks passed

EmilienM deleted the feat/3528-scoped-locks/EmilienM branch October 13, 2026 23:46

johntmyers removed the gator:in-review Gator is reviewing or awaiting PR review feedback label Oct 13, 2026

Conversation

EmilienM commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Oct 2, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

johntmyers commented Oct 6, 2026

Uh oh!

johntmyers commented Oct 6, 2026

PR Review Status

Uh oh!

johntmyers commented Oct 6, 2026

Uh oh!

johntmyers commented Oct 6, 2026

PR Review Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johntmyers commented Oct 7, 2026

Uh oh!

EmilienM commented Oct 13, 2026

Uh oh!

pimlock commented Oct 13, 2026

Uh oh!

johntmyers commented Oct 13, 2026

PR Review Status

Uh oh!

Uh oh!

johntmyers commented Oct 13, 2026

Monitoring Complete

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EmilienM commented Oct 2, 2026 •

edited

Loading