fix(envd): make CA install lock ctx-aware#2690
Merged
Merged
Conversation
…w predecessor The /init handler calls caCertInstaller.Install with context.WithoutCancel(ctx), which strips the orchestrator's 50 ms per-request cancel — appropriate, because a half-written bundle update is worse than a slow one. But there was no upper bound either: if a previous install's background cleanup goroutine was still doing NBD writes to the extra-certs dir, the foreground installer blocked on the CACertInstaller mutex for as long as those writes took, and initLock was held behind it. - Add a 5 s deadline context on the caller side (still WithoutCancel of the parent so client cancellation doesn't propagate). - Wire ctx through into install() and acquire the internal mutex via a ctx-aware helper that returns early if the deadline fires.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit a131599. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 11 Tests Failed:
View the full list of 11 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
There was a problem hiding this comment.
Code Review
The implementation of context-aware locking in lockMutexCtx spawns additional goroutines whenever a timeout or cancellation occurs, which can lead to a significant accumulation of blocked goroutines if the mutex is held for extended periods. Using golang.org/x/sync/semaphore would allow for context-aware locking without the overhead and potential leak of multiple goroutines per request.
Member
Author
|
Superseded by #2700 (move envd into a dedicated network namespace), which addresses the root cause. |
Replaces lockMutexCtx (which spawned a goroutine per acquisition that lingered until lock release) with the same channel-as-mutex pattern #2702 uses for initLock. Foreground respects ctx for lock acquisition only; the actual install work always runs to completion.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Removal of context.WithoutCancel allows cert install cancellation
- Restored context.WithoutCancel wrapper to ensure CA cert installation completes even if the HTTP request context is cancelled.
Or push these changes by commenting:
@cursor push a13159934e
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit a131599. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a13159934e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
sitole
approved these changes
May 18, 2026
arkamar
added a commit
that referenced
this pull request
Jul 4, 2026
The orchestrator's /init retry loop uses short client timeouts. Since 83ee89f ("fix(envd): make CA install lock ctx-aware (#2690)") the foreground CA lock acquire respects the caller's ctx while a previous install's background cleanup goroutine holds the lock doing slow I/O. A rapid retry carrying the same cert blocked on that lock, had its request ctx canceled, and surfaced Failed to set data: failed to install CA bundle: acquire CA install lock: context canceled logged at Error, even though the install's background routine was still progressing. Wrap that canceled acquire in a known sentinel (ErrCAInstallInProgress) so writeInitError can recognise it and log at Warn instead of Error, and return 503 (retryable) instead of 400.
arkamar
added a commit
that referenced
this pull request
Jul 4, 2026
The background CA cleanup goroutine acquired the lock with context.Background() (since 83ee89f, "fix(envd): make CA install lock ctx-aware (#2690)"), discarding any values (trace/span, request-scoped data) from the caller's ctx. Use context.WithoutCancel(ctx) instead so the goroutine still outlives the request's cancellation but inherits its values.
arkamar
added a commit
that referenced
this pull request
Jul 7, 2026
…3206) The orchestrator's /init retry loop uses short client timeouts. Since 83ee89f ("fix(envd): make CA install lock ctx-aware (#2690)") the foreground CA lock acquire respects the caller's ctx while a previous install's background cleanup goroutine holds the lock doing slow I/O. A rapid retry carrying the same cert blocked on that lock, had its request ctx canceled, and surfaced Failed to set data: failed to install CA bundle: acquire CA install lock: context canceled logged at Error, even though the install's background routine was still progressing. Wrap that canceled acquire in a known sentinel (ErrCAInstallInProgress) so writeInitError can recognise it and log at Warn instead of Error, and return 503 (retryable) instead of 400.
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
3 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.
Replaces
sync.MutexonCACertInstallerwithsemaphore.Weighted(1). Foreground /init's lock acquisition now respects the caller's ctx; the actual file I/O has no ctx checks so it runs to completion once acquired. Background cleanup goroutine usescontext.Backgroundso it outlives the caller.