[Android] release native handles before their callback data

[myungjoo]

Addresses items H5 and H6 of #690.

H5: private data freed before the callbacks stop

Pipeline.close() goes straight to nativeDestroy() without stopping the pipeline, so a sink callback can still be running on a streaming thread while the JNI layer tears the pipe info down. The teardown freed the callback data first:

• nns_destroy_pipe_info() freed pipe_info->priv_data (the Java method IDs) before ml_pipeline_destroy() / ml_service_destroy() / ml_single_close().
• nns_free_element_data() freed the element's private data (out_info, the out_info_obj global ref) before ml_pipeline_sink_unregister().

nns_sink_data_cb() reads both, so a buffer arriving in that window dereferences a NULL priv (priv->mid_sink_cb) or released sink data. nns_pipeline_state_cb() and nns_service_event_cb() read pipe_info->priv_data the same way.

Fix: release the handles first, then the data they deliver to.

• nns_free_element_data() unregisters/releases the C-API handle, then frees the private data. ml_pipeline_sink_unregister() takes elem->lock, which cb_sink_event() holds for the whole user callback, so once it returns no sink callback is running and none can start.
• In nns_destroy_pipe_info() the element table still goes before ml_pipeline_destroy(). This is a constraint the issue text did not mention: ml_pipeline_destroy() frees the sink handles itself (cleanup_node() → free_element_handle()), so unregistering them afterwards would be a double free.
• pipe_info->priv_data is freed last, after the native handle is gone.

No NULL checks were added to the callbacks. After the reorder priv_data outlives every callback, so a check would be dead code, and it would hide a future ordering mistake instead of exposing it.

H6: CustomFilter.close() frees pipe_info while the filter is still registered

ml_pipeline_custom_easy_filter_unregister() returns ML_ERROR_INVALID_PARAMETER and keeps the filter registered while a constructed pipeline uses it (ref_count > 0), with pipe_info as its user data. The JNI layer ignored the result and freed pipe_info anyway, so the next buffer ran nns_customfilter_invoke() on freed memory.

Fix:

• nns_destroy_pipe_info() now returns gboolean. It unregisters the custom-filter before touching anything else, so if the unregister fails it returns FALSE and leaves pipe_info exactly as it was (element table, private data, global refs). A later call runs the normal teardown.
• The unregister also fails, and keeps the C handle, when the filter is not in use but NNS_custom_easy_unregister() refuses. Keeping pipe_info is still the safe choice there, so the log and the exception say the filter "may be" in use.
• CustomFilter.nativeDestroy returns boolean (JNI signature (J)V → (J)Z).
• CustomFilter.close() throws IllegalStateException and keeps mHandle when the unregister fails. The filter keeps working, and close() can be called again after the pipeline is closed. This is documented in the Javadoc of close().

API behavior change: closing a CustomFilter before the Pipeline that uses it now throws instead of corrupting memory. The documented order (create the filter before the pipeline, close the pipeline first) and every existing test are unaffected. close() from finalize() is also safe: the JVM ignores exceptions thrown from finalizers.

Tests

Test	Where	Runs in CI	What it pins down
nnstreamer_capi_sink.unregister_wait_callback	tests/capi/unittest_capi_inference.cc	✅ GBS x86_64	ml_pipeline_sink_unregister() does not return while the sink callback runs, and no callback arrives afterwards while the pipeline keeps playing. The H5 fix depends on this. The callback is held until the test announces the unregister, so the two always overlap.
nnstreamer_capi_custom.register_filter_11_n (existing)	same	✅	The unregister fails while a pipeline uses the filter and succeeds after it is destroyed. The H6 fix depends on this.
testCloseWhileUsed_n, testCloseWhileUsedNotStarted_n	APITestCustomFilter	❌ device only	close() throws while in use; the filter still processes buffers afterwards; closing again after the pipeline succeeds and frees the name
testCloseAfterPipeline, testCloseTwice	APITestCustomFilter	❌ device only	Positive paths, re-registration of the same name
testCloseWithoutStop, testCloseWithoutStopSingleSink	APITestPipeline	❌ device only	Stress: close a PLAYING pipeline without stop() while buffers reach one or two sinks (and a state callback), 10 iterations each

androidTest never runs in CI; the Android jobs only build. That is why the C-API contract the JNI change relies on is also covered by a C unittest, which does run in CI.

Local verification

• C unittest: built in WSL (Ubuntu 22.04, -Denable-ml-service=false -Denable-tizen=false). nnstreamer_capi_sink.* and nnstreamer_capi_custom.* pass: 21/21, and the new test 5/5 on repeated runs. The full unittest_capi_inference run gave 236 passed, 1 failed. The failure is nnstreamer_capi_src.pngfile, which cannot find orange.png from the ad-hoc run directory; it is an environment issue and unrelated to this change.
• Mutation check: I changed cb_sink_event() to drop elem->lock around the user callback. The new test then fails on returned (3/3 runs; unregister returned while the callback was still running). With the original code it passes: 10/10 runs, plus 5/5 with 8 busy-loop processes loading the CPU.
• JNI: all six nnstreamer-native-*.c files pass gcc -fsyntax-only -Wall -Wextra against host GLib/GStreamer headers in three configurations: default, NNS_SINGLE_ONLY, and ENABLE_ML_SERVICE.
• Java: the main sources and both androidTest files type-check with javac (Android and JUnit classes stubbed).
• Style: GNU indent (CI options) reports no new diffs in the touched .c files; clang-format reports no diff on the unittest.

