Skip to content

use npm list instead of custom traversal to get native module versions#70

Open
achou11 wants to merge 3 commits into
mainfrom
ac/update-native-versions-detection
Open

use npm list instead of custom traversal to get native module versions#70
achou11 wants to merge 3 commits into
mainfrom
ac/update-native-versions-detection

Conversation

@achou11
Copy link
Copy Markdown
Member

@achou11 achou11 commented May 18, 2026

Currently not an issue, but somehow had a deps tree locally such that npm list sodium-native looked like this:

└─┬ @comapeo/core@7.1.0
  ├─┬ @hyperswarm/secret-stream@6.9.1
  │ └─┬ sodium-universal@5.0.1
  │   └── sodium-native@5.1.0
  └─┬ sodium-universal@4.0.1
    └── sodium-native@4.3.3

When running npm backend:build, only sodium-native@4.3.3 was detected and built for, which caused runtime errors related to failing to load the library for sodium-native@5.1.0. I've since updated locally such that it now looks like this, which works with the existing implementation:

└─┬ @comapeo/core@7.1.0
  ├─┬ @hyperswarm/secret-stream@6.9.1
  │ ├─┬ noise-curve-ed@2.1.0
  │ │ └─┬ sodium-universal@5.0.1
  │ │   └── sodium-native@5.1.0
  │ ├─┬ noise-handshake@4.2.0
  │ │ └─┬ sodium-universal@5.0.1
  │ │   └── sodium-native@5.1.0
  │ ├─┬ sodium-secretstream@1.2.0
  │ │ └─┬ sodium-universal@5.0.1
  │ │   └── sodium-native@5.1.0
  │ └─┬ sodium-universal@5.0.1
  │   └── sodium-native@5.1.0
  └─┬ sodium-universal@4.0.1
    └── sodium-native@4.3.3

The custom fs traversal for finding versions most likely doesn't account for the "extra" layer seen with sodium-universal in the failing example. Instead of updating the existing implementation, I think it'd be safer to use npm directly via its ls command (docs). Passing the --parseable flag causes the stdout to be a newline-separated list of absolute paths to the module e.g.

npm list sodium-native --parseable
/<redacted>/backend/node_modules/sodium-native
/<redacted>/backend/node_modules/@hyperswarm/secret-stream/node_modules/sodium-native
/<redacted>/backend/node_modules/noise-curve-ed/node_modules/sodium-native
/<redacted>/backend/node_modules/noise-handshake/node_modules/sodium-native
/<redacted>/backend/node_modules/sodium-secretstream/node_modules/sodium-native

From an eye test, this approach is slightly slower than the existing approach but is less prone to error.

@achou11 achou11 requested a review from gmaclennan May 18, 2026 19:54
gmaclennan added a commit that referenced this pull request May 19, 2026
…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>
@gmaclennan
Copy link
Copy Markdown
Member

Added 835f46a just because spawning an npm ls for every module seems a lot, and we can also skip the package.json lookup. 3.27s --> 0.57s on my machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants