fix(envd): return error when concurrent /init races on NFS mount - #3460
fix(envd): return error when concurrent /init races on NFS mount#3460AdaAibaby wants to merge 3 commits into
Conversation
37b9c37 to
dbaa103
Compare
setupNFS guards against concurrent callers with an atomic CAS, but on contention it returned nil (the named return zero value). The second caller then continued as if NFS was mounted, so the sandbox started with missing volume mounts and no indication of failure. Return an explicit error so the caller receives a 500 and can retry, rather than silently succeeding with no mount.
dbaa103 to
ac63647
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac6364798c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eaba58c2a3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ErrConcurrentNFSInit was mapped to HTTP 400 in writeInitError, but doRequestWithInfiniteRetries only retries on network errors, not on successful HTTP responses. So when a slow NFS mount (> EnvdInitRequestTimeout) kept isMountingNFS set while the orchestrator retried the per-request timeout, the second /init got a 400, initEnvd declared the sandbox start a failure, and the sandbox was aborted. Fix both sides: - envd: map ErrConcurrentNFSInit to 503 (consistent with ErrCAInstallInProgress, which already used 503 for the same "transient, retry" semantics) - orchestrator: treat a 503 response from envd as a transient error in doRequestWithInfiniteRetries — close the body and loop, exactly as for a network-level error
When two concurrent
/initrequests race onisMountingNFS, theCompareAndSwapguard insetupNFSreturns the named error valuee, which isnil. The second caller proceeds as if NFS mounted successfully — sandbox starts with no volume mounts and no error surfaced to the caller.Fix: return a package-level sentinel
ErrConcurrentNFSInitso the caller receives a 500 and can retry. Using a sentinel (rather thanfmt.Errorf) allows callers to match the error witherrors.Is(), consistent withErrAccessTokenMismatchand other sentinels in the same file.Change
1 file, 2 lines changed.
/cc @ben-fornefeld @jakubno @dobrac @ValentaTomas @arkamar @tvi Looking forward to your code review.