fix(computer-use): serialize X11 screenshot access#5049
fix(computer-use): serialize X11 screenshot access#5049Tushar-Khandelwal-2004 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libs/computer-use/pkg/computeruse/display.go">
<violation number="1" location="libs/computer-use/pkg/computeruse/display.go:106">
P2: X11 serialization lock held across expensive non-X11 CPU work (image encoding, base64)</violation>
</file>
<file name="libs/computer-use/pkg/computeruse/screenshot_capture.go">
<violation number="1" location="libs/computer-use/pkg/computeruse/screenshot_capture.go:25">
P2: Primary-display bounds cache can become stale across display restarts/geometry changes, so full-screen screenshots may capture with outdated coordinates.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| } | ||
|
|
||
| func capturePrimaryDisplay() (*image.RGBA, error) { | ||
| if !primaryDisplayBoundsCached || primaryDisplayBounds.Empty() { |
There was a problem hiding this comment.
P2: Primary-display bounds cache can become stale across display restarts/geometry changes, so full-screen screenshots may capture with outdated coordinates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/computer-use/pkg/computeruse/screenshot_capture.go, line 25:
<comment>Primary-display bounds cache can become stale across display restarts/geometry changes, so full-screen screenshots may capture with outdated coordinates.</comment>
<file context>
@@ -0,0 +1,65 @@
+}
+
+func capturePrimaryDisplay() (*image.RGBA, error) {
+ if !primaryDisplayBoundsCached || primaryDisplayBounds.Empty() {
+ primaryDisplayBounds = screenshot.GetDisplayBounds(0)
+ primaryDisplayBoundsCached = true
</file context>
Signed-off-by: Tushar-Khandelwal-2004 <khandelwal.tushar8527@gmail.com>
bfeea9d to
98779fe
Compare
|
View your CI Pipeline Execution ↗ for commit 98779fe
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
|
@cubic-dev-ai review |
@mu-hashmi I have started the AI code review. It will take a few minutes to complete. |
|
@Tushar-Khandelwal-2004 could you please update the PR body to match the current diff? It still mentions caching primary display bounds and resetting that cache on Start/Stop, but the current commit doesn’t include that cache/reset logic. |
|
Thanks for catching that @rovle. I updated the PR body so it matches the current diff and removed the stale wording. |
|
I looked a bit into this and I don’t think this PR fully fixes the reported X11 client exhaustion. I reproduced the issue through the real Computer Use HTTP wrappers + real The remaining leak appears to be in So unless I missed something, my read is that this PR is a concurrency mitigation for screenshot/display access, but not a complete fix for the reported X11 client exhaustion. A complete fix should avoid that |
|
Good catch. The serialization only prevented concurrent leaks but didn't fix the monotonic growth per call. The root cause is robotgo.GetTitle opening an xgbutil.NewConn() internally per window title lookup with no close. Working on replacing that path with a single explicit xgbutil connection that enumerates windows and titles in one shot and defers close, targeting the /computeruse/display/windows repro directly. Will push shortly. |
|
Will take a closer look later, but just a note that currently this PR changes the semantics of |
|
yeah my bad, restored both ID is PID again via |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="libs/computer-use/pkg/computeruse/getwindows_linux.go">
<violation number="1" location="libs/computer-use/pkg/computeruse/getwindows_linux.go:48">
P0: `WindowInfo.ID` is set to the process PID instead of the X11 window ID (`win`), causing multi-window apps to be silently deduplicated and the returned ID to be semantically wrong (PID, not a window handle).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
|
||
| seen[pid] = true | ||
| windows = append(windows, computeruse.WindowInfo{ | ||
| ID: int(pid), |
There was a problem hiding this comment.
P0: WindowInfo.ID is set to the process PID instead of the X11 window ID (win), causing multi-window apps to be silently deduplicated and the returned ID to be semantically wrong (PID, not a window handle).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At libs/computer-use/pkg/computeruse/getwindows_linux.go, line 48:
<comment>`WindowInfo.ID` is set to the process PID instead of the X11 window ID (`win`), causing multi-window apps to be silently deduplicated and the returned ID to be semantically wrong (PID, not a window handle).</comment>
<file context>
@@ -37,8 +43,9 @@ func getWindowsX11() ([]computeruse.WindowInfo, error) {
+ seen[pid] = true
windows = append(windows, computeruse.WindowInfo{
- ID: int(win),
+ ID: int(pid),
Title: title,
Position: computeruse.Position{
</file context>
2c15a7f to
b40f732
Compare
Summary
Fixes #5047.
Serialize X11 screenshot/display access through a shared mutex and replace the Linux GetWindows robotgo title lookup with direct X11 window enumeration.
What Changed
Why
Repeated calls to
/computeruse/display/windowscould monotonically increase X11 client count becauserobotgo.GetTitleopens xgbutil connections internally on Linux without closing them. Serialization alone did not fix this since the leak is monotonic even without concurrency. The complete fix owns the X11 window listing directly so connection lifecycle is deterministic.Testing