Skip to content

feat: HMR dev-sessions, ESM resolver hardening, dev-mode runtime globals#383

Open
NathanWalker wants to merge 14 commits into
mainfrom
feat/hmr-dev-sessions
Open

feat: HMR dev-sessions, ESM resolver hardening, dev-mode runtime globals#383
NathanWalker wants to merge 14 commits into
mainfrom
feat/hmr-dev-sessions

Conversation

@NathanWalker

@NathanWalker NathanWalker commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Adds robust Hot Module Replacement plus the supporting ESM resolver hardening and dev-session globals that make hot reload viable on iOS.

  • import.meta.hot: data, accept, dispose, prune, decline, invalidate, on/off/send event surface.
  • Dev-session globals (__nsStartDevSession, __nsReloadDevApp, __nsInvalidateModules, __nsRunHmrDispose, __nsRunHmrPrune, __nsKickstartHmrPrefetch, __nsGetLoadedModuleUrls, __nsApplyStyleUpdate, __nsConfigureDevRuntime, __nsTerminateAllWorkers).
  • Speculative HTTP module prefetch with canonical-key normalization so __ns_hmr__/v<N> and __ns_boot__/b<N> tag prefixes share hot.data identity across reload cycles.
  • ESM resolver hardening in ModuleInternalCallbacks.mm to:
    • Handle HTTP/HTTPS module URLs end-to-end (resolution, fetch, canonical-key collapse, dynamic import).
    • Compile .json imports into synthetic ES modules.

Summary by CodeRabbit

  • New Features

    • Added broader HMR and dev-session support, including hot data handling, prune/decline hooks, and module invalidation.
    • Introduced improved URL/blob handling for in-app modules and URL.createObjectURL/revokeObjectURL.
  • Bug Fixes

    • Made remote module allowlist matching stricter and more reliable.
    • Improved runtime/module error reporting so failures surface with clearer messages.
    • Reduced test and server teardown deadlocks by making socket and shutdown handling more resilient.
  • Tests

    • Expanded coverage for HTTP ESM loading, import maps, blob modules, HMR state, and quarantined specs.

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds HTTP ESM loading and a comprehensive HMR/dev-session system to the NativeScript iOS runtime (per-isolate module registry, import-map, blob URL polyfill, worker termination). CI is upgraded to macOS 15/Xcode 26 with a hung-app sampler and diagnostics collection; the Embassy test server is hardened against fatal errors; and new tests cover HMR hot.data, import-map resolution, blob caching, and URL canonicalization.

Changes

Runtime: HMR & HTTP module system

Layer / File(s) Summary
HTTP dev flags & URL allowlist security
NativeScript/runtime/DevFlags.h, NativeScript/runtime/DevFlags.mm
Declares and implements IsHttpModulePrefetchEnabled/IsHttpFetchUrlLogEnabled config flags with std::once_flag caching; replaces simple prefix-based allowlist matching with boundary-aware RemoteUrlMatchesAllowlistEntry.
HMR support API surface expansion
NativeScript/runtime/HMRSupport.h
Expands the header with DevSessionState struct, prune/event/dispose/decline callback APIs, HTTP prefetch cache management, V8 promise/property utilities, HMR global cleanup, and InitializeHmrDevGlobals consolidated entry point.
Per-isolate module registry & import-map API
NativeScript/runtime/ModuleInternalCallbacks.h
Replaces g_moduleRegistry global with ModuleRegistryFor(isolate); adds DestroyModuleStateForIsolate, LoadHttpModuleForUrl, InvalidateModules, GetLoadedModuleUrls, SetImportMap, SetVolatilePatterns, and CleanupImportMapGlobals.
RunModule signature & Runtime teardown/init
NativeScript/runtime/Runtime.h, NativeScript/runtime/Runtime.mm, NativeScript/runtime/ModuleInternal.h
Changes Runtime::RunModule and ModuleInternal::RunModule to return bool with optional outErrorMessage; rewrites destructor to use DestroyModuleStateForIsolate gating HMR/import-map cleanup to the main isolate; adds scheme-aware import.meta.url/import.meta.dirname; wires InitializeHmrDevGlobals and URLImpl::InstallBlobMethods.
HTTP ESM loading & error-handling overhaul
NativeScript/runtime/ModuleInternal.mm
Adds HTTP path helpers and local jsErrorOccurred; reworks RunModule fast path for HTTP ESM; overhauls CJS require() error handling diverging debug/worker behavior; refactors LoadESModule compilation, instantiation, and evaluation with HTTP vs local branching; rewrites TLA promise handling with HTTP async pumping and configurable timeouts.
Blob URL polyfill & HMR worker termination
NativeScript/runtime/URLImpl.h, NativeScript/runtime/URLImpl.cpp, NativeScript/runtime/Worker.h, NativeScript/runtime/Worker.mm
Adds URLImpl::InstallBlobMethods injecting URL.createObjectURL/revokeObjectURL and a synchronized URLSearchParams getter; adds Worker::TerminateAllWorkersCallback registered as __nsTerminateAllWorkers on the main isolate for HMR shutdown.

CI, test harness & test coverage

Layer / File(s) Summary
CI workflow: macOS 15, Xcode 26, diagnostics
.github/scripts/sample-hung-app.sh, .github/workflows/npm_release.yml
Pins Test job to macos-15/Xcode 26 targeting iPhone 16 Pro; adds a background hung-app sampler with trap-based cleanup; collects crash logs and simulator unified logs on failure; uploads test-diagnostics artifact.
Embassy server: remove fatal errors & bounded shutdown
TestRunnerTests/Embassy/DefaultHTTPServer.swift, TestRunnerTests/Embassy/TCPSocket.swift, TestRunnerTests/Embassy/Transport.swift
Replaces fatalError/assert in Transport read/write and TCPSocket.ignoreSigPipe with graceful closedByPeer() teardown; makes DefaultHTTPServer.handleNewConnection() fault-tolerant on accept()/getPeerName() failures; bounds stopAndWait() to 10 seconds.
Test runner quarantine, progress tracking & server routes
TestRunnerTests/TestRunnerTests.swift, TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js, TestRunnerTests/QUARANTINED_TESTS.md
Adds progressLock/lastSpecSeen to track the last active Jasmine spec; expands the embedded HTTP server with /progress, ESM module, timeout-delayed module, and hot.data routes; adds jasmine.fail() and a progress-beacon reporter; introduces QUARANTINED_SPEC_SUBSTRINGS spec filter; documents TNS Workers and HTTP ESM Loader quarantines.
HMR hot.data test modules
TestRunner/app/tests/esm/hmr/hot-data-ext.js, TestRunner/app/tests/esm/hmr/hot-data-ext.mjs
Adds .js/.mjs twin test modules exporting getHot, getHotData, setHotValue, getHotValue, and testHotApi for verifying import.meta.hot.data behavior across module aliases.
HTTP ESM, import-map, blob & security test coverage
TestRunner/app/tests/HttpEsmLoaderTests.js, TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs, TestRunner/app/tests/RemoteModuleSecurityTests.js, TestRunner/app/tests/MethodCallsTests.js
Adds HMR hot.data integration tests and URL canonicalization tests; adds import-map vendor resolution and blob-URL module caching tests; switches RemoteModuleSecurityTests to dead local ports; updates optional-property assertions in MethodCallsTests.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

  • NativeScript/ios#375: Modifies ModuleInternal.mm debug error-handling and ES module/require failure propagation in the same code paths this PR overhauls.

Poem

🐇 Hop hop, the modules now speak HTTP,
Each isolate holds its own registry neat.
Hot.data persists through the HMR dance,
Dead ports replace hosts—no external chance.
The Embassy server won't crash on a whim,
And sampler scripts catch when the runner goes dim!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.16% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: HMR dev-sessions, ESM resolver hardening, and dev-mode runtime globals.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

globalTemplate->Set(urlPropertyName, URLTemplate);
}

void URLImpl::InstallBlobMethods(v8::Local<v8::Context> context) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blob can likely be split into separate PR. Not directly attributable to this pull request (at a time, I used blob urls with hmr updates but found standard http networking of es modules to suffice fine) - but useful on it's own.

@NathanWalker NathanWalker force-pushed the feat/hmr-dev-sessions branch from 8310eac to 2c5d877 Compare June 17, 2026 00:01
@NathanWalker NathanWalker force-pushed the feat/hmr-dev-sessions branch 2 times, most recently from 6dfbacd to f7cdfcc Compare June 26, 2026 17:56
@edusperoni

Copy link
Copy Markdown
Collaborator

I'm not sure I understand the goal of these "dev sessions"? What do these need specifically from the runtime that's not an "user land" thing?

@NathanWalker

Copy link
Copy Markdown
Contributor Author

I'm not sure I understand the goal of these "dev sessions"? What do these need specifically from the runtime that's not an "user land" thing?

Yeah good question @edusperoni, the "dev session" naming here probably over (or mis) characterizes things.

The "session" part is just the contract for booting from a dev server over HTTP instead of the on-disk bundle. It does three things a one-shot bundle loader doesnt: point resolution at an HTTP origin + install the import map before the first import, give a re-entrant boot (import client > import entry) that can re-run for a full reload without relaunching the process, and bubble import failures back as a rejected promise so the client can show an overlay instead of the app just dying.

Could it be userland? I think the thing that trips people up (tripped me up too) is that on the web HMR is userland because the browser is the runtime; it already ships a spec ESM loader that fetches over HTTP and a host-owned module map you poke at by varying the URL. Vite's client gets to be "just JS" because it sits on top of that. Here V8 is embedded by us, and bare V8 ships no loader at all; every piece of it is an embedder host callback only native can install. So the litmus test is pretty clean: anything that has to install/drive a V8 host callback or mutate V8's module map cant be userland, everything else stays in JS.

This may help expand a few things:

  • The HTTP ESM loader, import 'http://...', is resolved inside ResolveModuleCallback / the dynamic-import callback, before any user code for that module even runs. And in V8 10.3.22 that static callback is synchronous, which is the whole reason the prefetch engine exists (issue with V8's sync walk from JS, and a serial network walk on the UI thread trips the launch watchdog; an iOS issue I sort of knew about long ago so was interesting to be reminded of here).
  • Identity-preserving eviction (__nsInvalidateModules), the registry is host-owned (g_moduleRegistry) and theres no JS API to evict/re-instantiate a compiled record. This is the most NS-specific one: the web just mints a new identity per save with ?t=, but we cant, because module identity is load-bearing for native interop - mint a fresh realm for a @nativescript/core module and the native patches collide leading to Cannot redefine property. So the runtime has to own eviction that collapses back to one canonical identity.
  • import.meta.hot: import.meta only gets populated in SetHostInitializeImportMetaObjectCallback, and hot.data only survives a swap if it's keyed to the runtime's canonical module key, which userland doesnt have.
  • Reboot teardown: terminating native-owned workers + ordered v8::Global/thread-local cleanup before isolate disposal.
    Everything protocol-ish I deliberately left in JS: the WebSocket, the wire format, evictPaths/closure computation, accept/dispose policy; all in @nativescript/vite's client. Runtime exposes mechanism, client owns policy; nothing links or version-pins vite, and unknown URLs just fall through to a generic HTTP loader.

So really these globals arent "a dev-session feature" so much as the embedder half of a spec ESM loader + an identity-preserving module map. The part the browser hands Vite for free. The two that could move to JS if we want a smaller surface are __nsApplyStyleUpdate (just Application.addCss + restyle) and __nsGetLoadedModuleUrls (introspection). Lmk if you see the boundary differently.

Adds the Hot Module Replacement runtime layer plus the supporting ESM resolver hardening and dev-session globals that make hot reload viable on iOS.

* `import.meta.hot`: `data`, `accept`, `dispose`, `prune`,
  `decline`, `invalidate`, `on`/`off`/`send` event surface.
* Dev-session globals (`__nsStartDevSession`, `__nsReloadDevApp`,
  `__nsInvalidateModules`, `__nsRunHmrDispose`, `__nsRunHmrPrune`,
  `__nsKickstartHmrPrefetch`, `__nsGetLoadedModuleUrls`,
  `__nsApplyStyleUpdate`, `__nsConfigureDevRuntime`,
  `__nsTerminateAllWorkers`).
* Speculative HTTP module prefetch with canonical-key normalization so
  `__ns_hmr__/v<N>` and `__ns_boot__/b<N>` tag prefixes share `hot.data`
  identity across reload cycles.
* ESM resolver hardening in `ModuleInternalCallbacks.mm` to:
  - Preserve synthetic-namespace identity (`ns-vendor://`,
    `optional:`, `node:`, `blob:`) — these are NOT filesystem paths.
  - Handle HTTP/HTTPS module URLs end-to-end (resolution, fetch,
    canonical-key collapse, dynamic import).
  - Compile `.json` imports into synthetic ES modules.
* `NodeBuiltinsAndOptionalModulesTests.mjs`, `HttpEsmLoaderTests.js`,
  `hot-data-ext.{js,mjs}` test fixtures, plus integration wiring in
  `TestRunnerTests.swift` and the Jasmine boot harness.
In debug builds the module loader swallows compile/require errors (CompileScript returns an empty script; RunModule logs and returns success) so a bad HMR edit doesn't abort the main app. That also swallowed a *worker's* entry-script error, so `worker.onerror` never fired (e.g. a worker loaded from a syntactically
invalid script hung the spec until the Jasmine async timeout).

Gate the debug swallow on `!isWorker`: worker isolates now propagate the error
(as release already does) and keep the V8 exception pending so the worker entry's TryCatch routes it to `worker.onerror`. Main-isolate HMR behavior is unchanged.
…harden test server

Quarantine (harness-level specFilter, no submodule edit; see
TestRunnerTests/QUARANTINED_TESTS.md):
- "HMR hot.data" + "URL Key Canonicalization" (8 specs): the in-runner Embassy test server can't answer the runtime's synchronous NSURLConnection GET (getPeerName EINVAL / no response delivered). The HMR loader itself works; this is a test-harness limitation, documented for re-enable.

Test-server robustness (kept; also pre-stages the un-quarantine):
- DefaultHTTPServer.handleNewConnection: tolerate getPeerName() failure and serve with a placeholder peer instead of crashing (fixes the DefaultHTTPServer.swift:87 EXC_BREAKPOINT) or dropping the connection.
- /esm/timeout.mjs: respond via non-blocking loop.call(withDelay:) instead of
  Thread.sleep, which wedged the single-threaded event loop.
- Serve the /ns/m/... hot-data aliases and /ns/core bridge endpoints.

[skip ci]
@NathanWalker NathanWalker force-pushed the feat/hmr-dev-sessions branch from d24c897 to 4289539 Compare June 27, 2026 20:12
…ocal

The module maps (registry / fallback / fallbackByRelative /
vendor) were thread_local, which is only correct because each isolate is
currently pinned to a single thread. Key them by v8::Isolate* instead so
they follow the owning isolate even if it's ever entered from another
thread under v8::Locker per @edusperoni feedback.

Per-isolate state now lives in a mutex-guarded side table and is torn down
from ~Runtime via DestroyModuleStateForIsolate() while the isolate is still
alive. This retires the leaky-pointer hack and also frees worker isolates'
fallback/vendor maps, which were previously leaked (only the main isolate's
were cleared via CleanupImportMapGlobals).
Behavior-preserving: each call site binds a same-named local alias
(auto& g_moduleRegistry = ModuleRegistryFor(isolate)), so resolver/loader
bodies are unchanged. The two standalone helpers (RemoveModuleFromRegistry,
GetLoadedModuleUrls) resolve the current isolate internally.
@NathanWalker NathanWalker marked this pull request as ready for review June 29, 2026 21:39

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
NativeScript/runtime/Runtime.mm (1)

445-458: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Honor the new RunModule failure contract in main startup.

Line 450 still discards the boolean result. Since ModuleInternal::RunModule now reports some failures by returning false without throwing, startup can continue after the main module failed.

Proposed fix
 void Runtime::RunMainScript() {
   Isolate* isolate = this->GetIsolate();
   v8::Locker locker(isolate);
   Isolate::Scope isolate_scope(isolate);
   HandleScope handle_scope(isolate);
-  this->moduleInternal_->RunModule(isolate, "./");
+  std::string err;
+  if (!this->moduleInternal_->RunModule(isolate, "./", &err)) {
+    throw NativeScriptException(
+        isolate,
+        err.empty() ? "Failed to run main module" : err,
+        "Error");
+  }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/Runtime.mm` around lines 445 - 458,
Runtime::RunMainScript currently ignores the boolean result from
ModuleInternal::RunModule, so startup can continue even when main module loading
fails without throwing. Update RunMainScript to use the same failure contract as
Runtime::RunModule by capturing the return value from moduleInternal_->RunModule
and handling a false result as a startup failure, using the existing Runtime and
ModuleInternal::RunModule symbols to locate the change.
🧹 Nitpick comments (4)
NativeScript/runtime/ModuleInternalCallbacks.h (1)

44-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Keep loaded-module introspection isolate-explicit.

Now that the registry is keyed by v8::Isolate*, GetLoadedModuleUrls() should take the target isolate like the other registry APIs. Relying on an implicit current isolate makes worker/main diagnostics easier to mix up.

Suggested API adjustment
-std::vector<std::string> GetLoadedModuleUrls();
+std::vector<std::string> GetLoadedModuleUrls(v8::Isolate* isolate);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/ModuleInternalCallbacks.h` around lines 44 - 45,
GetLoadedModuleUrls() is still using an implicit current isolate, which can mix
up worker and main-thread diagnostics now that the module registry is
isolate-keyed. Update the ModuleInternalCallbacks API so GetLoadedModuleUrls
takes a v8::Isolate* parameter, and propagate that isolate through the
implementation and any call sites to match the other registry helpers. Use the
existing registry symbols in ModuleInternalCallbacks to keep the diagnostics
explicitly scoped to the target isolate.
.github/workflows/npm_release.yml (1)

263-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Bound the failure diagnostics payload.

Copying the full CoreSimulator log tree plus an unrestricted log collect can make failed CI runs slow or produce oversized artifacts. Prefer a targeted logarchive window and avoid uploading the whole CoreSimulator directory.

Suggested tightening
           # Simulator app crashes land in the host's DiagnosticReports.
           cp -R ~/Library/Logs/DiagnosticReports/. "$DIAG/DiagnosticReports/" 2>/dev/null || true
-          cp -R ~/Library/Logs/CoreSimulator/. "$DIAG/CoreSimulator/" 2>/dev/null || true
+          # Avoid uploading the full CoreSimulator log tree; the targeted
+          # logarchive below contains the simulator logs needed for this run.
@@
-            xcrun simctl spawn "$UDID" log collect --output "$DIAG/simulator.logarchive" 2>/dev/null || true
+            xcrun simctl spawn "$UDID" log collect --last 45m --output "$DIAG/simulator.logarchive" 2>/dev/null || true

Please verify the log collect --last option on the macOS 15/Xcode 26 runner image.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/npm_release.yml around lines 263 - 279, The failure
diagnostics step is too broad: it copies the entire CoreSimulator log tree and
runs an unbounded log collect, which can create oversized artifacts and slow CI.
In the diagnostics block that uses DIAG, xcrun simctl spawn, and log collect,
stop archiving the full CoreSimulator directory and switch to a targeted
unified-log collection window using the macOS 15/Xcode 26-supported log collect
--last option so only recent logs are captured.
NativeScript/runtime/Runtime.mm (1)

252-253: 🩺 Stability & Availability | 🔵 Trivial

Track the worker queue race TODO.

This TODO names a possible worker queue leak during termination ordering. Please track it before merge or file a follow-up so it does not get lost. I can help draft the issue or a fix plan.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/Runtime.mm` around lines 252 - 253, The TODO in
Runtime.mm about a possible worker queue leak/race during termination ordering
needs to be tracked before merge. Follow up on the worker lifecycle path in the
Runtime-related initialization/termination flow, especially around the queue
handling and the Terminate-before-Initialize scenario, and either replace the
TODO with a concrete fix or create a tracked issue/fix plan linked to the worker
queue race so it is not lost.
NativeScript/runtime/URLImpl.cpp (1)

59-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Keep searchParams synchronized after url.search changes.

The getter returns the cached _searchParams forever. If code reads url.searchParams, then later assigns url.search, subsequent url.searchParams reads still expose the old query.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/URLImpl.cpp` around lines 59 - 90, The
URLImpl::searchParams getter caches a single _searchParams instance and never
refreshes it when url.search changes, so later reads can return stale query
data. Update the URL.prototype.searchParams handling in URLImpl.cpp so the
cached URLSearchParams is invalidated or resynced whenever the search setter
runs, and make sure the getter recreates/updates the instance from the current
search string before returning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@NativeScript/runtime/DevFlags.mm`:
- Around line 95-103: The allowlist check in RemoteUrlMatchesAllowlistEntry is
matching raw URL prefixes too early, which lets path-scoped entries be bypassed
with dot-segment paths. Update the matching logic to canonicalize or normalize
the URL path before applying the prefix/boundary rules, or explicitly reject
plain/encoded dot segments in DevFlags.mm so a trailing-slash allowlist entry
cannot match escaped paths.

In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 236-264: The debug handling in ModuleInternal.mm is swallowing
non-HTTP ES module failures by returning true or an empty namespace, which
prevents worker error propagation for .mjs loads. Update the
NativeScriptException catch and the moduleNamespace.IsEmpty path so debug mode
still surfaces failures to the caller for worker-loaded ESM, instead of always
pretending success; keep the existing HTTP debug logging, but ensure the
Worker.mm TryCatch can observe the error for the ES module load path.

In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 94-100: The install script execution in URLImpl should not fail
silently: the current Compile/Run flow can leave blob URL support partially
initialized and a pending V8 exception uncleared. Update the script path in
URLImpl to wrap the v8::Script::Compile and script->Run calls in a v8::TryCatch,
then explicitly handle both compile and runtime failures by logging the error
and propagating it (or throwing) instead of ignoring the result. Use the
existing blob_methods script setup in URLImpl as the place to add this error
handling.

In `@NativeScript/runtime/Worker.mm`:
- Around line 486-499: The HMR termination loop in Worker::TerminateWorkers
currently iterates all entries from Caches::Workers, which can affect workers
from other runtimes. Update this callback to filter worker wrappers by the
current main isolate, matching the existing Runtime::~Runtime() behavior via
WorkerWrapper::GetMainIsolate(). Keep the existing running/closing checks and
only call WorkerWrapper::Terminate() for workers belonging to the same isolate.

In `@TestRunner/app/tests/esm/hmr/hot-data-ext.js`:
- Around line 51-73: The hot-data fixture is mutating shared HMR state by
invoking hot.accept, hot.dispose, hot.decline, and hot.invalidate in the shared
test helper, which makes later specs order-dependent. Update hot-data-ext.js so
the shared fixture only checks for the presence of HMR APIs and data on hot, and
remove lifecycle/callback registration from this path. If coverage for those
methods is needed, move it into a separate throwaway module or dedicated test
helper that is not reused across specs.

In `@TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs`:
- Around line 55-78: The test cleanup in the __ns_test_vendor__ import-map spec
is not restoring the runtime’s prior import-map state, which can leak
configuration into later specs. Snapshot the existing import-map before calling
configureRuntime in this test, then in the finally block restore that original
import-map instead of resetting to an empty imports object. Keep the existing
__nsVendorRegistry restore logic intact so the test remains hermetic.

In `@TestRunnerTests/Embassy/TCPSocket.swift`:
- Around line 60-66: The SO_NOSIGPIPE setup in TCPSocket is currently ignoring
the result of setsockopt, which can leave the socket in a state where send may
trigger SIGPIPE before Transport.handleWrite() can observe EPIPE. Update the
socket setup path in the TCPSocket initializer/helper to check the setsockopt
return value, and if it fails on Darwin, immediately treat it as a socket error
by closing the socket and propagating an error back to the caller instead of
discarding it. Use the TCPSocket and Transport.handleWrite symbols to locate the
write-path setup and keep the failure handling aligned with the existing socket
lifecycle.

In `@TestRunnerTests/TestRunnerTests.swift`:
- Line 24: The test setup in DefaultHTTPServer is still binding the server to
127.0.0.1 even though TCPSocket.bind(interface:) currently treats the interface
as IPv6, so the listener is not truly IPv4. Update the TestRunnerTests/server
setup to use the matching loopback family consistently (for example, switch both
server and client-side expectations to ::1/[::1]), or if IPv4 is required,
adjust TCPSocket and the DefaultHTTPServer path to support AF_INET first. Use
the existing DefaultHTTPServer initializer and TCPSocket.bind interface handling
to locate the change.

---

Outside diff comments:
In `@NativeScript/runtime/Runtime.mm`:
- Around line 445-458: Runtime::RunMainScript currently ignores the boolean
result from ModuleInternal::RunModule, so startup can continue even when main
module loading fails without throwing. Update RunMainScript to use the same
failure contract as Runtime::RunModule by capturing the return value from
moduleInternal_->RunModule and handling a false result as a startup failure,
using the existing Runtime and ModuleInternal::RunModule symbols to locate the
change.

---

Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 263-279: The failure diagnostics step is too broad: it copies the
entire CoreSimulator log tree and runs an unbounded log collect, which can
create oversized artifacts and slow CI. In the diagnostics block that uses DIAG,
xcrun simctl spawn, and log collect, stop archiving the full CoreSimulator
directory and switch to a targeted unified-log collection window using the macOS
15/Xcode 26-supported log collect --last option so only recent logs are
captured.

In `@NativeScript/runtime/ModuleInternalCallbacks.h`:
- Around line 44-45: GetLoadedModuleUrls() is still using an implicit current
isolate, which can mix up worker and main-thread diagnostics now that the module
registry is isolate-keyed. Update the ModuleInternalCallbacks API so
GetLoadedModuleUrls takes a v8::Isolate* parameter, and propagate that isolate
through the implementation and any call sites to match the other registry
helpers. Use the existing registry symbols in ModuleInternalCallbacks to keep
the diagnostics explicitly scoped to the target isolate.

In `@NativeScript/runtime/Runtime.mm`:
- Around line 252-253: The TODO in Runtime.mm about a possible worker queue
leak/race during termination ordering needs to be tracked before merge. Follow
up on the worker lifecycle path in the Runtime-related
initialization/termination flow, especially around the queue handling and the
Terminate-before-Initialize scenario, and either replace the TODO with a
concrete fix or create a tracked issue/fix plan linked to the worker queue race
so it is not lost.

In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 59-90: The URLImpl::searchParams getter caches a single
_searchParams instance and never refreshes it when url.search changes, so later
reads can return stale query data. Update the URL.prototype.searchParams
handling in URLImpl.cpp so the cached URLSearchParams is invalidated or resynced
whenever the search setter runs, and make sure the getter recreates/updates the
instance from the current search string before returning it.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 5096dcca-ed10-47ea-be7c-64aafa7275ac

📥 Commits

Reviewing files that changed from the base of the PR and between e652dd0 and ada922f.

📒 Files selected for processing (28)
  • .github/scripts/sample-hung-app.sh
  • .github/workflows/npm_release.yml
  • NativeScript/runtime/DevFlags.h
  • NativeScript/runtime/DevFlags.mm
  • NativeScript/runtime/HMRSupport.h
  • NativeScript/runtime/HMRSupport.mm
  • NativeScript/runtime/ModuleInternal.h
  • NativeScript/runtime/ModuleInternal.mm
  • NativeScript/runtime/ModuleInternalCallbacks.h
  • NativeScript/runtime/ModuleInternalCallbacks.mm
  • NativeScript/runtime/Runtime.h
  • NativeScript/runtime/Runtime.mm
  • NativeScript/runtime/URLImpl.cpp
  • NativeScript/runtime/URLImpl.h
  • NativeScript/runtime/Worker.h
  • NativeScript/runtime/Worker.mm
  • TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js
  • TestRunner/app/tests/HttpEsmLoaderTests.js
  • TestRunner/app/tests/MethodCallsTests.js
  • TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
  • TestRunner/app/tests/RemoteModuleSecurityTests.js
  • TestRunner/app/tests/esm/hmr/hot-data-ext.js
  • TestRunner/app/tests/esm/hmr/hot-data-ext.mjs
  • TestRunnerTests/Embassy/DefaultHTTPServer.swift
  • TestRunnerTests/Embassy/TCPSocket.swift
  • TestRunnerTests/Embassy/Transport.swift
  • TestRunnerTests/QUARANTINED_TESTS.md
  • TestRunnerTests/TestRunnerTests.swift

Comment on lines +95 to +103
static bool RemoteUrlMatchesAllowlistEntry(const std::string& url,
const std::string& entry) {
if (entry.empty()) return false;
if (url.size() < entry.size()) return false;
if (url.compare(0, entry.size(), entry) != 0) return false;
if (url.size() == entry.size()) return true; // exact match
if (entry.back() == '/') return true; // entry ended at a boundary
const char next = url[entry.size()];
return next == '/' || next == '?' || next == '#';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Normalize paths before matching path-scoped allowlist entries.

Line 101 authorizes any raw URL with a trailing-slash entry prefix, so an allowlist entry like https://cdn.example.com/app/ also matches https://cdn.example.com/app/../admin.js. If the fetch layer or server normalizes dot segments, this escapes the intended path scope. Parse/canonicalize the URL path before matching, or reject encoded/plain dot segments before applying the prefix rule.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/DevFlags.mm` around lines 95 - 103, The allowlist check
in RemoteUrlMatchesAllowlistEntry is matching raw URL prefixes too early, which
lets path-scoped entries be bypassed with dot-segment paths. Update the matching
logic to canonicalize or normalize the URL path before applying the
prefix/boundary rules, or explicitly reject plain/encoded dot segments in
DevFlags.mm so a trailing-slash allowlist entry cannot match escaped paths.

Comment on lines +236 to +264
} catch (const NativeScriptException& ex) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][exception] %s message=%s",
NormalizeHttpModuleUrl(path).c_str(), ex.getMessage().c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule) {
Log(@"***** JavaScript exception occurred - detailed stack trace follows *****");
Log(@"Error loading ES module: %s", path.c_str());
Log(@"Exception: %s", ex.getMessage().c_str());
Log(@"***** End stack trace - continuing execution *****");
Log(@"Debug mode - ES module loading failed, but telling iOS it succeeded to prevent app termination");
return true; // avoid termination in debug
} else {
// Surface the inner exception's message so callers passing
// `outErrorMessage` see the real cause instead of just a
// false return.
SetOutErrorMessage(outErrorMessage, ex.getMessage());
return false;
}
ex.ReThrowToV8(isolate);
return false;
}
return true;
if (moduleNamespace.IsEmpty()) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][empty] %s",
NormalizeHttpModuleUrl(path).c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule) {
Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded");
return true;
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not swallow worker ESM failures in debug.

The CJS path rethrows worker failures so worker.onerror fires, but the ESM path returns true/empty for all non-HTTP debug failures. For .mjs workers, Worker.mm’s TryCatch will never see the failure.

Proposed direction
-      if (RuntimeConfig.IsDebug && !isHttpModule) {
+      if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
         Log(@"***** JavaScript exception occurred - detailed stack trace follows *****");
         Log(@"Error loading ES module: %s", path.c_str());
         Log(@"Exception: %s", ex.getMessage().c_str());
         Log(@"***** End stack trace - continuing execution *****");
         Log(@"Debug mode - ES module loading failed, but telling iOS it succeeded to prevent app termination");
         return true;  // avoid termination in debug
       } else {
         SetOutErrorMessage(outErrorMessage, ex.getMessage());
+        if (cache->isWorker) {
+          ex.ReThrowToV8(isolate);
+        }
         return false;
       }
@@
-      if (RuntimeConfig.IsDebug && !isHttpModule) {
+      if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
         Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded");
         return true;
       } else {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (const NativeScriptException& ex) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][exception] %s message=%s",
NormalizeHttpModuleUrl(path).c_str(), ex.getMessage().c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule) {
Log(@"***** JavaScript exception occurred - detailed stack trace follows *****");
Log(@"Error loading ES module: %s", path.c_str());
Log(@"Exception: %s", ex.getMessage().c_str());
Log(@"***** End stack trace - continuing execution *****");
Log(@"Debug mode - ES module loading failed, but telling iOS it succeeded to prevent app termination");
return true; // avoid termination in debug
} else {
// Surface the inner exception's message so callers passing
// `outErrorMessage` see the real cause instead of just a
// false return.
SetOutErrorMessage(outErrorMessage, ex.getMessage());
return false;
}
ex.ReThrowToV8(isolate);
return false;
}
return true;
if (moduleNamespace.IsEmpty()) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][empty] %s",
NormalizeHttpModuleUrl(path).c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule) {
Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded");
return true;
} else {
} catch (const NativeScriptException& ex) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][exception] %s message=%s",
NormalizeHttpModuleUrl(path).c_str(), ex.getMessage().c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
Log(@"***** JavaScript exception occurred - detailed stack trace follows *****");
Log(@"Error loading ES module: %s", path.c_str());
Log(@"Exception: %s", ex.getMessage().c_str());
Log(@"***** End stack trace - continuing execution *****");
Log(@"Debug mode - ES module loading failed, but telling iOS it succeeded to prevent app termination");
return true; // avoid termination in debug
} else {
// Surface the inner exception's message so callers passing
// `outErrorMessage` see the real cause instead of just a
// false return.
SetOutErrorMessage(outErrorMessage, ex.getMessage());
if (cache->isWorker) {
ex.ReThrowToV8(isolate);
}
return false;
}
}
if (moduleNamespace.IsEmpty()) {
if (isHttpModule && RuntimeConfig.IsDebug && IsScriptLoadingLogEnabled()) {
Log(@"[run-module][http-esm][empty] %s",
NormalizeHttpModuleUrl(path).c_str());
}
if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) {
Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded");
return true;
} else {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/ModuleInternal.mm` around lines 236 - 264, The debug
handling in ModuleInternal.mm is swallowing non-HTTP ES module failures by
returning true or an empty namespace, which prevents worker error propagation
for .mjs loads. Update the NativeScriptException catch and the
moduleNamespace.IsEmpty path so debug mode still surfaces failures to the caller
for worker-loaded ESM, instead of always pretending success; keep the existing
HTTP debug logging, but ensure the Worker.mm TryCatch can observe the error for
the ES module load path.

Comment on lines +94 to +100
v8::Local<v8::Script> script;
auto compiled = v8::Script::Compile(context, ToV8String(isolate, blob_methods)).ToLocal(&script);

if (compiled) {
v8::Local<v8::Value> outVal;
(void)script->Run(context).ToLocal(&outVal);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle install script failures explicitly.

If this script fails to compile or run, blob URL support is silently absent and a pending V8 exception may be left behind. Wrap it in TryCatch and log/throw the failure.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/URLImpl.cpp` around lines 94 - 100, The install script
execution in URLImpl should not fail silently: the current Compile/Run flow can
leave blob URL support partially initialized and a pending V8 exception
uncleared. Update the script path in URLImpl to wrap the v8::Script::Compile and
script->Run calls in a v8::TryCatch, then explicitly handle both compile and
runtime failures by logging the error and propagating it (or throwing) instead
of ignoring the result. Use the existing blob_methods script setup in URLImpl as
the place to add this error handling.

Comment on lines +486 to +499
int32_t terminatedCount = 0;
for (auto& state : snapshot) {
auto* worker = static_cast<WorkerWrapper*>(state->UserData());
if (worker == nullptr) {
continue;
}
if (!worker->IsRunning() || worker->IsClosing()) {
// Already torn down or in the process of closing. Counting these
// would inflate the diagnostic count returned to JS; skip.
continue;
}
try {
worker->Terminate();
++terminatedCount;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Filter termination to the current main isolate.

This callback terminates every worker in Caches::Workers. Runtime::~Runtime() already filters by WorkerWrapper::GetMainIsolate(), so this HMR helper should do the same to avoid one runtime/dev session killing another runtime’s workers.

Proposed fix
   int32_t terminatedCount = 0;
+  Isolate* mainIsolate = info.GetIsolate();
   for (auto& state : snapshot) {
     auto* worker = static_cast<WorkerWrapper*>(state->UserData());
     if (worker == nullptr) {
       continue;
     }
+    if (worker->GetMainIsolate() != mainIsolate) {
+      continue;
+    }
     if (!worker->IsRunning() || worker->IsClosing()) {
       continue;
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
int32_t terminatedCount = 0;
for (auto& state : snapshot) {
auto* worker = static_cast<WorkerWrapper*>(state->UserData());
if (worker == nullptr) {
continue;
}
if (!worker->IsRunning() || worker->IsClosing()) {
// Already torn down or in the process of closing. Counting these
// would inflate the diagnostic count returned to JS; skip.
continue;
}
try {
worker->Terminate();
++terminatedCount;
int32_t terminatedCount = 0;
Isolate* mainIsolate = info.GetIsolate();
for (auto& state : snapshot) {
auto* worker = static_cast<WorkerWrapper*>(state->UserData());
if (worker == nullptr) {
continue;
}
if (worker->GetMainIsolate() != mainIsolate) {
continue;
}
if (!worker->IsRunning() || worker->IsClosing()) {
// Already torn down or in the process of closing. Counting these
// would inflate the diagnostic count returned to JS; skip.
continue;
}
try {
worker->Terminate();
+terminatedCount;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@NativeScript/runtime/Worker.mm` around lines 486 - 499, The HMR termination
loop in Worker::TerminateWorkers currently iterates all entries from
Caches::Workers, which can affect workers from other runtimes. Update this
callback to filter worker wrappers by the current main isolate, matching the
existing Runtime::~Runtime() behavior via WorkerWrapper::GetMainIsolate(). Keep
the existing running/closing checks and only call WorkerWrapper::Terminate() for
workers belonging to the same isolate.

Comment on lines +51 to +73
try {
if (hot && typeof hot.accept === "function") {
hot.accept(function () {});
}
if (hot && typeof hot.dispose === "function") {
hot.dispose(function () {});
}
if (hot && typeof hot.decline === "function") {
hot.decline();
}
if (hot && typeof hot.invalidate === "function") {
hot.invalidate();
}
result.ok =
result.hasHot &&
result.hasData &&
result.hasAccept &&
result.hasDispose &&
result.hasDecline &&
result.hasInvalidate &&
result.hasPrune;
} catch (e) {
result.error = (e && e.message) ? e.message : String(e);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Don't mutate HMR state from this shared fixture.

decline()/invalidate() change module lifecycle state, and accept()/dispose() register callbacks on the same fixture that later specs re-import to verify shared hot.data. That makes the suite order-dependent. Keep this helper to shape-only checks, or move hook-invocation coverage to a dedicated throwaway module.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@TestRunner/app/tests/esm/hmr/hot-data-ext.js` around lines 51 - 73, The
hot-data fixture is mutating shared HMR state by invoking hot.accept,
hot.dispose, hot.decline, and hot.invalidate in the shared test helper, which
makes later specs order-dependent. Update hot-data-ext.js so the shared fixture
only checks for the presence of HMR APIs and data on hot, and remove
lifecycle/callback registration from this path. If coverage for those methods is
needed, move it into a separate throwaway module or dedicated test helper that
is not reused across specs.

Comment on lines +55 to +78
try {
configureRuntime({
importMap: {
imports: {
__ns_test_vendor__: "ns-vendor://__ns_test_vendor__",
},
},
});

const mod = await import("__ns_test_vendor__");
const modAgain = await import("__ns_test_vendor__");

expect(mod).toBeDefined();
expect(modAgain).toBe(mod);
expect(mod.default).toEqual({ source: "vendor-default" });
expect(mod.namedValue).toBe(7);
expect(mod.makeValue()).toBe("vendor-named");
} finally {
configureRuntime({ importMap: { imports: {} } });
if (typeof previousRegistry === "undefined") {
delete globalThis.__nsVendorRegistry;
} else {
globalThis.__nsVendorRegistry = previousRegistry;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Restore the previous import-map state here.

This spec snapshots __nsVendorRegistry, but it does not snapshot the preexisting runtime import-map before calling configureRuntime(...). Resetting to imports: {} in finally can clobber earlier resolver state for later specs, and if the runtime merges config, it may not even remove the temporary mapping. Restore the prior import-map alongside the registry so the test stays hermetic.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs` around lines 55
- 78, The test cleanup in the __ns_test_vendor__ import-map spec is not
restoring the runtime’s prior import-map state, which can leak configuration
into later specs. Snapshot the existing import-map before calling
configureRuntime in this test, then in the finally block restore that original
import-map instead of resetting to an empty imports object. Keep the existing
__nsVendorRegistry restore logic intact so the test remains hermetic.

Comment on lines +60 to 66
_ = setsockopt(
fileDescriptor,
SOL_SOCKET,
SO_NOSIGPIPE,
&value,
socklen_t(MemoryLayout<Int32>.size)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the relevant files and inspect the changed socket wrapper plus transport usage.
git ls-files 'TestRunnerTests/Embassy/*' | sed -n '1,120p'
printf '\n--- TCPSocket.swift ---\n'
nl -ba TestRunnerTests/Embassy/TCPSocket.swift | sed -n '1,220p'
printf '\n--- Transport.swift ---\n'
nl -ba TestRunnerTests/Embassy/Transport.swift | sed -n '1,220p'

Repository: NativeScript/ios

Length of output: 1497


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read only the relevant socket-related definitions/usages to understand failure handling.
rg -n "SO_NOSIGPIPE|SIGPIPE|ignoreSigPipe|send\\(" TestRunnerTests -S

Repository: NativeScript/ios

Length of output: 1654


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check platform availability of SO_NOSIGPIPE semantics from local headers/docs if present.
python3 - <<'PY'
import os,glob,subprocess,sys,shlex
candidates = [
    "/usr/include/sys/socket.h",
    "/usr/include/x86_64-linux-gnu/sys/socket.h",
    "/usr/local/include/sys/socket.h",
]
for p in candidates:
    if os.path.exists(p):
        print("FOUND", p)
        try:
            out = subprocess.check_output(["grep","-n","SO_NOSIGPIPE",p], text=True, stderr=subprocess.STDOUT)
            print(out)
        except subprocess.CalledProcessError as e:
            print("no SO_NOSIGPIPE in", p)
PY

Repository: NativeScript/ios

Length of output: 265


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the exact changed implementation and surrounding comments.
sed -n '1,120p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '1,120p' TestRunnerTests/Embassy/Transport.swift
printf '\n---\n'
sed -n '60,90p' TestRunnerTests/QUARANTINED_TESTS.md

Repository: NativeScript/ios

Length of output: 9189


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for the specific error handling around send/write to see whether EPIPE is handled and where SIGPIPE would matter.
sed -n '170,230p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '160,240p' TestRunnerTests/Embassy/Transport.swift

Repository: NativeScript/ios

Length of output: 4693


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check whether ignoreSigPipe is used anywhere else, especially before send.
rg -n "ignoreSigPipe\\s*=\\s*true|ignoreSigPipe\\s*=\\s*false|SO_NOSIGPIPE|SIGPIPE" TestRunnerTests/Embassy -S

Repository: NativeScript/ios

Length of output: 804


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the exact comments around the changed logic and the quarantine note.
sed -n '30,80p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '66,86p' TestRunnerTests/QUARANTINED_TESTS.md

Repository: NativeScript/ios

Length of output: 3520


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the write path that relies on send() errors being surfaced.
sed -n '170,235p' TestRunnerTests/Embassy/Transport.swift

Repository: NativeScript/ios

Length of output: 2235


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check for any global SIGPIPE handling or alternative socket flags in the harness.
rg -n "signal\\(SIGPIPE|SIG_IGN|MSG_NOSIGNAL|SO_NOSIGPIPE|ignoreSigPipe" TestRunnerTests -S

Repository: NativeScript/ios

Length of output: 1121


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect whether the transport can avoid calling send() on sockets that failed SO_NOSIGPIPE setup.
python3 - <<'PY'
from pathlib import Path
p = Path("TestRunnerTests/Embassy/TCPSocket.swift")
text = p.read_text()
start = text.index("var ignoreSigPipe")
end = text.index("init(blocking:", start)
print(text[start:end])
PY

Repository: NativeScript/ios

Length of output: 1119


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for mentions of the changed failure mode in repo notes to see if this is an accepted tradeoff.
rg -n "best-effort|assertionFailure|Executed 0 tests|SIGPIPE|SO_NOSIGPIPE" TestRunnerTests/QUARANTINED_TESTS.md TestRunnerTests/Embassy -S

Repository: NativeScript/ios

Length of output: 1543


🌐 Web query:

Darwin SO_NOSIGPIPE send SIGPIPE EPIPE setsockopt failure behavior documentation

💡 Result:

On Darwin (macOS/iOS), the SO_NOSIGPIPE socket option is the standard mechanism to prevent a process from receiving a SIGPIPE signal when writing to a stream-oriented socket that has no reader [1][2][3]. When SO_NOSIGPIPE is enabled via setsockopt, the following behavior occurs: 1. Signal Suppression: The kernel does not generate a SIGPIPE signal when a write operation fails due to a broken connection (e.g., the peer has closed the socket) [3][4]. 2. Error Reporting: Instead of terminating the process with a signal, the write operation (e.g., send, write) will fail and return -1, with errno set to EPIPE [3][5]. Usage: To enable this, call setsockopt with the SO_NOSIGPIPE option on the socket file descriptor, typically immediately after creation [6]: int value = 1; if (setsockopt(sock, SOL_SOCKET, SO_NOSIGPIPE, &value, sizeof(value)) < 0) { // Handle failure (e.g., log error or close socket) } Failure Behavior: If the setsockopt call fails, the socket will remain in its default state, which is vulnerable to SIGPIPE [7]. Because SO_NOSIGPIPE is a socket-level option (not a per-call flag like MSG_NOSIGNAL available on Linux), it is common practice for robust applications to check the return value of setsockopt [8][7]. If it fails, the application should consider the socket untrusted or handle the potential SIGPIPE signal globally (e.g., via signal(SIGPIPE, SIG_IGN)) to prevent unexpected process termination [7][6]. While failure of this system call is rare on valid sockets, unchecked failures can leave a process susceptible to process termination if a peer disconnects [8][7].

Citations:


Handle SO_NOSIGPIPE setup failures as socket errors. If setsockopt fails on Darwin, the next send(..., 0) can still raise SIGPIPE and abort the runner before Transport.handleWrite() sees EPIPE. Close the socket or return an error instead of discarding the result.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@TestRunnerTests/Embassy/TCPSocket.swift` around lines 60 - 66, The
SO_NOSIGPIPE setup in TCPSocket is currently ignoring the result of setsockopt,
which can leave the socket in a state where send may trigger SIGPIPE before
Transport.handleWrite() can observe EPIPE. Update the socket setup path in the
TCPSocket initializer/helper to check the setsockopt return value, and if it
fails on Darwin, immediately treat it as a socket error by closing the socket
and propagating an error back to the caller instead of discarding it. Use the
TCPSocket and Transport.handleWrite symbols to locate the write-path setup and
keep the failure handling aligned with the existing socket lifecycle.


loop = try! SelectorEventLoop(selector: try! KqueueSelector())
self.server = DefaultHTTPServer(eventLoop: loop!, port: port) {
self.server = DefaultHTTPServer(eventLoop: loop!, interface: "127.0.0.1", port: port) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

127.0.0.1 does not match the current socket bind path

DefaultHTTPServer still routes through TCPSocket.bind(..., interface:), which parses the interface as IPv6. With that implementation, "127.0.0.1" does not create a real IPv4 listener here, so the harness still depends on dual-stack behavior. If this test needs IPv4 specifically, TCPSocket needs AF_INET support first; otherwise keep both sides on ::1 / [::1].

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@TestRunnerTests/TestRunnerTests.swift` at line 24, The test setup in
DefaultHTTPServer is still binding the server to 127.0.0.1 even though
TCPSocket.bind(interface:) currently treats the interface as IPv6, so the
listener is not truly IPv4. Update the TestRunnerTests/server setup to use the
matching loopback family consistently (for example, switch both server and
client-side expectations to ::1/[::1]), or if IPv4 is required, adjust TCPSocket
and the DefaultHTTPServer path to support AF_INET first. Use the existing
DefaultHTTPServer initializer and TCPSocket.bind interface handling to locate
the change.

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