[C-Api] fix self-deadlock on single-shot invoke timeout

[myungjoo]

Addresses items H1 and H2 of #690.

H1 — self-deadlock after an invoke timeout (the actual defect)

ml_single_invoke() with a timeout registers the abandoned output in
destroy_data_list and, for a sub-plugin with allocate_in_invoke == TRUE,
sets data->destroy = ml_single_destroy_notify_cb.

invoke_thread() then finishes the invoke, re-takes single_h->mutex, and
releases that output with ml_tensors_data_destroy(). That call dispatches
ml_single_destroy_notify_cb(), which re-enters
ML_SINGLE_GET_VALID_HANDLE_LOCKED() and blocks on the non-recursive mutex the
same thread already holds. The macro takes the global magic lock before
the handle mutex and only releases it afterwards, so the thread parks while
holding magic and every single-shot API in the process stops.

Three call sites release such an output under the mutex — __process_output()
and the two ml_tensors_data_destroy (output) calls in invoke_thread() — but
they do not all fail the same way. Where the handle is still open, the callback
reaches the mutex and hangs. Where a close is already in progress,
ml_single_close() has zeroed magic through
ML_SINGLE_GET_VALID_HANDLE_LOCKED (single_h, single, 1), so the callback
returns ML_ERROR_INVALID_PARAMETER before touching the mutex,
_ml_tensors_data_destroy_internal() bails out on that error, and the handle
plus the framework buffers leak instead.

So: __process_output() deadlocks. The status != ML_ERROR_NONE || JOIN_REQUESTED branch deadlocks when an invoke fails after a timeout with no
close pending, and leaks when it is the close that woke it. The exit: label,
reached only on JOIN_REQUESTED, leaks. All three want the same fix.

The fix routes them through a small helper that calls __destroy_notify()
first (which clears the destroy callback) and only then
ml_tensors_data_destroy(), so the handle mutex is taken exactly once. Data
without a destroy callback keeps its existing path unchanged, and a NULL
output — reachable when the timeout fires before the invoke thread picks the
job up — is ignored as before.

Reproduction: ml_single_set_timeout() shorter than one invoke, on a
framework that allocates the output in invoke (tensorflow, custom filters with
allocate_invoke).

H2 — clearing the buffers handed back to the framework

Worth recording: the double free described in #690 H2 is not reachable
today. g_tensor_filter_single_destroy_notify() in nnstreamer already sets
mem[i].data = NULL for every output tensor, so the app's later
ml_tensors_data_destroy() finds NULL pointers and frees nothing.

But nothing in the vtable contract (tensor_filter_single.h) promises that,
and the H1 fix above now depends on it — __destroy_notify() is what makes the
following ml_tensors_data_destroy() a no-op on the buffers. So
__destroy_notify() clears the descriptor itself. The size is zeroed together
with the pointer: ml_tensors_data_set_tensor_data() on a STATIC handle
validates data_size against tensors[i].size and would otherwise memcpy()
into a NULL pointer.

Tests

Added to tests/capi/unittest_capi_inference_single.cc:

• invoke_timeout_alloc_in_invoke_p — times out twice against
  libnnstreamer_customfilter_scaler_allocator, once with the handle open (hits
  __process_output) and once with a close pending (hits the invoke_thread
  join path), then closes the handle. A regression deadlocks the process, so
  this hangs until the meson test timeout rather than failing an assertion; that
  is noted in the test.
• close_before_data_destroy_01_p — the close-before-destroy order documented
  in nnstreamer-single.h, on an allocating framework. Asserts the output
  reports no buffer after close and that writing into it is rejected instead of
  faulting.
• close_before_data_destroy_02_p — the same order on tensorflow-lite, which
  does not allocate in invoke. Asserts the buffer is still valid after close, so
  the clearing above cannot be widened to data the handle owns.

All three skip when their model or custom filter is unavailable, following the
existing convention in the file.

Verification

ml-api-inference-single.c and the test file were compiled clean
(-Wall -Wextra), and clang-format and the CI indent options report no
change in the touched regions. A full build could not be run locally — the
machine has no nnstreamer devel package or gtest — so the functional runs are
left to CI.

Scope

No public API, ABI, or documented behaviour changes. Only the single-shot
internals are touched; __destroy_notify and the new helper are static to this
file. The only externally observable difference is that an output from an
allocating framework reports size 0 after ml_single_close(), where it
previously reported the original size with a NULL pointer. ml_single_invoke_fast
outputs (the path the Android JNI binding uses) never enter the destroy list and
are untouched.

🤖 Generated with Claude Code

[myungjoo]

Code review (H1 / H2 of #690)

Note: this review was produced by a separate review agent and is transcribed here by me. The findings below were verified against this branch, upstream/main, and the nnstreamer tree (gst/nnstreamer/tensor_filter/tensor_filter_single.c).

Verdict summary

The fix itself is correct, minimal and correctly scoped. What is not yet in place is the merge gate: the added regression tests almost certainly do not execute in either PR-gating CI, and if they did, a regression would hang rather than fail. Details below.

---

1. Does it solve the stated problem? - Yes

Verified the deadlock chain and the fix:

• ML_SINGLE_GET_VALID_HANDLE_LOCKED (c/src/ml-api-inference-single.c:50-64) takes G_LOCK (magic), then blocks on g_mutex_lock (&single_h->mutex), and only unlocks magic afterwards. So a self-deadlock on single_h->mutex parks the process-wide magic lock - the "every single-shot API in the process stops" claim in the description is accurate.
• All three re-entrant call sites are covered: __process_output() (timeout branch), the status != NONE || JOIN_REQUESTED branch of invoke_thread(), and the exit: label of invoke_thread().
• I grepped every ml_tensors_data_destroy() / _ml_tensors_data_destroy_internal() call in ml-api-inference-single.c. The remaining ones under single_h->mutex (:571 input, :1622 _in, :1628 _out on the synchronous error path) all operate on handles whose destroy is NULL (_in is a ml_tensors_data_clone, _out only gets destroy set inside __process_output / set_destroy_notify). No fourth site is missed.
• ml_single_close()'s g_list_foreach (..., __destroy_notify, ...) runs after ML_SINGLE_HANDLE_UNLOCK, and calls __destroy_notify directly rather than ml_tensors_data_destroy, so it is unaffected.

The H2 hardening is also correct: ml_tensors_data_set_tensor_data() (c/src/ml-api-common.c:1115-1120) validates data_size against tensors[i].size before the memcpy, so leaving the size non-zero with a NULL pointer would indeed have allowed a memcpy() into NULL. Zeroing both is the right call.

2. Regression risk to other modules - low, and the claims check out

• Android JNI: java/.../nnstreamer-native-singleshot.c:251 uses ml_single_invoke_fast() only, i.e. need_alloc == FALSE, so those outputs never enter destroy_data_list and never get a destroy callback. Unaffected - confirmed.
• single_h->out_tensors is created by __setup_tensor_memory() with tensors[i].data = NULL, and _ml_tensors_data_clone_no_alloc() copies those NULLs, so there is no aliasing between the reusable wrapper and the per-invoke _out. The new helper cannot double-free the wrapper.
• No public API/ABI change, no header change, nothing outside c/src/ml-api-inference-single.c. The product diff is 48 lines - proportionate to the topic. Good.

---

Findings

[Major] The new regression test does not run in either PR-gating CI

libnnstreamer_customfilter_scaler_allocator.so is installed to /usr/lib/nnstreamer/customfilters/ and, in nnstreamer's packaging, is shipped only by the nnstreamer-unittests RPM sub-package (packaging/nnstreamer.spec, guarded by %if 0%{?release_test}) and by no Debian binary package at all (debian/nnstreamer-*.install contain no customfilters/ entry).

This repository's build dependencies do not pull it in:

• debian/control Build-Depends: nnstreamer, nnstreamer-dev, nnstreamer-dev-internal - none of which contain the custom filter.
• packaging/machine-learning-api.spec BuildRequires: nnstreamer-devel, nnstreamer-devel-internal, ... - no nnstreamer-unittests.

So nnsconf_get_custom_value_string ("filter", "customfilters") / g_file_test (test_model, ...) will take the goto skip_test path, and both invoke_timeout_alloc_in_invoke_p and close_before_data_destroy_01_p will silently no-op. The H1 deadlock would then have no CI protection at all.

(This is a pre-existing gap that invoke_11_p / invoke_12_p also suffer from, but a test added specifically as a deadlock gate should not inherit it.)

Please confirm against the actual GBS unit_test 1 job log whether these two tests are reported as run. If they are skipped, either add the packaging dependency, or reproduce H1 with a framework that is present in CI, or add a small allocating custom-easy filter under tests/ that this repo builds itself.

[Major] A regression manifests as a hang, and the CI test runner has no timeout

The test note says a regression "hangs until the meson test timeout". That timeout does not apply in CI:

• tests/capi/meson.build does declare test('unittest_capi_inference_single', ..., timeout: 100), but neither CI path uses meson test / ninja test.
• debian/rules:37-38 -> ./packaging/run_unittests.sh ./tests
• packaging/machine-learning-api.spec:428 -> bash %{test_script} ./tests/capi/unittest_capi_inference_single
• packaging/run_unittests.sh runs the gtest binary directly (${entry} --gtest_output=...) with no timeout wrapper.

Consequently a regression would deadlock the binary and hang the job until GitHub Actions' 6-hour default job timeout - technically still a red check, but a 6-hour one with no indication of which test caused it, and on GBS builders it may not be bounded at all. Note also that because the deadlocked thread holds the global magic lock, every subsequent single-shot test in the same binary blocks too, so the failure is maximally uninformative.

Suggest making the failure explicit and fast: run the invoke sequence on a worker GThread and have the test body wait on a GCond with g_cond_wait_until() (a few seconds), FAIL()ing if it does not signal; or arm a watchdog thread that abort()s with a message. Either turns the deadlock into a normal, attributable test failure that blocks the merge in minutes rather than hours.

[Minor] __destroy_notify() can now turn a leak into an invalid free when single_h->filter is NULL

if (G_LIKELY (single_h->filter)) {
  if (single_h->klass->allocate_in_invoke (single_h->filter)) {
    ...
    for (i = 0; i < data->num_tensors; i++) { data->tensors[i].data = NULL; ... }
  }
}
/* reset callback function */
data->destroy = NULL;

The pointer-clearing sits inside the filter != NULL && allocate_in_invoke guard, but data->destroy = NULL is unconditional. So if single_h->filter were ever NULL, __release_output_data() would fall through to ml_tensors_data_destroy() with destroy == NULL and g_free() framework-owned memory (here: malloc()-ed inside the custom filter). Before this PR the same situation produced ML_ERROR_INVALID_PARAMETER out of ml_single_destroy_notify_cb(), and _ml_tensors_data_destroy_internal() returned early freeing nothing - a leak, not an invalid free.

This looks unreachable today (single_h->filter is only cleared in ml_single_close() after g_thread_join()), hence Minor - but since this PR is explicitly hardening this function, it is cheap to make safe: either clear the descriptors unconditionally, or early-return from __release_output_data() when !single_h->filter.

[Minor] The clearing loop bound differs from the framework's

g_tensor_filter_single_destroy_notify() (nnstreamer, tensor_filter_single.c:358-372) releases i < priv->prop.output_meta.num_tensors; the new loop clears i < data->num_tensors. They coincide today, but the PR's stated motivation is precisely that "nothing in the vtable contract promises that". A short comment recording the assumption (or clamping to the smaller of the two) would keep this honest.

[Minor] Coverage of the three fixed sites is implicit and racy

invoke_timeout_alloc_in_invoke_p distinguishes the __process_output path from the invoke_thread JOIN_REQUESTED path only by timing (g_usleep (500000), then a second invoke immediately followed by ml_single_close). Nothing asserts which branch was actually taken, so if a future change makes the second invoke return ML_ERROR_TRY_AGAIN (which the test already tolerates) or makes the close lose the race, the JOIN_REQUESTED branch is silently no longer covered and the test still passes. The third fixed site - the exit: label with single_h->output, reached when close arrives before the thread picks the job up - is never exercised at all.

Also, EXPECT_EQ (status, ML_ERROR_TIMED_OUT) depends on a 3 MiB scaler invoke exceeding 10 ms. The scaler does two integer divisions per element in its inner loop so the margin is comfortable (tens of ms), but it is not guaranteed on a fast x86 builder, and on the failing branch output is leaked. Consider raising the input size or lowering the timeout to 1 ms for headroom.

This matters for the "detect future breakage from other modules" requirement: if nnstreamer later changes the invoke/allocate contract, this test set would need to still be pointing at the right branches to notice.

[Minor] __release_output_data() mutates the data descriptor outside the per-data lock

The previous path mutated data->tensors[] / data->destroy from inside _ml_tensors_data_destroy_internal(), i.e. under G_LOCK_UNLESS_NOLOCK (*_data). The new helper does it before taking that lock. This is safe today because the handle is abandoned - but only as long as #690 M11 holds: a spurious g_cond_wait_until() wakeup at :1600 still lets _ml_single_invoke_internal() publish _out to the caller while the invoke thread owns it. If M11 is fixed separately this becomes moot; otherwise a one-line comment recording the invariant would help the next reader.

[Minor] New observable behaviour is not documented

After ml_single_close(), an output from an allocating framework now reports data == NULL, size == 0 from ml_tensors_data_get_tensor_data(), while data->info still reports the real tensor size - a visible inconsistency between the two views of the same handle. nnstreamer-single.h (the @note on ml_single_invoke()) already warns that the buffer "will not [be] available for use later", so I do not think this needs an ACR or a header signature change - agreed there is no API/ABI/architecture change here. But an @note on ml_single_close() spelling out "the output data handle reports a NULL buffer of size 0 after this call" would make explicit the contract the fix now relies on.

[Nit] Test duplication

close_before_data_destroy_01_p repeats ~55 lines of invoke_11_p / invoke_12_p setup verbatim (now the 4th copy of the scaler_allocator boilerplate in this file). A small static helper would keep the file from growing. Relatedly, invoke_12_p already covers the close-then-destroy order on an allocating framework; the new test's added value is only the get_tensor_data / set_tensor_data assertions, so folding them into the existing test is an option.

[Nit] ASSERT_* bypasses the cleanup label

In all three new tests an ASSERT_EQ / ASSERT_TRUE failure returns without reaching skip_test:, leaking lib_path, test_model, the info handles and the single handle. This matches the existing style in the file, so it is not a new problem - just noting that three more instances were added.

---

Documentation / architecture

No API, ABI, or architecture change. __destroy_notify() and __release_output_data() are static to ml-api-inference-single.c. nnstreamer-single.h already documents both the close-before-destroy order and the "not available after close" property, so no mandatory doc update is missing - only the optional @note in the Minor item above.

---

Overall verdict: Request changes - not approvable as-is

The product-code change is correct and I would approve it on its own merits. What blocks approval is the verification story, which is the explicit bar for this PR:

• Must fix before merge - the two Major items. As written, a future re-introduction of H1 would most likely not be caught by CI at all (the tests skip), and in an environment where they do run it would hang the job for hours rather than fail. The PR does not yet establish the merge gate it claims to.
• Should fix - the single_h->filter == NULL fall-through in __destroy_notify() (first Minor), since it is inside the very function this PR is hardening.
• Remaining Minor / Nit items are fine as follow-ups.

For the record, the PR is currently a Draft and carries the DO NOT MERGE label, which is consistent with the above.

Verified against 4764a2f on fix/690-single-invoke-timeout-deadlock, base upstream/main (8952123).

[myungjoo]

Thanks — both Majors were correct, and I verified them before acting.

• %files unittests in nnstreamer.spec:1445 is the only place customfilters/*.so is packaged, it is gated on release_test, and nnstreamer's debian/ ships no equivalent. Neither BuildRequires in packaging/machine-learning-api.spec nor Build-Depends in debian/control asks for it, so the two custom-filter tests were dead weight — as are the pre-existing invoke_10_p/11_p/12_p.
• packaging/run_unittests.sh runs each gtest binary bare. meson.build's timeout: 100 never applies, since neither debian/rules nor the spec %check goes through meson test.

Fixed in cb22123.

Major 1 — tests/capi/ml_api_customfilter_slow_allocator.c, built by this suite's own meson.build. It allocates the output in invoke and sleeps 200 ms there, so both custom-filter tests now run wherever the tests are built and no longer depend on an external package. The sleep also removes the timing assumption I had (a large tensor and a 1 ms timeout): the timeout is now 10 ms against a 200 ms invoke. The library is located through MLAPI_BUILD_ROOT_PATH, which both debian/rules and the spec %check export, with a fallback to the build-relative path that run_unittests.sh leaves as the cwd. Added to debian/ml-api-unittests.install so install-test runs keep working.

Major 2 — a watchdog thread ends the process after 30 s. It cannot be softer than that: the deadlocked thread parks holding the global magic lock, so the test binary is unusable afterwards and every later test would hang too. run_unittests.sh propagates the non-zero exit, which stops the run. A 6-hour stall becomes a 30-second failure with a message naming the cause.

Minor 1 (filter == NULL turning a leak into an invalid free) — fixed. __destroy_notify() now falls back to data->destroy != NULL, which set_destroy_notify() sets only for framework-allocated data, so the descriptor is cleared even when the filter is already gone. It was unreachable (both call paths check single_h->filter first), but the code no longer depends on that.

Minor 3 (path coverage was timing-dependent) — deterministic now. The first invoke plus the 500 ms sleep goes through __process_output(); the second, closed immediately after, goes through the invoke_thread join branch. The third site, the exit: label, shares the same helper but needs the close to land between the job being queued and the thread picking it up, which I did not find a way to force from the public API.

Not changed, with reasons:

Minor 2 (loop bound) — data->num_tensors is deliberate. It is the exact range _ml_tensors_data_destroy_internal() would g_free(), so clearing it is what makes the double free impossible. nnstreamer's output_meta.num_tensors is not reachable from here, and were it ever smaller, the difference would be a leak rather than an invalid free.

Minor 4 (descriptor mutated outside the per-data lock) — taking _data->lock in __destroy_notify() would deadlock: _ml_tensors_data_destroy_internal() holds that non-recursive lock while it calls _data->destroy, which is how ml_single_destroy_notify_cb() reaches __destroy_notify() today. The data reaching __release_output_data() was abandoned by a timed-out caller, which never received the handle, so this path has sole ownership.

Minor 5 / Nit 1 / Nit 2 — the header already documents that the data is not available after close. The two custom-filter tests now share _get_test_custom_filter(); I left the older tests alone to keep the diff on topic, and the ASSERT_* cleanup pattern follows the rest of the file.

🤖 Generated with Claude Code

[myungjoo]

Re-review of cb22123 (H1 / H2 of #690)

Note: this review was produced by a separate review agent and is transcribed here by me. Verified against cb22123c8c50e3988ca98d6ecdebf60a8e7479e2, base upstream/main, the nnstreamer tree (gst/nnstreamer/tensor_filter/tensor_filter_custom.c, gst/nnstreamer/include/, nnstreamer*.pc.in), this repo's packaging (debian/, packaging/), and the CI logs of run 33857014167.

---

1. Status of the previous review items

Item	Status
Major 1 - tests do not run in either PR-gating CI	Approach resolved, effect not yet achieved. Building the filter in-tree is the right answer and removes the dependency on nnstreamer's unittests RPM. But the new target does not compile (Blocker below), and a missing filter still yields a green test (Major 2 below), so the gate is not in place yet.
Major 2 - regression hangs, runner has no timeout	Resolved. The watchdog is correct and the margin is comfortable - see section 3. _exit(1) propagates through run_unittests.sh (run_entry returns $?, the -f branch does exit $?) and through the spec's %build, so a deadlock becomes a ~30 s red job.
Minor 1 - single_h->filter == NULL turning a leak into an invalid free	Resolved. fw_allocated = (data->destroy != NULL) is the right fallback: set_destroy_notify() sets destroy only on the allocate_in_invoke branch, so the descriptor is now cleared even with the filter gone, and the worst case reverts to a leak.
Minor 2 - clearing loop bound differs from the framework's	Not changed, reason accepted. data->num_tensors is exactly the range _ml_tensors_data_destroy_internal() would g_free(), which is the property the fix needs.
Minor 3 - path coverage implicit and racy	Resolved for two of the three sites. With a 200 ms invoke, a 10 ms timeout and the 500 ms sleep, the first release provably goes through __process_output() and the second through the JOIN_REQUESTED branch of invoke_thread(); the ML_ERROR_TRY_AGAIN tolerance is now dead code. The exit: site stays uncovered - agreed it is not forceable from the public API.
Minor 4 - descriptor mutated outside the per-data lock	Not changed, reasoning is correct (_ml_tensors_data_destroy_internal() holds G_LOCK_UNLESS_NOLOCK (*_data) across the destroy call, so re-taking it would deadlock). The one-line invariant comment was still not added - see Nit 3.
Minor 5 / Nit 1 / Nit 2 - doc @note, duplication, ASSERT_* cleanup	Not changed, accepted. nnstreamer-single.h:139-140 already states the buffer "will not [be] available for use later". _get_test_custom_filter() removes the worst of the duplication.

Product-code diff (43/-14) re-verified: __release_output_data() covers all three re-entrant sites, g_list_remove() on a non-member is a harmless no-op on the paths where the output was never registered, and ml_single_close()'s g_list_foreach (..., __destroy_notify, ...) still runs after ML_SINGLE_HANDLE_UNLOCK. Behaviour for a non-allocating framework is unchanged. No new issue found in the C source.

---

2. New findings

[Blocker] The new meson target does not compile - the CI job that runs these tests is red

tests/capi/ml_api_customfilter_slow_allocator.c:21 includes <nnstreamer_plugin_api.h>, which pulls in <gst/gst.h>, but tests/capi/meson.build gives the target only dependencies: [glib_dep, nnstreamer_dep]. nnstreamer.pc declares Requires: nnstreamer-single and nnstreamer-single.pc declares an empty Requires:, with Cflags: -I${includedir}/nnstreamer - neither pulls in gstreamer's include path. Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") fails:

[29/58] Compiling C object tests/capi/libml_api_customfilter_slow_allocator.so.p/ml_api_customfilter_slow_allocator.c.o
FAILED: ...
cc ... -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/nnstreamer ...
/usr/include/nnstreamer/nnstreamer_plugin_api.h:19:10: fatal error: gst/gst.h: No such file or directory
ninja: build stopped: subcommand failed.
error: Bad exit status from /var/tmp/rpm-tmp.QgtaVh (%build)

(https://github.com/nnstreamer/api/actions/runs/33857014167/job/100972550388)

The file needs only gst_tensor_info_get_size() and gst_tensors_info_copy(), and both are declared in nnstreamer_plugin_api_util.h, which includes nothing beyond glib. So the minimal fix is:

• #include <nnstreamer_plugin_api_util.h> instead of <nnstreamer_plugin_api.h>, and
• dependencies: [glib_dep, nnstreamer_single_dep] instead of nnstreamer_dep.

That is also the more correct dependency: tensor_filter_custom.c is part of nnstreamer_single_sources, gst_tensor_info_get_size() / gst_tensors_info_copy() live in nnstreamer_plugin_api_util_impl.c (likewise single), and c/meson.build:10 deliberately keeps the single-shot side on [glib_dep, gmodule_dep, nnstreamer_single_dep]. As written, the .so would carry a DT_NEEDED on libnnstreamer.so and drag GStreamer into a test binary that is otherwise single-only. If you prefer to keep nnstreamer_plugin_api.h, then add gst_dep, as nnstreamer's own tests/nnstreamer_example/meson.build:36 does for nnstreamer_customfilter_scaler_allocator.

Note that install-test is false in the GBS build, yet the target still fails: meson builds it regardless of install:. So this breaks Tizen unit_test 1 unconditionally, not only test-installing configurations.

[Major] A missing custom filter still passes silently

test_model = _get_test_custom_filter ();
if (test_model == NULL) {
  /* cannot find the custom filter built along with this test */
  return;
}

Both custom-filter tests return green when the filter is not found. That was defensible while the filter came from an external package; it is not any more - the suite builds it itself, so absence means a build or layout bug, and the test should say so. ASSERT_TRUE (test_model != nullptr) (or at minimum GTEST_SKIP() << ..., which at least shows up in the report and the XML) is what makes the gate real. As it stands, renaming or dropping the meson target would silently remove the H1 regression gate again - precisely the failure mode the previous round was about.

[Major] The installed test can never find the filter, so the new debian/ml-api-unittests.install line is inert

_get_test_custom_filter() tries $MLAPI_BUILD_ROOT_PATH/tests/capi/<name> and then tests/capi/<name> relative to the cwd. Both are build-tree layouts:

• debian/rules exports MLAPI_BUILD_ROOT_PATH and run_unittests.sh does pushd build -> found. Good.
• packaging/machine-learning-api.spec exports it too -> found. Good.
• An installed run is not covered. packaging/run-unittest.sh does pushd /usr/bin/unittest-ml/tests and runs the binaries from there with no MLAPI_BUILD_ROOT_PATH; the .install line puts the library at /usr/lib/nnstreamer/bin/unittest-ml/tests/libml_api_customfilter_slow_allocator.so, i.e. next to the binary, never under a tests/capi/ subdirectory. Both tests skip there.

Add a lookup beside the executable (the cwd, or via /proc/self/exe) so the installed / on-device runs exercise it too. Otherwise the .install entry only silences a dh_install --list-missing warning and buys no coverage. Combined with the Major above, this is the difference between "packaged and skipped" and "packaged and gating".

[Minor] The custom filter indexes the input arrays with the output tensor count

for (i = 0; i < prop->output_meta.num_tensors; i++) {
  gsize size = gst_tensor_info_get_size (&prop->output_meta.info[i]);
  gsize in_size = gst_tensor_info_get_size (&prop->input_meta.info[i]);
  output[i].data = g_malloc (size);
  memcpy (output[i].data, input[i].data, MIN (size, in_size));
}

