Fix async capture lifecycle and expose health diagnostics - #142
Open
JubaKitiashvili wants to merge 2 commits into
Open
JubaKitiashvili wants to merge 2 commits into
JubaKitiashvili wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds session health tracking, asynchronous native capture lifecycle APIs, stateful ChangesCapture session health
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DeviceSession
participant NativeCapture
participant SessionHealth
Client->>DeviceSession: Request stream or health
DeviceSession->>NativeCapture: Await capture startup
NativeCapture-->>DeviceSession: Frames or startup failure
DeviceSession->>SessionHealth: Record lifecycle and frame state
DeviceSession-->>Client: Stream or status response
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/serve-sim/src/device-session.ts`:
- Around line 300-301: Update the MJPEG and AVCC stream cleanup around the
unsubscribe listeners to use one release function that removes both event
listeners and observes any promise returned by unsubscribe. Ensure cleanup is
triggered by either close or error without allowing rejected asynchronous native
cleanup to become an unhandled rejection, and apply the same behavior in both
stream paths.
- Around line 222-225: Update the cleanup flow around the mjpegClients and
avccClients sets to explicitly end or destroy every active HTTP response before
clearing its set; keep the existing hidSockets closure and perform the
client-set clearing only after both response collections have been terminated.
In `@packages/serve-sim/src/session-health.ts`:
- Around line 71-75: Update markStarting() so retries from the "failed" phase
reset startedAt using this.now() and clear any stale frame state before
transitioning to "starting"; preserve the existing initialization behavior for
other phases and continue clearing error.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 159c71d7-7b06-4503-8de1-156faf88895d
📒 Files selected for processing (7)
packages/serve-sim/Sources/SimNative/CaptureEngine.swiftpackages/serve-sim/src/__tests__/device-session-health.test.tspackages/serve-sim/src/__tests__/session-health.test.tspackages/serve-sim/src/device-session.tspackages/serve-sim/src/middleware.tspackages/serve-sim/src/native.tspackages/serve-sim/src/session-health.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
start()/stop()methods as async and await their lifecycleBackground
#117 made the native capture lifecycle asynchronous, but the TypeScript handle still declared
start()andstop()as synchronousvoidmethods. Starting capture for a shutdown simulator could therefore leave a rejected Promise unobserved and terminate the Node process.Teardown had a second race: Swift
CaptureEngine.stop()launched framebuffer cleanup in a detached task and returned immediately. A concurrent startup could finish afterward and make a stopped session appear live again.This PR is a focused follow-up to #117. It intentionally isolates the lifecycle and recovery concern that is currently bundled with the larger Xcode 27 / Device Hub work in #140.
Changes
DeviceSession.start()andclose()with shared PromisesCaptureEnginestarting,ok,stalled,failed, andstoppedthrough/healthVerification
bun test packages/serve-sim/src/__tests__/session-health.test.ts packages/serve-sim/src/__tests__/device-session-health.test.ts— 9 passedbun run typecheckbun run lintbun run packages/serve-sim/build.ts503 faileddiagnostic, then boot + stream retry recovered to200 okin the same server processThe full local suite has one pre-existing/flaky camera-helper cleanup failure when run as a group (
shm-probe.integration.test.tsreports a null child exit code). The same integration file passes 7/7 when rerun in isolation.Summary by CodeRabbit
/healthendpoint with meaningful HTTP status responses.