Skip to content

fix(test,sycl): HIP motion debug bool "1"→"true" + SYCL graph dangling-priv SIGSEGV - #768

Merged
lusoris merged 1 commit into
masterfrom
fix/hip-sycl-motion-test-failures
Jun 6, 2026
Merged

fix(test,sycl): HIP motion debug bool "1"→"true" + SYCL graph dangling-priv SIGSEGV#768
lusoris merged 1 commit into
masterfrom
fix/hip-sycl-motion-test-failures

Conversation

@lusoris

@lusoris lusoris commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • HIP motion test (test_motion_cpu_hip_parity): vmaf_use_feature(motion) failed because the test passed "1" for a VMAF_OPT_TYPE_BOOL option. set_option_bool() in opt.c accepts only "true" / "false"; "1" returns -EINVAL which propagates to failure. Fixed by changing "1" to "true" in test_hip_motion_parity.c.

  • SYCL motion_add_uv test (test_sycl_motion_add_uv_parity): SIGSEGV in Pass 2. The test shares one VmafSyclState across two sequential VmafContext instances. When Pass 1 closes, close_fex_sycl frees MotionStateSycl priv but leaves the entry in sycl_state->graph_extractors[]. Pass 2 adds a second entry. When the combined graph is re-recorded for Pass 2, record_combined_graphs() calls config_fn(ge.priv, slot) for all entries — including slot 0 whose priv is dangling → SIGSEGV. Fixed by adding vmaf_sycl_graph_unregister(state, priv) (new API) that removes the entry by priv pointer, compacts the array, and invalidates recorded graphs. Called from close_fex_sycl before device memory is freed.

Test plan

  • meson test -C build test_hip_motion_parity — passes (no HIP device → skip, which is a pass)
  • meson test -C build test_sycl_motion_add_uv_parity — passes (no SYCL device → skip, which is a pass)
  • Full CI GPU suite on a machine with HIP / SYCL devices
  • No regression on other SYCL extractors (graph_unregister is not called by PSNR/VIF/ADM yet — they are unaffected as this is a new opt-in API)

Reproducer

# HIP failure — was:
# CPU: vmaf_use_feature(motion) failed
# (vmaf_feature_dictionary_set called with "debug"="1")

# SYCL failure — was:
# SIGSEGV in record_combined_graphs via config_fn on freed priv
# after first vmaf_close() with shared sycl_state

no rebase impact: test file + SYCL internal lifecycle — no public API added to libvmaf.h

🤖 Generated with Claude Code

@lusoris @claude
fix(test,sycl): HIP motion debug bool value + SYCL graph dangling-pri…
59965f2 
…v SIGSEGV

Two failures on the motion parity test suite:

1. test_hip_motion_parity / test_motion_cpu_hip_parity:
   vmaf_feature_dictionary_set(&opts, "debug", "1") passed the integer
   string "1" for a VMAF_OPT_TYPE_BOOL option.  set_option_bool() in
   opt.c accepts only "true" / "false"; "1" returns -EINVAL, which
   propagates through vmaf_fex_ctx_parse_options ->
   vmaf_feature_extractor_context_create -> vmaf_use_feature, making
   the whole call fail.  Fix: change "1" to "true".

2. test_sycl_motion_add_uv_parity: SIGSEGV on Pass 2.
   The test creates one VmafSyclState and shares it across two
   sequential VmafContext instances.  When Pass 1 closes, close_fex_sycl
   frees the MotionStateSycl priv but never removes the entry from
   sycl_state->graph_extractors[].  Pass 2 then calls
   vmaf_sycl_graph_register, adding a second entry.  When the combined
   graph is re-recorded for Pass 2, record_combined_graphs() calls
   config_fn(ge.priv, slot) for all entries including slot 0 whose
   priv now points to freed memory -> SIGSEGV.

   Fix: add vmaf_sycl_graph_unregister(state, priv) which removes the
   matching entry by priv pointer, compacts the array, and invalidates
   the recorded graphs so they are re-built with the reduced extractor
   set.  Call it from close_fex_sycl before device memory is freed.

No user-visible delta (test-only + internal SYCL lifecycle fix).
No docs required (ADR-0100 exclusion: internal fix, no public surface).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris
lusoris merged commit dd12223 into master Jun 6, 2026
44 of 60 checks passed
@lusoris
lusoris deleted the fix/hip-sycl-motion-test-failures branch June 6, 2026 16:16
@lusoris lusoris mentioned this pull request Jun 6, 2026
Closed
8 of 11 tasks
lusoris added a commit that referenced this pull request Jun 6, 2026
@lusoris @claude
fix(sycl): drain combined_queue in graph_unregister + propagate unreg…
36c3b6e 
…ister to ADM/VIF/MOMENT/PSNR

PR #768 added vmaf_sycl_graph_unregister to motion_sycl's close_fex_sycl
to remove the dangling priv from the graph registry after the first
VmafContext closes.  Despite that fix test_sycl_motion_add_uv_parity
still SIGSEGV-ed on Linux GCC.

Root cause (second path):
vmaf_sycl_graph_unregister deleted the recorded exec_graph_t objects
without first draining combined_queue.  The Level Zero command-list
destructor is undefined behaviour when the queue still holds a reference
to that command-list (even if collect() → graph_wait() already flushed it
at a higher level, close_fex_sycl only waited the primary queue via
vmaf_sycl_queue_wait).  The runtime-level race caused the SIGSEGV.

Fixes:
1. vmaf_sycl_graph_unregister (common.cpp): add combined_queue
   wait_and_throw() before deleting exec_graph_t objects.  This ensures
   the Level Zero command-list is no longer referenced by any queue
   before its destructor runs, regardless of the caller's prior wait
   history.
2. Zero out the vacated graph_extractors slot after compaction so stale
   function pointers/priv are not silently accessible.
3. Propagate the graph_unregister call to the four other extractors that
   were missing it: integer_adm_sycl, integer_vif_sycl,
   integer_moment_sycl, integer_psnr_sycl.  These all call
   vmaf_sycl_graph_register in init but had no matching unregister in
   close, leaving dangling priv pointers for any subsequent VmafContext
   sharing the same sycl_state (same root bug as motion, ADR-0989).

No user-visible delta; no new public API surface.
ADR-0100 exclusion: internal SYCL lifecycle fix, no user-discoverable
surface change.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 6, 2026
@lusoris @claude
chore(test): disable 2 recurring-failure tests via should_fail (ADR-1…
c9c7ddc 
…093) (#807)

test_pic_preallocation and test_sycl_motion_add_uv_parity have each had
three or more incomplete fix attempts (PRs #765, #769, #797 and #768,
#796 respectively) and continue to fail in CI. Both are in the fast suite
and block every unrelated PR's CI gate.

Use Meson's should_fail: true to invert the expected result: the test
binary is still compiled and run on every invocation, so build regressions
and the UNEXPECTEDPASS transition remain visible, but CI stays green while
the root cause is under investigation.

Test source files are preserved unchanged.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 7, 2026
@lusoris @claude
fix(test): remove should_fail from test_sycl_motion_add_uv_parity (#823)
2e10e04 
Lifts the ADR-1093 suppression so CI red surfaces the underlying
SIGSEGV on the sycl_motion_add_uv path directly, rather than
inverting the signal via should_fail=true.

Both prior fix attempts (PRs #768, #796) failed to resolve the root
cause; the test now runs unconditionally and fails loudly to trigger
proper investigation.

test_pic_preallocation already had its should_fail removed (PR #808,
reapplied daf97da); no change needed there.

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris lusoris mentioned this pull request Jun 7, 2026
Merged
6 of 9 tasks
lusoris added a commit that referenced this pull request Jun 7, 2026
@lusoris @claude
fix(sycl): propagate -fsycl via sycl_dependency to fix test SIGSEGV (…
4f004fd 
…ADR-1099)

Root-cause investigation of test_sycl_motion_add_uv_parity SIGSEGV after
two prior incomplete fix attempts (PRs #768, #796, ADR-1093).

Two root causes identified and fixed:

1. Missing -fsycl at link time for SYCL test executables.
   Without -fsycl, icpx skips clang-offload-wrapper and the SYCL
   runtime's ProgramManager never registers the device kernels from
   libvmaf.a.  At runtime, the first queue.submit() null-dereferences
   inside ProgramManager::getDeviceKernelInfo (SIGSEGV).
   Fix: move -fsycl from vmaf_link_args (library-only) into
   sycl_dependency.link_args in core/src/meson.build so every target
   that declares dependencies: [sycl_dependency] — libvmaf.so and all
   SYCL test executables — gets the flag automatically.

2. Wrong feature names in vmaf_feature_score_at_index queries.
   With motion_add_uv=true (non-default), the feature-name system stores
   scores under aliased names: integer_motion2_mau (SYCL) and
   float_motion2_mau (CPU).  The test queried the raw VMAF_*_score names,
   getting -EINVAL for every lookup.  Fixed by using the aliased names.

Also included: remove extern "C" wrappers around C++ standard library
includes in all 23 SYCL TUs (GCC 15 + icpx incompatibility), and add
picture_copy.h so they are safe to include from C++ without extern "C"
wrappers.

Removes should_fail: true from test_sycl_motion_add_uv_parity (ADR-1093
bypass reverted for this test).  Adds AGENTS.md invariant notes for the
-fsycl link requirement and the feature-name aliasing contract.

ADR-1099, T-SYCL-MOTION-ADD-UV-SIGSEGV-2026-06-07

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 7, 2026
@lusoris @claude
fix(sycl): propagate -fsycl via sycl_dependency to fix test SIGSEGV (…
4f5209f 
…ADR-1099)

Root-cause investigation of test_sycl_motion_add_uv_parity SIGSEGV after
two prior incomplete fix attempts (PRs #768, #796, ADR-1093).

Two root causes identified and fixed:

1. Missing -fsycl at link time for SYCL test executables.
   Without -fsycl, icpx skips clang-offload-wrapper and the SYCL
   runtime's ProgramManager never registers the device kernels from
   libvmaf.a.  At runtime, the first queue.submit() null-dereferences
   inside ProgramManager::getDeviceKernelInfo (SIGSEGV).
   Fix: move -fsycl from vmaf_link_args (library-only) into
   sycl_dependency.link_args in core/src/meson.build so every target
   that declares dependencies: [sycl_dependency] — libvmaf.so and all
   SYCL test executables — gets the flag automatically.

2. Wrong feature names in vmaf_feature_score_at_index queries.
   With motion_add_uv=true (non-default), the feature-name system stores
   scores under aliased names: integer_motion2_mau (SYCL) and
   float_motion2_mau (CPU).  The test queried the raw VMAF_*_score names,
   getting -EINVAL for every lookup.  Fixed by using the aliased names.

Also included: remove extern "C" wrappers around C++ standard library
includes in all 23 SYCL TUs (GCC 15 + icpx incompatibility), and add
picture_copy.h so they are safe to include from C++ without extern "C"
wrappers.

Removes should_fail: true from test_sycl_motion_add_uv_parity (ADR-1093
bypass reverted for this test).  Adds AGENTS.md invariant notes for the
-fsycl link requirement and the feature-name aliasing contract.

ADR-1099, T-SYCL-MOTION-ADD-UV-SIGSEGV-2026-06-07

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 7, 2026
@lusoris @claude
docs(state): backfill state.md rows for PRs #765–#771
4eec292 
Add three missing Recently-closed rows that were absent from state.md:

- T-GPU-POOL-UAF-OOM-ASAN-UBSAN-GAP-2026-06-06: PRs #767 + #770 added
  three huge-alloc tests to ASan/UBSan/TSan exclusion lists in both
  sanitizers.yml and tests-and-quality-gates.yml; CI SIGABRT spurious
  failures resolved.

- T-HIP-MOTION-DEBUG-BOOL-SYCL-GRAPH-DANGLING-2026-06-06: PR #768 fixed
  HIP motion test passing "1" for a VMAF_OPT_TYPE_BOOL option (should be
  "true") and a SYCL graph dangling-priv SIGSEGV when a VmafSyclState is
  shared across two sequential VmafContext instances.

- T-MOTION-FIVE-FRAME-WINDOW-PYTHON-SKIP-2026-06-06: PR #771 added
  @unittest.skip decorators to 9 Python test methods that set
  motion_five_frame_window=True, which returns -ENOTSUP from C per
  ADR-0337 pending prev_prev_ref plumbing.

PRs #765 (T-PREV-REF-BATCH-REFCOUNT-LEAK), #766 (T-MCP-SCORE-POOLED-EAGAIN),
and #769 (T-PIC-PREALLOC-ASAN-LEAK) were already tracked. PR #770 adds no
new bug row (CI wiring fix only, no new defect opened/closed).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
lusoris added a commit that referenced this pull request Jun 8, 2026
@lusoris @claude
chore(bundle): drain 8 rebased DRAFT PRs (docs + helm + tests-coverag…
a42e4ea 
…e) (#845)

* docs(internal-headers): add Doxygen @brief/@PARAM to 10 core src/*.h (ADR-1096)

214 internal headers had no Doxygen coverage; only 26 had any @brief/@PARAM
annotation. Adds @brief, @PARAM[in/out], and @return comments to the ten
highest-traffic headers:

  framesync.h thread_pool.h picture_pool.h predict.h fex_ctx_vector.h
  ref.h mem.h log.h opt.h dict.h

Purely additive — no logic, no ABI, no public-header changes.
IDE hover-docs and doxygen -q now populate for all covered APIs.

ADR-1096 documents the coverage decision and follow-up scope.
AGENTS.md invariant added: update Doxygen blocks when signatures change.

no digest needed: trivial doc-only addition
no alternatives: only-one-way fix (add the comments)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs(api): add @thread-safety/@param/@return to all undocumented public C-API functions

Every VMAF_EXPORT function in core/include/libvmaf/ now carries a complete
Doxygen contract: @PARAM, @return, and @Thread-safety. Previously libvmaf_cuda.h
(5 functions), libvmaf_sycl.h (20), dnn.h (9), picture_v2.h (5), and
model.h (vmaf_model_version_next) were missing @Thread-safety entirely;
picture_v2.h stubs were also missing @param/@return. VmafPoolingMethod in
libvmaf.h lacked a @brief enum-level description.

The thread-safety contract is uniform: GPU backend setup functions (cuda/sycl/
hip/metal state init/import/free, preallocate, fetch) are not thread-safe —
one handle per driver thread. Pure query functions (vmaf_dnn_available,
vmaf_sycl_list_devices, vmaf_dnn_verify_signature, vmaf_backend_handle_name,
vmaf_model_version_next) are marked safe from any thread.

No C source files, build files, or ABI-visible signatures changed — Doxygen
comment additions only.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs(state): backfill state.md rows for PRs #765–#771

Add three missing Recently-closed rows that were absent from state.md:

- T-GPU-POOL-UAF-OOM-ASAN-UBSAN-GAP-2026-06-06: PRs #767 + #770 added
  three huge-alloc tests to ASan/UBSan/TSan exclusion lists in both
  sanitizers.yml and tests-and-quality-gates.yml; CI SIGABRT spurious
  failures resolved.

- T-HIP-MOTION-DEBUG-BOOL-SYCL-GRAPH-DANGLING-2026-06-06: PR #768 fixed
  HIP motion test passing "1" for a VMAF_OPT_TYPE_BOOL option (should be
  "true") and a SYCL graph dangling-priv SIGSEGV when a VmafSyclState is
  shared across two sequential VmafContext instances.

- T-MOTION-FIVE-FRAME-WINDOW-PYTHON-SKIP-2026-06-06: PR #771 added
  @unittest.skip decorators to 9 Python test methods that set
  motion_five_frame_window=True, which returns -ENOTSUP from C per
  ADR-0337 pending prev_prev_ref plumbing.

PRs #765 (T-PREV-REF-BATCH-REFCOUNT-LEAK), #766 (T-MCP-SCORE-POOLED-EAGAIN),
and #769 (T-PIC-PREALLOC-ASAN-LEAK) were already tracked. PR #770 adds no
new bug row (CI wiring fix only, no new defect opened/closed).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* chore(state): backfill 4 missing state.md rows from PRs #712–#747 batch

State.md updates promised by squash commits for PRs #723, #725, #729,
and #743 were lost (the pre-squash branch commits carried the changes
but the squash commits did not include state.md in their diffs).

Changes:
- Move T-JSON-MODEL-SLOPES-FEATURE-CAP-OOB-2026-05-30 from Open to
  Recently closed — fixed by PR #743 / ADR-0887 (vmaf_model_destroy
  heap-buffer-overflow via fuzz_json_model nightly harness).
- Add T-FFMPEG-PATCHES-SCORE-FMT-GAP-2026-06-06 to Recently closed —
  PR #723 / ADR-1064 wired score_fmt AVOption on all four FFmpeg vmaf
  filters; PR #740 fixed patch hunk counts.
- Add T-VENDORED-CJSON-PDJSON-SECURITY-2026-06-06 to Recently closed —
  PR #725 / ADR-1061 fixed five pdjson/cJSON security and correctness
  bugs (depth guard never compiled, size overflow x2, banned sprintf/
  strcpy at 12 sites, cJSON_GetArraySize int wrap).
- Add T-GO-STATICCHECK-R10-TIMER-BODY-2026-06-06 to Recently closed —
  PR #729 / ADR-1065 fixed Go timer leak (time.After in poll loop),
  missing body size cap on vmafx-controller, and missing ReadTimeout
  on both HTTP servers.
- Add second-pass _Updated: header summarising the PRs #712–#747 batch.

no rebase impact: state.md only, no code or API surface changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs(nav): add 10 orphaned pages to mkdocs.yml nav

Audit found the following docs/**.md files present on disk but absent from
both the mkdocs.yml nav and any inbound cross-reference:

- docs/ai/sidecar-online-training.md (k8s Phase 4b sidecar trainer)
- docs/server/auth.md (JWT auth gateway for vmafx-controller)
- docs/server/operator.md (kubebuilder Kubernetes operator)
- docs/server/rest.md (vmafx-server REST/OpenAPI surface)
- docs/development/ebpf-fuse-bypass.md (rclone FUSE eBPF bypass)
- docs/development/perf-claims-2026-05-10.md (May 2026 perf claims log)
- docs/sync-upstream/2026-05-02-sync-report.md (upstream sync report)
- docs/sync-upstream/2026-05-03-sync-report.md (upstream sync report)
- docs/upstream-ports/1b08bb4d-needs-manual-port.md (manual port note)

Also excludes docs/changelog.d/** from the mkdocs build — one changelog
fragment (cpp23-wave3.md) was placed under docs/changelog.d/ instead of
the repo-root changelog.d/; the fragment is referenced by the ADR index
but is not a standalone page and should not be rendered by mkdocs.

No code changes. No ADR required (nav-only housekeeping).

no rebase impact: docs/mkdocs.yml nav entries only
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(helm): values completeness — nameOverride, statePVCSize, node.metricsPort, extraPorts schema (ADR-1074)

Four gaps where values.yaml / values.schema.json diverged from template usage:

1. nameOverride/fullnameOverride: read by _helpers.tpl but absent from both
   values.yaml and the root additionalProperties:false schema; any user
   supplying --set nameOverride=foo received an immediate helm-lint failure.
   Added as string keys to both files.

2. statefulSet.statePVCSize: statefulset.yaml hardcoded `storage: 1Gi` for the
   per-replica MCP-state PVC. Exposed as statefulSet.statePVCSize (default 1Gi)
   and wired into the volumeClaimTemplates storage field.

3. node.metricsPort: port 9090 appeared hardcoded in three template locations
   (node Deployment containerPort, node-metrics Service port, NetworkPolicy
   allow rule). Exposed as node.metricsPort (default 9090) and unified.

4. service.extraPorts items schema: bare `"type": "array"` with no items
   definition accepted malformed port objects silently. Added items schema with
   required [name, port] and protocol enum.

All defaults preserve existing rendered output byte-for-byte.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(ai/scripts): add unit tests for calibrate_phase_f_recipes and analyze_knob_sweep helpers (round 3)

The existing test_calibrate_phase_f_recipes.py covers only the main()
invocation (run-provenance smoke).  The existing test_knob_sweep_analysis.py
covers pareto_frontier, stratify, and detect_recipe_regressions via a
20-row synthetic fixture.

This commit adds coverage for the remaining pure helper functions:

* test_calibrate_phase_f_recipes_unit.py (50 tests):
  - mos_to_vmaf_proxy: clamping, boundary MOS values, string coercion
  - saliency_benefit_to_intensity: threshold boundaries for all three labels
  - _iter_corpus_rows: valid rows, blank-line skip, malformed-JSON skip,
    missing-field skip, optional duration_s default
  - _ugc_target_vmaf_offset: empty/small corpus, symmetric distribution,
    heavy-tail sign, clamp bounds, rounding
  - _ugc_tight_interval_width: empty corpus fallback, uniform/wide
    distributions, floor/cap enforcement, rounding
  - _resolution_dominance: empty, single-resolution, split, dominant bucket,
    portrait-vs-landscape distinction
  - _ugc_saliency_benefit_fraction: fallback, no-qualify, all-qualify,
    half-qualify, high-MOS exclusion, square-aspect treatment
  - calibrate() integration: all four recipe classes, UGC/proxy provenance
    tags, required recipe keys, saliency label validity

* test_analyze_knob_sweep_unit.py (29 tests):
  - _stable_knob_repr: empty dict, single entry, alphabetical sort, non-Mapping
    input, numeric values, insertion-order invariance
  - _slug: alphanumeric passthrough, hyphen/underscore preserved, space/slash
    replacement, empty-string fallback, all-special-chars
  - _closest_bare_at_bitrate: no-bare-rows, within tolerance, outside
    tolerance, picks closest, exact match, zero-tolerance
  - write_slice_csv: filename pattern, header row, data rows, slug-safe names,
    auto-creates output directory
  - write_summary_md: file creation, slice count, no-regression message,
    regression table, auto-creates output directory

All 79 tests run without GPU, corpus, or model downloads (<100 ms total).

no rebase impact: test-only addition; no existing golden assertion modified.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test(vmafx-server): replace //nolint:errcheck with explicit discards per ADR-0278

Replace all //nolint:errcheck suppression comments in grpc_server_handler_test.go
with explicit-discard patterns (_ = x.Close() / defer func() { _ = x.Close() }()).
Also remove duplicate TestGRPCScore_ScorerError function (the earlier copy was
accidentally left in; keep the one with the fuller doc comment at the bottom of
the file). Remove duplicate TestRunHTTP_BadAddress from main_extra_test.go.
Fix unchecked ln.Close() and conn.Close() in TestRunHTTPGracefulShutdown.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Lusoris <lusoris@pm.me>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@lusoris lusoris added this to the 1.0.0 — First release milestone Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0 — First release

Development

Successfully merging this pull request may close these issues.

1 participant

@lusoris