set_inputDim() copies in -> out, so the counts always coincide today and this is safe. But the loop reads input[i] / prop->input_meta.info[i] out of range the moment someone reuses this filter with a differing output count. Bound the loop by MIN (input_meta.num_tensors, output_meta.num_tensors), or assert equality in set_inputDim().

Otherwise the filter honours the tensor_filter_custom contract correctly, and I checked the two contract gates in custom_loadlib() / custom_open(): getInputDim == NULL, setInputDim != NULL, getOutputDim == NULL satisfies (!getInputDim != !setInputDim) && (!getOutputDim != !setInputDim), and invoke == NULL with allocate_invoke != NULL satisfies the invoke check. g_malloc in allocate_invoke paired with g_free in destroy_notify is a correct pair - better than the upstream scaler example, which malloc()s with no destroy_notify and relies on custom_destroyNotify()'s g_free fallback. Not setting output[i].size matches the upstream example (tensor_filter fills it from output_meta).

[Minor] The gate now rests on a single CI job

Only Tizen GBS ... unit_test 1 compiles and runs the unittests for a PR here. .github/workflows/pdebuild.yml produced no run for this head (gh run list shows only Spell Check, Static checkers, Tizen/GBS and Android Build Test). So after fixing the Blocker, please confirm from the GBS log that both custom-filter tests are reported as run and OK, not merely built. Worth checking carefully: because invoke_10_p / invoke_11_p / invoke_12_p have apparently never executed in this CI, the single-shot ML_NNFW_TYPE_CUSTOM_FILTER path is unproven in this environment, and the first green run is the only evidence that the new tests actually exercise anything.

[Nit] _exit() / <unistd.h>

The test file already special-cases __APPLE__ for the shared-object suffix; the new unconditional #include <unistd.h> and _exit() would not build on a Windows test configuration. Practically fine today, since the tests are POSIX-only.

[Nit] shared_module() would be more accurate than shared_library()

The artifact is only ever g_module_open()ed, never linked against.

[Nit] The invariant comment from the previous Minor 4 is still missing

The explanation in your reply - that __release_output_data() has sole ownership because the timed-out caller never received the handle, and that taking the per-data lock would deadlock against _ml_tensors_data_destroy_internal() - is exactly what the next reader will need. Two lines above __release_output_data().

---

3. Watchdog: correct, and no false-positive risk

Verified the timing is machine-independent rather than merely generous:

• INVOKE_DELAY_USEC is 200 ms of g_usleep(), not CPU work, so a slow or loaded builder does not extend it; the 10 ms ml_single_set_timeout() therefore fires deterministically and EXPECT_EQ (status, ML_ERROR_TIMED_OUT) cannot flake. This is a real improvement over the previous "a 3 MiB scaler invoke must exceed 10 ms" assumption.
• Expected wall time for invoke_timeout_alloc_in_invoke_p is about 0.71 s (10 ms + 500 ms sleep + 10 ms + ~190 ms of ml_single_close() waiting on invoking). Against a 30 s budget that is a ~40x margin, all of it in g_usleep / g_cond_wait_until, so it does not shrink on a slow CI machine.
• The window covers the whole failure mode: on a regression the invoke thread parks holding single_h->mutex with G_LOCK (magic) taken, so the second ml_single_invoke() or the ml_single_close() on the main thread blocks inside ML_SINGLE_GET_VALID_HANDLE_LOCKED - inside the watchdog's window either way.

The one caveat is that _exit(1) skips the gtest XML writeout, so the CI artifact will not name the test; the g_printerr() message covers that in the console log. Acceptable.

---

4. Scope, packaging, regression risk

• Scope is proportionate. 43/-14 in ml-api-inference-single.c, 285 lines of test, a 104-line test-only filter, 8 lines of meson, 1 line of debian. Nothing outside the single-shot internals and the test suite; __destroy_notify() and __release_output_data() remain static.
• RPM %files is safe. %files -n capi-machine-learning-unittests uses the directory glob %{_bindir}/unittest-ml, so the new .so is packaged automatically when install-test is on; and in the normal unit_test 1 GBS build install_test is -Dinstall-test=false anyway (only release_test flips it), so nothing changes there.
• Debian dh_install is safe. unittest_install_dir maps to /usr/lib/nnstreamer/bin/unittest-ml/tests under --bindir=lib/nnstreamer/bin, and the added glob matches. --list-missing (not --fail-missing) would only have warned.
• unit_test 0 GBS (armv7l / aarch64) is unaffected - -Denable-test=false means subdir('tests') is never entered.
• Android is unaffected - it does not go through meson; all four Android Build Test jobs are green.
• No single-only meson configuration exists that would need the new target excluded.
• No sub-plugin name collision: nnstreamer scans configured directories for the libnnstreamer_customfilter_ prefix; libml_api_customfilter_* cannot be picked up accidentally, and it is not installed into a scanned directory.
• Documentation: none required. nnstreamer-single.h:139-140 already documents that a framework-allocated output is freed at close and unavailable afterwards. The optional @note on ml_single_close() remains a Nit.

---

Overall verdict: Request changes - not approvable

The C fix is correct and I would approve it on its own; the two Majors from the previous round were answered in substance, and the watchdog in particular is now well engineered. What blocks merge is concrete and mechanical:

• Must fix: the Blocker - Tizen GBS ... unit_test 1 fails to compile the new custom filter, so the branch cannot be merged in any case. One include and one dependency.
• Must fix: the silent-skip Major - with the filter now built in-tree, _get_test_custom_filter() == NULL should fail the test, not pass it. Without this the merge gate is still nominal.
• Should fix: the installed-run lookup, so the shipped debian/ml-api-unittests.install entry is actually usable.
• After the fix: please confirm from the GBS log that invoke_timeout_alloc_in_invoke_p and close_before_data_destroy_01_p are reported as run and OK - this repo's CI has never executed the custom-filter single-shot path before.

Remaining Minor / Nit items are fine as follow-ups. The PR is still a Draft carrying DO NOT MERGE, which is consistent with the above.

[myungjoo]

The Blocker was already fixed in 2f3c00f, pushed before this review landed — the review ran against cb22123. gst_dep was added to the target, exactly as suggested; nnstreamer's own custom filter examples declare it for the same reason.

The two new Majors are fixed in 47bae8c.

Installed runs could not find the filter — right, and I had only checked packaging/run_unittests.sh, not packaging/run-unittest.sh, which starts from /usr/bin/unittest-ml/tests. _get_test_custom_filter() now also looks next to the running binary, resolved through /proc/self/exe. That covers the installed layout, where the filter is installed into the same directory, and also a developer running the binary from an arbitrary cwd. The candidate is skipped where /proc does not exist, and the two build-tree candidates remain.

Silent skip — fixed, and the reasoning is the point: a silent skip is exactly how the timeout test lost its teeth to begin with. Now that this suite builds the filter itself, not finding it means the test setup is broken rather than an optional dependency being absent, so both tests ASSERT_TRUE (test_model != NULL). The library target is unconditional in tests/capi/meson.build and follows the same install-test option as the test binary, so if the binary runs, the filter exists.

Input indexed with the output count — fixed. pt_allocate_invoke() returns -1 when the two counts differ instead of relying on setInputDim having copied the info.

Only the GBS unit_test 1 job runs tests — matches what I see: pdebuild.yml did not produce a run for this PR, so that job is the only place these tests execute. It follows that the custom-filter single-shot path has not run in this CI before, which is what the earlier nnstreamer-unittests finding predicted. I am waiting on that job and will not take this out of draft until it is green.

Nits — <unistd.h> and _exit() are POSIX and available on the platforms this suite targets, including the __APPLE__ case the file already handles. I kept shared_library() rather than shared_module() to match how nnstreamer declares its own custom filters; the artifact is dlopen-ed either way.

🤖 Generated with Claude Code

[myungjoo]

CI is green on 47bae8c, and the Tizen GBS … unit_test 1 log settles the question of whether these tests do anything:

