Fix gpu metrics

[giurgiur99]

Fixes # .

Changes proposed in this PR:

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[giurgiur99]

/run-security-scan

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
The PR elegantly fixes the Koffi process-global struct registration collisions by using a module-level Promise cache. The transition from caching a boolean result to caching the initialization Promise effectively prevents race conditions from concurrent detect() calls across single or multiple instances.

Comments:
• [INFO][style] Excellent use of a module-level Promise (sharedBindings) to guarantee that Koffi structs are built exactly once per Node process. Caching the Promise itself avoids complex locking mechanisms and natively handles async concurrency.
• [INFO][other] Just a minor robustness observation: if this.loadKoffi() or this.buildBindings(koffi) were to throw an unhandled exception, sharedBindings would reject. Because await sharedBindings is not wrapped in a try/catch, probe() (and consequently detect()) will reject rather than returning false gracefully.

This behavior carries over from the previous implementation, so it doesn't introduce a regression, but consider if wrapping this in a try/catch might be safer to ensure detect() always resolves cleanly:

-    const bindings = await sharedBindings
-    if (!bindings) return false
+    let bindings: NvmlBindings | null = null
+    try {
+      bindings = await sharedBindings
+    } catch (e: any) {
+      CORE_LOGGER.warn(`GPU metrics (nvidia): Bindings init failed — disabled (${e?.message})`)
+      return false
+    }
+    if (!bindings) return false

Otherwise, this looks exceptionally solid. LGTM!

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This PR effectively addresses the issue with duplicate Koffi struct initializations by moving the FFI bindings build to a module-scoped, lazily-evaluated Promise (sharedBindings). The introduction of the this.detecting Promise prevents race conditions in concurrent detect() calls. The test suite updates appropriately mirror the stateful changes. Code is clean and correctly implements the fix. LGTM!

Comments:
• [INFO][style] Excellent approach using a module-level variable to cache the Koffi bindings globally while allowing individual instances of NvmlGpuCollector to maintain their own nvmlInit() / nvmlShutdown() lifecycle. This elegantly avoids Koffi's process-global structure collisions without permanently tying up the NVML context.
• [INFO][style] Good use of the promise memoization pattern here. By caching this.probe() as this.detecting, you gracefully protect against concurrent calls to detect() that might occur before the initialization phase completes.

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
The PR effectively addresses the Koffi struct re-declaration error by introducing a process-global cache (sharedBindings) for the FFI bindings. It also fixes potential race conditions during concurrent initialization by caching the probe promise at the instance level rather than relying on a boolean state flag. The implementation is clean, handles initialization failures gracefully, and ensures that teardown/re-initialization works safely. LGTM!

Comments:
• [INFO][style] Good use of the promise caching pattern here (this.detecting = this.probe()). This effectively resolves race conditions from concurrent detect() calls, yielding a predictable initialization flow.
• [INFO][architecture] The module-level variable sharedBindings is populated using instance methods (this.loadKoffi() and this.buildBindings()). While this works perfectly to prevent the Koffi struct duplication error, it means the global cache is permanently determined by the first instance that initializes it. If these methods do not rely on instance-specific state, consider making them static in the future to clarify that they are independent of the specific NvmlGpuCollector instance.
• [INFO][style] Assigning directly to collector.detecting bypasses the private access modifier in TypeScript. While this is common in the existing test code (as seen with initialized and bindings), it's generally a better practice to mock the public API (e.g., jest.spyOn(collector, 'detect').mockResolvedValue(true)) rather than mutating private internal state. This is just a minor style note for future test improvements.

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
The PR successfully addresses the issue with duplicate Koffi struct names by caching the bindings process-globally using the sharedBindings promise. The use of promise memoization (this.detecting) to handle concurrent detection callers cleanly is an excellent design choice. However, there is a minor race condition during the async resolution where stopping the collector while detection is in-flight could unintentionally leave NVML initialized in the background.

Comments:
• [WARNING][bug] There is a potential race condition between detect() and stop(). If stop() is called while probe() is awaiting sharedBindings, this.detecting will be set to null to signal a shutdown. However, once sharedBindings resolves, probe() will blindly continue, call bindings.init(), and set this.initialized = true despite the shutdown intent. This could lead to a resource leak where NVML remains active in the background.

To fix this, check if the detection process was aborted before proceeding:

     const bindings = await sharedBindings
     if (!bindings) return false
+
+    // Abort if stop() was called while waiting for sharedBindings
+    if (!this.detecting) return false
+
     try {
       const rc = bindings.init()

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
The PR effectively addresses the Koffi process-global struct redefinition error by moving the FFI bindings resolution to a module-scoped Promise (sharedBindings). It also elegantly handles concurrent initializations at the instance level by caching the probe() Promise in this.detecting. The state resets correctly during shutdown. Excellent work.

Comments:
• [INFO][other] Good architectural decision to cache the NvmlBindings in a module-level Promise. This perfectly solves the Koffi struct redefinition issue across instances while avoiding repeated FFI overhead.
• [INFO][performance] Using this.detecting to cache the Promise of the probe() invocation correctly prevents race conditions if detect() is called concurrently by multiple callers. The logic here is very clean. LGTM!

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
Excellent fix for Koffi's process-global struct name collision. The introduction of sharedBindings correctly scopes the FFI types and function pointers to the process, while the detecting Promise safely deduplicates concurrent initialization calls on a per-instance basis. The code is clean and adheres to best practices. LGTM!

Comments:
• [INFO][bug] There is a minor lifecycle edge case here: if shutdown() is called concurrently while probe() is still awaiting sharedBindings, this.detecting is cleared by shutdown(). However, probe() will eventually resume and execute bindings.init(), setting this.initialized = true and effectively bypassing the shutdown request. This could leave the collector with an open NVML handle. For a typical metrics collector lifecycle, this is highly unlikely to cause real issues, but you can prevent it defensively by checking if the initialization was cancelled (or if the promise reference changed).

@@ -138,2 +138,5 @@
     const bindings = await sharedBindings
     if (!bindings) return false
+    
+    // Check if shutdown() was called while we were awaiting bindings
+    if (!this.detecting) return false
 
     try {

[alexcos20]

AI automated code review (Gemini 3).

Overall risk: low

Summary:
This PR successfully resolves the issue with koffi struct names throwing errors upon duplicate initialization by caching the bindings globally per process. It also correctly implements a promise-based singleton for concurrent detect() calls. I have identified a minor edge case involving async race conditions when stop() is called during initialization.

Comments:
• [INFO][architecture] Good use of a module-level promise to cache the Koffi bindings. This correctly prevents the koffi library from throwing duplicate struct name errors across multiple instances in the same process.
• [WARNING][bug] There is a potential async race condition here. If stop() is called while probe() is awaiting sharedBindings, stop() will clear this.detecting and this.bindings. However, when sharedBindings resolves, the suspended probe() function will resume, call bindings.init(), and forcefully re-assign this.bindings. This leaves the collector initialized and running when it should be stopped, potentially leaking the NVML handle.

You can easily prevent this by checking if this.detecting was cleared during the await:

@@ -134,3 +134,4 @@
     const bindings = await sharedBindings
-    if (!bindings) return false
+    // Abort if stop() was called while we were awaiting
+    if (!bindings || !this.detecting) return false
     try {

• [INFO][other] Just a small note on error handling: if this.loadKoffi() or this.buildBindings(koffi) throws an error, sharedBindings will become a rejected promise. Any subsequent calls to probe() across any instances will then automatically reject. This is likely the intended behavior (since if Koffi fails to load once, it usually won't succeed on a retry), but it's good to be aware of.