diff --git a/api/openapi-v1.yaml b/api/openapi-v1.yaml index a0d56e66b..e1a8af3b5 100644 --- a/api/openapi-v1.yaml +++ b/api/openapi-v1.yaml @@ -36,7 +36,13 @@ components: properties: status: type: string - description: "ok when healthy; a non-ok string when degraded" + enum: [ok, degraded] + description: >- + Whether the KB can do its job, derived from `blockers`, not whether + this handler ran. `degraded` means it answered and cannot work; the + reasons are in `blockers`. A degraded KB is still up: treat it as + reachable and read the blockers, do not retry it as a transport + failure. Advisory findings live in `warnings` and leave status `ok`. db2_ok: type: boolean description: DB2 (Postgres) reachable @@ -58,6 +64,14 @@ components: type: integer warnings: type: array + description: Advisory findings. Noteworthy, but the KB still works. + items: + type: string + blockers: + type: array + description: >- + Reasons the KB cannot do its job, each naming its remedy. Empty + exactly when status is ok. This is the field to show an operator. items: type: string diff --git a/docs/runbooks/retrieval-readiness-and-recovery.md b/docs/runbooks/retrieval-readiness-and-recovery.md index 901f1e16e..8b6753015 100644 --- a/docs/runbooks/retrieval-readiness-and-recovery.md +++ b/docs/runbooks/retrieval-readiness-and-recovery.md @@ -14,6 +14,21 @@ The shipped Compose files use `restart: unless-stopped` and intentionally health `/v1/health` and readiness at `/v1/ready`. A 503 from readiness is an operational signal, not a reason to kill the process. +The healthcheck is `curl -fsS`, so it reads only the HTTP status. A container can be +`healthy` while the KB inside it cannot embed or search. That is deliberate: a width +mismatch or a missing embedder is not fixed by restarting, and failing the healthcheck +would turn a diagnosable fault into a restart loop. **The body carries the verdict the +HTTP status cannot.** Read `status` and `blockers`: + +| `status` | Meaning | +|-------------|------------------------------------------------------------| +| `ok` | The KB can serve retrieval. | +| `degraded` | It answered and cannot work. `blockers` says why, and names the remedy for each. | + +`blockers` is empty exactly when `status` is `ok`. `warnings` is advisory (a stale +ingest, unembedded chunks) and leaves `status` at `ok`. `aimee status` surfaces each +blocker on its own `BLOCKED:` line. + ## Diagnose Run these from a host that can reach the relevant listeners (add authentication or TLS diff --git a/src/cli_v1_routes_b.c b/src/cli_v1_routes_b.c index 12b2ac751..27ccf84e1 100644 --- a/src/cli_v1_routes_b.c +++ b/src/cli_v1_routes_b.c @@ -1600,6 +1600,17 @@ void print_server_health(cJSON *resp) { const char *kbs = json_str(kb, "status"); printf("aimee-kb: %s\n", (kbs && kbs[0]) ? kbs : "unknown"); + /* Why the kb cannot work, straight from the kb, before any detail line. + * These are the sentences someone is running this command to find; burying + * them under the store/vector/embedder triple means reading "ok" first and + * inferring the rest. */ + cJSON *blockers = cJSON_GetObjectItemCaseSensitive(kb, "blockers"); + if (cJSON_IsArray(blockers) && cJSON_GetArraySize(blockers) > 0) + { + cJSON *b; + cJSON_ArrayForEach(b, blockers) if (cJSON_IsString(b)) + printf(" BLOCKED: %s\n", b->valuestring); + } /* An open transport breaker refuses every call locally, so the kb can be * "ok" here while nothing works. Print it before the detail lines: this is * the line that explains an index that answers "unavailable" on a server @@ -1614,7 +1625,11 @@ void print_server_health(cJSON *resp) printf(" next retry in %lldms; see the server log for the cause.\n", (long long)retry->valuedouble); } - if (kbs && strcmp(kbs, "ok") == 0) + /* "degraded" means the kb ANSWERED and told us what is broken, so the detail + * lines below are real and worth printing. Only a kb that never answered + * gets the "did not answer" text — testing `== "ok"` would have sent every + * degraded install down that branch and reported a running kb as absent. */ + if (kbs && (strcmp(kbs, "ok") == 0 || strcmp(kbs, "degraded") == 0)) { cJSON *vec = cJSON_GetObjectItemCaseSensitive(kb, "vectors"); printf(" store: %s\n", @@ -1634,6 +1649,17 @@ void print_server_health(cJSON *resp) { printf(" the knowledge base did not answer; memory and kb search will not work.\n"); printf(" `aimee kb status` has the detail.\n"); + /* "did not answer" reads as a network problem, and the most common cause + * is not. A kb that refuses to start fails CLOSED before it ever binds + * the health port, so its diagnosis never reaches this response and + * exists only in the container log. Measured: booting a 768-dimension + * embedder over a corpus recorded at 384 logs the width, both sides, and + * the remedy, then holds DB2 unready until the container crashloops -- + * and every operator-facing surface said "unreachable", pointing away + * from the one place that already knew the answer. Name that place. */ + printf(" if it never became healthy, the reason is in its own log and not\n"); + printf(" on the network: `docker logs aimee-kb` (compose) or the kb\n"); + printf(" service log for your deployment.\n"); } } } diff --git a/src/hud.c b/src/hud.c index 6c83b18ce..07a2cb1cf 100644 --- a/src/hud.c +++ b/src/hud.c @@ -62,7 +62,14 @@ int hud_gather(hud_status_t *out) if (kb_client_health(&kbh) == 0) { out->kb_health_checked = 1; - if (!kbh.pgvec_ok || !kbh.pgvec_collection_ok || kbh.freshness_days > 30) + /* Prefer the kb's own verdict: this block used to re-derive one from two + * booleans, which made it a fourth copy of the same judgement and left the + * HUD blind to everything those two did not cover — a missing embedder and + * a width mismatch both showed a clean HUD on a kb that could not embed. + * The boolean terms stay as a fallback for an older kb that sends no + * status, where they are all the evidence there is. */ + if (strcmp(kbh.status, "degraded") == 0 || !kbh.pgvec_ok || !kbh.pgvec_collection_ok || + kbh.freshness_days > 30) out->kb_health_fail = 1; else if (kbh.freshness_days > 7 || (kbh.chunk_count > 0 && kbh.embedding_count < kbh.chunk_count * 9 / 10)) diff --git a/src/kb/kb_service_kb.c b/src/kb/kb_service_kb.c index f8b9f8971..ee544e36d 100644 --- a/src/kb/kb_service_kb.c +++ b/src/kb/kb_service_kb.c @@ -301,12 +301,36 @@ static void kb_health_add_curator(cJSON *resp, kb_curator_queue_counts_t *out_co } } +/* THE ONE PLACE a capability verdict is computed. Add a blocker at the site that + * discovers the evidence and the summary follows automatically — that is the whole + * point of this function existing. + * + * `status` used to be the literal string "ok", written as the first statement of + * kb_service_health_object and never revised. Every finding below it -- db2_ok, + * pgvec_ok, embed_ok, the dimension-refusal counter -- was published as a SIBLING + * field that nothing aggregated, so the response could say "ok" while carrying the + * proof it was not. That is not a bug in any one check; it is the absence of a + * place where the checks add up, and it reproduced three times (here, in + * server_health_add_kb, and in the transport-breaker patch that answered it by + * adding yet another sibling for the CLI to special-case). + * + * A blocker means CANNOT WORK, not "worth mentioning". Advisory findings stay in + * `warnings` and leave the verdict at ok: a stale ingest, a disabled synthesis + * tier, or zero vectors on a fresh install are all supported states, and + * degrading on them would dilute the signal in exactly the direction this + * function exists to correct. */ +static const char *kb_health_verdict(cJSON *blockers) +{ + return (cJSON_IsArray(blockers) && cJSON_GetArraySize(blockers) > 0) ? "degraded" : "ok"; +} + static cJSON *kb_service_health_object(void) { cJSON *resp = cJSON_CreateObject(); if (!resp) return NULL; - cJSON_AddStringToObject(resp, "status", "ok"); + /* No `status` here. It is derived from the blockers array at the bottom of this + * function, once the evidence it summarises actually exists. */ /* DB2: generic schema + KB-specific tables */ int schema_ok = 0, have_pg_trgm = 0, kb_tables_ok = 0; @@ -406,14 +430,45 @@ static cJSON *kb_service_health_object(void) cJSON_AddNumberToObject(resp, "chunk_count", stats.chunks); cJSON_AddNumberToObject(resp, "embedding_count", stats.embeddings); - /* Warning accumulation */ + /* Warning accumulation. + * + * Two arrays, two meanings, kept deliberately distinct: `warnings` is + * everything worth saying, `blockers` is the subset that means the kb cannot + * do its job. Only the latter moves `status`. A blocker names its remedy — + * the operator reading it is by definition looking at something broken, and + * "vector store unavailable" without "reinstall the extension" costs them the + * search that follows. */ cJSON *warnings = cJSON_AddArrayToObject(resp, "warnings"); + cJSON *blockers = cJSON_AddArrayToObject(resp, "blockers"); if (!db2_ok) + { cJSON_AddItemToArray(warnings, cJSON_CreateString("DB2 schema not ready")); + cJSON_AddItemToArray(blockers, + cJSON_CreateString("store unavailable: the KB database schema is not " + "ready, so nothing can be stored or retrieved")); + } if (!pgvec_ok) + { cJSON_AddItemToArray(warnings, cJSON_CreateString("pgvector extension not loaded in DB2")); + cJSON_AddItemToArray(blockers, + cJSON_CreateString("vector store unavailable: the pgvector extension is " + "not loaded, so dense retrieval cannot run")); + } if (!pgvec_collection_ok) + { cJSON_AddItemToArray(warnings, cJSON_CreateString("KB vector table missing")); + cJSON_AddItemToArray(blockers, cJSON_CreateString("vector table missing: the KB chunk vector " + "table is absent, so search returns " + "nothing")); + } + /* An unconfigured embedder is the single most common way this install reaches + * "accepted, reports healthy, cannot work": deploy succeeds, the container goes + * healthy, and the first `aimee memory store` fails with no mention of an + * embedder anywhere. */ + if (!embed_ok) + cJSON_AddItemToArray(blockers, + cJSON_CreateString("no embedder configured: set embedder_model (or " + "EMBEDDER_URL) — memory and KB search cannot embed")); /* A curator that fails every job used to leave the kb reporting a clean * bill of health. The counters said pending=41485 done=0 — indistinguishable * from "queued, not started yet" — and nothing in warnings mentioned that @@ -496,8 +551,15 @@ static cJSON *kb_service_health_object(void) "(`aimee kb reembed`).", dim_refused, db2_embedding_dim_last_offered(), active_dim); cJSON_AddItemToArray(warnings, cJSON_CreateString(msg)); + /* The comment above says this state means "dense retrieval is dead". It said + * so while the response reported status ok, because saying it in a warning + * was the end of the sentence. It is a blocker. */ + cJSON_AddItemToArray(blockers, cJSON_CreateString(msg)); } + /* Derived last, from the evidence above, and never written anywhere else. */ + cJSON_AddStringToObject(resp, "status", kb_health_verdict(blockers)); + /* Maintenance stats */ char last_maintenance_at[64] = ""; db2_kb_runtime_state_get("last_maintenance_at", last_maintenance_at, diff --git a/src/modules/kb_client/kb_client.c b/src/modules/kb_client/kb_client.c index 455bd07be..e7de95610 100644 --- a/src/modules/kb_client/kb_client.c +++ b/src/modules/kb_client/kb_client.c @@ -584,6 +584,35 @@ char *kb_client_curator_json(void) return out ? out : kb_status_unavailable_json("curator status serialization failed"); } +/* Flatten a JSON array of strings into a newline-separated buffer, truncating at + * the buffer rather than overrunning it. Extracted when `blockers` joined + * `warnings` and the second caller made a copy-paste of the pointer arithmetic + * the obvious alternative. A non-array (including a missing key, which is how an + * older kb answers) leaves the buffer untouched. */ +static void kb_client_join_strings(cJSON *arr, char *buf, size_t cap) +{ + if (!cJSON_IsArray(arr) || !buf || cap == 0) + return; + size_t pos = 0; + cJSON *item; + cJSON_ArrayForEach(item, arr) + { + if (!cJSON_IsString(item)) + continue; + if (pos > 0 && pos < cap - 1) + buf[pos++] = '\n'; + size_t rem = cap - pos - 1; + if (rem == 0) + break; + size_t len = strlen(item->valuestring); + if (len > rem) + len = rem; + memcpy(buf + pos, item->valuestring, len); + pos += len; + buf[pos] = '\0'; + } +} + int kb_client_health(kb_health_t *out) { if (!out) @@ -605,8 +634,18 @@ int kb_client_health(kb_health_t *out) if (!resp) return -1; + /* process_ok means SOMETHING ANSWERED, which is the only thing this check can + * honestly establish. It used to demand status == "ok" exactly and return -1 + * otherwise — so the moment the kb learned to say "degraded", a kb that was up + * and telling us precisely what was wrong would have been reported to every + * caller as unreachable, and its blockers discarded unread. The verdict is + * carried in out->status for callers to act on; it is not this function's job + * to turn a diagnosis into a transport failure. + * + * Any status string counts as an answer. An unparseable or non-200 response + * has already returned -1 above, which is the real "did not answer". */ cJSON *s = cJSON_GetObjectItemCaseSensitive(resp, "status"); - if (!cJSON_IsString(s) || strcmp(s->valuestring, "ok") != 0) + if (!cJSON_IsString(s)) { cJSON_Delete(resp); return -1; @@ -643,6 +682,7 @@ int kb_client_health(kb_health_t *out) COPY_INT(pgvec_indexed, "pgvec_indexed_vectors"); COPY_BOOL(embed_ok, "embed_ok"); COPY_STR(embed_command, "embed_command"); + COPY_STR(status, "status"); COPY_INT(freshness_days, "freshness_days"); COPY_STR(last_ingest_at, "last_ingest_at"); COPY_INT(chunk_count, "chunk_count"); @@ -669,28 +709,10 @@ int kb_client_health(kb_health_t *out) #undef COPY_INT #undef COPY_STR - cJSON *warns = cJSON_GetObjectItemCaseSensitive(resp, "warnings"); - if (cJSON_IsArray(warns)) - { - size_t pos = 0; - cJSON *w; - cJSON_ArrayForEach(w, warns) - { - if (!cJSON_IsString(w)) - continue; - if (pos > 0 && pos < sizeof(out->warnings) - 1) - out->warnings[pos++] = '\n'; - size_t rem = sizeof(out->warnings) - pos - 1; - if (rem == 0) - break; - size_t wlen = strlen(w->valuestring); - if (wlen > rem) - wlen = rem; - memcpy(out->warnings + pos, w->valuestring, wlen); - pos += wlen; - out->warnings[pos] = '\0'; - } - } + kb_client_join_strings(cJSON_GetObjectItemCaseSensitive(resp, "warnings"), out->warnings, + sizeof(out->warnings)); + kb_client_join_strings(cJSON_GetObjectItemCaseSensitive(resp, "blockers"), out->blockers, + sizeof(out->blockers)); cJSON_Delete(resp); diff --git a/src/modules/kb_client/kb_client.h b/src/modules/kb_client/kb_client.h index 0e0def182..2fe7ea941 100644 --- a/src/modules/kb_client/kb_client.h +++ b/src/modules/kb_client/kb_client.h @@ -32,6 +32,12 @@ typedef struct int chunk_count; int embedding_count; char warnings[512]; /* newline-separated warning strings */ + /* The kb's own verdict on whether it can do its job: "ok" | "degraded". + * Distinct from process_ok, which only says something answered. Empty when an + * older kb omits it — callers must treat empty as "no verdict offered", not as + * ok, or they reintroduce exactly the gap this field closes. */ + char status[16]; + char blockers[512]; /* newline-separated incapacity reasons; empty when ok */ char last_maintenance_at[64]; int last_maintenance_rows_decayed; int last_maintenance_orphans_pruned; diff --git a/src/server/server_api_status.c b/src/server/server_api_status.c index f259d2f48..c1f2114a6 100644 --- a/src/server/server_api_status.c +++ b/src/server/server_api_status.c @@ -106,18 +106,41 @@ void server_health_add_kb(cJSON *resp) if (!kbo) return; int reachable = (kb_rc == 0 && kb.process_ok); - cJSON_AddStringToObject(kbo, "status", reachable ? "ok" : "unreachable"); - - /* `status` answers "is the kb process up", which is not the same question as - * "can I query it". With the transport breaker open every call is refused - * locally without the kb ever being contacted, and this block still said - * "ok" — so an operator watching status saw a healthy kb while every lookup - * failed, and had no way to connect the two. Report the breaker here, in both - * the reachable and unreachable cases, since that is where people look. */ + + /* `status` used to be exactly `reachable ? "ok" : "unreachable"` — "is the kb + * process up", which is not the same question as "can I query it". Every + * capability the kb reported arrived as a sibling field beside it, so this + * object could say "ok" and "embed_configured": false in the same breath, and + * `aimee status` printed "aimee-kb: ok" directly above "embedder: not + * configured". The transport breaker hit the same wall and was answered by + * adding ANOTHER sibling (queries_suppressed) for the CLI to special-case; + * that is the gap reproducing rather than closing. + * + * Three states now, and they are ordered by what the operator can act on: + * + * unreachable nothing answered — the kb's own verdict does not exist + * degraded something answered and told us it cannot work, OR the breaker + * is refusing every call locally before the kb is ever contacted + * ok answered and claims capability + * + * The breaker is folded into the verdict rather than left as a parallel flag: + * an open breaker means queries fail, which is the definition of degraded, and + * leaving it beside `status` is what forced the bespoke CLI branch. */ kb_client_dependency_health_t dep; kb_client_dependency_health(&dep); + int breaker_open = strcmp(dep.state, "open") == 0; + + const char *status; + if (!reachable) + status = "unreachable"; + else if (breaker_open || strcmp(kb.status, "degraded") == 0) + status = "degraded"; + else + status = "ok"; + cJSON_AddStringToObject(kbo, "status", status); + cJSON_AddStringToObject(kbo, "transport_state", dep.state); - if (strcmp(dep.state, "open") == 0) + if (breaker_open) { cJSON_AddBoolToObject(kbo, "queries_suppressed", 1); cJSON_AddNumberToObject(kbo, "retry_after_ms", (double)dep.retry_after_ms); @@ -125,6 +148,25 @@ void server_health_add_kb(cJSON *resp) } if (!reachable) return; + /* Pass the reasons through verbatim. The kb composed them next to the evidence + * and named the remedy; re-deriving them here from the booleans below would be + * a second place for the verdict to drift out of step with the facts. */ + if (kb.blockers[0]) + { + cJSON *arr = cJSON_AddArrayToObject(kbo, "blockers"); + for (const char *p = kb.blockers; p && *p;) + { + const char *nl = strchr(p, '\n'); + size_t len = nl ? (size_t)(nl - p) : strlen(p); + char line[320]; + if (len >= sizeof(line)) + len = sizeof(line) - 1; + memcpy(line, p, len); + line[len] = '\0'; + cJSON_AddItemToArray(arr, cJSON_CreateString(line)); + p = nl ? nl + 1 : NULL; + } + } cJSON_AddBoolToObject(kbo, "store_ok", kb.db2_ok ? 1 : 0); cJSON_AddBoolToObject(kbo, "vectors_ok", kb.pgvec_ok ? 1 : 0); cJSON_AddBoolToObject(kbo, "embed_configured", kb.embed_ok ? 1 : 0); diff --git a/src/tests/test_kb_client_search.c b/src/tests/test_kb_client_search.c index c95ba95a2..4dc72c356 100644 --- a/src/tests/test_kb_client_search.c +++ b/src/tests/test_kb_client_search.c @@ -814,12 +814,115 @@ static void test_health_uses_v1_api_when_configured(void) assert(strstr(health.warnings, "warn-a") != NULL); assert(strstr(health.warnings, "warn-b") != NULL); + assert(strcmp(health.status, "ok") == 0); + assert(health.blockers[0] == '\0'); + assert(g_get_seen == 2); unsetenv("AIMEE_KB_API_URL"); runtime_secret_remove("AIMEE_KB_API_BEARER_TOKEN"); mock_agent_http_reset(); } +/* A kb that answers "degraded" is UP and is telling us exactly what is wrong. + * + * kb_client_health used to require status == "ok" and return -1 for anything + * else, which predates the kb having any other verdict to send. The moment it + * gained one, that check would have converted every degraded kb into a transport + * failure: `aimee status` would print "the knowledge base did not answer" about a + * running kb, and the blockers explaining the fault would be discarded unread — + * the original defect inverted, and strictly worse, because "unreachable" sends + * the operator to look at the network. + * + * Reachability and capability are separate answers. Assert they stay separate. */ +static int degraded_health_get_handler(const char *url, const char *extra_headers, + char **response_buf, int timeout_ms) +{ + (void)timeout_ms; + (void)extra_headers; + assert(url); + g_get_seen++; + if (strstr(url, "/v1/version")) + { + if (response_buf) + *response_buf = strdup("{\"version\":\"v0.3.0-test\",\"service\":\"aimee-kb\"}"); + return 200; + } + assert(strcmp(url, "http://127.0.0.1:4010/v1/health") == 0); + if (response_buf) + *response_buf = strdup("{\"status\":\"degraded\",\"db2_ok\":true," + "\"db2_kb_tables_ok\":true,\"pgvec_ok\":true," + "\"pgvec_collection_ok\":true,\"pgvec_vectors\":0," + "\"embed_ok\":false,\"embed_command\":\"\"," + "\"chunk_count\":0,\"embedding_count\":0," + "\"warnings\":[]," + "\"blockers\":[\"no embedder configured: set embedder_model\"," + "\"embedder width mismatch: 3 vector(s) refused\"]}"); + return 200; +} + +static void test_health_degraded_is_reachable_and_carries_blockers(void) +{ + g_get_seen = 0; + mock_agent_http_reset(); + mock_agent_http_set_get_handler(degraded_health_get_handler); + assert(setenv("AIMEE_KB_API_URL", "http://127.0.0.1:4010/", 1) == 0); + assert(runtime_secret_store("AIMEE_KB_API_BEARER_TOKEN", "test-token") == 0); + + kb_health_t health; + /* Not -1: the kb answered. */ + assert(kb_client_health(&health) == 0); + assert(health.process_ok == 1); + assert(strcmp(health.status, "degraded") == 0); + /* Every blocker survives the boundary, newline-joined and in order. */ + assert(strstr(health.blockers, "no embedder configured") != NULL); + assert(strstr(health.blockers, "width mismatch") != NULL); + assert(strchr(health.blockers, '\n') != NULL); + /* The siblings still parse — a degraded response is a full response. */ + assert(health.embed_ok == 0); + assert(health.db2_ok == 1); + + unsetenv("AIMEE_KB_API_URL"); + runtime_secret_remove("AIMEE_KB_API_BEARER_TOKEN"); + mock_agent_http_reset(); +} + +/* An older kb sends no `status` field shape we recognise beyond the string, and + * no blockers at all. It must still read as reachable with an empty verdict — + * callers distinguish "said ok" from "said nothing" and must not read the latter + * as the former. */ +static int legacy_health_get_handler(const char *url, const char *extra_headers, + char **response_buf, int timeout_ms) +{ + (void)timeout_ms; + (void)extra_headers; + assert(url); + g_get_seen++; + if (strstr(url, "/v1/version")) + return 404; + if (response_buf) + *response_buf = strdup("{\"status\":\"ok\",\"db2_ok\":true,\"pgvec_ok\":true}"); + return 200; +} + +static void test_health_legacy_kb_without_blockers(void) +{ + g_get_seen = 0; + mock_agent_http_reset(); + mock_agent_http_set_get_handler(legacy_health_get_handler); + assert(setenv("AIMEE_KB_API_URL", "http://127.0.0.1:4010/", 1) == 0); + assert(runtime_secret_store("AIMEE_KB_API_BEARER_TOKEN", "test-token") == 0); + + kb_health_t health; + assert(kb_client_health(&health) == 0); + assert(health.process_ok == 1); + assert(strcmp(health.status, "ok") == 0); + assert(health.blockers[0] == '\0'); + + unsetenv("AIMEE_KB_API_URL"); + runtime_secret_remove("AIMEE_KB_API_BEARER_TOKEN"); + mock_agent_http_reset(); +} + static void test_status_uses_v1_api_when_configured(void) { g_get_seen = 0; @@ -1294,6 +1397,8 @@ static void test_index_scan_uses_v1_api_when_configured(void) int main(void) { test_health_uses_v1_api_when_configured(); + test_health_degraded_is_reachable_and_carries_blockers(); + test_health_legacy_kb_without_blockers(); test_status_uses_v1_api_when_configured(); test_search_uses_v1_api_when_configured(); test_search_v1_reports_http_status(); diff --git a/src/tests/test_server_dispatch.c b/src/tests/test_server_dispatch.c index a855b6f63..3c25bcd55 100644 --- a/src/tests/test_server_dispatch.c +++ b/src/tests/test_server_dispatch.c @@ -8,7 +8,8 @@ #include "../db1/db.h" #include "../db1/eval.h" #include "../db1/server_sessions.h" -#include "kb_client.h" /* kb_health_t for the stub below */ +#include "kb_client.h" /* kb_health_t for the stub below */ +#include "server_internal.h" /* server_health_add_kb, for the kb verdict tests */ #include "agent_config.h" #include "config_fields.h" /* config_field_lookup / _set_value for the config_set stub */ #include "agent_eval.h" @@ -697,23 +698,38 @@ int handle_curator_invalidated(server_ctx_t *ctx, server_conn_t *conn, cJSON *re } /* server_api_status.c (linked here for handle_api_*) probes the kb from * server.health. This test exercises the dispatch table, not the kb, so answer - * "unreachable" without linking the whole kb client. */ + * "unreachable" by default without linking the whole kb client. + * + * The health-verdict tests below drive it instead: set g_kb_health_rc and the + * fields they care about, so server_health_add_kb's aggregation can be exercised + * without a kb. */ +static int g_kb_health_rc = -1; +static kb_health_t g_kb_health; + int kb_client_health(kb_health_t *out) { if (out) - memset(out, 0, sizeof(*out)); - return -1; + *out = g_kb_health; + return g_kb_health_rc; +} + +static void kb_health_stub_reset(void) +{ + memset(&g_kb_health, 0, sizeof(g_kb_health)); + g_kb_health_rc = -1; } /* server.health also reports the kb transport breaker, so an operator can see * that calls are being refused locally while the kb itself looks fine. Same * reasoning as above: report a closed breaker without linking the kb client. */ +static const char *g_kb_breaker_state = "closed"; + void kb_client_dependency_health(kb_client_dependency_health_t *out) { if (!out) return; memset(out, 0, sizeof(*out)); - snprintf(out->state, sizeof(out->state), "closed"); + snprintf(out->state, sizeof(out->state), "%s", g_kb_breaker_state); } int handle_kb_build(server_ctx_t *ctx, server_conn_t *conn, cJSON *req) @@ -2211,8 +2227,94 @@ static void test_session_brief_assemble(void) printf("test_session_brief_assemble: PASS\n"); } +/* The kb block in server.health is where "accepted, reports healthy, cannot work" + * became visible to users: `status` was `reachable ? "ok" : "unreachable"`, and + * every capability the kb reported sat beside it as a sibling that nothing read. + * `aimee status` printed "aimee-kb: ok" one line above "embedder: not + * configured" and both were true. + * + * These pin the three states apart, because the failure mode is not that any one + * of them is wrong — it is that two of them used to collapse into one. */ +static const char *kb_status_of(cJSON *resp) +{ + cJSON *kb = cJSON_GetObjectItemCaseSensitive(resp, "kb"); + cJSON *s = kb ? cJSON_GetObjectItemCaseSensitive(kb, "status") : NULL; + return cJSON_IsString(s) ? s->valuestring : NULL; +} + +static void test_health_kb_verdict_states(void) +{ + /* 1. Nothing answered — the only case that may say unreachable. */ + kb_health_stub_reset(); + cJSON *resp = cJSON_CreateObject(); + server_health_add_kb(resp); + assert(strcmp(kb_status_of(resp), "unreachable") == 0); + cJSON_Delete(resp); + + /* 2. Answered and capable. */ + kb_health_stub_reset(); + g_kb_health_rc = 0; + g_kb_health.process_ok = 1; + snprintf(g_kb_health.status, sizeof(g_kb_health.status), "ok"); + resp = cJSON_CreateObject(); + server_health_add_kb(resp); + assert(strcmp(kb_status_of(resp), "ok") == 0); + cJSON_Delete(resp); + + /* 3. Answered and told us it cannot work. This is the case that used to + * report "ok"; it must be degraded, NOT unreachable — the kb is up, and + * sending an operator to debug the network would be a fresh wrong answer. */ + kb_health_stub_reset(); + g_kb_health_rc = 0; + g_kb_health.process_ok = 1; + snprintf(g_kb_health.status, sizeof(g_kb_health.status), "degraded"); + snprintf(g_kb_health.blockers, sizeof(g_kb_health.blockers), + "no embedder configured: set embedder_model\nvector table missing"); + resp = cJSON_CreateObject(); + server_health_add_kb(resp); + assert(strcmp(kb_status_of(resp), "degraded") == 0); + /* The reasons reach the client, split back into one string per blocker. */ + cJSON *kb = cJSON_GetObjectItemCaseSensitive(resp, "kb"); + cJSON *blockers = cJSON_GetObjectItemCaseSensitive(kb, "blockers"); + assert(cJSON_IsArray(blockers) && cJSON_GetArraySize(blockers) == 2); + assert(strcmp(cJSON_GetArrayItem(blockers, 0)->valuestring, + "no embedder configured: set embedder_model") == 0); + assert(strcmp(cJSON_GetArrayItem(blockers, 1)->valuestring, "vector table missing") == 0); + cJSON_Delete(resp); + + /* 4. An open transport breaker refuses every call locally, so a kb that + * considers itself perfectly healthy still cannot be queried. The breaker is + * part of the verdict rather than a flag beside it. */ + kb_health_stub_reset(); + g_kb_health_rc = 0; + g_kb_health.process_ok = 1; + snprintf(g_kb_health.status, sizeof(g_kb_health.status), "ok"); + g_kb_breaker_state = "open"; + resp = cJSON_CreateObject(); + server_health_add_kb(resp); + assert(strcmp(kb_status_of(resp), "degraded") == 0); + kb = cJSON_GetObjectItemCaseSensitive(resp, "kb"); + assert(cJSON_IsTrue(cJSON_GetObjectItemCaseSensitive(kb, "queries_suppressed"))); + cJSON_Delete(resp); + g_kb_breaker_state = "closed"; + + /* 5. An older kb sends no verdict. Absence is not a blocker, and must not be + * read as one — inventing "degraded" out of silence would make every + * pre-upgrade install look broken. */ + kb_health_stub_reset(); + g_kb_health_rc = 0; + g_kb_health.process_ok = 1; + resp = cJSON_CreateObject(); + server_health_add_kb(resp); + assert(strcmp(kb_status_of(resp), "ok") == 0); + cJSON_Delete(resp); + + kb_health_stub_reset(); +} + int main(void) { + test_health_kb_verdict_states(); test_invalid_json(); test_session_brief_assemble(); test_conn_update_events_null_evloop();