[Fix] free custom connection when the custom library fails to load

[myungjoo]

nns_edge_custom_load() leaks the custom_connection_s it allocated whenever _load_custom_library() fails: the error: label handed the partially constructed handle to nns_edge_custom_release(), which early-returns on a NULL instance and therefore never reaches its free(). 24 bytes per failed attempt, on a path any application reaches by retrying nns_edge_custom_create_handle() with a bad or unavailable lib_path.

Reproduced under LeakSanitizer against the existing test suite before any change:

Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #1 nns_edge_custom_load        src/libnnstreamer-edge/nnstreamer-edge-custom-impl.c:91
    #2 nns_edge_custom_create_handle   src/libnnstreamer-edge/nnstreamer-edge-internal.c:1356
    #3 edgeCustom_createHandleLoadFail_n_Test::TestBody()
SUMMARY: AddressSanitizer: 24 byte(s) leaked in 1 allocation(s).

What changed

The fix. nns_edge_custom_load() now handles its two failure paths separately instead of funnelling both through one error: label:

failure	state	cleanup
_load_custom_library() failed	instance is NULL, dl_handle already closed by _load_custom_library()'s own error path	SAFE_FREE (custom)
plugin create() failed	instance is valid	nns_edge_custom_release (custom) — unchanged

nns_edge_custom_release() itself is untouched, so nothing else that calls it changes. Return values and the out-param contract (*handle is written only on success) are identical to before.

Tests. The create()-failure branch had no coverage at all, so tests/nnstreamer-edge-custom-test.c gains an NNS_EDGE_CUSTOM_TEST_FAIL_CREATE environment switch that makes its nns_edge_custom_create() return an error. Five tests:

• loadFail_n / loadCreateFail_n — repeat each failure 100x, so a returning leak reports as 100 allocations rather than one easily-missed 24-byte record. Both also assert the out-param is not left pointing at freed memory, which is exactly what nns_edge_release_handle() relies on.
• createHandleCreateFail_n — the public-API negative test for the create() path, mirroring the existing createHandleLoadFail_n.
• createHandleAfterFailure — a failed load and a failed create must not disturb a later successful create on the same library (covers the dlclose() on the failure path).
• liveHandleAfterFailedLoads — a live handle keeps working through 50 failed loads of the library it is using.

CI. The leak is invisible to a normal test run: every gtest assertion passes with or without the fix. Without a sanitizer run, this and any future leak merges silently. ubuntu_clean_cmake_build.yml now builds and runs the suite a second time under AddressSanitizer. The sanitizer steps sit inside the existing build job on purpose: main's required status checks are ["DCO", "build"], so a separate job would report a leak without actually blocking the merge. Verified: against main the sanitizer step fails with the 24-byte leak above; with this branch it is clean, and the rest of the suite is already sanitizer-clean, so it does not go red for unrelated reasons.

Log noise. nns_edge_release_handle() warned Failed to close custom connection. whenever it released a handle whose custom_connection_h was NULL - which is every failed custom create, i.e. exactly the path this PR governs. It pointed at a close that was never attempted. Guarded.

Docs. include/nnstreamer-edge-custom.h now states that nns_edge_custom_close() is still called with the priv of a failed create(). This is existing behaviour on the create()-failure path, deliberately left as is; it was just never written down for custom library authors. Changing it would alter the contract for out-of-tree plugins and belongs in its own change if wanted.

Verification

	plain build	ASan build
this branch	31/31 custom + full suite pass	pass, zero leaks
fix reverted, tests kept	31/31 still pass	fails — 2448 bytes in 102 allocations

Also run with --gtest_shuffle --gtest_repeat=3 to confirm the environment switch never leaks between tests. clang-format and GNU indent checked locally against the repo config.

Fixes #252
Fixes #253

🤖 Generated with Claude Code

[myungjoo]

