Skip to content

[RNE Rewrite] fix(ts): release native resources when a create<Task> fails - #1386

Merged
msluszniak merged 4 commits into
rne-rewritefrom
@ms/dispose-on-construction-failure
Aug 27, 2026
Merged

[RNE Rewrite] fix(ts): release native resources when a create<Task> fails#1386
msluszniak merged 4 commits into
rne-rewritefrom
@ms/dispose-on-construction-failure

Conversation

@msluszniak

Copy link
Copy Markdown
Member

Description

Fixes the finding recorded in #1355: a create<Task> that throws part-way through construction abandoned everything it had already allocated.

A factory allocates as it goes, and only hands back a dispose at the very end. Anything that threw in between left the caller with no reference to what was already there, and native memory is not garbage collected, so it stayed alive for the rest of the process. Depending on the task that is a model, a tokenizer, a phonemizer, an LLM runner, or a whole nested pipeline (Whisper owns both a tokenizer and a VAD). useModel re-runs its factory whenever the config changes, so an app pointed at a mismatched model leaked a full resource set per attempt.

src/core/lifetime.ts adds createResourceScope, which gives a factory one teardown path for both outcomes: it tracks each resource as it is created, releases them in reverse order, and the same function becomes the pipeline's dispose. Every factory now allocates through a scope and wraps its body in try/catch:

const scope = createResourceScope();
const dispose = scope.dispose;

try {
  const model = scope.track(await wrapAsync(loadModel, runtime)(modelPath));
  const { dims } = validateSpec(model.schema, { ... }); // may throw
  const tensors = [tensor('float32', outShape)] as const;
  tensors.forEach(scope.track);
  return { classify, classifyWorklet, dispose };
} catch (error) {
  dispose();
  throw error;
}

createKokoroTextToSpeech already did this with a local array and moves to the shared helper. That also closes a smaller hole it had: its two models were loaded before the try block, so one load rejecting stranded the other. Parallel loads elsewhere now track inside each promise for the same reason.

createTokenizer is unchanged. It loads a tokenizer and returns with nothing in between that can throw, so it has no window to leak through.

The helper is internal, so the public export surface is unchanged.

Introduces a breaking change?

  • Yes
  • No

Type of change

  • Bug fix (change which fixes an issue)
  • New feature (change which adds functionality)
  • Documentation update (improves or adds clarity to existing documentation)
  • Other (chores, tests, code style improvements etc.)

Tested on

  • iOS
  • Android

Testing instructions

yarn workspace react-native-executorch test
yarn typecheck
yarn lint

Expected: 28 suites, 3117 tests, 4 snapshots, 0 skipped; typecheck and lint clean.

To see the tests bite, stash src/ and re-run: every construction-failure case fails.

Screenshots

N/A

Related issues

Follows up the finding recorded in #1355.

Checklist

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation accordingly
  • My changes generate no new warnings

Additional notes

