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

Aperta
#245 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
5/5
Tempo stimato
Più di una settimana
Idoneità per principianti
35/100
Tipo di issue
Refactoring
Chiarezza
Abbastanza chiara
Stato di attività
Tranquilla
Stack tecnologico
javascript, kotlin, node.js, swift

Direzione di ricerca

Inizia da ARCHITECTURE §§3, 4, 5.4 e 8.1, quindi segui ComapeoCoreService.onBind, NodeJSService.swift:199/646-660 e ComapeoCoreModule.kt:106-137. Esegui IPCLifecycleTests.swift e il flow esistente maestro/fgs-restart.yaml; il lavoro è completato quando sono verificati i callback del bridge, il comportamento snapshot/push/death di Messenger, gli scenari del ciclo di vita preservati, la documentazione aggiornata e la suite e2e invariata.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

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.

Lingua principale
Kotlin
Stelle
1
Fork
0
Merge medio
1g 6h
PR unite (30g)
5

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di digidem/comapeo-core-react-native

Tutte le issue di digidem/comapeo-core-react-native

Issue simili

Altre issue su Kotlin

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.