From 4885c5e9bac0a39247e9cc77d46ec64285cb11c1 Mon Sep 17 00:00:00 2001
From: gmega <giuliano.mega@gmail.com>
Date: Thu, 18 Jun 2026 12:25:24 -0300
Subject: [PATCH] address copilots comments

---
 build.nims                                      |  1 +
 library/logosmetrics.nim                        |  2 +-
 .../requests/node_info_request.nim              | 17 ++++++++++++++---
 tests/testLibstorage.nim                        |  2 ++
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/build.nims b/build.nims
index 60d48301..388aca3b 100644
--- a/build.nims
+++ b/build.nims
@@ -104,6 +104,7 @@ task test, "Run tests":
 
 task testAll, "Run all tests (except for Taiko L2 tests)":
   testStorageTask()
+  testLibstorageTask()
   testIntegrationTask()
 
 import strutils
diff --git a/library/logosmetrics.nim b/library/logosmetrics.nim
index cd830c94..ebf83671 100644
--- a/library/logosmetrics.nim
+++ b/library/logosmetrics.nim
@@ -2,7 +2,7 @@ import std/[json, locks, times, sets]
 
 import pkg/metrics
 
-proc toJson(collector: Collector, metrics: var seq[JsonNode] = @[]) =
+proc toJson(collector: Collector, metrics: var seq[JsonNode]) =
   # We know the closure won't outlive `metrics` so this is
   # an acceptable hack.
   let metricsPtr = addr metrics
diff --git a/library/storage_thread_requests/requests/node_info_request.nim b/library/storage_thread_requests/requests/node_info_request.nim
index 98e9399a..91cde598 100644
--- a/library/storage_thread_requests/requests/node_info_request.nim
+++ b/library/storage_thread_requests/requests/node_info_request.nim
@@ -79,10 +79,21 @@ proc process*(
       return err($res.error)
     return res
   of METRICS:
-    # We're running on the main thread so it should be fine to access
-    # defaultRegistry.
+    # We're making a few assumptions here, which need to be double-checked:
+    # 1. That defaultRegistry and all collectors we're using were
+    #    created on the host thread that called libstorageNimMain as globals
+    #    were initialized.
+    # 2. That collectors themselves are thread-safe.
+    # 3. That reading from defaultRegistry from this (worker) thread is safe.
+    #
+    # For (3) in particular, we lean on chronos_httpserver which does the exact
+    # thing we are doing here: reading defaultRegistry and its collectors and
+    # metrics from another thread.
+    #
+    # TODO double-check these assumptions.
     {.cast(gcsafe).}:
       # Excludes nim_runtime_info as dumpHeapInstances seems to be returning
-      # an infinite number of objects.
+      # an infinite number of objects (could be related to some assumption
+      # failure above).
       # FIXME figure out what's going on and add this back.
       return ok($defaultRegistry.toJson(exclude = @["nim_runtime_info"]))
diff --git a/tests/testLibstorage.nim b/tests/testLibstorage.nim
index a0669c5e..a703f263 100644
--- a/tests/testLibstorage.nim
+++ b/tests/testLibstorage.nim
@@ -1,2 +1,4 @@
 # Tests the Nim side of libstorage.
 import ./libstorage/logosmetrics
+
+{.warning[UnusedImport]: off.}