이 리뷰는 별도 컨텍스트의 코드 리뷰 에이전트가 작성한 내용을 옮긴 것입니다. (gh pr diff, 관련 이슈 #252/#253, 그리고 WSL 환경에서의 ASan/plain 빌드+ctest 실제 실행을 통해 검증했습니다.)

검토 범위 및 방법

• nnstreamer-edge-custom-impl.c, nnstreamer-edge-custom.h, nnstreamer-edge-custom-test.c, unittest_nnstreamer-edge-custom.cc, .github/workflows/ubuntu_clean_cmake_build.yml 변경분 전체를 정독.
• nns_edge_custom_release(), nns_edge_custom_create_handle(), nns_edge_release_handle() 등 diff 밖에 있는 호출부/피호출부까지 추적해 out-param 계약(성공 시에만 *handle 기록) 및 custom->dl_handle 초기화 흐름을 확인.
• 로컬 WSL에서 ASan 빌드(-DCMAKE_BUILD_TYPE=Debug -fsanitize=address, ASAN_OPTIONS=detect_leaks=1)와 plain 빌드로 각각 cmake --build + ctest 실행 → 둘 다 2/2 테스트 통과, 컴파일 경고 0건.
• unittest_nnstreamer-edge-custom 바이너리에서 신규 테스트 5종을 --gtest_filter로 골라 --gtest_shuffle --gtest_repeat=3 옵션으로 재실행 → ASan 하에서도 5/5 통과, 누수 없음. 전체 31개 테스트도 PR 설명의 수치와 일치.
• GitHub 저장소의 branch protection 설정(repos/.../branches/main/protection)을 조회해 required status checks 목록 확인.

주제 해결 여부

nns_edge_custom_load()가 _load_custom_library() 실패 시 부분 생성된 custom_connection_s를 nns_edge_custom_release()에 넘겨 조기 return(!instance 체크)으로 인해 free()가 호출되지 않던 24바이트 누수(#252)를 두 실패 경로를 분리하는 방식으로 정확히 수정했습니다. _load_custom_library() 실패 시엔 custom->dl_handle/custom->instance가 calloc으로 NULL 상태이므로 SAFE_FREE(custom)만으로 충분하고(dl_handle은 _load_custom_library() 자신의 에러 경로에서 이미 dlclose()됨), create() 실패 시엔 instance가 유효하므로 기존과 동일하게 nns_edge_custom_release()를 사용합니다. 성공 경로(*handle = custom 후 NNS_EDGE_ERROR_NONE 반환)와 create() 실패 시의 반환값(ret)도 원래 동작과 동일하게 유지되어 있음을 확인했습니다. 커버리지 부재였던 #253(create() 실패 경로 테스트)도 NNS_EDGE_CUSTOM_TEST_FAIL_CREATE 환경변수 기반 failure-injection과 5개 테스트로 해결했습니다.

지적 사항

[major] build-asan 잡이 branch protection의 required status checks에 없어 CI 실패가 병합을 막지 못함

repos/nnstreamer/nnstreamer-edge/branches/main/protection을 조회하면 required_status_checks.contexts가 ["DCO", "build"]로, 이번에 추가된 build-asan은 포함되어 있지 않습니다. 즉 이 PR이 잘 만든 ASan 잡이 실제로 리크/메모리 오류를 잡아 빨간불이 켜지더라도, GitHub UI의 "Merge" 버튼은 막히지 않고 관리자가 수동으로 확인해야만 하는 상태입니다. PR 설명에서 "CI가 조용히 리크를 통과시킨다"는 문제의식으로 이 잡을 추가했는데, required check로 등록되지 않으면 동일한 문제(사람이 놓치면 병합됨)가 형태만 바뀐 채 남습니다.
이는 PR diff(워크플로 yml)만으로는 고칠 수 없는 저장소 설정(Settings → Branches → Branch protection rules) 영역이라 이 PR을 막을 이유는 아니지만, 병합 후 반드시 build-asan을 required status check로 추가하는 후속 조치가 필요합니다. (본인이 저장소 관리자이시니 직접 처리 가능하실 것으로 보입니다.)

[minor] create() 실패 후 close(priv=NULL) 호출 시 로그 소음

createHandleCreateFail_n 실행 시 nns_edge_custom_release()가 nns_edge_custom_close(NULL)을 호출하고, 테스트 커스텀 라이브러리 구현은 이를 INVALID_PARAMETER로 거부하면서 Invalid param, handle should not be null. 에러 로그를 남깁니다. 이어서 nns_edge_release_handle()도 nns_edge_custom_release(NULL)을 한 번 더 호출해 Failed to close custom connection. 경고를 추가로 남깁니다(ASan 실행 로그에서 실제 확인됨). 크래시나 이중 해제는 아니고 이번 PR이 만든 동작도 아니며(기존에도 동일 경로가 존재), 헤더에 새로 추가한 문서("close()는 실패한 priv에 대해 tolerate해야 한다")와도 부합하는 동작이라 блокер는 아니지만, 실패 경로를 재현하는 애플리케이션 로그에 매 시도마다 오해의 소지가 있는 에러/경고가 두 줄씩 쌓이는 점은 참고하시면 좋겠습니다.

회귀/부작용 점검

• nns_edge_custom_release() 자체는 변경되지 않아 이 함수를 쓰는 다른 모든 호출부(start/stop/connect/disconnect/send_data 등)에 영향 없음.
• 성공 경로의 반환값·out-param 기록 시점이 이전과 동일 (*handle은 성공 시에만 기록) → nns_edge_custom_create_handle()이 의존하는 "실패 시 out-param을 dangling 상태로 남기지 않는다"는 계약(nns_edge_custom_create_handle() leaves *edge_h dangling when the custom library fails to load #249/#250에서 확립)과 일치.
• .c/.h 포맷은 CI의 clang-format(.cc/.hh/.hpp/.cpp 대상) 검사 범위 밖이고 GNU indent 검사는 애초에 경고만 내고 빌드를 막지 않는 non-blocking 체크인데, 실제 PR의 "Static checks" 잡은 SUCCESS로 통과했습니다.

코드 크기/범위

핵심 수정은 nnstreamer-edge-custom-impl.c 6줄 추가/8줄 삭제로 최소화되어 있고, 나머지 증가분(테스트 125+5줄, CI 워크플로 28줄, 헤더 문서 4줄)은 모두 이슈 #252/#253이 명시적으로 요구한 "재현·검증·회귀 방지" 목적에 직접 대응됩니다. 다른 모듈(HYBRID/MQTT 연결 등)에는 손대지 않았습니다.

테스트 커버리지 평가

loadFail_n/loadCreateFail_n은 실패를 100회 반복해 한 번의 24바이트 누수가 아니라 다중 할당 리포트로 만들어 리그레션 감지력을 높였고, out-param이 해제된 메모리를 계속 가리키지 않는지도 함께 검증합니다. createHandleAfterFailure/liveHandleAfterFailedLoads는 실패한 로드가 동일 라이브러리의 이후 성공적인 로드나 이미 살아있는 핸들에 부작용을 주지 않는지 캡처해 dlclose() 처리 경로까지 검증합니다. 실제로 로컬 ASan 재실행(--gtest_shuffle --gtest_repeat=3)에서도 안정적으로 통과했습니다. build-asan CI 잡이 이 테스트들을 정기적으로 ASan 하에서 실행하므로, 향후 다른 모듈 변경이 이 경로의 메모리 안전성을 깨뜨릴 경우 (위의 required-check 이슈만 해결되면) 감지 가능합니다.

문서

include/nnstreamer-edge-custom.h에 close()가 실패한 create()의 priv로도 호출된다는 계약을 명시한 것은 적절하며, PR 범위 안에서 함께 처리되었습니다. 이 외에 ARCHITECTURE 변경이나 공개 API 시그니처 변경은 없어 추가 문서 수정은 불필요합니다.

결론

Merge 가능(approve) 으로 판단합니다. 핵심 버그 수정은 정확하고 최소 범위이며, 회귀 테스트가 충실하고 실측(ASan)으로 검증되었습니다. 위 major 지적은 이 PR 자체의 diff로 해결할 수 없는 저장소 설정 후속 작업이므로 병합을 막을 이유는 아니지만, 병합 직후 build-asan을 required status check로 등록하는 것을 강력히 권장합니다.

[myungjoo]

리뷰 감사합니다. 두 지적 모두 이번 PR 안에서 처리했습니다.

[major] build-asan가 required status check가 아니라는 지적

타당합니다. 확인해보니 main의 required status checks는 ["DCO", "build"] 두 개뿐이라, 별도 잡으로 둔 ASan은 리크를 잡아도 Merge 버튼을 막지 못합니다. "CI가 조용히 리크를 통과시킨다"는 문제가 형태만 바뀌어 남는다는 지적 그대로입니다.

저장소 설정 변경 없이 PR 안에서 해결했습니다. build-asan을 별도 잡으로 두는 대신, sanitizer 빌드/테스트를 required check인 build 잡의 단계로 편입했습니다. 이제 리크가 검출되면 build 체크가 실패하므로 실제로 Merge가 막힙니다. 부수적으로 러너 하나와 apt install 한 번을 아낍니다.

의도가 코드에서 드러나지 않으므로 워크플로에 이유를 주석으로 남겼습니다.

[minor] 실패 시도마다 오해를 부르는 로그 두 줄

두 줄 중 아래쪽(nns_edge_release_handle()의 Failed to close custom connection. 경고)은 고쳤습니다. 이 경고는 eh->custom_connection_h가 NULL일 때 — 즉 custom create가 실패한 모든 경우, 바로 이 PR이 다루는 경로에서 — 시도조차 하지 않은 close를 실패했다고 알리고 있었습니다. NULL 가드 한 줄을 추가했습니다.

지적하신 대로 PR이 만든 동작은 아니지만, 이번에 추가한 테스트가 실행당 150회 이상 이 경로를 밟기 때문에 CI 로그가 이 경고로 뒤덮입니다. 이 PR의 소관이라고 판단해 포함했습니다.

위쪽 한 줄(Failed to stop custom connection.)은 플러그인의 close(priv=NULL)이 내는 로그라 라이브러리 동작을 바꾸지 않고는 없앨 수 없습니다. create() 실패 후 close()를 부르는 것이 계약인지 여부는 #253이 "문서화하거나 고칠 것"으로 남긴 항목인데, 고치는 쪽은 out-of-tree 플러그인의 계약을 바꾸는 일이라 이번 PR에서는 헤더에 계약을 명시하는 쪽을 택했습니다.

참고

nnstreamer-edge-internal.c를 건드리게 되면서 indent 체커가 이 파일의 기존 스타일 이탈(nns_edge_start_discovery/nns_edge_stop_discovery의 반환 타입 줄바꿈, 파일 끝 빈 줄)을 함께 보고할 수 있습니다. 모두 main에 이미 있던 것이고 제 수정 라인은 indent clean이며, indent 체커는 warning-only(exit 0)입니다. 무관한 재포맷으로 diff를 늘리지 않으려 그대로 두었습니다.

검증: plain / ASan 빌드 모두 통과(경고 0), --gtest_shuffle --gtest_repeat=3 무누수, 수정을 되돌리면 ASan이 2448 bytes / 102 allocations로 실패. 직전 푸시에서 CI의 sanitizer 실행이 ubuntu-24.04에서 57초에 통과한 것도 확인했습니다.

[myungjoo]

이 리뷰는 별도 컨텍스트의 코드 리뷰 에이전트가 작성한 내용을 옮긴 것입니다. 1차 리뷰(major/minor 두 건)에 대한 대응을 검증하는 2차 리뷰입니다. 1차 리뷰 내용과 중복되는 부분은 다시 적지 않고, 갱신된 코드에 대한 판단과 새로 확인한 내용 위주로 남깁니다.

검증 방법

• 현재 push된 diff(.github/workflows/ubuntu_clean_cmake_build.yml, nnstreamer-edge-internal.c)를 정독.
• gh api repos/nnstreamer/nnstreamer-edge/branches/main/protection --jq .required_status_checks 로 required check 목록 재확인.
• gh pr checks 255 로 이 PR의 실제 커밋에 대해 리포트된 체크 이름과 결과를 확인 (workflow job id와 required check context가 실제로 일치하는지).
• WSL에서 이 worktree를 대상으로 plain / ASan 빌드 및 ctest 재실행(둘 다 2/2 통과).
• unittest_nnstreamer-edge-custom을 직접 실행해 두 로그 라인(Failed to close custom connection. / Failed to stop custom connection.) 각각의 발생 여부를 grep으로 확인.

[major] → 해결 확인

build-asan을 별도 잡 대신 required check인 build 잡의 스텝으로 편입한 방식은 실제로 동작합니다.

• branches/main/protection의 required_status_checks.contexts는 여전히 ["DCO", "build"]이고, workflow(name: CMake)의 job id도 build이므로 컨텍스트명이 정확히 일치합니다.
• gh pr checks 255 결과, 이 PR 헤드 커밋에서 보고되는 체크 이름이 정확히 build이며 (job 자체의 이름이 곧 컨텍스트가 됨), 이 체크 안에 일반 빌드/테스트 스텝 뒤에 ASan configure/build/test 스텝이 이어져 1m16s에 pass로 기록된 것을 확인했습니다. 즉 ASan 단계가 실패하면 이 required check 자체가 fail하므로 merge 버튼이 막히는 구조가 맞습니다.
• 로컬에서도 별도로 plain/ASan 빌드 + ctest를 재실행해 둘 다 2/2 통과를 재확인했습니다.
• 워크플로에 남긴 주석("이 job에 있어야 required check로 커버된다")도 의도를 명확히 설명하고 있어 향후 유지보수 시에도 실수로 다시 분리되는 것을 방지하는 데 도움이 됩니다.

[minor] → 부분 해결, 판단은 적절함

nnstreamer-edge-internal.c의 eh->custom_connection_h NULL 가드로 아래쪽 로그(Failed to close custom connection.)는 실제로 사라졌습니다. createHandleCreateFail_n/createHandleAfterFailure/liveHandleAfterFailedLoads/loadCreateFail_n을 모아 직접 실행해 grep한 결과 이 문자열은 0건이었습니다.

위쪽 로그(Failed to stop custom connection., nnstreamer-edge-custom-impl.c의 nns_edge_custom_release()가 테스트 플러그인의 nns_edge_custom_close(NULL) 실패를 받아 남기는 것)는 동일한 실행에서 여전히 매 실패 시도마다 찍히는 것을 확인했습니다. 코드 변경 없이 헤더에 계약("close()는 생성되지 않은 priv도 tolerate해야 한다")만 명시한 선택은 합리적입니다. 이 로그는 core 라이브러리가 아니라 테스트용 참조 플러그인이 priv == NULL을 스스로 에러로 처리해서 나는 것이므로, 계약을 지키는 실제 out-of-tree 플러그인이라면 이 경로에서 굳이 로그를 남기지 않아도 됩니다. 다만 이번에 추가된 테스트가 실행마다 150회 이상 이 경로를 밟기 때문에 CI 로그에는 여전히 노이즈가 상당히 남는다는 점은 그대로입니다.

• [nit] 참고로, 테스트 플러그인(tests/nnstreamer-edge-custom-test.c)의 nns_edge_custom_close()가 새 헤더 계약("NULL priv를 tolerate")에 맞춰 조용히 성공을 반환하도록 고치면 이 로그도 사라지고, 헤더에 문서화한 계약을 참조 구현이 직접 시연하는 효과도 있습니다. 하지만 이는 이번 PR의 스코프(nns_edge_custom_load() leaks the connection struct when the custom library fails to load #252/No test coverage for the nns_edge_custom_create() failure path in nns_edge_custom_load() #253) 밖이라 이 PR을 막을 이유는 아닙니다.

회귀 점검 (NULL 가드 관련, 추가 확인)

nns_edge_release_handle()의 NULL 가드는 switch (eh->connect_type)의 case NNS_EDGE_CONNECT_TYPE_CUSTOM: 블록 안에서만 적용되어 HYBRID/MQTT/기타 connect type 경로에는 전혀 영향이 없습니다. 또한 eh->custom_connection_h는 핸들 생성 시 NULL로 초기화되고(nnstreamer-edge-internal.c:1299), CUSTOM 핸들이 nns_edge_custom_load() 실패로 인해 NULL로 남는 경우(nns_edge_custom_create_handle() 내부, 1356행 부근)를 제외하면 성공적으로 연결된 핸들에서 이 값이 다시 NULL이 되는 다른 경로는 없습니다(release_handle 자신이 마지막에 NULL로 지우는 라인은 이 switch 이후라 무관). 즉 이 가드는 "생성 실패로 애초에 연결이 없었던" 경우만 걸러내며, 정상적으로 연결된 CUSTOM 핸들에 대한 실제 close 실패 경고를 숨기지 않습니다. 회귀 없음으로 판단합니다.

결론

**Merge 가능(approve)**으로 판단합니다. 1차 리뷰의 major 지적(ASan이 merge를 막지 못함)은 코드/설정 양쪽 다 확인한 결과 실질적으로 해결되었고, minor 지적도 핵심 노이즈(이중 경고)는 제거되었으며 남은 한 줄에 대한 판단(문서화로 계약 명시, 실제 동작 변경은 보류)도 타당합니다. 새로 발견한 문제는 없으며, 위 nit은 후속 개선 제안일 뿐 이 PR을 막을 이유는 아닙니다.

[myungjoo]

2차 리뷰 감사합니다. 남기신 nit도 반영했습니다.

지적하신 대로, 헤더에 "nns_edge_custom_close()는 create되지 않은 priv를 허용해야 한다"는 계약을 새로 적어놓고 정작 그 헤더가 예제로 가리키는 참조 구현(tests/nnstreamer-edge-custom-test.c)은 NULL priv를 에러로 거절하고 있었습니다. 제가 문서를 추가하면서 만든 불일치이므로 스코프 밖이라고 보기 어렵다고 판단해, 참조 구현이 계약을 따르도록 고쳤습니다.

결과적으로 실패 경로에서 나오던 오해 소지 있는 로그 세 종류가 모두 사라졌습니다. 테스트 1회 실행 기준:

Failed to close custom connection.        0건
Failed to stop custom connection.         0건
Invalid param, handle should not be null. 0건

검증: plain / ASan 빌드 모두 통과, --gtest_shuffle --gtest_repeat=3 31/31 × 3회 통과, 누수 0, GNU indent clean.

[myungjoo]

이 리뷰는 별도 컨텍스트의 코드 리뷰 에이전트가 작성한 내용을 옮긴 것입니다. 1차 리뷰(major 1 + minor 1)와 2차 리뷰(approve + nit 1)에 이어지는 3차 리뷰이며, 2차 리뷰의 nit에 대한 이번 대응분만 집중 검토했습니다. 이전에 다룬 내용은 반복하지 않습니다.

검증 방법

• 현재 push된 커밋(d3e1189, PR head와 일치 확인)의 diff를 gh pr diff 255로 재확인. 2차 리뷰 이후 실제로 바뀐 파일은 tests/nnstreamer-edge-custom-test.c 하나뿐임을 확인했습니다.
• nns_edge_custom_release() (nnstreamer-edge-custom-impl.c) → nns_edge_release_handle() (nnstreamer-edge-internal.c)로 이어지는 반환값/로그 연쇄를 코드로 재추적.
• WSL에서 plain / ASan 빌드 재수행 (ctest, 둘 다 2/2 통과).
• unittest_nnstreamer-edge-custom을 (ctest와 동일하게 LD_LIBRARY_PATH에 테스트 디렉터리를 포함시켜) 직접 실행, --gtest_shuffle --gtest_repeat=3으로 31개 테스트 × 3회 재실행 → 전부 PASSED, ASan 오류/누수 0건.
• 세 로그 문자열(Failed to close custom connection. / Failed to stop custom connection. / Invalid param, handle should not be null.)을 grep → 3회 반복 전체 로그에서 0건, 작성자 코멘트의 수치와 일치.

이번 변경(tests/nnstreamer-edge-custom-test.c의 nns_edge_custom_close())에 대한 판단

변경 자체는 diff 6줄 수준으로 최소이며, 2차 리뷰에서 지적한 "헤더 계약과 참조 구현의 불일치"를 정확히 해소합니다. priv(=custom_h)가 NULL이면 에러 로그 없이 NNS_EDGE_ERROR_NONE을 반환하도록 바뀌었고, non-NULL인 경우의 로직(SAFE_FREE(peer_address), SAFE_FREE(custom_h), 반환값)은 그대로입니다.

회귀 점검 (요청하신 연쇄 추적): nns_edge_custom_release()는 custom->priv를 그대로 close()에 넘기고 그 반환값을 최종 리턴값으로 사용합니다(nnstreamer-edge-custom-impl.c:134-148). custom->priv가 NULL인 유일한 경우는 nns_edge_custom_load()에서 create()가 실패해 *priv가 한 번도 채워지지 않은 채로 남는 경로뿐입니다(custom은 calloc되므로 초기값 NULL, 그리고 이 경로는 이번 라운드가 아니라 이전 라운드에서 이미 두 실패 경로를 분리하며 확립된 것). 정상적으로 create()가 성공한 살아있는 핸들이라면 custom->priv는 항상 non-NULL이므로, 이번 변경이 적용되는 if (!custom_h) return NNS_EDGE_ERROR_NONE; 분기에 진입하지 않습니다. 즉:

• 정상 핸들의 실제 close() 실패는 여전히 non-NULL priv로 호출되어 원래 로직(SAFE_FREE 두 번 + 성공 리턴)이 그대로 실행되고, 그 반환값이 그대로 nns_edge_custom_release() → nns_edge_release_handle()의 nns_edge_logw("Failed to close custom connection.") 경고로 전파되는 경로는 전혀 손대지 않았습니다. (참고로 테스트용 참조 구현 자체는 non-NULL 경로에서 항상 성공을 반환하므로 "실제 close 실패"를 스스로 재현하지는 않지만, 이는 이번 PR 이전부터의 상태이며 이번 diff의 범위 밖입니다.)
• 이 변경이 실제로 바꾸는 것은 "create()가 실패해서 애초에 만들어진 적 없는 priv==NULL"에 대한 처리뿐이며, 이는 헤더에 새로 문서화한 계약(“close()는 생성되지 않은 priv도 tolerate해야 한다”)이 요구하는 바와 정확히 일치합니다.

다른 connect type(HYBRID/MQTT 등)이나 nns_edge_custom_release()/nns_edge_release_handle() 자체의 코드는 이번 diff에서 전혀 건드리지 않았고, 실측으로도 기존 테스트(31개)가 모두 그대로 통과함을 확인해 회귀는 없다고 판단합니다.

범위 재확인

2차 리뷰 이후 diff는 tests/nnstreamer-edge-custom-test.c 단일 파일, 6줄 변경으로 국한되어 있으며 core 라이브러리(src/), 헤더, CI 워크플로는 이번 라운드에서 변경되지 않았습니다. PR 전체 diff(6개 파일)도 여전히 #252/#253이 요구하는 "리크 수정 + 회귀 테스트 + CI 게이팅 + 문서화" 범위 안에 있고, 다른 모듈로의 확산은 없습니다.

결론

Merge 가능(approve) 입니다. 2차 리뷰의 nit이 정확히 해소되었고, 요청하신 대로 close() 반환값이 nns_edge_custom_release() → nns_edge_release_handle()로 전파되는 경로를 직접 추적한 결과 정상 핸들의 실제 close 실패 감지 능력은 이번 변경으로 손상되지 않았습니다. 새로 발견한 이슈는 없습니다.

[myungjoo-bot]

Note: this review was produced by an automated review agent and is transcribed here by the bot account.

이 리뷰는 자동 코드 리뷰 에이전트(github-code-review 스킬)가 작성한 결과를 봇 계정이 옮겨 적은 것입니다.

d3e11898 기준 리뷰입니다.

리뷰 범위

src/libnnstreamer-edge/nnstreamer-edge-custom-impl.c (+6/-8), src/libnnstreamer-edge/nnstreamer-edge-internal.c (+2/-1), include/nnstreamer-edge-custom.h (+4), tests/nnstreamer-edge-custom-test.c (+8/-5), tests/unittest_nnstreamer-edge-custom.cc (+125), .github/workflows/ubuntu_clean_cmake_build.yml (+20/-1).

확인한 것

• 주제 해결: 릭 분석이 코드상 정확합니다. 기존 error: 라벨은 두 실패를 한데 모아 nns_edge_custom_release (custom)로 넘겼는데, 이 함수는 !custom->instance이면 NNS_EDGE_ERROR_INVALID_PARAMETER로 조기 반환해 아래쪽 free()에 닿지 못합니다. dlopen/dlsym 실패 시 instance는 항상 NULL이므로 매 실패마다 custom_connection_s가 샙니다. 두 경로를 분리해 SAFE_FREE (custom) / nns_edge_custom_release (custom)로 나눈 것이 최소 수정이면서 정확합니다.
• 이중 해제·dangling 없음: _load_custom_library()는 자신의 error: 경로에서 이미 dlclose()를 하므로, 그 뒤 SAFE_FREE (custom)만 하는 것이 맞습니다. nns_edge_custom_release() 본체는 손대지 않아 다른 호출자(nns_edge_release_handle(), releaseInvalidParam*_n)의 동작이 그대로입니다.
• out-param 계약 유지: *handle은 성공 시에만 기록되고, 실패 시 호출자 변수는 그대로 남습니다. #250에서 세운 nns_edge_custom_create_handle()의 *edge_h = NULL 계약과도 정합합니다.
• nns_edge_release_handle() 가드: eh->custom_connection_h가 NULL이면 release를 건너뛰도록 한 변경은, 커스텀 생성 실패 핸들을 정리할 때마다 뜨던 "Failed to close custom connection." 오탐 경고를 없앱니다. NULL일 때 기존 동작은 "INVALID_PARAMETER 반환 → 경고 로그"뿐이었으므로 기능 회귀가 없습니다. ENABLE_CUSTOM_CONNECTION=0 빌드에서도 custom_connection_h가 항상 NULL이라 동작이 동일합니다.
• CI가 회귀를 실제로 막습니다 — 이 PR의 가장 중요한 부분: 릭은 모든 gtest 단언을 통과하므로 일반 실행으로는 절대 잡히지 않습니다. ASan 스텝을 별도 job이 아니라 required check인 build job 안에 넣은 판단이 정확합니다(이 리포의 required check는 DCO, build 둘뿐이므로 별도 job은 빨개져도 머지를 막지 못합니다). 실행 로그로 확인했습니다: build-asan에서 4개 타깃이 모두 새로 빌드되고(Built target unittest_nnstreamer-edge-custom), ASAN_OPTIONS: detect_leaks=1로 100% tests passed, 0 tests failed out of 2. 즉 스텝이 형식적으로만 도는 것이 아니라 sanitizer 바이너리를 실제로 실행합니다.
• ENVIRONMENT 속성과의 상호작용 확인: #251이 커스텀 테스트에 건 ENVIRONMENT "LD_LIBRARY_PATH=..."는 해당 변수만 덮어쓰므로 ASAN_OPTIONS는 그대로 전달됩니다. build-asan 디렉터리 기준으로 경로가 다시 생성되는 것도 맞습니다.
• 테스트 설계: loadFail_n / loadCreateFail_n의 100회 반복은 sanitizer 리포트를 24바이트 1건이 아니라 100건으로 키워 눈에 띄게 하려는 의도이고, liveHandleAfterFailedLoads는 살아 있는 핸들이 dlopen 참조를 잡고 있어 50회 실패 로드가 그 핸들을 무너뜨리지 않음을 확인합니다. 실패 경로에서 dlclose()가 도는데도 안전하다는 것을 짚는 좋은 케이스입니다.
• 문서: nns_edge_custom_create() 실패 시에도 nns_edge_custom_close()가 호출된다는 기존 동작이 공개 헤더에 처음으로 명시되었습니다. 동작 변경 없이 계약만 적은 것이라 out-of-tree 플러그인에 영향이 없습니다. 구조체 레이아웃/시그니처 변경이 없어 ABI 영향도 없습니다.
• 범위: production 변경은 8줄이고 src/의 다른 모듈을 건드리지 않습니다. 테스트 라이브러리의 nns_edge_custom_close()가 NULL priv를 에러 대신 성공으로 처리하도록 바뀌었는데, 이는 새로 문서화한 계약("close는 만들어진 적 없는 priv를 견뎌야 한다")을 예제 구현이 따르도록 맞춘 것이라 일관됩니다.
• 보안: 백도어로 볼 만한 코드 없음. 릭 제거는 보안상 개선입니다. CI 워크플로의 트리거·권한·시크릿 사용에 변화가 없습니다.
• CI 전 항목 green (GBS 3종, pdebuild 3종, static checks 포함).

참고 제안 (blocking 아님)

1. getenv() 검사가 !priv 검증보다 앞에 있습니다. nns_edge_custom_create()에서 스위치가 켜져 있으면 priv == NULL이어도 INVALID_PARAMETER 대신 UNKNOWN을 반환합니다. 지금 테스트 중에 이를 밟는 것은 없지만, 이 파일은 공개 헤더가 "Refer to the example in nnstreamer-edge-custom-test.c"로 가리키는 참조 구현이라 파라미터 검증 순서는 그대로 두는 편이 낫습니다. if (!priv) 뒤로 한 칸 내리면 해결됩니다.
2. 환경변수 스위치는 프로세스 전역이고 fatal 단언에 취약합니다. 지금은 setenv()와 unsetenv() 사이에 ASSERT_*가 없어 안전하지만(확인했습니다), 나중에 누가 그 구간에 ASSERT_를 하나 넣으면 unsetenv()가 실행되지 않고 이후 모든 테스트가 실패 주입 상태로 돌게 됩니다. RAII 가드(소멸자에서 unsetenv)나 TearDown()을 쓰는 fixture로 감싸면 구조적으로 막힙니다 — 참고로 #254가 같은 목적을 fixture + 제어 블록으로 풀고 있습니다.
3. #254와 겹칩니다. 이 PR은 #252와 #253을 함께 닫고, #254는 #253만 닫습니다. 두 브랜치 모두 tests/nnstreamer-edge-custom-test.c의 create()/close()와 tests/unittest_nnstreamer-edge-custom.cc의 같은 영역을 고쳐서 실제로 텍스트 충돌합니다(로컬에서 두 head를 merge해 확인: tests/nnstreamer-edge-custom-test.c, tests/unittest_nnstreamer-edge-custom.cc 두 파일 CONFLICT). 릭 수정이 들어 있는 이쪽을 먼저 머지하고, #254는 이 PR이 커버하지 못하는 부분(close_count/close_had_priv로 close 호출을 관측하는 것, 환경변수 대신 fixture로 주입하는 것)만 남겨 리베이스하는 순서를 권합니다. 판단은 메인테이너 몫입니다.

릭 수정이 정확하고, 일반 테스트로는 잡히지 않는 회귀를 required check 안에서 실제로 막도록 CI가 구성된 것을 로그로 확인했으므로 approve합니다.