Skip to content

fix(computers): a restarted desktop clears the X lock a killed X left #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
earakely-scale wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(computers): a restarted desktop clears the X lock a killed X left #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/agent_env/computers/artifacts/ubuntu-24.04/start-desktop
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ case "${1:-}" in
exec /usr/bin/supervisord -c /etc/supervisor/supervisord.conf
;;
xvfb)
# supervisord runs one X at a time, so a lock here is one a killed X left, and X refuses to start while the pid
# it names is taken, which after a restart it often is.
rm -f "/tmp/.X${DISPLAY#:}-lock"
exec Xvfb "$DISPLAY" -screen 0 "${resolution}x24" -nolisten tcp -ac
;;
with-x)
Expand Down
14 changes: 14 additions & 0 deletions tst/integration/computer/base_computer_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,20 @@ def test_the_desktop_comes_back_however_often_x_dies(base):
assert "window id #" in wm


def test_the_desktop_comes_back_from_a_restart_that_killed_x_with_its_lock_naming_a_running_pid(base):
"""A restart gives pids out again, so the lock a killed X left can name a running process; here tini, pid 1."""
name = f"agentenv-test-base-{uuid.uuid4().hex[:8]}"
docker("run", "-d", "--name", name, base)
try:
_all_running(name)
_exec(name, "sh", "-c", 'printf "%10d\\n" 1 > /tmp/lock && chmod 444 /tmp/lock && mv -f /tmp/lock /tmp/.X99-lock')
docker("restart", "-t", "0", name)
running = _all_running(name)
finally:
subprocess.run(["docker", "rm", "-f", name], capture_output=True)
assert all(" RUNNING " in line for line in running.splitlines()), running


@pytest.mark.parametrize("setting, value, error", [
Comment on lines +98 to 101

@greptile-apps greptile-apps Bot Oct 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Failed startup passes the test

The new test passes if _all_running(name) returns an empty string after the restart. That helper returns the last command's output even when every attempt fails, and all(...) over empty output is true. Assert that running is nonempty before checking its lines so a desktop that never starts cannot pass this regression test.

Suggested change
assert all(" RUNNING " in line for line in running.splitlines()), running
@pytest.mark.parametrize("setting, value, error", [
assert running and all(" RUNNING " in line for line in running.splitlines()), running
@pytest.mark.parametrize("setting, value, error", [
Prompt To Fix With AI 
This is a comment left during a code review.
Path: tst/integration/computer/base_computer_test.py
Line: 98-101

Comment:
**Failed startup passes the test**

The new test passes if `_all_running(name)` returns an empty string after the restart. That helper returns the last command's output even when every attempt fails, and `all(...)` over empty output is true. Assert that `running` is nonempty before checking its lines so a desktop that never starts cannot pass this regression test.

```suggestion
    assert running and all(" RUNNING " in line for line in running.splitlines()), running

@pytest.mark.parametrize("setting, value, error", [
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Cursor Fix in Claude Code Fix in Codex

("RESOLUTION", "big", "resolution is <width>x<height>, each 1 to 32767 pixels, not 'big'"),
("RESOLUTION", "40000x800", "resolution is <width>x<height>, each 1 to 32767 pixels, not '40000x800'"),
Expand Down
7 changes: 5 additions & 2 deletions tst/integration/computer/recording_chaos_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,17 +124,20 @@ async def _desk(image: str, *docker_args: str):

async def _ready(name: str) -> Driver:
"""A client for the container's driver once its computer is ready; a restart may move its port."""
status = None
for _ in range(240):
port = subprocess.run(["docker", "port", name, str(PORT)], capture_output=True, text=True).stdout
if port:
try:
driver = await Driver.connect(f"http://{port.splitlines()[0]}", 30)
if (await driver.invoke(COMPUTER_READY, "status"))["ready"]:
status = await driver.invoke(COMPUTER_READY, "status")
if status["ready"]:
return driver
except Exception: # not serving yet
pass
await asyncio.sleep(0.25)
raise TimeoutError(f"{name}'s computer never became ready")
failing = [check for check in (status or {}).get("checks", []) if not check["ok"]]
raise TimeoutError(f"{name}'s computer never became ready: {failing or 'its driver never answered'}")


async def _layout(driver: Driver) -> dict:
Expand Down
Loading