Stacked on @ms/api-tests (#1355), because that is where the suite recording this behavior lives. Merge #1355 first and this retargets to rne-rewrite cleanly.

tasks/constructionFailure.test.ts was written to record the leak as current behavior so it would fail loudly the day a factory started cleaning up. That is this PR, so it now asserts the opposite, and covers all fifteen factories rather than five. It checks every kind of handle, so a failure names the factory and the resource rather than just reporting that something leaked.

The per-pipeline suites and hooks/taskHooks.test.ts drop the allowNativeLeaks() calls they needed for the same reason. Nothing in the suite leaks any more, so the setup file's global leak check now asserts this on every construction-failure test for free.

The diff is large mostly because indenting a factory body inside try touches every line of it. The behavioral change per file is the scope, the track calls and the catch.

@msluszniak msluszniak self-assigned this Aug 26, 2026
@msluszniak msluszniak added the bug fix PRs that are fixing bugs label Aug 26, 2026
Comment thread packages/react-native-executorch/src/core/lifetime.ts
Comment thread packages/react-native-executorch/src/core/lifetime.ts Outdated
Comment thread packages/react-native-executorch/src/core/lifetime.ts Outdated
Comment thread packages/react-native-executorch/src/core/lifetime.ts Outdated
Comment thread packages/react-native-executorch/src/core/lifetime.ts
Comment thread packages/react-native-executorch/src/extensions/nlp/tasks/textEmbedding.ts Outdated
Comment thread packages/react-native-executorch/src/core/lifetime.ts
@msluszniak
msluszniak force-pushed the @ms/dispose-on-construction-failure branch from ac50f46 to 98b652e Compare August 27, 2026 07:11
Base automatically changed from @ms/api-tests to rne-rewrite August 27, 2026 08:01
msluszniak added a commit that referenced this pull request Aug 27, 2026
## Description

Adds TS API tests:
- hooks
- task pipelines
- core primitives
- the resource fetcher
- model registry.

Also adds necessary skills.

To find details about this approach please follow:
`__tests__/README.md`.

No source changes: the three contract violations the suites originally
surfaced (`SpecMatch.dim`, the missing `'worklet'` directive on
`getRegisteredBackends`, and `randomNormal`'s millisecond-resolution
default seed) have all since been fixed on `rne-rewrite`, so `src/` is
untouched by this PR.

During the testing I spotted a problem that is addressed in #1386.

### Introduces a breaking change?

- [ ] Yes
- [x] No

### Type of change

- [ ] Bug fix (change which fixes an issue)
- [ ] New feature (change which adds functionality)
- [ ] Documentation update (improves or adds clarity to existing
documentation)
- [x] Other (chores, tests, code style improvements etc.)

### Tested on

- [ ] iOS
- [ ] Android

### Testing instructions

```sh
yarn workspace react-native-executorch test
yarn typecheck
yarn lint
```

Expected: 28 suites, 3072 tests, 4 snapshots, 0 skipped; typecheck and
lint clean. `yarn prepare` still emits only `src/` into `lib/`.

### Screenshots

N/A

### Related issues

Closes #1352.

### Checklist

- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have updated the documentation accordingly
- [x] My changes generate no new warnings

### Additional notes

**Layout**

| Path | Contents |
| --- | --- |
| `core/` | tensor, model, runtime, the coded error type, and the spec
matcher (symbol binding, variants, runtime constraints, authoring
errors) |
| `fetcher/` | caching, forced re-download, byte-weighted progress,
failures, cancellation, requests shared between concurrent callers, iOS
resume, the Android DownloadManager backend, telemetry |
| `tasks/` | one suite per pipeline, plus the shared
construction-failure behavior |
| `hooks/` | `useModel`, `useResourceDownload`, and the task hooks end
to end |
| `extensions/` | box and point scaling, box decoding, seeded generators
|
| `api/` | export snapshot, registry rules, label constants,
source-level conventions |
| `support/` | the fake runtime, the mocks, the fixtures |

**Deliberately not covered:** the numerical behavior of the native
operators (that is `cpp/tests/`, and duplicating it here would only test
the fake); the parts of a pipeline whose behavior depends on real
weights: Whisper's decode loop, the VAD rolling window and the SDXS
diffusion step get schema acceptance, rejection and full disposal
instead; and the worklet thread hop, since worklets run inline. The
`'worklet'` directive convention that makes that hop possible is
enforced by parsing `src/` with the TypeScript compiler.

That line is drawn per pipeline, not per suite. The privacy filter's
logits are weights but its BIOES decode and sliding window are not, so
they run end to end; the LLM's generation belongs to the native runner
but the chat session's history, KV cache bookkeeping and tool loop are
covered against a scripted one; Kokoro's waveform is weights but its
argument validation, chunking and streaming are not; PaddleOCR's
probability map is weights but the quad decode, CTC collapse and reading
order run over a map the test paints. The fake runtime grew
`llm.createLLMRunner`, `speech.createPhonemizer`, `math.gather`,
`cv.extractDbnetTextQuads`, `cv.rectifyQuad` and `fs.readFile` to make
that possible.
A factory allocates as it goes: it loads a model, maybe a tokenizer, a
phonemizer, an LLM runner or a nested pipeline, validates the schema,
pre-allocates its tensors, and only at the end hands back a `dispose`.
Anything that threw in between left the caller with no reference to what
was already allocated, and native memory is not garbage collected, so it
stayed alive for the rest of the process.

`useModel` re-runs its factory whenever the config changes, so an app
pointed at a mismatched model leaked a full resource set per attempt.

`createResourceScope` gives a factory one teardown path for both
outcomes: it tracks each resource as it is created and releases them in
reverse order, and the same function becomes the pipeline's `dispose`.
Every factory now allocates through a scope and wraps its body in
try/catch. Kokoro already did this with a local array and moves to the
shared helper, which also closes a smaller hole it had: its two models
were loaded before the try block, so one load rejecting stranded the
other. Parallel loads elsewhere track inside each promise for the same
reason.

`createTokenizer` is unchanged. It loads a tokenizer and returns with
nothing in between that can throw, so it has no window to leak through.

The helper is internal, not exported from `src/index.ts`, so the public
surface is unchanged.

Test-side, `constructionFailure.test.ts` was recording the leak as
current behavior. It now asserts the opposite and covers all fifteen
factories rather than five, checking every kind of handle so a failure
names the factory and the resource. The per-pipeline suites and
`hooks/taskHooks.test.ts` drop the `allowNativeLeaks()` calls they
needed for the same reason: nothing in the suite leaks any more, so the
global leak check now asserts this on every construction-failure test
for free.
@msluszniak
msluszniak force-pushed the @ms/dispose-on-construction-failure branch from 98b652e to f5a59f1 Compare August 27, 2026 08:01
- core/lifetime docs no longer reference extension-layer concepts
- example moved under @example, drop the unused @module tag
- export createResourceScope so custom pipelines can use the pattern
- load models sequentially: Promise.all rejects on the first failure but
  leaves the other loads running, so their resources land after the scope
  has already been disposed
@msluszniak
msluszniak requested a review from barhanc August 27, 2026 08:56
Avoids shadowing the built-in TS `Disposable` for consumers importing
from the package root.
@msluszniak
msluszniak force-pushed the @ms/dispose-on-construction-failure branch from 8ce3b13 to b786c69 Compare August 27, 2026 09:43

@barhanc barhanc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a style comment. Otherwise looks solid.

Comment thread packages/react-native-executorch/src/extensions/nlp/tasks/privacyFilter.ts Outdated
The straight-forward sequential await needs no explanation.
@msluszniak
msluszniak merged commit b885bff into rne-rewrite Aug 27, 2026
4 checks passed
@msluszniak
msluszniak deleted the @ms/dispose-on-construction-failure branch August 27, 2026 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug fix PRs that are fixing bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants