Skip to content

fix(envd): return error when concurrent /init races on NFS mount - #3460

Open
AdaAibaby wants to merge 3 commits into
e2b-dev:mainfrom
AdaAibaby:fix/envd-nfs-concurrent-init-v2
Open

fix(envd): return error when concurrent /init races on NFS mount#3460
AdaAibaby wants to merge 3 commits into
e2b-dev:mainfrom
AdaAibaby:fix/envd-nfs-concurrent-init-v2

Conversation

@AdaAibaby

@AdaAibaby AdaAibaby commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

When two concurrent /init requests race on isMountingNFS, the CompareAndSwap guard in setupNFS returns the named error value e, which is nil. 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 ErrConcurrentNFSInit so the caller receives a 500 and can retry. Using a sentinel (rather than fmt.Errorf) allows callers to match the error with errors.Is(), consistent with ErrAccessTokenMismatch and other sentinels in the same file.

Change

// before
if !a.isMountingNFS.CompareAndSwap(false, true) {
    logger.Debug().Msg("NFS volumes already mounting")
    return e  // nil — silent failure
}

// after
var ErrConcurrentNFSInit = errors.New("NFS mount already in progress")

if !a.isMountingNFS.CompareAndSwap(false, true) {
    logger.Debug().Msg("NFS volumes already mounting")
    return ErrConcurrentNFSInit
}

1 file, 2 lines changed.

/cc @ben-fornefeld @jakubno @dobrac @ValentaTomas @arkamar @tvi Looking forward to your code review.

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.
@AdaAibaby
AdaAibaby force-pushed the fix/envd-nfs-concurrent-init-v2 branch from dbaa103 to ac63647 Compare August 4, 2026 08:29

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/envd/internal/api/init.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread packages/envd/internal/api/init.go
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants