From e0aba9a4714153a78f8933236b72264621dbf018 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 01:50:45 -0700 Subject: [PATCH 1/8] fix(envd): bound CA bundle install so initLock can't be held by a slow predecessor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/envd/internal/api/init.go | 16 +++++++++++++- packages/envd/internal/host/cacerts.go | 30 ++++++++++++++++++++++++-- packages/envd/pkg/version.go | 2 +- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/packages/envd/internal/api/init.go b/packages/envd/internal/api/init.go index 4d0081b0a2..95b8b281a8 100644 --- a/packages/envd/internal/api/init.go +++ b/packages/envd/internal/api/init.go @@ -30,6 +30,13 @@ var ( const ( maxTimeInPast = 50 * time.Millisecond maxTimeInFuture = 5 * time.Second + + // caBundleInstallTimeout caps how long the CA bundle install can block + // the /init handler. The foreground append is on tmpfs and finishes in + // sub-ms; the cap covers the case where a background cleanup goroutine + // from a previous install (NBD write to the persistent extra-certs dir) + // is still holding CACertInstaller's mutex. + caBundleInstallTimeout = 5 * time.Second ) // validateInitAccessToken validates the access token for /init requests. @@ -214,7 +221,14 @@ func (a *API) SetData(ctx context.Context, logger zerolog.Logger, data PostInitJ } if data.CaBundle != nil && *data.CaBundle != "" { - err := a.caCertInstaller.Install(context.WithoutCancel(ctx), *data.CaBundle) + // WithoutCancel(ctx) so a 50 ms orchestrator-side client cancel doesn't + // abort a half-written bundle update. Bound the work with a hard deadline + // so the installer cannot hold initLock indefinitely if the underlying + // mutex (held by the background cleanup goroutine doing NBD writes) is + // slow to release — see CACertInstaller in packages/envd/internal/host. + installCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), caBundleInstallTimeout) + err := a.caCertInstaller.Install(installCtx, *data.CaBundle) + cancel() if err != nil { return fmt.Errorf("failed to install CA bundle: %w", err) } diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index f0101c48c6..88e505986e 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -64,7 +64,7 @@ func (c *CACertInstaller) Install(ctx context.Context, certPEM string) error { // // All goroutine work runs under mu to keep the bundle and extra-certs file // consistent with concurrent foreground appends. -func (c *CACertInstaller) install(_ context.Context, certPEM, bundlePath, extraPath string) error { +func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extraPath string) error { if certPEM == "" { return nil } @@ -75,7 +75,9 @@ func (c *CACertInstaller) install(_ context.Context, certPEM, bundlePath, extraP // consistent regardless of how the caller formatted the PEM. normalized := strings.TrimRight(certPEM, "\n") + "\n" - c.mu.Lock() + if err := lockMutexCtx(ctx, &c.mu); err != nil { + return fmt.Errorf("acquire CA install lock: %w", err) + } defer c.mu.Unlock() if c.lastCACert == normalized { @@ -147,6 +149,30 @@ func (c *CACertInstaller) install(_ context.Context, certPEM, bundlePath, extraP return nil } +// lockMutexCtx acquires mu but bails out if ctx is cancelled first. Lets a +// caller bound how long /init holds initLock waiting on a slow background +// goroutine still holding mu (typically a previous install's NBD write). +func lockMutexCtx(ctx context.Context, mu *sync.Mutex) error { + acquired := make(chan struct{}) + go func() { + mu.Lock() + close(acquired) + }() + select { + case <-acquired: + return nil + case <-ctx.Done(): + // The lock will be acquired by the goroutine above eventually; release + // it as soon as it gets the lock so we don't leak ownership. + go func() { + <-acquired + mu.Unlock() + }() + + return ctx.Err() + } +} + // removeCertFromBundle rewrites bundlePath removing all occurrences of certPEM. // The write is atomic (write to temp file in the same directory, then rename) // so the bundle is never partially written from the perspective of concurrent diff --git a/packages/envd/pkg/version.go b/packages/envd/pkg/version.go index 88bc32c769..4cfbb08b29 100644 --- a/packages/envd/pkg/version.go +++ b/packages/envd/pkg/version.go @@ -1,3 +1,3 @@ package pkg -const Version = "0.5.23" +const Version = "0.5.24" From 01a90ae039cc7768d273269166bdedc0ee119eb5 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 02:32:11 -0700 Subject: [PATCH 2/8] chore: trim verbose comments --- packages/envd/internal/api/init.go | 11 +---------- packages/envd/internal/host/cacerts.go | 14 ++++---------- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/packages/envd/internal/api/init.go b/packages/envd/internal/api/init.go index 95b8b281a8..864024fe92 100644 --- a/packages/envd/internal/api/init.go +++ b/packages/envd/internal/api/init.go @@ -31,11 +31,7 @@ const ( maxTimeInPast = 50 * time.Millisecond maxTimeInFuture = 5 * time.Second - // caBundleInstallTimeout caps how long the CA bundle install can block - // the /init handler. The foreground append is on tmpfs and finishes in - // sub-ms; the cap covers the case where a background cleanup goroutine - // from a previous install (NBD write to the persistent extra-certs dir) - // is still holding CACertInstaller's mutex. + // caBundleInstallTimeout caps how long /init waits for the CA install lock. caBundleInstallTimeout = 5 * time.Second ) @@ -221,11 +217,6 @@ func (a *API) SetData(ctx context.Context, logger zerolog.Logger, data PostInitJ } if data.CaBundle != nil && *data.CaBundle != "" { - // WithoutCancel(ctx) so a 50 ms orchestrator-side client cancel doesn't - // abort a half-written bundle update. Bound the work with a hard deadline - // so the installer cannot hold initLock indefinitely if the underlying - // mutex (held by the background cleanup goroutine doing NBD writes) is - // slow to release — see CACertInstaller in packages/envd/internal/host. installCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), caBundleInstallTimeout) err := a.caCertInstaller.Install(installCtx, *data.CaBundle) cancel() diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 88e505986e..2362981d74 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -149,9 +149,9 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr return nil } -// lockMutexCtx acquires mu but bails out if ctx is cancelled first. Lets a -// caller bound how long /init holds initLock waiting on a slow background -// goroutine still holding mu (typically a previous install's NBD write). +// lockMutexCtx acquires mu or returns ctx.Err() if ctx is cancelled first. +// On cancellation, releases the lock when the background goroutine eventually +// acquires it so ownership is never leaked. func lockMutexCtx(ctx context.Context, mu *sync.Mutex) error { acquired := make(chan struct{}) go func() { @@ -162,13 +162,7 @@ func lockMutexCtx(ctx context.Context, mu *sync.Mutex) error { case <-acquired: return nil case <-ctx.Done(): - // The lock will be acquired by the goroutine above eventually; release - // it as soon as it gets the lock so we don't leak ownership. - go func() { - <-acquired - mu.Unlock() - }() - + go func() { <-acquired; mu.Unlock() }() return ctx.Err() } } From d8efffc629da243e92bae2b593983fe49167a151 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 02:49:31 -0700 Subject: [PATCH 3/8] fix(cacerts): nlreturn --- packages/envd/internal/host/cacerts.go | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 2362981d74..70bbc56639 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -163,6 +163,7 @@ func lockMutexCtx(ctx context.Context, mu *sync.Mutex) error { return nil case <-ctx.Done(): go func() { <-acquired; mu.Unlock() }() + return ctx.Err() } } From d4b4a9b0d1397b0ab328240177aed9df461842b7 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 22:34:39 -0700 Subject: [PATCH 4/8] simplify: channel-as-mutex, no goroutine leak 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. --- packages/envd/internal/host/cacerts.go | 42 +++++++++----------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 70bbc56639..8cb56f3e82 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -6,7 +6,6 @@ import ( "os" "path/filepath" "strings" - "sync" "time" "github.com/rs/zerolog" @@ -28,7 +27,10 @@ const ( // ExecStartPre), so all reads and writes bypass the NBD-backed filesystem and // atomic cert rotation via os.Rename works within the same device. type CACertInstaller struct { - mu sync.Mutex + // mu is a buffered-1 channel acting as a ctx-aware mutex. Lock acquisition + // respects the caller's ctx; the actual install work always runs to + // completion once the lock is held. + mu chan struct{} logger *zerolog.Logger // lastCACert caches the most recently installed PEM so that resume (same @@ -40,7 +42,10 @@ type CACertInstaller struct { } func NewCACertInstaller(logger *zerolog.Logger) *CACertInstaller { - return &CACertInstaller{logger: logger} + return &CACertInstaller{ + logger: logger, + mu: make(chan struct{}, 1), + } } // Install injects certPEM into the system CA bundle. Returns an error if the @@ -75,10 +80,12 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr // consistent regardless of how the caller formatted the PEM. normalized := strings.TrimRight(certPEM, "\n") + "\n" - if err := lockMutexCtx(ctx, &c.mu); err != nil { - return fmt.Errorf("acquire CA install lock: %w", err) + select { + case c.mu <- struct{}{}: + case <-ctx.Done(): + return fmt.Errorf("acquire CA install lock: %w", ctx.Err()) } - defer c.mu.Unlock() + defer func() { <-c.mu }() if c.lastCACert == normalized { c.logger.Debug(). @@ -112,8 +119,8 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr go func() { cleanStart := time.Now() - c.mu.Lock() - defer c.mu.Unlock() + c.mu <- struct{}{} + defer func() { <-c.mu }() // A newer install has taken over; let that goroutine handle cleanup. if c.lastCACert != normalized { @@ -149,25 +156,6 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr return nil } -// lockMutexCtx acquires mu or returns ctx.Err() if ctx is cancelled first. -// On cancellation, releases the lock when the background goroutine eventually -// acquires it so ownership is never leaked. -func lockMutexCtx(ctx context.Context, mu *sync.Mutex) error { - acquired := make(chan struct{}) - go func() { - mu.Lock() - close(acquired) - }() - select { - case <-acquired: - return nil - case <-ctx.Done(): - go func() { <-acquired; mu.Unlock() }() - - return ctx.Err() - } -} - // removeCertFromBundle rewrites bundlePath removing all occurrences of certPEM. // The write is atomic (write to temp file in the same directory, then rename) // so the bundle is never partially written from the perspective of concurrent From 0bbbbc06a1a1688dbacbb779b74a5ed042676792 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 22:44:18 -0700 Subject: [PATCH 5/8] use semaphore.Weighted instead of channel mutex --- packages/envd/internal/host/cacerts.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 8cb56f3e82..953bb44f76 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -9,6 +9,7 @@ import ( "time" "github.com/rs/zerolog" + "golang.org/x/sync/semaphore" ) const ( @@ -27,10 +28,9 @@ const ( // ExecStartPre), so all reads and writes bypass the NBD-backed filesystem and // atomic cert rotation via os.Rename works within the same device. type CACertInstaller struct { - // mu is a buffered-1 channel acting as a ctx-aware mutex. Lock acquisition - // respects the caller's ctx; the actual install work always runs to - // completion once the lock is held. - mu chan struct{} + // mu is a ctx-aware mutex; Acquire respects the caller's ctx so a stuck + // background cleanup can't permanently wedge a foreground /init. + mu *semaphore.Weighted logger *zerolog.Logger // lastCACert caches the most recently installed PEM so that resume (same @@ -44,7 +44,7 @@ type CACertInstaller struct { func NewCACertInstaller(logger *zerolog.Logger) *CACertInstaller { return &CACertInstaller{ logger: logger, - mu: make(chan struct{}, 1), + mu: semaphore.NewWeighted(1), } } @@ -80,12 +80,10 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr // consistent regardless of how the caller formatted the PEM. normalized := strings.TrimRight(certPEM, "\n") + "\n" - select { - case c.mu <- struct{}{}: - case <-ctx.Done(): - return fmt.Errorf("acquire CA install lock: %w", ctx.Err()) + if err := c.mu.Acquire(ctx, 1); err != nil { + return fmt.Errorf("acquire CA install lock: %w", err) } - defer func() { <-c.mu }() + defer c.mu.Release(1) if c.lastCACert == normalized { c.logger.Debug(). @@ -119,8 +117,8 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr go func() { cleanStart := time.Now() - c.mu <- struct{}{} - defer func() { <-c.mu }() + _ = c.mu.Acquire(context.Background(), 1) + defer c.mu.Release(1) // A newer install has taken over; let that goroutine handle cleanup. if c.lastCACert != normalized { From 3c32f52a38e7bf8da8992e9d1dc76d11be1ddfde Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 22:47:18 -0700 Subject: [PATCH 6/8] drop redundant cacerts mu comment --- packages/envd/internal/host/cacerts.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 953bb44f76..9a1fa64132 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -28,8 +28,6 @@ const ( // ExecStartPre), so all reads and writes bypass the NBD-backed filesystem and // atomic cert rotation via os.Rename works within the same device. type CACertInstaller struct { - // mu is a ctx-aware mutex; Acquire respects the caller's ctx so a stuck - // background cleanup can't permanently wedge a foreground /init. mu *semaphore.Weighted logger *zerolog.Logger From e0ca9601c992417eee5269cc4f5ee1e2f3517186 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 22:50:17 -0700 Subject: [PATCH 7/8] nolint contextcheck on detached cleanup goroutine --- packages/envd/internal/host/cacerts.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/envd/internal/host/cacerts.go b/packages/envd/internal/host/cacerts.go index 9a1fa64132..d6f9839c46 100644 --- a/packages/envd/internal/host/cacerts.go +++ b/packages/envd/internal/host/cacerts.go @@ -112,7 +112,7 @@ func (c *CACertInstaller) install(ctx context.Context, certPEM, bundlePath, extr Dur("append_duration", time.Since(start)). Msg("CA cert appended to bundle") - go func() { + go func() { //nolint:contextcheck // background cleanup must outlive the caller's ctx cleanStart := time.Now() _ = c.mu.Acquire(context.Background(), 1) From a13159934e94c490f3e25eaf9805e0fbb8598ee2 Mon Sep 17 00:00:00 2001 From: ValentaTomas Date: Sun, 17 May 2026 22:52:09 -0700 Subject: [PATCH 8/8] drop caBundleInstallTimeout, use caller ctx --- packages/envd/internal/api/init.go | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/envd/internal/api/init.go b/packages/envd/internal/api/init.go index 864024fe92..d63e32ba2d 100644 --- a/packages/envd/internal/api/init.go +++ b/packages/envd/internal/api/init.go @@ -30,9 +30,6 @@ var ( const ( maxTimeInPast = 50 * time.Millisecond maxTimeInFuture = 5 * time.Second - - // caBundleInstallTimeout caps how long /init waits for the CA install lock. - caBundleInstallTimeout = 5 * time.Second ) // validateInitAccessToken validates the access token for /init requests. @@ -217,10 +214,7 @@ func (a *API) SetData(ctx context.Context, logger zerolog.Logger, data PostInitJ } if data.CaBundle != nil && *data.CaBundle != "" { - installCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), caBundleInstallTimeout) - err := a.caCertInstaller.Install(installCtx, *data.CaBundle) - cancel() - if err != nil { + if err := a.caCertInstaller.Install(ctx, *data.CaBundle); err != nil { return fmt.Errorf("failed to install CA bundle: %w", err) } }