I have completed my review. Here is my assessment.

Adversarial Review: Ridge GCV → cuSOLVER SPD solve (Phase 1)

VERDICT: fix-then-ship

The actual Phase‑1 deliverable — spd_solve, the cuSOLVER handle, the gating helper, the 3 call‑site swaps, the CMake link/compile‑def edits, and the equivalence test — is correct and faithful to the approved design. I could not find a correctness, ABI, or fold‑isolation bug in any of it.

There is one real problem: an unrelated, out‑of‑scope rewrite of the hot gemm() dispatch path is bundled into the same diff. It isn’t in the approved design, it touches a far broader blast radius than the isolated solve, and it’s not covered by the equivalence test. Split it out (or justify+test it separately) before merging; the Ridge work itself is shippable.

Findings

1. [SCOPE] gemm() was rewritten to use static thread_local ReusableDeviceBuffer scratch — not part of this design. cpp/src/core/cuda_dispatch.cpp:1559-1572 (and the dA/dB/dC deref changes at :1597-1603). The approved design (§3) scopes Phase 1 to spd_solve + the cuSOLVER handle + CMake + the 3 dual call‑site swaps + the test. This gemm change is a separate performance optimization (replacing per‑call DevicePtr with grow‑only per‑thread scratch to avoid cudaMalloc/cudaFree device‑sync stalls). It is plausibly correct — dA/dB are fully overwritten by copy_h2d each call, dC is only read when beta != 0 (same contract as the original uninitialized DevicePtr), grow‑only ensure() is sound, and thread_local gives per‑thread isolation — but:

  • It modifies the primitive used by every CUDA GEMM in libn4m, including the K = Xc·Xcᵀ build for the dual design and all PLS moment builds — much wider than the isolated spd_solve.

  • It is not exercised by the equivalence test (which only calls spd_solve directly), and the design’s optional moment‑sweep parity sanity (§5) wasn’t added.

  • §4.2 of the design explicitly cautioned against introducing reused/persistent device buffers in Phase 1.

  • Fix: extract this into its own PR with its own benchmark + a run_moment_sweep parity check (N4M_CUDA_RIDGE_DISABLE=1 vs not, scores agree ≤1e‑8). If it must ship together, call it out explicitly and add the parity gate.

