diff --git a/.github/actions/run-browserstack-maestro/action.yml b/.github/actions/run-browserstack-maestro/action.yml index bfcf82d0..a6136169 100644 --- a/.github/actions/run-browserstack-maestro/action.yml +++ b/.github/actions/run-browserstack-maestro/action.yml @@ -35,6 +35,15 @@ inputs: connection resetting during launchApp), never on a real test failure. required: false default: "3" + artifact_name: + description: >- + Name of the workflow artifact that receives failure diagnostics + (device logs, screenshots, session JSON). Defaults to + browserstack-diagnostics-. Artifact names must be unique + per workflow run, so override this if one workflow calls this action + more than once per platform. + required: false + default: "" runs: using: composite @@ -73,6 +82,7 @@ runs: PLATFORM: ${{ inputs.platform }} TIMEOUT: ${{ fromJson(inputs.timeout) }} MAX_ATTEMPTS: ${{ fromJson(inputs.max_attempts) }} + DIAG_DIR: ${{ runner.temp }}/browserstack-diagnostics shell: bash run: | #shell set -o pipefail @@ -184,6 +194,61 @@ runs: done } + # Saves what BrowserStack already recorded (deviceLogs: true above) + # for each failed session into DIAG_DIR — device logs, screenshots, + # session JSON — for the upload step, and prints the app-relevant + # device-log lines so the stalled/failed spec is named in the job log. + # Best-effort by design: always returns 0 so a fetch error here can + # never mask the real failure. + collect_diagnostics() { + local build_id=$1 attempt=$2 + mkdir -p "$DIAG_DIR" || return 0 + printf '%s\n' "$BUILD_RESPONSE" > "$DIAG_DIR/attempt${attempt}-build.json" || true + while IFS=$'\t' read -r device session_id; do + [ -n "$session_id" ] || continue + local slug prefix detail + slug=$(printf '%s' "$device" | tr -cs 'A-Za-z0-9._-' '-') + prefix="$DIAG_DIR/attempt${attempt}-${slug}" + detail=$(curl --show-error -s -u "$AUTH" "$API/builds/$build_id/sessions/$session_id") || continue + printf '%s\n' "$detail" > "${prefix}-session.json" || true + while IFS=$'\t' read -r tc_name tc_status device_log screenshots video; do + [ -n "$tc_name" ] || continue + local tc_slug base + tc_slug=$(printf '%s' "$tc_name" | tr -cs 'A-Za-z0-9._-' '-') + base="${prefix}-${tc_slug}" + if [ -n "$device_log" ]; then + curl --show-error -s -u "$AUTH" -o "${base}-device.log" "$device_log" || true + if [ -s "${base}-device.log" ]; then + echo "::group::Device log (app lines) — $device / $tc_name ($tc_status)" + matches=$({ grep -E 'ReactNativeJS|\[e2e\]|ComapeoCore|Comapeo:NodeJS' "${base}-device.log" || true; } | tail -n 200) + if [ -n "$matches" ]; then + printf '%s\n' "$matches" + else + echo "(no app-tagged lines — expected on iOS, where console output is not persisted to the device log; use the screenshots)" + fi + echo "::endgroup::" + fi + fi + if [ -n "$screenshots" ]; then + curl --show-error -sL -u "$AUTH" -o "${base}-screenshots.zip" "$screenshots" || true + if [ -s "${base}-screenshots.zip" ] \ + && unzip -o -q -d "${base}-screenshots" "${base}-screenshots.zip"; then + rm -f "${base}-screenshots.zip" + fi + fi + if [ -n "$video" ]; then + echo "Video for $device / $tc_name: $video" + fi + done < <(printf '%s\n' "$detail" \ + | jq -r '.testcases.data[]?.testcases[]? | select(.status != "passed") + | [.name, .status, .device_log // "", .screenshots // "", .video // ""] | @tsv') + done < <(printf '%s\n' "$BUILD_RESPONSE" \ + | jq -r '.devices[]? as $d | $d.sessions[]? | select(.status != "passed") + | [(($d.device // $d.os // "device") + "-" + ($d.os_version // "")), .id] | @tsv') + echo "Diagnostics saved for upload as a workflow artifact." + return 0 + } + # Returns 0 only if there is at least one failed session and every # failed session is infra-class: either its structured session # `error.message` matches SESSION_ERR_RE (the session never started) @@ -243,12 +308,23 @@ runs: continue fi + collect_diagnostics "$BUILD_ID" "$attempt" || true echo "Tests failed (status: $STATUS)" exit 1 done exit 1 + # Runs only after the run step has already failed, so uploading (or a + # failure to upload) cannot change the action's outcome or exit code. + - name: Upload BrowserStack diagnostics + if: failure() + uses: actions/upload-artifact@v7 + with: + name: ${{ inputs.artifact_name != '' && inputs.artifact_name || format('browserstack-diagnostics-{0}', inputs.platform) }} + path: ${{ runner.temp }}/browserstack-diagnostics + if-no-files-found: ignore + - name: Stop BrowserStack build on cancel if: cancelled() && steps.run.outputs.build_id != '' env: diff --git a/.github/workflows/android-tests.yml b/.github/workflows/android-tests.yml index 969bbe87..e64abe36 100644 --- a/.github/workflows/android-tests.yml +++ b/.github/workflows/android-tests.yml @@ -17,7 +17,7 @@ permissions: contents: read env: - NODEJS_MOBILE_VERSION: v18.20.4 + NODEJS_MOBILE_VERSION: v24.19.0-0 # NDK the generated example app builds against. Sourced from React Native's # node_modules/react-native/gradle/libs.versions.toml (Expo's default); keep # in sync on RN bumps. We install it explicitly (with retry) so Gradle never diff --git a/.github/workflows/e2e-reusable.yml b/.github/workflows/e2e-reusable.yml index 0901172b..1d8c3cda 100644 --- a/.github/workflows/e2e-reusable.yml +++ b/.github/workflows/e2e-reusable.yml @@ -41,7 +41,7 @@ on: required: true env: - NODEJS_MOBILE_VERSION: v18.20.4 + NODEJS_MOBILE_VERSION: v24.19.0-0 jobs: # Decide whether the expensive build + paid BrowserStack device jobs run. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f2ee699f..67cb5bac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,11 @@ npm run setup # fetch nodejs-mobile, build the backend, install the test ap `npm run setup` runs, in order: - `download:nodejs-mobile` — pulls `NodeMobile.xcframework` (iOS) and `libnode.so` - per ABI plus headers (Android) into place; these are not committed. + per ABI plus headers (Android) into place; these are not committed. It fetches + the `lite` runtime flavour, which drops ICU, the inspector, `node:sqlite` and + TypeScript type-stripping — none of which we use. `NODEJS_MOBILE_FLAVOR=full` + fetches the full one; the two ship identical headers, so addon prebuilds work + against either. - `backend:build` — bundles the Node.js backend (`backend/`) that gets embedded in the app. `npm install` alone does **not** build it. - installs dependencies for the two test apps (`apps/integration`, `apps/e2e`). @@ -40,11 +44,13 @@ debug ID in the bundle, the consuming app uploads the maps with `comapeo-rn-upload-sourcemaps`, and Sentry matches them by that ID. The backend deliberately does **not** run with `--enable-source-maps` in any -variant. nodejs-mobile pins Node 18, whose `findSourceMap()` re-parses the whole -map on every `Error.stack` format — roughly 320 ms and 250–470 MB of garbage per -error for our 19 MB map, enough to wedge the event loop for tens of seconds on a -low-end device. For a stack you have in a terminal rather than in Sentry, -`comapeo-rn-symbolicate` remaps it offline from the shipped maps. +variant. Measured on the Node 18 nodejs-mobile used to pin, `findSourceMap()` +re-parsed the whole map on every `Error.stack` format — roughly 320 ms and +250–470 MB of garbage per error for our 19 MB map, enough to wedge the event +loop for tens of seconds on a low-end device. Node has since reworked its +source-map cache; the flag stays off until that's re-measured on device. For a +stack you have in a terminal rather than in Sentry, `comapeo-rn-symbolicate` +remaps it offline from the shipped maps. ## Repository layout diff --git a/README.md b/README.md index 36f1d48b..8f72bcfa 100644 --- a/README.md +++ b/README.md @@ -364,10 +364,10 @@ place of the flags. The maps live in sibling `nodejs-sourcemaps/` directories (not under the bundled `nodejs-project/` assets), so they are **not** shipped inside your APK/IPA. The -backend runs without Node's `--enable-source-maps` in every variant — on the -Node 18 that nodejs-mobile pins, that flag re-parses the entire map on every -error stack and can wedge the event loop for tens of seconds on a low-end -device. To remap a stack you have in a terminal rather than in Sentry: +backend runs without Node's `--enable-source-maps` in every variant — measured +on Node 18, that flag re-parsed the entire map on every error stack and could +wedge the event loop for tens of seconds on a low-end device. To remap a stack +you have in a terminal rather than in Sentry: ```sh adb logcat -d | npx comapeo-rn-symbolicate diff --git a/android/CMakeLists.txt b/android/CMakeLists.txt index 32d48654..67d24a87 100644 --- a/android/CMakeLists.txt +++ b/android/CMakeLists.txt @@ -33,6 +33,11 @@ add_library(${CMAKE_PROJECT_NAME} SHARED include_directories(libnode/include/node/) include_directories(src/main/cpp) +# Node 24's v8config.h #errors below C++20; the NDK's clang defaults to gnu++17. +set_target_properties(${CMAKE_PROJECT_NAME} PROPERTIES + CXX_STANDARD 20 + CXX_STANDARD_REQUIRED ON) + add_library( libnode SHARED IMPORTED ) diff --git a/android/src/androidTest/java/com/comapeo/core/NodeJSIPCTest.kt b/android/src/androidTest/java/com/comapeo/core/NodeJSIPCTest.kt index 74f2f530..cf747373 100644 --- a/android/src/androidTest/java/com/comapeo/core/NodeJSIPCTest.kt +++ b/android/src/androidTest/java/com/comapeo/core/NodeJSIPCTest.kt @@ -61,16 +61,21 @@ class NodeJSIPCTest { * To match, we bind a [LocalSocket] to the filesystem address and pass * its file descriptor to [LocalServerSocket]. */ - private fun startMockServer(onConnection: (DataInputStream, DataOutputStream) -> Unit) { + private fun bindServer(): LocalServerSocket { val bindSocket = LocalSocket(LocalSocket.SOCKET_STREAM) val address = LocalSocketAddress(socketFile.absolutePath, LocalSocketAddress.Namespace.FILESYSTEM) bindSocket.bind(address) boundSocket = bindSocket serverSocket = LocalServerSocket(bindSocket.fileDescriptor) + return serverSocket!! + } + + private fun startMockServer(onConnection: (DataInputStream, DataOutputStream) -> Unit) { + val server = bindServer() Thread { try { - val client = serverSocket!!.accept() + val client = server.accept() val input = DataInputStream(client.inputStream) val output = DataOutputStream(client.outputStream) onConnection(input, output) @@ -351,4 +356,127 @@ class NodeJSIPCTest { // The IPC should handle the server disconnect without crashing ipc.disconnect() } + + /** + * With reconnectOnDrop enabled, an unexpected server-side close followed by + * the server accepting again must converge back to a working connection + * without any sendMessage()/connect() nudge — the low-memory-kill recovery + * path where the FGS process restarts a few seconds later. + */ + @Test + fun reconnectsAfterUnexpectedServerDrop() { + val server = bindServer() + val firstAccepted = CountDownLatch(1) + val received = CountDownLatch(1) + + Thread { + try { + val first = server.accept() + firstAccepted.countDown() + Thread.sleep(300) + first.close() // unexpected drop + val second = server.accept() + Thread.sleep(300) // let the reconnected client's receive loop attach + writeFramedMessage( + DataOutputStream(second.outputStream), + """{"type":"after-reconnect"}""", + ) + Thread.sleep(5000) + } catch (e: IOException) { + // Server closed, expected during teardown + } + }.start() + + val ipc = NodeJSIPC(socketFile, reconnectOnDrop = true) { msg -> + receivedMessages.add(msg) + received.countDown() + } + try { + assertTrue("Should connect within 10s", firstAccepted.await(10, TimeUnit.SECONDS)) + assertTrue( + "Should auto-reconnect and receive within 15s", + received.await(15, TimeUnit.SECONDS) + ) + assertEquals("""{"type":"after-reconnect"}""", receivedMessages[0]) + } finally { + ipc.close() + } + } + + @Test + fun closeSuppressesReconnect() { + val server = bindServer() + val acceptCount = java.util.concurrent.atomic.AtomicInteger(0) + // Retain every accepted socket: an unreferenced LocalSocket can be + // GC-finalized (closed) mid-test, dropping the connection and causing + // a spurious reconnect before close() is even called. + val acceptedSockets = CopyOnWriteArrayList() + + Thread { + try { + while (true) { + acceptedSockets.add(server.accept()) + acceptCount.incrementAndGet() + } + } catch (e: IOException) { + // Server closed, expected during teardown + } + }.start() + + val ipc = NodeJSIPC(socketFile, reconnectOnDrop = true) { msg -> + receivedMessages.add(msg) + } + val deadline = System.currentTimeMillis() + 10_000 + while (acceptCount.get() == 0 && System.currentTimeMillis() < deadline) { + Thread.sleep(50) + } + assertEquals("Should connect once", 1, acceptCount.get()) + Thread.sleep(200) + + ipc.close() + + // Longer than several backoff steps (immediate + 250 + 500 + 1000ms); a + // post-close reconnect would show up as a second accept. + Thread.sleep(3000) + assertEquals("close() must not trigger reconnect attempts", 1, acceptCount.get()) + acceptedSockets.forEach { try { it.close() } catch (_: IOException) {} } + } + + @Test + fun doesNotReconnectByDefaultAfterUnexpectedDrop() { + val server = bindServer() + val acceptCount = java.util.concurrent.atomic.AtomicInteger(0) + + Thread { + try { + while (true) { + val client = server.accept() + acceptCount.incrementAndGet() + Thread.sleep(300) + client.close() // unexpected drop + } + } catch (e: IOException) { + // Server closed, expected during teardown + } + }.start() + + val ipc = NodeJSIPC(socketFile) { msg -> receivedMessages.add(msg) } + try { + val deadline = System.currentTimeMillis() + 10_000 + while (acceptCount.get() == 0 && System.currentTimeMillis() < deadline) { + Thread.sleep(50) + } + assertEquals("Should connect once", 1, acceptCount.get()) + + // Ample time for an (unwanted) reconnect after the drop at +300ms. + Thread.sleep(3000) + assertEquals( + "Default (reconnectOnDrop=false) must not reconnect", + 1, + acceptCount.get() + ) + } finally { + ipc.close() + } + } } diff --git a/android/src/main/cpp/jni-bridge.cpp b/android/src/main/cpp/jni-bridge.cpp index 0c43b39c..362447b6 100644 --- a/android/src/main/cpp/jni-bridge.cpp +++ b/android/src/main/cpp/jni-bridge.cpp @@ -97,6 +97,16 @@ class NodeJSService : public JavaClass { log("initialize: %s", nativeDataDir.c_str()); } + /// node reads TMPDIR and NODE_COMPILE_CACHE while the Environment is + /// created, so assigning `process.env` from JS is too late — callers must + /// set them here, before `startNodeWithArguments`. + static void setEnv(alias_ref, alias_ref name, alias_ref value) { + const auto nativeName = name->toStdString(); + const auto nativeValue = value->toStdString(); + setenv(nativeName.c_str(), nativeValue.c_str(), 1); + log("setEnv: %s=%s", nativeName.c_str(), nativeValue.c_str()); + } + static jint startNodeWithArguments(alias_ref, alias_ref> arguments) { log("Starting NodeJS with arguments."); @@ -141,6 +151,8 @@ class NodeJSService : public JavaClass { javaClassStatic()->registerNatives({ makeNativeMethod("initialize", NodeJSService::initialize), + makeNativeMethod("setEnv", + NodeJSService::setEnv), makeNativeMethod("startNodeWithArguments", NodeJSService::startNodeWithArguments), }); diff --git a/android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt b/android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt index 59845aeb..7bbedff0 100644 --- a/android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt +++ b/android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt @@ -30,6 +30,38 @@ class ComapeoCoreModule : Module() { /** Cleared on any non-ERROR transition so a fresh cycle can't surface stale details. */ private var lastError: Map? = null + /** + * Boot nonce from the latest `ready` frame — identifies the backend + * process currently serving. Null until the first `ready` (or from a + * backend that predates the field). JS compares it across recoveries to + * tell an app-side reconnect from a genuine backend restart. + */ + private var bootNonce: String? = null + + /** + * STARTED transition from a `ready` frame. Unlike the generic [setState] + * dedupe, a repeat `ready` carrying a NEW nonce must still emit — it means + * the backend restarted without this side ever observing a non-STARTED + * state in between. + */ + private fun setStarted(nonce: String?) { + val eventToEmit: Map? = synchronized(stateLock) { + val nonceChanged = nonce != null && nonce != bootNonce + if (nonce != null) bootNonce = nonce + if (jsState == JsState.STARTED && !nonceChanged) { + null + } else { + jsState = JsState.STARTED + lastError = null + buildMap { + put("state", JsState.STARTED.name) + bootNonce?.let { put("bootNonce", it) } + } + } + } + eventToEmit?.let { sendEvent("stateChange", it) } + } + private fun setState(next: JsState, errorPayload: Map? = null) { val eventToEmit: Map? = synchronized(stateLock) { when { @@ -73,19 +105,43 @@ class ComapeoCoreModule : Module() { val controlSocketFile = File(appContext.persistentFilesDirectory, ComapeoCoreService.CONTROL_SOCKET_FILENAME) - ipc = NodeJSIPC(socketFile) { message -> + // reconnectOnDrop on both sockets: when the system kills and restarts + // the :ComapeoCore process, the control socket is read-only from this + // side so nothing else would ever reconnect it, and the message socket + // would only recover if JS happened to call postMessage. + ipc = NodeJSIPC( + socketFile, + reconnectOnDrop = true, + onConnectionStateChange = { connState -> + // JS uses "disconnected"/"error" to reject in-flight RPC calls + // immediately instead of letting them run into the 30s timeout. + val transportState = when (connState) { + is NodeJSIPC.State.Connected -> "connected" + is NodeJSIPC.State.Disconnected -> "disconnected" + is NodeJSIPC.State.Error -> "error" + // Connecting/Disconnecting are transient; JS only cares + // whether the transport is usable. + else -> null + } + transportState?.let { + sendEvent("transportStateChange", mapOf("state" to it)) + } + }, + ) { message -> sendEvent("message", mapOf("data" to message)) } // The control socket replays `started`/`ready` to late-connecting clients, // so a fresh module instance always converges on the right state even if - // it joined after the FGS finished bootstrapping. + // it joined after the FGS finished bootstrapping — and a reconnect after + // an FGS restart converges back to STARTED the same way. controlIpc = NodeJSIPC( controlSocketFile, + reconnectOnDrop = true, onMessage = { message -> when (val frame = ControlFrame.parse(message)) { ControlFrame.Started -> setState(JsState.STARTING) - ControlFrame.Ready -> setState(JsState.STARTED) + is ControlFrame.Ready -> setStarted(frame.bootNonce) ControlFrame.Stopping -> setState(JsState.STOPPING) is ControlFrame.Error -> setState( JsState.ERROR, @@ -123,14 +179,23 @@ class ComapeoCoreModule : Module() { ) } } - is NodeJSIPC.State.Error -> setState( - JsState.ERROR, - mapOf( - "errorPhase" to "ipc", - "errorMessage" to (connState.exception.message - ?: connState.exception.javaClass.simpleName), - ), - ) + is NodeJSIPC.State.Error -> { + // Mirror the Disconnected guard: after a graceful stop the + // auto-reconnect exhausts its window into State.Error, which + // must not reclassify a clean STOPPED as ERROR — only a + // backend we believed live is an error. + when (synchronized(stateLock) { jsState }) { + JsState.STARTING, JsState.STARTED -> setState( + JsState.ERROR, + mapOf( + "errorPhase" to "ipc", + "errorMessage" to (connState.exception.message + ?: connState.exception.javaClass.simpleName), + ), + ) + JsState.ERROR, JsState.STOPPING, JsState.STOPPED -> {} + } + } // .Connected: just "we have a socket"; wait for `started`/`ready`. else -> {} } @@ -157,7 +222,7 @@ class ComapeoCoreModule : Module() { Name("ComapeoCore") - Events("message", "messageerror", "stateChange") + Events("message", "messageerror", "stateChange", "transportStateChange") Function("postMessage") { message: String -> ipc.sendMessage(message) @@ -171,6 +236,14 @@ class ComapeoCoreModule : Module() { synchronized(stateLock) { lastError } } + // Boot nonce of the backend currently serving (from its latest `ready` + // frame); null before the first `ready`. Lets JS read the nonce outside + // a stateChange event — needed when recovery completes on the + // transport-connected edge, where no stateChange fires. + Function("getBootNonce") { + synchronized(stateLock) { bootNonce } + } + // `sentryConfig` — baked-in by app.plugin.js at prebuild; spread into // `Sentry.init(...)` by the JS `/sentry` sub-export. Empty map when the // plugin isn't registered so spreading is always safe. `userId` is diff --git a/android/src/main/java/com/comapeo/core/ControlFrame.kt b/android/src/main/java/com/comapeo/core/ControlFrame.kt index 5a956d15..117a5891 100644 --- a/android/src/main/java/com/comapeo/core/ControlFrame.kt +++ b/android/src/main/java/com/comapeo/core/ControlFrame.kt @@ -11,7 +11,15 @@ import org.json.JSONObject */ sealed class ControlFrame { object Started : ControlFrame() - object Ready : ControlFrame() + + /** + * `bootNonce` identifies the backend process that emitted the frame (one + * random UUID per process). Null from backends that predate the field. + * Consumers compare it against the last-seen value to tell an app-side + * reconnect (same nonce — the replayed `ready` came from the same + * process) from a backend restart (new nonce). + */ + data class Ready(val bootNonce: String?) : ControlFrame() /** Graceful shutdown — sent before close so peers can tell expected from crash. */ object Stopping : ControlFrame() @@ -46,7 +54,7 @@ sealed class ControlFrame { } return when (val type = json.optString("type", "")) { "started" -> Started - "ready" -> Ready + "ready" -> Ready(json.optString("bootNonce", "").takeIf { it.isNotEmpty() }) "stopping" -> Stopping "error" -> Error( phase = json.optString("phase", "unknown"), diff --git a/android/src/main/java/com/comapeo/core/NodeJSIPC.kt b/android/src/main/java/com/comapeo/core/NodeJSIPC.kt index aa06b5fc..9ade3f3d 100644 --- a/android/src/main/java/com/comapeo/core/NodeJSIPC.kt +++ b/android/src/main/java/com/comapeo/core/NodeJSIPC.kt @@ -30,6 +30,10 @@ import java.nio.ByteOrder @OptIn(ExperimentalCoroutinesApi::class) class NodeJSIPC( private val socketFile: File, + // Auto-reconnect with backoff after an unexpected socket drop (e.g. the + // backend process was killed and restarted). Off by default: + // NodeJSService's usage must not chase a socket it owns the lifecycle of. + private val reconnectOnDrop: Boolean = false, // Optional first so the trailing-lambda call form // `NodeJSIPC(file) { msg -> ... }` keeps binding to `onMessage` (the // last function-type parameter). Reordering after `onMessage` would @@ -37,6 +41,8 @@ class NodeJSIPC( // state observer, which the kotlinc reports as // "Argument type mismatch: actual type is 'NodeJSIPC.State', but // 'String!' was expected." in CI. + // Contract: terminal transitions (Disconnected/Error) may be delivered + // twice (collector + imperative path); observers must be idempotent. private val onConnectionStateChange: ((State) -> Unit)? = null, private val onMessage: (String) -> Unit, ) { @@ -49,6 +55,15 @@ class NodeJSIPC( private var connectJob: Job? = null private var sendChannel = Channel(Channel.UNLIMITED) + // Terminal-close flag: close() sets it before cancelling the scope, so an + // IO-loop failure racing close() can neither schedule a reconnect (flag) + // nor run one (reconnects launch in the now-cancelled scope). + @Volatile + private var closed = false + + // Incremented on every successful (re)connect; see disconnect(epoch). + private val connectionEpoch = java.util.concurrent.atomic.AtomicLong(0) + // Reusable buffers to reduce GC pressure; larger messages use temporary buffers. private val receiveLengthBuffer = ByteArray(4) private val sendLengthBuffer = ByteArray(4) @@ -65,6 +80,17 @@ class NodeJSIPC( private val state = MutableStateFlow(State.Disconnected) val connectionState: State get() = state.value + // Imperative delivery for the load-bearing terminal transitions: the state + // collector rides a conflating StateFlow, so a Disconnected that is CASed + // to Connecting by the auto-reconnect microseconds later can be dropped + // before the collector runs — and consumers hang recovery off exactly that + // emission. Duplicate delivery (collector + this) is fine per the observer + // contract above. Suppressed after close(). + private fun notifyObserver(newState: State) { + if (closed) return + onConnectionStateChange?.invoke(newState) + } + init { log("NodeJSIPC initialized with socket file: ${socketFile.absolutePath}") // Forward subsequent state transitions to the optional observer. @@ -82,7 +108,16 @@ class NodeJSIPC( connect() } - fun connect() { + fun connect() = connect( + deadlineMs = CONNECT_DEADLINE_MS, + initialIntervalMs = CONNECT_INTERVAL_MS, + maxIntervalMs = CONNECT_INTERVAL_MS, + ) + + private fun connect(deadlineMs: Long, initialIntervalMs: Long, maxIntervalMs: Long) { + if (closed) { + return + } if (state.value is State.Connected || state.value is State.Connecting) { return } @@ -108,23 +143,41 @@ class NodeJSIPC( try { socket.close() } catch (_: Exception) {} } try { - socket = connectWithRetry(socketAddress).apply { + socket = connectWithRetry( + socketAddress, + deadlineMs, + initialIntervalMs, + maxIntervalMs, + ).apply { dataOutputStream = DataOutputStream(outputStream) dataInputStream = DataInputStream(inputStream) } } catch (e: Exception) { log("Failed to connect to socket: ${e.message}") - state.value = State.Error(e) + val error = State.Error(e) + state.value = error + notifyObserver(error) return@launch } + // close() may have won while connectWithRetry was between suspension + // points; don't overwrite its terminal Disconnected with Connected. + if (closed) { + closeStreamsAndSocket() + return@launch + } + val epoch = connectionEpoch.incrementAndGet() state.value = State.Connected val receiveJob = launch { while (isActive) { try { receiveMessage() } catch (e: IOException) { - disconnect() + // break, don't retry: a second disconnect() from this + // loop can outlive the teardown+reconnect it triggers + // and would tear down the replacement connection. + disconnect(epoch) + break } } } @@ -137,7 +190,7 @@ class NodeJSIPC( sendMessageInternal(message) } catch (e: IOException) { log("Send failed, disconnecting: ${e.message}") - disconnect() + disconnect(epoch) break } } @@ -166,20 +219,66 @@ class NodeJSIPC( onMessage(buffer.decodeToString(0, messageLength)) } - fun disconnect() { - if (state.value is State.Disconnecting || state.value is State.Disconnected) { + fun disconnect() = disconnect(ANY_EPOCH) + + // `epoch` scopes the teardown to one connection: the IO loops pass the epoch + // of the connection that failed, so a disconnect that runs late — after a + // reconnect has already replaced that connection — returns instead of tearing + // down the replacement (State.Connected is a singleton, so the state CAS + // alone cannot tell two connections apart). Public disconnect() passes + // ANY_EPOCH: a deliberate teardown targets whatever connection is current. + private fun disconnect(epoch: Long) { + val observed = state.value + if (observed is State.Disconnected) { return } - sendChannel.close() + // A Disconnecting held by a STALE-epoch disconnect is transient — it + // rolls back to Connected (see the post-CAS re-read below). A drop of + // the CURRENT connection must not be swallowed by that window, or state + // wedges at Connected with dead IO loops; only bail here when this + // teardown cannot be the one the rollback would restore. + if (observed is State.Disconnecting && + (epoch == ANY_EPOCH || connectionEpoch.get() != epoch) + ) { + return + } + // Teardown is single-flight: concurrent disconnect calls (send + receive + // loops failing together) all launch jobs, but only the one that wins the + // Connected -> Disconnecting CAS tears down and runs the completion side + // effects; losers return without touching state or scheduling a reconnect. + val wonTeardown = java.util.concurrent.atomic.AtomicBoolean(false) val disconnectJob = scope.launch { while (isActive) { when (state.value) { - is State.Disconnecting, is State.Disconnected -> return@launch + is State.Disconnected -> return@launch + is State.Disconnecting -> { + // Same stale-rollback hazard as the entry guard: wait + // out a Disconnecting that may roll back to Connected + // and re-evaluate, so a current-epoch drop is never + // swallowed. Any other teardown resolves to + // Disconnected and we return on the next pass. + if (epoch != ANY_EPOCH && connectionEpoch.get() == epoch) { + state.first { it !is State.Disconnecting } + } else { + return@launch + } + } is State.Connecting -> { state.first { it is State.Connected || it is State.Error } } is State.Connected -> { + if (epoch != ANY_EPOCH && connectionEpoch.get() != epoch) return@launch if (state.compareAndSet(State.Connected, State.Disconnecting)) { + // Epoch can advance between the check above and the + // CAS (full teardown + reconnect in the gap). Once + // Disconnecting is ours the epoch is frozen, so this + // re-read is authoritative: on mismatch we captured + // the replacement connection — hand it back untouched. + if (epoch != ANY_EPOCH && connectionEpoch.get() != epoch) { + state.value = State.Connected + return@launch + } + wonTeardown.set(true) break } } @@ -189,6 +288,10 @@ class NodeJSIPC( } } } + // Close the channel only after winning the CAS: a losing (stale) + // disconnect must not close the replacement connection's channel — + // trySend on a closed channel drops messages silently. + sendChannel.close() // `shutdown` before `cancelAndJoin`: the receive loop is parked in a // blocking `readFully` that `cancelAndJoin` cannot interrupt, so without // first waking it the join blocks until the node backend sends a message @@ -200,10 +303,25 @@ class NodeJSIPC( closeStreamsAndSocket() } disconnectJob.invokeOnCompletion { cause -> - state.value = when (cause) { + if (!wonTeardown.get()) return@invokeOnCompletion + val terminal = when (cause) { null, is EOFException, is IOException, is CancellationException -> State.Disconnected else -> State.Error(cause) } + state.value = terminal + notifyObserver(terminal) + // Terminal teardown goes through close(), so a disconnect() that + // completes with `closed` unset is an unexpected drop (the IO loops' + // IOException handlers) — the auto-reconnect trigger. Backoff, not + // the 50ms cold-start cadence: a backend restart takes seconds. + if (reconnectOnDrop && !closed) { + log("Unexpected disconnect; auto-reconnecting with backoff") + connect( + deadlineMs = RECONNECT_DEADLINE_MS, + initialIntervalMs = RECONNECT_INITIAL_INTERVAL_MS, + maxIntervalMs = RECONNECT_MAX_INTERVAL_MS, + ) + } } } @@ -232,6 +350,7 @@ class NodeJSIPC( * Not reusable after close; construct a new instance. */ fun close() { + closed = true scope.cancel() // Mark terminal before shutdownSocket() wakes the receive loop's blocking // readFully: its IOException handler calls disconnect(), which then @@ -263,27 +382,46 @@ class NodeJSIPC( } } +// Explicit connect(): 50 ms cadence is invisible to TTI; the 30 s deadline +// matches the prior `waitForFile` timeout so the startup wait budget is unchanged. +private const val CONNECT_DEADLINE_MS = 30_000L +private const val CONNECT_INTERVAL_MS = 50L + +// disconnect(epoch) wildcard: tear down the current connection, whichever it is. +private const val ANY_EPOCH = -1L + +// Auto-reconnect after an unexpected drop: a killed backend process takes a few +// seconds to be restarted by the system — and a debug build or slow device can +// take over a minute to boot Node — so back off instead of burning battery on a +// tight loop, and give up (State.Error) only after a window that comfortably +// exceeds a slow restart (the FGS-kill test budgets 90 s for a debug boot). +private const val RECONNECT_DEADLINE_MS = 120_000L +private const val RECONNECT_INITIAL_INTERVAL_MS = 250L +private const val RECONNECT_MAX_INTERVAL_MS = 4_000L + /** - * Connect with a fixed-cadence retry loop bounded by an overall deadline. + * Connect with a retry loop bounded by an overall deadline. * * Retries fire on every `IOException` from `LocalSocket.connect`, which covers * both "socket file does not exist yet" (`ENOENT`) and "file exists but the * server is not yet `accept`ing" (`ECONNREFUSED`) — the same primitive handles - * both phases of backend startup. The 50 ms cadence is fast enough to be - * invisible to TTI; the 30 s deadline matches the prior `waitForFile` timeout - * so the cumulative startup wait budget is unchanged. + * both phases of backend startup, and a stale socket file left behind by a + * killed backend process. * - * No exponential backoff: this is a one-shot startup wait, not a network call, - * and the failure mode we're tolerating is "backend not finished booting yet" - * — it doesn't get worse from retrying tightly. + * The interval between attempts starts at [initialIntervalMs] and doubles up to + * [maxIntervalMs]. Cold-start callers pass equal values (fixed cadence — a + * startup wait doesn't get worse from retrying tightly); the auto-reconnect + * path passes a widening backoff because the peer needs seconds to come back. */ private suspend fun connectWithRetry( socketAddress: LocalSocketAddress, - deadlineMs: Long = 30_000, - intervalMs: Long = 50, + deadlineMs: Long, + initialIntervalMs: Long, + maxIntervalMs: Long, ): LocalSocket { var lastFailure: IOException? = null var attempts = 0 + var intervalMs = initialIntervalMs val connected = try { withTimeout(deadlineMs) { // `LocalSocket.connect` opens a real fd before it can throw @@ -303,6 +441,7 @@ private suspend fun connectWithRetry( try { candidate.close() } catch (_: Exception) {} lastFailure = e delay(intervalMs) + intervalMs = (intervalMs * 2).coerceAtMost(maxIntervalMs) } } s diff --git a/android/src/main/java/com/comapeo/core/NodeJSService.kt b/android/src/main/java/com/comapeo/core/NodeJSService.kt index 5a6813d5..45e2db95 100644 --- a/android/src/main/java/com/comapeo/core/NodeJSService.kt +++ b/android/src/main/java/com/comapeo/core/NodeJSService.kt @@ -211,6 +211,9 @@ class NodeJSService( @JvmStatic external fun initialize(dataDir: String) + @JvmStatic + external fun setEnv(name: String, value: String) + @JvmStatic external fun startNodeWithArguments(args: Array): Int @@ -353,6 +356,44 @@ class NodeJSService( startupWatchdogJob.getAndSet(null)?.cancel() } + /** + * Environment node inherits from this process. Must run before + * [startNodeWithArguments]: `NODE_COMPILE_CACHE` is read while the + * Environment is created, so setting it from JS would be too late. + * + * An Android app process has no `TMPDIR`, and there is no `/tmp` for + * `os.tmpdir()` to fall back to, so anything writing there fails with + * ENOENT. `NODE_COMPILE_CACHE` is V8's on-disk code cache; the backend + * flushes it at `ready` rather than leaving it to node's exit hook, which + * the low-memory killer routinely denies us. + * + * Both live under `cacheDir`: regenerable, and reclaimable under storage + * pressure. A variable is left unset rather than pointed at a directory we + * failed to create — node falling back to its own default beats handing it + * a path that ENOENTs on first use. + */ + private fun applyNodeEnvironment() { + // `mkdirs()` returns false when the directory already exists, so the + // result to trust is `isDirectory`, not the return value. + fun ensureDir(name: String): File? { + val dir = File(cacheDir, name) + dir.mkdirs() + if (dir.isDirectory) return dir + logCapture( + SentryCategories.BOOT, + "could not create node $name dir; leaving its env var unset", + level = "warning", + tags = mapOf("dir" to name), + ) + return null + } + + ensureDir("tmp")?.let { setEnv("TMPDIR", it.absolutePath) } + ensureDir("node-compile-cache")?.let { + setEnv("NODE_COMPILE_CACHE", it.absolutePath) + } + } + /** Positionals are read by backend/index.js; `--sentry*` flags by backend/loader.mjs. */ private fun buildBackendArgs(entryPath: String): Array { // 4th positional: default config path, or "" when the app bundled @@ -508,6 +549,8 @@ class NodeJSService( bootSpans["node-spawn"] = it } + withContext(Dispatchers.IO) { applyNodeEnvironment() } + val exitCode = startNodeWithArguments( buildBackendArgs(jsFile.absolutePath) ) @@ -619,7 +662,7 @@ class NodeJSService( applyAndEmit { it.copy(backendState = BackendState.ControlBound) } sendInitFrame() } - ControlFrame.Ready -> { + is ControlFrame.Ready -> { logCrumb(SentryCategories.CONTROL, "received: ready") applyAndEmit { it.copy(backendState = BackendState.Ready) } } diff --git a/android/src/test/java/com/comapeo/core/ControlFrameTest.kt b/android/src/test/java/com/comapeo/core/ControlFrameTest.kt index 6cb9e9c7..4ea06af8 100644 --- a/android/src/test/java/com/comapeo/core/ControlFrameTest.kt +++ b/android/src/test/java/com/comapeo/core/ControlFrameTest.kt @@ -22,8 +22,22 @@ class ControlFrameTest { } @Test - fun parsesReady() { - assertEquals(ControlFrame.Ready, ControlFrame.parse("""{"type":"ready"}""")) + fun parsesReadyWithBootNonce() { + assertEquals( + ControlFrame.Ready(bootNonce = "5a1e4a1c-0000-4000-8000-000000000000"), + ControlFrame.parse( + """{"type":"ready","bootNonce":"5a1e4a1c-0000-4000-8000-000000000000"}""" + ), + ) + } + + @Test + fun parsesReadyWithoutBootNonce() { + // Back-compat: a backend that predates the nonce sends a bare frame. + assertEquals( + ControlFrame.Ready(bootNonce = null), + ControlFrame.parse("""{"type":"ready"}"""), + ) } @Test diff --git a/apps/e2e/package-lock.json b/apps/e2e/package-lock.json index 9faad643..38cee964 100644 --- a/apps/e2e/package-lock.json +++ b/apps/e2e/package-lock.json @@ -20,7 +20,7 @@ }, "devDependencies": { "@babel/core": "7.29.0", - "@comapeo/core": "7.1.0", + "@comapeo/core": "7.2.0", "@mapeo/mock-data": "5.0.0", "@types/jasmine": "5.1.15", "@types/react": "19.2.14", @@ -1051,12 +1051,14 @@ } }, "node_modules/@comapeo/core": { - "version": "7.1.0", + "version": "7.2.0", + "resolved": "https://registry.npmjs.org/@comapeo/core/-/core-7.2.0.tgz", + "integrity": "sha512-U0grkQp+XMrcJO5rRpn2a8NBNCL0ms5NLPEFlZi8H/v47GwnaujCQdw2AnirgQjg8Nnq4+oLpL8I4ZWnkrQypg==", "dev": true, "license": "MIT", "dependencies": { "@comapeo/fallback-smp": "^1.0.0", - "@comapeo/schema": "2.2.0", + "@comapeo/schema": "2.3.0", "@digidem/types": "^2.3.0", "@fastify/error": "^3.4.1", "@fastify/type-provider-typebox": "^4.1.0", @@ -1183,13 +1185,15 @@ } }, "node_modules/@comapeo/schema": { - "version": "2.2.0", + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/@comapeo/schema/-/schema-2.3.0.tgz", + "integrity": "sha512-W39EqtKHPmnd49KbbMkTCL9YHSeM23h71Dk40BnD1z0RUQyM+HwRr3HFyEv+d3OyH/0BFbUCW0Rg9DBln53WNA==", "dev": true, "license": "MIT", "dependencies": { "@comapeo/geometry": "^1.1.1", "compact-encoding": "^2.12.0", - "protobufjs": "^7.2.5", + "protobufjs": "^7.5.5", "type-fest": "^4.26.0" } }, diff --git a/apps/e2e/package.json b/apps/e2e/package.json index 79a37206..99e4b673 100644 --- a/apps/e2e/package.json +++ b/apps/e2e/package.json @@ -22,7 +22,7 @@ }, "devDependencies": { "@babel/core": "7.29.0", - "@comapeo/core": "7.1.0", + "@comapeo/core": "7.2.0", "@mapeo/mock-data": "5.0.0", "@types/jasmine": "5.1.15", "@types/react": "19.2.14", diff --git a/apps/e2e/src/TestRunner.tsx b/apps/e2e/src/TestRunner.tsx index 17ed48a7..ed68c144 100644 --- a/apps/e2e/src/TestRunner.tsx +++ b/apps/e2e/src/TestRunner.tsx @@ -1,187 +1,341 @@ -import { useState } from 'react' +import { useState } from "react"; import jasmineRequire, { - type JasmineDoneInfo, -} from 'jasmine-core/lib/jasmine-core/jasmine' -import { Button, ScrollView, Text, View } from 'react-native' + type JasmineDoneInfo, +} from "jasmine-core/lib/jasmine-core/jasmine"; +import { Button, ScrollView, Text, View, type ErrorUtils } from "react-native"; -import { test as basicTest } from './tests/basic' -import { test as mapServerTest } from './tests/map-server' -import { test as projectCrudTest } from './tests/project-crud' +import { test as basicTest } from "./tests/basic"; +import { test as mapServerTest } from "./tests/map-server"; +import { test as projectCrudTest } from "./tests/project-crud"; type TestResult = { - id: string - name: string - passed: boolean - errors: Array<{ message: string; stack: string }> -} + id: string; + name: string; + passed: boolean; + errors: Array<{ message: string; stack: string }>; +}; type TestState = - | { status: 'idle' | 'pending'; results: Array } - | { status: 'done'; info: JasmineDoneInfo; results: Array } + | { status: "idle" | "pending"; results: Array } + | { + status: "done"; + overallStatus: JasmineDoneInfo["overallStatus"] | "timedOut"; + timedOutDuring?: string; + results: Array; + }; // Default of 5s is too short for IPC-heavy tests on slow CI devices. -const DEFAULT_TIMEOUT_INTERVAL_MS = 60_000 +const DEFAULT_TIMEOUT_INTERVAL_MS = 60_000; -export function TestRunner() { - const [testState, setTestState] = useState({ - status: 'idle', - results: [], - }) - - async function runTests() { - const jasmineCore = jasmineRequire.core(jasmineRequire) - - const jasmineEnv = jasmineCore.getEnv({ - suppressLoadErrors: true, - GlobalErrors: NoopGlobalErrors, - }) - - jasmineEnv.addReporter({ - jasmineStarted: () => { - console.log('[e2e] jasmine started') - setTestState({ status: 'pending', results: [] }) - }, - jasmineDone: (info) => { - console.log(`[e2e] jasmine done: ${info.overallStatus}`) - setTestState((prev) => { - if (prev.status === 'done') { - throw new Error( - `Invalid state transition from '${prev.status}' to 'done'.`, - ) - } - - return { - status: 'done', - info, - results: prev.results, - } - }) - }, - specStarted: (result) => { - console.log(`[e2e] spec started: ${result.fullName}`) - }, - specDone: (result) => { - const describeText = result.fullName.replaceAll(result.description, '') - - if (result.status === 'passed') { - console.log(`[e2e] PASS: ${result.fullName}`) - } else { - console.log( - `[e2e] FAIL: ${result.fullName} — ${result.failedExpectations - .map((e) => e.message) - .join(' | ')}`, - ) - for (const err of result.failedExpectations) { - if (err.stack) console.log(`[e2e] stack: ${err.stack}`) - } - } - - setTestState((prev) => { - if (prev.status === 'done') { - throw new Error( - `Invalid state transition from '${prev.status}' to 'done'.`, - ) - } - - return { - status: 'pending', - results: [ - ...prev.results, - { - id: result.id, - name: describeText - ? `${describeText} > ${result.description}` - : result.description, - passed: result.status === 'passed', - errors: result.failedExpectations.map((err) => ({ - message: err.message, - stack: err.stack, - })), - }, - ], - } - }) - }, - }) - - const { describe, it, expect, expectAsync, jasmine, beforeEach, afterEach } = - jasmineRequire.interface(jasmineCore, jasmineEnv) - - jasmine.DEFAULT_TIMEOUT_INTERVAL = DEFAULT_TIMEOUT_INTERVAL_MS - - const ctx = { - describe, - it, - expect, - expectAsync, - jasmine, - beforeEach, - afterEach, - } - - // 👇 Register tests here! - basicTest(ctx) - mapServerTest(ctx) - projectCrudTest(ctx) - - await jasmineEnv.execute() - } - - return ( - -