Skip to content

fix: update output callback to use OutputCallbackJs for improved memory management#2133

Merged
GustavoA1604 merged 5 commits into
mainfrom
fix/whisper-port-output-callback-to-addon-cpp
May 21, 2026
Merged

fix: update output callback to use OutputCallbackJs for improved memory management#2133
GustavoA1604 merged 5 commits into
mainfrom
fix/whisper-port-output-callback-to-addon-cpp

Conversation

@freddy311082

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

  • Removes the whisper-local WhisperOutputCallBackJs subclass introduced in 0.7.0 as a workaround for the iOS bare-kit transcribe()-after-unload() crash (Mach exception 309 / EXC_BAD_ACCESS / PAC failure inside js_delete_reference / js_open_handle_scope).
  • That workaround duplicated upstream behavior inside this addon and added a setImmediate × 2 defensive yield in WhisperInterface.destroyInstance(), both of which are no longer needed.
  • The actual root cause — qvac-lib-inference-addon-cpp 1.1.6/1.1.7 deferring js_delete_reference() into a uv_close close-callback that races worklet js_env_t* invalidation on iOS — has now been fixed in qvac-lib-inference-addon-cpp@1.2.0, which transcription-whispercpp already depends on (vcpkg.json requires >=1.2.0).

📝 How does it solve it?

  • Deletes packages/transcription-whispercpp/addon/src/addon/WhisperOutputCallbackJs.hpp (~298 lines) and its #include from AddonJs.hpp.
  • createInstance in addon/src/addon/AddonJs.hpp now constructs the upstream qvac_lib_inference_addon_cpp::OutputCallBackJs directly (1.2.0 ordering: deleteJsReferences() runs synchronously inside ~OutputCallBackJs(), before uv_close, so the close-callback never touches js_env_t*).
  • Removes the two await new Promise(resolve => setImmediate(resolve)) belt-and-braces yields in WhisperInterface.destroyInstance() plus the long defense-in-depth comment that explained them — the close-callback is now safe regardless of when libuv runs it.

🧪 How was it tested?

  • Rebuilt transcription-whispercpp for ios-arm64 against qvac-lib-inference-addon-cpp@1.2.0 from the registry (no overlay-port, no patched destructor).
  • Ran the full tests-qvac transcription suite on a physical iPhone via npx qvac-test run:local:ios --filter transcription:
    • 20/20 tests passed, 100% success rate, 62.35s total duration, peak task_vm_info.physFootprint ≈ 417 MB.
    • 15 non-streaming + 2 batch/metadata-streaming + 3 transcribeStream event tests, including transcribe-stream-events-happy/disabled/invalid (which exercise the streaming destruction path that originally triggered the crash on iOS bare-kit with 1.1.7).
  • Verified end-to-end that the binary running on-device was the one rebuilt for this PR: Mach-O UUID of the .bare installed into the consumer matched the UUID of the freshly-built prebuilds/ios-arm64/qvac__transcription-whispercpp.bare (the framework only differs in code signature).

💥 Breaking Changes

None. Public JS API (WhisperInterface, runStreaming, transcribeStream, unload, etc.) is unchanged. The whisper-local C++ subclass was an internal implementation detail.

🔌 API Changes

None.

📦 Version bump

⚠️ packages/transcription-whispercpp/package.json still reports 0.7.0. This PR reverts a 0.7.0 fix and needs a follow-up bump (suggested 0.7.1) plus a matching CHANGELOG.md entry, e.g.:

## [0.7.1]

### Removed
- Reverted the whisper-local `WhisperOutputCallBackJs` workaround introduced in `0.7.0` and the `setImmediate` defense-in-depth in `WhisperInterface.destroyInstance()`. The upstream fix in `qvac-lib-inference-addon-cpp@1.2.0` (synchronous `js_delete_reference()` inside `~OutputCallBackJs()`) resolves the iOS bare-kit teardown race directly, so the local override is no longer needed. Verified by running all 20 `tests-qvac` transcription tests on a physical iPhone.

@freddy311082 freddy311082 requested review from a team as code owners May 20, 2026 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tier1 verified Authorize secrets / label-gate in PR workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants