fix: mjpeg framerate wda race condition #119
Conversation
WalkthroughThe PR modifies WDA session management by introducing strongly-typed session request structures and in-memory session caching, adds retry logic for framerate settings application, enhances endpoint handling with context timeouts and proper resource cleanup, and disables timing stat logging in device enumeration. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant AppiumSettings
participant Requests
participant WDA as WDA Service
rect rgba(200, 220, 255, 0.3)
note over Caller,WDA: SetMjpegFramerate with Retry
Caller->>AppiumSettings: SetMjpegFramerate(frameRate)
loop Retry up to 5 attempts
AppiumSettings->>AppiumSettings: Check session validity
alt Session missing/invalid
AppiumSettings->>Requests: GetOrCreateSession()
Requests->>WDA: CreateSession
WDA-->>Requests: sessionId
Requests-->>AppiumSettings: sessionId
end
AppiumSettings->>Requests: PostEndpoint(framerate setting)
Requests->>WDA: POST framerate request
alt Success
WDA-->>Requests: 200 OK
Requests-->>AppiumSettings: success
AppiumSettings-->>Caller: return nil
else Failure
WDA-->>Requests: error
Requests-->>AppiumSettings: error logged
AppiumSettings->>AppiumSettings: Attempt N failed, retry...
end
end
rect rgba(255, 200, 200, 0.3)
note over AppiumSettings: All 5 attempts failed
AppiumSettings-->>Caller: Aggregated error
end
end
rect rgba(200, 255, 220, 0.3)
note over Requests: Session Lifecycle (Session Creation/Deletion)
Caller->>Requests: CreateSession()
Requests->>Requests: Build sessionRequest (typed)
Requests->>WDA: POST with structured payload
WDA-->>Requests: {sessionId, ...}
Requests->>Requests: Cache sessionId in-memory
Requests-->>Caller: {sessionId, ...}
Caller->>Requests: DeleteSession(sessionId)
Requests->>WDA: DELETE /session/{id}
WDA-->>Requests: 200 OK
Requests->>Requests: Clear cached sessionId if match
Requests-->>Caller: success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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 (2)
devices/common.go (1)
119-122: Bug: all iOS device entries share the same pointerIn this loop:
for _, device := range iosDevices { allDevices = append(allDevices, &device) }
deviceis a loop variable reused on each iteration, so every&devicepoints to the same memory location, and all entries inallDeviceswill effectively reference the last iOS device.Use the slice element address instead:
- for _, device := range iosDevices { - allDevices = append(allDevices, &device) - } + for i := range iosDevices { + allDevices = append(allDevices, &iosDevices[i]) + }This ensures each
ControllableDevicepoints to a distinct underlying device.devices/wda/requests.go (1)
160-177: Safer handling ofsessionIdextraction recommendedBuilding the session request via
sessionRequest{Capabilities: ...}is clean, and posting it throughPostEndpoint("session", request)fits the existing helper.The only concern is:
sessionId := response["sessionId"].(string)If WDA ever returns an unexpected structure (e.g., missing key, nested under another field, or non-string type), this will panic. Consider a safer extraction pattern:
- sessionId := response["sessionId"].(string) - return sessionId, nil + rawSessionID, ok := response["sessionId"] + if !ok { + return "", fmt.Errorf("create session response missing sessionId: %v", response) + } + + sessionId, ok := rawSessionID.(string) + if !ok { + return "", fmt.Errorf("create session response has non-string sessionId: %T", rawSessionID) + } + + return sessionId, nilThis keeps failures as regular errors instead of panics when the remote end misbehaves.
🧹 Nitpick comments (2)
devices/common.go (1)
147-150: Timing log effectively disabled viaif falseGuarding the verbose timing log with
if falseis a simple way to silence it without touching surrounding logic, and it won’t affect compilation since the variables are still used. If this is meant to be permanent rather than a temporary toggle, consider either removing the block or guarding it with a config / verbosity flag instead of a hardcodedfalseto keep intent clearer.devices/wda/appium-settings.go (1)
25-42: Retry loop is good; align logging and consider backoffThe retry logic around
SetAppiumSettingsis straightforward and should help with transient WDA races. Two small polish points:
- Use the project’s logging utility (e.g.
utils.Verboseor similar) instead offmt.Printfso these messages respect the CLI’s log configuration.- Optionally add a short sleep between retries to avoid hammering WDA in quick succession and to give it time to recover.
For example:
- for i := 0; i < maxRetries; i++ { + for i := 0; i < maxRetries; i++ { err = c.SetAppiumSettings(map[string]interface{}{ "mjpegServerFramerate": framerate, }) if err == nil { return nil } - fmt.Printf("attempt %d/%d failed to set mjpeg framerate: %v\n", i+1, maxRetries, err) + utils.Verbose("attempt %d/%d failed to set mjpeg framerate: %v", i+1, maxRetries, err) + time.Sleep(500 * time.Millisecond) }(Adjust sleep and logging helper to taste.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
devices/common.go(1 hunks)devices/wda/appium-settings.go(2 hunks)devices/wda/requests.go(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint
🔇 Additional comments (6)
devices/wda/appium-settings.go (1)
5-23: Session handling inSetAppiumSettingslooks solidUsing
GetOrCreateSessionhere is a good improvement: it reuses a valid cached WDA session and falls back to creating a new one when needed, and the updated error message is accurate. The endpoint construction and payload shape ({"settings": settings}) remain correct.devices/wda/requests.go (5)
15-25: Typed session payload definitions are clear and appropriateThe
alwaysMatch,sessionCapabilities, andsessionRequeststructs mirror the W3C/WebDriver capabilities structure and keep the session creation payload explicit and type-safe. Keeping them unexported is appropriate since they’re internal to the WDA client.
43-43: Good: response body is now reliably closed inGetEndpointAdding the deferred
resp.Body.Close()ensures no leaked connections from GET calls, which is important under sustained use of the WDA client.
80-80: Good: explicit JSON content type on POSTSetting
Content-Type: application/jsonimproves interoperability with servers that strictly validate headers. This is a nice robustness fix.
187-205: Cached-session reuse and invalidation flow still looks good
GetOrCreateSessioncorrectly:
- Returns the cached
c.sessionIdwhenisSessionStillValidsucceeds.- Logs and clears
c.sessionIdwhen the cached session is no longer valid, then creates and caches a new session viaCreateSession.This aligns with the new usage from
SetAppiumSettingsand should reduce unnecessary session churn while still recovering cleanly from invalid sessions.
208-219: Nice: cachedsessionIdcleared when deleting the active sessionClearing
c.sessionIdwhenDeleteSessionsuccessfully deletes that same session keeps the client’s in-memory cache consistent with WDA’s state and prevents future calls from trying to reuse a dead session.if c.sessionId == sessionId { c.sessionId = "" }This is a simple but important correctness improvement for the session lifecycle.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.