Add opt-in fast extension upload endpoint - #341
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
masnwilliams
left a comment
masnwilliams
left a comment
There was a problem hiding this comment.
the CDP fast path is a good direction, but there are correctness and maintainability issues to address before merging:
-
Make extension installation transactional (
server/cmd/api/api/chromium.go,applyExtensionZipItems). The function commits policy entries one extension at a time, but its failure cleanup only removes directories. IfAddExtensionsucceeds for one item and a later item fails, the deferred cleanup deletes both directories while leaving the first persisted policy entry pointing to a nonexistent path. Please stage and validate the whole batch before commit, or snapshot/restore directories, policy, and flags as one transaction. -
Decouple the shared core from the old endpoint's generated types (
uploadExtensions). The new handler translates throughUploadExtensionsAndRestartRequestObjectand switches overUploadExtensionsAndRestartResponseObject. That makes one HTTP route's generated model the internal domain API and requires a fragile response adapter. Please have the shared core accept the multipart reader and return neutral result/error types, leaving generated response mapping in each handler. Both routes can also reference one shared multipart schema in OpenAPI. -
Exercise the deadlock regression through the production path.
TestPatchDisplayLockedDoesNotRelockChromiumConfigdirectly calls the private locked helper, so it does not verify lock ownership throughchromiumRunPatchDisplay/ChromiumConfigure, where the nested acquisition occurred. Please test the production orchestration path under a timeout. -
Bound work performed while holding
chromiumConfigMu.loadUnpackedExtensionsderives its timeout from caller-controlled item count while the global configuration mutex is held. A sufficiently large request can block unrelated configuration operations for a long time. Please add explicit upload byte/count limits and use a fixed overall activation deadline. -
Test the new endpoint's restart decisions in e2e. The enterprise-policy fixture still calls
UploadExtensionsAndRestart, which always restarts and therefore cannot catch a broken policy-required branch inUploadExtensions. Please route that case through the new endpoint and add coverage for CDP activation failure falling back to restart.
Locally, go vet ./... passed and the changed packages passed with the race detector. I did not run the full e2e suite. The PR also currently conflicts with main.
2 participants
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.