| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Replaces the single `NodeJSService.State` with three component states (NodeRuntime, BackendState, stopRequested) and a pure deriveState function. Closes #42. The previous single-variable model conflated three independently-stateful components, and gaps surfaced when they disagreed: - Node exits cleanly without an error frame → STOPPED (wrong if unexpected; indistinguishable from graceful shutdown). - Node crashes (non-zero exit) without an error frame → same shape. - FGS-side rootkey load fails → main-app process can't see it (Node hangs on `await initPromise`). Per-component state makes each of these directly representable and the derivation pure — easy to reason about and test. Cross-process attribution (Android): - Backend broadcasts {type:"stopping"} before graceful shutdown so natives can distinguish expected from unexpected disconnect. - FGS sends {type:"error-native",phase,message} for local failures; backend re-broadcasts as a real error frame so the main-app process gets full attribution rather than a generic "unexpected disconnect". JS surface (ComapeoState, getState(), getLastError(), stateChange event) is unchanged. ERROR remains per-instance terminal. Hard crashes (SIGSEGV, OOM kill, process.abort()) still lack in-band detail — by definition no code can run to broadcast — and surface as ERROR with phase `node-runtime-unexpected`. Detailed crash diagnostics belong in Sentry/Crashlytics, not getLastError(). Tests: new DeriveStateTests (iOS) and DeriveStateTest (Android), 9 table-driven cases each pinning every branch of the derivation. New behavioral tests for the previously-broken paths (unexpected exit → ERROR, stopping frame → STOPPING). Three existing iOS tests updated to wait for STOPPING before signaling exit — the new model correctly detects the previously-racy ordering as unexpected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Addresses code-review feedback on PR #47: - Android: in `runNode()`'s catch branch, also mark `nodeRuntime = Exited(_, REQUESTED)` so the component triple stays consistent after the thread unwinds (visible state was already correct via the backend-error rule, but the runtime field was stale). - iOS + Android: clear `lastError` / `_lastError` explicitly on the fresh-start reset. Defense-in-depth — the start guard already refuses ERROR, but explicit clear removes any chance of a stale ErrorInfo leaking across start cycles if that invariant weakens. - Android `sendErrorNativeFrame`: bound `ipcDeferred.await()` with `withTimeoutOrNull(2_000ms)`. Without this, a never-completing deferred (FGS fails before NodeJSIPC is constructed) would pin a coroutine. Logged as dropped on miss. - iOS `applyAndEmit`: document the `mutate` closure's lock discipline — runs under non-recursive NSLock; must not call any other locked method, fire onStateChange, or recurse. - Android: dedupe the 7-rule decision-order docstring on `NodeJSService.Companion.deriveState` — `LifecycleStateDerivation` is the canonical source. https://claude.ai/code/session_01V56VETR6m6sAXf5DffeBxS
Addresses code-review feedback on PR #47: - Android: in `runNode()`'s catch branch, also mark `nodeRuntime = Exited(_, REQUESTED)` so the component triple stays consistent after the thread unwinds (visible state was already correct via the backend-error rule, but the runtime field was stale). - iOS + Android: clear `lastError` / `_lastError` explicitly on the fresh-start reset. Defense-in-depth — the start guard already refuses ERROR, but explicit clear removes any chance of a stale ErrorInfo leaking across start cycles if that invariant weakens. - Android `sendErrorNativeFrame`: bound `ipcDeferred.await()` with `withTimeoutOrNull(2_000ms)`. Without this, a never-completing deferred (FGS fails before NodeJSIPC is constructed) would pin a coroutine. Logged as dropped on miss. - iOS `applyAndEmit`: document the `mutate` closure's lock discipline — runs under non-recursive NSLock; must not call any other locked method, fire onStateChange, or recurse. - Android: dedupe the 7-rule decision-order docstring on `NodeJSService.Companion.deriveState` — `LifecycleStateDerivation` is the canonical source. https://claude.ai/code/session_01V56VETR6m6sAXf5DffeBxS
There was a problem hiding this comment.
Refactors lifecycle tracking to use per-component internal states and a pure derivation function, improving correctness around unexpected Node exits and cross-process error attribution while keeping the JS-facing ComapeoState API unchanged.
Changes:
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| ios/NodeJSService.swift | Refactors iOS service to component states + derived public state; updates control-frame handling and exit classification. |
| ios/ControlFrame.swift | Adds .stopping frame parsing. |
| ios/Tests/DeriveStateTests.swift | New table-driven unit tests for iOS derivation function. |
| ios/Tests/ControlFrameTests.swift | Adds stopping-frame parse test. |
| ios/Tests/NodeJSServiceTests.swift | Adds lifecycle regression tests for unexpected exit and stopping frame; updates concurrency shutdown test sequencing. |
| ios/Tests/IPCLifecycleTests.swift | Updates stop test sequencing to wait for STOPPING before signaling exit. |
| ios/Tests/Helpers/MockBackend.swift | Mirrors backend behavior by sending stopping before closing on shutdown. |
| android/src/main/java/com/comapeo/core/NodeJSService.kt | Refactors Android FGS service to component snapshot + derived state; adds error-native send and exit classification. |
| android/src/main/java/com/comapeo/core/LifecycleStateDerivation.kt | Extracts pure derivation function for JVM unit tests (avoids JNI load). |
| android/src/main/java/com/comapeo/core/ControlFrame.kt | Adds Stopping frame parsing/type. |
| android/src/main/java/com/comapeo/core/ComapeoCoreModule.kt | Handles Stopping and improves disconnect handling to surface unexpected disconnects as ERROR. |
| android/src/test/java/com/comapeo/core/DeriveStateTest.kt | New table-driven derivation tests on JVM. |
| android/src/test/java/com/comapeo/core/ControlFrameTest.kt | Adds stopping-frame parse test. |
| backend/lib/simple-rpc.js | Adds broadcast() helper for non-replayed lifecycle frames. |
| backend/index.js | Broadcasts stopping on shutdown; adds error-native handler that funnels into fatal error path. |
| docs/ARCHITECTURE.md | Updates lifecycle model documentation to the new per-component derivation approach and protocol frames. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
- iOS runNode() early-return: when resolveJSEntryPoint() is nil, the thread is exiting and the semaphore is signaled, but nodeRuntime was left at .running — same class of inconsistency that PR #48 fixed in the Android catch branch. Set nodeRuntime = .exited(_, .requested) in the same applyAndEmit so the component triple stays coherent. Visible state is unchanged (rule 1 still wins on the backend error). - Both platforms: drop the redundant final `if exited(_, .requested) → STOPPED` line in deriveState / deriveLifecycleState. The only triple that reached it was (Exited.REQUESTED, Unknown, false) which falls through to the default STOPPED return. Decision-order docstring's rule 7 ("Otherwise → STOPPED") already describes the default — only the dead code is removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Inventory of every wait in the lifecycle path — local timeouts, caller-side bounds, and unbounded waits with their safety nets. Catalogued so a "why did this hang for N seconds" debug session doesn't require reading three codebases. Also notes the ~10-12 s worst case for Android error-native drop and the resulting degraded phase attribution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three corrections after auditing the doc against this branch: §3.1 Replay semantics — state explicitly that `stopping` and `error` are NOT replayed (the new `broadcast()` and existing `broadcastError()` are one-shot announcements paired with the natural socket close, not state-machine transitions to be replayed). §5.3 JS-visible state — note the deliberate `lastError` persistence asymmetry: cleared by `start()`, preserved by `cleanup()`. Today this is invisible to JS (which only reads `lastError` inside the ERROR `stateChange` callback), but a poller calling `getLastError()` standalone after cleanup would see the prior cycle's error. §5.5 Errors — rewrite §1 as "native-local failures" applying to both platforms (the iOS app process has the same path as the Android FGS process), with `error-native` called out as Android-only because only Android has a second process that needs cross-process attribution. Removes the misleading sentence in §2 that said iOS local failures funnel through `handleFatal` — they don't; they set `backendState=.error` directly on the iOS NodeJSService. Other docs (bare-architecture, runtime-alternatives, build-architecture-plan, libnode-size-reduction, root-key-storage-and-migration-plan, ForegroundService, README) do not reference the lifecycle state machine and need no updates from this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two end-to-end behavioural tests for previously-uncovered branches of §5.5 of ARCHITECTURE.md: testRootKeyLoadFailureDerivesError — injects a throwing rootKeyProvider; verifies sendInitFrame() catches it, sets backendState=.error(rootkey, ...) via applyAndEmit, and the derivation lands in ERROR with the thrown error's localized description as the lastError message. Mirrors on iOS what the Android FGS-side rootkey path does. testBackendErrorFrameDerivesError — drives the service to STARTED, then has MockBackend send an `error` frame post-handshake. Verifies the control-frame parse → backendState transition → ERROR derivation chain works at the integration level (not just the parser level covered by ControlFrameTests). Adds a `sendFrame(_:)` helper on MockBackend so tests can inject arbitrary post-handshake frames without further harness changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ycleStateTransitions (#71) ## Summary Fixes a race in [`testFullLifecycleStateTransitions`](ios/Tests/NodeJSServiceTests.swift#L582) that intermittently fails the macOS Swift Package Tests job ([example failure on #70](https://github.com/digidem/comapeo-core-react-native/actions/runs/26056852629/job/76606978112)): ``` NodeJSServiceTests.swift:624: error: -[ComapeoCoreTests.NodeJSServiceTests testFullLifecycleStateTransitions] : XCTAssertLessThan failed: ("3") is not less than ("2") - STOPPING should come before STOPPED ``` The recorded `transitions` array ended up `[starting, started, stopped, stopping]` rather than the expected `[starting, started, stopping, stopped]`. ## The race Same shape as PR #59, slightly different underlying mechanic. PR #59 fixed the sister test `testStopSendsShutdownMessageOverIPC`; this test was introduced in PR #47 and wasn't covered by that fix. `NodeJSService.applyAndEmit` releases the service lock *before* invoking `onStateChange?(derived)` ([NodeJSService.swift:277](ios/NodeJSService.swift#L277), then [:319](ios/NodeJSService.swift#L319)) — a deliberate requirement of [`testObserverCanReenterLockedMethodFromCallback`](ios/Tests/NodeJSServiceTests.swift#L634), which asserts the callback can re-enter locked methods like `cleanup()` without deadlocking. The state mutations are linearised under the lock, but the post-unlock observer invocations are not. The test calls `signalExit()` *before* `service.stop(timeout: 1)`: ```swift // Stop signalExit() service.stop(timeout: 1) ``` That makes the node thread's `applyAndEmit` (writing `nodeRuntime = .exited(.requested)` → derives STOPPED) race the main thread's `applyAndEmit` from inside `stop()` (writing `stopRequested = true` → derives STOPPING). Even when the locked state transitions run in the right order, the post-unlock `onStateChange` callbacks can be reordered by thread scheduling, so the recorded array sees STOPPED before STOPPING. On a quiet runner the main thread usually wins; on a loaded macOS-14-arm64 CI runner it sometimes loses. ## The fix Identical pattern to PR #59: dispatch `stop()` to a background queue, register a STOPPING expectation, and `wait(for: [stoppingExpectation])` before calling `signalExit()`. That pins the observer ordering — STOPPING is appended to `transitions` before the node thread is ever unblocked, so the later STOPPED append from the node thread necessarily lands at a higher index. ```swift let stopFinished = expectation(description: "stop() returned") DispatchQueue.global().async { service.stop(timeout: 1) stopFinished.fulfill() } wait(for: [stoppingExpectation], timeout: 5) signalExit() wait(for: [stopFinished], timeout: 5) ``` ## Test plan - [ ] `swift test --filter NodeJSServiceTests/testFullLifecycleStateTransitions` × 50 — all pass - [ ] `swift test` (full suite) — clean (I'm on a Linux container so can't run `swift test` here; verifying via CI on this PR.) https://claude.ai/code/session_01BRgG9bgdFy9pZCov2uHyef --- _Generated by [Claude Code](https://claude.ai/code/session_01BRgG9bgdFy9pZCov2uHyef)_ Co-authored-by: Claude <noreply@anthropic.com>
## Optic Release Automation This **draft** PR is opened by Github action [optic-release-automation-action](https://github.com/nearform-actions/optic-release-automation-action). A new **draft** GitHub release [v1.0.0-pre.2](https://github.com/digidem/comapeo-core-react-native/releases/tag/untagged-c499977757c9745e56b2) has been created. Release author: @gmaclennan #### If you want to go ahead with the release, please merge this PR. When you merge: - The GitHub release will be published - The npm package with tag pre will be published according to the publishing rules you have configured - No major or minor tags will be updated as configured #### If you close the PR - The new draft release will be deleted and nothing will change ## What's Changed * Android Testing Infrastructure & Bug Fixes by @gmaclennan in #3 * chore: prebuild example/android; harden instrumented tests by @gmaclennan in #10 * Integrate @comapeo/core via IPC over Unix sockets by @gmaclennan in #5 * chore: adjust repo setup by @achou11 in #12 * chore: minor fixes based on expo-doctor by @achou11 in #13 * Add iOS support & test infrastructure by @gmaclennan in #6 * chore: add architecture docs & plans by @gmaclennan in #11 * update some native deps used in backend by @achou11 in #14 * iOS Phase 1: unified JS bundle + smoke test (simulator-only) by @gmaclennan in #15 * iOS Phase 2: xcframework Embed & Sign for native addons by @gmaclennan in #16 * Phase 2 Android: jniLibs packaging + unified rollup loader plugin by @gmaclennan in #17 * chore: post-Phase-2 cleanup — comments, plan docs, agents.md by @gmaclennan in #33 * android: read abiFilters from reactNativeArchitectures (#30) by @gmaclennan in #35 * refactor: simplify build-backend.ts; rollup writes directly to native asset trees by @gmaclennan in #34 * chore: fix eslint configuration by @achou11 in #41 * android: audit 16 KB page alignment on every shipped .so by @gmaclennan in #43 * Add rootkey persistence and lifecycle state management by @gmaclennan in #36 * chore: move example app into apps directory by @achou11 in #18 * refactor: per-component lifecycle state with derived ComapeoState by @gmaclennan in #47 * android: fold waitForFile into connect retry loop by @gmaclennan in #52 * chore: add e2e testing app by @achou11 in #49 * fix(android): drop setUnlockedDeviceRequired from rootkey wrapper key by @gmaclennan in #57 * fix(backend): cache stopping/error frames for late joiners by @gmaclennan in #58 * fix(ios-tests): wait for STOPPING before signalling node exit by @gmaclennan in #59 * fix(android): drain JNI stdio pumps before returning from node::Start by @gmaclennan in #60 * Sentry integration: Phase 1 + Phase 2a + Phase 2b by @gmaclennan in #54 * feat(backend): polywasm-backed undici on iOS, re-enable maps plugin by @gmaclennan in #62 * ci: drop unreliable Android emulator snapshot caching by @gmaclennan in #64 * feat(sentry): land Phase 3 — backend loader + RPC tracing by @gmaclennan in #63 * fix(ios-tests): serialise STOPPING/STOPPED observers in testFullLifecycleStateTransitions by @gmaclennan in #71 * use npm list instead of custom traversal to get native module versions by @achou11 in #70 * feat(sentry): land Phases 6 + 7a — Android exit reasons & iOS MetricKit app-exit telemetry by @gmaclennan in #72 * fix(sentry): make exit telemetry lossless and stop cross-process clobbering by @gmaclennan in #84 * chore(e2e): add e2e tests on browserstack via Maestro by @achou11 in #56 * feat(sentry): migrate to @sentry/react-native v8; exit telemetry as Application Metrics by @gmaclennan in #73 * Map server integration by @gmaclennan in #86 * chore(deps): upgrade to Expo SDK 56 (React Native 0.85) by @gmaclennan in #87 * chore(ci): add release workflow by @gmaclennan in #90 * chore: fix npm script and release build script by @gmaclennan in #91 * chore(pack): don't try to package build files by @gmaclennan in #92 * fix: start fastify listening by @gmaclennan in #93 * perf(backend): switch bundler from rollup to rolldown by @gmaclennan in #94 * fix(ci): ignore-scripts in ios npm installs by @gmaclennan in #96 * fix(ci): replace --ignore-scripts with npm strict-allow-scripts allowlist by @gmaclennan in #106 * feat(config): let the consuming app supply the default project config by @gmaclennan in #95 * chore(release): merge prerelease branch. by @gmaclennan in #110 ## New Contributors * @achou11 made their first contribution in #12 **Full Changelog**: https://github.com/digidem/comapeo-core-react-native/commits/v1.0.0-pre.2 <!-- <release-meta>{"id":342868678,"version":"v1.0.0-pre.2","npmTag":"pre","opticUrl":"https://optic-zf3votdk5a-ew.a.run.app/api/generate/"}</release-meta> -->
## Optic Release Automation This **draft** PR is opened by Github action [optic-release-automation-action](https://github.com/nearform-actions/optic-release-automation-action). A new **draft** GitHub release [v1.0.0-pre.2](https://github.com/digidem/comapeo-core-react-native/releases/tag/untagged-352a6c41c12fd02dec37) has been created. Release author: @gmaclennan #### If you want to go ahead with the release, please merge this PR. When you merge: - The GitHub release will be published - The npm package with tag pre will be published according to the publishing rules you have configured - No major or minor tags will be updated as configured #### If you close the PR - The new draft release will be deleted and nothing will change <!-- Release notes generated using configuration in .github/release.yml at 7fe80b4 --> ## What's Changed ### 🚀 Features * Integrate @comapeo/core via IPC over Unix sockets by @gmaclennan in #5 * Add iOS support & test infrastructure by @gmaclennan in #6 * iOS Phase 1: unified JS bundle + smoke test (simulator-only) by @gmaclennan in #15 * iOS Phase 2: xcframework Embed & Sign for native addons by @gmaclennan in #16 * Phase 2 Android: jniLibs packaging + unified rollup loader plugin by @gmaclennan in #17 * android: read abiFilters from reactNativeArchitectures (#30) by @gmaclennan in #35 * Add rootkey persistence and lifecycle state management by @gmaclennan in #36 * Sentry integration: Phase 1 + Phase 2a + Phase 2b by @gmaclennan in #54 * feat(backend): polywasm-backed undici on iOS, re-enable maps plugin by @gmaclennan in #62 * feat(sentry): land Phase 3 — backend loader + RPC tracing by @gmaclennan in #63 * feat(sentry): land Phases 6 + 7a — Android exit reasons & iOS MetricKit app-exit telemetry by @gmaclennan in #72 * feat(sentry): migrate to @sentry/react-native v8; exit telemetry as Application Metrics by @gmaclennan in #73 * Map server integration by @gmaclennan in #86 * feat(config): let the consuming app supply the default project config by @gmaclennan in #95 ### 🐛 Bug Fixes * fix(android): drop setUnlockedDeviceRequired from rootkey wrapper key by @gmaclennan in #57 * fix(backend): cache stopping/error frames for late joiners by @gmaclennan in #58 * fix(ios-tests): wait for STOPPING before signalling node exit by @gmaclennan in #59 * fix(android): drain JNI stdio pumps before returning from node::Start by @gmaclennan in #60 * fix(ios-tests): serialise STOPPING/STOPPED observers in testFullLifecycleStateTransitions by @gmaclennan in #71 * fix(sentry): make exit telemetry lossless and stop cross-process clobbering by @gmaclennan in #84 * fix: start fastify listening by @gmaclennan in #93 * fix(ci): ignore-scripts in ios npm installs by @gmaclennan in #96 * fix(ci): replace --ignore-scripts with npm strict-allow-scripts allowlist by @gmaclennan in #106 * fix(release): stop `npm pack --dry-run` leaking dry-run into backend install by @gmaclennan in #129 ### ⚡ Performance * perf(backend): switch bundler from rollup to rolldown by @gmaclennan in #94 ### ⬆️ Dependencies * update some native deps used in backend by @achou11 in #14 * chore(deps): upgrade to Expo SDK 56 (React Native 0.85) by @gmaclennan in #87 ### 🏗️ Maintenance * Android Testing Infrastructure & Bug Fixes by @gmaclennan in #3 * chore: prebuild example/android; harden instrumented tests by @gmaclennan in #10 * chore: adjust repo setup by @achou11 in #12 * chore: minor fixes based on expo-doctor by @achou11 in #13 * chore: add architecture docs & plans by @gmaclennan in #11 * chore: post-Phase-2 cleanup — comments, plan docs, agents.md by @gmaclennan in #33 * refactor: simplify build-backend.ts; rollup writes directly to native asset trees by @gmaclennan in #34 * chore: fix eslint configuration by @achou11 in #41 * android: audit 16 KB page alignment on every shipped .so by @gmaclennan in #43 * chore: move example app into apps directory by @achou11 in #18 * refactor: per-component lifecycle state with derived ComapeoState by @gmaclennan in #47 * android: fold waitForFile into connect retry loop by @gmaclennan in #52 * chore: add e2e testing app by @achou11 in #49 * ci: drop unreliable Android emulator snapshot caching by @gmaclennan in #64 * use npm list instead of custom traversal to get native module versions by @achou11 in #70 * chore(e2e): add e2e tests on browserstack via Maestro by @achou11 in #56 * chore(ci): add release workflow by @gmaclennan in #90 * chore: fix npm script and release build script by @gmaclennan in #91 * chore(pack): don't try to package build files by @gmaclennan in #92 * chore(release): merge prerelease branch. by @gmaclennan in #110 * ci(e2e): retry BrowserStack builds on infra-class flakes by @gmaclennan in #113 ### Other Changes * ci: derive changelog labels from PR titles + add Dependabot by @gmaclennan in #114 ## New Contributors * @achou11 made their first contribution in #12 * @optic-release-automation[bot] made their first contribution in #112 **Full Changelog**: https://github.com/digidem/comapeo-core-react-native/commits/v1.0.0-pre.2 <!-- <release-meta>{"id":342970724,"version":"v1.0.0-pre.2","npmTag":"pre","opticUrl":"https://optic-zf3votdk5a-ew.a.run.app/api/generate/"}</release-meta> -->
| Back | FazBrowse Home | New Git URL |
Closes #42.
Summary
Replaces the single NodeJSService.State enum with three independently-stateful components and a pure deriveState function. The JS surface (ComapeoState, getState(), getLastError(), stateChange event payload) is unchanged.
The problem
The previous single-variable model conflated three independent concerns. Gaps surfaced when they disagreed:
The model
Each component has its own state; ComapeoState is derived as a pure function of the triple:
NodeRuntime ∈ { NotRunning, Running, Exited(code, reason) } reason ∈ { Requested, Unexpected } BackendState ∈ { Unknown, ControlBound, Ready, Stopping, Error{phase, message} } stopRequested ∈ { false, true }deriveState is a 7-step decision tree, identical in spirit on both platforms. The previously-broken paths now resolve correctly:
What changed
Tests
Notes for review
Test plan
🤖 Generated with Claude Code