fix: collect adb errors when launching MjpegServer or AvcServer#168
fix: collect adb errors when launching MjpegServer or AvcServer#168
Conversation
WalkthroughAdds server startup monitoring and output streaming capabilities to Android screen capture functionality. Introduces helper functions for stderr/stdout parsing, error detection, and asynchronous startup monitoring. Reworks StartScreenCapture to establish pipes, spawn monitoring goroutines, wait for readiness, and stream output. 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
🧪 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 |
![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
🤖 Fix all issues with AI agents
In `@devices/android.go`:
- Around line 915-933: In monitorServerStartup, stderr is read in chunks so
containsServerError can miss tokens split across reads; change the loop to
accumulate reads into a growing buffer (e.g., bytes.Buffer or a
bufio.Reader/Scanner that emits full lines) and run containsServerError against
the accumulated data (or each complete line) so multi-chunk patterns like
"ClassNotFoundException" are detected; keep using parseServerError and errorChan
for reporting, and add a reasonable cap or truncation to the buffer (to avoid
unbounded growth) using the existing stderrBufferSize as a guide.
- Around line 946-974: The streamServerOutput function (and similarly
waitForServerStartup) currently kills the process but never calls cmd.Wait(),
leaking OS resources; update streamServerOutput to call cmd.Wait() after any
call to cmd.Process.Kill() and also call cmd.Wait() on the normal exit path
(after the read loop) to reap the child, and ensure waitForServerStartup
likewise calls cmd.Wait() after killing the process or when the process exits;
ignore or log the returned error from cmd.Wait() as appropriate but always
invoke it to prevent zombies.
🧹 Nitpick comments (2)
devices/android.go (2)
885-900: The"Error:"pattern is broad and may cause false positives.The other patterns are specific (
ClassNotFoundException,SIGABRT, etc.), but"Error:"could match benign log output. Consider using a more specific pattern or removing it in favor of the already-specific patterns handled inparseServerError.
946-964: First-read error check on stdout duplicates stderr monitoring logic.
streamServerOutputchecks the first stdout read for server error patterns. This means server errors must appear on stdout (not just stderr) for this check to trigger. If the server writes errors only to stderr, this check is dead code. If errors appear on both stdout and stderr, there's a race between this check andmonitorServerStartup. Consider clarifying which stream carries error messages and relying on a single detection path.
| func (d *AndroidDevice) monitorServerStartup(stderr io.Reader, serverClass string, errorChan chan<- error) { | ||
| buffer := make([]byte, stderrBufferSize) | ||
|
|
||
| for { | ||
| n, err := stderr.Read(buffer) | ||
| if n > 0 { | ||
| msg := string(buffer[:n]) | ||
| utils.Verbose("stderr: %s", msg) | ||
|
|
||
| if containsServerError(msg) { | ||
| errorChan <- d.parseServerError(msg, serverClass) | ||
| return | ||
| } | ||
| } | ||
| if err != nil { | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Stderr error detection can miss errors split across read boundaries.
stderr.Read(buffer) may return partial data. If an error string like "ClassNotFoundException" is split across two reads, containsServerError won't match it on either chunk. Consider accumulating stderr into a bytes.Buffer (or using bufio.Scanner with line-based reading) so pattern matching operates on the full output.
Proposed fix using accumulated buffer
func (d *AndroidDevice) monitorServerStartup(stderr io.Reader, serverClass string, errorChan chan<- error) {
- buffer := make([]byte, stderrBufferSize)
+ var accumulated strings.Builder
+ readBuf := make([]byte, stderrBufferSize)
for {
- n, err := stderr.Read(buffer)
+ n, err := stderr.Read(readBuf)
if n > 0 {
- msg := string(buffer[:n])
+ accumulated.Write(readBuf[:n])
+ msg := accumulated.String()
utils.Verbose("stderr: %s", msg)
if containsServerError(msg) {
errorChan <- d.parseServerError(msg, serverClass)
return
}
}
if err != nil {
return
}
}
}📝 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.
| func (d *AndroidDevice) monitorServerStartup(stderr io.Reader, serverClass string, errorChan chan<- error) { | |
| buffer := make([]byte, stderrBufferSize) | |
| for { | |
| n, err := stderr.Read(buffer) | |
| if n > 0 { | |
| msg := string(buffer[:n]) | |
| utils.Verbose("stderr: %s", msg) | |
| if containsServerError(msg) { | |
| errorChan <- d.parseServerError(msg, serverClass) | |
| return | |
| } | |
| } | |
| if err != nil { | |
| return | |
| } | |
| } | |
| } | |
| func (d *AndroidDevice) monitorServerStartup(stderr io.Reader, serverClass string, errorChan chan<- error) { | |
| var accumulated strings.Builder | |
| readBuf := make([]byte, stderrBufferSize) | |
| for { | |
| n, err := stderr.Read(readBuf) | |
| if n > 0 { | |
| accumulated.Write(readBuf[:n]) | |
| msg := accumulated.String() | |
| utils.Verbose("stderr: %s", msg) | |
| if containsServerError(msg) { | |
| errorChan <- d.parseServerError(msg, serverClass) | |
| return | |
| } | |
| } | |
| if err != nil { | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@devices/android.go` around lines 915 - 933, In monitorServerStartup, stderr
is read in chunks so containsServerError can miss tokens split across reads;
change the loop to accumulate reads into a growing buffer (e.g., bytes.Buffer or
a bufio.Reader/Scanner that emits full lines) and run containsServerError
against the accumulated data (or each complete line) so multi-chunk patterns
like "ClassNotFoundException" are detected; keep using parseServerError and
errorChan for reporting, and add a reasonable cap or truncation to the buffer
(to avoid unbounded growth) using the existing stderrBufferSize as a guide.
| func (d *AndroidDevice) streamServerOutput(stdout io.Reader, cmd *exec.Cmd, config ScreenCaptureConfig) error { | ||
| buffer := make([]byte, stdoutBufferSize) | ||
| firstRead := true | ||
|
|
||
| for { | ||
| n, err := stdout.Read(buffer) | ||
| if err != nil { | ||
| break | ||
| } | ||
|
|
||
| if n > 0 { | ||
| if firstRead { | ||
| firstRead = false | ||
| output := string(buffer[:n]) | ||
| if containsServerError(output) { | ||
| _ = cmd.Process.Kill() | ||
| return fmt.Errorf("server error: %s", strings.TrimSpace(output)) | ||
| } | ||
| } | ||
|
|
||
| if !config.OnData(buffer[:n]) { | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| _ = cmd.Process.Kill() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
cmd.Wait() is never called — process resources will leak.
After cmd.Process.Kill() (or when the process exits naturally), cmd.Wait() must be called to release associated OS resources (file descriptors, process table entry). Without it, the process becomes a zombie. This applies to both the normal exit path and the error path in waitForServerStartup.
Proposed fix
func (d *AndroidDevice) streamServerOutput(stdout io.Reader, cmd *exec.Cmd, config ScreenCaptureConfig) error {
buffer := make([]byte, stdoutBufferSize)
firstRead := true
for {
n, err := stdout.Read(buffer)
if err != nil {
break
}
if n > 0 {
if firstRead {
firstRead = false
output := string(buffer[:n])
if containsServerError(output) {
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return fmt.Errorf("server error: %s", strings.TrimSpace(output))
}
}
if !config.OnData(buffer[:n]) {
break
}
}
}
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return nil
}Also add _ = cmd.Wait() in waitForServerStartup after the Kill:
func (d *AndroidDevice) waitForServerStartup(errorChan <-chan error, cmd *exec.Cmd) error {
time.Sleep(serverStartupTimeout)
select {
case startupErr := <-errorChan:
_ = cmd.Process.Kill()
+ _ = cmd.Wait()
return startupErr
default:
return nil
}
}🤖 Prompt for AI Agents
In `@devices/android.go` around lines 946 - 974, The streamServerOutput function
(and similarly waitForServerStartup) currently kills the process but never calls
cmd.Wait(), leaking OS resources; update streamServerOutput to call cmd.Wait()
after any call to cmd.Process.Kill() and also call cmd.Wait() on the normal exit
path (after the read loop) to reap the child, and ensure waitForServerStartup
likewise calls cmd.Wait() after killing the process or when the process exits;
ignore or log the returned error from cmd.Wait() as appropriate but always
invoke it to prevent zombies.
1 participant
Summary by CodeRabbit
Release Notes