Enforce browser control ownership#119
Merged
glittercowboy merged 3 commits intomainfrom Apr 30, 2026
Merged
Conversation
📝 WalkthroughWalkthroughExtends browser session management with per-session control versioning and ownership tracking. Adds richer frame metadata (viewport dimensions, device pixel ratio) to session messages. Enhances Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser Client
participant Manager as Session Manager
participant Transport as Protocol Transport
participant Service as Backend Service
Client->>Manager: UserInput (controlVersion, frameSeq, ...)
alt Control Version is Stale
Manager->>Transport: Send BrowserUserInputAck<br/>(ControlVersion, Reason: Stale)
Transport-->>Client: UserInputAck (rejected)
else Control Version is Fresh
Manager->>Service: Invoke UserInput RPC<br/>(augmented params)
Service-->>Manager: Response
Manager->>Transport: Send BrowserUserInputAck<br/>(ControlVersion, Timestamp)
Transport-->>Client: UserInputAck (accepted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 2/10 reviews remaining, refill in 42 minutes and 34 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager_test.go`:
- Around line 283-336: The test reads sent.msgs and svc.calls while the
manager's frameLoop may append to them, causing races; protect access by adding
a mutex to recordingSender and fakeService (guarding recordingSender.msgs and
fakeService.calls) and update any helpers (e.g., hasCall) and the test's
inspection code to use locked accessor methods (e.g.,
recordingSender.Messages(), fakeService.HasCall(name) or similar) so the slices
are read under the mutex; alternatively stop the session/frame loop before
iterating by calling the manager shutdown method if one exists, but the minimal
change is to serialize access to sent.msgs and svc.calls via mutex-protected
getters used in TestUserInputRejectsStaleControlVersion.
In `@internal/browser/manager.go`:
- Around line 78-80: The OpenRequest fields InitialURL, BridgeMode, and
PreviewID are populated in manager.go but not propagated into the LocalService
implementation; update the LocalService methods that create/start sessions (the
Open/OpenSession/Start methods in the LocalService implementation in
internal/browser/service.go) to accept and forward these fields from OpenRequest
into the browser startup/open call so the new protocol fields are used when
launching sessions; locate the OpenRequest type and the LocalService method
implementations and ensure InitialURL, BridgeMode, and PreviewID are read from
the OpenRequest and passed into the underlying browser start/open logic.
- Around line 253-265: The Claim and Release flows mutate state.owner and
state.controlVersion before calling m.sender.Send, so if Send fails the local
state remains changed; fix by capturing the previous owner and controlVersion,
perform the state mutation, then attempt m.sender.Send, and if Send returns an
error revert state.owner and state.controlVersion back to the captured values
while holding m.mu (apply same pattern in both Claim and Release around
m.sender.Send), ensuring the revert occurs only on send failure so the local
state matches what the peer actually observed.
- Around line 175-194: The code advances current.lastFrameSeq regardless of
whether m.sender.Send succeeded; change it to check the error returned by
m.sender.Send(ctx, &protocol.BrowserFrame{...}) and only update
m.byID[browserID].lastFrameSeq when send returns nil. Specifically, capture the
error from m.sender.Send, handle/log it as appropriate, and move the
m.mu.Lock()/unlock and the conditional that updates current.lastFrameSeq (using
current == state && frame.Sequence > current.lastFrameSeq) so it runs only when
the send error is nil; keep references to m.sender.Send, protocol.BrowserFrame,
m.byID[browserID], lastFrameSeq, and frame.Sequence to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c35a2357-bb15-44b6-8590-ffc79b82c46f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.modinternal/browser/manager.gointernal/browser/manager_test.gointernal/browser/service.gointernal/browser/types.go
Comment on lines
+78
to
+80
| InitialURL: msg.InitialURL, | ||
| BridgeMode: msg.BridgeMode, | ||
| PreviewID: msg.PreviewID, |
There was a problem hiding this comment.
Wire these new open-session fields through LocalService.
InitialURL, BridgeMode, and PreviewID are copied into OpenRequest here, but the only shown service implementation in internal/browser/service.go:41-70 never uses them when starting/opening the browser. As written, sessions opened through this manager still ignore the new protocol fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 78 - 80, The OpenRequest fields
InitialURL, BridgeMode, and PreviewID are populated in manager.go but not
propagated into the LocalService implementation; update the LocalService methods
that create/start sessions (the Open/OpenSession/Start methods in the
LocalService implementation in internal/browser/service.go) to accept and
forward these fields from OpenRequest into the browser startup/open call so the
new protocol fields are used when launching sessions; locate the OpenRequest
type and the LocalService method implementations and ensure InitialURL,
BridgeMode, and PreviewID are read from the OpenRequest and passed into the
underlying browser start/open logic.
Comment on lines
175
to
+194
| _ = m.sender.Send(ctx, &protocol.BrowserFrame{ | ||
| Type: protocol.MsgTypeBrowserFrame, | ||
| BrowserID: browserID, | ||
| SessionID: req.SessionID, | ||
| ChannelID: req.ChannelID, | ||
| Seq: frame.Sequence, | ||
| ContentType: frame.ContentType, | ||
| DataBase64: frame.DataBase64, | ||
| Width: frame.Width, | ||
| Height: frame.Height, | ||
| CapturedAt: frame.CapturedAt, | ||
| Type: protocol.MsgTypeBrowserFrame, | ||
| BrowserID: browserID, | ||
| SessionID: req.SessionID, | ||
| ChannelID: req.ChannelID, | ||
| Seq: frame.Sequence, | ||
| ContentType: frame.ContentType, | ||
| DataBase64: frame.DataBase64, | ||
| Width: frame.Width, | ||
| Height: frame.Height, | ||
| ViewportWidth: frame.ViewportWidth, | ||
| ViewportHeight: frame.ViewportHeight, | ||
| DevicePixelRatio: frame.DevicePixelRatio, | ||
| CapturedAt: frame.CapturedAt, | ||
| }) | ||
| m.mu.Lock() | ||
| if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq { | ||
| current.lastFrameSeq = frame.Sequence | ||
| } | ||
| m.mu.Unlock() |
There was a problem hiding this comment.
Only advance lastFrameSeq after a successful frame send.
lastFrameSeq is updated even when m.sender.Send drops the BrowserFrame, because the send error is ignored. That can make later input look stale relative to a frame the client never received.
Suggested guard
- _ = m.sender.Send(ctx, &protocol.BrowserFrame{
+ if err := m.sender.Send(ctx, &protocol.BrowserFrame{
Type: protocol.MsgTypeBrowserFrame,
BrowserID: browserID,
SessionID: req.SessionID,
ChannelID: req.ChannelID,
Seq: frame.Sequence,
ContentType: frame.ContentType,
DataBase64: frame.DataBase64,
Width: frame.Width,
Height: frame.Height,
ViewportWidth: frame.ViewportWidth,
ViewportHeight: frame.ViewportHeight,
DevicePixelRatio: frame.DevicePixelRatio,
CapturedAt: frame.CapturedAt,
- })
- m.mu.Lock()
- if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq {
- current.lastFrameSeq = frame.Sequence
+ }); err == nil {
+ m.mu.Lock()
+ if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq {
+ current.lastFrameSeq = frame.Sequence
+ }
+ m.mu.Unlock()
}
- m.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 175 - 194, The code advances
current.lastFrameSeq regardless of whether m.sender.Send succeeded; change it to
check the error returned by m.sender.Send(ctx, &protocol.BrowserFrame{...}) and
only update m.byID[browserID].lastFrameSeq when send returns nil. Specifically,
capture the error from m.sender.Send, handle/log it as appropriate, and move the
m.mu.Lock()/unlock and the conditional that updates current.lastFrameSeq (using
current == state && frame.Sequence > current.lastFrameSeq) so it runs only when
the send error is nil; keep references to m.sender.Send, protocol.BrowserFrame,
m.byID[browserID], lastFrameSeq, and frame.Sequence to locate the change.
Comment on lines
+253
to
+265
| state.owner = owner | ||
| state.controlVersion++ | ||
| out := &protocol.BrowserControlClaim{ | ||
| Type: protocol.MsgTypeBrowserControlClaim, | ||
| BrowserID: msg.BrowserID, | ||
| SessionID: state.openRequest.SessionID, | ||
| ChannelID: state.openRequest.ChannelID, | ||
| Owner: string(owner), | ||
| Reason: msg.Reason, | ||
| ControlVersion: state.controlVersion, | ||
| } | ||
| m.mu.Unlock() | ||
| return m.sender.Send(ctx, out) |
There was a problem hiding this comment.
Rollback local control state if the outbound event send fails.
Both Claim and Release update state.owner / state.controlVersion before calling m.sender.Send. If that send fails, the method returns an error but keeps the mutated state, so later ownership and stale-version checks run against a version the peer never saw.
Suggested rollback pattern
+ prevOwner := state.owner
+ prevVersion := state.controlVersion
state.owner = owner
state.controlVersion++
out := &protocol.BrowserControlClaim{
Type: protocol.MsgTypeBrowserControlClaim,
BrowserID: msg.BrowserID,
SessionID: state.openRequest.SessionID,
ChannelID: state.openRequest.ChannelID,
Owner: string(owner),
Reason: msg.Reason,
ControlVersion: state.controlVersion,
}
m.mu.Unlock()
- return m.sender.Send(ctx, out)
+ if err := m.sender.Send(ctx, out); err != nil {
+ m.mu.Lock()
+ if current := m.byID[msg.BrowserID]; current == state {
+ current.owner = prevOwner
+ current.controlVersion = prevVersion
+ }
+ m.mu.Unlock()
+ return err
+ }
+ return nilApply the same rollback pattern in Release.
Also applies to: 287-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 253 - 265, The Claim and Release
flows mutate state.owner and state.controlVersion before calling m.sender.Send,
so if Send fails the local state remains changed; fix by capturing the previous
owner and controlVersion, perform the state mutation, then attempt
m.sender.Send, and if Send returns an error revert state.owner and
state.controlVersion back to the captured values while holding m.mu (apply same
pattern in both Claim and Release around m.sender.Send), ensuring the revert
occurs only on send failure so the local state matches what the peer actually
observed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/browser/manager_test.go (1)
371-380: ⚡ Quick winConsider asserting
ControlVersionin the stale-input ack as well.You already verify reject reason/accepted state; adding one assertion here would cover the newly-forwarded ack metadata too.
Suggested test tightening
for _, msg := range sentMessages { ack, ok := msg.(*protocol.BrowserUserInputAck) if ok && ack.InputID == "input-1" { foundAck = true + if ack.ControlVersion == 0 { + t.Fatalf("ack controlVersion not populated: %+v", ack) + } if ack.Accepted || ack.Reason != protocol.BrowserInputRejectStaleFrame { t.Fatalf("ack = %+v", ack) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager_test.go` around lines 371 - 380, The test currently checks that a BrowserUserInputAck for InputID "input-1" is present and that ack.Accepted is false and ack.Reason equals protocol.BrowserInputRejectStaleFrame; add an assertion that ack.ControlVersion equals the expected forwarded control version (e.g., the version value used when sending the stale input) to validate the forwarded ack metadata. Locate the loop that examines sent.snapshot() and the ack variable of type *protocol.BrowserUserInputAck and add a check against the expected ControlVersion for that stale-input ack.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/browser/manager_test.go`:
- Around line 371-380: The test currently checks that a BrowserUserInputAck for
InputID "input-1" is present and that ack.Accepted is false and ack.Reason
equals protocol.BrowserInputRejectStaleFrame; add an assertion that
ack.ControlVersion equals the expected forwarded control version (e.g., the
version value used when sending the stale input) to validate the forwarded ack
metadata. Locate the loop that examines sent.snapshot() and the ack variable of
type *protocol.BrowserUserInputAck and add a check against the expected
ControlVersion for that stale-input ack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6041f93a-7e0c-45e6-93d8-e02059c9db35
📒 Files selected for processing (1)
internal/browser/manager_test.go
glittercowboy
force-pushed
the
codex/unified-web-surface-daemon
branch
from
ce36791 to
6129557
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/browser/manager.go (2)
253-265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback control state if the outbound claim/release event fails.
Both paths mutate
state.owner/state.controlVersionbefore the send. Ifm.sender.Sendfails, local ownership/version diverges from what the peer observed, and later stale-version checks can reject otherwise valid requests.Suggested pattern
+ prevOwner := state.owner + prevVersion := state.controlVersion state.owner = owner state.controlVersion++ out := &protocol.BrowserControlClaim{ Type: protocol.MsgTypeBrowserControlClaim, BrowserID: msg.BrowserID, SessionID: state.openRequest.SessionID, ChannelID: state.openRequest.ChannelID, Owner: string(owner), Reason: msg.Reason, ControlVersion: state.controlVersion, } m.mu.Unlock() - return m.sender.Send(ctx, out) + if err := m.sender.Send(ctx, out); err != nil { + m.mu.Lock() + if current := m.byID[msg.BrowserID]; current == state { + current.owner = prevOwner + current.controlVersion = prevVersion + } + m.mu.Unlock() + return err + } + return nilApply the same rollback pattern in
Releasewhen that method changes the owner/version.Also applies to: 283-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager.go` around lines 253 - 265, The code updates state.owner and state.controlVersion before calling m.sender.Send (creating a protocol.BrowserControlClaim) which can fail; capture the previous owner and version (e.g., oldOwner, oldVersion) before mutating, call m.sender.Send, and if it returns an error revert state.owner and state.controlVersion back under m.mu to the old values so local state stays consistent with the peer; apply the same rollback pattern to the Release path that mutates state.owner/state.controlVersion (referencing the Release method and the same m.sender.Send usage).
175-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly advance
lastFrameSeqafter a successful frame send.Lines 175-194 still move
lastFrameSeqforward even whenm.sender.Senddrops theBrowserFrame. That makes later input look stale relative to a frame the client never received.Suggested fix
- _ = m.sender.Send(ctx, &protocol.BrowserFrame{ + if err := m.sender.Send(ctx, &protocol.BrowserFrame{ Type: protocol.MsgTypeBrowserFrame, BrowserID: browserID, SessionID: req.SessionID, ChannelID: req.ChannelID, Seq: frame.Sequence, ContentType: frame.ContentType, DataBase64: frame.DataBase64, Width: frame.Width, Height: frame.Height, ViewportWidth: frame.ViewportWidth, ViewportHeight: frame.ViewportHeight, DevicePixelRatio: frame.DevicePixelRatio, CapturedAt: frame.CapturedAt, - }) - m.mu.Lock() - if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq { - current.lastFrameSeq = frame.Sequence + }); err == nil { + m.mu.Lock() + if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq { + current.lastFrameSeq = frame.Sequence + } + m.mu.Unlock() } - m.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/manager.go` around lines 175 - 194, The code advances current.lastFrameSeq regardless of whether m.sender.Send succeeded; change m.sender.Send(ctx, &protocol.BrowserFrame{...}) to capture its returned error and only update lastFrameSeq when send returns nil—i.e., call err := m.sender.Send(...), then if err == nil acquire m.mu.Lock(), lookup current := m.byID[browserID], confirm current == state and frame.Sequence > current.lastFrameSeq, set current.lastFrameSeq = frame.Sequence, and finally m.mu.Unlock(); do not advance lastFrameSeq on send failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager.go`:
- Around line 314-320: The code reads state.owner after releasing m.mu, risking
a data race; capture the owner under lock and use that captured value in the
error/reporting calls. Specifically, inside the block that references
parseControlOwner, OwnerLex, state.openRequest and state.controlVersion, add a
local variable (e.g., capturedOwner := state.owner) before calling
m.mu.Unlock(), then use capturedOwner in the fmt.Errorf message (and any related
logging or error reporting such as the value sent to
m.sendInputAck/BrowserInputRejectOwnerMismatch) instead of reading state.owner
after unlocking.
---
Duplicate comments:
In `@internal/browser/manager.go`:
- Around line 253-265: The code updates state.owner and state.controlVersion
before calling m.sender.Send (creating a protocol.BrowserControlClaim) which can
fail; capture the previous owner and version (e.g., oldOwner, oldVersion) before
mutating, call m.sender.Send, and if it returns an error revert state.owner and
state.controlVersion back under m.mu to the old values so local state stays
consistent with the peer; apply the same rollback pattern to the Release path
that mutates state.owner/state.controlVersion (referencing the Release method
and the same m.sender.Send usage).
- Around line 175-194: The code advances current.lastFrameSeq regardless of
whether m.sender.Send succeeded; change m.sender.Send(ctx,
&protocol.BrowserFrame{...}) to capture its returned error and only update
lastFrameSeq when send returns nil—i.e., call err := m.sender.Send(...), then if
err == nil acquire m.mu.Lock(), lookup current := m.byID[browserID], confirm
current == state and frame.Sequence > current.lastFrameSeq, set
current.lastFrameSeq = frame.Sequence, and finally m.mu.Unlock(); do not advance
lastFrameSeq on send failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a19d3f-5f0f-469e-a8fb-ace0bb0a517f
📒 Files selected for processing (4)
internal/browser/manager.gointernal/browser/manager_test.gointernal/browser/service.gointernal/browser/types.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/browser/types.go
- internal/browser/service.go
- internal/browser/manager_test.go
Comment on lines
+314
to
320
| inputOwner, ownerOK := parseControlOwner(msg.Owner) | ||
| if !ownerOK || inputOwner != OwnerLex || state.owner != OwnerLex { | ||
| req := state.openRequest | ||
| version := state.controlVersion | ||
| m.mu.Unlock() | ||
| _ = m.sendInputAck(ctx, req, msg, false, protocol.BrowserInputRejectOwnerMismatch, version) | ||
| return fmt.Errorf("browser control belongs to %s", state.owner) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the owner-mismatch branch and confirm `state.owner` is read after unlocking.
nl -ba internal/browser/manager.go | sed -n '314,320p'Repository: gsd-build/daemon
Length of output: 101
🏁 Script executed:
sed -n '314,330p' internal/browser/manager.go | cat -nRepository: gsd-build/daemon
Length of output: 957
🏁 Script executed:
sed -n '300,330p' internal/browser/manager.go | cat -nRepository: gsd-build/daemon
Length of output: 1529
🏁 Script executed:
# Find the state struct definition to confirm state.owner is a field
rg -A 20 "type.*state\s*struct" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 42
🏁 Script executed:
# Search for Claim/Release functions that might modify owner
rg "func.*Claim|func.*Release" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 240
🏁 Script executed:
# Look for other similar patterns where state fields are accessed after unlock
rg -B 5 -A 5 "m.mu.Unlock()" internal/browser/manager.go | head -80Repository: gsd-build/daemon
Length of output: 2118
🏁 Script executed:
# Search for "type" and "state" together more broadly
rg "type\s+\w+\s*struct" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 158
🏁 Script executed:
# Look at the Claim function implementation to see if it modifies owner
rg -A 30 "func (m \*Manager) Claim" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 42
🏁 Script executed:
# Look at the Release function implementation
rg -A 30 "func (m \*Manager) Release" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 42
🏁 Script executed:
# Find the sessionState struct definition with its fields
rg -A 20 "type sessionState struct" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 568
🏁 Script executed:
# Search for where owner is assigned or modified
rg "\.owner\s*=" internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 108
🏁 Script executed:
# Look for Claim and Release more broadly
rg "Claim|Release" internal/browser/manager.go | head -20Repository: gsd-build/daemon
Length of output: 432
🏁 Script executed:
# Show context for owner assignments
rg -B 10 "state\.owner = " internal/browser/manager.goRepository: gsd-build/daemon
Length of output: 675
Capture state.owner before unlocking to prevent data race.
Line 21 reads state.owner after m.mu.Unlock() on line 19. Since Claim and Release operations modify this field under lock protection, the error message can report a stale or incorrect owner value due to concurrent modification.
Suggested fix
inputOwner, ownerOK := parseControlOwner(msg.Owner)
if !ownerOK || inputOwner != OwnerLex || state.owner != OwnerLex {
req := state.openRequest
version := state.controlVersion
+ currentOwner := state.owner
m.mu.Unlock()
_ = m.sendInputAck(ctx, req, msg, false, protocol.BrowserInputRejectOwnerMismatch, version)
- return fmt.Errorf("browser control belongs to %s", state.owner)
+ return fmt.Errorf("browser control belongs to %s", currentOwner)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| inputOwner, ownerOK := parseControlOwner(msg.Owner) | |
| if !ownerOK || inputOwner != OwnerLex || state.owner != OwnerLex { | |
| req := state.openRequest | |
| version := state.controlVersion | |
| m.mu.Unlock() | |
| _ = m.sendInputAck(ctx, req, msg, false, protocol.BrowserInputRejectOwnerMismatch, version) | |
| return fmt.Errorf("browser control belongs to %s", state.owner) | |
| inputOwner, ownerOK := parseControlOwner(msg.Owner) | |
| if !ownerOK || inputOwner != OwnerLex || state.owner != OwnerLex { | |
| req := state.openRequest | |
| version := state.controlVersion | |
| currentOwner := state.owner | |
| m.mu.Unlock() | |
| _ = m.sendInputAck(ctx, req, msg, false, protocol.BrowserInputRejectOwnerMismatch, version) | |
| return fmt.Errorf("browser control belongs to %s", currentOwner) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 314 - 320, The code reads
state.owner after releasing m.mu, risking a data race; capture the owner under
lock and use that captured value in the error/reporting calls. Specifically,
inside the block that references parseControlOwner, OwnerLex, state.openRequest
and state.controlVersion, add a local variable (e.g., capturedOwner :=
state.owner) before calling m.mu.Unlock(), then use capturedOwner in the
fmt.Errorf message (and any related logging or error reporting such as the value
sent to m.sendInputAck/BrowserInputRejectOwnerMismatch) instead of reading
state.owner after unlocking.
This was referenced May 1, 2026
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
Dependency / merge order
Verification
GOCACHE=/private/tmp/gsd-daemon-unified-web-surface-gocache GOMODCACHE=/private/tmp/gsd-daemon-unified-web-surface-gomodcache go test ./...GOCACHE=/private/tmp/gsd-daemon-unified-web-surface-gocache GOMODCACHE=/private/tmp/gsd-daemon-unified-web-surface-gomodcache go build -o gsd-cloud .Post-merge
Summary by CodeRabbit
New Features
Bug Fixes