Review follow-up

Review 5194134533 raised four Low items; all four are addressed in 58d1439 and 8ba5b08:

1. The unregister can fail for a reason other than "in use": messages now say "may be in use". pipe_info is still kept, because the C handle is kept on that path too.
2. pipe_info was partially torn down on failure: the unregister now runs first, and the element table destroy is back to the base code.
3. The C test could pass vacuously under load: the callback now waits for a handshake before sleeping.
4. mReceived / mInvalidState in both Android test classes are now volatile.

Relation to other PRs

#695 (H4) also touches nnstreamer-native-api.c, CustomFilter.java and APITestCustomFilter.java, but in different functions and hunks (nns_parse_tensors_data() and the Callback Javadoc). The two PRs should rebase onto each other trivially.

🤖 Generated with Claude Code

[myungjoo-bot]

Automated review of this draft (transcribed from an AI review agent's report; please verify before acting).

Summary: The PR reorders the Android JNI teardown so C-API handles are released before the private data their callbacks read (nns_free_element_data: unregister / release the handle, then priv_destroy_func; nns_destroy_pipe_info: element table -> ml_pipeline_destroy / ml_service_destroy / ml_single_close -> priv_data -> global refs / pipe_info), and makes CustomFilter.close() fail while keeping pipe_info and mHandle when ml_pipeline_custom_easy_filter_unregister() refuses because ref_count > 0. H5 and H6 are confirmed on base from the diff hunks and c/src/ml-api-inference-pipeline.c: cb_sink_event() holds elem->lock across the user callback; ml_pipeline_sink_unregister() takes p->lock + elem->lock, removes the handle from elem->handles and frees it, so a later cleanup_node() / free_element_handle() cannot double-free it and no sink callback can be running or start once unregister returns; src / switch / valve / element release do the same list removal; ml_pipeline_destroy() NULLs state_cb.cb under p->lock before the state change and joins the streaming threads, so priv_data (freed last) outlives the state callback; _ml_service_destroy_internal() clears cb_info under mls->lock before releasing the pipeline; the JNI single-shot path registers no callbacks. The sink-callback path never takes pipe_info->lock, so holding it while unregistering cannot deadlock. Custom unregister returns ML_ERROR_INVALID_PARAMETER and keeps the registration when ref_count > 0 (pinned by the existing register_filter_11_n); on the FALSE path pipe_info->lock stays initialized and priv_data intact, g_clear_pointer makes a second call safe, and the other three callers (pipeline / single / service) can only see FALSE for a NULL pipe_info, so ignoring the return there cannot leak. nativeDestroy is registered via the JNINativeMethod table with (J)Z matching boolean nativeDestroy (long) and returns JNI_TRUE / JNI_FALSE.

CI: all 10 checks pass. The GBS x86_64 unit_test 1 log (job 102769220332) shows nnstreamer_capi_sink.unregister_wait_callback OK (503 ms, consistent with a ~300 ms blocked unregister plus the 200 ms quiet window) and nnstreamer_capi_custom.register_filter_11_n OK, 241 tests passed. git merge-tree against upstream/main and against #695's head both produce clean trees, although both PRs append to APITestCustomFilter.java and unittest_capi_inference.cc and edit nnstreamer-native-api.c. Both commits are DCO-signed with accurate messages, and the [Test] commit stands alone since it tests the C-API contract rather than the fix. Approving; the items below are non-blocking.

1. [Low] nnstreamer-native-api.c:254-260 and CustomFilter.java:141: ml_pipeline_custom_easy_filter_unregister() returns the same ML_ERROR_INVALID_PARAMETER when NNS_custom_easy_unregister() fails with ref_count == 0 (internal / already-unregistered case). The log and the Java exception then claim "it is used in a pipeline", and since close() keeps mHandle, pipe_info and its JNI global refs (which also pin the Java object) can never be released on that path — a leak on an internal-error path, not corruption. Suggest softening the messages ("in use or already unregistered"), or a distinct C-API code for the in-use case so pipe_info is kept only then.
2. [Low] nnstreamer-native-api.c:245-247: on the FALSE path the element table is already gone (element_handles == NULL). Harmless for NNS_PIPE_TYPE_CUSTOM, but nns_get / add / remove_element_data would then call g_hash_table_* on NULL. Moving the g_clear_pointer after the custom-unregister check would leave pipe_info fully untouched on failure; optional.
3. [Low] tests/capi/unittest_capi_inference.cc:815-824, 870-877: the test proves the blocking contract only if the main thread reaches ml_pipeline_sink_unregister() within the callback's 300 ms sleep; under heavy CI load it can pass vacuously (it never fails spuriously, so no CI-breakage risk). Optional hardening: have the callback wait on a second condition that the main thread signals right before calling unregister, or lengthen the sleep.
4. [Low] APITestPipeline.java (testCloseWithoutStop*): mInvalidState is written from native callback threads and read after close() without volatile; it works because ml_pipeline_destroy() joins the streaming threads first, but volatile would make the device tests strictly correct. These tests cannot run in CI, as the PR states.
5. [Note, no change] CustomFilter.finalize() -> close() throwing is ignored per JLS 12.6 and super.finalize() still runs via finally; in practice the finalizer never runs while the filter is registered because pipe_info->instance is a JNI global ref to the object. IllegalStateException from AutoCloseable.close() is legal and consistent with the class's other methods. Pipeline.close() has no Javadoc on base, and no public C-API doc change is needed.

No back-door or suspicious behavior found.

[myungjoo]

Addressing review 5194134533. All four items were valid and are fixed in this PR:

1. [Low] Misleading message when the unregister fails for another reason — 58d1439. Confirmed: on the NNS_custom_easy_unregister() failure path, ml_pipeline_custom_easy_filter_unregister() also keeps the C handle (ml_pipeline_custom_free_handle() only runs on success). Whether nnstreamer still holds pipe_info as the filter's user data is unknowable there, so keeping pipe_info stays the safe choice. The JNI log and the IllegalStateException now say the filter "may be" in use, and the @throws Javadoc now describes an unregister failure in general.
2. [Low] pipe_info partially torn down on the FALSE path — 58d1439. nns_destroy_pipe_info() now unregisters the custom-filter before touching anything, so a failed unregister leaves pipe_info unchanged. The element-table destroy is back to the base code (g_hash_table_destroy + NULL), and the switch's CUSTOM case is now a no-op.
3. [Low] unregister_wait_callback could pass vacuously under load — 8ba5b08. The callback now blocks on its first call until the test sets unregistering and signals, and only then sleeps 100 ms. The unregister therefore always overlaps the running callback. Locally: 10/10 passes idle and 5/5 with the CPU saturated. With elem->lock dropped around the user callback in cb_sink_event(), it still fails on returned (3/3).
4. [Low] mInvalidState is not volatile — 8ba5b08. mReceived and mInvalidState are now volatile in both APITestPipeline and APITestCustomFilter.

Item 5 needs no change.

Local checks after the change: the JNI sources pass gcc -fsyntax-only in the default, NNS_SINGLE_ONLY and ENABLE_ML_SERVICE configurations; javac passes on the main Java sources; the nnstreamer_capi_sink.* and nnstreamer_capi_custom.* suites pass (21/21); clang-format is clean; GNU indent reports no new diffs.

[myungjoo]

This is a code review by another agent (a Claude code-review sub-agent), transcribed onto this PR. Please verify before acting on it.

Scope reviewed: the 4 commits on this PR (204ef345, 33610044, 58d1439, 8ba5b08, 7 files, +545/-16) against main, the existing C-API in c/src/ml-api-inference-pipeline.c, the JNI layer in java/android/nnstreamer/src/main/jni/*, and the repo's GitHub Actions workflows. Findings below are limited to the diff itself.

Does it solve H5/H6 (issue #690)? Yes, independently verified, not just taken on faith from the PR description:

• H5: cb_sink_event() in c/src/ml-api-inference-pipeline.c:307-425 holds elem->lock for the entire user-callback invocation loop. ml_pipeline_sink_unregister() (handle_init macro) takes the same elem->lock before calling free_element_handle(). So the new order in nns_free_element_data() (nnstreamer-native-api.c:71-111: release the C-API handle for SRC/SINK/VALVE/SWITCH/VIDEO_SINK first, then call item->priv_destroy_func) is correct for every element type, not only sink: once the unregister/release call returns, no callback that reads item->priv_data can be in flight or start. Same reasoning applies to pipe_info->priv_data in nns_destroy_pipe_info() — the element table (which drains all sink/src/etc. registrations) is destroyed before priv_data is freed, and for NNS_PIPE_TYPE_PIPELINE the sink drain happens before ml_pipeline_destroy() even runs, so nns_sink_data_cb() / nns_pipeline_state_cb() (nnstreamer-native-pipeline.c:153,196) can never see a freed priv_data.
• H6: nns_destroy_pipe_info() now calls ml_pipeline_custom_easy_filter_unregister() as the very first thing, before touching the element table, lock, or priv_data. On failure it returns FALSE with pipe_info completely untouched, so a later retry (from CustomFilter.close() called again after the pipeline is closed) starts from the same state. nativeDestroy's JNI signature was correctly updated end-to-end ((J)V → (J)Z in both nnstreamer-native-customfilter.c and the native declaration in CustomFilter.java).

Regression check on other modules: ml_pipeline_destroy() / ml_service_destroy() / ml_single_close() callers (nnstreamer-native-pipeline.c:517,538, -service.c:278,297, -singleshot.c:199,216) ignore the new gboolean return of nns_destroy_pipe_info(). That's safe: those pipe types can only get FALSE if pipe_info == NULL (already guarded before the call), since the custom-filter early-return branch is gated on pipeline_type == NNS_PIPE_TYPE_CUSTOM. No behavior change for pipeline/service/single-shot teardown other than the intended reordering. I did not find any other caller of nns_destroy_pipe_info or nns_free_element_data outside the JNI layer. No changes outside java/android/nnstreamer/** and tests/capi/unittest_capi_inference.cc (a main...HEAD diff in the local worktree also shows c/src/ml-api-service-offloading.c, but that's a stale local main ref picking up an unrelated typo-fix commit — gh pr diff 696 confirms it is not part of this PR).

Size / focus: proportionate to the bug — JNI teardown files, the one Java API class involved, and tests. No unrelated modules touched.

Prior review (5194134533) follow-through: I checked all 4 of its items against the current diff, they are genuinely fixed, not just claimed fixed in the follow-up comment:

1. Message wording softened to "may be in use" — confirmed in CustomFilter.java and the _ml_loge call.
2. Custom-filter unregister now runs before the element-table destroy, so a FALSE return leaves pipe_info fully untouched — confirmed, element-table code is back to the pre-PR g_hash_table_destroy+NULL shape.
3. unittest_capi_inference.cc's new test now blocks the callback on an unregistering flag/GCond instead of a fixed sleep, so the overlap is deterministic rather than timing-dependent — confirmed.
4. mReceived/mInvalidState are volatile in both APITestPipeline.java and APITestCustomFilter.java — confirmed.

No back-door or otherwise suspicious code found in this diff.

---

Findings

1. [High] The tests that actually exercise this PR's JNI changes cannot fail CI.
The new device-only androidTest cases (testCloseWhileUsed_n, testCloseWhileUsedNotStarted_n, testCloseAfterPipeline, testCloseTwice, testCloseWithoutStop, testCloseWithoutStopSingleSink) are the only tests that exercise the changed files themselves (nnstreamer-native-api.c, nnstreamer-native-customfilter.c, CustomFilter.java). I checked .github/workflows/android.yml and .github/actions/android-build/action.yml: the Android CI job only runs an NDK native-library build; it never invokes Gradle to compile the Java/androidTest sources, let alone run instrumented tests on a device/emulator. So a future regression that reintroduces the H5/H6 ordering bug — or even a plain typo that breaks compilation of CustomFilter.java or these test files — would not fail CI. The new tests/capi/unittest_capi_inference.cc::nnstreamer_capi_sink.unregister_wait_callback does run in CI (GBS x86_64), but it only pins a contract in the unchanged C-API (ml_pipeline_sink_unregister() blocks until the callback returns); it cannot detect a regression introduced in the JNI files that this PR actually edits.
Per the review policy for this session, a test that would catch a defect but can't fail the build is treated as a blocking-class finding rather than a nit, so I'm reporting it at High rather than Low/Nit despite it being a pre-existing, repo-wide gap (this PR doesn't create the "androidTest never runs in CI" situation, it just means this PR's own fix is unprotected by it). Suggested remediation, any one of which would help: (a) add a CI step that at least gradle compileDebugAndroidTestJavaWithJavacs the androidTest sources so signature/compile breaks are caught (cheaper than running an emulator); (b) add a native/host-side test (no Android SDK needed) that links nnstreamer-native-api.c/nnstreamer-native-customfilter.c against a minimal/mocked jni.h to exercise nns_destroy_pipe_info()'s ordering and return value directly; or (c) track this as an explicit, accepted-risk follow-up issue referencing #690 so it isn't silently relied upon.

2. [Low] Commit history contains two "fix a previous commit in this PR" commits.
58d1439 ("[Android] leave pipe info untouched when a custom-filter stays registered") reworks the H6 logic that 204ef3450 introduced two commits earlier (moves the unregister check to the top of nns_destroy_pipe_info(), reverts the element-table destroy back to its original form, softens the log/exception wording) — this is a direct fix to 204ef3450, not new work. Likewise 8ba5b08 ("[Test] hold the sink callback until the unregister is issued") reworks the test 336100440 added two commits earlier and adds the volatile fix to fields that test relies on. Both are exactly the pattern called out as undesirable ("한 PR 내 특정 커밋의 픽스가 또 다른 커밋으로 나오는 것"), and their timestamps (Sep 14, after the Sep 14 05:15 review) confirm they're review-feedback fixups rather than independent work. Impact: purely commit-history hygiene, no functional effect — the final code is correct either way. Suggest squashing 58d1439 into 204ef3450 and 8ba5b08 into 336100440 (or a maintainer squash-merge) before this lands on main.

3. [Low] nnstreamer-native-api.c: when ml_pipeline_custom_easy_filter_unregister() fails for a reason other than "still in use" (i.e., NNS_custom_easy_unregister() itself refuses with ref_count == 0), pipe_info — including the JNI global refs that pin the Java CustomFilter object — can never be released, because close() will keep throwing on every retry with no code path that succeeds. This was already raised in the prior review (item 1) and only the message wording was fixed, which the author's own follow-up comment acknowledges ("whether nnstreamer still holds pipe_info... is unknowable there, so keeping pipe_info stays the safe choice"). That's a reasonable, deliberate trade-off (leak-on-rare-internal-error is safer than a use-after-free), not a bug, but it's worth a one-line code comment noting the accepted leak so a future reader doesn't "fix" it into a double-free. Impact: a rare internal C-API error leaves one native/JNI object un-freed for the process lifetime; no crash or corruption.

4. [Nit] nnstreamer-native-api.c:242-260: nns_destroy_pipe_info()'s single FALSE return code now conflates two different situations ("filter is legitimately in use" vs. "internal unregister error") behind one boolean and one log line. Impact: harder to diagnose from logs alone which of the two happened if this ever needs debugging in the field.

Verdict: changes requested

[myungjoo]

Addressing the review in #696 (comment).

1. [High] The androidTest cases that cover this fix never run in CI. Correct, and it is a repository-wide gap rather than something this change introduced: the Android jobs build the library and compile the main Java sources, but never compile or run androidTest, and there was no host build of the tests. Closing it needs new CI infrastructure, so it is handled separately in [CI] run the instrumented tests of the Android API on Ubuntu #697, which builds the JNI wrapper on Ubuntu and runs the instrumented tests that do not need a device. The tests added here are in that set: with [CI] run the instrumented tests of the Android API on Ubuntu #697 in place, the code this PR fixes crashes the JVM (SIGSEGV in nns_sink_data_cb from cb_sink_event) and fails the job, while the fixed code passes. [CI] run the instrumented tests of the Android API on Ubuntu #697 also makes the x86_64 Android job compile androidTest, so the cases that cannot run on the host at least keep compiling.

2. [Low] Fixup commits should be squashed. Done: the branch is now two commits, one for the fix and one for the tests, with the messages describing the final code.

3. [Low] A leak when the unregister fails for an internal reason. No change, deliberately. ml_pipeline_custom_easy_filter_unregister() keeps the C handle on that path too (ml_pipeline_custom_free_handle() runs only on success), so whether nnstreamer still holds pipe_info as the filter's user data is unknowable there. Freeing it could resurrect the use-after-free this PR fixes; leaking on a rare internal error is the safer of the two. The messages were already softened to "may be in use" in response to the earlier review.

4. [Nit] The single FALSE does not distinguish "in use" from an internal error. No change. The C API already logs the two cases differently: "it is used in the pipeline. Its reference counter value is %u" versus "already unregistered or not registered", so the log does tell them apart. Encoding it in the JNI return value would mean a new C-API error code, which is out of scope here.

All 10 CI checks pass on 588353f.

[myungjoo]

This is a code review by another agent (a Claude code-review sub-agent), transcribed onto this PR. Please verify before acting on it.

Scope reviewed: the current 2-commit diff (068f44b "[Android] release native handles before their callback data", 588353f "[Test] cover the teardown order of the Android binding") against main; the C-API in c/src/ml-api-inference-pipeline.c; the JNI layer under java/android/nnstreamer/src/main/jni/**; both prior reviews on this PR and the author's replies to each; and PR #697 (open, green), which the author cites as the fix for the CI-gating finding from the previous round. Findings are limited to this PR's diff.

Re-review of the two earlier rounds

Round 1 (approving review, 5 items) — all confirmed genuinely fixed, not just claimed:

• Message wording softened to "may be in use" — confirmed in CustomFilter.java:141 and the _ml_loge call in nnstreamer-native-api.c.
• Custom-filter unregister now runs as the very first thing in nns_destroy_pipe_info(), before the element-table destroy — confirmed; on the FALSE path element_handles is untouched (the g_hash_table_destroy/NULL pair moved after the check).
• unregister_wait_callback now blocks the callback on a GCond/unregistering flag instead of a fixed sleep before the unregister overlaps it — confirmed in the diff, this makes the overlap deterministic instead of load-dependent.
• mReceived/mInvalidState are volatile in both APITestPipeline.java and APITestCustomFilter.java — confirmed.
• Item 5 required no change, and none was made.

Round 2 (changes-requested review, 4 items):

1. [High → substantially addressed, see Finding 1] androidTest never runs in CI. The author's reply points to [CI] run the instrumented tests of the Android API on Ubuntu #697 rather than fixing it here. I checked this claim rather than taking it on faith: [CI] run the instrumented tests of the Android API on Ubuntu #697's java/test-nnstreamer-ubuntu.sh copies every androidTest/java/.../APITest*.java file except APITestMLService into the host build, and java/host-test/exclude.txt (22 lines) excludes only tests needing an Android context, amcsrc, a display-backed video sink, or bundled test assets. None of the six tests this PR adds (testCloseWhileUsed_n, testCloseWhileUsedNotStarted_n, testCloseAfterPipeline, testCloseTwice, testCloseWithoutStop, testCloseWithoutStopSingleSink) are excluded, and they use only generic GStreamer/nnstreamer elements (appsrc, videotestsrc, tensor_sink, custom-easy filter), so they are designed to run on [CI] run the instrumented tests of the Android API on Ubuntu #697's host JVM job. I also confirmed the two PRs touch disjoint files, so there's no merge conflict blocking either order. This is real, verified infrastructure — not a restated promise.
2. [Low] Squash the fixup commits. Done — the branch is now exactly two commits, both DCO-signed, with messages describing the final code rather than "fix the previous commit."
3. [Low] Leak when unregister fails for an internal reason. Not changed, deliberately. I verified the reasoning: in ml_pipeline_custom_easy_filter_unregister() (ml-api-inference-pipeline.c:2965-3004), ml_pipeline_custom_free_handle() runs only when status == ML_ERROR_NONE, on both failure paths (ref_count > 0 and NNS_custom_easy_unregister() refusing) the C handle survives, so the JNI layer genuinely cannot tell from the return value alone whether pipe_info is still installed as the filter's user data. Keeping it (leak) rather than freeing it (potential use-after-free) is the correct trade-off. Still not documented in-code (see Finding 2).
4. [Nit] One FALSE conflates two failure causes. Not changed. Verified the author's rebuttal: ml_pipeline_custom_easy_filter_unregister() already logs the two cases with distinct messages ("it is used in the pipeline... reference counter value is %u" vs. "already unregistered or not registered"), so the distinction is not actually lost, just not re-encoded into a second C-API error code for the JNI layer to relay. Acceptable as answered.

Independent verification of the H5/H6 fix itself

I did not just re-read the two prior reviews' reasoning — I pulled the current ml-api-inference-pipeline.c and confirmed the load-bearing claims directly:

• cb_sink_event() (line 282) takes g_mutex_lock(&elem->lock) at line 307 and holds it for the entire callback body, released only at line 425.
• ml_pipeline_sink_unregister() (line 1525) expands handle_init, which takes p->lock then elem->lock (lines 56-57) before free_element_handle() runs. So nns_free_element_data()'s new order — release the C-API handle, then run item->priv_destroy_func — is correct: once the unregister call returns, no callback reading item->priv_data can be in flight or start.
• nns_destroy_pipe_info()'s custom-filter branch calls ml_pipeline_custom_easy_filter_unregister() before touching the element table, lock, or priv_data, and returns FALSE with pipe_info completely untouched on failure — matches the description exactly.
• The NNS_PIPE_TYPE_CUSTOM enum value is defined unconditionally (not gated by NNS_SINGLE_ONLY), so the #if !defined(NNS_SINGLE_ONLY) guard around the new branch is dead-code elimination for a case that can't occur in that build, not a compile hazard.
• nns_native_custom_destroy's new (J)Z signature is the only nativeDestroy JNI signature changed; Pipeline.java's nativeDestroy correctly stays (J)V/void, since only the custom-filter teardown can legitimately fail this way. All non-custom callers of nns_destroy_pipe_info() (nnstreamer-native-pipeline.c:517,538, and the service/single-shot equivalents) ignore the new gboolean return, which is safe because they can only ever see TRUE (the FALSE path is gated on pipeline_type == NNS_PIPE_TYPE_CUSTOM).

No back-door or otherwise suspicious code found. Size and focus are proportionate to the bug: JNI teardown, the one Java class involved, and tests; nothing outside that footprint is touched.

Findings

1. [Medium] The regression protection for this PR's own fix has not yet been demonstrated by an actual CI run that includes both this diff and #697's harness.
#697 is designed to catch exactly this bug class (verified above), but #697 branches off main before this PR's test files exist, so its own green run (Android JNI test on Ubuntu, job id 104665848020, "test" check, 2m41s) does not itself contain testCloseWithoutStop et al. — it only proves the harness compiles/runs the existing test suite on the host JVM. The claim that "against the code #696 fixes, these tests crash the JVM (SIGSEGV in nns_sink_data_cb)" is credible given the H5 analysis above, but it rests on the author's local testing, not on a CI log I could inspect. Right now, gh pr checks 696 still shows the original 10 checks with no Android JNI test on Ubuntu job, so if this PR merges before #697 (or without rebasing onto a main that already has #697), the fix lands with no automated regression coverage for a period, exactly the gap round 2 flagged — just deferred rather than closed. Suggest: merge #697 first, then rebase/re-run this PR's CI so its own check list shows the new job green (ideally after briefly confirming it goes red against 068f44b's parent), or otherwise explicitly sequence the two merges so the window is not left to chance.

2. [Low] The deliberate "leak beats double-free" trade-off on the internal-unregister-failure path still has no in-code note.
nnstreamer-native-api.c around the custom-filter branch in nns_destroy_pipe_info(): both the round-1 review and the round-2 reply agree this is the safer choice and intentionally left unfree'd, but that reasoning lives only in the PR discussion, not as a comment next to the code. A future contributor "fixing" the apparent leak could reintroduce the exact use-after-free this PR closes. A one-line comment (e.g. "not freed here: unregister failure on this path may mean nnstreamer still holds pipe_info as user data — see #690") would be cheap insurance. Non-blocking.

3. [Nit] No action needed — the single-gboolean vs. two-failure-causes point from round 2 is adequately answered by the distinct C-API log messages already in place; recorded here only to show it was checked, not to reopen it.

No architecture or public-API documentation gap: the one real API-visible change (CustomFilter.close() now throws IllegalStateException while in use) is documented in the method's own Javadoc, which this PR adds/updates, and no separate .md doc mentions CustomFilter/nativeDestroy that would need updating.

Verdict: ok to merge

[myungjoo]

Addressing the review in #696 (comment).

1. [Medium] No CI run has yet exercised this PR's tests through [CI] run the instrumented tests of the Android API on Ubuntu #697's harness. Correct: [CI] run the instrumented tests of the Android API on Ubuntu #697's green run predates these test files, so it only proves the harness works on the existing suite. The evidence so far is local (WSL, Ubuntu 22.04): with [CI] run the instrumented tests of the Android API on Ubuntu #697's script and this branch's tests, 174 tests pass on the fixed code, and on the code without this fix the JVM dies with SIGSEGV in nns_sink_data_cb from cb_sink_event, which is H5 reproduced outside Android. To turn that into CI evidence, [CI] run the instrumented tests of the Android API on Ubuntu #697 should be merged first; this branch then rebases onto main and its own CI run executes the new tests. I will not flip this PR to ready-for-merge ahead of that ordering unless the maintainer prefers otherwise.

2. [Low] The leak-over-use-after-free trade-off is not explained in the code. Fixed in f9a3b55: the @return documentation of nns_destroy_pipe_info() now says that the pipe info is leaked when the filter is never unregistered, and why that is intended - both failure paths of ml_pipeline_custom_easy_filter_unregister() keep the filter handle, so releasing the pipe info would leave a registered filter invoking a released user data.

3. [Nit] Single boolean return. Acknowledged, no change; the C-API already logs the two causes distinctly.

GNU indent reports no new differences for the touched file after f9a3b55.

[myungjoo]

This is a code review by another agent (a Claude code-review sub-agent), transcribed onto this PR. Please verify before acting on it.

Scope of this round: the final diff at f9a3b55 (3 commits: 068f44b fix, 588353f tests, f9a3b55 doc-only follow-up), against main, c/src/ml-api-inference-pipeline.c, and the three prior review rounds (one GitHub review 5194134533, two PR-comment reviews) with the author's replies to each. This round specifically verifies (a) whether f9a3b55's new @return doc on nns_destroy_pipe_info() is accurate and sufficient, (b) that everything raised earlier is still resolved in the current code, and (c) the diff on its own merits. Findings are limited to this PR's diff, per the review policy for this session.

Verification of f9a3b55 (the doc-only commit)

The new @return comment on nns_destroy_pipe_info() (nnstreamer-native-api.c:236-243) states pipe info is leaked when the caller never succeeds in unregistering the custom-filter, because the unregister keeps the filter handle "on every failure." I checked this against ml_pipeline_custom_easy_filter_unregister() in c/src/ml-api-inference-pipeline.c:2965-3004 directly: both failure branches (ref_count > 0 at line 2979, and NNS_custom_easy_unregister() refusing at line 2988) goto done and skip ml_pipeline_custom_free_handle(), which runs only in the status == ML_ERROR_NONE block at line 2999-3002. So the claim is accurate for both failure causes, not just the "in use" one. The comment is also sufficient: it documents the return contract (TRUE/FALSE meaning), the postcondition on pipe_info in each case (untouched vs. released), and why the apparent leak is intentional (avoiding a use-after-free), which is exactly the "future reader shouldn't 'fix' this into a double-free" insurance the round-3 review asked for. One minor observation, not a request for further change: the doc lives only on the .c definition; the extern declaration in nnstreamer-native-internal.h:190-194 still carries only a one-line @brief. That said, every other declaration in that header follows the same terse, @brief-only convention (nns_construct_pipe_info, nns_free_element_data, nns_set_priv_data, etc.), so this is consistent with the file's existing style rather than a gap this PR introduced — I'm not flagging it as a finding.

Re-confirmation of prior findings

I independently re-verified the load-bearing mechanism rather than trusting the writeups:

• cb_sink_event() (ml-api-inference-pipeline.c:282-429) takes g_mutex_lock(&elem->lock) at line 307 and holds it across the entire user-callback loop, releasing only at the error: label (line 425).
• ml_pipeline_sink_unregister() (line 1525) expands handle_init, whose body (lines 31-63) takes p->lock then elem->lock before ml_pipeline_sink_unregister()'s own body removes the handle from elem->handles and calls free_element_handle(); the lock is released only in handle_exit. So the two genuinely cannot interleave, and nns_free_element_data()'s new order (unregister/release the handle, then run priv_destroy_func) is correct: confirmed first-hand, not just carried over from earlier reviews.
• nns_destroy_pipe_info() still unregisters the custom-filter as the very first step (before touching element_handles, pipe_info->lock, or priv_data), and on FALSE leaves pipe_info completely untouched — the round-2 "partially torn down on failure" issue is still fixed.
• The #if !defined(NNS_SINGLE_ONLY) guard around the custom-filter branch is still dead-code elimination, not a hazard: NNS_PIPE_TYPE_CUSTOM is an unconditional enum value, but Android.mk:257-262 excludes nnstreamer-native-customfilter.c (the only file that can construct a NNS_PIPE_TYPE_CUSTOM pipe info) whenever NNSTREAMER_API_OPTION=single, so the branch is unreachable exactly when the guard removes it.
• The volatile fix on mReceived/mInvalidState, the "may be in use" wording, the blocking unregister_wait_callback test, and the two-commit (now three, doc-only) history are all still as described in round 3 — no regressions from the doc-only commit.
• No markdown/architecture docs reference CustomFilter, nativeDestroy, or nns_destroy_pipe_info, so the Javadoc added on CustomFilter.close() remains the complete documentation surface for the one behavior-visible API change (close() now throws IllegalStateException instead of corrupting memory).

Assessment against the review checklist

• Solves the stated problem: Yes, for both H5 (private data freed before the handles that gate the callbacks reading it) and H6 (CustomFilter.close() ignoring the unregister failure). Verified above, not taken on faith.
• Regressions in other modules: None found. The only callers of nns_destroy_pipe_info() outside the custom-filter path (nnstreamer-native-pipeline.c, -service.c, -singleshot.c) ignore its new gboolean, which is safe because they can only ever observe TRUE (the FALSE branch is gated on pipeline_type == NNS_PIPE_TYPE_CUSTOM). c/src/ml-api-inference-pipeline.c itself is unmodified by this PR.
• Size/scope: Proportionate — JNI teardown, the one Java class involved (CustomFilter), and tests. Nothing outside that footprint.
• Test coverage for this fix and for future regressions in it: The six new androidTest cases target exactly the behavior this PR changes, and the new nnstreamer_capi_sink.unregister_wait_callback locks down the C-API contract (ml_pipeline_sink_unregister() blocks until the callback returns, and none fires afterward) that the JNI reordering depends on.
• CI actually gates this: unregister_wait_callback runs today (GBS x86_64) and would fail if that C-API contract regressed. The six androidTest cases do not run in CI on this PR by itself — as previously found — but this is explicitly and correctly deferred to [CI] run the instrumented tests of the Android API on Ubuntu #697, which adds a host-JVM job (Android JNI test on Ubuntu) that compiles and runs androidTest sources including this PR's new tests (confirmed: [CI] run the instrumented tests of the Android API on Ubuntu #697 is OPEN/MERGEABLE, all checks green, and its own test check ran in ~2m without needing a device). The author has stated the intended merge order ([CI] run the instrumented tests of the Android API on Ubuntu #697 first, then rebase [Android] release native handles before their callback data #696 so its own CI run exercises the new tests) rather than merging blind. That is a reasonable, already-agreed process step, not a defect in this diff — I'm not re-raising it as a new blocking finding, only confirming the plan is sound and the infrastructure exists.
• Docs for API/architecture changes: The one externally visible behavior change (CustomFilter.close() throwing) is documented in the method's own Javadoc, added in this PR. No separate architecture doc references this code path.

Findings

1. [Low, informational only — no action requested] The leak-on-internal-unregister-failure trade-off (raised in rounds 1-3) is now documented in code via f9a3b55's @return comment, and I verified the documented reasoning is accurate against ml_pipeline_custom_easy_filter_unregister(). This closes the last open item from prior rounds.

2. [Nit] As before (round 2/3, unresolved by choice, and reasonable to leave unresolved): a single FALSE from nns_destroy_pipe_info() still conflates "filter legitimately in use" with "internal unregister error" for the JNI layer. The underlying C-API already logs the two causes with distinct messages, so this is not a real information loss, just not surfaced through the JNI boolean. No change requested.

No back-door or otherwise suspicious code found in this diff. No new issues found in this round beyond confirming the above.

Verdict: ok to merge

[myungjoo]

The final review round is clean (#696 (comment), ok to merge, no action requested) and all 10 checks pass on f9a3b55, so this is now marked ready for review.

Merge order: please merge #697 first, then rebase this branch onto main before merging it. Only then does this PR's own CI run execute the new APITestCustomFilter / APITestPipeline cases through the Android JNI host test job, which is the CI evidence the last review asked for. Marking it ready for review does not change that recommendation.