fix: BrainBar socket stall — serial queue deadlock blocks all new MCP connections#87
Merged
fix: BrainBar socket stall — serial queue deadlock blocks all new MCP connections#87
Conversation
…ilent failures P0 fix: sendResponse() had an infinite usleep retry loop when write() returned EAGAIN. One slow/hung socat client pinned the serial DispatchQueue, blocking acceptClient() for ALL new connections. Also fixes: - Startup race: socket now binds BEFORE database init so connections are accepted immediately after launch - Silent DB failure: logs prominently when database fails to open - Raw JSON fallback: framing parser now handles bare JSON-RPC without Content-Length headers (prevents silent hangs on manual tests) - NSNull notifications: handles id:null as a proper notification - Missing MCP methods: resources/list, prompts/list, ping now return proper empty results instead of -32601 errors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Owner
Author
|
@coderabbitai review |
Owner
Author
Owner
Author
|
@codex review |
|
You need to increase your spend limit or enable usage-based billing to run background agents. Go to Cursor |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ 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). (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2026-03-18T00:12:29.551ZApplied to files:
📚 Learning: 2026-03-18T00:12:08.774ZApplied to files:
📚 Learning: 2026-03-17T01:04:22.497ZApplied to files:
🔇 Additional comments (5)
📝 WalkthroughWalkthroughRouter and socket setup have been moved ahead of database initialization; database wiring is deferred until after the server listens. Socket write logic gains an EAGAIN retry/backoff with a max cap and centralized client disconnect handling. Framing gains a raw JSON-RPC fallback and router adds new RPC methods and notification handling. Changes
Sequence DiagramsequenceDiagram
participant Start as Server Start
participant Router as Router
participant Socket as Socket
participant DB as Database
participant Client as Client
rect rgba(100, 200, 150, 0.5)
Note over Start,Router: Initialization sequence
Start->>Router: Initialize (no DB dependency)
Router-->>Start: Ready
Start->>Socket: Create & bind socket
Socket-->>Start: Listening
Start->>DB: Initialize asynchronously
DB-->>Start: Opened or failed (logged)
alt DB opened
DB->>Router: Wire DB to router
end
end
rect rgba(150, 150, 200, 0.5)
Note over Client,Socket: Write with EAGAIN backoff
Client->>Socket: Send response
alt Write success (partial/full)
Socket-->>Client: Bytes written → reset retries
else EAGAIN / EWOULDBLOCK
Client->>Client: Increment eagainRetries
alt eagainRetries < 50
Client->>Socket: Retry write after short backoff
else eagainRetries >= 50
Client->>Client: Drop client, log and disconnect
end
else Other write error
Client->>Client: Log error and disconnectClient(fd:)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 (1)
brain-bar/Sources/BrainBar/BrainBarServer.swift (1)
181-214:⚠️ Potential issue | 🟠 MajorDisconnect during send can be undone by stale state writeback.
sendResponsenow callsdisconnectClient(fd:)on stall/error, butreadFromClientlater unconditionally writesclients[fd] = state(Line 178). That can reinsert a disconnected client entry and leave stale tracking.Suggested fix
- clients[fd] = state + if var current = clients[fd] { + current.framing = state.framing + clients[fd] = current + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@brain-bar/Sources/BrainBar/BrainBarServer.swift` around lines 181 - 214, sendResponse calls disconnectClient(fd:) on errors but readFromClient still unconditionally does clients[fd] = state, which can reinsert a disconnected client and revive stale state; fix by making readFromClient check that the client is still present and not cancelled before writing back (e.g., verify clients[fd] exists and clients[fd]?.source.isCancelled == false or guard let client = clients[fd] else { return }), or update disconnectClient to mark a removed/closed flag that readFromClient checks before assigning, ensuring clients[fd] is only updated if the client remains connected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@brain-bar/Sources/BrainBar/BrainBarServer.swift`:
- Around line 118-129: The startup now accepts connections before the DB is
ready but MCPRouter's brain_search handler returns a false-success `"[]"` when
database == nil; change the handler in MCPRouter (the brain_search route
implementation around where it checks for a nil database) to detect a nil or
not-ready database and return an explicit readiness error/HTTP error message
(matching the contract used elsewhere, e.g., the graceful error path referenced
in BrainBarServer.swift and other tools/call behavior) instead of returning an
empty array string; ensure you reference the router.setDatabase/database
property semantics so once BrainBarServer assigns database the brain_search
handler resumes normal behavior.
In `@brain-bar/Sources/BrainBar/MCPRouter.swift`:
- Around line 39-41: The handler for the "notifications/initialized" branch
currently returns an empty dictionary even when an RPC id is present, which
leaves request-mode clients waiting; update the "notifications/initialized" case
in MCPRouter.swift so that when an id has been validated as non-notification it
produces a proper RPC response containing that id and a success result (e.g., an
empty result body or explicit status) instead of returning [:], ensuring the
response shape matches other RPC responses emitted by the router.
---
Outside diff comments:
In `@brain-bar/Sources/BrainBar/BrainBarServer.swift`:
- Around line 181-214: sendResponse calls disconnectClient(fd:) on errors but
readFromClient still unconditionally does clients[fd] = state, which can
reinsert a disconnected client and revive stale state; fix by making
readFromClient check that the client is still present and not cancelled before
writing back (e.g., verify clients[fd] exists and
clients[fd]?.source.isCancelled == false or guard let client = clients[fd] else
{ return }), or update disconnectClient to mark a removed/closed flag that
readFromClient checks before assigning, ensuring clients[fd] is only updated if
the client remains connected.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b449472-969f-4b0b-9b80-e87f739efc9b
📒 Files selected for processing (3)
brain-bar/Sources/BrainBar/BrainBarServer.swiftbrain-bar/Sources/BrainBar/MCPFraming.swiftbrain-bar/Sources/BrainBar/MCPRouter.swift
📜 Review details
⏰ 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). (3)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 0
File: :0-0
Timestamp: 2026-03-17T01:04:11.749Z
Learning: The socket path `/tmp/brainbar.sock` is intentional for the BrainBar Swift daemon (brain-bar/) and must NOT be changed to `/tmp/brainlayer.sock`. BrainBar is a new daemon that coexists with the existing Python `brainlayer-mcp` (which uses `/tmp/brainlayer.sock`) during the migration period. The different paths avoid conflicts and allow A/B testing. Once BrainBar is proven stable, the Python server will be retired and `.mcp.json` will point to `/tmp/brainbar.sock` via socat.
📚 Learning: 2026-03-17T01:04:22.497Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 0
File: :0-0
Timestamp: 2026-03-17T01:04:22.497Z
Learning: In BrainLayer, the BrainBar daemon uses the socket path `/tmp/brainbar.sock` (NOT `/tmp/brainlayer.sock`). BrainBar is a new native Swift daemon designed to coexist with the existing Python `brainlayer-mcp` server during the migration period. Different socket paths avoid conflicts and enable A/B testing. Once BrainBar is proven stable, the Python server will be retired.
Applied to files:
brain-bar/Sources/BrainBar/BrainBarServer.swift
📚 Learning: 2026-03-17T01:04:11.749Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 0
File: :0-0
Timestamp: 2026-03-17T01:04:11.749Z
Learning: The socket path `/tmp/brainbar.sock` is intentional for the BrainBar Swift daemon (brain-bar/) and must NOT be changed to `/tmp/brainlayer.sock`. BrainBar is a new daemon that coexists with the existing Python `brainlayer-mcp` (which uses `/tmp/brainlayer.sock`) during the migration period. The different paths avoid conflicts and allow A/B testing. Once BrainBar is proven stable, the Python server will be retired and `.mcp.json` will point to `/tmp/brainbar.sock` via socat.
Applied to files:
brain-bar/Sources/BrainBar/BrainBarServer.swift
🔇 Additional comments (3)
brain-bar/Sources/BrainBar/MCPFraming.swift (1)
74-102: Raw JSON fallback is a good resilience addition.The newline-first + tail parse strategy is pragmatic and keeps incomplete trailing data buffered correctly.
brain-bar/Sources/BrainBar/MCPRouter.swift (1)
46-51: New MCP methods and shared result helper look correct.Returning protocol-valid empty payloads for
resources/list,prompts/list, andpingis the right behavior.Also applies to: 226-232
brain-bar/Sources/BrainBar/BrainBarServer.swift (1)
185-197: Good fix: bounded EAGAIN retries eliminate queue pinning.Capping retries and dropping stalled clients addresses the prior infinite-loop deadlock class.
…ation ack - brain_search now throws noDatabase error instead of returning "[]" when DB hasn't loaded yet (reachable during socket-before-DB startup) - notifications/initialized with an id now returns a proper ack instead of empty response (which would hang the client) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
EtanHey
added a commit
that referenced
this pull request
Mar 18, 2026
After sendResponse() calls disconnectClient(fd:) to drop a stalled client, readFromClient() would write the ClientState copy back into the clients dictionary at line 178, re-inserting a zombie entry with a cancelled DispatchSource. Now checks clients[fd] after each sendResponse() and bails if the client was removed. Also: write() returning 0 now properly disconnects instead of silently breaking the write loop. Found by code review agent on PR #87. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
EtanHey
added a commit
that referenced
this pull request
Mar 18, 2026
After sendResponse() calls disconnectClient(fd:) to drop a stalled client, readFromClient() would write the ClientState copy back into the clients dictionary at line 178, re-inserting a zombie entry with a cancelled DispatchSource. Now checks clients[fd] after each sendResponse() and bails if the client was removed. Also: write() returning 0 now properly disconnects instead of silently breaking the write loop. Found by code review agent on PR #87. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
3 tasks
EtanHey
added a commit
that referenced
this pull request
Mar 29, 2026
…gle-instance Quick Capture controller with capture→store and search→results flows: - QuickCaptureController: capture (brain_store) and search (brain_search) with formatted output. Empty content validation. Default importance 5. - QuickCapturePanelState: mode (capture/search), visibility, toggle/dismiss - HotkeyManager + GestureStateMachine: CGEventTap for F4 hotkey (keycodes 118+129), hold/tap/double-tap detection. Ported from VoiceBar PR #87. Single-instance enforcement: - BrainBarApp checks NSRunningApplication on launch, exits if another instance is already running. Prevents duplicate menu bar icons. - build-app.sh kills existing BrainBar processes before installing. 11 new tests: capture flow (3), search flow (2), panel state (2), hotkey config (1), gesture state machine (3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
EtanHey
added a commit
that referenced
this pull request
Mar 29, 2026
…gle-instance Quick Capture controller with capture→store and search→results flows: - QuickCaptureController: capture (brain_store) and search (brain_search) with formatted output. Empty content validation. Default importance 5. - QuickCapturePanelState: mode (capture/search), visibility, toggle/dismiss - HotkeyManager + GestureStateMachine: CGEventTap for F4 hotkey (keycodes 118+129), hold/tap/double-tap detection. Ported from VoiceBar PR #87. Single-instance enforcement: - BrainBarApp checks NSRunningApplication on launch, exits if another instance is already running. Prevents duplicate menu bar icons. - build-app.sh kills existing BrainBar processes before installing. 11 new tests: capture flow (3), search flow (2), panel state (2), hotkey config (1), gesture state machine (3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
EtanHey
added a commit
that referenced
this pull request
Mar 29, 2026
* feat: add BrainBar quick capture foundation — controller, hotkey, single-instance Quick Capture controller with capture→store and search→results flows: - QuickCaptureController: capture (brain_store) and search (brain_search) with formatted output. Empty content validation. Default importance 5. - QuickCapturePanelState: mode (capture/search), visibility, toggle/dismiss - HotkeyManager + GestureStateMachine: CGEventTap for F4 hotkey (keycodes 118+129), hold/tap/double-tap detection. Ported from VoiceBar PR #87. Single-instance enforcement: - BrainBarApp checks NSRunningApplication on launch, exits if another instance is already running. Prevents duplicate menu bar icons. - build-app.sh kills existing BrainBar processes before installing. 11 new tests: capture flow (3), search flow (2), panel state (2), hotkey config (1), gesture state machine (3). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: @BugBot comprehensive review - 8 bugs identified (2 critical, 3 high, 3 medium) Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>
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
sendResponse()had an infiniteusleep(1000)retry loop whenwrite()returned EAGAIN — one slow socat client pinned the serial DispatchQueue, blockingacceptClient()for all new connectionsresources/list,prompts/list,pingnow return proper empty results instead of-32601errorsTest plan
swift build -c releaseclean🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes