feat(lume, computer-server): VNC backend rewrite and VirtioFS port discovery - #1205
Conversation
…scovery Rewrite the VNC backend to use Twisted's global reactor directly instead of vncdotool's api.connect(), which deadlocks inside uvicorn/asyncio on Python 3.13. Each VNC operation now creates a fresh connection via reactor.callFromThread() with threading.Event synchronization. Add VirtioFS-based VNC port discovery: lume creates a shared "lume-config" directory, writes vnc.env (port + password) after VNC starts, and the guest reads it on boot — eliminating hardcoded VNC port/password defaults. Key changes: - vnc.py: global reactor pattern, @defer.inlineCallbacks closures, manual drag (replaces mouseDrag/doPoll), client.screen fix - VM.swift: VirtioFS lume-config share, VNC URL parsing fix (URLComponents with vnc:// → http:// replacement) - cli.py: only override CUA_VNC_PORT from CLI when explicitly provided - setup-cua.sh: renamed from setup-cua-computer.sh, added "already mounted" check for lume-config VirtioFS share Tested E2E: 19/19 operations passing (screenshot, click, type, scroll, drag, hotkey, cursor position, screen size).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📦 Publishable packages changed
Add |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce optional VNC password configuration throughout the Lume VM startup pipeline, expand the CUA setup script with noVNC web interface support and auto-login capabilities, and refactor the Python VNC backend from persistent client connections to per-operation connections using Twisted reactor-based lifecycle management. Changes
Sequence DiagramsequenceDiagram
participant User
participant Run as Run Command
participant Ctrl as LumeController
participant VM as VM
participant VNC as VNCService
participant SharedDir as Shared Directory<br/>(lume-config)
User->>Run: lume run --vnc-password secret
Run->>Ctrl: runVM(vncPassword: "secret")
Ctrl->>VM: run(vncPassword: "secret")
VM->>VM: Create "lume-config"<br/>shared directory
VM->>VNC: startVNCService(password: "secret")
VNC->>VNC: Use provided password<br/>for VNC auth
VNC-->>VM: Return vncInfo URL
VM->>VM: Parse VNC port & password<br/>from vncInfo
VM->>SharedDir: Write vnc.env<br/>(VNC_PORT, VNC_PASSWORD)
SharedDir-->>VM: File written
VM-->>User: VNC ready with<br/>custom password
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
f-trycua
added
release:lume
📦 Publishable packages changed
Add |
📦 Publishable packages changed
|
- Fix isort ordering in vnc.py (twisted.internet imports) - Add vncPassword parameter to MockVM.run() to match VM superclass - Regenerate lume CLI and HTTP API reference docs
📦 Publishable packages changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/lume/src/VNC/VNCService.swift (1)
72-109:⚠️ Potential issue | 🟠 MajorEscape or validate custom VNC passwords before building the URL.
With the new
passwordparameter, values containing reserved userinfo characters such as@,:,/,?, or#will corrupt the assembledvncURL below. That can breakURLComponentsparsing in the session loader andvnc.envwriter even though VNC authentication itself succeeded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/src/VNC/VNCService.swift` around lines 72 - 109, The code builds vnc URL strings by interpolating raw password into "vnc://:\(password)@..." which breaks if password contains reserved userinfo characters; update start(port:password:virtualMachine:) to percent-encode or validate the password before using it in URLs (keep self.vncPassword as the original for auth). Replace manual string interpolation with URLComponents (scheme "vnc", host "127.0.0.1"/hostIP, port Int(assignedPort)) and set the user/password via URLComponents.user and URLComponents.password, or explicitly percent-encode the password using addingPercentEncoding(withAllowedCharacters: .urlUserAllowed) and use that encoded value when constructing the local and external URL strings; if encoding fails, throw a VMError (e.g., vncInvalidPassword) to avoid emitting malformed URLs.
🧹 Nitpick comments (1)
libs/lume/tests/Mocks/MockVNCService.swift (1)
32-35: Mirror the real VNC URL contract in the mock.
VM.run()now extracts bothcomponents.passwordand the assigned port fromvncInfo. This mock still dropspasswordand returnsvnc://localhost:\(port), so tests bypass the new password/auto-port discovery path entirely.Suggested fix
func start(port: Int, password: String? = nil, virtualMachine: Any?) async throws { isRunning = true - url = "vnc://localhost:\(port)" + let resolvedPort = port == 0 ? 5901 : port + let resolvedPassword = password ?? "mock-password" + url = "vnc://:\(resolvedPassword)@localhost:\(resolvedPort)" _attachedVM = virtualMachine }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/lume/tests/Mocks/MockVNCService.swift` around lines 32 - 35, The mock start(port:password:virtualMachine:) currently ignores the password and always returns "vnc://localhost:\(port)", so tests skip the VM.run() path that reads vncInfo.components.password and the auto-assigned port; update MockVNCService.start to populate url with both the password (when non-nil) and the effective port (simulate auto-assigned port if port==0 by picking a non-zero port), e.g. emit a vnc URL that includes the password component and the chosen port, and leave isRunning and _attachedVM behavior unchanged so VM.run() can read components.password and the assigned port from the mock vncInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/lume/scripts/setup-cua.sh`:
- Around line 419-425: The script currently uses eval on the contents of
LUME_CONFIG and later injects LUME_VNC_PASSWORD unescaped into shell and XML,
which allows code execution or malformed output; replace eval "$(cat
"$LUME_CONFIG")" with a safe parser that reads LUME_CONFIG line-by-line (e.g.,
while IFS='=' read -r key val; do case "$key" in VNC_PORT)
LUME_VNC_PORT="$val";; VNC_PASSWORD) LUME_VNC_PASSWORD="$val";; esac; done <
"$LUME_CONFIG" so values are not executed, preserve characters by using read -r
and no word-splitting, and when writing the password into shell scripts or
plist/XML use proper escaping (e.g., use printf '%s' "$LUME_VNC_PASSWORD" into
files and replace &,<,>,",\' with XML entities or write the value inside a
single-quoted shell heredoc or use xmllint/PlistBuddy when available) so special
characters like $,`,/,&,<,> are not interpreted or break the output; update
every injection site where LUME_VNC_PASSWORD or LUME_VNC_PORT is inserted
(references: LUME_CONFIG, LUME_VNC_PASSWORD, LUME_VNC_PORT, LOG_FILE) to use the
safe read and escaping approach.
In `@libs/lume/src/VM/VM.swift`:
- Around line 218-228: The deterministic lume-config host path (lumeConfigDir)
can serve stale vnc.env across runs; change creation to use a unique per-run
directory (e.g., append a UUID or run-timestamp to
"lume-config-\(vmDirContext.name)") and mirror this change for the USB-storage
path creation code, and ensure the directory is removed during VM shutdown/stop
(implement cleanup in the VM stop/teardown routine so the lumeConfigDir and the
USB-storage temp dir are deleted and not reused). Use the same unique-name
pattern where SharedDirectory is constructed (lumeConfigSharedDir and the USB
shared directory) and add removal logic in the VM stop/teardown function to
delete those temporary host directories.
In `@libs/python/computer-server/computer_server/cli.py`:
- Around line 123-125: The current check using "if args.vnc_port != 5900 or
'CUA_VNC_PORT' not in os.environ" cannot tell if the user explicitly passed
--vnc-port 5900; change the CLI to make args.vnc_port default to None (or
argparse.SUPPRESS) and then set the env only when the arg is provided: use "if
args.vnc_port is not None: os.environ['CUA_VNC_PORT'] = str(args.vnc_port)".
Update the argparse.add_argument call that defines vnc_port and the conditional
around os.environ["CUA_VNC_PORT"] accordingly.
In `@libs/python/computer-server/computer_server/handlers/vnc.py`:
- Around line 135-145: The reactor startup must be serialized to avoid
concurrent reactor.run() calls: add a module-level threading.Lock (e.g.,
reactor_start_lock) and a threading.Event (e.g., reactor_started_event), then in
_with_client() wrap the "if not reactor.running" check and reactor thread spawn
inside reactor_start_lock to ensure only one thread starts the reactor; after
starting the thread set reactor_started_event from the reactor thread (or
wait-loop until reactor.running is true) and have callers wait on
reactor_started_event.wait(timeout=...) before calling
reactor.callFromThread(_work) and done_event.wait(...). Reference: reactor,
reactor.run, _with_client(), done_event, threading.Thread; ensure you release
the lock quickly and use the event to avoid races where reactor.running is not
yet true immediately after starting the thread.
- Around line 117-156: The _with_client() flow never closes the VNC client
transport, leaking TCP sessions; update the inner _do() in _with_client to
always close the connection in its finally block: after obtaining client = yield
factory.deferred and after the work (or on exception), check if client is not
None and has a transport and call client.transport.loseConnection() (or
client.close() if the client exposes that) to ensure the TCP session is closed;
keep the done_event.set() but move/extend the finally to close the transport so
connections are cleaned up on both success and error (references: _with_client,
inner _do, factory.deferred, client.transport).
---
Outside diff comments:
In `@libs/lume/src/VNC/VNCService.swift`:
- Around line 72-109: The code builds vnc URL strings by interpolating raw
password into "vnc://:\(password)@..." which breaks if password contains
reserved userinfo characters; update start(port:password:virtualMachine:) to
percent-encode or validate the password before using it in URLs (keep
self.vncPassword as the original for auth). Replace manual string interpolation
with URLComponents (scheme "vnc", host "127.0.0.1"/hostIP, port
Int(assignedPort)) and set the user/password via URLComponents.user and
URLComponents.password, or explicitly percent-encode the password using
addingPercentEncoding(withAllowedCharacters: .urlUserAllowed) and use that
encoded value when constructing the local and external URL strings; if encoding
fails, throw a VMError (e.g., vncInvalidPassword) to avoid emitting malformed
URLs.
---
Nitpick comments:
In `@libs/lume/tests/Mocks/MockVNCService.swift`:
- Around line 32-35: The mock start(port:password:virtualMachine:) currently
ignores the password and always returns "vnc://localhost:\(port)", so tests skip
the VM.run() path that reads vncInfo.components.password and the auto-assigned
port; update MockVNCService.start to populate url with both the password (when
non-nil) and the effective port (simulate auto-assigned port if port==0 by
picking a non-zero port), e.g. emit a vnc URL that includes the password
component and the chosen port, and leave isRunning and _attachedVM behavior
unchanged so VM.run() can read components.password and the assigned port from
the mock vncInfo.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 181f9ee0-c133-41b6-b1c8-2b695b403e4d
📒 Files selected for processing (8)
libs/lume/scripts/setup-cua.shlibs/lume/src/Commands/Run.swiftlibs/lume/src/LumeController.swiftlibs/lume/src/VM/VM.swiftlibs/lume/src/VNC/VNCService.swiftlibs/lume/tests/Mocks/MockVNCService.swiftlibs/python/computer-server/computer_server/cli.pylibs/python/computer-server/computer_server/handlers/vnc.py
| # Wait up to 10s for vnc.env to appear (written by host after VNC starts) | ||
| for i in $(seq 1 10); do | ||
| if [ -f "$LUME_CONFIG" ]; then | ||
| eval "$(cat "$LUME_CONFIG")" | ||
| LUME_VNC_PORT="${VNC_PORT:-}" | ||
| LUME_VNC_PASSWORD="${VNC_PASSWORD:-}" | ||
| echo "Read VNC config from lume shared dir: port=$LUME_VNC_PORT" >> "$LOG_FILE" |
There was a problem hiding this comment.
Avoid eval and raw template injection for VNC secrets.
Line 422 executes the contents of vnc.env, and Lines 458 and 493-494 inject the same password into shell/XML without escaping. A password containing $, backticks, /, &, <, or > will either execute code or produce a broken startup script/plist.
Also applies to: 455-458, 489-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/scripts/setup-cua.sh` around lines 419 - 425, The script currently
uses eval on the contents of LUME_CONFIG and later injects LUME_VNC_PASSWORD
unescaped into shell and XML, which allows code execution or malformed output;
replace eval "$(cat "$LUME_CONFIG")" with a safe parser that reads LUME_CONFIG
line-by-line (e.g., while IFS='=' read -r key val; do case "$key" in VNC_PORT)
LUME_VNC_PORT="$val";; VNC_PASSWORD) LUME_VNC_PASSWORD="$val";; esac; done <
"$LUME_CONFIG" so values are not executed, preserve characters by using read -r
and no word-splitting, and when writing the password into shell scripts or
plist/XML use proper escaping (e.g., use printf '%s' "$LUME_VNC_PASSWORD" into
files and replace &,<,>,",\' with XML entities or write the value inside a
single-quoted shell heredoc or use xmllint/PlistBuddy when available) so special
characters like $,`,/,&,<,> are not interpreted or break the output; update
every injection site where LUME_VNC_PASSWORD or LUME_VNC_PORT is inserted
(references: LUME_CONFIG, LUME_VNC_PASSWORD, LUME_VNC_PORT, LOG_FILE) to use the
safe read and escaping approach.
| // Create a lume-config shared directory so the guest can discover | ||
| // the VNC port/password at boot. The directory is created empty now | ||
| // and populated after the VNC server starts (VirtioFS exposes live | ||
| // host directory contents, so the guest will see the file once written). | ||
| let lumeConfigDir = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("lume-config-\(vmDirContext.name)") | ||
| try? FileManager.default.createDirectory(at: lumeConfigDir, withIntermediateDirectories: true) | ||
| let lumeConfigSharedDir = SharedDirectory( | ||
| hostPath: lumeConfigDir.path, tag: "lume-config", readOnly: true) | ||
| var allSharedDirectories = sharedDirectories | ||
| allSharedDirectories.append(lumeConfigSharedDir) |
There was a problem hiding this comment.
Reusing the same lume-config path can serve stale vnc.env.
The host path is deterministic per VM name and never cleared. On a restart, the guest wait loop can see the previous run's vnc.env immediately and keep the old port/password if this run has not rewritten it yet—or if startup fails before rewrite, the stale file persists indefinitely.
Suggested fix
- let lumeConfigDir = FileManager.default.temporaryDirectory
- .appendingPathComponent("lume-config-\(vmDirContext.name)")
- try? FileManager.default.createDirectory(at: lumeConfigDir, withIntermediateDirectories: true)
+ let lumeConfigDir = FileManager.default.temporaryDirectory
+ .appendingPathComponent("lume-config-\(vmDirContext.name)-\(UUID().uuidString)")
+ try FileManager.default.createDirectory(at: lumeConfigDir, withIntermediateDirectories: true)Apply the same change to the USB-storage path, and clean the directory up on stop.
Also applies to: 263-274, 930-937, 954-962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/lume/src/VM/VM.swift` around lines 218 - 228, The deterministic
lume-config host path (lumeConfigDir) can serve stale vnc.env across runs;
change creation to use a unique per-run directory (e.g., append a UUID or
run-timestamp to "lume-config-\(vmDirContext.name)") and mirror this change for
the USB-storage path creation code, and ensure the directory is removed during
VM shutdown/stop (implement cleanup in the VM stop/teardown routine so the
lumeConfigDir and the USB-storage temp dir are deleted and not reused). Use the
same unique-name pattern where SharedDirectory is constructed
(lumeConfigSharedDir and the USB shared directory) and add removal logic in the
VM stop/teardown function to delete those temporary host directories.
| # Only override env vars from CLI args when explicitly provided | ||
| if args.vnc_port != 5900 or "CUA_VNC_PORT" not in os.environ: | ||
| os.environ["CUA_VNC_PORT"] = str(args.vnc_port) |
There was a problem hiding this comment.
--vnc-port 5900 still can't override an inherited env value.
argparse always yields 5900 here, so Line 124 cannot distinguish “option omitted” from “user explicitly passed --vnc-port 5900”. If CUA_VNC_PORT is already set by the guest, an explicit CLI override back to 5900 is silently ignored, and the log at Line 129 can report the wrong effective port.
Suggested fix
- parser.add_argument(
- "--vnc-port",
- type=int,
- default=5900,
- help="VNC server port (default: 5900)",
- )
+ parser.add_argument(
+ "--vnc-port",
+ type=int,
+ default=None,
+ help="VNC server port (default: 5900)",
+ )
...
- if args.vnc_port != 5900 or "CUA_VNC_PORT" not in os.environ:
- os.environ["CUA_VNC_PORT"] = str(args.vnc_port)
+ if args.vnc_port is not None:
+ os.environ["CUA_VNC_PORT"] = str(args.vnc_port)
+ elif "CUA_VNC_PORT" not in os.environ:
+ os.environ["CUA_VNC_PORT"] = "5900"
...
- logger.info(f"VNC backend enabled → {vnc_host}:{args.vnc_port}")
+ logger.info(f"VNC backend enabled → {vnc_host}:{os.environ['CUA_VNC_PORT']}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/computer-server/computer_server/cli.py` around lines 123 - 125,
The current check using "if args.vnc_port != 5900 or 'CUA_VNC_PORT' not in
os.environ" cannot tell if the user explicitly passed --vnc-port 5900; change
the CLI to make args.vnc_port default to None (or argparse.SUPPRESS) and then
set the env only when the arg is provided: use "if args.vnc_port is not None:
os.environ['CUA_VNC_PORT'] = str(args.vnc_port)". Update the
argparse.add_argument call that defines vnc_port and the conditional around
os.environ["CUA_VNC_PORT"] accordingly.
| def _work(): | ||
| factory = VNCDoToolFactory() | ||
| factory.password = self._password or None | ||
|
|
||
| @defer.inlineCallbacks | ||
| def _do(): | ||
| try: | ||
| reactor.connectTCP(self._host, self._port, factory) | ||
| client = yield factory.deferred | ||
| res = yield defer.maybeDeferred(fn, client) | ||
| result_holder[0] = res | ||
| except Exception as e: | ||
| error_holder[0] = e | ||
| finally: | ||
| done_event.set() | ||
|
|
||
| _do() | ||
|
|
||
| # Ensure the reactor is running in a background thread | ||
| if not reactor.running: | ||
| t = threading.Thread( | ||
| target=reactor.run, | ||
| kwargs={"installSignalHandlers": False}, | ||
| daemon=True, | ||
| ) | ||
| client.timeout = 30 | ||
| self._client = client | ||
| logger.info("VNC connected") | ||
| return client | ||
| t.start() | ||
|
|
||
| reactor.callFromThread(_work) | ||
| done_event.wait(timeout=30) | ||
| if not done_event.is_set(): | ||
| raise TimeoutError("VNC operation timed out") | ||
| if error_holder[0] is not None: | ||
| raise error_holder[0] | ||
| return result_holder[0] | ||
|
|
||
| def disconnect(self): | ||
| with self._lock: | ||
| if self._client is not None: | ||
| try: | ||
| self._client.disconnect() | ||
| except Exception: | ||
| pass | ||
| self._client = None | ||
| pass # No persistent connection to close | ||
|
|
||
| def _reset_on_error(self): | ||
| """Disconnect so next call reconnects.""" | ||
| try: | ||
| self.disconnect() | ||
| except Exception: | ||
| pass | ||
| pass # No persistent connection to reset |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 24741
Close VNC transport in finally block to prevent connection leaks.
_with_client() creates a new TCP session for every operation but never closes client.transport. The finally block only sets the done event, leaving the connection open indefinitely. This contradicts the docstring claim of a "connect, execute, disconnect" lifecycle and causes accumulation of live VNC sessions over multiple operations.
Suggested fix
`@defer.inlineCallbacks`
def _do():
+ client = None
try:
reactor.connectTCP(self._host, self._port, factory)
client = yield factory.deferred
res = yield defer.maybeDeferred(fn, client)
result_holder[0] = res
except Exception as e:
error_holder[0] = e
finally:
+ if client is not None and getattr(client, "transport", None) is not None:
+ client.transport.loseConnection()
done_event.set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/computer-server/computer_server/handlers/vnc.py` around lines 117
- 156, The _with_client() flow never closes the VNC client transport, leaking
TCP sessions; update the inner _do() in _with_client to always close the
connection in its finally block: after obtaining client = yield factory.deferred
and after the work (or on exception), check if client is not None and has a
transport and call client.transport.loseConnection() (or client.close() if the
client exposes that) to ensure the TCP session is closed; keep the
done_event.set() but move/extend the finally to close the transport so
connections are cleaned up on both success and error (references: _with_client,
inner _do, factory.deferred, client.transport).
| # Ensure the reactor is running in a background thread | ||
| if not reactor.running: | ||
| t = threading.Thread( | ||
| target=reactor.run, | ||
| kwargs={"installSignalHandlers": False}, | ||
| daemon=True, | ||
| ) | ||
| client.timeout = 30 | ||
| self._client = client | ||
| logger.info("VNC connected") | ||
| return client | ||
| t.start() | ||
|
|
||
| reactor.callFromThread(_work) | ||
| done_event.wait(timeout=30) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n libs/python/computer-server/computer_server/handlers/vnc.py | head -200Repository: trycua/cua
Length of output: 7872
🏁 Script executed:
# Search for the _with_client function and surrounding context
rg -n "_with_client|reactor" libs/python/computer-server/computer_server/handlers/vnc.py -B 5 -A 15Repository: trycua/cua
Length of output: 10734
🏁 Script executed:
# Check for any threading synchronization (locks, events, etc.) in the file
rg -n "Lock|Condition|Event|RLock|Semaphore|threading" libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 163
🌐 Web query:
Twisted reactor.running multiple threads concurrent start behavior
💡 Result:
Twisted's reactor is a singleton and must run in a single thread. Calling reactor.run (which internally calls startRunning) concurrently from multiple threads is not supported and will lead to undefined behavior, such as hangs, crashes, or corrupted data, because most Twisted APIs are not thread-safe except for specific methods like callFromThread. Key points: - The reactor is designed to run in one "reactor thread" where reactor.run executes the event loop. - reactor.run can only be called once per process; subsequent calls are prevented by internal state flags (_started, running). - From other threads, use reactor.callFromThread or reactor.callInThread to safely interact with the reactor. - Running reactor.run in a background thread (e.g., Thread(target=reactor.run, args=(False,)).start) is possible with some reactors but requires careful coordination; all other Twisted calls must then use thread-safe APIs from that thread, and non-reactor threads must use callFromThread. - Concurrent calls to reactor.run or startRunning from multiple threads simultaneously will race on shared state without locks, causing failures. To handle concurrency, use the reactor's thread pool (callInThread/deferToThread) for blocking work or integrate with other event loops via threaded reactors.
Citations:
- 1: https://docs.twisted.org/en/stable/core/howto/threading.html
- 2: https://docs.twistedmatrix.com/en/stable/core/howto/threading.html
- 3: https://twisted.org/documents/8.2.0/api/twisted.internet.base.ReactorBase.html
- 4: https://twisted.org/documents/21.2.0/api/twisted.internet.interfaces.IReactorCore.html
- 5: https://stackoverflow.com/questions/14274916/execute-twisted-reactor-run-in-a-thread
- 6: https://stackoverflow.com/questions/4084090/multiple-reactors-main-loops-in-one-application-through-threading-or-alternat
- 7: https://docs.twistedmatrix.com/en/twisted-21.2.0/api/twisted.internet.base.ReactorBase.html
🌐 Web query:
Twisted can only have one running reactor instance
💡 Result:
Yes, Twisted can only have one running reactor instance per process. The reactor acts as a global singleton: importing twisted.internet.reactor provides a reference to the single instance, which is automatically installed if none exists. Attempting to install a different reactor after one is already present raises ReactorAlreadyInstalledError. Similarly, calling reactor.run multiple times in the same process raises ReactorAlreadyRunning. Official documentation explicitly states that multiple reactors are not currently possible, though code should prefer passing the reactor explicitly for future compatibility. For multiple reactors, separate processes are required.
Citations:
- 1: https://docs.twisted.org/en/latest/api/twisted.internet.reactor.html
- 2: https://docs.twisted.org/en/latest/core/howto/reactor-basics.html
- 3: https://docs.twisted.org/en/twisted-22.8.0/api/twisted.internet.error.ReactorAlreadyInstalledError.html
- 4: https://docs.twisted.org/en/stable/api/twisted.internet.reactor.html
Reactor bootstrap is still racy under concurrent first use.
The if not reactor.running check is unsynchronized. Two requests hitting _with_client() at the same time can both spawn reactor.run() before the first thread flips that flag. Twisted only supports a single running reactor instance; concurrent calls to reactor.run() from multiple threads cause undefined behavior (hangs, crashes, or data corruption).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/python/computer-server/computer_server/handlers/vnc.py` around lines 135
- 145, The reactor startup must be serialized to avoid concurrent reactor.run()
calls: add a module-level threading.Lock (e.g., reactor_start_lock) and a
threading.Event (e.g., reactor_started_event), then in _with_client() wrap the
"if not reactor.running" check and reactor thread spawn inside
reactor_start_lock to ensure only one thread starts the reactor; after starting
the thread set reactor_started_event from the reactor thread (or wait-loop until
reactor.running is true) and have callers wait on
reactor_started_event.wait(timeout=...) before calling
reactor.callFromThread(_work) and done_event.wait(...). Reference: reactor,
reactor.run, _with_client(), done_event, threading.Thread; ensure you release
the lock quickly and use the event to avoid races where reactor.running is not
yet true immediately after starting the thread.
| self._with_client(lambda c: c.mouseMove(x, y)) | ||
| self._cursor_x = x | ||
| self._cursor_y = y | ||
|
|
||
| def mouse_click(self, x: int, y: int, button: int = 1, clicks: int = 1): | ||
| client = self._ensure_connected() | ||
| client.mouseMove(x, y) | ||
| from twisted.internet import defer | ||
|
|
||
| @defer.inlineCallbacks | ||
| def _do(client): | ||
| yield client.mouseMove(x, y) | ||
| for _ in range(clicks): | ||
| yield client.mousePress(button) | ||
| self._with_client(_do) | ||
| self._cursor_x = x | ||
| self._cursor_y = y | ||
| for _ in range(clicks): | ||
| client.mousePress(button) | ||
|
|
||
| def mouse_down(self, x: int, y: int, button: int = 1): | ||
| client = self._ensure_connected() | ||
| client.mouseMove(x, y) | ||
| from twisted.internet import defer | ||
|
|
||
| @defer.inlineCallbacks | ||
| def _do(client): | ||
| yield client.mouseMove(x, y) | ||
| yield client.mouseDown(button) | ||
| self._with_client(_do) | ||
| self._cursor_x = x | ||
| self._cursor_y = y | ||
| client.mouseDown(button) | ||
|
|
||
| def mouse_up(self, x: int, y: int, button: int = 1): | ||
| client = self._ensure_connected() | ||
| client.mouseMove(x, y) | ||
| from twisted.internet import defer | ||
|
|
||
| @defer.inlineCallbacks | ||
| def _do(client): | ||
| yield client.mouseMove(x, y) | ||
| yield client.mouseUp(button) | ||
| self._with_client(_do) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and understand the VNC handler
wc -l libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 117
🏁 Script executed:
# Let's look at the methods in question and the _with_client implementation
sed -n '170,220p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1635
🏁 Script executed:
# Look at the drag_to method and lines 437-455
sed -n '430,460p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1218
🏁 Script executed:
# Let's find the _with_client implementation
grep -n "_with_client" libs/python/computer-server/computer_server/handlers/vnc.py | head -20Repository: trycua/cua
Length of output: 841
🏁 Script executed:
# Look for the class definition and _with_client method
sed -n '1,100p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 3175
🏁 Script executed:
# Look at _with_client implementation
sed -n '102,125p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 888
🏁 Script executed:
# Find and examine drag_to method
grep -n "def drag_to" libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 158
🏁 Script executed:
# Look at drag_to implementation
sed -n '213,245p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1270
🏁 Script executed:
# Get the methods around lines 437-455
sed -n '430,460p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1218
🏁 Script executed:
# Let's see the full drag_to and mouse_drag implementations
sed -n '213,280p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 2667
🏁 Script executed:
# Let's also see the full _with_client implementation to confirm connection behavior
sed -n '102,150p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1738
🏁 Script executed:
# Verify the connection cleanup behavior - check if connections are explicitly closed
sed -n '102,160p' libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 2009
🏁 Script executed:
# Check if there's any state persistence between _with_client calls
rg "class _VNCConnection" -A 30 libs/python/computer-server/computer_server/handlers/vnc.pyRepository: trycua/cua
Length of output: 1262
Low-level mouse down/up calls do not preserve button state across separate VNC connections.
Each call to mouse_down(), mouse_up(), and move_cursor() creates a fresh VNC connection via _with_client(). This breaks button state preservation for sequences like mouse_down → move_cursor → mouse_up because the button press from the first connection is lost when mouse_up runs on a different connection. The drag_to(), mouse_drag(), and drag_path() methods work around this by executing all operations in a single connection, but the public low-level API remains broken for multi-call flows.
- vnc.py: close client.transport in finally block of _with_client() to prevent TCP connection leaks accumulating over multiple operations - VM.swift: remove old vnc.env before starting VM so the guest can't read stale port/password from a previous run
📦 Publishable packages changed
|
📦 Publishable packages changed
|
1 participant
Summary
vnc.py): Replacedvncdotool.api.connect()(which deadlocks inside uvicorn/asyncio on Python 3.13) with a global Twisted reactor pattern. Each VNC operation creates a fresh connection viareactor.callFromThread()+threading.Eventsynchronization. All closures use@defer.inlineCallbacks/yield. Also fixesmouseDrag(doPollnot available onSelectReactor) by using manualmouseMoveincrements, and fixesclient.screenaccess.VM.swift,Run.swift,LumeController.swift,VNCService.swift): Lume now creates a temp "lume-config" VirtioFS shared directory, writesvnc.env(dynamic port + password) after VNC starts. The guest mounts the share and reads the config on boot — no more hardcoded VNC port/password. Includes a VNC URL parsing fix (URLComponentswithvnc://→http://replacement) since Swift'sURLcan't parse thevnc://scheme.cli.py):CUA_VNC_PORTis now only overridden from CLI args when explicitly provided, preventing the default (5900) from clobbering the env var set by the guest'sstart_server.sh.setup-cua.sh): Renamed fromsetup-cua-computer.sh. Added "already mounted" check (mount | grep -q "lume-config") somount_virtiofsdoesn't fail on server restarts. Movedvnc.envwait loop outside the mount if/else so it runs regardless of mount state.Test plan
lume runpicks up dynamic VNC port from lume-configsetup-cua.shprovisions a new VM correctlySummary by CodeRabbit
Release Notes
--vnc-passwordcommand-line option for configuring VNC authentication