fix: wda context was getting cancelled #158
Conversation
WalkthroughAdds per-device synchronization and resource tracking for iOS devices, per-device cleanup orchestration, a ShutdownHook registry with global wiring for graceful shutdown, a tunnel running-state accessor, and a WDA API method to retrieve the active foreground app. Changes
Possibly related PRs
🚥 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
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
devices/ios.go (2)
434-451: Data race: concurrent read/write towdaCancelwithout synchronization.The goroutine writes
d.wdaCancel = nil(line 450) while the main code path reads it (line 425). Without a mutex or atomic operation, this is a data race that can cause undefined behavior.This should be protected by the same mutex suggested for the check-and-set logic.
Proposed fix with mutex
go func() { _, err := testmanagerd.RunTestWithConfig(ctx, testmanagerd.TestConfig{ BundleId: bundleID, TestRunnerBundleId: testRunnerBundleID, XctestConfigName: xctestConfig, Env: map[string]any{}, Args: []string{}, Device: device, Listener: testmanagerd.NewTestListener(io.Discard, io.Discard, "/tmp"), }) if err != nil { utils.Verbose("WebDriverAgent process ended with error: %v", err) } else { utils.Verbose("WebDriverAgent process ended") } + d.wdaMu.Lock() d.wdaCancel = nil + d.wdaMu.Unlock() }()
384-400: Critical:mjpegClientnot set when port forwarder is already running.
portForwarderMjpegis a global variable shared across all device instances, whilemjpegClientis a per-device field. When multiple devices are initialized, the second device will skip the entire block (sinceportForwarderMjpeg.IsRunning()is true from the first device), leavingd.mjpegClientuninitialized. This causes a nil pointer dereference whenStartScreenCapturecallsd.mjpegClient.StartScreenCapture()at line 786.The
mjpegClientmust be initialized for each device instance even when port forwarding is already established.Proposed fix
// set up mjpeg port forwarding if not already running if portForwarderMjpeg == nil || !portForwarderMjpeg.IsRunning() { portMjpeg, err := findAvailablePortInRange(portRangeStart, portRangeEnd) if err != nil { return fmt.Errorf("failed to find available port for mjpeg: %w", err) } portForwarderMjpeg = ios.NewPortForwarder(d.ID()) err = portForwarderMjpeg.Forward(portMjpeg, 9100) if err != nil { return fmt.Errorf("failed to forward port for mjpeg: %w", err) } + } - mjpegUrl := fmt.Sprintf("http://localhost:%d/", portMjpeg) - d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl) - utils.Verbose("Mjpeg client set up on %s", mjpegUrl) + if d.mjpegClient == nil { + srcPort, _ := portForwarderMjpeg.GetPorts() + mjpegUrl := fmt.Sprintf("http://localhost:%d/", srcPort) + d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl) + utils.Verbose("Mjpeg client set up on %s", mjpegUrl) }
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 424-431: The LaunchTestRunner method has a race on
IOSDevice.wdaCancel: the non-atomic check (d.wdaCancel != nil) and subsequent
assignment (d.wdaCancel = cancel) can allow concurrent calls to both pass and
launch duplicate WDA instances; protect this check-and-set by adding a mutex
(e.g., add wdaMu sync.Mutex to IOSDevice) and wrap the critical section in
wdaMu.Lock()/Unlock() around the nil check and assignment, or alternatively use
a sync/atomic boolean flag to perform an atomic compare-and-set before creating
the context; update LaunchTestRunner to use the chosen synchronization primitive
to ensure only one launcher proceeds and set wdaCancel only while holding the
lock/after successful CAS.
- Around line 40-42: The package-level globals portForwarder and
portForwarderMjpeg are overwritten when StartAgent runs for multiple devices;
move these to per-device state by adding fields to the IOSDevice struct (e.g.,
PortForwarder and PortForwarderMjpeg) and update StartAgent, StopAgent, and any
cleanup logic to use d.PortForwarder / d.PortForwarderMjpeg instead of the
globals; mirror the existing wdaCancel handling so each IOSDevice manages its
own forwarders (or alternatively implement a map keyed by device UDID) to avoid
cross-device interference.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 338-342: The code unconditionally replaces d.portForwarder by
calling ios.NewPortForwarder(d.ID()) which can leak the previous forwarder;
update the StartAgent (or the function containing this snippet) to first check
d.portForwarder and its running state (e.g., if d.portForwarder == nil ||
!d.portForwarder.IsRunning()) before creating a new one, and if an existing
forwarder is running, either reuse it or call its Stop/Close method to cleanly
shut it down before assigning a new ios.NewPortForwarder(d.ID()); ensure the
Forward call on the forwarder remains and propagate errors as before.
♻️ Duplicate comments (1)
devices/ios.go (1)
403-410: Race condition onwdaCancelcheck-and-set remains unaddressed.The check at line 404 (
d.wdaCancel != nil) and assignment at line 410 (d.wdaCancel = cancel) are not atomic. ConcurrentLaunchTestRunnercalls could both pass the nil check before either sets the value, launching duplicate WDA instances. Additionally, line 429 (d.wdaCancel = nil) in the goroutine creates another data race with readers.Consider protecting with a mutex as previously suggested.
🧹 Nitpick comments (1)
devices/ios.go (1)
426-429: Callcancel()before clearingwdaCancelto release context resources.When the WDA process ends, the cancel function should be called to release the context's resources before setting
d.wdaCancelto nil.Suggested fix
if err != nil { utils.Verbose("WebDriverAgent process ended with error: %v", err) } else { utils.Verbose("WebDriverAgent process ended") } + cancel() // release context resources d.wdaCancel = nil
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 100-101: The call to RegisterDevice(&device) is invalid and
unsafe: RegisterDevice is undefined and you are registering a pointer to a local
value while the function returns IOSDevice by value, causing registry to track a
stale copy. Fix by (1) implementing or importing the RegisterDevice(symbol)
function that accepts *IOSDevice, (2) change the factory/constructor to allocate
and return a pointer (*IOSDevice) instead of a value so callers operate on the
same instance, and (3) register that heap-allocated pointer (e.g., devicePtr)
with RegisterDevice(devicePtr); update all callers and signatures that currently
expect IOSDevice to use *IOSDevice to maintain correct, mutable state in the
registry.
In `@main.go`:
- Around line 24-34: The signal handler calls devices.CleanupAllDevices which
doesn't exist; either implement that function or call the correct existing
cleanup API. Update the signal branch in the select (the case <-sigChan block)
to invoke the proper cleanup symbol (e.g., devices.CleanupDevices or
devices.CleanupAll) or add a new exported function named CleanupAllDevices in
the devices package that performs the same teardown, and ensure its signature
matches the usage here (no args, returns nothing) so the code compiles.
In `@server/server.go`:
- Around line 149-151: The call to devices.CleanupAllDevices is undefined;
either implement that exported function in the devices package or change this
call to the correct existing API. Add an exported function named
CleanupAllDevices in the devices package (e.g., func CleanupAllDevices() error)
that performs the current global cleanup logic (iterate stored device instances
and call their Close/Shutdown/Remove methods, handle errors) and return or log
errors, or update this reference to the existing cleanup function (for example
CleanupDevices, CloseAllDevices, or Devices.Cleanup) so the symbol matches the
devices package; ensure the signature and error handling match other callers.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@devices/registry.go`:
- Around line 18-23: RegisterDevice currently dereferences device.Udid without
checks which can panic on nil and allow empty UDID to overwrite entries; add
validation at the start of RegisterDevice to return early if device is nil or
device.Udid is empty (zero-length string), and optionally log or return an
error; then only lock globalRegistry.mu and insert into
globalRegistry.devices[device.Udid] after validation. Reference: RegisterDevice,
IOSDevice, globalRegistry.mu, globalRegistry.devices.
- Around line 33-49: In CleanupAllDevices, avoid calling device.Cleanup() while
holding globalRegistry.mu; instead, under the lock take a snapshot of
globalRegistry.devices (e.g., copy the map or collect devices into a slice),
clear or reassign globalRegistry.devices to an empty map and release the lock,
then iterate the snapshot and call device.Cleanup() (logging errors with
utils.Verbose) outside the lock so Cleanup and potential callbacks like
UnregisterDevice cannot deadlock the registry.
devices/registry.go
Outdated
| // RegisterDevice adds a device to the global registry for cleanup tracking | ||
| func RegisterDevice(device *IOSDevice) { | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
| globalRegistry.devices[device.Udid] = device | ||
| } |
There was a problem hiding this comment.
Guard against nil/empty devices before registering.
RegisterDevice dereferences device.Udid; a nil device will panic and an empty UDID can overwrite another entry. Consider a small validation guard.
🛠️ Suggested fix
func RegisterDevice(device *IOSDevice) {
+ if device == nil || device.Udid == "" {
+ utils.Verbose("Skipping register: nil device or empty UDID")
+ return
+ }
globalRegistry.mu.Lock()
defer globalRegistry.mu.Unlock()
globalRegistry.devices[device.Udid] = device
}📝 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.
| // RegisterDevice adds a device to the global registry for cleanup tracking | |
| func RegisterDevice(device *IOSDevice) { | |
| globalRegistry.mu.Lock() | |
| defer globalRegistry.mu.Unlock() | |
| globalRegistry.devices[device.Udid] = device | |
| } | |
| // RegisterDevice adds a device to the global registry for cleanup tracking | |
| func RegisterDevice(device *IOSDevice) { | |
| if device == nil || device.Udid == "" { | |
| utils.Verbose("Skipping register: nil device or empty UDID") | |
| return | |
| } | |
| globalRegistry.mu.Lock() | |
| defer globalRegistry.mu.Unlock() | |
| globalRegistry.devices[device.Udid] = device | |
| } |
🤖 Prompt for AI Agents
In `@devices/registry.go` around lines 18 - 23, RegisterDevice currently
dereferences device.Udid without checks which can panic on nil and allow empty
UDID to overwrite entries; add validation at the start of RegisterDevice to
return early if device is nil or device.Udid is empty (zero-length string), and
optionally log or return an error; then only lock globalRegistry.mu and insert
into globalRegistry.devices[device.Udid] after validation. Reference:
RegisterDevice, IOSDevice, globalRegistry.mu, globalRegistry.devices.
devices/registry.go
Outdated
| // CleanupAllDevices gracefully cleans up all registered devices | ||
| func CleanupAllDevices() { | ||
| globalRegistry.mu.Lock() | ||
| defer globalRegistry.mu.Unlock() | ||
|
|
||
| if len(globalRegistry.devices) == 0 { | ||
| return | ||
| } | ||
|
|
||
| for udid, device := range globalRegistry.devices { | ||
| if err := device.Cleanup(); err != nil { | ||
| utils.Verbose("Error cleaning up device %s: %v", udid, err) | ||
| } | ||
| } | ||
|
|
||
| // clear the registry | ||
| globalRegistry.devices = make(map[string]*IOSDevice) |
There was a problem hiding this comment.
Avoid holding the registry lock while running Cleanup.
device.Cleanup() is external code and may block or call back into the registry (e.g., UnregisterDevice), risking deadlock and stalling other operations. Snapshot + clear under lock, then clean up outside the lock.
🛠️ Suggested fix
func CleanupAllDevices() {
globalRegistry.mu.Lock()
- defer globalRegistry.mu.Unlock()
-
if len(globalRegistry.devices) == 0 {
- return
+ globalRegistry.mu.Unlock()
+ return
}
- for udid, device := range globalRegistry.devices {
- if err := device.Cleanup(); err != nil {
- utils.Verbose("Error cleaning up device %s: %v", udid, err)
- }
- }
+ devices := make(map[string]*IOSDevice, len(globalRegistry.devices))
+ for udid, device := range globalRegistry.devices {
+ devices[udid] = device
+ }
// clear the registry
globalRegistry.devices = make(map[string]*IOSDevice)
+ globalRegistry.mu.Unlock()
+
+ for udid, device := range devices {
+ if err := device.Cleanup(); err != nil {
+ utils.Verbose("Error cleaning up device %s: %v", udid, err)
+ }
+ }
}🤖 Prompt for AI Agents
In `@devices/registry.go` around lines 33 - 49, In CleanupAllDevices, avoid
calling device.Cleanup() while holding globalRegistry.mu; instead, under the
lock take a snapshot of globalRegistry.devices (e.g., copy the map or collect
devices into a slice), clear or reassign globalRegistry.devices to an empty map
and release the lock, then iterate the snapshot and call device.Cleanup()
(logging errors with utils.Verbose) outside the lock so Cleanup and potential
callbacks like UnregisterDevice cannot deadlock the registry.
There was a problem hiding this comment.
@gmegidish, understood. I'll defer to your judgment on this.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
1196-1216: DeviceKit port forwarders are not tracked for cleanup.The
httpForwarderandstreamForwardercreated here are local variables, not stored onIOSDevice. They won't be cleaned up byCleanup(), which only handlesd.portForwarderandd.portForwarderMjpeg.Consider either:
- Adding
portForwarderDeviceKitHTTPandportForwarderDeviceKitStreamfields toIOSDeviceand cleaning them incleanupPortForwarders()- Returning the forwarders in
DeviceKitInfoso callers can manage their lifecycle
🧹 Nitpick comments (4)
cli/device.go (1)
42-58: Avoid double StartAgent/FindDevice indevice infoCLI path.
deviceInfoCmdstarts the agent and then callscommands.InfoCommand, which now does the same. IfStartAgentisn’t strictly idempotent, this can reinitialize WDA and undermine the per-device lifecycle work.♻️ Suggested simplification
- // Find the target device - targetDevice, err := commands.FindDeviceOrAutoSelect(deviceId) - if err != nil { - response := commands.NewErrorResponse(fmt.Errorf("error finding device: %v", err)) - printJson(response) - return fmt.Errorf("%s", response.Error) - } - - // Start agent - err = targetDevice.StartAgent(devices.StartAgentConfig{ - Registry: commands.GetRegistry(), - }) - if err != nil { - response := commands.NewErrorResponse(fmt.Errorf("error starting agent: %v", err)) - printJson(response) - return fmt.Errorf("%s", response.Error) - } - response := commands.InfoCommand(deviceId)If you go this route, you can also drop the
devicesimport since it’s only used by the removed block.commands/commands.go (1)
36-54: Consider thread-safety for concurrent access todeviceRegistry.The global
deviceRegistryvariable is accessed without synchronization. While the comment statesSetRegistryshould be called once at startup, a data race could occur ifGetRegistryis called concurrently withSetRegistry. Since the startup flow guarantees ordering (server callsSetRegistrybefore starting HTTP handlers), this is acceptable in practice.If future changes might alter this ordering, consider using
sync.Onceoratomic.Pointerfor safer initialization.♻️ Optional: Thread-safe initialization using sync.Once
+var ( + deviceRegistry *devices.DeviceRegistry + registryOnce sync.Once +) // SetRegistry sets the global device registry for cleanup tracking. func SetRegistry(registry *devices.DeviceRegistry) { - deviceRegistry = registry + registryOnce.Do(func() { + deviceRegistry = registry + }) }devices/ios.go (2)
442-475: Minor TOCTOU window in port forwarder creation.The check at lines 443-445 and creation at lines 453-462 aren't atomic. If
StartAgentis called concurrently for the same device, both calls could pass theneedsPortForwardercheck and create forwarders, with the second assignment orphaning the first.This is low risk since
StartAgentis typically called once per device, but for robustness consider holding the lock through creation:Suggested approach
- d.mu.Lock() - needsPortForwarder := d.portForwarder == nil || !d.portForwarder.IsRunning() - d.mu.Unlock() - - if needsPortForwarder { + d.mu.Lock() + if d.portForwarder == nil || !d.portForwarder.IsRunning() { + d.mu.Unlock() // unlock during port search and Forward() call port, err := findAvailablePortInRange(portRangeStart, portRangeEnd) ... forwarder := ios.NewPortForwarder(d.ID()) err = forwarder.Forward(port, 8100) ... d.mu.Lock() + // Double-check after re-acquiring lock + if d.portForwarder == nil || !d.portForwarder.IsRunning() { d.portForwarder = forwarder d.wdaClient = wda.NewWdaClient(...) + } else { + _ = forwarder.Stop() // another goroutine won the race + } d.mu.Unlock() + } else { + d.mu.Unlock() + ... + }
892-917: Same TOCTOU pattern as WDA forwarder.The MJPEG forwarder creation has the same check-then-create pattern noted earlier for the WDA forwarder. Consider applying the same double-check pattern if you address the WDA forwarder race.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
1156-1266: DeviceKit forwarders not tracked for cleanup.The
httpForwarderandstreamForwardercreated inStartDeviceKitare local variables. On success, they're not stored on the device struct or registered with the shutdown hook, so they won't be cleaned up whenShutdown()is called.🔧 Suggested approach
Either store these forwarders on
IOSDevice(e.g.,deviceKitHTTPForwarder,deviceKitStreamForwarder) and clean them up incleanupPortForwarders(), or register individual cleanup functions for them:if hook != nil { hook.Register(fmt.Sprintf("ios-devicekit-http-%s", d.Udid), httpForwarder.Stop) hook.Register(fmt.Sprintf("ios-devicekit-stream-%s", d.Udid), streamForwarder.Stop) }
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 893-918: The mjpeg forwarder creation has a TOCTOU race on
d.portForwarderMjpeg; fix it by re-checking and serializing assignment under the
mutex: keep the initial check (needsMjpegForwarder), perform the expensive work
(findAvailablePortInRange, ios.NewPortForwarder and forwarder.Forward) outside
the lock, then immediately take d.mu.Lock() again and re-check
d.portForwarderMjpeg (or d.portForwarderMjpeg.IsRunning())—if another forwarder
was installed in the meantime, stop/close the newly-created forwarder and use
the existing one; otherwise assign d.portForwarderMjpeg = forwarder and set
d.mjpegClient as before. Reference symbols: d.portForwarderMjpeg, d.mjpegClient,
ios.NewPortForwarder, forwarder.Forward, d.mu.
- Around line 443-476: The TOCTOU race occurs between checking d.portForwarder
and creating/storing a new ios.PortForwarder; fix by serializing access around
creation: either hold d.mu for the entire create/store sequence (lock before
checking needsPortForwarder and only unlock after assigning d.portForwarder and
d.wdaClient), or implement double-checked locking—create the forwarder, then
immediately lock d.mu and re-check if d.portForwarder is nil/is running; if
another goroutine already set one, call forwarder.Close()/Stop() on the newly
created forwarder and use the existing d.portForwarder (or set d.wdaClient if
needed); ensure you use d.mu, d.portForwarder, forwarder.Forward(),
forwarder.GetPorts(), and d.wdaClient to locate and update the relevant code
paths.
🧹 Nitpick comments (3)
main.go (1)
14-41: Graceful shutdown implementation looks correct.The signal handling correctly intercepts SIGINT/SIGTERM and triggers cleanup before exit. The CLI runs in a goroutine to allow concurrent signal monitoring.
One optional improvement: call
signal.Stop(sigChan)before exiting in the normal completion path to explicitly unregister the signal handler.♻️ Optional: explicit signal handler cleanup
case err := <-done: + signal.Stop(sigChan) // normal exit: let WDA and other resources persist if err != nil { fmt.Fprintln(os.Stderr, err) os.Exit(1) } }devices/shutdown_test.go (1)
36-53: Test coverage looks good, but could verify all hooks execute despite errors.The test verifies error propagation and hook clearing, but doesn't confirm that
successandsuccess2hooks actually executed despite thefailurehook's error.♻️ Optional: verify all hooks execute
func TestShutdownHook_ErrorHandling(t *testing.T) { hook := NewShutdownHook() - hook.Register("success", func() error { return nil }) + successCalled := false + success2Called := false + hook.Register("success", func() error { successCalled = true; return nil }) hook.Register("failure", func() error { return errors.New("cleanup failed") }) - hook.Register("success2", func() error { return nil }) + hook.Register("success2", func() error { success2Called = true; return nil }) err := hook.Shutdown() if err == nil { t.Error("Expected error from failed hook") } + if !successCalled || !success2Called { + t.Error("All hooks should execute even when one fails") + } + // all hooks should still be cleared if hook.Count() != 0 { t.Errorf("Expected hooks to be cleared even after error, got %d", hook.Count()) } }server/server.go (1)
153-154: Consider logging errors fromhook.Shutdown().The error returned by
hook.Shutdown()is currently ignored. While this shouldn't block server shutdown, logging the error would help with debugging cleanup failures.🔧 Suggested improvement
// cleanup all resources first - hook.Shutdown() + if err := hook.Shutdown(); err != nil { + utils.Info("Cleanup completed with errors: %v", err) + }
| // set up mjpeg port forwarding if not already running | ||
| d.mu.Lock() | ||
| needsMjpegForwarder := d.portForwarderMjpeg == nil || !d.portForwarderMjpeg.IsRunning() | ||
| d.mu.Unlock() | ||
|
|
||
| if needsMjpegForwarder { | ||
| portMjpeg, err := findAvailablePortInRange(portRangeStart, portRangeEnd) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to find available port for mjpeg: %w", err) | ||
| } | ||
|
|
||
| forwarder := ios.NewPortForwarder(d.ID()) | ||
| err = forwarder.Forward(portMjpeg, 9100) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to forward port for mjpeg: %w", err) | ||
| } | ||
|
|
||
| mjpegUrl := fmt.Sprintf("http://localhost:%d/", portMjpeg) | ||
|
|
||
| d.mu.Lock() | ||
| d.portForwarderMjpeg = forwarder | ||
| d.mjpegClient = mjpeg.NewWdaMjpegClient(mjpegUrl) | ||
| d.mu.Unlock() | ||
|
|
||
| utils.Verbose("Mjpeg client set up on %s", mjpegUrl) | ||
| } |
There was a problem hiding this comment.
Same TOCTOU race as WDA port forwarder.
This mjpeg forwarder creation has the same race condition pattern as the WDA forwarder in StartAgent. Apply the same double-check-after-lock fix or serialize the entire creation under the mutex.
🤖 Prompt for AI Agents
In `@devices/ios.go` around lines 893 - 918, The mjpeg forwarder creation has a
TOCTOU race on d.portForwarderMjpeg; fix it by re-checking and serializing
assignment under the mutex: keep the initial check (needsMjpegForwarder),
perform the expensive work (findAvailablePortInRange, ios.NewPortForwarder and
forwarder.Forward) outside the lock, then immediately take d.mu.Lock() again and
re-check d.portForwarderMjpeg (or d.portForwarderMjpeg.IsRunning())—if another
forwarder was installed in the meantime, stop/close the newly-created forwarder
and use the existing one; otherwise assign d.portForwarderMjpeg = forwarder and
set d.mjpegClient as before. Reference symbols: d.portForwarderMjpeg,
d.mjpegClient, ios.NewPortForwarder, forwarder.Forward, d.mu.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 472-475: The assignment to d.wdaClient is done without holding
d.mu which can cause a race when StartAgent runs concurrently; wrap the
check-and-set of d.wdaClient with d.mu (e.g., lock d.mu before checking
d.wdaClient and only call wda.NewWdaClient and assign to d.wdaClient while
holding the mutex, or use a double-checked pattern: read under lock, if nil
create client then re-lock to assign) so access to d.wdaClient is always
protected by d.mu.
- Around line 1165-1170: StartDeviceKit currently creates httpForwarder and
streamForwarder as local variables so Cleanup cannot stop them; persist these
forwarders on the IOSDevice instance (add fields like httpForwarder and
streamForwarder to IOSDevice) and assign the created forwarders there in
StartDeviceKit, then update Cleanup to stop/close those stored forwarders;
alternatively, after creating the forwarders in StartDeviceKit register a
dedicated ShutdownHook entry that stops both forwarders (use the same
hook.Register pattern), ensuring all error and success paths cleanly stop the
forwarders.
In `@main.go`:
- Around line 31-38: The calls to hook.Shutdown() in the signal and normal-exit
branches currently ignore its returned error; update both places (the case
<-sigChan and case err := <-done branches where hook.Shutdown() is invoked) to
capture the returned error from hook.Shutdown(), check it, and handle or log it
(e.g., via log.Printf or similar) before exiting so cleanup failures are not
silently ignored and errcheck is satisfied.
| case <-sigChan: | ||
| // cleanup resources on signal | ||
| hook.Shutdown() | ||
| os.Exit(0) | ||
| case err := <-done: | ||
| // cleanup resources on normal exit | ||
| hook.Shutdown() | ||
| if err != nil { |
There was a problem hiding this comment.
Handle ShutdownHook errors to avoid silent cleanup failures.
hook.Shutdown() returns an error; ignoring it triggers errcheck and hides cleanup failures (Line 33, Line 37).
🔧 Suggested fix
- case <-sigChan:
- // cleanup resources on signal
- hook.Shutdown()
- os.Exit(0)
+ case <-sigChan:
+ // cleanup resources on signal
+ if err := hook.Shutdown(); err != nil {
+ fmt.Fprintln(os.Stderr, err)
+ }
+ os.Exit(0)
case err := <-done:
// cleanup resources on normal exit
- hook.Shutdown()
+ if shutdownErr := hook.Shutdown(); shutdownErr != nil {
+ fmt.Fprintln(os.Stderr, shutdownErr)
+ }
if err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}📝 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.
| case <-sigChan: | |
| // cleanup resources on signal | |
| hook.Shutdown() | |
| os.Exit(0) | |
| case err := <-done: | |
| // cleanup resources on normal exit | |
| hook.Shutdown() | |
| if err != nil { | |
| case <-sigChan: | |
| // cleanup resources on signal | |
| if err := hook.Shutdown(); err != nil { | |
| fmt.Fprintln(os.Stderr, err) | |
| } | |
| os.Exit(0) | |
| case err := <-done: | |
| // cleanup resources on normal exit | |
| if shutdownErr := hook.Shutdown(); shutdownErr != nil { | |
| fmt.Fprintln(os.Stderr, shutdownErr) | |
| } | |
| if err != nil { |
🧰 Tools
🪛 GitHub Check: lint
[failure] 37-37:
Error return value of hook.Shutdown is not checked (errcheck)
[failure] 33-33:
Error return value of hook.Shutdown is not checked (errcheck)
🤖 Prompt for AI Agents
In `@main.go` around lines 31 - 38, The calls to hook.Shutdown() in the signal and
normal-exit branches currently ignore its returned error; update both places
(the case <-sigChan and case err := <-done branches where hook.Shutdown() is
invoked) to capture the returned error from hook.Shutdown(), check it, and
handle or log it (e.g., via log.Printf or similar) before exiting so cleanup
failures are not silently ignored and errcheck is satisfied.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
148-155: Run cleanup on the server-error path too.
IfListenAndServereturns an error after devices are started, the hook never runs. Consider invoking it before returning.🧹 Suggested fix
case err := <-serverErr: + hook.Shutdown() return fmt.Errorf("server error: %w", err)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 5: Fix the typo in the changelog entry: change the word "forwader" to
"forwarder" in the line that reads "* iOS: Multiple real devices would overwrite
each other's wda tcp forwader
([`#158`](https://github.com/mobile-next/mobilecli/pull/158))" so it becomes "*
iOS: Multiple real devices would overwrite each other's wda tcp forwarder
([`#158`](https://github.com/mobile-next/mobilecli/pull/158))".
|
|
||
| * iOS: Fixed a bug where tcp forwarders were created on every request for mjpeg ([#158](https://github.com/mobile-next/mobilecli/pull/158)) | ||
| * iOS: Lazily creating mjpeg tcp forwarder only when needed ([#158](https://github.com/mobile-next/mobilecli/pull/158)) | ||
| * iOS: Multiple real devices would overwrite each other's wda tcp forwader ([#158](https://github.com/mobile-next/mobilecli/pull/158)) |
There was a problem hiding this comment.
Typo: "forwader" should be "forwarder".
-* iOS: Multiple real devices would overwrite each other's wda tcp forwader ([`#158`](https://github.com/mobile-next/mobilecli/pull/158))
+* iOS: Multiple real devices would overwrite each other's wda tcp forwarder ([`#158`](https://github.com/mobile-next/mobilecli/pull/158))📝 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.
| * iOS: Multiple real devices would overwrite each other's wda tcp forwader ([#158](https://github.com/mobile-next/mobilecli/pull/158)) | |
| * iOS: Multiple real devices would overwrite each other's wda tcp forwarder ([`#158`](https://github.com/mobile-next/mobilecli/pull/158)) |
🤖 Prompt for AI Agents
In `@CHANGELOG.md` at line 5, Fix the typo in the changelog entry: change the word
"forwader" to "forwarder" in the line that reads "* iOS: Multiple real devices
would overwrite each other's wda tcp forwader
([`#158`](https://github.com/mobile-next/mobilecli/pull/158))" so it becomes "*
iOS: Multiple real devices would overwrite each other's wda tcp forwarder
([`#158`](https://github.com/mobile-next/mobilecli/pull/158))".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
devices/ios.go (1)
1223-1255: DeviceKit forwarders are always recreated; repeated calls will leak ports.
StartDeviceKitunconditionally allocates new HTTP/AVC forwarders and overwrites the fields without stopping/reusing existing ones. Multiple calls (or concurrent calls) will leak port forwarders and consume ports. Consider guarding withd.mu, reusing existing running forwarders, or stopping old ones before replacement, and only assigning to the struct afterForwardsucceeds.
🤖 Fix all issues with AI agents
In `@devices/ios.go`:
- Around line 40-54: The IOSDevice struct embeds a sync.Mutex (mu) and many
methods currently use value receivers which would copy the mutex; change every
listed method to use pointer receivers (func (d *IOSDevice) ...) instead of
value receivers so the mutex is never copied — update ID, Name, Version,
Platform, DeviceType, State, TakeScreenshot, Reboot, Boot, Shutdown, Tap,
LongPress, Swipe, Gesture, ListTunnels, PressButton, getEnhancedDevice,
LaunchApp, TerminateApp, SendKeys, OpenURL, ListApps, Info, DumpSource,
DumpSourceRaw, InstallApp, UninstallApp, GetOrientation, SetOrientation to use
*IOSDevice; while doing so ensure accesses to fields like
wdaClient/tunnelManager/portForwarder* are properly protected by d.mu (use
d.mu.Lock()/Unlock() or RLock/RUnlock where appropriate) and update any
interface implementations or callers expecting value receivers to accept pointer
receivers.
| type IOSDevice struct { | ||
| Udid string `json:"UniqueDeviceID"` | ||
| DeviceName string `json:"DeviceName"` | ||
| OSVersion string `json:"Version"` | ||
|
|
||
| tunnelManager *ios.TunnelManager | ||
| wdaClient *wda.WdaClient | ||
| mjpegClient *mjpeg.WdaMjpegClient | ||
| mu sync.Mutex // protects fields below | ||
| tunnelManager *ios.TunnelManager | ||
| wdaClient *wda.WdaClient | ||
| mjpegClient *mjpeg.WdaMjpegClient | ||
| wdaCancel context.CancelFunc | ||
| portForwarderWda *ios.PortForwarder | ||
| portForwarderMjpeg *ios.PortForwarder | ||
| portForwarderDeviceKit *ios.PortForwarder // devicekit http forwarder | ||
| portForwarderAvc *ios.PortForwarder // devicekit h264 stream forwarder | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'func \(d IOSDevice\)' devices/ios.go | head -20Repository: mobile-next/mobilecli
Length of output: 1065
🏁 Script executed:
# First, let's verify the IOSDevice struct definition at lines 40-54
sed -n '40,54p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 675
🏁 Script executed:
# Search for all IOSDevice methods (both value and pointer receivers)
rg -n 'func \(d (\*)?IOSDevice\)' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 2544
🏁 Script executed:
# Also check for IOSDevice interface implementations
rg -n 'func \([a-zA-Z]+ (\*)?IOSDevice\)' devices/ios.go | wc -lRepository: mobile-next/mobilecli
Length of output: 67
🏁 Script executed:
# Check a sample of value receiver methods to see if they access the mutex-protected fields
# Let's check a few methods like Reboot, Boot, Shutdown which seem like they might need mutex
sed -n '131,153p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 577
🏁 Script executed:
# Check the Tap method and others that might access protected resources
sed -n '162,185p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 687
🏁 Script executed:
# Check TakeScreenshot which likely needs access to wdaClient (protected field)
sed -n '127,130p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 158
🏁 Script executed:
# Check if any of the pointer receiver methods properly lock the mutex
# Let's look at StartAgent which should use the mutex
sed -n '404,430p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 1331
🏁 Script executed:
# Check Cleanup method to see mutex usage pattern
sed -n '227,255p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 704
🏁 Script executed:
# Check cleanupWDA to see if it locks the mutex
sed -n '271,285p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 365
🏁 Script executed:
# Check cleanupPortForwarders to see mutex usage
sed -n '286,331p' devices/ios.goRepository: mobile-next/mobilecli
Length of output: 1576
Convert all IOSDevice methods to pointer receivers to prevent unsafe mutex copying.
The struct embeds sync.Mutex (line 45), which Go explicitly forbids from being copied. Currently, many methods use value receivers (func (d IOSDevice)), which creates copies of the struct and its mutex on each call. This violates Go's concurrency guarantees and creates potential race conditions—for example, Tap(), Swipe(), and TakeScreenshot() access d.wdaClient without holding the lock, while pointer receiver methods like cleanupWDA() properly synchronize access. All methods must use pointer receivers (func (d *IOSDevice)).
♻️ Required conversion pattern
-func (d IOSDevice) ID() string {
+func (d *IOSDevice) ID() string {
return d.Udid
}Affected methods include: ID(), Name(), Version(), Platform(), DeviceType(), State(), TakeScreenshot(), Reboot(), Boot(), Shutdown(), Tap(), LongPress(), Swipe(), Gesture(), ListTunnels(), PressButton(), getEnhancedDevice(), LaunchApp(), TerminateApp(), SendKeys(), OpenURL(), ListApps(), Info(), DumpSource(), DumpSourceRaw(), InstallApp(), UninstallApp(), GetOrientation(), SetOrientation().
🤖 Prompt for AI Agents
In `@devices/ios.go` around lines 40 - 54, The IOSDevice struct embeds a
sync.Mutex (mu) and many methods currently use value receivers which would copy
the mutex; change every listed method to use pointer receivers (func (d
*IOSDevice) ...) instead of value receivers so the mutex is never copied —
update ID, Name, Version, Platform, DeviceType, State, TakeScreenshot, Reboot,
Boot, Shutdown, Tap, LongPress, Swipe, Gesture, ListTunnels, PressButton,
getEnhancedDevice, LaunchApp, TerminateApp, SendKeys, OpenURL, ListApps, Info,
DumpSource, DumpSourceRaw, InstallApp, UninstallApp, GetOrientation,
SetOrientation to use *IOSDevice; while doing so ensure accesses to fields like
wdaClient/tunnelManager/portForwarder* are properly protected by d.mu (use
d.mu.Lock()/Unlock() or RLock/RUnlock where appropriate) and update any
interface implementations or callers expecting value receivers to accept pointer
receivers.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.