digidem / digidem/comapeo-core-react-native

refactor(android): retire the control socket for a bound Messenger between the FGS and main process

Open
#245 0 comments 0 reactions 0 assignees View on GitHub

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:

  1. Current state on attach — the main process reloads constantly under Fast Refresh.
  2. Push on change.
  3. 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.js entirely, and with it ServerHelper's control-socket use, the readiness-phase replay, the terminal-frame cache, and the method table.
  • control.sock from the backend: the bind, the path argv positional, the delete-before-bind cleanup, and the ordering constraint that it bind before comapeo.sock.
  • ios/ControlFrame.swift, android/.../ControlFrame.kt, and the second NodeJSIPC instance on both platforms.
  • MockBackend.swift and MockNodeServer.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 reply Messenger; service responds immediately with the current stateFlow.value snapshot. This is the free late-join.
  • MSG_STATE — service → client on every stateFlow change, carrying state plus errorPhase/errorMessage.
  • Service holds client Messengers weakly and drops them on linkToDeath.

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/MockNodeServer with a fake bridge conforming to the same protocol NodeJSService consumes. DeriveStateTests.swift should 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 → binderDied fires and the main process lands in ERROR. The last one pairs with the existing maestro/fgs-restart.yaml flow.
  • 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/e2e needs 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.