Add opt-in fast extension upload endpoint - #341
Open
rgarcia wants to merge 5 commits into
Open
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Partial CDP load leaves inconsistent state
- When CDP extension loading fails, the handler now rolls back newly created extension directories plus policy/flags snapshots and restarts Chromium so the browser and persisted config return to their pre-request state.
Or push these changes by commenting:
@cursor push f38376c9a3
Preview (f38376c9a3)
diff --git a/server/cmd/api/api/chromium.go b/server/cmd/api/api/chromium.go
--- a/server/cmd/api/api/chromium.go
+++ b/server/cmd/api/api/chromium.go
@@ -2,6 +2,7 @@
import (
"context"
+ "errors"
"fmt"
"io"
"mime/multipart"
@@ -28,6 +29,12 @@
name string
}
+type optionalFileSnapshot struct {
+ path string
+ data []byte
+ exists bool
+}
+
// chromiumFlagsPath is the runtime flags file read by the chromium-launcher at startup.
const chromiumFlagsPath = "/chromium/flags"
@@ -145,6 +152,17 @@
extItems = append(extItems, extensionZipItem{zipTemp: p.zipTemp, name: p.name})
}
+ flagsSnapshot, err := captureOptionalFileSnapshot(chromiumFlagsPath)
+ if err != nil {
+ log.Error("failed to snapshot chromium flags", "error", err)
+ return oapi.UploadExtensionsAndRestart500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "internal error"}}, nil
+ }
+ policySnapshot, err := captureOptionalFileSnapshot(policy.PolicyPath)
+ if err != nil {
+ log.Error("failed to snapshot chromium policy", "error", err)
+ return oapi.UploadExtensionsAndRestart500JSONResponse{InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: "internal error"}}, nil
+ }
+
requiresRestart, reqMsg, err := s.applyExtensionZipItems(ctx, extItems)
if reqMsg != "" {
return oapi.UploadExtensionsAndRestart400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: reqMsg}}, nil
@@ -160,6 +178,13 @@
}, nil
}
} else if err := s.loadUnpackedExtensions(ctx, extItems); err != nil {
+ if rollbackErr := s.rollbackExtensionUploadAfterCDPFailure(ctx, extItems, flagsSnapshot, policySnapshot); rollbackErr != nil {
+ return oapi.UploadExtensionsAndRestart500JSONResponse{
+ InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{
+ Message: fmt.Sprintf("%s (rollback failed: %v)", err.Error(), rollbackErr),
+ },
+ }, nil
+ }
return oapi.UploadExtensionsAndRestart500JSONResponse{
InternalErrorJSONResponse: oapi.InternalErrorJSONResponse{Message: err.Error()},
}, nil
@@ -310,6 +335,60 @@
return requiresRestart, "", nil
}
+func captureOptionalFileSnapshot(path string) (optionalFileSnapshot, error) {
+ data, err := os.ReadFile(path)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return optionalFileSnapshot{path: path}, nil
+ }
+ return optionalFileSnapshot{}, err
+ }
+ return optionalFileSnapshot{
+ path: path,
+ data: data,
+ exists: true,
+ }, nil
+}
+
+func restoreOptionalFileSnapshot(snapshot optionalFileSnapshot) error {
+ if !snapshot.exists {
+ if err := os.Remove(snapshot.path); err != nil && !os.IsNotExist(err) {
+ return err
+ }
+ return nil
+ }
+ if err := os.MkdirAll(filepath.Dir(snapshot.path), 0o755); err != nil {
+ return err
+ }
+ return os.WriteFile(snapshot.path, snapshot.data, 0o644)
+}
+
+func (s *ApiService) rollbackExtensionUploadAfterCDPFailure(ctx context.Context, items []extensionZipItem, flagsSnapshot, policySnapshot optionalFileSnapshot) error {
+ log := logger.FromContext(ctx)
+ var rollbackErr error
+
+ for _, item := range items {
+ path := filepath.Join("/home/kernel/extensions", item.name)
+ if err := os.RemoveAll(path); err != nil {
+ rollbackErr = errors.Join(rollbackErr, fmt.Errorf("failed to remove extension dir %s: %w", item.name, err))
+ }
+ }
+ if err := restoreOptionalFileSnapshot(policySnapshot); err != nil {
+ rollbackErr = errors.Join(rollbackErr, fmt.Errorf("failed to restore policy: %w", err))
+ }
+ if err := restoreOptionalFileSnapshot(flagsSnapshot); err != nil {
+ rollbackErr = errors.Join(rollbackErr, fmt.Errorf("failed to restore flags: %w", err))
+ }
+
+ if err := s.restartChromiumAndWait(ctx, "extension upload rollback"); err != nil {
+ rollbackErr = errors.Join(rollbackErr, fmt.Errorf("failed to restart chromium during rollback: %w", err))
+ }
+ if rollbackErr != nil {
+ log.Error("failed to rollback extension upload after CDP load failure", "error", rollbackErr)
+ }
+ return rollbackErr
+}
+
func (s *ApiService) loadUnpackedExtensions(ctx context.Context, items []extensionZipItem) error {
log := logger.FromContext(ctx)
for _, item := range items {You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Configure/display mutex deadlock
- I refactored display patching to allow ChromiumConfigure to call PatchDisplay logic without re-locking chromiumConfigMu and added a regression test that verifies this path no longer blocks while the configure lock is held.
Or push these changes by commenting:
@cursor push 6ddea6c7d0
Preview (6ddea6c7d0)
diff --git a/server/cmd/api/api/chromium_configure.go b/server/cmd/api/api/chromium_configure.go
--- a/server/cmd/api/api/chromium_configure.go
+++ b/server/cmd/api/api/chromium_configure.go
@@ -699,7 +699,7 @@
}
func chromiumRunPatchDisplay(ctx context.Context, s *ApiService, body *oapi.PatchDisplayJSONRequestBody) oapi.ChromiumConfigureResponseObject {
- resp, err := s.PatchDisplay(ctx, oapi.PatchDisplayRequestObject{Body: body})
+ resp, err := s.patchDisplay(ctx, oapi.PatchDisplayRequestObject{Body: body}, false)
if err != nil {
return cfg500ConfigureStep(chromiumConfigureStepDisplay, err.Error())
}
diff --git a/server/cmd/api/api/chromium_configure_test.go b/server/cmd/api/api/chromium_configure_test.go
--- a/server/cmd/api/api/chromium_configure_test.go
+++ b/server/cmd/api/api/chromium_configure_test.go
@@ -2,12 +2,15 @@
import (
"bytes"
+ "context"
"errors"
"io"
"mime/multipart"
"strings"
"testing"
+ "time"
+ oapi "github.com/kernel/kernel-images/server/lib/oapi"
"github.com/stretchr/testify/require"
)
@@ -231,3 +234,26 @@
require.Equal(t, "one", st.extItems[0].name)
require.Equal(t, "two", st.extItems[1].name)
}
+
+func TestChromiumRunPatchDisplayWhileConfigureLockHeld(t *testing.T) {
+ svc := &ApiService{}
+ width := -1
+ body := &oapi.PatchDisplayJSONRequestBody{
+ Width: &width,
+ }
+
+ done := make(chan struct{})
+ svc.chromiumConfigMu.Lock()
+ defer svc.chromiumConfigMu.Unlock()
+
+ go func() {
+ _ = chromiumRunPatchDisplay(context.Background(), svc, body)
+ close(done)
+ }()
+
+ select {
+ case <-done:
+ case <-time.After(2 * time.Second):
+ require.FailNow(t, "chromiumRunPatchDisplay blocked while chromiumConfigMu already held")
+ }
+}
diff --git a/server/cmd/api/api/display.go b/server/cmd/api/api/display.go
--- a/server/cmd/api/api/display.go
+++ b/server/cmd/api/api/display.go
@@ -24,6 +24,10 @@
// This method automatically detects whether the system is running with Xorg (headful)
// or Xvfb (headless) and uses the appropriate method to change resolution.
func (s *ApiService) PatchDisplay(ctx context.Context, req oapi.PatchDisplayRequestObject) (oapi.PatchDisplayResponseObject, error) {
+ return s.patchDisplay(ctx, req, true)
+}
+
+func (s *ApiService) patchDisplay(ctx context.Context, req oapi.PatchDisplayRequestObject, acquireConfigLock bool) (oapi.PatchDisplayResponseObject, error) {
log := logger.FromContext(ctx)
if req.Body == nil {
@@ -35,8 +39,10 @@
return oapi.PatchDisplay400JSONResponse{BadRequestErrorJSONResponse: oapi.BadRequestErrorJSONResponse{Message: "no display parameters to update"}}, nil
}
- s.chromiumConfigMu.Lock()
- defer s.chromiumConfigMu.Unlock()
+ if acquireConfigLock {
+ s.chromiumConfigMu.Lock()
+ defer s.chromiumConfigMu.Unlock()
+ }
// Get current resolution with refresh rate
currentWidth, currentHeight, currentRefreshRate, err := s.getCurrentResolution(ctx)You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 02ae37c. Configure here.
rgarcia
changed the title
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
1 participant
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.
Summary
/chromium/upload-extensionsas the opt-in fast path for ordinary unpacked extensions/chromium/upload-extensions-and-restartunconditionally restarting Chromium for a safe control-plane rollout--load-extensionpaths so extensions return after later browser restartsPerformance
/chromium/upload-extensionsThe opt-in fast path avoids about 3.2–3.7 seconds of restart latency and was roughly 100× faster in these measurements. The 32 ms result is an end-to-end API request against the locally built headless image. Restart averages come from the CI
TestChromiumRestartTimingbenchmark.Testing
go vet ./...go test -race $(go list ./... | grep -v /e2e$)/chromium/upload-extensionsreturned 201 in 32 ms and preserved the DevTools browser ID/chromium/upload-extensions-and-restartreturned 201 in 3.18 seconds and changed the DevTools browser ID5aad65fNote
Medium Risk
Touches Chromium extension install, enterprise policy, and live CDP loading with restart fallback; incorrect restart vs CDP choice could leave extensions inactive or disrupt sessions.
Overview
Adds
POST /chromium/upload-extensions, an opt-in path that installs ordinary unpacked extensions and activates them via CDPExtensions.loadUnpackedwithout restarting Chromium./chromium/upload-extensions-and-restartstill always restarts; both shareuploadExtensionsandapplyExtensionZipItems, which now returns whether enterprise-policy extensions force a restart.When no restart is required, the API loads extensions over DevTools (with restart fallback on CDP failure).
chromiumConfigMuserializes extension uploads, flags, policies, display patches, and batched configure so concurrent Chromium config cannot race.The CDP client gains
LoadUnpackedExtension; e2e tests assert the fast path preserves the browser WebSocket ID and the legacy endpoint still restarts.Reviewed by Cursor Bugbot for commit 5aad65f. Bugbot is set up for automated code reviews on this repo. Configure here.