From ce200ee0a257d3d0eda22067a1284f2ce88b58d1 Mon Sep 17 00:00:00 2001 From: Marcin Czenko Date: Thu, 18 Jun 2026 17:54:39 +0200 Subject: [PATCH 1/4] fix: seg fault while running libstorage tests and adds some defensive checks --- storage.nim | 3 +- tests/cbindings/storage.c | 65 ++++++++++++++++++++------------------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/storage.nim b/storage.nim index aaff21ad..729fa2c2 100644 --- a/storage.nim +++ b/storage.nim @@ -95,8 +95,7 @@ when isMainModule: config.dataDir / config.netPrivKeyFile privateKey = setupKey(keyPath).valueOr: - fatal "Failed to set up the network private key", - path = keyPath, err = error.msg + fatal "Failed to set up the network private key", path = keyPath, err = error.msg quit QuitFailure server = diff --git a/tests/cbindings/storage.c b/tests/cbindings/storage.c index c27ed4ff..c06f4e10 100644 --- a/tests/cbindings/storage.c +++ b/tests/cbindings/storage.c @@ -168,9 +168,6 @@ static void callback(int ret, const char *msg, size_t len, void *userData) return; } - // Assign the return code to the response structure. - r->ret = ret; - // If the reponse already has a message, just free it first. if (r->msg) { @@ -213,6 +210,10 @@ static void callback(int ret, const char *msg, size_t len, void *userData) r->msg = NULL; r->len = 0; } + + // Publish the return code last. wait_resp uses this as the completion flag, + // so setting it earlier lets the caller read/free a partially written Resp. + r->ret = ret; } static int read_file(const char *filepath, char **res) @@ -343,9 +344,9 @@ int check_repo(void *storage_ctx) int ret = is_resp_ok(r, &res); - if (strcmp(res, "./data-dir") != 0) + if (res == NULL || strcmp(res, "./data-dir") != 0) { - printf("repo mismatch: %s\n", res); + printf("repo mismatch: %s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -368,9 +369,9 @@ int check_debug(void *storage_ctx) int ret = is_resp_ok(r, &res); // Simple check to ensure the response contains spr - if (strstr(res, "spr") == NULL) + if (res == NULL || strstr(res, "spr") == NULL) { - fprintf(stderr, "debug content mismatch, res:%s\n", res); + fprintf(stderr, "debug content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -392,9 +393,9 @@ int check_spr(void *storage_ctx) int ret = is_resp_ok(r, &res); - if (strstr(res, "spr") == NULL) + if (res == NULL || strstr(res, "spr") == NULL) { - fprintf(stderr, "spr content mismatch, res:%s\n", res); + fprintf(stderr, "spr content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -562,7 +563,7 @@ int check_upload_file(void *storage_ctx, const char *filepath, char **res) int ret = is_resp_ok(r, res); - if (res == NULL || strlen(*res) == 0) + if (res == NULL || *res == NULL || strlen(*res) == 0) { fprintf(stderr, "CID is missing\n"); return RET_ERR; @@ -600,9 +601,9 @@ int check_download_stream(void *storage_ctx, const char *cid, const char *filepa int ret = is_resp_ok(r, &res); - if (strncmp(res, "Hello World!", strlen("Hello World!")) != 0) + if (res == NULL || strncmp(res, "Hello World!", strlen("Hello World!")) != 0) { - fprintf(stderr, "downloaded content mismatch, res:%s\n", res); + fprintf(stderr, "downloaded content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -612,9 +613,9 @@ int check_download_stream(void *storage_ctx, const char *cid, const char *filepa ret = RET_ERR; } - if (strncmp(res, "Hello World!", strlen("Hello World!")) != 0) + if (res == NULL || strncmp(res, "Hello World!", strlen("Hello World!")) != 0) { - fprintf(stderr, "downloaded content mismatch, res:%s\n", res); + fprintf(stderr, "downloaded content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -652,9 +653,9 @@ int check_download_chunk(void *storage_ctx, const char *cid) int ret = is_resp_ok(r, &res); - if (strncmp(res, "Hello World!", strlen("Hello World!")) != 0) + if (res == NULL || strncmp(res, "Hello World!", strlen("Hello World!")) != 0) { - fprintf(stderr, "downloaded chunk content mismatch, res:%s\n", res); + fprintf(stderr, "downloaded chunk content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -691,9 +692,9 @@ int check_download_manifest(void *storage_ctx, const char *cid) const char *expected_manifest = "{\"manifestVersion\":0,\"treeCid\":\"zDzSvJTf8JYwvysKPmG7BtzpbiAHfuwFMRphxm4hdvnMJ4XPJjKX\",\"datasetSize\":12,\"blockSize\":65536,\"filename\":\"hello_world.txt\",\"mimetype\":\"text/plain\"}"; - if (strncmp(res, expected_manifest, strlen(expected_manifest)) != 0) + if (res == NULL || strncmp(res, expected_manifest, strlen(expected_manifest)) != 0) { - fprintf(stderr, "downloaded manifest content mismatch, res:%s\n", res); + fprintf(stderr, "downloaded manifest content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -717,9 +718,9 @@ int check_list(void *storage_ctx) const char *expected_manifest = "{\"manifestVersion\":0,\"treeCid\":\"zDzSvJTf8JYwvysKPmG7BtzpbiAHfuwFMRphxm4hdvnMJ4XPJjKX\",\"datasetSize\":12,\"blockSize\":65536,\"filename\":\"hello_world.txt\",\"mimetype\":\"text/plain\"}"; - if (strstr(res, expected_manifest) == NULL) + if (res == NULL || strstr(res, expected_manifest) == NULL) { - fprintf(stderr, "downloaded manifest content mismatch, res:%s\n", res); + fprintf(stderr, "downloaded manifest content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -742,9 +743,9 @@ int check_space(void *storage_ctx) int ret = is_resp_ok(r, &res); // Simple check to ensure the response contains totalBlocks - if (strstr(res, "totalBlocks") == NULL) + if (res == NULL || strstr(res, "totalBlocks") == NULL) { - fprintf(stderr, "list content mismatch, res:%s\n", res); + fprintf(stderr, "space content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } @@ -768,17 +769,17 @@ int check_exists(void *storage_ctx, const char *cid, bool expected) if (expected) { - if (strcmp(res, "true") != 0) + if (res == NULL || strcmp(res, "true") != 0) { - fprintf(stderr, "exists content mismatch, res:%s\n", res); + fprintf(stderr, "exists content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } } else { - if (strcmp(res, "false") != 0) + if (res == NULL || strcmp(res, "false") != 0) { - fprintf(stderr, "exists content mismatch, res:%s\n", res); + fprintf(stderr, "exists content mismatch, res:%s\n", res ? res : "(null)"); ret = RET_ERR; } } @@ -813,9 +814,9 @@ int check_toggle_private_queries(void *storage_ctx) } int ret = is_resp_ok(r, &res); - if (strcmp(res, "false") != 0) + if (res == NULL || strcmp(res, "false") != 0) { - fprintf(stderr, "toggle private queries content mismatch, res:%s\n", res); + fprintf(stderr, "toggle private queries content mismatch, res:%s\n", res ? res : "(null)"); free(res); return RET_ERR; } @@ -830,9 +831,9 @@ int check_toggle_private_queries(void *storage_ctx) } ret = is_resp_ok(r, &res); - if (strcmp(res, "true") != 0) + if (res == NULL || strcmp(res, "true") != 0) { - fprintf(stderr, "toggle private queries content mismatch, res:%s\n", res); + fprintf(stderr, "toggle private queries content mismatch, res:%s\n", res ? res : "(null)"); free(res); return RET_ERR; } @@ -860,7 +861,7 @@ int check_get_metrics(void *storage_ctx) } // Checks that response contains a metric we are SURE must exist - if (strstr(res, "libp2p_successful_dials_total") == NULL) + if (res == NULL || strstr(res, "libp2p_successful_dials_total") == NULL) { fprintf(stderr, "get_metrics missing expected metric\n"); free(res); @@ -928,4 +929,4 @@ int main(void) RUN_TEST(cleanup(storage_ctx)); END_SUITE -} \ No newline at end of file +} From ccb6ed130330eb5c358703c25f42a540fc025099 Mon Sep 17 00:00:00 2001 From: Marcin Czenko Date: Thu, 18 Jun 2026 18:20:24 +0200 Subject: [PATCH 2/4] fix: better (??) thread synchronization --- tests/cbindings/storage.c | 79 +++++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 12 deletions(-) diff --git a/tests/cbindings/storage.c b/tests/cbindings/storage.c index c06f4e10..0ad23102 100644 --- a/tests/cbindings/storage.c +++ b/tests/cbindings/storage.c @@ -1,8 +1,11 @@ +#include +#include #include #include #include #include #include +#include #include #include "../../library/libstorage.h" @@ -51,6 +54,9 @@ typedef struct { + pthread_mutex_t mutex; + pthread_cond_t cond; + bool done; int ret; char *msg; char *chunk; @@ -60,6 +66,9 @@ typedef struct static Resp *alloc_resp(void) { Resp *r = (Resp *)calloc(1, sizeof(Resp)); + pthread_mutex_init(&r->mutex, NULL); + pthread_cond_init(&r->cond, NULL); + r->done = false; r->msg = NULL; r->chunk = NULL; r->ret = -1; @@ -83,6 +92,8 @@ static void free_resp(Resp *r) free(r->chunk); } + pthread_cond_destroy(&r->cond); + pthread_mutex_destroy(&r->mutex); free(r); } @@ -93,7 +104,11 @@ static int get_ret(Resp *r) return RET_ERR; } - return r->ret; + pthread_mutex_lock(&r->mutex); + int ret = r->ret; + pthread_mutex_unlock(&r->mutex); + + return ret; } // wait_resp waits until the async response is ready or max retries is reached. @@ -101,13 +116,33 @@ static int get_ret(Resp *r) // indicate that the response is ready to be consumed. static void wait_resp(Resp *r) { - int retries = 0; - - while (get_ret(r) == -1 && retries < MAX_RETRIES) + if (!r) { - usleep(1000 * 100); // 100 ms - retries++; + return; } + + const long timeout_ms = MAX_RETRIES * 100; + struct timespec deadline; + + clock_gettime(CLOCK_REALTIME, &deadline); + deadline.tv_sec += timeout_ms / 1000; + deadline.tv_nsec += (timeout_ms % 1000) * 1000000; + if (deadline.tv_nsec >= 1000000000) + { + deadline.tv_sec += 1; + deadline.tv_nsec -= 1000000000; + } + + pthread_mutex_lock(&r->mutex); + while (!r->done) + { + int rc = pthread_cond_timedwait(&r->cond, &r->mutex, &deadline); + if (rc == ETIMEDOUT) + { + break; + } + } + pthread_mutex_unlock(&r->mutex); } // is_resp_ok checks if the async response indicates success. @@ -122,6 +157,8 @@ static int is_resp_ok(Resp *r, char **res) wait_resp(r); + pthread_mutex_lock(&r->mutex); + int ret = (r->ret == RET_OK) ? RET_OK : RET_ERR; // If a response pointer is provided, it’s safe to initialize it to NULL. @@ -142,6 +179,8 @@ static int is_resp_ok(Resp *r, char **res) *res = strdup(r->msg); } + pthread_mutex_unlock(&r->mutex); + free_resp(r); return ret; @@ -168,6 +207,8 @@ static void callback(int ret, const char *msg, size_t len, void *userData) return; } + pthread_mutex_lock(&r->mutex); + // If the reponse already has a message, just free it first. if (r->msg) { @@ -184,8 +225,8 @@ static void callback(int ret, const char *msg, size_t len, void *userData) r->len = len; } - // For other cases, copy the message data. - if (msg && len > 0) + // For terminal responses, copy the message data. + if (ret != RET_PROGRESS && msg && len > 0) { // Allocate memory for the message plus null terminator. r->msg = (char *)malloc(len + 1); @@ -194,6 +235,10 @@ static void callback(int ret, const char *msg, size_t len, void *userData) if (!r->msg) { r->len = 0; + r->ret = RET_ERR; + r->done = true; + pthread_cond_signal(&r->cond); + pthread_mutex_unlock(&r->mutex); return; } @@ -205,15 +250,25 @@ static void callback(int ret, const char *msg, size_t len, void *userData) r->len = len; } - else + else if (ret != RET_PROGRESS) { r->msg = NULL; r->len = 0; } - // Publish the return code last. wait_resp uses this as the completion flag, - // so setting it earlier lets the caller read/free a partially written Resp. + // Progress updates are intermediate callbacks. Keep any copied chunk data, + // but wait for the final RET_OK/RET_ERR before completing the response. + if (ret == RET_PROGRESS) + { + pthread_mutex_unlock(&r->mutex); + return; + } + + // Publish completion last so wait_resp can only observe a fully written Resp. r->ret = ret; + r->done = true; + pthread_cond_signal(&r->cond); + pthread_mutex_unlock(&r->mutex); } static int read_file(const char *filepath, char **res) @@ -257,7 +312,7 @@ int setup(void **storage_ctx) wait_resp(r); - if (r->ret != RET_OK) + if (get_ret(r) != RET_OK) { free_resp(r); return RET_ERR; From 94e0abd579cc8eb0f7a7c68e1f38e08a5ce756a1 Mon Sep 17 00:00:00 2001 From: Marcin Czenko Date: Thu, 18 Jun 2026 18:55:35 +0200 Subject: [PATCH 3/4] build: minor fixes --- Makefile | 1 + build.nims | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2220f87f..f7a8edb4 100644 --- a/Makefile +++ b/Makefile @@ -85,6 +85,7 @@ endif test \ testAll \ testIntegration \ + testLibstorageC \ testLibstorage \ update diff --git a/build.nims b/build.nims index 388aca3b..60d48301 100644 --- a/build.nims +++ b/build.nims @@ -104,7 +104,6 @@ task test, "Run tests": task testAll, "Run all tests (except for Taiko L2 tests)": testStorageTask() - testLibstorageTask() testIntegrationTask() import strutils From ebd8a427557604b973365ffd48f0d7b40a543a3c Mon Sep 17 00:00:00 2001 From: Marcin Czenko Date: Thu, 18 Jun 2026 19:47:34 +0200 Subject: [PATCH 4/4] feat: include metric type and help --- library/logosmetrics.nim | 23 ++++++++- tests/libstorage/logosmetrics.nim | 82 +++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 12 deletions(-) diff --git a/library/logosmetrics.nim b/library/logosmetrics.nim index ebf83671..338e1092 100644 --- a/library/logosmetrics.nim +++ b/library/logosmetrics.nim @@ -1,11 +1,28 @@ -import std/[json, locks, times, sets] +import std/[json, locks, strutils, times, sets] import pkg/metrics +proc jsonHelp(collector: Collector): string = + let prefix = "# HELP " & collector.name & " " + if collector.help.startsWith(prefix): + return collector.help[prefix.len .. ^1].strip(leading = false, trailing = true) + + return "" + +proc jsonType(collector: Collector): string = + let parts = collector.typ.splitWhitespace() + if parts.len == 4 and parts[0] == "#" and parts[1] == "TYPE" and + parts[2] == collector.name: + return parts[3] + + return "unknown" + 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 + let help = collector.jsonHelp() + let typ = collector.jsonType() proc serializeMetric( name: string, @@ -22,7 +39,9 @@ proc toJson(collector: Collector, metrics: var seq[JsonNode]) = for i in 0 ..< min(labelValues.len, labels.len): labelMap[labels[i]] = %labelValues[i] - metricsPtr[].add(%*{"name": name, "value": value, "labels": labelMap}) + metricsPtr[].add( + %*{"name": name, "type": typ, "help": help, "value": value, "labels": labelMap} + ) collector.collect(serializeMetric) diff --git a/tests/libstorage/logosmetrics.nim b/tests/libstorage/logosmetrics.nim index e04f2c83..8c7517b4 100644 --- a/tests/libstorage/logosmetrics.nim +++ b/tests/libstorage/logosmetrics.nim @@ -56,15 +56,69 @@ suite "Metrics": check filteredMetrics == %*{ "metrics": [ - {"name": "myCounter_total", "value": 1.0, "labels": {"part_type": "screws"}}, - {"name": "myCounter_total", "value": 1.0, "labels": {"part_type": "washers"}}, - {"name": "myGauge", "value": 42.0, "labels": {}}, - {"name": "myHistogram_sum", "value": 15.0, "labels": {}}, - {"name": "myHistogram_count", "value": 5.0, "labels": {}}, - {"name": "myHistogram_bucket", "value": 0.0, "labels": {"le": "0.0"}}, - {"name": "myHistogram_bucket", "value": 1.0, "labels": {"le": "1.0"}}, - {"name": "myHistogram_bucket", "value": 2.0, "labels": {"le": "2.0"}}, - {"name": "myHistogram_bucket", "value": 5.0, "labels": {"le": "+Inf"}}, + { + "name": "myCounter_total", + "type": "counter", + "help": "Parts Counter", + "value": 1.0, + "labels": {"part_type": "screws"}, + }, + { + "name": "myCounter_total", + "type": "counter", + "help": "Parts Counter", + "value": 1.0, + "labels": {"part_type": "washers"}, + }, + { + "name": "myGauge", + "type": "gauge", + "help": "My gauge", + "value": 42.0, + "labels": {}, + }, + { + "name": "myHistogram_sum", + "type": "histogram", + "help": "My histogram", + "value": 15.0, + "labels": {}, + }, + { + "name": "myHistogram_count", + "type": "histogram", + "help": "My histogram", + "value": 5.0, + "labels": {}, + }, + { + "name": "myHistogram_bucket", + "type": "histogram", + "help": "My histogram", + "value": 0.0, + "labels": {"le": "0.0"}, + }, + { + "name": "myHistogram_bucket", + "type": "histogram", + "help": "My histogram", + "value": 1.0, + "labels": {"le": "1.0"}, + }, + { + "name": "myHistogram_bucket", + "type": "histogram", + "help": "My histogram", + "value": 2.0, + "labels": {"le": "2.0"}, + }, + { + "name": "myHistogram_bucket", + "type": "histogram", + "help": "My histogram", + "value": 5.0, + "labels": {"le": "+Inf"}, + }, ] } @@ -76,9 +130,17 @@ suite "Metrics": check metrics == %*{ "metrics": [ - {"name": "bad_metric", "value": 1.0, "labels": {"label1": "value1"}}, { "name": "bad_metric", + "type": "gauge", + "help": "Badly behaved collector", + "value": 1.0, + "labels": {"label1": "value1"}, + }, + { + "name": "bad_metric", + "type": "gauge", + "help": "Badly behaved collector", "value": 1.0, "labels": {"label1": "value1", "label2": "value2"}, },