[  OK ] nnstreamer_capi_singleshot.invoke_10_p                      (0 ms)
[  OK ] nnstreamer_capi_singleshot.invoke_11_p                      (0 ms)
[  OK ] nnstreamer_capi_singleshot.invoke_12_p                      (0 ms)
[  OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_p   (200 ms)
[  OK ] nnstreamer_capi_singleshot.close_before_data_destroy_02_p   (0 ms)
[  OK ] nnstreamer_capi_singleshot.invoke_timeout_alloc_in_invoke_p (801 ms)

The three pre-existing custom-filter tests report 0 ms, which is them taking goto skip_test — the nnstreamer-unittests finding from the first review, confirmed at runtime. They have never tested anything in this CI.

The new ones did run. 200 ms is one invoke through the filter this suite builds, whose pt_allocate_invoke() sleeps exactly that long. 801 ms is two timed-out invokes plus the 500 ms wait between them, so the whole H1 path executed: two outputs abandoned by a timed-out caller against a framework that allocates in invoke, both released by the invoke thread, ml_single_close() returning, and the watchdog never firing. close_before_data_destroy_02_p logs accl = cpu from opening add.tflite; 0 ms is just how long a one-element model takes.

To be explicit about what is not empirically shown: I did not push a revert of the C change to watch the test go red. The gate rests on the deadlock being certain from the code (a non-recursive single_h->mutex taken twice on one thread), the timing above proving the test reaches that path, and run_unittests.sh propagating a non-zero exit through break into the spec %check. Say the word if you want that revert run before this is approved.

🤖 Generated with Claude Code

[myungjoo]

3rd review of 47bae8c (H1 / H2 of #690)

Note: this review was produced by a separate review agent and is transcribed here by me. Verified against 47bae8c2282ebd369d1135a24b9cbf7b72c8783d, base upstream/main (8952123), the nnstreamer tree (gst/nnstreamer/tensor_filter/meson.build, gst/nnstreamer/include/, nnstreamer*.pc.in), this repo's packaging, and the raw log of job 100976391347 (run 33858234109), which I downloaded rather than taking the quoted excerpt on trust.

---

1. Status of every item from the previous two rounds

Round	Item	Status
2	[Blocker] new meson target does not compile (gst/gst.h not found)	Resolved in 2f3c00f. gst_dep is defined unconditionally at meson.build:29, so nothing else can break by adding it. The GBS x86_64 log confirms [28/58] Compiling ... ml_api_customfilter_slow_allocator.c.o and [42/58] Linking target tests/capi/libml_api_customfilter_slow_allocator.so. This is the review's fallback option rather than the preferred nnstreamer_plugin_api_util.h + nnstreamer_single_dep; see Nit 1 - a cosmetic difference, not a defect.
2	[Major] a missing custom filter still passes silently	Resolved in 47bae8c. Both tests now ASSERT_TRUE (test_model != NULL).
2	[Major] the installed test can never find the filter, so the .install line is inert	Resolved in 47bae8c. The /proc/self/exe candidate covers both installed layouts - Tizen %{_bindir}/unittest-ml/tests (where run-unittest.sh pushdes) and Debian /usr/lib/nnstreamer/bin/unittest-ml/tests - because unittest_install_dir puts the .so in the same directory as the binary. It also covers a developer running the binary from an arbitrary cwd.
2	[Minor] filter indexes the input arrays with the output tensor count	Resolved, and more strictly than suggested: pt_allocate_invoke() returns -1 on a count mismatch instead of clamping with MIN(). For a test filter, failing loudly is the better of the two.
2	[Minor] the gate rests on a single CI job	Confirmed and settled empirically - see section 3.
2	[Nit] _exit() / <unistd.h>; shared_module() vs shared_library()	Not changed, accepted.
2	[Nit] invariant comment above __release_output_data()	Partially addressed. The @note added ("its destroy callback takes the same mutex again") records the deadlock reason, which is the half a future reader needs most. The sole-ownership half is still unwritten. Fine as-is.
1	Major 1 / Major 2 / Minor 1 / Minor 3	Confirmed still resolved at HEAD; the round-2 assessment holds unchanged.
1	Minor 2 / Minor 4 / Minor 5 / Nit 1 / Nit 2	Not changed, reasons previously accepted; nothing at HEAD changes that.

I re-verified the C diff at HEAD independently rather than carrying the earlier conclusion forward:

• __destroy_notify(): the fw_allocated = (data->destroy != NULL) fallback is only read when single_h->filter is NULL; otherwise allocate_in_invoke() overrides it, which is the pre-existing semantics. Every entry that can reach it with destroy == NULL comes from set_destroy_notify (…, add=TRUE) at :1616 on a non-allocating framework, where allocate_in_invoke() is FALSE, so nothing is cleared and the behaviour is byte-for-byte the old one. No path clears buffers that the data handle owns.
• __release_output_data(): g_list_remove() on a non-member is a no-op; ml_tensors_data_destroy() after __destroy_notify() sees destroy == NULL and g_free (NULL) per tensor - neither a leak nor a double free.
• The three re-entrant sites (__process_output timeout branch, invoke_thread's status != NONE || JOIN_REQUESTED branch, invoke_thread's exit:) are all routed through the helper. The synchronous (timeout == 0) ml_tensors_data_destroy (_out) at :1628 still runs under the mutex, but _out there never had destroy set - unchanged, and still not a fourth site.

---

2. Do the new lookup and the ASSERT transition create false failures?

I went through this specifically, because turning a skip into an assertion is exactly the kind of change that trades one CI problem for another. I could not construct a legitimate configuration that now fails spuriously.

• No single-only meson configuration exists to break. gst_dep (meson.build:29) and nnstreamer_dep (:32) are unconditional top-level dependencies; the project already fails to configure without GStreamer. Adding gst_dep to one test target cannot narrow the set of buildable configurations.
• enable-test=false (GBS unit_test 0, i.e. the armv7l and aarch64 jobs) - meson.build:219 gates subdir('tests'), so neither the filter nor the test binary is built and %check never runs these. Both jobs are green on 47bae8c.
• install-test on or off - the shared_library() target is unconditional and carries the same install: option as unittest_capi_inference_single. If the binary exists, the filter exists next to it. There is no combination that builds the binary without the filter.
• All four runner layouts are covered:
  • debian/rules exports MLAPI_BUILD_ROOT_PATH -> candidate 1.
  • packaging/machine-learning-api.spec:414 exports it too -> candidate 1.
  • packaging/run_unittests.sh does pushd build -> candidate 2 (tests/capi/…).
  • packaging/run-unittest.sh (installed / on-device) pushdes /usr/bin/unittest-ml/tests -> candidate 3 (/proc/self/exe).
  • meson test / ninja test -> tests/meson.build sets MLAPI_BUILD_ROOT_PATH in testenv -> candidate 1.
• ML_NNFW_TYPE_CUSTOM_FILTER is always available, so the ASSERT_EQ on ml_single_open() that now becomes reachable cannot fail for lack of a sub-plugin: tensor_filter_custom.c is in nnstreamer_single_sources unconditionally (gst/nnstreamer/tensor_filter/meson.build:1-6), so wherever libnnstreamer-single exists, custom exists. Empirically confirmed - this CI had never executed that path before, and it works.
• gcov builds skip this binary entirely (spec:427, %if 0%{?unit_test} && !0%{?gcov}).
• Cross builds (armv7l / aarch64 GBS) are the unit_test 0 case above - %check is not reached for this binary.
• The only gap is macOS running from an installed layout - no /proc, so it would now hard-fail instead of skipping. macOS development runs from the build tree, which candidates 1 and 2 cover, and this project has no macOS CI. Nit at most.

The gst_dep route is also not a runtime hazard, which was worth confirming since it puts a DT_NEEDED on libnnstreamer.so into a .so that is dlopen()ed by an otherwise single-only test binary (b_asneeded=false means the entry is kept even though unused). nnstreamer's gst/nnstreamer/meson.build:107 does nnstreamer_deps += nnstreamer_single_dep, i.e. libnnstreamer.so links against libnnstreamer-single.so rather than statically re-containing it, so there is no duplicated sub-plugin registry and no duplicate-symbol interposition. The cost is load time, not correctness.

---

3. Is the CI log interpretation sound? Yes - I re-derived it from the raw log

Downloaded job 100976391347 and confirmed the quoted block verbatim, plus one line the excerpt omitted:

[ RUN      ] nnstreamer_capi_singleshot.close_before_data_destroy_02_p
** Message: 09:46:01.910: accl = cpu
[       OK ] nnstreamer_capi_singleshot.close_before_data_destroy_02_p (0 ms)

• 0 ms = skip is correct for invoke_10_p / invoke_11_p / invoke_12_p (and set_input_info_success_02, same cause): they take goto skip_test because nnsconf_get_custom_value_string ("filter", "customfilters") finds no libnnstreamer_customfilter_*. The first review's nnstreamer-unittests finding is confirmed at runtime.
• 0 ms is not a skip for close_before_data_destroy_02_p. The accl = cpu message is emitted between RUN and OK, which can only happen after ml_single_open() succeeded on add.tflite. tflite is genuinely enabled in this build (invoke_01 takes 132 ms on the same model). So that test really ran; 0 ms is just resolution on a one-element model.
• 200 ms is one real invoke through pt_allocate_invoke()'s g_usleep (200000). Nothing else in that test sleeps.
• 801 ms decomposes exactly as claimed, and - more to the point - it proves which branches ran: 10 ms to the first timeout, the invoke thread completes at t≈200 ms and takes __process_output()'s timeout branch; 500 ms gap; the second invoke times out at t≈520 ms, ml_single_close() sets JOIN_REQUESTED and blocks on invoking, the invoke thread completes at t≈710 ms and takes the status != NONE || JOIN_REQUESTED branch. Two of the three fixed sites are demonstrably executed, in the order the test comment claims. That is a stronger statement than "the test ran".

One clarification on what the green run does and does not prove. cb22123's GBS job was red and 47bae8c's is green, so the job demonstrably gates on this binary; and run_unittests.sh's -f branch (exit $?) inside rpm %build (set -e) propagates a failure. What has never executed is the watchdog's _exit(1) arm itself. I agree with your reasoning that the deadlock is certain from the code and that a revert experiment is not required for correctness - but if you want one datum for the price of one run, temporarily lowering the watchdog budget below the test's own ~0.71 s would exercise the abort path and its exit-code plumbing without touching the C fix. Entirely optional.

---

4. Final pass over the cumulative change (4764a2f + cb22123 + 2f3c00f + 47bae8c)

• Topic solved: yes. The H1 self-deadlock is closed at all three re-entrant sites, and H2 is hardened so the fix no longer silently depends on nnstreamer's g_tensor_filter_single_destroy_notify() NULLing the descriptors for it.
• Regression risk: low. Product diff is 43/-14, entirely inside ml-api-inference-single.c; __destroy_notify() and __release_output_data() are static. No public API, ABI, or header change. ml_single_invoke_fast() (the Android JNI path) never enters destroy_data_list and is untouched; all four Android jobs are green.
• Size: 454/-14 total, of which 402 lines are test scaffolding (295 test + 107 filter). Proportionate for a defect whose regression signature is a process-wide hang.
• Test completeness: good, and the pairing is the part I would call out as well designed. close_before_data_destroy_01_p pins the new "NULL buffer, size 0 after close" behaviour on an allocating framework, and close_before_data_destroy_02_p pins the opposite on tflite - which is what stops the clearing from later being widened to buffers the data handle owns. invoke_timeout_alloc_in_invoke_p covers two of the three fixed sites with machine-independent timing plus a watchdog. The exit: site remains uncovered; I agree it is not forceable through the public API.
• Documentation: none required. nnstreamer-single.h:139-141 already states that a framework-allocated output "will not [be] available for use later". An @note on ml_single_close() remains optional.

---

5. Remaining items, all trivially fixable inside this PR if you want them

None of these is a merge condition.

• [Nit] The gst_dep route is the heavier of the two options. Two lines get the lighter one: #include <nnstreamer_plugin_api_util.h> instead of <nnstreamer_plugin_api.h> in ml_api_customfilter_slow_allocator.c:21, and dependencies: [glib_dep, nnstreamer_single_dep] in tests/capi/meson.build:4. I checked that the file needs nothing else: gst_tensor_info_get_size() is declared at nnstreamer_plugin_api_util.h:48, gst_tensors_info_copy() at :227, and tensor_filter_custom.h pulls only tensor_typedef.h + nnstreamer_plugin_api_filter.h, which itself includes only tensor_typedef.h. That drops both libgstreamer-1.0 and libnnstreamer.so from the test .so. The current form is correct and green, so this is aesthetics plus load time.
• [Nit] The ML_ERROR_TRY_AGAIN tolerance is now dead code. EXPECT_TRUE (status == ML_ERROR_TIMED_OUT || status == ML_ERROR_TRY_AGAIN) in invoke_timeout_alloc_in_invoke_p: with a 200 ms invoke and a 500 ms gap the first job is always long finished, so TRY_AGAIN is unreachable. Tightening it to EXPECT_EQ (status, ML_ERROR_TIMED_OUT) makes the second release site's coverage asserted rather than merely tolerated - the same argument that motivated replacing the silent skip. One line.
• [Nit] macOS + installed layout would now hard-fail rather than skip, since /proc/self/exe is absent and only build-tree candidates remain. No CI configuration hits it. If you want it airtight, _NSGetExecutablePath() under __APPLE__ - but I would not bother.
• [Nit] The three older custom-filter tests stay permanently dead in this CI (0 ms, confirmed). Now that the suite ships its own allocating filter, invoke_11_p / invoke_12_p could be pointed at it and would start testing something. Agreed this is off-topic for this PR; worth a follow-up issue so the 0 ms lines are not rediscovered a third time.

And one observation that is not a defect in this PR, recorded so it is not mistaken for verified coverage:

• [Minor - informational] The Debian side of this change is untested and cannot be tested here. .github/workflows/pdebuild.yml is disabled_inactivity at the repository level (gh api repos/nnstreamer/api/actions/workflows), so it neither runs on PRs nor fires its nightly cron - which is why no pdebuild run appeared for any commit on this branch. The new debian/ml-api-unittests.install line and the override_dh_auto_test execution of these tests have therefore never run. By inspection the line is correct: --bindir=lib/nnstreamer/bin makes unittest_install_dir = /usr/lib/nnstreamer/bin/unittest-ml/tests, the added glob matches, and debian/rules hardcodes -Dinstall-test=true, so dh_install --list-missing has nothing to warn about. Pre-existing repo condition, not caused by this PR, and not a reason to hold it. Re-enabling that workflow deserves its own issue.

---

Overall verdict: Approve - safe to merge

Both round-2 Majors and the round-2 Blocker are resolved in code, not merely in intent, and I verified each against the artefacts rather than the commit messages. The merge gate this PR set out to build now exists and is demonstrably live: the new tests execute in Tizen GBS … unit_test 1, the 801 ms figure proves they reach two of the three fixed sites, a missing filter now fails instead of passing, and a regression turns into a ~30 s red job instead of a multi-hour stall.

The C fix is correct, minimal and correctly scoped, and the round-1 conclusion that no fourth re-entrant site exists still holds at HEAD. Nothing outstanding rises above Nit. From my side the DO NOT MERGE label and Draft status can be lifted.

[myungjoo]

Thanks. On the one gap you flagged — the watchdog's _exit(1) path never having executed — I verified the mechanism locally rather than spending a CI round on it. Same watchdog function, budget shortened, with the main thread never setting the flag to stand in for the deadlock:

normal path (flag set):        "normal path completed"   exit=0
flag never set:                "watchdog fired"          exit=1
run_unittests.sh-shaped loop:                            exit=1

So the abort fires, the exit code survives run_entry's return $?, and the break carries it out of the runner into the spec %check. That is the whole chain the gate depends on.

Not taking the remaining Nits into this PR, with reasons:

• nnstreamer_plugin_api_util.h + nnstreamer_single_dep — you are right that it would drop the gst and libnnstreamer dependency, and you confirmed there is no runtime risk either way. But it changes a build declaration that just went green, for tidiness, and every change here costs another review and GBS round. The current form is also how nnstreamer declares its own custom filter examples.
• ML_ERROR_TRY_AGAIN allowance — I would rather keep it. It is unreachable when the 500 ms wait comfortably covers the 200 ms invoke, which is the point, but a loaded runner that eats 300 ms of slack would turn a tightened EXPECT_EQ into a flake. The cost of keeping it is that the second fix site goes uncovered on such a run, not a wrong result.
• macOS installed layout, and invoke_10/11/12_p still dead at 0 ms — real, and outside what this change is about. The three of them cover multi-tensor handling and destroy ordering, so pointing them at the in-tree filter deserves its own change where those semantics get looked at properly. Happy to file it as an issue if you want it tracked.
• pdebuild.yml disabled at the repository level — noting it for the record: the debian/ml-api-unittests.install line and override_dh_auto_test in this PR are correct but will not be exercised by CI until that workflow is re-enabled.

Taking this out of draft.

🤖 Generated with Claude Code

[myungjoo-bot]

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

Summary: The PR routes the three in-thread releases of a timed-out, framework-allocated output (__process_output, the status != NONE || JOIN_REQUESTED branch, and the exit: label of invoke_thread) through a new __release_output_data() that calls __destroy_notify() (clearing data->destroy) before ml_tensors_data_destroy(), and makes __destroy_notify() NULL/zero the descriptors it hands back to the framework (H2). The defect was confirmed on upstream/main: ML_SINGLE_GET_VALID_HANDLE_LOCKED takes G_LOCK (magic) then g_mutex_lock (&single_h->mutex) and only then releases magic; _ml_tensors_data_destroy_internal (ml-api-common.c:692-727) invokes data->destroy = ml_single_destroy_notify_cb -> the macro -> self-deadlock on the non-recursive handle mutex while holding magic. Every other ml_tensors_data_destroy under the handle mutex was checked and operates on destroy == NULL data; ml_single_close calls __destroy_notify directly after unlocking; ml_single_invoke_fast and the JNI path never enter the list; ml_single_invoke_dynamic funnels into ml_single_invoke. __destroy_notify takes no locks and only reaches the sub-plugin's destroyNotify with no re-entry into ML-API. The GBS x86_64 unit_test 1 log shows close_before_data_destroy_01_p, close_before_data_destroy_02_p, and invoke_timeout_alloc_in_invoke_p all OK; all 10 checks green at 47bae8c; merge-tree clean; DCO on all four commits. Approving; items below are non-blocking.

1. [Low] Commit hygiene — cb22123 (tests/capi/meson.build) does not build on Tizen on its own (missing gst_dep; its GBS run failed) and is fixed only by 2f3c00f. It also carries a product-code change (the __destroy_notify fallback on data->destroy) inside a [Test] commit. Suggest squashing 2f3c00f into cb22123 (and ideally 47bae8c) so every commit builds, or moving the __destroy_notify hunk into 4764a2f.
2. [Low] Description precision — "three call sites are affected [by the self-deadlock]" is imprecise: the JOIN_REQUESTED branch (:583) and exit: (:609) run after ml_single_close zeroed magic, so on base the callback returned ML_ERROR_INVALID_PARAMETER and _ml_tensors_data_destroy_internal bailed early — a leak of the handle and framework buffers, not a hang. The fix is still correct and desirable there. Suggest wording it as "deadlock at __process_output, leak at the two close-time sites"; the second half of invoke_timeout_alloc_in_invoke_p (~:3716-3721) guards the leak path.
3. [Low] Docs — H2 is consistent with the contract in c/include/nnstreamer-single.h:139-141 (close-before-destroy). The only observable delta is ml_tensors_data_get_tensor_data() reporting size == 0 after close (previously original size with a NULL pointer). Acceptable; optionally extend the @note with "after ml_single_close(), such an output reports a NULL buffer of size 0".
4. [Low] Test note — unittest_capi_inference_single.cc:~3709: with a 10 ms timeout, if the invoke thread is not scheduled before the timeout fires, the thread sees NULL input/output (pre-existing #690 L9). With this PR's NULL guard that is a leak, not a failure, so the test cannot flake on it; noted so nobody mistakes the leak for a regression of this PR.
5. [Low] Custom filter — tests/capi/ml_api_customfilter_slow_allocator.c:74-75 indexes &prop->output_meta.info[i] directly; nnstreamer's examples use gst_tensors_info_get_nth_info() so tensors beyond NNS_TENSOR_SIZE_LIMIT resolve to extra. Harmless here (one tensor); switch for parity. Everything else checks out (setInputDim, 200 ms sleep + g_malloc per output in allocate_invoke, destroy_notify frees, deps match nnstreamer's example filters, Doxygen on every function).
6. [Low] "fail loudly" commit — the filter is built unconditionally alongside the test binary, so ASSERT_TRUE (test_model != NULL) cannot fire in any lane that builds the tests; the /proc/self/exe fallback covers the installed /usr/bin/unittest-ml/tests layout. Packaging is consistent: debian/ml-api-unittests.install matches unittest_install_dir, the spec's %files -n capi-machine-learning-unittests uses the %{_bindir}/unittest-ml directory glob, and %check exports MLAPI_BUILD_ROOT_PATH. The pdebuild workflow did not run for this PR (check-rebuild gating), so the Debian line is unverified by CI but correct by inspection.
7. [Low] Follow-up outside this PR — c/src/ml-api-common.c:~909: after H2 zeroing, ml_tensors_data_clone() on a closed output does memcpy (dst, NULL, 0) (technically UB; previously a use-after-free read, so strictly an improvement). A if (_in->tensors[i].size) guard would make it clean.

Verified no-issues: the helper preserves behavior for destroy == NULL data and for NULL output; the app's later ml_tensors_data_destroy after close is a guaranteed no-op; ml_tensors_data_set_tensor_data on a STATIC handle now returns ML_ERROR_INVALID_PARAMETER instead of memcpy into NULL; the tests would hang on base at the first ml_single_invoke (watchdog _exit (1) after 30 s, propagated by run_unittests.sh); cleanup is complete on the pass path.

No back-door or suspicious behavior found.

[myungjoo]

Thanks for the approval. Acted on 2, 3 and 5 in d9b344f; 1 needs your call.

2 — description precision. You are right that I flagged all three sites as deadlocks, and that the close-time ones are not. ml_single_close() zeroes magic via ML_SINGLE_GET_VALID_HANDLE_LOCKED (single_h, single, 1) before setting JOIN_REQUESTED, so the callback returns ML_ERROR_INVALID_PARAMETER without reaching the mutex and _ml_tensors_data_destroy_internal() bails on that error — a leak of the handle and the framework buffers.

One correction to the suggested wording, though: it is not cleanly "deadlock at __process_output, leak at the two close-time sites". The status != ML_ERROR_NONE || JOIN_REQUESTED branch has both modes. When an invoke fails after the caller timed out and no close is pending, magic is still valid and that site deadlocks too; only the JOIN_REQUESTED half of it leaks. The description now says exactly that, per site.

3 — docs. Added to the @details of ml_single_close(), next to the close-before-destroy note where a reader would look for it.

5 — custom filter. Switched to gst_tensors_info_get_nth_info() for both input and output, matching nnstreamer_customfilter_example_passthrough_variable.c.

4, 6 — noted, no change. Agreed on both, and thanks for pinning down that the L9 NULL path is a leak rather than a flake under this PR's guard; that is worth having on the record.

7 — ml_tensors_data_clone() memcpy (dst, NULL, 0). Agreed it is out of scope here and agreed it is strictly better than the use-after-free read it replaces. Leaving it; happy to file it against #690 as its own item.

1 — commit hygiene. You are right that it matters here: main is linear and PR commits land individually, so cb22123 would be a commit that does not build on Tizen sitting in the history, and git bisect would trip on it. It also carries a product hunk under a [Test] subject.

I did not rewrite it. Collapsing to one product commit plus one test commit needs a force-push over a branch that three review rounds and an approval already point at, and that felt like your decision rather than mine. Two ways to settle it:

• Say the word and I will rewrite it as [C-Api] fix … (all of ml-api-inference-single.c and the header) plus [Test] … (filter, meson, debian, tests), force-push, and re-run CI. Each commit builds on its own.
• Or squash-merge this PR, which makes the intermediate state moot.

CI is running on d9b344f; I will report when it lands.

[myungjoo]

CI is green on d9b344f — all 10 checks — and the tests still do their work after the gst_tensors_info_get_nth_info() swap:

[  OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_p   (201 ms)
[  OK ] nnstreamer_capi_singleshot.close_before_data_destroy_02_p   (0 ms)
[  OK ] nnstreamer_capi_singleshot.invoke_timeout_alloc_in_invoke_p (801 ms)

Same 200 ms and 800 ms shape as on 47bae8c, so the filter still allocates in invoke and both timeouts still fire.

The only thing still open is the commit hygiene question above: rewrite into one product commit plus one test commit and force-push, or squash-merge and leave the branch as it is.

[myungjoo]

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

Scope: git diff 47bae8c d9b344f only. The 47bae8c review (APPROVED) is not re-litigated. Items 4, 6 there were informational and item 7 was out of scope; item 1 (commit hygiene) is left to the maintainer as stated.

Verdict: no Blocker, no Major, no Minor. Two Nits, both cosmetic. Approval stands and extends to d9b344f.

---

1. c/include/nnstreamer-single.h — the Doxygen addition is accurate

"An output buffer allocated by the inference engine is handed back to it while closing, so such an output reports a NULL buffer of size 0 afterwards."

Traced end to end; the claim holds and does not overreach.

• Which outputs it covers. set_destroy_notify() (ml-api-inference-single.c:459-472) forces add = TRUE whenever klass->allocate_in_invoke() is true, and __process_output() (:522-523) calls it for every successful allocating invoke. So on an allocating framework every live output is in destroy_data_list, and ml_single_close() (:1419) runs __destroy_notify over all of them. The sentence therefore describes the common case, not a corner case.
• Which outputs it does not cover. __destroy_notify() (:372-387) recomputes fw_allocated = klass->allocate_in_invoke (filter), and at :1419 single_h->filter is guaranteed non-NULL by the if (single_h->filter) guard at :1418. For tensorflow-lite the descriptor is left untouched, so an output from a non-allocating framework stays valid after close. No contradiction — the sentence is scoped by "allocated by the inference engine", and close_before_data_destroy_02_p pins exactly that.
  • Note the timeout path (:1616) adds to destroy_data_list with add = TRUE even for a non-allocating framework, but fw_allocated is still FALSE there, so those descriptors are not cleared either. The scoping is on allocate_in_invoke, not on list membership — which is what the doc says.
• "reports a NULL buffer of size 0". ml_tensors_data_get_tensor_data() (ml-api-common.c:1022-1057) returns ML_ERROR_NONE and copies out tensors[i].data / .size verbatim; it does not reject a NULL/0 descriptor. So the app really does observe NULL / 0 rather than an error. Correct.
• Consistency with the existing contract. ml_single_invoke()'s @note (nnstreamer-single.h:141-143) already says such a buffer "will be freed when closing the @A single automatically by the neural network framework, and will not available for use later". The new sentence documents the observable form of that already-documented behaviour rather than introducing a new one. Nothing in the header now contradicts anything else.

Not Major. Not Minor. Two Nits:

• [Nit] Terminology. The header says "neural network framework" everywhere else (:141, ml_single_open docs); the new line says "inference engine". Same thing, but a public Tizen header reads better with one term.
• [Nit] Actionability. The new sentence tells the app what it will see but not how to know whether its own handle is in that category. The existing @note at :141 names ML_NNFW_TYPE_CUSTOM_FILTER as an example; an @see ml_single_invoke() or a pointer back to that note would close the loop. Purely optional.

2. tests/capi/ml_api_customfilter_slow_allocator.c — the gst_tensors_info_get_nth_info() switch is correct

• The fix is real, not cosmetic. GstTensorsInfo.info is GstTensorInfo info[NNS_TENSOR_MEMORY_MAX] (tensor_typedef.h:276), i.e. 16 entries, with extra holding 16..255 (:277). The old &prop->output_meta.info[i] ran off the array at i >= 16. output[i] itself is safe up to 255 (ml_tensors_data_s.tensors[ML_TENSOR_SIZE_LIMIT], ml-api-internal.h:221, 256 entries), so the info side was the only overrun and it is now closed.
• The const cast is safe and idiomatic. gst_tensors_info_get_nth_info() takes a non-const GstTensorsInfo * because it lazily allocates info->extra (nnstreamer_plugin_api_util_impl.c:332-338). That is the only write. It cannot fire here: it needs index >= 16, and the tests use one tensor. nnstreamer does the same cast throughout its own util impl (:415, :419, :468, :513, :545) and in nnstreamer_customfilter_example_passthrough_variable.c:75, so the commit message's "as nnstreamer's own example filters do" checks out. The target is the filter instance's own writable prop, not read-only storage.
• The loop bound is exactly equivalent. out_meta aliases &prop->output_meta, so out_meta->num_tensors is prop->output_meta.num_tensors — same object, no semantic change. The input_meta.num_tensors != output_meta.num_tensors guard at the top (kept from 47bae8c) still makes the shared bound valid for in_meta too.
• nth_info cannot return NULL here. It only returns NULL for index >= NNS_TENSOR_SIZE_LIMIT (256), and num_tensors is capped at that limit. Even if it somehow did, gst_tensor_info_get_size(NULL) is g_return_val_if_fail-guarded and returns 0 (:160), so the failure mode is a g_malloc(0) and a zero-length memcpy, not a NULL deref. No NULL check needed.

[Nit] Commit message constant. d9b344f says "tensors past NNS_TENSOR_SIZE_LIMIT would resolve to the extra info rather than run off the array." The threshold is NNS_TENSOR_MEMORY_MAX (16); NNS_TENSOR_SIZE_LIMIT (256) is where nth_info returns NULL instead. The code is right, the message names the wrong constant — and the wording was inherited from item 5 of the 47bae8c review, so this is that review's error propagating. Only worth fixing if the branch is force-pushed for item 1 anyway.

3. The corrected failure-mode analysis in the PR body is verified correct

All three sub-claims confirmed against the source:

(a) ml_single_close() zeroes magic before setting JOIN_REQUESTED. Confirmed. ML_SINGLE_GET_VALID_HANDLE_LOCKED (single_h, single, 1) at :1400 sets single_h->magic = 0 inside the macro (:60-61) while holding G_LOCK (magic); single_h->state = JOIN_REQUESTED is only at :1405. Ordering as described.

(b) The close path leaks rather than deadlocks. Confirmed. ml_single_destroy_notify_cb() (:433) re-enters the macro with reset = 0; magic is already 0, so the magic != ML_SINGLE_MAGIC arm returns ML_ERROR_INVALID_PARAMETER at :58 after G_UNLOCK (magic) and before g_mutex_lock (&single_h->mutex). No mutex is touched, so no hang. _ml_tensors_data_destroy_internal() (ml-api-common.c:706-713) then returns early on that non-NONE status without freeing _data->info, without g_mutex_clear, and without g_free (_data) — a genuine leak of the handle plus, on base, the framework buffers. Exactly as the body states.

(c) The status != ML_ERROR_NONE || JOIN_REQUESTED branch really has both modes. Confirmed, and the deadlock half is reachable:

• Leak half — state == JOIN_REQUESTED: falls under (b).
• Deadlock half — status != ML_ERROR_NONE with state == RUNNING: reachable. ml_single_invoke() on timeout (:1610-1617) sets status = ML_ERROR_TIMED_OUT, calls set_destroy_notify (single_h, _out, TRUE) — installing data->destroy for an allocating framework — then clears single_h->input/output and unlocks, leaving state == RUNNING. invoke_thread had already copied output into its local (:571-572), so when __invoke() subsequently fails, it re-locks, sees status != ML_ERROR_NONE and state != JOIN_REQUESTED (:581), and on base called ml_tensors_data_destroy (output) on data whose destroy callback is set, under the handle mutex → the callback blocks on the non-recursive mutex while holding G_LOCK (magic). Both modes are live; the branch needed the fix for both reasons.

The body's "__process_output() deadlocks" and "the exit: label, reached only on JOIN_REQUESTED, leaks" also check out (exit: is entered from :583, :565 and by falling out of the state <= RUNNING loop — all with state == JOIN_REQUESTED, hence magic == 0).

4. Regressions

None. d9b344f touches one comment block in a public header and one test-only custom filter; no product code path changes. The filter change is behaviour-identical at the tensor counts the tests use. All 10 checks green at d9b344f, and the GBS x86_64 unit_test 1 log still shows close_before_data_destroy_01_p (201 ms), close_before_data_destroy_02_p (0 ms) and invoke_timeout_alloc_in_invoke_p (801 ms) all OK — the timings match 47bae8c, so the filter refactor did not perturb the timeout window.

---

Final verdict at d9b344f: mergeable. Items 2, 3 and 5 of the previous review are properly addressed; nothing new was introduced. The three Nits above (header terminology, an optional @see, and a wrong constant in the d9b344f commit message) are all non-blocking and only worth folding in if the branch is force-pushed for the commit-hygiene item, which remains the maintainer's call.

[myungjoo]

Confirmed the constant correction, and it upgrades what item 5 actually was. GstTensorsInfo.info is GstTensorInfo info[NNS_TENSOR_MEMORY_MAX] — 16 entries — while num_tensors ranges up to NNS_TENSOR_SIZE_LIMIT, 256. So &prop->output_meta.info[i] was not merely missing the extra indirection, it read past the array for i >= 16. The gst_tensors_info_get_nth_info() switch is a real out-of-bounds fix, not just parity with the example filters.

That also means d9b344f's commit message is wrong where it says NNS_TENSOR_SIZE_LIMIT; the bound it should name is NNS_TENSOR_MEMORY_MAX. Noting it here since correcting it needs the same history rewrite as item 1 — if the commit hygiene rewrite happens, that message gets fixed with it, and if this is squash-merged the message is composed fresh anyway.

The other two Nits (wording "inference engine" vs "neural network framework" in the header, the optional @see) I would fold into the same rewrite rather than spend a commit and a 40-minute GBS round on their own. Still waiting on your call between rewriting the history and squash-merging.

[myungjoo]

History rewritten as agreed. Five commits became two, force-pushed with a lease on d9b344f:

• 328e762 [C-Api] fix self-deadlock on single-shot invoke timeout — c/src/ml-api-inference-single.c and c/include/nnstreamer-single.h. The __destroy_notify() fallback on data->destroy, which used to sit inside a [Test] commit, is here where it belongs, and the message now describes the per-site failure modes correctly rather than calling all three a deadlock.
• 608a2aa [Test] cover the single-shot invoke timeout in CI — the custom filter, its meson target with gst_dep already in place, debian/ml-api-unittests.install, and the tests.

Each builds on its own now, so git bisect will not land on the Tizen build failure that cb22123 carried until 2f3c00f fixed it.

The two remaining Nits went in with the rewrite:

• The header sentence now says "neural network framework" rather than "inference engine", matching the terminology the rest of the file uses and, in particular, the @note on ml_single_invoke() that already documents this same buffer contract.
• The NNS_TENSOR_SIZE_LIMIT slip is gone with the commit message that carried it. For the record, in the code the switch to gst_tensors_info_get_nth_info() is about GstTensorsInfo.info being NNS_TENSOR_MEMORY_MAX entries while num_tensors goes up to NNS_TENSOR_SIZE_LIMIT.

Only content change against d9b344f is that header wording; git diff between the old and new heads is those two lines and nothing else. I left the optional @see out, since the header uses @see once in total and the ml_single_invoke() note is adjacent enough to find.

CI is running. The approval on 47bae8c is necessarily stale now — happy to have it re-run against 608a2aa once the checks land.