fix(browser): one Browser app + host-capable Pi serves streamed sessions#1285
Conversation
Two related fixes so there is a single Browser app that works on the Pi: 1. Remove the duplicate 'Browser (Streamed)' app. The Browser app already has a per-tab Proxy/Streamed toggle (BrowserModeToggle) that attaches the tab to a real Neko/WebRTC session via the same LiveBrowserView the standalone app imported, so StreamedBrowserApp was pure duplication. Drops the registry entry, deletes the component, removes the stale vitest exclude. 2. Fix 'the Pi isn't capable': POST /api/browser/sessions only called pick_browser_node (Tier-2 workers, never the host), so a 16GB host with no browser workers returned no_capable_node. It now uses resolve_browser_target (explicit worker -> host if RAM-capable -> best worker) and starts on the host via browser_container_runner, mirroring the already-working /sessions/mine path. 2 tests: capable host places on host; non-capable host + no workers -> 409. Browser + registry suites green.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 24 minutes and 52 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoves the ChangesBrowser Session Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review 👍 Approved with suggestions 0 resolved / 1 findingsConsolidates browser streaming into a single app and enables host-capable Raspberry Pi nodes for session hosting. Ensure session records are properly cleaned up if the worker lookup fails post-creation. 💡 Edge Case: Orphaned session record when worker lookup fails after create📄 tinyagentos/routes/browser_sessions.py:129-143 In the new This is a behavior change from the previous code, where the Impact is minor (rare race, leftover idle record), but over time these orphans can accumulate. Consider resolving the worker before calling Resolve the worker before creating the session record so a failed lookup returns 409 without leaving an orphan.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 6 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| mgr = request.app.state.browser_sessions | ||
| session = await mgr.create_session( | ||
| "user", user_id, body.url, body.profile or "default" | ||
| ) | ||
| vol = f"taos-browser-{session['id']}" | ||
| auth_token = getattr(request.app.state, "browser_worker_auth_token", None) | ||
| try: | ||
| session = await mgr.start_on_worker( | ||
| session["id"], | ||
| node=node, | ||
| worker_url=worker.url, | ||
| profile_volume=f"taos-browser-{session['id']}", | ||
| auth_token=auth_token, | ||
| ) | ||
| if kind == "host": | ||
| runner = request.app.state.browser_container_runner | ||
| # _connecting_host_ip does a blocking DNS lookup; offload it so the | ||
| # event loop is not stalled while the host is resolved. | ||
| nat1to1_ip = await asyncio.to_thread(_connecting_host_ip, request) | ||
| session = await mgr.start_on_host( | ||
| session["id"], profile_volume=vol, runner=runner, nat1to1_ip=nat1to1_ip | ||
| ) |
There was a problem hiding this comment.
💡 Edge Case: Orphaned session record when worker lookup fails after create
In the new create_session, the session record is now created (mgr.create_session(...) at line 130) before the worker is resolved. In the worker branch, cluster.get_worker(node) can return None (line 145-147), which returns a 409 no_capable_node but leaves a freshly-created session row persisted in the store with no container ever started (an orphan in pending/idle state).
This is a behavior change from the previous code, where the worker is None check ran before create_session, so no orphan was created. resolve_browser_target returns ("worker", node) based on pick_browser_node / list_browser_nodes, and there's a small TOCTOU window (or stale node listing) where get_worker can then return None.
Impact is minor (rare race, leftover idle record), but over time these orphans can accumulate. Consider resolving the worker before calling create_session, or cleaning up the session record before returning 409. Note the same orphan exists for the BrowserWorkerError/502 path, but that was pre-existing.
Resolve the worker before creating the session record so a failed lookup returns 409 without leaving an orphan.:
kind, node = target
worker = None
if kind == "worker":
worker = cluster.get_worker(node)
if worker is None:
return JSONResponse({"error": "no_capable_node"}, status_code=409)
mgr = request.app.state.browser_sessions
session = await mgr.create_session(
"user", user_id, body.url, body.profile or "default"
)
vol = f"taos-browser-{session['id']}"
auth_token = getattr(request.app.state, "browser_worker_auth_token", None)
try:
if kind == "host":
...
else:
session = await mgr.start_on_worker(session["id"], node=node, worker_url=worker.url, profile_volume=vol, auth_token=auth_token)
Was this helpful? React with 👍 / 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_routes_browser_sessions.py (1)
116-126: ⚡ Quick winConsider asserting
create_sessionwas not called on early 409.When
resolve_browser_targetreturnsNone, the route returns 409 beforemgr.create_sessionis invoked. Adding an assertion strengthens the test by verifying no orphan session is created on this path.assert resp.status_code == 409 assert resp.json()["error"] == "no_capable_node" + mgr.create_session.assert_not_called()🤖 Prompt for 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. In `@tests/test_routes_browser_sessions.py` around lines 116 - 126, The test_create_409_when_no_host_and_no_workers method should verify that when a 409 response is returned due to no capable nodes, the mgr.create_session method is not invoked. Add an assertion after the existing resp.status_code and resp.json() assertions to call assert_not_called() on the mgr.create_session mock, ensuring that no orphan session is created when the route returns early with a 409 error.tinyagentos/routes/browser_sessions.py (1)
119-154: 💤 Low valueOrphan session on TOCTOU race when worker disappears.
If
resolve_browser_targetreturns("worker", node)butcluster.get_worker(node)returnsNoneat line 145-147 (worker deregistered between the two calls), the session created at line 130-132 is left orphaned inpendingstatus. This is a rare race, but worth noting.Consider either:
- Moving session creation after the worker lookup succeeds, or
- Deleting the orphan session before returning 409.
Given the rarity of this race and that pending sessions can be garbage-collected later, this is low-priority.
♻️ Optional fix: clean up orphan session
else: worker = cluster.get_worker(node) if worker is None: + await mgr.delete_session(session["id"]) return JSONResponse({"error": "no_capable_node"}, status_code=409)🤖 Prompt for 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. In `@tinyagentos/routes/browser_sessions.py` around lines 119 - 154, There is a race condition where a session is created by calling mgr.create_session() before verifying the worker exists. If cluster.get_worker(node) returns None (worker was deregistered between the resolution and lookup), the session is left orphaned. Move the mgr.create_session() call to after the worker is successfully retrieved in the worker branch, so the session is only created once the worker is confirmed to exist. Alternatively, if you need to keep session creation early, capture the session id before the worker check and call a cleanup method on mgr to delete the orphan session before returning the 409 error for no_capable_node.
🤖 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.
Nitpick comments:
In `@tests/test_routes_browser_sessions.py`:
- Around line 116-126: The test_create_409_when_no_host_and_no_workers method
should verify that when a 409 response is returned due to no capable nodes, the
mgr.create_session method is not invoked. Add an assertion after the existing
resp.status_code and resp.json() assertions to call assert_not_called() on the
mgr.create_session mock, ensuring that no orphan session is created when the
route returns early with a 409 error.
In `@tinyagentos/routes/browser_sessions.py`:
- Around line 119-154: There is a race condition where a session is created by
calling mgr.create_session() before verifying the worker exists. If
cluster.get_worker(node) returns None (worker was deregistered between the
resolution and lookup), the session is left orphaned. Move the
mgr.create_session() call to after the worker is successfully retrieved in the
worker branch, so the session is only created once the worker is confirmed to
exist. Alternatively, if you need to keep session creation early, capture the
session id before the worker check and call a cleanup method on mgr to delete
the orphan session before returning the 409 error for no_capable_node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93d1efe2-f75f-4f04-88df-58a84b0062c2
📒 Files selected for processing (7)
desktop/src/apps/StreamedBrowserApp/StreamedBrowserApp.test.tsxdesktop/src/apps/StreamedBrowserApp/StreamedBrowserApp.tsxdesktop/src/apps/StreamedBrowserApp/index.tsdesktop/src/registry/app-registry.tsdesktop/vite.config.tstests/test_routes_browser_sessions.pytinyagentos/routes/browser_sessions.py
💤 Files with no reviewable changes (4)
- desktop/src/apps/StreamedBrowserApp/index.ts
- desktop/vite.config.ts
- desktop/src/apps/StreamedBrowserApp/StreamedBrowserApp.tsx
- desktop/src/apps/StreamedBrowserApp/StreamedBrowserApp.test.tsx
…1285) create_session created the session record before resolving the worker, so a failed get_worker in the worker branch returned 409 while leaving an orphaned session row in pending/idle. Move the worker lookup ahead of create_session. Test: worker-lookup failure 409s and create_session is never called.
1 participant
Two related fixes so there is one Browser app that actually works on the Pi (Jay flagged both: the duplicate app, and 'the Pi isn't capable').
1. Remove the duplicate streamed app
The Browser app already has a per-tab Proxy/Streamed toggle (
BrowserModeToggle) that attaches the active tab to a real Neko/WebRTC Chromium session, rendered by the sameLiveBrowserViewthe standaloneStreamedBrowserAppimported. So 'Browser (Streamed)' was pure duplication. Removed the registry entry + the component + the stale vitest exclude. The recent neko fixes (#1223 sandbox, #1228/#1230 backend) live inLiveBrowserView+/api/browser/sessions, which the toggle uses, so they are preserved.2. Fix 'the Pi isn't capable' (no_capable_node)
POST /api/browser/sessions(what the toggle hits) only calledpick_browser_node, which by design returns Tier-2 worker nodes and never the host. So a 16GB Pi with no browser workers always returnedno_capable_node, even though there is a host path (start_on_hostviabrowser_container_runner, RAM floor 6GB) that the/sessions/mineroute already uses. The create path now uses the canonicalresolve_browser_target(explicit worker -> host if RAM-capable -> best worker) and starts on the host, mirroring the mine path. The host runtime is already wired in lifespan (wire_browser_runtime).Tests
2 new: capable host (16GB, no workers) places on host (201,
node=host,start_on_hostcalled); non-capable host (4GB) + no workers -> 409. Existing browser session + manager + registry suites green; compileall clean.Guardrails: no new auth, no DB migration, no installer/boot.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes