Skip to content

fix(server): bound the provisioning-deadline scan's lock wait - #4549

Open
EmilienM wants to merge 1 commit into
NVIDIA:mainfrom
EmilienM:fix/4547-provisioning-deadline-lock-wait/EmilienM
Open

EmilienM wants to merge 1 commit into
NVIDIA:mainfrom
EmilienM:fix/4547-provisioning-deadline-lock-wait/EmilienM

Conversation

@EmilienM

@EmilienM EmilienM commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

The provisioning-deadline scan waited with no deadline for each sandbox's local mutation keys, so a slow provider or profile write could stall lifecycle operations on an unrelated sandbox, or stall provider writes behind a busy sandbox. The scan now waits at most 500 ms per pass and picks a busy sandbox up on the next pass.

Related Issue

Closes #4547 (needs state:accepted for the issue acceptance check)
Part of #4307

Changes

  • reconcile_provisioning_deadlines gives each pass a 500 ms lock-wait budget and skips a sandbox whose keys stay busy. Once the budget is spent, the rest of the pass only takes keys that are free, so a busy workspace or global key costs one wait per pass, not one per sandbox. The acquisition is cancel-safe, so a skip releases the keys it took.
  • Skips log at debug and count in openshell_server_provisioning_deadline_skips_total{overdue}.
  • Trade-off: a skipped sandbox gives up its place in the lock queue, so expiry can run a few passes late, against a 300-second repair window. The counter shows it.
  • Doc updates: the new counter in the gateway metrics page and the debug-openshell-cluster skill, the lock table in mutation_lock.rs, and the lock_sandbox_local_in_workspace doc comment.
  • Merge note: feat(kubernetes): add template-driven warm pooling #3508 moves the post-lock scan body into refresh_provisioning_deadline and conflicts textually with this PR. The lock acquisition stays in the scan loop there, so keep the per-pass budget and skip around it.

Testing

  • mise run rust:lint, rust:format:check, cargo test -p openshell-server --features test-support and mise run test:rust:postgres pass.
  • Four tests replace the one that pinned the old wait: one per key order, one that checks a skip doesn't end the pass, and one that checks sandboxes behind a busy workspace share one wait. All four fail on main.
  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated (if applicable)
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@copy-pr-bot

copy-pr-bot Bot commented Oct 14, 2026

Copy link
Copy Markdown

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.

@mrunalp

mrunalp commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 859406f

@mrunalp mrunalp added the test:e2e Requires end-to-end coverage label Oct 14, 2026
@github-actions

Copy link
Copy Markdown

Label test:e2e applied for 859406f. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@FrostGod
FrostGod self-requested a review October 16, 2026 08:29
@FrostGod

Copy link
Copy Markdown
Collaborator

if one workspace stays busy and has many sandboxes setting up, each one costs 500 ms before it's skipped. For example, 50 sandboxes make a pass take about 25 seconds. Should the scan skip the rest of that workspace after its first timeout?

@FrostGod

Copy link
Copy Markdown
Collaborator

with a steady stream of writers, an overdue sandbox could be skipped on every pass, and nothing bounds that. The only signal is a warning log.
and Log noise: in that same case, the warning fires once a second for each sandbox.

The provisioning-deadline scan takes each provisioning sandbox's local
S(global) S(workspace) X(sandbox) keys in ascending key order and waited
for them with no deadline. When a sandbox's key sorts before its
workspace's key, the scan held X(sandbox) while it waited behind a
provider or profile writer, so start, stop and delete for that sandbox
waited for the writer too. In the other order, the scan held
S(workspace) while it waited for a busy sandbox, and provider writers
on that workspace queued behind it. Either way the rest of the pass
waited as well.

Each pass now has a 500 ms budget for waiting on these keys. A sandbox
whose keys stay busy is skipped and picked up on the next pass a second
later. Once the budget is spent, the rest of the pass only takes keys
that are free when it reaches them, so a busy workspace or global key
costs one wait per pass rather than one per sandbox. The local
acquisition is cancel-safe, so giving up releases the keys it already
took.

Skips log at debug and count in the new
openshell_server_provisioning_deadline_skips_total counter, labeled by
whether the sandbox was already past its deadline. A steady rate of
delayed expiries can then be alerted on without a warning every pass.
The gateway metrics page and the cluster debugging skill describe it.

The test that pinned the old wait is replaced by tests for both key
orders: a lifecycle lock or a provider writer proceeds and the scan
finishes while the other side holds its key, the skipped sandbox is
untouched and expires once the writer releases, and a skip doesn't end
the pass for other sandboxes. Another test checks that sandboxes
behind one busy workspace share a single wait.

Part of NVIDIA#4307
Closes NVIDIA#4547

Signed-off-by: Emilien Macchi <emacchi@redhat.com>
@EmilienM
EmilienM force-pushed the fix/4547-provisioning-deadline-lock-wait/EmilienM branch from 859406f to 098b69c Compare October 16, 2026 12:33
@EmilienM

Copy link
Copy Markdown
Contributor Author
  1. Agreed on the cost, but I'd bound the pass rather than the workspace. The timeout can come from the sandbox's own key, so one slow lifecycle op would skip its whole workspace on every pass. It can also come from the global key, where a global write costs 500 ms in every workspace. Next push gives each pass one 500 ms wait budget. Once it's spent, the rest of the pass only takes locks that are free right now, so a pass waits about 500 ms at most, whichever key is busy.

  2. Fair on the noise: skips drop to debug and get a counter labeled overdue or not, so you can alert on it. I'd leave starvation unbounded, though. The lock is FIFO, so a skip needs more than 500 ms of writes already queued when the scan arrives. Starving a sandbox means that's true on every pass, i.e. the workspace lock is saturated, which mutation_lock_wait_seconds{scope="workspace"} already shows. A hard bound means waiting longer, which brings back the stall this PR removes.

Both are in 098b69c (openshell_server_provisioning_deadline_skips_total{overdue}).

@FrostGod

Copy link
Copy Markdown
Collaborator

/ok to test 098b69c

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(server): bound the provisioning-deadline scan's lock wait

3 participants