2. [STYLE] static thread_local lifetime footnote (only if finding #1 stays). cuda_dispatch.cpp:1564-1566. The scratch destructors run cudaFree at thread/process exit, where the CUDA runtime may already be torn down (cudaErrorCudartUnloading). The return is ignored, so it’s benign, and it mirrors the existing CublasState dtor pattern — but it’s a new lifetime that the per‑call DevicePtr didn’t have, and the buffers stay resident at high‑water mark for the process lifetime. Acceptable as a documented tradeoff; noting for completeness.

3. [OK] spd_solve correctness (cuda_dispatch.cpp:1156-1260). Verified against §3.1 and the §4 traps:

  • A uploaded as‑is, no transpose — correct (A symmetric ⇒ row‑major image == column‑major image; uplo immaterial, documented at :1207).

  • B packed column‑major Bcm[target*n+i]=B[i*q+target] (:1200-1204) and X unpacked inverse X[i*q+target]=Xcm[target*n+i] (:1242-1245) — both correct.

  • devInfo checked after both potrf (:1219-1228) and potrs (:1234-1238).

  • Return codes exactly per spec: potrf info>0 → 1 (not‑PD), info<0 → 2, potrs info!=0 → 2, and check_cusolver throws → caught by the bad_alloc/exception/... envelope → 2 (:1248-1259).

  • Workspace sized via cusolverDnDpotrf_bufferSize (:1210-1213), no assumed size.

  • Overflow/validity guards are thorough: null/zero, n,q ≤ INT_MAX, mul_overflows(n,n), mul_overflows(n,q), and byte‑size overflow (:1170-1189).

  • No leaks (RAII DevicePtr for dA/dB/dInfo/dWork, std::vector staging). Default‑stream ordering is correct — copy_d2h of info is a synchronous cudaMemcpy on the default stream, so it waits for potrf/potrs (no explicit sync needed).

4. [OK] Scope of the swaps. Exactly the 3 dual sites route to solve_dual_spd: predict_ridge_from_dual_design (sweep.cpp:529), ridge_dual_cross_heldout_sse (:577), fit_ridge_from_dual_design (:623). The k×k PᵀW inverse (sweep.cpp:721), the primal moment solve (:913), and the PLS‑prefix inverse (:1403) all remain on host solve_square_qr. ridge.cpp is untouched (confirmed via diff‑stat). No Phase‑2 eigh code was added.

5. [OK] Fallback + gating (sweep.cpp:435-469). solve_dual_spd falls through to host QR on s==1 and s==2 (the if (s==0) return; leaves both other codes falling out of the #if block to solve_square_qr). ridge_cuda_dual_enabled: n<256 → false, N4M_CUDA_RIDGE_DISABLE honored, cuda_runtime_available() checked, #else → false. The λ=0 generic case is handled correctly: potrf reports not‑PD → 1 → host QR, which itself rejects the singular R with N4M_ERR_NUMERICAL_FAILURE — identical to the pre‑change behavior. The [[maybe_unused]] on ridge_cuda_dual_enabled correctly prevents an unused‑function -Werror failure in the non‑CUDA build.

6. [OK] Fold‑clean. spd_solve uses only per‑call DevicePtrs and the passed‑in A/B; no persistent or cross‑fold device buffer. K/Y_work are fold‑local. No fold‑to‑fold state in the solve path. (Caveat: finding #1’s gemm scratch is reused, but it’s fully overwritten each call so it carries no numerical state — not a leakage vector, but it’s the kind of reuse §4.2 flagged.)

7. [OK] ABI. spd_solve is declared only in the internal cuda_dispatch.hpp, has no N4M_API, and the project compiles with -fvisibility=hidden (CMakeLists.txt:77-79, n4m_targets.cmake:111-112) plus the n4m_linux.map version script — so it stays out of the dynamic symbol table, exactly like the existing gemm/pls1_* internals. No cpp/abi/, cpp/include/, bindings/, or catalog/ changes (confirmed via diff‑stat). cuSOLVER handle lifecycle is correct: created after cuBLAS, available gated on both succeeding, cuBLAS rolled back (cublasDestroy_v2 + handle=nullptr) if cusolverDnCreate fails, and destroyed before cuBLAS in the dtor (cuda_dispatch.cpp:47-76).

8. [OK] The test (test_internal_linalg.cpp:304-371). It’s a real test: builds a deterministic SPD A = (GᵀG)/n + I (eigenvalues ≥ 1, well‑conditioned), solves via spd_solve and a host QR reference (qr_square_multi, byte‑for‑byte the same algorithm as the production solve_square_qr), and asserts max abs diff ≤ 1e-8. It self‑skips with a printed message when no GPU is present. The not‑PD path is covered with a diag(1,…,1,0) matrix (a clean zero pivot → potrf info=n>0), asserting spd_solve(...) == 1. The #if defined(N4M_USE_CUDA) guards (helper, test, and invocation) plus the cpp/tests/CMakeLists.txt:94-95 generator‑expression compile‑def correctly make the TU see the macro that n4m_c_static keeps PRIVATE. Note: it tests n=64 < 256 but calls spd_solve directly, bypassing the n<256 gate — correct, so the device path is actually exercised.

9. [STYLE] N4M_CUDA_RIDGE_DISABLE semantics (sweep.cpp:439-442). Any non‑empty value other than a leading '0' disables — so N4M_CUDA_RIDGE_DISABLE=false would also disable CUDA, diverging from the truthy_env idiom used elsewhere (cuda_dispatch.cpp:386). This matches the design’s §3.4 spec verbatim and is defensible for a kill‑switch, so it’s not a bug — just a minor inconsistency worth a one‑line comment if you care about env‑var uniformity.

Bottom line: the Ridge spd_solve change is clean, correct, ABI‑safe, fold‑isolated, and well‑tested. Pull the gemm() thread_local‑scratch optimization out of this diff (finding #1) and the Phase‑1 work ships as designed. The orchestrator’s GPU run should confirm the ≤1e-8 equivalence and the not‑PD ==1 assertions, which my static read indicates will pass.