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()