feat!: reconnect IPC and recover subscriptions after a backend restart - #226
Open
gmaclennan wants to merge 32 commits into
Open
feat!: reconnect IPC and recover subscriptions after a backend restart#226gmaclennan wants to merge 32 commits into
gmaclennan wants to merge 32 commits into
Conversation
Member
Author
|
Reworked in response to review (this PR + its stack base + the upstream PRs all got follow-ups):
Jest 85/85, build + lint clean against the branch-built upstream APIs. |
gmaclennan
force-pushed
the
feat/rpc-transport-recovery
branch
from
August 13, 2026 21:47
dcbf6de to
039359e
Compare
Moves the embedded runtime from nodejs-mobile v18.20.4 to the digidem fork's v24.19.0-0. The fork versions releases `<node>-<mobile-rev>` and renamed its assets, so the download script changes shape as well as version. Two things the upgrade forces rather than merely allows: `--no-experimental-fetch` is gone from iOS argv — Node removed the flag in 23, and passing an unknown flag aborts before any JS runs. The iOS build now serves WebAssembly through a polyfill inside nodejs-mobile itself and has a working `fetch`, so the whole iOS-only shim stack goes with it: `index.ios.js`, the polywasm/undici installers, the SIMD-wasm alias and the loader-entry redirect. Both platforms now bundle from one entry, differing only in the `__loadAddon` banner. better-sqlite3 11 doesn't compile against V8 13.6 (Node 24 dropped the `ObjectTemplate::SetAccessor` overload it uses), and as a raw V8 addon it needs an ABI-matched prebuild per Node version, so the tree is pinned to one 12.10.0 through `overrides` and consumes the new ABI 137 prebuilds. Also picked up along the way: - TMPDIR now points at a real directory. An Android app process has none and there is no `/tmp`, which is where `os.tmpdir()` otherwise lands; reading it needed the credentials fix that arrived with this release. - V8's on-disk code cache is enabled via `NODE_COMPILE_CACHE`. Env var rather than `module.enableCompileCache()` so it covers `loader.mjs` and the Sentry chunk, which compile before any of our JS could call the runtime API. The backend flushes it at `ready` instead of leaving it to node's exit hook — the low-memory killer and iOS's suspended-app kill both skip that hook, so the cache would rarely be written at all. - The 24.x NodeMobile.xcframework has no x86_64 simulator slice, so the x64-simulator prebuild leg and its `lipo` pass are gone. - `readNodeJsMobileVersions()` matched `NODE_MODULE_VERSION (.+)`, which on the Node 24 header hits the `NODE_EMBEDDER_MODULE_VERSION` passthrough first and yields a garbage ABI in prebuild URLs. - Node 24's `v8config.h` #errors below C++20; the NDK defaults to gnu++17. - Sentry events carry the mobile revision as a `nodejs_mobile` tag; `contexts.runtime` only has the upstream Node version.
`lite` drops ICU, the inspector, `node:sqlite` and TypeScript type-stripping, and on iOS also V8's compiled tiers — dead weight there, since it runs jitless and serves WebAssembly through the bundled polyfill. None of it is a loss. The v18 build we came from reports 0 ICU symbols and no `icudt` data, i.e. it was already `--with-intl=none`, so `full` would *add* an `Intl` this backend has never had; the two `Intl` references that survive bundling are a Sentry helper that returns early unless `process.versions.icu` is set, and mapbox style-spec expression evaluators whose exposure is unchanged from v18. We use the `better-sqlite3` addon rather than `node:sqlite`, ship plain JS, and every `node:inspector` reference in the bundle is an `await import()` inside a try/catch or an opt-in Sentry integration. Addon prebuilds are flavour-neutral: both flavours ship byte-identical headers and export the same V8 symbol set, and every symbol our better-sqlite3 prebuild imports resolves against the lite `libnode`. Sizes against the v18 build this replaces, rather than against full: android arm64 61 -> 65 MB android armv7a 57 -> 59 MB android x86_64 63 -> 64 MB ios device slice 53 -> 44 MB
Bumps the minor-and-patch group with 1 update in the /backend directory: [rolldown](https://github.com/rolldown/rolldown/tree/HEAD/packages/rolldown). Updates `rolldown` from 1.2.3 to 1.2.4 - [Release notes](https://github.com/rolldown/rolldown/releases) - [Changelog](https://github.com/rolldown/rolldown/blob/main/CHANGELOG.md) - [Commits](https://github.com/rolldown/rolldown/commits/v1.2.4/packages/rolldown) --- updated-dependencies: - dependency-name: rolldown dependency-version: 1.2.4 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com>
12.10.0 built against Node 24 crashes on real hardware. The backend reaches ready and aborts ~86ms later, on every boot: # node[2591]: node::RemoveEnvironmentCleanupHook(...) at ../src/api/hooks.cc:142 # Assertion failed: (env) != nullptr Fatal signal 6 (SIGABRT) Everything downstream in the e2e run is fallout — RPC_TIMEOUT, socket connect timeouts, `device-id` never rendering. `libbetter-sqlite3__12.10.0.so` was the only shipped addon importing `Add`/`RemoveEnvironmentCleanupHook`, the raw V8 embedder API; the other six are N-API and import none of it. 13.0.0 migrated to N-API. The shipped binary now has zero V8 symbols and zero cleanup-hook references, so the crash class is gone by construction rather than patched. It also stops being ABI-locked: one artifact serves every Node version, which is why `usesNapi` flips and the prebuild URL loses its `-node-<abi>` infix. 13.x dropped `bindings` for its own `lib/binding.js`, which knows only linux/darwin/win32 prebuild paths before falling back to node-gyp build dirs — none of which exist on device — so the addon-loader gains a pattern for it. Because every pattern here matches upstream source text, a miss would silently ship a resolver that only fails on a device; the plugin now fails the build instead when that specific resolver goes unrewritten. Verified: 11/11 Android lifecycle tests and 8/8 iOS XCTest, 12 clean boots, zero occurrences of the crash signature.
Removing this alias alongside the polywasm/fetch installers was wrong. Those installers are genuinely redundant now — the runtime supplies `WebAssembly` and `fetch` — but the alias targets a different thing: the npm `undici@6` this backend bundles, not Node's built-in undici. nodejs-mobile 24's bootstrap sets `UNDICI_NO_WASM_SIMD=1` to steer undici off the SIMD build of llhttp, but that env var is an undici 7.x feature, so the bundled 6.23.0 copy ignores it and calls `WebAssembly.compile(llhttp_simd-wasm)` unconditionally. polywasm compiles function bodies lazily, so that compile succeeds and then throws `Unsupported instruction: 0xFD` on the first parser callback — past the try/catch undici wraps the compile in. `@comapeo/core`'s maps plugin and `secret-stream-http` (via `@comapeo/map-server`) both import `fetch` from that bundled copy, so this covers online map styles and peer blob/SMP fetches. Verified: the SIMD payload is present in both bundles without the alias, and absent from the iOS bundle with it. The e2e suite could not have caught this — its map-server tests use the built-in fallback map only, with no project, no uploaded SMP and no network. Also from review: don't set TMPDIR/NODE_COMPILE_CACHE to a directory we failed to create (and check `isDirectory`, since `mkdirs()` returns false when the directory already exists); correct the JNI comment, which claimed TMPDIR must be set before startup — only NODE_COMPILE_CACHE must; drop stale `lipo` and non-NAPI references.
…bundle The redirect that keeps undici's SIMD llhttp away from polywasm matches an upstream import specifier, so an undici reshuffle turns it into a silent no-op and the SIMD bytes come back. Nothing downstream notices: the failure needs a real network fetch on a jitless device, and the e2e map-server tests deliberately use the built-in fallback map with no project, no uploaded SMP and no network. That is why removing the redirect went unnoticed in the first place. Assert the outcome rather than the mechanism — after writing the iOS bundle, fail if it still contains the SIMD payload. The marker is the first slice of that module's base64 that differs from its non-SIMD sibling, so it identifies the specific wasm build without depending on module names surviving minification. No undici installed means nothing to assert, which is the right answer if the dependency ever goes away (see #232). Verified in both directions: the build passes as-is, and fails with an actionable message when the redirect is made not to match.
- Drop NODE_COMPILE_CACHE_PORTABLE on both platforms. It keys cache entries by path relative to the cache dir, which only helps when the modules and the cache move together. Ours never do: the cache is in cacheDir/Library Caches and the JS is in filesDir/the app bundle, so the relative path still contains the varying component. Verified by moving the module dir between runs — the entry hash changes either way, so the setting bought nothing. - Report a failure to create either directory to Sentry via `logCapture`, the existing helper for notable non-exception events, rather than a local log line. A device that can never write there silently pays the cold-compile cost on every launch, so the rate is worth seeing. - Trim the addon-loader comments to what a maintainer of the current code needs: no better-sqlite3 version history, and no restating between the file header and the pattern it describes.
`broadcast()` only queues a frame — streamx defers the real write to the next tick — while `ServerHelper.close()` calls `destroySoon()` in the same tick. The deferred write then hit an ended socket, so the `stopping` frame never reached native, `close()` rejected on the resulting socket error and left `#state` stuck at "closing", and every shutdown logged a bogus "Client sent invalid message" plus an `ipcError` metric sample. `SocketMessagePort` now exposes `drained()`, `SimpleRpcServer.close()` awaits it for every client before delegating to `super.close()`, and `ServerHelper.close()` waits on the socket's "close" event directly instead of `once()`, which rejects if the socket errors first. Closes #231
nodejs-mobile 24 embeds undici, so the npm copy that @comapeo/core and secret-stream-http import was 389 KB of duplicate bundle — the largest package in the backend bundle. Alias `undici` at bundle time to a shim that exports the global `fetch` and recovers `Agent` from Node's global dispatcher. The dispatcher only exists once Node's internal undici has initialised, which `new Request()` forces cheaply; secret-stream-http subclasses `Agent` at module scope, so the shim has to resolve eagerly and throws if it cannot. With the npm copy gone, the llhttp wasm payloads leave the bundle entirely, so `aliasUndiciSimdWasmPlugin` and its build-time assertion have nothing left to guard.
The in-app jasmine suite intermittently never renders all-tests-done, and a stalled run left nothing to diagnose: no spec name in CI output, no device logs, no screenshot (Maestro's extendedWaitUntil dies before the screenshot step). In the e2e app, always render the currently-running spec (testID current-spec), give the progress counter a testID (test-progress), and wrap jasmineEnv.execute() in a 240s watchdog — inside Maestro's 300s window — that renders all-tests-done plus a 'Suite timed out during: <spec>' failure, so a stall reaches the screenshot step and names the culprit. Replace the NoopGlobalErrors stub with a real implementation over ErrorUtils and Hermes' rejection tracker (neither is wired up in Release), so uncaught errors and unhandled rejections are console.error'd, routed to jasmine, and surfaced in the UI instead of silently hanging the suite. In the run-browserstack-maestro action, on the final failed attempt download each failed session's device logs and screenshots (URLs verified against a real past build's session JSON), print the app-tagged device-log lines into the job log, and upload everything as a browserstack-diagnostics-<platform> artifact. Collection is best-effort and the upload step runs only after the run step has already failed, so diagnostics can never mask the real failure or change the action's exit semantics.
… minor-and-patch group across 1 directory (#229)
… reconnect window Review follow-ups on the reconnect machinery: - Deliver Disconnected/Error to the connection-state observer imperatively at the two terminal commit points (teardown completion, connect failure): the conflating StateFlow lets the auto-reconnect CAS Disconnected away before the collector runs, so a drop could be missed entirely — and JS recovery hangs off that exact emission. Duplicate delivery is documented as part of the observer contract; close() still suppresses all calls. - Guard ComapeoCoreModule's State.Error mapping to STARTING/STARTED so reconnect exhaustion after a graceful stop cannot reclassify a clean STOPPED as a spurious terminal ERROR (mirrors the Disconnected guard). - Widen the auto-reconnect window 60s -> 120s: a debug build or slow device can take over a minute to restart the backend, and the FGS-kill test itself budgets 90s for a debug boot. - Close the stale-epoch rollback wedge: a current-epoch disconnect now waits out a transient Disconnecting (which a stale disconnect rolls back to Connected) instead of being swallowed, so state can no longer wedge at Connected with dead IO loops. - Test hardening: retain accepted server sockets in closeSuppressesReconnect (GC finalization could close them mid-test and fake a reconnect), and assert >= 2 ready frames in FgsKillRecoveryTest (each control connection gets a ready replay, so a transient drop legitimately delivers more).
… backend restart When Android kills and restarts the :ComapeoCore service, the sockets now reconnect (PR #225) — but in-flight RPC calls still hung until the 30s timeout, and the restarted backend had lost every event subscription, so listeners went permanently deaf. The message socket now reports its connection state to JS as a transportStateChange event (declared on iOS for parity; never fires there — in-process Node death ends the app). On a drop, the module calls @comapeo/ipc's transport-reset helpers: in-flight calls reject with TransportClosedError (code RPC_TRANSPORT_CLOSED, re-exported here) so callers can tell "backend restarted, a read is safe to retry" from a real failure; subscriptions on the long-lived channels are re-sent through the native send queue; stale per-project clients are hard-closed so the next getProject mints a working one. subscribeToBackendRestart() fires once the backend is STARTED again after a drop — wire it to @comapeo/core-react's new subscribeToBackendRestart provider prop so its query caches re-fetch. docs/ARCHITECTURE.md §5.8 documents the recovery layers and the host-app state (module-scope captures in comapeo-mobile) that recovery cannot reach. Requires @comapeo/ipc >= the release containing digidem/comapeo-ipc#87 (which itself needs digidem/rpc-reflector#52); the dependency bump lands here once released.
… sockets Review found three interacting flaws in the drop-time wiring: resubscribing at drop time nudged the native connect out of its terminal Error state in a tight retry loop for as long as the backend stayed down; the restart signal keyed solely on the control-socket lifecycle state, so a message-socket-only drop hard-closed project clients but never told the app layer to re-fetch; and one throwing restart listener silenced the rest. Recovery is now two-phase: at drop time the ipc reset helpers only reject in-flight calls and hard-close stale project clients (the reshaped upstream API no longer bundles resubscription); once BOTH the transport is reconnected and the backend reports STARTED — in whichever order — the module replays subscriptions (idempotent server-side) and fires restart listeners, each isolated in try/catch. Also corrects the TransportClosedError wording (the response will never arrive; the call may have executed) and documents the recovery-window state flap and the not-covered surfaces in ARCHITECTURE.md §5.8.
@comapeo/ipc v10 makes project channels stable per projectPublicId and owns project lifecycle server-side, so the client-side recovery no longer hard-closes project references or distinguishes core/services reset helpers. Phase 1 (drop) calls the polymorphic notifyTransportReset on both clients, rejecting in-flight calls with rpc-reflector's ChannelClosedError (RPC_CHANNEL_CLOSED); phase 2 (reconnected AND STARTED) calls resubscribe on both. TransportClosedError no longer exists in v10 — RpcChannelClosedError is re-exported instead. The dependency is bumped to ^10.0.0 in the module and the backend; until v10 publishes it must be installed from a local checkout (npm install ../comapeo-ipc ../rpc-reflector --no-save), so the lockfiles intentionally stay at 9.0.1 and npm ci will fail.
The control server replays started/ready to late-connecting clients, so state transitions cannot distinguish an app-side reconnect (backend alive — must not fire restart listeners) from a real backend restart (must fire). The backend now generates one crypto.randomUUID() per process and stamps it on the ready frame; SimpleRpcServer replays the extra fields identically to late clients. The Kotlin control observer parses the nonce into ControlFrame.Ready, stores it, carries it on the stateChange payload, and exposes it via getBootNonce (a repeat ready with a NEW nonce re-emits STARTED even without an observed intermediate state). JS recovery compares the post-recovery nonce with the last one seen outside a recovery window: same nonce resubscribes silently, a new nonce also fires subscribeToBackendRestart listeners, the first-ever nonce is initial boot (no signal), and a missing nonce (older backend) fires conservatively. A ready frame without the field still parses, so mixed versions degrade gracefully.
@comapeo/ipc v10 removes the client-side project close, so the v9
close-semantics specs (close then expect rejections; close/re-open via a
fresh getProject) no longer describe the contract. Replace them with the
v10 contract: calls transparently survive a backend-side project close,
and getProject returns the same permanent reference afterwards. The
per-test mass-close teardown goes away with them — references are
permanent and there is nothing to tear down.
A backend-side close needs a trigger the production surface deliberately
does not expose, so the backend gains an e2e-only RPC channel,
@@comapeo-debug/lifecycle (rpc-reflector server over an { id, message }
sub-channel on the message socket), exposing closeProject(projectPublicId)
= manager.getProject + project.close. It is served only when the backend
starts with COMAPEO_DEBUG_LIFECYCLE=1; otherwise the id is ignored like
any foreign channel (@comapeo/ipc routes only its own @@comapeo/ prefix)
and debug calls time out — the specs then mark themselves pending, since
the flag is not yet plumbed through the native start path.
Covered by backend unit tests (served/unserved/coexistence over a real
socket). The e2e specs typecheck but are untested on device here. The
e2e app's @comapeo/core devDependency moves to 7.2.0 to match the
version @comapeo/ipc v10 types against.
Every closeProjectOnBackend call paid the full 10s RPC timeout before concluding the debug channel is unserved — ~100s per run across the ten lifecycle specs, and today every run pays it since the env flag is not yet plumbed into the native start path. Worse, the timeout was ambiguous: with the channel served, a closeProject that hung >10s was mistaken for an unserved channel and silently marked the spec pending. The debug server gains a ping() liveness method; the e2e client probes it once per app session and caches the result. An unserved channel now costs a single 10s probe with every later call skipping straight to pending, and once the probe has succeeded any closeProject rejection — timeouts included — propagates as a real failure.
If the message socket reconnects to a restarted backend before its drop event is processed, no recovery window ever opens and the new boot nonce arrives via stateChange with transportDropped false — previously it was recorded silently and the restart never fired. A nonce change observed outside a recovery window now runs the same resubscribe-then-fire sequence recovery would have (the transport is by definition connected at that point). Covered by a unit test for that ordering. Also documents in ARCHITECTURE.md §5.8 the pre-existing post-deadline wedge from the reconnect layer: past the ~120s reconnect window both sockets sit in terminal Error, a later RPC revives only the message socket, and the control socket reconnects only on the next foreground transition — until then getState() stays ERROR and resubscription is deferred while calls quietly work without events.
gmaclennan
force-pushed
the
feat/rpc-transport-recovery
branch
from
August 20, 2026 17:50
039359e to
0bf57a8
Compare
gmaclennan
marked this pull request as ready for review
August 20, 2026 17:50
gmaclennan
changed the title
RangerMauve
approved these changes
Aug 26, 2026
RangerMauve
left a comment
RangerMauve
left a comment
Contributor
There was a problem hiding this comment.
LGTM overall. minor comments
| @@ -1,187 +1,341 @@ | |||
| import { useState } from 'react' | |||
| import { useState } from "react"; | |||
Contributor
There was a problem hiding this comment.
Can we undo these style changes or maybe do them in main then rebase so that we can have less noise?
| * late-connecting clients, so a reconnect delivers the SAME nonce when the | ||
| * backend kept running and a NEW one when it restarted; the recovery logic | ||
| * fires `subscribeToBackendRestart` listeners only on a nonce change. | ||
| * Absent on iOS and from backends that predate the field. |
Contributor
There was a problem hiding this comment.
What does from backends that predate the field mean?
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
2 participants
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.
Makes an unexpected death of the
:ComapeoCoreforeground service recoverable end to end: the sockets reconnect on their own, in-flight RPC calls fail fast instead of hanging 30 s, event subscriptions are replayed, and the app is told — once, and only when it is true — that the backend lost its in-memory state.This PR now carries the Kotlin reconnect layer from #225 unchanged (its five commits are patch-identical here, verified by range-diff; that PR is closed in favour of one review surface) plus the recovery layers above it, aligned with
@comapeo/ipcv10 (digidem/comapeo-ipc#88).Kotlin: reconnect on drop
NodeJSIPCgainsreconnectOnDrop: exponential backoff (250 ms → 4 s, 120 s deadline), epoch-scoped single-flight teardown so a stale disconnect can never tear down its replacement connection, and imperative delivery of terminal transitions (the conflatingStateFlowcould drop aDisconnectedthat auto-reconnect CASes away microseconds later). The data socket also gains atransportStateChangeevent into JS — previously it had no state callback at all, which is why a backgrounded app never recovered.JS: two-phase recovery
On
disconnected/error,notifyTransportResetrejects every pending call on both clients withRPC_CHANNEL_CLOSED— no more 30 s hangs, and reads become distinguishable-retryable while mutations surface the error. Onconnectedwith the control channelSTARTED,resubscribereplays every live subscription; under v10's stable per-project channels that transparently re-opens the projects the app is actually observing. Nothing resubscribes at drop time (ON frames written into a down transport feed the native reconnect a hot loop). There are no wrapper hard-closes and no generation guards — v10 made both unnecessary, and the earlierTransportClosedErroris gone in favour of the shipped code.Boot nonce: restarts are detected, reconnects are not
subscribeToBackendRestart(listener)fires only on a genuine backend restart. Detection is a per-process nonce in the control channel'sreadyframe ({type: "ready", bootNonce}) — necessary because the control server deliberately replaysreadyto late-connecting clients, so state transitions alone cannot distinguish an app-side reconnect (backend alive, caches warm, must not fire) from a real restart (must fire). The frame is backward compatible (a barereadyparses with a null nonce, which conservatively fires). A nonce change observed outside a recovery window also fires, as defense in depth against a reconnect racing the control socket's drop.e2e
The v9 close-semantics specs are replaced by v10 contract specs: calls transparently survive a backend-side project close, and
getProjecthands back the same working reference afterwards. Backend-side closes are driven through a debug-only lifecycle channel (@@comapeo-debug/lifecycle), served per-connection only whenCOMAPEO_DEBUG_LIFECYCLE=1and invisible to the production router. The suite probes the channel once per session: unserved costs a single 10 s timeout and the specs self-mark pending; once the probe succeeds, a hang is a failure, not a pending. Plumbing the env flag into the native start path is follow-up work, so these specs report pending on device today.State of the branch
Root/backend lockfiles are intentionally untouched: manifests declare
@comapeo/ipc ^10.0.0(and the backendrpc-reflector ^4.5.0), both unpublished, sonpm cifails until digidem/rpc-reflector#55 and digidem/comapeo-ipc#88 release — local development links the checkouts withnpm install <path> --no-save. Test state: root suite 89 passing, backend suite 106 passing, Kotlin JVM unit suite green, e2e typechecks; on-device e2e needs the published packages.docs/ARCHITECTURE.md§5.8 documents the recovery design, including one known pre-existing limitation: past the 120 s reconnect deadline both sockets go terminal, and the control socket only reconnects on the next foreground transition.