Fix JNI exception handling and resource cleanup bugs#328
Merged
Conversation
Follow-up hardening and cleanup for the concurrency/exception-safety work merged in #326, addressing every finding from its post-merge review: Correctness / concurrency: - delete(): drain the in-flight user refcount BEFORE stopping the worker and freeing the vocab-only model (previously inner resources were torn down pre-drain, leaving a narrow window where e.g. an in-flight vocab-only tokenize could use the model after llama_model_free). The closing flag is now set unconditionally. - setLogger(): fully close the callback-swap race. The trampoline counts active invocations (g_log_active) and setLogger drains them before DeleteGlobalRef-ing the old callback — an in-flight trampoline can no longer call into a just-deleted global ref. Documented that setLogger must not be called from within a log callback. - startAttachedNativeServer(): acquire the model context through acquire_jllama_context_impl (field read + user increment under g_ctx_mutex) instead of a raw handle read + fetch_add, closing the stale-pointer window against a concurrent LlamaModel.close(). - receiveCompletionJson / receiveChatCompletionChunk: release the reader map entry in the new catch paths (was leaked until close()). - parse_jstring / parse_string_array: clear the pending JNI exception on the OOM/degraded paths (the JNI spec forbids further calls — including the caller's ThrowNew — while an exception is pending). - getModelMetaJson(): read general.architecture into a char vector instead of writing the API's terminating NUL into std::string's internal terminator slot. Cleanup / de-duplication: - native_server.cpp: replace the local jstring_to_utf8 (with its poisonable one-shot std::call_once init) by reusing jllama.cpp's parse_jstring — one UTF-8 extraction path, initialized in JNI_OnLoad. - Rename the misspelled filter_ouetts_codec_tokens / k_ouetts_codec_* to filter_outetts_codec_tokens / k_outetts_codec_*. - Remove all references to the non-existent docs/plan-bugs-and-issues.md and the private triage codes (H1/H2/M2..M6/L3..L6/MED #n); comments now state the constraint directly. - Drop the stale "pinned llama.cpp is now b9941" comment (point at the GIT_TAG instead of embedding a version that goes stale). - NativeServer attach-constructor Javadoc: document that closing the model while the server runs now blocks until the server stops (the usage-reference behavior), instead of the outdated freed-state wording. - Replace the tautological OuteTtsCodecFilter.V02AndV03ShareWindow test with explicit boundary coverage in the remaining tests (window constants, off-by-one on both ends). - Reformat the C++ tree with the pinned clang-format 22.1.5 — fixes the clang-format CI check that #326 left red (REQUIRE_SERVER_CONTEXT macro continuation) and brings the merged code back into compliance. - CLAUDE.md: refresh the C++ test table (50/5, total 485). Verified: full C++ suite 485/485 green (ctest), Java suite 1405 tests 0 failures (model-gated skips only), NativeLibraryLoadSmokeTest green against the rebuilt lib, javadoc:jar green, clang-format --dry-run -Werror clean, spotless:apply no-op. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DsCs5bvL9kmJDwB8FodL2M
bernardladenthin
had a problem deploying
to
maven-central
GitHub Actions
Failure
bernardladenthin
had a problem deploying
to
startgate
GitHub Actions
Error
bernardladenthin
had a problem deploying
to
maven-central
GitHub Actions
Failure
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes several critical JNI exception handling and resource cleanup bugs:
JNI exception clearing: Added missing
ExceptionClear()calls inparse_jstring()andparse_string_array()to comply with JNI spec (most JNI calls are forbidden while an exception is pending).Resource leaks in string parsing: Fixed byte array element release logic in
parse_string_array()to ensureReleaseByteArrayElements()is always called, even on allocation failures.Logger callback use-after-free: Added
g_log_activecounter and condition variable to safely drain in-flight log callbacks before deleting the old global reference insetLogger().Model metadata buffer safety: Changed
std::stringtostd::vector<char>when callingllama_model_meta_val_str()to avoid writing the API's terminating\0to the string's internal terminator slot (undefined behavior per C++ standard).Reader cleanup on exception: Added missing
erase_reader()calls in exception handlers forreceiveCompletionJson()andreceiveChatCompletionJson()to prevent reader leaks.Teardown ordering in delete(): Moved the closing flag set and reader cleanup before the user-count wait, and ensured the wait completes before stopping the worker/freeing models. This prevents use-after-free when concurrent JNI calls are still executing during teardown.
NativeServer attach safety: Changed to use
acquire_jllama_context_impl()instead of raw handle casting, ensuring proper user reference counting soLlamaModel.close()blocks until the server exits (preventing use-after-free of the server context).Code deduplication: Moved
parse_jstring()declaration tonative_server.cppto reuse the standard UTF-8 extraction path instead of duplicating it.Test fixes: Updated OuteTTS codec filter tests to use named constants and fixed typo in function name (
filter_ouetts_codec_tokens→filter_outetts_codec_tokens).Documentation: Removed internal issue tracker references (e.g., "H2", "M4") and clarified lifecycle contracts in Javadoc.
Test plan
Related issues / PRs
Fixes multiple use-after-free and resource leak bugs identified in internal issue tracking.
Checklist
CONTRIBUTING.mdandCODE_OF_CONDUCT.mdhttps://claude.ai/code/session_01DsCs5bvL9kmJDwB8FodL2M