From 83da6cb818a022047fe50670fbb24e841a8d8827 Mon Sep 17 00:00:00 2001 From: "claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla)" Date: Tue, 11 Aug 2026 01:32:54 +0000 Subject: [PATCH] pin what the native bind borrows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@number0/iroh`'s async `Endpoint.bind` takes its `RelayMode` by reference and keeps using it after the call returns, so the temporary Radial passed was collectable mid-bind: the finalizer dropped the Rust box and the running bind read freed memory. That is the intermittent `radiald run` startup crash — SIGSEGV, SIGBUS or an allocator SIGABRT, always inside bind, gone on a rerun. `pinned()` holds every native object handed to a native async call for the length of the call. The fixture died in 9 fresh starts of 12 before and 0 of 12 after; the probe now runs 25 starts with relays disabled and 8 with default relays without a crash. Co-Authored-By: claudebot.disnetdev.com (did:plc:n6ku5xddiuguwze3f356evla) --- docs/adr-private-mode-iroh.md | 30 ++++++++++ packages/daemon/README.md | 19 ++++++- packages/transport-iroh/README.md | 22 ++++++++ packages/transport-iroh/src/index.ts | 55 ++++++++++++++++--- .../test/argument-lifetime.test.mjs | 43 +++++++++++++++ packages/transport-iroh/test/bind-once.mjs | 24 ++++++++ .../transport-iroh/test/startup-probe.mjs | 41 +++++++++++--- 7 files changed, 218 insertions(+), 16 deletions(-) create mode 100644 packages/transport-iroh/test/argument-lifetime.test.mjs create mode 100644 packages/transport-iroh/test/bind-once.mjs diff --git a/docs/adr-private-mode-iroh.md b/docs/adr-private-mode-iroh.md index e4360e0..90263e7 100644 --- a/docs/adr-private-mode-iroh.md +++ b/docs/adr-private-mode-iroh.md @@ -1538,3 +1538,33 @@ Wire `from` claims, candidate polling, and held unidentified links are removed; no attached space authorizes closes immediately. §30.2–30.3's lasting safety rules remain: the folded ever-member connection directory, inventory-free partial catch-up, cursor retention, and blob/gossip suppression on a retained bootstrap route. + +## 32. Decision: a native object handed to a native async call is pinned for the call + +`radiald run` died intermittently at startup — `SIGSEGV`, `SIGBUS`, or a `SIGABRT` out of the +allocator — after the phase log that says the native endpoint is binding and before the one that says +it bound, and came up cleanly on a rerun. That is one use-after-free wearing three signals. + +`@number0/iroh@1.1.0` declares `async fn bind(_, relay_mode: Option<&RelayMode>)`. The future borrows +the Rust box the JavaScript `RelayMode` owns, and it runs on after the call has returned to +JavaScript; napi keeps nothing reachable on this side meanwhile. Written into the argument list +directly — which is how §14's adapter wrote it — the `RelayMode` is garbage the moment the call is +made, so a collection landing in the window frees the box under the running bind. A +`FinalizationRegistry` sees exactly that: the object finalized while `bind` is still pending. The +window is a few microseconds wide, which is the whole reason it reads as flaky rather than broken: +the fixture in `transport-iroh/test/bind-once.mjs` died in 9 fresh starts out of 12 before the fix, +and 0 out of 12 after. Three arms over one script separate the reference from the timing — the +temporary died 11 times in 12, holding it for the call died 0, and a control that perturbed the same +statement without holding anything died 9. + +**The rule is the argument's lifetime, not the signature's.** `pinned()` holds a native object in a +module-level set for the length of the native call, and everything `transport-iroh` hands to a native +async call goes through it — `bind`'s relay mode, and `dial`'s endpoint address, which a finalizer +probe never caught being collected but which is the same borrow and runs every sync round. Do not +re-derive which of the binding's signatures are safe to pass a temporary to; pin it. + +There is nothing upstream to move to. 1.1.0 is the current release and the borrow is in the binding's +shape, so §8's exact pin stays where it is; a report belongs to `n0-computer/iroh-ffi` with the +fixture above, and this repository does not need it to be accepted before it is safe. Nothing here +touches the wire, the fold, or the endpoint's identity: an endpoint that survived its bind was always +correct, and the ones that did not never reached a peer. diff --git a/packages/daemon/README.md b/packages/daemon/README.md index 6027cca..1ff89a3 100644 --- a/packages/daemon/README.md +++ b/packages/daemon/README.md @@ -238,15 +238,32 @@ production uses. ### Troubleshooting a native startup crash +**The one found so far is fixed.** `radiald run` died intermittently with `SIGSEGV`, `SIGBUS`, or a +`SIGABRT` out of the allocator, after `private transport: binding native iroh endpoint` and before +the line that reports the bound endpoint — and started cleanly on a rerun. `@number0/iroh@1.1.0` +borrows the `RelayMode` argument of its async `Endpoint.bind` and keeps using it after the call +returns, so the argument was collectable while the bind was still reading it; +`packages/transport-iroh` now pins every native object it hands to a native async call +(`transport-iroh`'s README and ADR §32). A build from before that fix crashed 9 starts in 12 on +Linux/arm64 under `node --test packages/transport-iroh/test/argument-lifetime.test.mjs`, and 0 in 12 +after. There is no version to upgrade to: 1.1.0 is the current release, and the defect is in how the +binding lends the argument, not in a fixable pin. + +Anything else that dies this way is a new fault, and the procedure below is how to characterise it. + pnpm's `ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL` message only reports that `radiald` died. If it names `SIGSEGV`, collect the last `private transport:` startup phase and a native backtrace: on Linux use `coredumpctl info` and `coredumpctl gdb` (or the platform equivalent). Core dumps may contain credentials and private records; keep the dump secret and share only a redacted, symbolized backtrace. Record the Radial commit, Node and pnpm versions, OS, architecture, libc, container use, whether `run.privateSpaces` is non-empty, and the relay mode. Redact DIDs, paths, keys, and tokens. +Read the last phase as a lower bound: under pnpm the daemon's stdout is a pipe, Node writes to a pipe +asynchronously, and a crash drops whatever it had not flushed. The phase you see is one the process +reached, not provably the last one. The isolated probe exercises import, bind, online/address discovery, and close in fresh child -processes. A crash in one iteration is reported by signal without losing the remaining results: +processes. A crash in one iteration is reported by signal without losing the remaining results, and a +start that neither exits nor dies is killed and reported too (`--start-timeout-ms`, default 120000): ```sh pnpm --filter @radial/transport-iroh probe -- --iterations 100 --relays disabled diff --git a/packages/transport-iroh/README.md b/packages/transport-iroh/README.md index 0e1d632..1c4c7c6 100644 --- a/packages/transport-iroh/README.md +++ b/packages/transport-iroh/README.md @@ -21,5 +21,27 @@ a stream die (ADR §21). Which failure is which stays `core`'s decision; the onl is that there is still a stream to answer on. A failure that is not the frame's still resets both halves, because there is nothing legible to say. +## A native object handed to a native async call must be pinned + +`@number0/iroh@1.1.0` takes native objects **by reference** on its async entry points — +`Endpoint.bind(options, relayMode)` is `async fn bind(_, relay_mode: Option<&RelayMode>)`. The Rust +future borrows the box the JavaScript object owns and keeps running after the call has returned, and +napi holds nothing on this side while it does. A `RelayMode` written straight into the argument list +is unreachable the instant the call is made, so V8 may collect it mid-bind: the finalizer drops the +Rust box and the running bind reads freed memory. + +That was the intermittent `radiald run` startup crash — SIGSEGV, SIGBUS, or a SIGABRT out of the +allocator, always inside `bind`, always gone on the next start, because it turns on whether a +collection happened to land in a window a few microseconds wide. `pinned()` in `src/index.ts` holds +the argument in a module-level set for the length of the call, and everything handed to a native +async call goes through it. The evidence, and the fixture that reproduces it, are in +`test/argument-lifetime.test.mjs` and `test/bind-once.mjs`; `test/startup-probe.mjs` is the same +reproduction at operator scale (ADR §32). + +A native async *method* also has its receiver to keep alive, and a finalizer probe never caught +`connect`'s `EndpointAddr` being collected — but an unobserved collection is not a guarantee, and +`pinned()` costs a set insertion, so `dial` uses it too. If a future call hands the binding a native +object, pin it; do not re-derive which signatures are safe. + This package is intentionally a leaf and is not browser-compatible. An official iroh browser/WASM binding is still required before the UI can fill a private replica from peers. diff --git a/packages/transport-iroh/src/index.ts b/packages/transport-iroh/src/index.ts index 908ba2d..84ca0ed 100644 --- a/packages/transport-iroh/src/index.ts +++ b/packages/transport-iroh/src/index.ts @@ -31,6 +31,39 @@ const CLOSE_REASON = [...new TextEncoder().encode('radial transport closed')] const bytes = (value: Array): Uint8Array => Uint8Array.from(value) const numbers = (value: Uint8Array): number[] => [...value] +/** + * Keep a native object reachable for as long as a native async call is using it. + * + * `@number0/iroh` is a napi binding, and its async entry points take native objects **by reference**: + * `Endpoint.bind(options, relayMode)` is `async fn bind(_, relay_mode: Option<&RelayMode>)`. The + * Rust future borrows the box the JavaScript object owns, and it keeps running after the call has + * returned to us — but nothing holds the JavaScript object while it does. A `RelayMode` passed + * straight into the call is a temporary the moment the call is made, so V8 may collect it mid-bind; + * the finalizer drops the Rust box and the still-running bind reads freed memory. + * + * That is the intermittent `radiald run` startup crash, and its signature is exactly a + * use-after-free: SIGSEGV, SIGBUS, or SIGABRT out of the allocator ("malloc(): unsorted double + * linked list corrupted"), always inside `bind`, and gone on the next run because it depends on + * whether a collection happened to land in the window. A `FinalizationRegistry` sees the `RelayMode` + * finalized while `bind` is still pending; over 12 fresh starts the temporary crashed 11 and the + * pinned argument crashed 0. + * + * So every native object handed to a native async call goes through here, for the length of the + * call. A module-level set is the whole mechanism: it is a GC root, and the `finally` releases it. + * Each call pins a holder of its own rather than the object itself, so two overlapping calls that + * happened to be handed the same object cannot have the first to finish unpin the second. + */ +const pinnedForNativeCall = new Set<{ value: unknown }>() +const pinned = async (value: unknown, call: () => Promise): Promise => { + const holder = { value } + pinnedForNativeCall.add(holder) + try { + return await call() + } finally { + pinnedForNativeCall.delete(holder) + } +} + /** A live iroh connection presented as core's transport-neutral frame link. */ export class IrohPeerLink implements PeerLink { readonly id: string @@ -104,12 +137,16 @@ export class IrohTransport { } static async bind(options: IrohTransportOptions): Promise { - const endpoint = await Endpoint.bind( - { - alpns: [ALPN], - ...(options.secretKey ? { secretKey: numbers(options.secretKey) } : {}), - }, - options.relays === 'disabled' ? RelayMode.disabled() : RelayMode.defaultMode(), + const relayMode = + options.relays === 'disabled' ? RelayMode.disabled() : RelayMode.defaultMode() + const endpoint = await pinned(relayMode, () => + Endpoint.bind( + { + alpns: [ALPN], + ...(options.secretKey ? { secretKey: numbers(options.secretKey) } : {}), + }, + relayMode, + ), ) return new IrohTransport(endpoint, options) } @@ -141,7 +178,11 @@ export class IrohTransport { const id = EndpointId.fromString(peer.endpointId) const relay = peer.relays?.[0] const address = new EndpointAddr(id, relay, peer.directAddresses) - const connection = await this.#endpoint.connect(address, ALPN) + // `connect` borrows this address the same way `bind` borrows a relay mode. A finalizer probe did + // not catch this one being collected mid-dial — an async *method* also has its receiver to keep + // alive — but that is an unobserved collection and not a guarantee, and a dial runs every sync + // round. Pinned by the same rule rather than by the difference between the two signatures. + const connection = await pinned(address, () => this.#endpoint.connect(address, ALPN)) const link = new IrohPeerLink(connection) if (link.id !== peer.endpointId) { link.close() diff --git a/packages/transport-iroh/test/argument-lifetime.test.mjs b/packages/transport-iroh/test/argument-lifetime.test.mjs new file mode 100644 index 0000000..5410b9a --- /dev/null +++ b/packages/transport-iroh/test/argument-lifetime.test.mjs @@ -0,0 +1,43 @@ +import assert from 'node:assert/strict' +import { spawn } from 'node:child_process' +import { fileURLToPath } from 'node:url' +import { test } from 'node:test' + +// The intermittent `radiald run` startup crash was a native object collected while the native call +// that borrowed it was still running (`src/index.ts` has the whole argument). It cannot be asserted +// in-process: the failure is a signal, not an exception, and it would take the runner with it. So +// each start is a fresh child, and what is asserted is that the child exited rather than died. +// +// This is a reproduction and not a shape check. `bind-once.mjs` died in 9 of 12 starts against the +// build that passed the relay mode straight into `Endpoint.bind`, and in 0 of 12 against this one. +// Five starts put the chance of a regression slipping through at about one in a thousand while +// costing the suite five endpoint binds; the fixture's comment says why its shape is not free to +// change. +const FIXTURE = fileURLToPath(new URL('./bind-once.mjs', import.meta.url)) +const STARTS = 5 + +const start = () => + new Promise((resolve) => { + const child = spawn(process.execPath, [FIXTURE, '--relays', 'disabled'], { + stdio: ['ignore', 'ignore', 'pipe'], + }) + let stderr = '' + child.stderr.on('data', (chunk) => { + stderr += chunk + }) + child.on('error', (error) => resolve({ error: error.message, stderr })) + child.on('exit', (code, signal) => resolve({ code, signal, stderr })) + }) + +test('binds an endpoint without collecting what the native call borrowed', async () => { + for (let attempt = 1; attempt <= STARTS; attempt += 1) { + const result = await start() + assert.equal(result.error, undefined, `start ${attempt} could not run: ${result.error}`) + assert.equal( + result.signal, + null, + `start ${attempt} died with ${result.signal}: a native argument was collected mid-call\n${result.stderr}`, + ) + assert.equal(result.code, 0, `start ${attempt} exited ${result.code}\n${result.stderr}`) + } +}) diff --git a/packages/transport-iroh/test/bind-once.mjs b/packages/transport-iroh/test/bind-once.mjs new file mode 100644 index 0000000..e9e74c7 --- /dev/null +++ b/packages/transport-iroh/test/bind-once.mjs @@ -0,0 +1,24 @@ +#!/usr/bin/env node +// One start of the native endpoint in a fresh process: bind, read the identity, close. +// +// This is the regression fixture for the argument-lifetime crash in `src/index.ts`, and its shape is +// load-bearing. The unsafe window is a few microseconds wide — the native `bind` reads the relay +// mode it borrowed almost immediately — so what decides whether a collection lands inside it is the +// state of a young heap on the FIRST bind of a fresh process. On the build that let the argument be +// collected this file died in 9 starts out of 12; forcing a collection instead, or binding a second +// time in the same process, both drop that to 1 in 12. Measure before tidying this up: a tidier +// fixture is very easily one that no longer reproduces anything. +// +// Driven by `argument-lifetime.test.mjs`. `startup-probe.mjs` reproduced the same crash at operator +// scale and stays the tool for a report from the field; this is the bounded version CI can afford. +import { writeSync } from 'node:fs' + +const argv = process.argv.slice(2) +const relays = argv.includes('--relays') ? argv[argv.indexOf('--relays') + 1] : 'disabled' +if (relays !== 'default' && relays !== 'disabled') throw new Error('--relays must be default or disabled') + +const { IrohTransport } = await import('../dist/index.js') +writeSync(2, `probe: binding (relays: ${relays})\n`) +const transport = await IrohTransport.bind({ endpoint: { async accept() {} }, relays }) +if (!transport.endpointId) throw new Error('bound endpoint has no id') +await transport.close() diff --git a/packages/transport-iroh/test/startup-probe.mjs b/packages/transport-iroh/test/startup-probe.mjs index 6eba8a9..4e10470 100644 --- a/packages/transport-iroh/test/startup-probe.mjs +++ b/packages/transport-iroh/test/startup-probe.mjs @@ -30,10 +30,14 @@ async function child() { ...(secretKey ? { secretKey } : {}), }) try { + let timer const online = await Promise.race([ transport.online().then(() => true), - new Promise((resolve) => setTimeout(() => resolve(false), timeoutMs)), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), timeoutMs) + }), ]) + clearTimeout(timer) console.log(JSON.stringify({ endpointId: transport.endpointId, relays: transport.relays, @@ -47,23 +51,44 @@ async function child() { async function parent() { const iterations = integer('--iterations', 1) + // A start that neither finishes nor dies is its own result, and without this the whole run stops + // on it. Generous by default: this bounds a wedge, it does not measure startup latency. + const startTimeoutMs = integer('--start-timeout-ms', 120_000) const forwarded = args.filter((argument, index) => - argument !== '--iterations' && args[index - 1] !== '--iterations' && argument !== '--child') + argument !== '--iterations' && args[index - 1] !== '--iterations' && + argument !== '--start-timeout-ms' && args[index - 1] !== '--start-timeout-ms' && + argument !== '--child') let failures = 0 for (let iteration = 1; iteration <= iterations; iteration += 1) { const result = await new Promise((resolve) => { const childProcess = spawn(process.execPath, [fileURLToPath(import.meta.url), '--child', ...forwarded], { stdio: 'inherit', }) - childProcess.on('error', (error) => resolve({ error })) - childProcess.on('exit', (code, signal) => resolve({ code, signal })) + const wedged = setTimeout(() => { + childProcess.kill('SIGKILL') + resolve({ timedOut: true }) + }, startTimeoutMs) + childProcess.on('error', (error) => { + clearTimeout(wedged) + resolve({ error }) + }) + childProcess.on('exit', (code, signal) => { + clearTimeout(wedged) + resolve({ code, signal }) + }) }) - if (result.error || result.code !== 0) failures += 1 - console.log(`probe: iteration ${iteration}/${iterations}: ${result.error ? result.error.message : result.signal ? `signal ${result.signal}` : `exit ${result.code}`}`) + if (result.timedOut || result.error || result.code !== 0) failures += 1 + console.log(`probe: iteration ${iteration}/${iterations}: ${result.timedOut ? `no exit within ${startTimeoutMs}ms` : result.error ? result.error.message : result.signal ? `signal ${result.signal}` : `exit ${result.code}`}`) } console.log(`probe: ${iterations - failures} succeeded, ${failures} failed`) if (failures > 0) process.exitCode = 1 } -if (args.includes('--child')) await child() -else await parent() +if (args.includes('--child')) { + await child() + // A closed transport is the end of the measurement, but not necessarily the end of the event loop: + // `online()` under `relays: disabled` is a native call that is deliberately abandoned when the + // timeout wins the race, and it goes on holding libuv open. Report the clean start by exiting on + // it. A close that hangs never reaches this line and the parent's `--start-timeout-ms` names it. + process.exit(0) +} else await parent() -- 2.51.2