digidem / digidem/comapeo-core-react-native
refactor(android): retire the control socket for a bound Messenger between the FGS and main process
Nobody has claimed this yet.
- Dominant language
- Kotlin
- Stars
- 1
- Forks
- 0
- Avg merge
- 8h 24m
- Merged PRs (30d)
- 9
Description
After #243 and #244, control.sock carries four frames — started, ready, stopping, error — plus shutdown and error-native inbound. On iOS every one of those endpoints is in the same process as Node. On Android all of them are too, except one: the main app process, which needs lifecycle visibility into state that physically lives in the :ComapeoCore FGS.
This issue finishes the job: the in-process endpoints become addon calls, and the one genuinely cross-process channel moves from a second socket connection to a bound Messenger.
Lowest value of the three. Do it if the disconnect-inference rules are causing grief; skip it otherwise.
Why the remaining socket is the wrong shape
ARCHITECTURE §8.1 already scored this and picked the second NodeJSIPC connection. Two of its rejection reasons were premised on the current architecture and no longer hold once the addon exists:
Messenger/ bound service — Reject: moves state into Kotlin, duplicates Node's state machine, breaks "Node owns the truth"
With the addon, Node reports state to Kotlin in-process. State is already in Kotlin, delivered by Node rather than re-derived, so there is nothing to duplicate and Node still owns the truth. The objection evaporates. So does "replay logic lives in JS where the rest of the lifecycle lives" — there is no JS-side client list left to replay to.
Re-scored, the channel needs exactly three things:
- Current state on attach — the main process reloads constantly under Fast Refresh.
- Push on change.
- FGS-death detection.
A bound service gives all three: onServiceConnected → snapshot query, a reply Messenger for updates, and linkToDeath / onServiceDisconnected for liveness. Intent broadcasts give only (2) — sticky broadcasts are deprecated so (1) needs a separate query path, and there is no death signal at all. §8.1 was right to reject them and stays right. ContentProvider + ContentObserver, §8.1's runner-up, gives (1) and (2) but not (3).
Binder death notification is the real prize: it is the OS's own liveness signal and distinguishes "the process died" from "a socket closed", which lets the disconnect-inference block in ComapeoCoreModule.kt:106-137 be replaced by an explicit callback.
Two things make this cheaper than §8.1's ~100 LOC estimate: ComapeoCoreService is already a Service with an onBind stub returning null (line 482), and the state is already a MutableStateFlow (NodeJSService.kt:199).
What this removes
backend/lib/simple-rpc.jsentirely, and with itServerHelper's control-socket use, the readiness-phase replay, the terminal-frame cache, and the method table.control.sockfrom the backend: the bind, the path argv positional, the delete-before-bind cleanup, and the ordering constraint that it bind beforecomapeo.sock.ios/ControlFrame.swift,android/.../ControlFrame.kt, and the secondNodeJSIPCinstance on both platforms.MockBackend.swiftandMockNodeServer.swift, which exist to speak the control protocol over a socket during macOS swift-test runs.- The disconnect-inference rules in
ComapeoCoreModule.kt, replaced by an explicit death callback.
NodeJSIPC itself stays — comapeo.sock still needs it on both platforms. This does not eliminate the socket dependency, only this instance of it. That is the honest limit of the win.
Not a performance change
Stated explicitly so nobody reaches for it as justification: this channel carries roughly six messages per process lifetime. There is no throughput or allocation win here, and the copy-minimisation discipline in #244 does not apply. The case rests entirely on deleting simple-rpc.js, the second NodeJSIPC, and the disconnect-inference rules — and on linkToDeath being a better liveness signal than socket EOF.
Cost
The test harness is the bill. Roughly 1,100 lines assume a socket seam: MockBackend.swift (175), MockNodeServer.swift (146), IPCLifecycleTests.swift (271), ControlFrameTests.swift (164), ControlFrameTest.kt (165), simple-rpc.test.mjs (253). Production code deleted is smaller than test code rewritten. The replacement seam is a fake bridge injected into NodeJSService, which MockNodeService.swift shows is already an established pattern.
Hard-crash detection changes on iOS. Today a SIGSEGV or OOM in the runtime is observed as control.sock closing. Without the socket, the signal is nodeEntryPoint(args) returning — which runNode() already classifies (NodeJSService.swift:646-660), so the path exists, but it becomes the only one and needs to be verified rather than assumed.
Plan
1. Addon surface
Node → native, fire-and-forget:
notifyState(phase) // "started" | "ready" | "stopping"
notifyError(phase, message) // terminal
Native → Node needs a threadsafe function, since shutdown and error-native originate on arbitrary native threads:
requestShutdown()
reportNativeError(phase, message)
Both must work mid-boot, before the backend has finished initialising — the equivalent of today's sendMessageSync queueing into the IPC's pending buffer when still connecting (NodeJSService.swift:574). Register the TSFN as early as the addon is loaded and buffer until the backend installs its handlers.
2. Backend
Replace controlIpcServer.setReadinessPhase("started" | "ready") and the two broadcast({type:"stopping"|"error"}) sites with the corresponding notifyState / notifyError calls, at exactly the same points. Register the requestShutdown and reportNativeError handlers where the SimpleRpcServer method table sits today, preserving the existing semantics — handleFatal still broadcasts, flushes and exits; shutdown still closes servers in the same order.
Delete simple-rpc.js and its test. Drop the control-socket argv positional and renumber the remaining ones, or better, switch the backend to named flags while touching this — the positional list is up to six entries and a renumber is exactly the kind of change that silently misassigns.
3. iOS
handleControlMessage becomes a set of callbacks registered on the bridge. backendState transitions stay identical. sendShutdownFrame / sendErrorNativeFrame become bridge calls. Delete controlIPC, ControlFrame.swift, and the control-socket path plumbing.
The started callback keeps its two side effects: closing the node-spawn boot span and setting backendState = .controlBound. Consider renaming that state now that nothing is bound.
4. Android FGS
Same as iOS for the Node-facing side. Additionally, fill in ComapeoCoreService.onBind to return a Messenger backed by a Handler, with a small protocol:
MSG_REGISTER— client sends its replyMessenger; service responds immediately with the currentstateFlow.valuesnapshot. This is the free late-join.MSG_STATE— service → client on everystateFlowchange, carrying state pluserrorPhase/errorMessage.- Service holds client
Messengers weakly and drops them onlinkToDeath.
Collect stateFlow in serviceScope and fan out. No new state derivation — deriveLifecycleState is unchanged and stays the single source.
5. Android main process
ComapeoCoreModule binds to the service in OnCreate (bindService with BIND_AUTO_CREATE — check the interaction with the dataSync FGS type, see below) and unbinds in OnDestroy. It consumes the published state directly instead of re-deriving it from frames, which deletes both the frame when block and the disconnect-inference rules.
onServiceDisconnected / binderDied becomes the explicit "FGS process died" signal, mapping to ERROR with node-runtime-unexpected only when the pre-death state was STARTING/STARTED — the same rule as today, but triggered by a real death notification rather than inferred from a socket close.
6. Docs
Rewrite ARCHITECTURE §3 (one socket, not two), §4 (no init handshake), §5.4, and §8.1 — the last should keep the original table and add why the scoring changed, rather than being replaced. §3.2's three justifications for two sockets are all gone by this point and the section should say so.
Testing
- Replace
MockBackend/MockNodeServerwith a fake bridge conforming to the same protocolNodeJSServiceconsumes.DeriveStateTests.swiftshould not need to change at all — if it does, the derivation was coupled to the transport and that is worth knowing. - New Android instrumented test for the bind protocol: bind → receive snapshot, state change → receive push, kill the FGS →
binderDiedfires and the main process lands inERROR. The last one pairs with the existingmaestro/fgs-restart.yamlflow. - Keep
IPCLifecycleTests.swift's scenarios and re-point them at the bridge; they encode real orderings that are worth preserving even though the transport changed. - The e2e suite should be unchanged. If
apps/e2eneeds edits, the JS-facing contract moved, which it must not.
Risks
bindService from the main process to a dataSync foreground service interacts with the FGS lifecycle — a bound service is kept alive by the binding, which could mask a stop the user requested via the notification action. Verify against ForegroundService.md's user-stop handling and the fgs-restart Maestro flow before committing to BIND_AUTO_CREATE; BIND_NOT_FOREGROUND or an unbound Messenger handshake may be the safer shape.
TSFN lifetime versus NodeMobileStartNode's once-per-process constraint on iOS: a botched acquire/release is unrecoverable without an app restart. Release on backend shutdown only, and never on JS reload — the module reloads, the Node thread does not.
Ordering between notifyState and the native stateFlow fan-out. Node is authoritative; make the FGS apply state synchronously on receipt so the main process cannot observe ready before the FGS's own backendState reflects it.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with ARCHITECTURE §§3, 4, 5.4, and 8.1, then trace ComapeoCoreService.onBind, NodeJSService.swift:199/646-660, and ComapeoCoreModule.kt:106-137. Run IPCLifecycleTests.swift and the existing maestro/fgs-restart.yaml flow; done means the bridge callbacks, Messenger snapshot/push/death behavior, preserved lifecycle scenarios, updated docs, and unchanged e2e suite are verified.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, kotlin, node.js, swift
- Domain
- backend, distributed-systems, mobile, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100