[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

tests/capi/ml_api_customfilter_slow_allocator.c builds a custom filter for
this suite. It allocates the output in invoke, so the single-shot handle takes
the allocate_in_invoke path, and each invoke sleeps long enough for a short
ml_single_set_timeout() to fire regardless of how fast the machine is. It is
found through MLAPI_BUILD_ROOT_PATH, which meson, debian/rules and the spec
%check all export, with fallbacks for the build directory that
run_unittests.sh leaves as the cwd and for the installed layout.

Added to tests/capi/unittest_capi_inference_single.cc:

• invoke_timeout_alloc_in_invoke_p — times out twice, 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 watchdog thread ends
  the process after 30 seconds, so a regression that deadlocks fails the job
  instead of stalling it until the CI job timeout.
• close_before_data_destroy_01_p — the close-before-destroy order documented
  in nnstreamer-single.h, on an allocating framework. Asserts the output
  reports a NULL buffer of size 0 after close.
• close_before_data_destroy_01_n — writing into that reclaimed output is
  rejected rather than reaching released memory, both for a full-size write and
  for a single element. This is the static-format contract: the rejection comes
  from ml_tensors_data_set_tensor_data() comparing against the size the
  handback zeroed.
• 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.

close_before_data_destroy_02_p skips when tensorflow-lite is unavailable. The
other three assert on the custom filter rather than skip: it is built alongside
the test binary, so not finding it means the test setup is broken.

Verification

clang-format reports no change on the test file, and the CI indent options
none on ml-api-inference-single.c. 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, where the three tests that existed before the negative case was
added all passed on the Tizen GBS x86_64 job.

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.

[myungjoo]

Note: This comment was written by a separate review agent (an independent code-review pass run outside this conversation) and is being posted here verbatim on the author's behalf. It is an independent re-review against 608a2aa and does not assume the conclusions of the earlier reviews already on this thread (based on 47bae8c/d9b344f).

Scope of this review

Reviewed git diff 8952123 608a2aa in full: c/src/ml-api-inference-single.c, c/include/nnstreamer-single.h, tests/capi/unittest_capi_inference_single.cc, tests/capi/ml_api_customfilter_slow_allocator.c, tests/capi/meson.build, debian/ml-api-unittests.install. Cross-checked against _ml_tensors_data_destroy_internal() in c/src/ml-api-common.c (unchanged) and against nnstreamer's tensor_filter_single.c/tensor_filter_custom.h upstream (g_tensor_filter_single_destroy_notify, NNStreamer_custom_class.destroy_notify calling convention) to verify the fix's assumptions about the sub-plugin contract. Also inspected the actual CI job logs for this PR (job 101218491195, "Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1")"), not just the pass/fail badges.

Does it solve H1/H2 (issue #690)?

Yes. All three sites that used to call ml_tensors_data_destroy() on a destroy-list-tracked output while already holding single_h->mutex (__process_output(), the status != ML_ERROR_NONE || JOIN_REQUESTED branch, and the exit: label in invoke_thread()) now go through the new __release_output_data() helper, which calls __destroy_notify() directly and clears data->destroy before calling ml_tensors_data_destroy(). That removes the re-entrant ML_SINGLE_GET_VALID_HANDLE_LOCKED → g_mutex_lock(&single_h->mutex) call that caused the self-deadlock (H1). I traced all remaining ml_tensors_data_destroy() call sites in the file (lines 578, 606, 679, 1437‑1438, 1629, 1635) and confirmed none of them can hold a data->destroy-bearing handle, so no 4th deadlock site was missed.

For H2, __destroy_notify() now zeroes tensors[i].data/tensors[i].size itself instead of relying on nnstreamer's own g_tensor_filter_single_destroy_notify() NULLing behavior (which is real today but was never part of the documented vtable contract). This makes the ml-api-side fix self-contained and not dependent on an undocumented behavior of the nnstreamer dependency — a good defensive choice given the review criterion about being resilient to future changes in other modules.

Regression check

• No other module is touched; the change is confined to single-shot internals (__destroy_notify, new static __release_output_data, and their three call sites) plus a 2-line doc addition. single_h->filter/single_h->klass lifetime invariants used by the new code were checked and hold at every call site (filter/klass are only nulled in ml_single_close() after g_thread_join(), and the g_list_foreach there is itself guarded by if (single_h->filter)).
• Actual CI evidence: in the run for this PR, tests/capi/unittest_capi_inference_single executed cleanly, including all 37 nnstreamer_capi_singleshot.* tests (13.6s total), with the three new tests passing: close_before_data_destroy_01_p (201 ms), close_before_data_destroy_02_p (0 ms), invoke_timeout_alloc_in_invoke_p (801 ms). No other single-shot test regressed.

Test coverage

The three added tests exercise exactly the scenarios described in the PR body: an allocating custom filter with the handle open (__process_output path), the same with a close pending (invoke_thread's join branch), and a non-allocating tensorflow-lite model to prove the buffer-clearing is not over-broadened to data the handle still owns. _singleshot_watchdog forces _exit(1) after 30s so a reintroduced deadlock fails the CI job quickly and legibly (well under the 100s meson-level timeout on this binary), rather than only being caught by an opaque infra timeout — this satisfies the "test-caught regression must actually fail CI" criterion.

Documentation

The observable behavior change (an fw-allocated output reports a NULL/size-0 buffer after ml_single_close()) is documented in c/include/nnstreamer-single.h's ml_single_close() doc comment. No public API/ABI signature changed, so no further doc/API-reference updates are needed.

Findings

[Minor] Dead/unreachable branch in __destroy_notify()
c/src/ml-api-inference-single.c, in __destroy_notify():

fw_allocated = (data->destroy != NULL);

if (G_LIKELY (single_h->filter)) {
  fw_allocated = single_h->klass->allocate_in_invoke (single_h->filter);
  ...

The first assignment is always overwritten inside the G_LIKELY (single_h->filter) branch, and single_h->filter is non-NULL at every actual call site of __destroy_notify(): ml_single_close()'s g_list_foreach is itself inside if (single_h->filter); ml_single_destroy_notify_cb() bails out earlier if !single_h->filter; and __release_output_data() only runs from invoke_thread(), which has already exited (via g_thread_join) by the time ml_single_close() nulls filter. So the data->destroy != NULL fallback for a NULL single_h->filter is currently dead code. It's harmless, but it reads as if there's a meaningful fallback path when there isn't — worth a comment or removal for clarity. Not blocking.

[Minor] New tests' "skip when unavailable" claim doesn't fully match their behavior
tests/capi/unittest_capi_inference_single.cc, _get_test_custom_filter() (~line 3448) plus its callers close_before_data_destroy_01_p and invoke_timeout_alloc_in_invoke_p (ASSERT_TRUE (test_model != NULL) at ~3532 and ~3680).
The PR description states all three new tests "skip when their model or custom filter is unavailable, following the existing convention in the file." That's true for close_before_data_destroy_02_p (tensorflow-lite, uses the existing goto skip_test pattern), but the two tests using _get_test_custom_filter() instead do ASSERT_TRUE(test_model != NULL), which is a fatal test failure, not a skip — unlike the pre-existing convention for other custom-filter tests in this same file (e.g. invoke_10_p, invoke_11_p), which use nnsconf_get_custom_value_string() + goto skip_test when the shared object can't be found. _get_test_custom_filter()'s third fallback also depends on Linux's /proc/self/exe, which doesn't exist on the platforms this file already special-cases (SO_FILE_EXTENSION ".dylib" for __APPLE__). In the actual CI (Linux/Tizen-GBS, binary and the new .so installed side by side per the debian/ml-api-unittests.install change) this resolves correctly, as the passing CI run confirms — so this is not currently causing failures — but it's a latent portability/robustness gap and a minor inconsistency with the PR's own stated intent. Consider using goto skip_test instead of ASSERT_TRUE for consistency, if this is touched again.

[Info, non-blocking] CI job "Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1")" was cancelled after ~6h on this PR's run
This is the job that actually runs unittest_capi_inference_single. I pulled its raw log (job 101218491195): the three new tests ran and passed at ~480s into the run (close_before_data_destroy_01_p 201 ms, close_before_data_destroy_02_p 0 ms, invoke_timeout_alloc_in_invoke_p 801 ms), and the job only got stuck/cancelled ~6h later inside an unrelated suite (MLOffloadingService.registerInvalidParam01_n), which this PR does not touch. The same job (same name) has been cancelled after a similar multi-hour hang on unrelated branches before this PR existed (e.g. runs 32819951684 on capi-svc-event-message, 32241944702 on handle-event-message), so this looks like pre-existing, occasional CI infra flakiness unrelated to this diff, not a regression introduced here. A retry was in progress at the time of this review. Recommend the maintainer just confirm the retry goes green before merging, but I would not block on this job's history given the above.

Verdict

No Blocker or Major issues found. The fix is correctly scoped to the described defect, does not touch unrelated modules, includes tests that reproduce both failure modes from #690 and were confirmed passing against real CI logs, and documents the one observable behavior change. The two Minor items above are code-clarity / test-robustness nits and do not need to block merging; happy to see them folded in opportunistically.

[myungjoo]

Thanks — both Minors are addressed on d84a424, and one gap the review did not raise is closed too.

Minor 1, the dead fallback in __destroy_notify() — agreed, removed (4307ba0).

Re-checked the three call sites before dropping it, and they do all guarantee a
non-NULL filter:

• ml_single_close() walks the destroy list inside if (single_h->filter) (c/src/ml-api-inference-single.c:1418-1419),
• ml_single_destroy_notify_cb() returns ML_ERROR_INVALID_PARAMETER before reaching it when the filter is NULL (:435-441),
• __release_output_data() runs from the invoke thread, which ml_single_close() joins before it drops the filter.

fw_allocated is now plain FALSE until allocate_in_invoke() sets it, so the
descriptor is cleared exactly when the framework has been told to take its
buffers back. The paragraph of 328e762's message that described the fallback is
gone with it.

Minor 2, the skip claim — the PR description was stale, not the tests.

47bae8c deliberately turned the missing-custom-filter case from a skip into a
hard failure, and the description was never updated to match. Rewrote the Tests
section: close_before_data_destroy_02_p skips when tensorflow-lite is missing,
the custom-filter tests assert, and the watchdog (not "the meson test timeout",
which the old text still mentioned) is what keeps a deadlock from stalling the
job. The description also still named libnnstreamer_customfilter_scaler_allocator,
which this PR stopped using two commits later; that is corrected as well.

On the /proc/self/exe fallback being Linux-only: leaving it as is. Every
build-tree run gets MLAPI_BUILD_ROOT_PATH from tests/meson.build:8,
debian/rules or packaging/machine-learning-api.spec:414, so the first branch
covers any OS. The fallback only matters for the installed layout, and the
only install paths in this repository are the Debian package and the Tizen RPM.
Adding _NSGetExecutablePath() for a macOS install that does not exist would be
code with no caller.

Not raised in the review: there was no _n case. close_before_data_destroy_01_p
was asserting a negative — that a write into the reclaimed output is rejected —
under a _p name. Moved it to close_before_data_destroy_01_n and widened it to
cover a full-size write and a single-element one (d84a424). Both are rejected for
the same reason: ml_tensors_data_set_tensor_data() compares data_size against
tensors[index].size, which is 0 once the framework has taken the buffer back
(c/src/ml-api-common.c:1114). _01_p now only asserts the positive half, that
the output reports NULL/0.

On the CI job that was cancelled after six hours: agreed that it is not this
PR. The hang is in MLOffloadingService.registerInvalidParam01_n
(tests/capi/unittest_capi_service_offloading.cc:272), an nnstreamer-edge path
with no relation to single-shot, and the same job passed on d9b344f, which
differs from 608a2aa by two lines of header comment. I re-ran the failed job
before pushing; the new push supersedes it either way.

[myungjoo]

Note: this review was produced by a separate review agent and is transcribed here verbatim on the author's behalf, per the pattern already used on this thread. It is an independent re-verification, not a repeat of the conclusions already posted here.

Scope

• Delta review: git diff 608a2aa d84a424 (the two commits added since the last review round — 4307ba0 product, d84a424 test — and the reply comment describing them).
• Full-PR sanity re-check: git diff 8952123 d84a424.
• Traced the runtime call graph in c/src/ml-api-inference-single.c and c/src/ml-api-common.c directly rather than trusting prior write-ups; checked CI (gh pr checks 691) and the comment history on this thread.

Verdict: no Blocker, no Major. One new Minor (test-coverage/behavioral-claim gap, not a safety defect). No regression found from the two new commits — the delta strictly narrows scope versus 608a2aa and improves test coverage.

---

1. 4307ba0 — dropping the fallback in __destroy_notify()

Verified independently, not taken on the commit message's word. __destroy_notify() is reached from exactly three call sites, and all three guarantee single_h->filter != NULL at the point of the call:

• ml_single_close() — the g_list_foreach (single_h->destroy_data_list, __destroy_notify, single_h) call sits inside if (single_h->filter) { ... } (c/src/ml-api-inference-single.c:1415-1416). Directly guarded.
• ml_single_destroy_notify_cb() — returns ML_ERROR_INVALID_PARAMETER via goto exit before reaching __destroy_notify() when !single_h->filter (:432-437). Directly guarded.
• __release_output_data() (called from __process_output() and from two sites in invoke_thread()) — all three call sites run inside invoke_thread()'s execution. ml_single_close() sets single_h->filter = NULL only at :1423, and that line is reached only after g_thread_join (single_h->thread) at :1412, which blocks until invoke_thread() has already returned. Since invoke_thread() cannot still be running once it has returned, single_h->filter cannot be NULL for the duration of any __release_output_data() call. I also confirmed single_h->filter is set to non-NULL exactly once, at ml_single_open() (:957), and to NULL only at the one line above — no other writer exists (grep -n "single_h->filter =" returns just those two lines).

So the fallback really was unreachable, and removing it (replacing the data->destroy != NULL derivation with a plain gboolean fw_allocated = FALSE; initializer) is behavior-preserving. No regression.

2. d84a424 — moving the negative assertion into close_before_data_destroy_01_n

Compared the two versions line by line (git diff 608a2aa d84a424 -- tests/capi/unittest_capi_inference_single.cc):

• close_before_data_destroy_01_p keeps both ml_tensors_data_get_tensor_data() assertions (data_ptr == NULL, data_size == 0U) unchanged. The only thing removed is the ml_tensors_data_set_tensor_data() write-should-fail check, which does not belong in a _p test per this file's own naming convention.
• close_before_data_destroy_01_n (new) re-opens the handle from scratch and checks two write attempts instead of the one the old _01_p had: a full 20-byte write (sizeof(dummy), 10×int16_t) and a single-element 2-byte write (sizeof(int16_t)), both EXPECT_NE (status, ML_ERROR_NONE). Net effect is more coverage, not less.

I confirmed the failure path this test actually exercises: after ml_single_close(), __destroy_notify() has set tensors[0].size = 0. In ml_tensors_data_set_tensor_data() (c/src/ml-api-common.c:1063-1129), the tensor info for this handle is the default _NNS_TENSOR_FORMAT_STATIC (the test only calls ml_tensors_info_set_tensor_type/set_tensor_dimension, never touches format/flexible), so the FLEXIBLE branch at :1100-1113 is skipped entirely and the rejection comes from the STATIC-format size check at :1115 (_data->tensors[index].size < data_size → 0 < 2 and 0 < 20, both true → ML_ERROR_INVALID_PARAMETER). So the test does hit a real, intentional failure path, not an incidental one (e.g. not the raw_data == NULL or index OOB checks).

3. [Minor] The negative test — and the PR's "both must be rejected" claim — cover only the STATIC branch of ml_tensors_data_set_tensor_data(), not FLEXIBLE

ml_tensors_data_set_tensor_data() has two branches for a NULL/size-0 descriptor:

• STATIC (what close_before_data_destroy_01_n exercises): the size check at c/src/ml-api-common.c:1115 catches size(0) < data_size and rejects the write. This is what __destroy_notify()'s zeroing was written to guarantee.
• FLEXIBLE (c/src/ml-api-common.c:1100-1113): when _info->info.format != _NNS_TENSOR_FORMAT_STATIC and !_data->tensors[index].data, the code does not reject — it treats the NULL pointer as "not yet allocated" and calls _ml_tensor_data_alloc() to give the descriptor a fresh buffer, then proceeds to memcpy() into it and returns ML_ERROR_NONE.

This branch is reachable for a single-shot output: single_h->invoke_dynamic (settable via ml_option "invoke_dynamic" in ml_single_open_full(), c/src/ml-api-inference-single.c:1350-1352) sets the output's GstTensorsInfo.format to _NNS_TENSOR_FORMAT_FLEXIBLE (:775-777), and ml_single_invoke_dynamic() goes through the same _ml_single_invoke_internal() → set_destroy_notify() / __process_output() / __release_output_data() machinery this PR modifies — nothing in the diff special-cases flexible-format outputs. So for a handle opened with invoke_dynamic on a framework with allocate_in_invoke == TRUE, the same "write after close" scenario this PR sets out to guard against would not be rejected: it would silently succeed by allocating new, ml-api-owned memory instead of returning an error.

To be precise about severity: this is not a memory-safety bug — _ml_tensor_data_alloc() allocates genuinely fresh memory, so there's no reach into the buffer the framework took back, and the eventual ml_tensors_data_destroy() frees it normally (no leak). It is a behavioral/contract gap: the PR description states "Both must be rejected: the descriptor the framework took back reports size 0, so neither can be copied in" as a general property, but that reasoning (and the only test added to verify it) is STATIC-only. Neither this PR nor any pre-existing test exercises invoke_dynamic + allocate_in_invoke + close-before-destroy together, so a reader could reasonably believe the invariant is unconditional when it is not.

Given ml_tensors_data_set_tensor_data() itself is untouched by this PR and the flexible-format path is a narrower, Tizen-internal-option combination, I would not block merge on this — but it's worth either a one-line qualifier in the header/PR text ("for tensors in the default static format") or a follow-up test, since a future reader relying on the stated invariant for a flexible-format single-shot handle would be wrong.

4. Full-PR (8952123..d84a424) regression re-check

Re-walked the complete product diff (c/src/ml-api-inference-single.c, +54/-14 net across the whole PR) against _ml_tensors_data_destroy_internal() in c/src/ml-api-common.c (untouched by this PR): with data->destroy cleared by __destroy_notify() before ml_tensors_data_destroy() runs, the destroy-internal call takes the else branch and does g_clear_pointer (&_data->tensors[i].data, g_free) on already-NULLed pointers — a no-op, confirming H1 (deadlock avoided, no re-entrant lock) and H2 (no double free) both hold end-to-end, not just at the call-site level. No other module is touched (git diff 8952123 d84a424 --stat — 6 files, all under c/ and tests/capi/ plus debian/ml-api-unittests.install); size is proportionate to the two-item scope (H1/H2 of #690).

Header doc (c/include/nnstreamer-single.h) and the PR's Tests/Verification sections were checked against the actual code and match: the "NULL buffer of size 0 after close" claim, the MLAPI_BUILD_ROOT_PATH export chain (debian/rules:19-20, packaging/machine-learning-api.spec:413-414 — both pre-existing, unmodified by this PR), and the watchdog/assert-vs-skip behavior all check out.

5. CI

At review time, Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") — the only job that actually executes these gtest binaries per this thread's own prior findings — was still pending on d84a424. Given the maintainer's own stated policy earlier in this thread ("I will not take this out of draft until it is green"), recommend confirming that job is green on d84a424 specifically before merging. All other checks (build ×4 ABIs, Spell Check, Static checks, DCO) were passing.

Merge recommendation

No Blocker/Major. The one new Minor (§3) is a documentation/coverage gap in a narrower configuration than this PR's stated scope, not a defect in the fix itself — does not need to block merge, but is worth a follow-up note or test. Recommend merging once the pending Tizen GBS ... unit_test 1 job on d84a424 is confirmed green.

[myungjoo]

The flexible-format Minor is accurate, and it is now scoped in the test rather than left implied. Pushed as a34fa08.

Confirmed the mechanism. ml_single_open_full() sets single_h->invoke_dynamic from the invoke_dynamic option, and ml_single_update_gst_info() then stamps _NNS_TENSOR_FORMAT_FLEXIBLE on the output info (c/src/ml-api-inference-single.c:774-777). With that format, ml_tensors_data_set_tensor_data() takes the branch at c/src/ml-api-common.c:1100-1113 before the size check: a cleared descriptor reads as "not allocated yet", so it is reallocated and the write succeeds instead of being refused. So the rejection close_before_data_destroy_01_n asserts is the static contract, not a universal one.

What changed. The test's @detail now says a static output, and the PR description names where the rejection comes from. The assertions are unchanged — they were already correct for the handle the test opens.

Why no flexible test comes with this PR. The property the Minor describes is not one this PR creates or alters:

• The release path under test is shared. ml_single_invoke_dynamic() does its info juggling and then calls ml_single_invoke() (c/src/ml-api-inference-single.c:1832), so a dynamic invoke reaches the same __process_output() / invoke_thread() sites, gets the same set_destroy_notify() treatment, and is protected by the same fix. invoke_timeout_alloc_in_invoke_p gates a regression there for both.
• The divergence is entirely inside ml_tensors_data_set_tensor_data(), which this PR does not touch. Its flexible branch behaved exactly this way before the PR for any descriptor with a NULL data pointer; the handback merely produces one more way to reach it.
• Covering it would mean driving the slow-allocator custom filter through ml_single_update_info() with flexible tensors — new test surface for the dynamic API, not for the deadlock this PR fixes.

Recording it here rather than opening a follow-up, since there is no defect to fix: the flexible write allocates a fresh buffer, __destroy_notify() has already cleared data->destroy, and the later ml_tensors_data_destroy() frees it normally. No leak, no use-after-free, no double free.

On the merge gate: agreed that Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") is the job that actually runs these cases, and it is the one being watched on the current head.

[myungjoo]

Note: this review was produced by a separate review agent and is transcribed here verbatim on the author's behalf, following the same pattern already used on this thread. It is an independent re-verification of the latest delta, not a repeat of the conclusions already posted here.

Scope

• Delta review: git diff d84a424 a34fa08 (the one commit added since the previous round's Minor finding on the FLEXIBLE-format gap).
• Full-PR sanity re-check: git diff 8952123 a34fa08 (6 files, +523/-14).
• Verified the response comment's technical claims directly against source rather than taking them on trust: the ml_single_invoke_dynamic() → ml_single_invoke() call, the format assignment on invoke_dynamic, the ml_tensors_data_set_tensor_data() STATIC/FLEXIBLE branches, and the allocator used on the FLEXIBLE realloc path.
• Checked current CI state (gh pr checks 691, gh run list).

Verdict: no Blocker, no Major, no Minor. One cosmetic Nit. The round-2 Minor is resolved precisely, without overreach. Full PR is otherwise unchanged from the round-3 assessment and remains mergeable pending the in-flight CI run on a34fa08.

---

1. The delta (d84a424 → a34fa08) is exactly what it claims to be

- * @detail Write into an output that the framework has taken its buffers
- *         from while closing the handle. The write should be rejected instead
- *         of reaching the released memory.
+ * @detail Write into a static output that the framework has taken its buffers
+ *         back from while closing the handle. The write should be rejected
+ *         instead of reaching the released memory.

Confirmed via git diff d84a424 a34fa08: one file (tests/capi/unittest_capi_inference_single.cc), a 3-line Doxygen comment change on close_before_data_destroy_01_n, zero code changes. No other file in the PR moved. This matches the "one commit, doc-only" description in the round-3 reply.

2. The narrowed claim is accurate, not merely plausible

Traced the STATIC/FLEXIBLE distinction end to end rather than accepting the reply's account:

• close_before_data_destroy_01_n opens the handle with ml_single_open() (not _open_full()/invoke_dynamic), so single_h->invoke_dynamic is FALSE. ml_single_update_gst_info() only stamps gst_info->format = _NNS_TENSOR_FORMAT_FLEXIBLE when single_h->invoke_dynamic is true (c/src/ml-api-inference-single.c:774-777); otherwise the info keeps the default _NNS_TENSOR_FORMAT_STATIC (ml_tensors_info_s default, unset by this test). So the output handle under test genuinely is STATIC, and "a static output" is a correct, non-hedging description of what the test opens — not a defensive weakening of a claim that used to be true.
• ml_tensors_data_set_tensor_data() (c/src/ml-api-common.c:1063-1129) only takes the STATIC-format rejection path (data_size <= 0 || tensors[index].size < data_size at :1115) for this handle. For a FLEXIBLE-format descriptor with a NULL data pointer (the FLEXIBLE branch at :1100-1113), it instead treats the cleared descriptor as "not yet allocated," calls _ml_tensor_data_alloc(), and returns ML_ERROR_NONE — the opposite of what this test's EXPECT_NE (status, ML_ERROR_NONE) asserts. The old wording ("Write into an output...") read as unconditional; the new wording scopes it to the branch the test actually exercises. Correctly narrowed, not overcorrected — the two EXPECT_NE assertions and the rest of the test body are untouched and still valid for the STATIC case they test.
• The PR description's Tests section already carried the matching sentence ("This is the static-format contract...") before this round; the code comment was the only place still overreaching. That gap is now closed and the two descriptions (PR body vs. inline Doxygen) agree.

3. The response comment's two supporting claims both hold up

Claim: "The release path under test is shared" — confirmed.
ml_single_invoke_dynamic() (c/src/ml-api-inference-single.c:1832) calls ml_single_invoke (single, input, output) directly, with no bypass of the timeout/release machinery this PR fixed. _ml_single_invoke_internal(), set_destroy_notify(), __process_output(), and the invoke_thread() join branch are therefore identical for a dynamic invoke and a static one; invoke_timeout_alloc_in_invoke_p (which uses the ordinary ml_single_invoke() path) does exercise the H1 fix that a dynamic-invoke caller would also rely on. This is an argument for why a redundant FLEXIBLE-specific deadlock test isn't needed — it is not a claim that the existing tests cover invoke_dynamic output handling, and the comment doesn't present it as such.

Claim: no leak/UAF/double-free on the FLEXIBLE realloc path — confirmed.
In ml_tensors_data_set_tensor_data()'s FLEXIBLE branch, before calling _ml_tensor_data_alloc() the code does g_clear_pointer (&_data->tensors[index].data, g_free) (c/src/ml-api-common.c:1106), which is a no-op here since the pointer is already NULL post-close — so there is nothing to double-free. _ml_tensor_data_alloc() then g_malloc0()s a genuinely fresh buffer and the subsequent memcpy() writes into that new allocation, never into the buffer the framework reclaimed — no UAF. The eventual ml_tensors_data_destroy() frees this fresh, ml-api-owned buffer normally — no leak. So for an invoke_dynamic handle, "write after close" on an allocating framework silently succeeds into new memory instead of being rejected — a behavioral/contract quirk, exactly as scoped — but it is not a memory-safety defect. ml_tensors_data_set_tensor_data() itself is untouched by every commit in this PR (git diff 8952123 a34fa08 -- c/src/ml-api-common.c is empty), so this PR neither introduces nor worsens that quirk; it only stopped implying its own test covers it.

4. Full PR (8952123..a34fa08) merge-readiness re-check

• Diff stat unchanged from the prior round's conclusion in substance: c/include/nnstreamer-single.h (+2), c/src/ml-api-inference-single.c (+54/-14, all inside __destroy_notify/__release_output_data/their three call sites plus the two-line close-doc addition), debian/ml-api-unittests.install (+1), tests/capi/meson.build (+8), tests/capi/ml_api_customfilter_slow_allocator.c (+113, new test-only filter), tests/capi/unittest_capi_inference_single.cc (+359, tests). No product file outside c/src/ml-api-inference-single.c changed, no header signature changed, no other module touched. Size remains proportionate: the two-line functional H1/H2 fix is the entire product delta; everything else is test scaffolding for a defect whose signature is a process-wide hang.
• H1 (self-deadlock) and H2 (buffer handback hardening) are both still addressed exactly as verified in the prior three rounds; this round's delta touches neither __destroy_notify() nor __release_output_data(), so that analysis is unchanged and was not re-litigated in depth here per the review brief.
• Test completeness: the three new tests (invoke_timeout_alloc_in_invoke_p, close_before_data_destroy_01_p/_n, close_before_data_destroy_02_p) still cover two of the three re-entrant sites deterministically, pin the "NULL/size-0 after close" contract on an allocating framework, and pin the opposite on tensorflow-lite so the clearing can't silently widen. The watchdog thread still turns a regression into a ~30 s failure rather than a multi-hour CI stall. Nothing in this round's delta changes any of that.
• Documentation: no API/ABI/architecture change; the one observable behavior change (NULL/size-0 buffer after ml_single_close() on an allocating framework) is documented in nnstreamer-single.h, unchanged by this round's commit.

5. CI status at review time

build (x86, x86_64, arm64-v8a, armeabi-v7a):        in_progress (a34fa08, pushed 07:54:19)
Tizen GBS build on Ubuntu (aarch64/armv7l, unit_test 0): in_progress
Tizen GBS build on Ubuntu (x86_64, unit_test 1):    in_progress  <- the job that runs unittest_capi_inference_single
Static checks, Spell Check, DCO:                    pass

a34fa08 is a comment-only change on top of d84a424; 608a2aa (functionally equivalent product code, prior to the two follow-up test commits) was fully green across all jobs, including Tizen GBS ... unit_test 1, so there is no code-path reason to expect a different outcome here. But per the standard already applied throughout this thread, this should not be called green until the unit_test 1 job actually completes on a34fa08 — the same job stalled to a 6-hour cancellation once on this branch's history (round 3) for reasons unrelated to this PR's code, so a confirmation rather than an inference is warranted before merging.

6. New finding

[Nit] Comment wording: "back from" split across the line break reads slightly awkwardly.
tests/capi/unittest_capi_inference_single.cc, the new @detail:

 * @detail Write into a static output that the framework has taken its buffers
 *         back from while closing the handle. The write should be rejected

"taken its buffers back from" wraps as "...has taken its buffers / back from while closing..." which reads awkwardly on the second line in isolation. Purely cosmetic; e.g. "that the framework has reclaimed while closing the handle" would read more smoothly. Not worth a review round on its own.

Merge recommendation

No Blocker/Major/Minor outstanding. The round-2 Minor (STATIC-only scope of the negative test's claim) is resolved precisely — the new wording matches what the test actually exercises, with no overreach or unwarranted narrowing, and the response's two supporting technical claims (shared release path via ml_single_invoke_dynamic(), no memory-safety issue on the FLEXIBLE realloc path) both check out against the source. The full PR (8952123..a34fa08) remains correctly scoped, proportionate, and adequately tested for the stated H1/H2 defect. Mergeable once the in-flight Tizen GBS ... unit_test 1 job on a34fa08 is confirmed green — the same bar this thread has applied at every prior round.

[myungjoo]

Nit taken as written — 502ec2e uses "whose buffers the framework has reclaimed while closing the handle", which drops the awkward wrap entirely.

It would have been tidier folded into a34fa08, but git commit --amend is not available in this environment, so it lands as its own commit. Nothing else moved: git diff a34fa08 502ec2e is the three comment lines.

No open items from any round now. Waiting on Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") for 502ec2e before this comes out of draft, per the bar this thread has used throughout.

[myungjoo]

Note: this review was produced by a separate review agent and is transcribed here verbatim on the author's behalf, following the same pattern already used on this thread. It is an independent final-round verification, not a repeat of the conclusions already posted here.

Scope

• Delta review: git diff a34fa08 502ec2e (the one commit added since the previous round's cosmetic Nit).
• Full-PR sanity re-check: git diff 8952123 502ec2e (6 files, +523/-14, unchanged stat from the round-4 assessment).
• Re-read the actual test body (close_before_data_destroy_01_n, tests/capi/unittest_capi_inference_single.cc:3595-3655) line by line to check the new wording against what the test literally does, rather than trusting the diff alone.
• Checked current CI state (gh pr checks 691).

Verdict: no Blocker, no Major, no Minor, no Nit. The delta is exactly the claimed comment-only reword, the new wording is accurate, and the full PR remains correctly scoped and adequately tested. Mergeable on code grounds; the green confirmation of Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") on 502ec2e is tracked separately and is not part of this code review.

---

1. The delta (a34fa08 → 502ec2e) is exactly what it claims to be

- * @detail Write into a static output that the framework has taken its buffers
- *         back from while closing the handle. The write should be rejected
- *         instead of reaching the released memory.
+ * @detail Write into a static output whose buffers the framework has reclaimed
+ *         while closing the handle. The write should be rejected instead of
+ *         reaching the released memory.

git diff a34fa08 502ec2e touches exactly one file (tests/capi/unittest_capi_inference_single.cc), and the change is confined to this 3-line Doxygen @detail block on close_before_data_destroy_01_n. No code line, no other test, no build/meson/debian file moved. This matches the round-4 Nit exactly (the "back from" line-wrap awkwardness) and the response's own description of the fix.

2. The new wording matches what the test actually does

Re-read the full test body (close_before_data_destroy_01_n, lines 3595-3655):

• The handle is opened with ml_single_open() (static-format output, no invoke_dynamic), matching "a static output".
• ml_single_invoke() is called once to obtain output, which is the framework-allocated (custom-filter, allocate_in_invoke) buffer — this is the "buffers" being referred to.
• ml_single_close() is called next. Per the round-3/round-4 analysis already verified on this thread, this is exactly where __destroy_notify() hands the buffer back to the framework (single_h->klass->destroy_notify()) and zeroes data->tensors[i].data/size — i.e., "the framework has reclaimed [the buffers] while closing the handle" is a precise, literal description of this call, not a loose paraphrase.
• Only after that does the test attempt two writes (ml_tensors_data_set_tensor_data(), full-size and single-element), both asserted EXPECT_NE (status, ML_ERROR_NONE) — matching "the write should be rejected instead of reaching the released memory."

So the new wording is not just grammatically smoother than the round-3 version (which was the only complaint raised), it is at least as accurate, and arguably clearer about causality ("whose buffers ... has reclaimed" directly names the actor and the action, vs. the previous "has taken its buffers back from" construction). No new inaccuracy introduced.

3. Full PR (8952123..502ec2e) merge-readiness re-check

• Diff stat is unchanged from every prior round's conclusion: c/include/nnstreamer-single.h (+2), c/src/ml-api-inference-single.c (+54/-14, confined to __destroy_notify(), the new __release_output_data() helper, and their three call sites, plus a two-line doc addition), debian/ml-api-unittests.install (+1), tests/capi/meson.build (+8, new test-only shared library), tests/capi/ml_api_customfilter_slow_allocator.c (+113, new test-only custom filter), tests/capi/unittest_capi_inference_single.cc (+359, tests). No product file outside c/src/ml-api-inference-single.c changed, no header signature changed, no other nnstreamer/api module touched.
• H1 (self-deadlock on single-shot invoke timeout) and H2 (buffer-handback hardening) were both verified correct and complete across rounds 1-4 (call-graph traced, all ml_tensors_data_destroy() sites checked, Android JNI path confirmed unaffected, _ml_tensors_data_destroy_internal() no-op path confirmed after zeroing). This round's delta touches none of that code, so nothing here changes that conclusion.
• Test completeness and CI-gating: the three tests (invoke_timeout_alloc_in_invoke_p, close_before_data_destroy_01_p/_n, close_before_data_destroy_02_p) still cover both re-entrant deadlock sites deterministically, the 30s _singleshot_watchdog still turns a reintroduced deadlock into a fast, legible CI failure instead of an hours-long stall, and the STATIC/FLEXIBLE scoping raised in round 2 and resolved in round 3 is unaffected by this round's wording-only change.
• Documentation: no API/ABI/architecture change; the one observable behavior change (NULL/size-0 buffer after ml_single_close() on an allocating framework) remains documented in c/include/nnstreamer-single.h, unchanged by this round.

4. CI status at review time

Status of the checks on 502ec2e as of writing this review (facts only, no waiting implied):

• Passed: DCO, Spell Check with Typos, Static checks (cppcheck, clang-format, etc.), build (x86_64), build (armeabi-v7a).
• Pending / still running: the remaining Android build jobs and all Tizen GBS jobs — including Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1"), which is the job that actually executes unittest_capi_inference_single and therefore the new regression tests.

Per the standard already applied at every round on this thread, this PR should not be called green until that unit_test 1 job completes on 502ec2e specifically (it stalled to a multi-hour, unrelated cancellation once earlier in this branch's history). No code-path reason exists to expect a different outcome than the prior green run on 608a2aa/a34fa08, since this round changes only a comment. Confirming that job's final result is left to the PR author/maintainer rather than to this review.

Merge recommendation

No Blocker/Major/Minor/Nit outstanding. This closes out the review thread for PR #691: the H1/H2 fix (328e762) is correct and minimally scoped, the follow-up commits added deterministic regression coverage for both deadlock sites with a CI-fast watchdog, all Minor/Nit findings raised across rounds 2-4 were resolved precisely without overreach, and this final round's delta is a verified-accurate, code-free comment reword. Mergeable on code grounds; the green confirmation of Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") on 502ec2e is a separate, still-outstanding check that is not part of this review. Given the repeated amend limitation noted earlier in this thread (six commits where fewer would normally be squashed), a squash-merge is recommended to keep main's history to the logical set of changes, but the commit count itself is not a blocking concern.

[myungjoo]

All ten checks are green on 502ec2e, so this is out of draft.

The job that matters, Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1"), ran the four cases:

[ OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_p   (200 ms)
[ OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_n   (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 200 ms on both custom-filter cases is the slow allocator's invoke delay, so the filter really did load and allocate in invoke rather than the tests skipping past it; 0 ms on the tensorflow-lite case is the same shape as before.

The six-hour cancellation from the earlier round did not recur: unittest_capi_service_offloading completed all 12 cases in this run, including MLOffloadingService.registerInvalidParam01_n, which is where the stall happened. That job took 27 minutes end to end. Nothing in this PR reaches that suite, and it now passes on the same code, which is as close to confirmation of a flaky stall as this gives.

History is six commits because git commit --amend and git reset were unavailable while addressing review rounds; each one builds on its own, so bisect is unaffected, but squash-merge is the tidier landing.

[myungjoo-bot]

Automated re-review of head 502ec2e (transcribed from an AI review agent's report; please verify before acting).

Summary: The delta since the previously approved head 47bae8c is small (4 files, +78/-9) and addresses the earlier review. The commit series was rebuilt so the test commit 608a2aa now carries gst_dep in tests/capi/meson.build and builds standalone, and the __destroy_notify fallback moved out of the [Test] commit into product commits (328e762 adds it, 4307ba0 removes it as unreachable). The PR body and 328e762's message now state precisely that __process_output() deadlocks while the JOIN_REQUESTED and exit: sites leak. The header @details in nnstreamer-single.h gained the "reports a NULL buffer of size 0 afterwards" sentence. The custom filter uses gst_tensors_info_get_nth_info(). The set_tensor_data-rejected assertion moved from close_before_data_destroy_01_p into a new _n test covering a full-size and a single-element write, correctly narrowed to the STATIC contract: on main, ml_tensors_data_set_tensor_data (ml-api-common.c:1100-1113) reallocates a NULL buffer on non-STATIC handles via _ml_tensor_data_alloc before the size check, so a reclaimed flexible output is not a crash path.

Re-verified on the new head: __release_output_data (c/src/ml-api-inference-single.c:396-410) is the single release helper at all three sites (:517, :580, :606) and calls __destroy_notify before ml_tensors_data_destroy when data->destroy is set. The dropped fallback was indeed unreachable: __destroy_notify is called only from ml_single_close inside if (single_h->filter), from ml_single_destroy_notify_cb after an early return on !single_h->filter, and from the invoke thread, which exists only after filter is created and is joined before filter is NULLed. No remaining path calls ml_tensors_data_destroy on data with a live ml_single_destroy_notify_cb under the handle mutex. All 10 checks green; the four new tests ran and passed on the GBS x86_64 lane; every commit is DCO-signed with an accurate message; merge-tree against main is clean. Approval stands.

Remaining Low, non-blocking items:

1. tests/capi/unittest_capi_inference_single.cc:3777-3788 (invoke_timeout_alloc_in_invoke_p): still no comment that the 10 ms timeout relies on the invoke thread picking up the job before the timeout fires; if it loses that race the output is NULL and the release site is skipped (leak-shaped, never a failure). A one-line note above ml_single_set_timeout (single, 10), or widening to 50 ms against the 200 ms filter delay, would document the assumption.
2. :3743-3744 and siblings: ASSERT_TRUE (test_model != NULL) cannot fire in any lane that builds tests, since the filter is built by the same meson.build. Harmless.
3. Follow-up outside this PR: c/src/ml-api-common.c:895-908 ml_tensors_data_clone() on a reclaimed STATIC output does memcpy (dst, NULL, 0) (technically UB). Suggest a separate PR guarding the copy with if (_in->tensors[i].size > 0).
4. c/include/nnstreamer-single.h:119-120: the new sentence sits inside the existing @details rather than a separate @note; this matches the surrounding style and reads correctly. No change needed.

No back-door or suspicious behavior found.

[myungjoo]

Back to draft for one more round: item 1 is a real coverage hole and is fixed in f457e74. Items 2 and 4 need nothing, and item 3 stays out of this PR for the reason you gave.

1 — the 10 ms timeout could skip the site under test. Fixed, and it is worse than "leak-shaped".

Traced it: _ml_single_invoke_internal() publishes single_h->output (c/src/ml-api-inference-single.c:1595), broadcasts, and waits. On timeout it registers the abandoned output (:1612) and then falls through exit:, which sets single_h->input = single_h->output = NULL (:1646) before unlocking. The state stays RUNNING, so an invoke thread that had not yet taken the job wakes up, reads NULL for both, and __invoke() returns ML_ERROR_STREAMS_PIPE on the invalid handle (:487-489). That lands in the status != ML_ERROR_NONE branch with output == NULL, where __release_output_data() returns immediately.

So on a lost handover the test still passes, EXPECT_EQ (status, ML_ERROR_TIMED_OUT) and all, while never reaching __process_output() or the invoke_thread release site — the two things it exists to cover. Nothing leaks, since ml_single_close() walks destroy_data_list afterwards, but the regression gate silently goes quiet. That is the failure mode worth removing, so this is a fix rather than a comment: 50 ms against the filter's 200 ms invoke, with a one-line note on why the value sits between the two.

2 — ASSERT_TRUE (test_model != NULL) cannot fire. Agreed, and intended.

That is what 47bae8c set out to do. The filter comes from the same tests/capi/meson.build as the binary, so in any lane that builds the tests it is present, and in a lane that somehow is not, the assert is exactly the loud failure that should happen instead of a silent skip. Leaving it.

3 — ml_tensors_data_clone() on a reclaimed output. Out of scope here, and this PR already improves it.

Confirmed the memcpy (dst, NULL, 0) at c/src/ml-api-common.c:909. Worth recording that the state it reaches is strictly better than before: g_tensor_filter_single_destroy_notify() in nnstreamer nulls the pointer but leaves size alone, so cloning a reclaimed output on main today is memcpy (dst, NULL, size) with a non-zero size — a real fault, not a technicality. Zeroing the size alongside the pointer is what turns it into the benign zero-length case you found.

Keeping the guard out of this PR, as you suggest. ml-api-common.c is untouched by this change, the defect predates it and is reachable from the pipeline and service APIs too, and a fix belongs where it can be tested on its own rather than inside a single-shot deadlock fix. Happy to open that PR next.

4 — header @details placement. Agreed, no change.

CI is running on f457e74; this comes out of draft once the unit_test 1 lane is green again.

[myungjoo]

This review was written by a separate AI review agent and is transcribed here verbatim (per the requester's process); please verify before acting.

Scope: Re-review of the delta since the last approved head (502ec2e → f457e74), plus a merge-readiness check of the full PR (8952123 → f457e74). The H1/H2 core fix in c/src/ml-api-inference-single.c was already verified across two prior approved rounds and is not re-litigated here.

(a) Delta 502ec2e → f457e74

The entire delta is one test-only change in tests/capi/unittest_capi_inference_single.cc (invoke_timeout_alloc_in_invoke_p): ml_single_set_timeout(single, 10) → 50, plus a one-line comment. This responds to the prior review's Low item #1 ("no comment that the 10 ms timeout relies on the invoke thread picking up the job before the timeout fires").

I traced the race independently rather than taking the PR description at face value:

• _ml_single_invoke_internal() (c/src/ml-api-inference-single.c:1598-1648): on timeout it sets status = ML_ERROR_TIMED_OUT, calls set_destroy_notify(single_h, _out, TRUE), then at exit: (:1646) unconditionally does single_h->input = single_h->output = NULL; before unlocking.
• invoke_thread() (:544-614): after waking on the shared cond var, it reads input = single_h->input; output = single_h->output; and nulls them itself (:564-567) — but only after it has been scheduled and has re-acquired the mutex.
• If the timeout in the main thread fires and runs exit: before invoke_thread gets to :564-567, then invoke_thread reads back NULL/NULL. __invoke() (:484-488) explicitly guards this ("Prevent error case when input or output is null in invoke thread") and returns ML_ERROR_STREAMS_PIPE without touching the framework. __release_output_data() (:400-401, if (!data) return;) then no-ops instead of exercising the release/destroy-notify path this test exists to cover.
• Net effect confirmed: no crash, no hang — the caller-visible contract (status == ML_ERROR_TIMED_OUT, output == NULL) still holds, so the test would still pass green while silently skipping the coverage of the in-thread release logic. This matches the PR's stated rationale exactly.

On the 50 ms value: the custom filter's pt_allocate_invoke() sleeps INVOKE_DELAY_USEC = 200000 (tests/capi/ml_api_customfilter_slow_allocator.c:26), so 50 ms still fires well inside the invoke (consistent with the new comment "fires inside the filter's invoke"), while giving invoke_thread 5x more scheduling headroom than the old 10 ms to win the race described above. I found no sanitizer/valgrind lanes in .github/workflows/*.yml, and this test only runs under the Tizen GBS x86_64 unit_test=1 lane, so the added 40 ms is immaterial against that test's own 500 ms sleep and the 30 s watchdog budget, and does not affect other lanes (which don't build/run unit tests at all).

• [Minor] tests/capi/unittest_capi_inference_single.cc:3773 — widening to 50 ms lowers the probability of the race, it does not eliminate it; the race is still theoretically present at 50 ms (or any finite value) if the scheduler stalls long enough. Given the residual failure mode degrades to a harmless one-time handle leak rather than a crash/hang/flake (as already agreed in the prior review round), this is an acceptable pragmatic mitigation for a test-only file. A fully deterministic fix (e.g., invoke_thread signaling "job taken" before the timeout wait can resolve) would be more correct but is out of proportion for this change. No action required, noting for the record only.

No regression risk: this commit touches only one test's timeout constant and a comment; it cannot affect any other module's function or performance.

(b) The 3 unaddressed Low items from myungjoo-bot's 502ec2e review

1. ASSERT_TRUE (test_model != NULL) cannot fire — agreed, no action needed. Verified tests/capi/meson.build:2-14: libml_api_customfilter_slow_allocator and unittest_capi_inference_single are both built unconditionally by the same meson.build, gated only by the same install-test option used elsewhere in this file for install, not for building. Any build that produces the test binary also produces the filter next to it (or in the install dir via debian/ml-api-unittests.install, confirmed already added in this PR), so the assert is a correct "fail loudly if the build is broken" guard rather than a dead/incorrect check. Concur with treating this as non-actionable.

2. memcpy(dst, NULL, 0) in ml_tensors_data_clone() — agreed, correctly deferred to a separate PR. Verified c/src/ml-api-common.c:897-909: when _out->tensors[i].data is already set (as happens post-__destroy_notify), it skips the alloc branch and goes straight to memcpy(_out->tensors[i].data, _in->tensors[i].data, _in->tensors[i].size). Verified __destroy_notify() (c/src/ml-api-inference-single.c:376-384) zeroes both data->tensors[i].data and .size on reclaim, so a clone of a reclaimed static output does hit memcpy(ptr, NULL, 0) — technically UB per the C standard even at size 0, though a no-op in every real-world memcpy. Confirmed this PR does not introduce this: before H2, the same call site read a stale/dangling pointer with the original non-zero size, i.e. a real use-after-free read; after H2 it's UB-in-name-only. So the PR strictly improves this path and it is reasonable to fix the residual UB in a dedicated follow-up PR rather than scope-creep this one.

3. Header @details placement — agreed, no change needed. Spot-checked c/include/nnstreamer-single.h; the added sentence about the reclaimed-output contract reads naturally inline with the surrounding @details block and does not need to be split into a separate @note. Non-issue.

(c) Merge readiness (8952123 → f457e74)

• gh pr view 691 reports mergeable: MERGEABLE against current main.
• Full diff footprint: c/include/nnstreamer-single.h (+2 doc lines), c/src/ml-api-inference-single.c (the H1/H2 fix), debian/ml-api-unittests.install, tests/capi/meson.build, tests/capi/ml_api_customfilter_slow_allocator.c (new), tests/capi/unittest_capi_inference_single.cc. This stays tightly scoped to issue [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690 — no unrelated module is touched, and the size is proportionate to a concurrency bug fix plus the regression test infrastructure it needs (a real custom filter, a watchdog thread, and dedicated test cases). No concern under the "PR size vs. topic" or "touches unrelated modules" checks.
• No API/architecture change beyond the documented reclaimed-output contract in nnstreamer-single.h, which is already updated in this PR (confirmed present, not left for a follow-up).
• CI for f457e74 was just triggered at review time: DCO = pass, all other checks (Static checks, Spell Check, GBS builds incl. the x86_64 unit_test=1 lane that actually exercises these tests, Android build (*)) = pending. Per the project's own bar used in the prior two approvals ("all 10 checks green"), merge should wait for this lane to go green rather than being decided from this review alone; I did not poll for completion per instructions.
• Commit count is 7 (unfoldable in this review environment due to a tooling restriction, not a code issue). Not a blocker; squash-merge is recommended at merge time for a cleaner history, consistent with earlier "commit hygiene" feedback on this same PR.

Summary

No Blocker or Major findings in the 502ec2e → f457e74 delta or in the full PR diff. Two Minor/Nit notes, both non-blocking:

• The 50 ms timeout is a probabilistic (not deterministic) fix for the invoke-thread race; acceptable given the residual failure mode is a benign leak, not a flake or crash.
• Recommend squash-merge given the 7-commit history (environment limitation, not a code defect).

The three previously-deferred Low items are all judged correctly deferred/non-actionable, with independent verification of each claim against the current source (meson.build, ml-api-common.c, ml-api-inference-single.c, nnstreamer-single.h).

Merge recommendation: mergeable once the pending CI checks for f457e74 (in particular the x86_64 unit_test=1 GBS lane) come back green; no code changes requested.

[myungjoo-bot]

Automated re-review of head f457e74 (delta since the previously approved 502ec2e; verified directly by the review process, no separate agent this round).

Summary: The delta is confined to tests/capi/unittest_capi_inference_single.cc:3773-3774 in invoke_timeout_alloc_in_invoke_p: ml_single_set_timeout (single, 10) -> 50 with a one-line comment. This closes the remaining Low item from the previous round, and the author's analysis of why it matters checks out against the PR-side source: on a lost handover _ml_single_invoke_internal() registers the abandoned output on timeout and then exit: NULLs single_h->input / output before unlocking; the invoke thread then wakes to NULLs, __invoke() returns ML_ERROR_STREAMS_PIPE, and __release_output_data() returns immediately on output == NULL — so the test would still report ML_ERROR_TIMED_OUT and pass while never reaching __process_output() or the invoke_thread release site. With the custom filter's INVOKE_DELAY_USEC at 200 ms, a 50 ms timeout fires inside the filter's invoke but after the thread has taken the job, and the 500 ms g_usleep between the two invokes still exceeds 50 + 200 ms so the first release completes before the close-pending path is exercised. The core H1/H2 fix in c/src/ml-api-inference-single.c is unchanged from the two previously approved rounds and is not re-litigated.

CI on f457e74: DCO, Spell Check, Static checks, Android (4 ABIs) and Tizen GBS x86_64 unit_test 1 all pass; the GBS aarch64 / armv7l unit_test 0 build lanes were still pending at review time (build-only lanes, test-only delta). The x86_64 job log (job 101899143606) shows close_before_data_destroy_01_p OK (200 ms), close_before_data_destroy_01_n OK (200 ms), close_before_data_destroy_02_p OK, invoke_timeout_alloc_in_invoke_p OK (801 ms), 56/56 in unittest_capi_inference_single. The new commit carries Signed-off-by and an accurate message. Approval stands; nothing further requested. The ml_tensors_data_clone() zero-length guard remains a separate follow-up as agreed.

No back-door or suspicious behavior found.

[myungjoo]

All ten checks are green on f457e74, so this is out of draft again.

The unit_test 1 lane ran the four cases:

[ OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_p   (200 ms)
[ OK ] nnstreamer_capi_singleshot.close_before_data_destroy_01_n   (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 wider timeout does not cost anything measurable: the timeout test is still 801 ms, since it is dominated by the 500 ms gap between the two invokes and the filter's 200 ms invoke, not by the 10 ms the timeout grew by. MLOffloadingService.registerInvalidParam01_n, the case that stalled a job two rounds ago, passed in 25 ms.

Nothing outstanding from any review round. Squash-merge stays the tidier landing for the seven commits.

[myungjoo]

Seven commits became three, split by topic and force-pushed with a lease on f457e74.

• 5774dec [C-Api] fix self-deadlock on single-shot invoke timeout — c/src/ml-api-inference-single.c alone.
• 3f208e5 [Doc] note the output buffer state after ml_single_close() — c/include/nnstreamer-single.h alone.
• fa87dcd [Test] cover the single-shot invoke timeout in CI — the custom filter, its meson target, debian/ml-api-unittests.install and the four test cases.

The code did not change. git diff f457e74 fa87dcd is empty, and both heads have tree 763c159. Everything that was reviewed across the rounds above is byte for byte what is here now; only the commit boundaries and messages moved.

The messages absorbed what the follow-up commits were carrying, so nothing is lost with them:

• The __destroy_notify() fallback that 4307ba0 removed as unreachable was never introduced in the first place, and 5774dec's message states why allocate_in_invoke() alone decides whether the descriptor is cleared.
• fa87dcd's message carries the reasons the later test commits gave: why the negative case is a separate _n, why its refusal is the static contract rather than a general one, and why the timeout sits at 50 ms rather than tighter.

Each commit builds on its own — the fix does not depend on the header comment, and the tests come last — so bisect lands somewhere useful. CI is re-running on fa87dcd.

[myungjoo-bot]

Automated re-review of head fa87dcd (verified directly by the review process; no separate agent this round).

Summary: git diff f457e74 fa87dcd is empty and both heads resolve to tree 763c159, so the code, header, tests, meson wiring and Debian install line are byte-identical to what was approved in the previous rounds; only the commit boundaries and messages changed (7 commits -> 3). Checked per commit: 5774dec [C-Api] touches c/src/ml-api-inference-single.c alone (+40/-14) and already contains __release_output_data and its three call sites, so the fix builds without the header or the tests; 3f208e5 [Doc] touches c/include/nnstreamer-single.h alone (+2); fa87dcd [Test] carries the custom filter, its meson target, debian/ml-api-unittests.install and the four test cases (+482). Every commit has Signed-off-by. The rewritten fix message is accurate: it now distinguishes the __process_output() deadlock from the leak-vs-deadlock behaviour of the JOIN_REQUESTED branch and the exit: label, states why allocate_in_invoke() alone decides whether the descriptor is cleared (every caller reaches __destroy_notify() with filter non-NULL, which matches what was verified two rounds ago), and records the single observable change (NULL buffer of size 0 after ml_single_close()).

CI on fa87dcd: all 10 checks pass. The GBS x86_64 unit_test 1 job (101971492824) shows close_before_data_destroy_01_p OK (200 ms), close_before_data_destroy_01_n OK (200 ms), close_before_data_destroy_02_p OK, invoke_timeout_alloc_in_invoke_p OK (801 ms), 56/56 in unittest_capi_inference_single, and every other suite green. merge-base is current main (8952123). Approval stands; nothing further requested.

No back-door or suspicious behavior found.