From 013e1f7ec7d4b981a1cd28bf35d5767c195f8447 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Tue, 8 Sep 2026 20:28:58 +0200 Subject: [PATCH 1/4] fix(cli): scope the activation guard to the install-target HOME (amendment) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rendezvous directory the CLI's activation guard talks to is per OS account (service.h), not per HOME/CBM_CACHE_DIR: every daemon owned by one user meets at the same endpoint, and only the cache fingerprint carried in the cohort identity separates one HOME/CBM_CACHE_DIR namespace from another. `cli_activation_production_reserve` used `cbm_version_cohort_reserve_for_mutation`, which treats any lifetime- lock holder at that shared endpoint as the daemon this activation must drain, with no cache-fingerprint awareness at all. An `install` run against a second HOME (a sandbox, a second profile, `--skip-binary` into an alternate CBM_CACHE_DIR) therefore reached the LIVE host daemon at the shared endpoint and drained it — disconnecting every one of the host's real MCP clients — even though nothing the sandboxed install touched belonged to the host's cache namespace. The fix adds `cli_activation_resolve_scope`, a non-blocking probe via the already cache-fingerprint-aware `cbm_version_cohort_acquire`, run before the blind drain path. It distinguishes three cases from the resolved target of THIS activation: - nothing to quiesce at all: a `--skip-binary` install that also resets no index publishes nothing, so no session anywhere needs to stop (`quiesce_required` on `cli_activation_guard_scoped`, plumbed from `cbm_cmd_install`'s own binary/index-reset decision); - the active cohort's cache fingerprint doesn't match this activation's target cache: a foreign namespace, left untouched; - same cache fingerprint (or no active cohort): this activation really does replace that daemon, so the existing drain/quiesce path runs unchanged, including the barrier wait when another activation already holds maintenance. Verified against a real forked daemon (a live runtime service with one committed client) standing in for the host: an install into a foreign HOME/CBM_CACHE_DIR (`--skip-binary` and a full binary install) leaves that daemon serving with its client still attached, while an install that targets the host's own namespace still drains it and the activation audit still names the client it disconnected. New/updated tests in tests/test_cli.c (cli suite): - cli_install_skip_binary_into_foreign_home_never_drains_host_cohort - cli_install_binary_into_foreign_home_never_drains_host_cohort - cli_install_into_host_namespace_still_drains_host_cohort (regression) - cli_install_skip_binary_unchanged_in_host_namespace_quiesces_nothing - cli_activation_quiesce_does_not_wait_on_bootstrap_startup updated to carry a real cache fingerprint for its participant, now that scope is fingerprint-gated src/mcp/index_supervisor.{c,h}: CBM_CLI_ENABLE_TEST_API-gated setter so the fixture's forked daemon can carry the same build fingerprint the guard captures from the supervisor. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- src/cli/cli.c | 163 +++++++++++++-- src/mcp/index_supervisor.c | 10 + src/mcp/index_supervisor.h | 10 + tests/test_cli.c | 407 ++++++++++++++++++++++++++++++++++++- 4 files changed, 576 insertions(+), 14 deletions(-) diff --git a/src/cli/cli.c b/src/cli/cli.c index d85ebf0b6..6cf9ddca8 100644 --- a/src/cli/cli.c +++ b/src/cli/cli.c @@ -214,6 +214,12 @@ typedef struct { bool cleanup_ok; bool original_cache_environment_present; bool cache_environment_overridden; + /* Scope: what THIS activation replaces decides whether any session must + * be quiesced at all, and the target cache namespace decides WHICH + * cohort. A skipped coordination holds no lock and drains nobody. */ + bool quiesce_required; + bool coordination_skipped; + char scope_detail[CBM_SZ_1K]; } cli_activation_production_context_t; static cbm_cli_activation_ops_t g_cli_activation_test_ops; @@ -528,6 +534,91 @@ static void cli_activation_release_cleanup_lease(cli_activation_production_conte } } +typedef enum { + CLI_ACTIVATION_SCOPE_ACTIVE = 0, + CLI_ACTIVATION_SCOPE_NOTHING_TO_REPLACE, + CLI_ACTIVATION_SCOPE_FOREIGN_COHORT, + CLI_ACTIVATION_SCOPE_ERROR, +} cli_activation_scope_t; + +static void cli_activation_log_guard_decision(const cli_activation_production_context_t *context, + const char *decision, const char *active_cache) { + char clients[32]; + (void)snprintf(clients, sizeof(clients), "%llu", + (unsigned long long)context->daemon_result.active_clients); + const char *scope = cbm_daemon_ipc_endpoint_runtime_dir(context->endpoint); + cbm_log_info("activation.guard", "scope", scope ? scope : "", "cache", + context->cache_fingerprint, "clients", clients, "decision", decision, + "active_cache", active_cache ? active_cache : ""); +} + +/* Whose sessions does this activation have to stop? The rendezvous directory + * is per OS account (service.h), so the host daemon of another HOME / + * CBM_CACHE_DIR meets this activation at the very same endpoint; only the + * cache fingerprint in the cohort identity separates the namespaces. An + * `install` into a sandbox HOME used to drain the host daemon and every MCP + * client behind it although nothing it touched belonged to the host. + * + * Two questions, answered from the activation's own target: does it replace + * anything at all (a --skip-binary install without an index reset publishes + * nothing), and whose cohort is active. Admission with an immediate deadline + * is the cheapest authoritative read of the active lifetime record: OK means + * the cohort is ours or empty, CONFLICT names the active identity (its cache + * fingerprint is filled for every conflict kind), BUSY means another + * activation holds maintenance and the barrier waits for it as before. */ +static cli_activation_scope_t cli_activation_resolve_scope( + cli_activation_production_context_t *context) { + const char *runtime_dir = cbm_daemon_ipc_endpoint_runtime_dir(context->endpoint); + const char *scope = runtime_dir ? runtime_dir : ""; + const char *action = cli_activation_action_text(context->action); + if (!context->quiesce_required) { + (void)snprintf(context->scope_detail, sizeof(context->scope_detail), + "nothing to quiesce: published binary and indexes unchanged; scope=%s", + scope); + cli_activation_log_guard_decision(context, "nothing_to_replace", NULL); + printf("No CBM session needs to stop for %s: the published binary and indexes are " + "unchanged.\n", + action); + (void)fflush(stdout); + return CLI_ACTIVATION_SCOPE_NOTHING_TO_REPLACE; + } + cbm_version_cohort_lease_t *lease = NULL; + cbm_daemon_conflict_t conflict; + memset(&conflict, 0, sizeof(conflict)); + cbm_version_cohort_status_t status = cbm_version_cohort_acquire( + context->cohort_manager, &context->identity, cbm_now_ms(), &lease, &conflict); + cli_activation_release_cleanup_lease(context, &lease); + if (lease) { + return CLI_ACTIVATION_SCOPE_ERROR; + } + switch (status) { + case CBM_VERSION_COHORT_OK: + case CBM_VERSION_COHORT_BUSY: + return CLI_ACTIVATION_SCOPE_ACTIVE; + case CBM_VERSION_COHORT_CONFLICT: + break; + default: + return CLI_ACTIVATION_SCOPE_ERROR; + } + if (!conflict.active_cache_fingerprint[0] || + strcmp(conflict.active_cache_fingerprint, context->cache_fingerprint) == 0) { + /* Same cache namespace, another build or version: that IS the daemon + * this activation replaces. An unreadable active cache stays in scope + * rather than silently exempting a same-namespace daemon. */ + return CLI_ACTIVATION_SCOPE_ACTIVE; + } + (void)snprintf(context->scope_detail, sizeof(context->scope_detail), + "active cohort serves another cache namespace (%.12s), this %s targets " + "%.12s; scope=%s; nothing stopped", + conflict.active_cache_fingerprint, action, context->cache_fingerprint, scope); + cli_activation_log_guard_decision(context, "foreign_cohort", conflict.active_cache_fingerprint); + printf("Leaving active CBM sessions untouched: they serve another cache namespace than " + "this %s targets.\n", + action); + (void)fflush(stdout); + return CLI_ACTIVATION_SCOPE_FOREIGN_COHORT; +} + static int cli_activation_production_reserve(void *opaque, cbm_cli_activation_lock_t *lease_out) { cli_activation_production_context_t *context = opaque; if (lease_out) { @@ -536,6 +627,26 @@ static int cli_activation_production_reserve(void *opaque, cbm_cli_activation_lo if (!context || !context->cohort_manager || !lease_out) { return CLI_ERR; } + cli_activation_scope_t scope = cli_activation_resolve_scope(context); + if (scope == CLI_ACTIVATION_SCOPE_ERROR) { + return CLI_ERR; + } + if (scope != CLI_ACTIVATION_SCOPE_ACTIVE) { + /* Nothing in the target namespace is being replaced, or the only + * active cohort serves another namespace: hold no maintenance, + * admission, lifetime or startup lock (each wakes or blocks the other + * namespace's sessions) and send no drain request. */ + if (!cli_activation_log_event(context, "quiesce_skipped", context->scope_detail)) { + return CLI_ERR; + } + context->coordination_skipped = true; + context->mutation_authorized = true; + *lease_out = context; + return 1; + } + printf("Stopping active CBM sessions and operations for %s...\n", + cli_activation_action_text(context->action)); + (void)fflush(stdout); cbm_version_cohort_quiesce_result_t quiesce = CBM_VERSION_COHORT_QUIESCE_NOT_NEEDED; cbm_version_cohort_lease_t *lease = NULL; context->control_deadline_ms = cli_activation_deadline_after(CLI_ACTIVATION_CONTROL_TIMEOUT_MS); @@ -582,6 +693,8 @@ static int cli_activation_production_reserve(void *opaque, cbm_cli_activation_lo } context->cohort_lease = lease; + cli_activation_log_guard_decision( + context, context->shutdown_requested ? "cohort_drained" : "no_active_cohort", NULL); if (!cli_activation_log_event(context, "daemon_stopped", context->shutdown_requested ? "cohort drained" : "no active cohort")) { @@ -600,6 +713,13 @@ static void cli_activation_production_release(void *opaque, cbm_cli_activation_l if (!context) { return; } + if (context->coordination_skipped) { + /* Nothing was held: the token is the context itself. */ + if (lease != (cbm_cli_activation_lock_t)context) { + context->cleanup_ok = false; + } + return; + } /* Global release order is the inverse of acquisition: startup first, * then lifetime/admission/maintenance through the cohort lease. */ if (context->startup_lock) { @@ -626,9 +746,11 @@ static void cli_activation_production_diagnostic(void *opaque, const char *messa static bool cli_activation_production_context_init(cli_activation_production_context_t *context, cbm_daemon_runtime_activation_action_t action, const char *target_version, - const char *target_build) { + const char *target_build, + bool quiesce_required) { memset(context, 0, sizeof(*context)); context->action = action; + context->quiesce_required = quiesce_required; context->target_version = target_version; context->target_build = target_build; context->cleanup_ok = true; @@ -735,23 +857,26 @@ static void cli_activation_production_context_close(cli_activation_production_co context->original_cache_environment = NULL; } -static int cli_activation_guard(cbm_daemon_runtime_activation_action_t action, - const char *target_version, const char *target_build, - cbm_cli_activation_mutation_fn mutation, void *mutation_context) { +/* quiesce_required: false when the activation publishes no binary and resets + * no index (a config-only install) — nothing running is then replaced, and + * no session is stopped for it. */ +static int cli_activation_guard_scoped(cbm_daemon_runtime_activation_action_t action, + const char *target_version, const char *target_build, + bool quiesce_required, + cbm_cli_activation_mutation_fn mutation, + void *mutation_context) { if (g_cli_activation_test_ops_set) { return cbm_cli_activation_guard_with_ops(&g_cli_activation_test_ops, mutation, mutation_context); } cli_activation_production_context_t context; - if (!cli_activation_production_context_init(&context, action, target_version, target_build)) { + if (!cli_activation_production_context_init(&context, action, target_version, target_build, + quiesce_required)) { cli_activation_production_context_close(&context); cli_activation_production_diagnostic(NULL, CLI_ACTIVATION_REFUSED_MESSAGE); return CLI_TRUE; } - printf("Stopping active CBM sessions and operations for %s...\n", - cli_activation_action_text(action)); - (void)fflush(stdout); if (!cli_activation_log_event(&context, "requested", NULL)) { cli_activation_production_context_close(&context); (void)fprintf(stderr, "error: activation request could not be recorded safely; " @@ -793,6 +918,13 @@ static int cli_activation_guard(cbm_daemon_runtime_activation_action_t action, return rc; } +static int cli_activation_guard(cbm_daemon_runtime_activation_action_t action, + const char *target_version, const char *target_build, + cbm_cli_activation_mutation_fn mutation, void *mutation_context) { + return cli_activation_guard_scoped(action, target_version, target_build, true, mutation, + mutation_context); +} + /* Tar header field offsets */ #define TAR_NAME_LEN 101 /* filename field: bytes 0-99 + NUL */ #define TAR_SIZE_OFFSET 124 /* octal size field offset */ @@ -10756,11 +10888,16 @@ int cbm_cmd_install(int argc, char **argv) { .force = force, .dry_run = dry_run, }; - int activation_rc = - dry_run ? cli_install_activate(&activation) - : cli_activation_guard(CBM_DAEMON_RUNTIME_ACTIVATION_INSTALL, CBM_VERSION, - has_binary_validator ? binary_validator.fingerprint : NULL, - cli_install_activate, &activation); + /* What this install replaces decides whether any session must stop: with + * the published binary untouched (--skip-binary, or an externally managed + * binary) and no index reset, agent configs are refreshed while every + * session stays up. */ + bool quiesce_required = has_binary_validator || delete_indexes; + int activation_rc = dry_run ? cli_install_activate(&activation) + : cli_activation_guard_scoped( + CBM_DAEMON_RUNTIME_ACTIVATION_INSTALL, CBM_VERSION, + has_binary_validator ? binary_validator.fingerprint : NULL, + quiesce_required, cli_install_activate, &activation); if (activation.binary_transaction) { (void)cli_activation_transaction_abort(&activation.binary_transaction); } diff --git a/src/mcp/index_supervisor.c b/src/mcp/index_supervisor.c index eed78f418..a035e3d6d 100644 --- a/src/mcp/index_supervisor.c +++ b/src/mcp/index_supervisor.c @@ -149,6 +149,16 @@ const char *cbm_index_supervisor_build_fingerprint(void) { return g_build_fingerprint[0] ? g_build_fingerprint : NULL; } +#if defined(CBM_CLI_ENABLE_TEST_API) +void cbm_index_supervisor_set_build_fingerprint_for_test(const char *fingerprint) { + if (!fingerprint || !worker_fingerprint_valid(fingerprint)) { + return; + } + g_build_fingerprint_capture_attempted = true; + (void)snprintf(g_build_fingerprint, sizeof(g_build_fingerprint), "%s", fingerprint); +} +#endif + static bool worker_fingerprint_valid(const char *fingerprint) { if (!fingerprint || strlen(fingerprint) != CBM_INDEX_WORKER_BUILD_FINGERPRINT_LENGTH) { return false; diff --git a/src/mcp/index_supervisor.h b/src/mcp/index_supervisor.h index 31259df90..cbc38f696 100644 --- a/src/mcp/index_supervisor.h +++ b/src/mcp/index_supervisor.h @@ -76,6 +76,16 @@ void cbm_index_worker_log_begin(const char *args_json, const char *repo_path); bool cbm_index_supervisor_capture_build_fingerprint(void); const char *cbm_index_supervisor_build_fingerprint(void); +#if defined(CBM_CLI_ENABLE_TEST_API) +/* Test-only: the runner captures a synthetic stub (see the capture seam) so + * spawned workers start instantly, but a daemon runtime service can only carry + * the REAL image hash. A fixture that runs a real daemon and drives the + * production activation guard against it aligns the captured value with that + * hash for its duration and restores the previous value afterwards. Invalid + * input is ignored. Never compiled into production. */ +void cbm_index_supervisor_set_build_fingerprint_for_test(const char *fingerprint); +#endif + typedef struct { const char *expected_build_fingerprint; const char *args_json; diff --git a/tests/test_cli.c b/tests/test_cli.c index 976280868..3a4b4a851 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -23,7 +23,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -39,6 +41,7 @@ #include #include #include +#include #endif #ifdef __APPLE__ #include @@ -47,6 +50,12 @@ #include #include +/* Same guarded fallback every product TU carries; CI injects the real value + * through CFLAGS_EXTRA for product objects and test objects alike. */ +#ifndef CBM_VERSION +#define CBM_VERSION "dev" +#endif + /* Internal prompt seam used to restore process-global state after command * tests that exercise --yes. */ void cbm_set_auto_answer_for_test(int value); @@ -1216,6 +1225,23 @@ TEST(cli_activation_quiesce_does_not_wait_on_bootstrap_startup) { test_rmdir_r(tmpdir); FAIL("runtime parent setup failed"); } + /* The participant models the daemon of THIS namespace: its cohort + * identity carries the fingerprint of the cache the install below + * targets, derived as main.c derives it. A foreign cache would (rightly) + * be left alone by the scoped guard. */ + char participant_cache[512]; + char participant_cache_canonical[1024]; + char participant_cache_fingerprint[CBM_SHA256_HEX_LEN + 1] = {0}; + snprintf(participant_cache, sizeof(participant_cache), "%s/cache", tmpdir); + if (!cbm_mkdir_p(participant_cache, 0700) || + !cbm_canonical_path(participant_cache, participant_cache_canonical, + sizeof(participant_cache_canonical))) { + test_rmdir_r(tmpdir); + FAIL("participant cache setup failed"); + } + cbm_normalize_path_sep(participant_cache_canonical); + cbm_sha256_hex(participant_cache_canonical, strlen(participant_cache_canonical), + participant_cache_fingerprint); int ready_pipe[2] = {-1, -1}; if (pipe(ready_pipe) != 0) { test_rmdir_r(tmpdir); @@ -1231,7 +1257,7 @@ TEST(cli_activation_quiesce_does_not_wait_on_bootstrap_startup) { cbm_daemon_build_identity_t identity = { .semantic_version = "cli-activation-test", .build_fingerprint = fingerprint, - .cache_fingerprint = "cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc", + .cache_fingerprint = participant_cache_fingerprint, .protocol_abi = CBM_DAEMON_RUNTIME_WIRE_ABI, .store_abi = 1, .feature_abi = 1, @@ -1467,6 +1493,381 @@ TEST(cli_install_recovers_markerless_stale_rendezvous) { ASSERT_TRUE(anchor_removed); PASS(); } +/* ── Activation-guard namespace scope (2026-09-08) ────────────────── + * The rendezvous directory is per OS ACCOUNT, never per HOME (service.h): + * every daemon of one user meets at one endpoint, and the cohort identity's + * cache fingerprint is what separates one HOME / CBM_CACHE_DIR namespace + * from another. An `install` into a second HOME (a sandbox, a second + * profile) therefore reaches the LIVE host daemon at the shared endpoint — + * and used to drain it, disconnecting every MCP client, although nothing + * that install touched belonged to the host namespace. + * + * The fixture models the host the way host.c builds it: a forked process + * that admits itself to the cohort with the host cache fingerprint, runs a + * real runtime service at the shared endpoint, and exits (releasing the + * cohort lease LAST, the real teardown order) once that service has been + * drained. The parent keeps one committed client connected to it across the + * install under test and asks the daemon itself afterwards. */ +#define CLI_SCOPE_HOST_DRAINED 3 +#define CLI_SCOPE_TIMEOUT_MS 5000U + +typedef struct { + char tmpdir[256]; + char runtime_parent[512]; + char host_home[512]; + char host_cache[512]; + char host_cache_fingerprint[CBM_SHA256_HEX_LEN + 1]; + char self_build[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; + char previous_supervisor_build[CBM_DAEMON_BUILD_FINGERPRINT_SIZE]; + char conflict_log[640]; + cbm_daemon_build_identity_t identity; + cbm_daemon_ipc_endpoint_t *endpoint; + cbm_daemon_runtime_client_t *client; + pid_t host; + int release_fd; + char *old_home; + char *old_cache; + char *old_shell; +} cli_scope_fixture_t; + +static _Noreturn void cli_scope_host_child(const cli_scope_fixture_t *fixture, int ready_fd, + int release_fd) { + cbm_daemon_ipc_endpoint_t *endpoint = + cbm_daemon_bootstrap_endpoint_new(fixture->runtime_parent); + cbm_version_cohort_manager_t *manager = + endpoint ? cbm_version_cohort_manager_new(endpoint) : NULL; + cbm_version_cohort_lease_t *lease = NULL; + cbm_daemon_conflict_t conflict; + cbm_daemon_runtime_service_t *service = NULL; + bool admitted = endpoint && manager && + cbm_version_cohort_acquire(manager, &fixture->identity, cbm_now_ms() + 45000U, + &lease, &conflict) == CBM_VERSION_COHORT_OK; + if (admitted) { + cbm_daemon_runtime_service_config_t config = { + .endpoint = endpoint, + .identity = fixture->identity, + .conflict_log_path = fixture->conflict_log, + .conflict_log_cap_bytes = 64U * 1024U, + .max_clients = 8, + .lease_timeout_ms = CLI_SCOPE_TIMEOUT_MS, + .request_timeout_ms = CLI_SCOPE_TIMEOUT_MS, + .shutdown_timeout_ms = CLI_SCOPE_TIMEOUT_MS, + /* Born permanent: only a drain/stop ends it, never the parent's + * client leaving, so "still alive" is a statement about the + * drain alone. */ + .permanent = true, + }; + service = cbm_daemon_runtime_service_start(&config); + } + bool ready_ok = + service && cbm_daemon_runtime_service_state(service) == CBM_DAEMON_RUNTIME_SERVICE_RUNNING; + char ready = ready_ok ? 'R' : 'E'; + (void)write(ready_fd, &ready, 1); + close(ready_fd); + bool drained = false; + uint64_t deadline = cbm_now_ms() + 120000U; + while (ready_ok && cbm_now_ms() < deadline) { + if (cbm_daemon_runtime_service_wait_exited(service, 50U)) { + drained = true; + break; + } + struct pollfd release_poll = {.fd = release_fd, .events = POLLIN, .revents = 0}; + if (poll(&release_poll, 1, 0) > 0) { + break; + } + } + uint64_t cleanup_deadline = cbm_now_ms() + 2U * CLI_SCOPE_TIMEOUT_MS; + if (service) { + if (!drained) { + (void)cbm_daemon_runtime_service_stop(service, CLI_SCOPE_TIMEOUT_MS); + } + while (!cbm_daemon_runtime_service_free(service) && cbm_now_ms() < cleanup_deadline) { + cbm_usleep(1000); + } + } + while (lease && cbm_version_cohort_lease_release(&lease) != CBM_PRIVATE_FILE_LOCK_OK && + cbm_now_ms() < cleanup_deadline) { + cbm_usleep(1000); + } + while (manager && cbm_version_cohort_manager_free(&manager) != CBM_PRIVATE_FILE_LOCK_OK && + cbm_now_ms() < cleanup_deadline) { + cbm_usleep(1000); + } + cbm_daemon_ipc_endpoint_free(endpoint); + close(release_fd); + _exit(!ready_ok ? 1 : drained ? CLI_SCOPE_HOST_DRAINED : 0); +} + +static bool cli_scope_fixture_start(cli_scope_fixture_t *fixture, const char *tag) { + memset(fixture, 0, sizeof(*fixture)); + fixture->host = -1; + fixture->release_fd = -1; + snprintf(fixture->tmpdir, sizeof(fixture->tmpdir), "/tmp/cli-guard-scope-%s-XXXXXX", tag); + if (!cbm_mkdtemp(fixture->tmpdir)) { + return false; + } + snprintf(fixture->runtime_parent, sizeof(fixture->runtime_parent), "%s/runtime", + fixture->tmpdir); + snprintf(fixture->host_home, sizeof(fixture->host_home), "%s/host", fixture->tmpdir); + snprintf(fixture->host_cache, sizeof(fixture->host_cache), "%s/cache", fixture->host_home); + snprintf(fixture->conflict_log, sizeof(fixture->conflict_log), "%s/conflicts.ndjson", + fixture->host_home); + /* The host identity's cache fingerprint is derived exactly as main.c does + * for a real daemon: resolved dir -> canonical path -> SHA-256. */ + char canonical_cache[1024]; + if (test_mkdirp(fixture->runtime_parent) != 0 || !cbm_mkdir_p(fixture->host_cache, 0700) || + !cbm_canonical_path(fixture->host_cache, canonical_cache, sizeof(canonical_cache))) { + return false; + } + cbm_normalize_path_sep(canonical_cache); + cbm_sha256_hex(canonical_cache, strlen(canonical_cache), fixture->host_cache_fingerprint); + if (!cbm_daemon_runtime_process_build_fingerprint((uint64_t)getpid(), fixture->self_build)) { + return false; + } + /* The guard claims the supervisor's captured build; the runner stubs that + * capture while a runtime service must carry the real image hash. Align + * the two for this fixture exactly as one production process has them. */ + const char *previous_supervisor_build = cbm_index_supervisor_build_fingerprint(); + snprintf(fixture->previous_supervisor_build, sizeof(fixture->previous_supervisor_build), "%s", + previous_supervisor_build ? previous_supervisor_build : ""); + cbm_index_supervisor_set_build_fingerprint_for_test(fixture->self_build); + fixture->identity = (cbm_daemon_build_identity_t){ + .semantic_version = CBM_VERSION, + .build_fingerprint = fixture->self_build, + .cache_fingerprint = fixture->host_cache_fingerprint, + .protocol_abi = CBM_DAEMON_RUNTIME_WIRE_ABI, + .store_abi = 1, + .feature_abi = 1, + }; + int ready_pipe[2] = {-1, -1}; + int release_pipe[2] = {-1, -1}; + if (pipe(ready_pipe) != 0) { + return false; + } + if (pipe(release_pipe) != 0) { + close(ready_pipe[0]); + close(ready_pipe[1]); + return false; + } + pid_t child = fork(); + if (child == 0) { + close(ready_pipe[0]); + close(release_pipe[1]); + cli_scope_host_child(fixture, ready_pipe[1], release_pipe[0]); + } + close(ready_pipe[1]); + close(release_pipe[0]); + char ready = 0; + bool host_ready = child > 0 && read(ready_pipe[0], &ready, 1) == 1 && ready == 'R'; + close(ready_pipe[0]); + fixture->host = child; + fixture->release_fd = release_pipe[1]; + cli_activation_save_env(&fixture->old_home, &fixture->old_cache); + const char *shell = getenv("SHELL"); + fixture->old_shell = shell ? strdup(shell) : NULL; + cbm_setenv("SHELL", "/bin/zsh", 1); + if (!host_ready) { + return false; + } + fixture->endpoint = cbm_daemon_bootstrap_endpoint_new(fixture->runtime_parent); + cbm_daemon_runtime_connect_result_t connect_result = {0}; + fixture->client = fixture->endpoint + ? cbm_daemon_runtime_client_connect(fixture->endpoint, &fixture->identity, + CLI_SCOPE_TIMEOUT_MS, &connect_result) + : NULL; + return fixture->client != NULL; +} + +/* Ask the host daemon itself: still running, not stopping, and the parent's + * committed client still admitted. */ +static bool cli_scope_host_serving(const cli_scope_fixture_t *fixture) { + cbm_daemon_runtime_status_t status = {0}; + return fixture->endpoint && + cbm_daemon_runtime_request_status(fixture->endpoint, &fixture->identity, + CLI_SCOPE_TIMEOUT_MS, &status) && + !status.stopping && status.committed_clients == 1; +} + +static int cli_scope_install(cli_scope_fixture_t *fixture, const char *home, const char *cache, + const char *bin_dir, bool skip_binary) { + cbm_setenv("HOME", home, 1); + cbm_setenv("CBM_CACHE_DIR", cache, 1); + cbm_cli_set_activation_runtime_parent_for_test(fixture->runtime_parent); + char dir_arg[704]; + snprintf(dir_arg, sizeof(dir_arg), "--dir=%s", bin_dir); + char *install_argv[] = {skip_binary ? "--skip-binary" : "--force", "--skip-config", "--yes", + dir_arg}; + int rc = cli_test_cmd_install(4, install_argv); + cbm_cli_set_activation_runtime_parent_for_test(g_cli_suite_runtime_parent); + cbm_set_auto_answer_for_test(0); + return rc; +} + +/* Returns the host child's exit status: 0 released intact, + * CLI_SCOPE_HOST_DRAINED when an activation drained it, -1 unknown. */ +static int cli_scope_fixture_finish(cli_scope_fixture_t *fixture) { + if (fixture->client) { + (void)cbm_daemon_runtime_client_close(fixture->client, CLI_SCOPE_TIMEOUT_MS); + fixture->client = NULL; + } + if (fixture->release_fd >= 0) { + /* Closing the write end is the signal (the child's poll sees POLLHUP); + * a write would raise SIGPIPE once a drained child is already gone. */ + close(fixture->release_fd); + fixture->release_fd = -1; + } + int host_exit = -1; + if (fixture->host > 0) { + int status = 0; + if (waitpid(fixture->host, &status, 0) == fixture->host && WIFEXITED(status)) { + host_exit = WEXITSTATUS(status); + } + fixture->host = -1; + } + cbm_daemon_ipc_endpoint_free(fixture->endpoint); + fixture->endpoint = NULL; + if (fixture->previous_supervisor_build[0]) { + cbm_index_supervisor_set_build_fingerprint_for_test(fixture->previous_supervisor_build); + } + if (fixture->old_shell) { + cbm_setenv("SHELL", fixture->old_shell, 1); + } else { + cbm_unsetenv("SHELL"); + } + free(fixture->old_shell); + fixture->old_shell = NULL; + cli_activation_restore_env(fixture->old_home, fixture->old_cache); + fixture->old_home = NULL; + fixture->old_cache = NULL; + test_rmdir_r(fixture->tmpdir); + return host_exit; +} + +static void cli_scope_foreign_paths(const cli_scope_fixture_t *fixture, char home[512], + char cache[576], char bin_dir[640], char activation_log[704]) { + snprintf(home, 512, "%s/sandbox", fixture->tmpdir); + snprintf(cache, 576, "%s/cache", home); + snprintf(bin_dir, 640, "%s/custom/bin", home); + snprintf(activation_log, 704, "%s/logs/activation-events.ndjson", cache); +} + +/* (a) `HOME= install --skip-binary` — the observed incident shape: + * the sandbox shares the account rendezvous, its cache namespace differs. */ +TEST(cli_install_skip_binary_into_foreign_home_never_drains_host_cohort) { + cli_scope_fixture_t fixture; + bool ready = cli_scope_fixture_start(&fixture, "skipbin"); + char foreign_home[512]; + char foreign_cache[576]; + char foreign_bin[640]; + char activation_log[704]; + cli_scope_foreign_paths(&fixture, foreign_home, foreign_cache, foreign_bin, activation_log); + bool prepared = ready && cbm_mkdir_p(foreign_home, 0700); + int install_rc = + prepared ? cli_scope_install(&fixture, foreign_home, foreign_cache, foreign_bin, true) : -1; + bool host_serving = prepared && cli_scope_host_serving(&fixture); + const char *events = read_test_file(activation_log); + bool completed = events && strstr(events, "\"phase\":\"completed\"") != NULL; + bool nothing_drained = events && strstr(events, "cohort drained") == NULL; + int host_exit = cli_scope_fixture_finish(&fixture); + + ASSERT_TRUE(ready); + ASSERT_EQ(install_rc, 0); + ASSERT_TRUE(host_serving); + ASSERT_EQ(host_exit, 0); + ASSERT_TRUE(completed); + ASSERT_TRUE(nothing_drained); + PASS(); +} + +/* (b) A full install (binary published into the sandbox bin dir) is scoped + * the same way: the host namespace is not what is being replaced. */ +TEST(cli_install_binary_into_foreign_home_never_drains_host_cohort) { + cli_scope_fixture_t fixture; + bool ready = cli_scope_fixture_start(&fixture, "binary"); + char foreign_home[512]; + char foreign_cache[576]; + char foreign_bin[640]; + char activation_log[704]; + cli_scope_foreign_paths(&fixture, foreign_home, foreign_cache, foreign_bin, activation_log); + char target_path[704]; + snprintf(target_path, sizeof(target_path), "%s/codebase-memory-mcp", foreign_bin); + bool prepared = ready && cbm_mkdir_p(foreign_home, 0700); + int install_rc = + prepared ? cli_scope_install(&fixture, foreign_home, foreign_cache, foreign_bin, false) + : -1; + bool host_serving = prepared && cli_scope_host_serving(&fixture); + struct stat target_status; + bool target_exists = stat(target_path, &target_status) == 0; + const char *events = read_test_file(activation_log); + bool completed = events && strstr(events, "\"phase\":\"completed\"") != NULL; + bool nothing_drained = events && strstr(events, "cohort drained") == NULL; + int host_exit = cli_scope_fixture_finish(&fixture); + + ASSERT_TRUE(ready); + ASSERT_EQ(install_rc, 0); + ASSERT_TRUE(target_exists); + ASSERT_TRUE(host_serving); + ASSERT_EQ(host_exit, 0); + ASSERT_TRUE(completed); + ASSERT_TRUE(nothing_drained); + PASS(); +} + +/* (c) Regression: an install that replaces the binary inside the host's own + * namespace still drains that cohort — and the audit names its client. */ +TEST(cli_install_into_host_namespace_still_drains_host_cohort) { + cli_scope_fixture_t fixture; + bool ready = cli_scope_fixture_start(&fixture, "host"); + char host_bin[640]; + char activation_log[704]; + snprintf(host_bin, sizeof(host_bin), "%s/custom/bin", fixture.host_home); + snprintf(activation_log, sizeof(activation_log), "%s/logs/activation-events.ndjson", + fixture.host_cache); + int install_rc = + ready ? cli_scope_install(&fixture, fixture.host_home, fixture.host_cache, host_bin, false) + : -1; + bool host_serving = ready && cli_scope_host_serving(&fixture); + const char *events = read_test_file(activation_log); + bool drained_in_log = events && strstr(events, "cohort drained") != NULL && + strstr(events, "\"daemon_active_clients\":1") != NULL; + int host_exit = cli_scope_fixture_finish(&fixture); + + ASSERT_TRUE(ready); + ASSERT_EQ(install_rc, 0); + ASSERT_FALSE(host_serving); + ASSERT_EQ(host_exit, CLI_SCOPE_HOST_DRAINED); + ASSERT_TRUE(drained_in_log); + PASS(); +} + +/* (d) `--skip-binary` with nothing to publish (no binary at the target, no + * index reset) replaces nothing, so even the host's own namespace has + * nothing to quiesce: agent configs are refreshed, sessions stay up. */ +TEST(cli_install_skip_binary_unchanged_in_host_namespace_quiesces_nothing) { + cli_scope_fixture_t fixture; + bool ready = cli_scope_fixture_start(&fixture, "unchanged"); + char absent_bin[640]; + char activation_log[704]; + snprintf(absent_bin, sizeof(absent_bin), "%s/absent/bin", fixture.host_home); + snprintf(activation_log, sizeof(activation_log), "%s/logs/activation-events.ndjson", + fixture.host_cache); + int install_rc = + ready ? cli_scope_install(&fixture, fixture.host_home, fixture.host_cache, absent_bin, true) + : -1; + bool host_serving = ready && cli_scope_host_serving(&fixture); + const char *events = read_test_file(activation_log); + bool completed = events && strstr(events, "\"phase\":\"completed\"") != NULL; + bool nothing_drained = events && strstr(events, "cohort drained") == NULL; + int host_exit = cli_scope_fixture_finish(&fixture); + + ASSERT_TRUE(ready); + ASSERT_EQ(install_rc, 0); + ASSERT_TRUE(host_serving); + ASSERT_EQ(host_exit, 0); + ASSERT_TRUE(completed); + ASSERT_TRUE(nothing_drained); + PASS(); +} #endif TEST(cli_install_force_quiesces_active_cohort_before_replacing_binary) { @@ -14774,6 +15175,10 @@ SUITE(cli) { RUN_TEST(cli_activation_cleanup_failure_fail_stops_before_lease_release); RUN_TEST(cli_activation_quiesce_does_not_wait_on_bootstrap_startup); RUN_TEST(cli_install_recovers_markerless_stale_rendezvous); + RUN_TEST(cli_install_skip_binary_into_foreign_home_never_drains_host_cohort); + RUN_TEST(cli_install_binary_into_foreign_home_never_drains_host_cohort); + RUN_TEST(cli_install_into_host_namespace_still_drains_host_cohort); + RUN_TEST(cli_install_skip_binary_unchanged_in_host_namespace_quiesces_nothing); #endif RUN_TEST(cli_install_force_quiesces_active_cohort_before_replacing_binary); RUN_TEST(cli_install_dir_and_skip_config_stage_first_install_safely); From 92cb3b9a4bebbc79bd283e447104669ca068ea64 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Wed, 9 Sep 2026 00:56:55 +0200 Subject: [PATCH 2/4] fix(test): bound the cli activation-guard scope fixture's host waits (#2115) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four namespace-scope tests fork a real host daemon and, in the parent, waited on it with two UNBOUNDED calls: read() for the child's one-byte readiness signal in cli_scope_fixture_start, and waitpid(host, 0) in cli_scope_fixture_finish. If the forked child stalls before it can signal — a fork-time sanitizer allocator stall is the classic cause, the same hazard the earlier posix_spawn work addressed — the parent blocks forever and the WHOLE cli suite hangs to the CI wall-clock kill (the observed rc=124 at 900 s on the ubuntu-24.04-arm gcc shard, which passed on every other #2115 shard). Attribution: the production drain path is already bounded (DRAIN 15 s, CONTROL 2 s) and a foreign-namespace install takes the non-blocking scope probe and skips draining entirely, so this is not a production deadlock — it is a test-fixture determinism gap, amplified by ASan on a loaded arm runner. Fix, test-only: - cli_scope_wait_ready(): poll the ready pipe to a generous, bounded deadline (90 s, comfortably past the child's own 45 s cohort-admission deadline plus service start), so a wedged child yields a clean ASSERT_TRUE(ready) failure instead of an unbounded read(). - cli_scope_reap_host(): reap with WNOHANG to a bound (60 s), then SIGKILL and reap, so a wedged child is force-killed rather than hanging finish(); a killed child leaves host_exit == -1, which fails the caller's assertion cleanly. This also prevents a stuck child from holding cohort locks that would poison later tests. - cli_scope_host_serving(): raise the status probe deadline from 5 s to 15 s so a slow-but-alive daemon on a loaded runner is not misread as "drained" (the flaky ASSERT(host_serving) failure this fixture showed); it still fails cleanly when the daemon is genuinely gone or reports stopping. None of these decides a healthy test — the child signals in well under a second on a healthy runner; the bounds only convert an otherwise unbounded hang into a deterministic pass/fail. The scoping contract still binds: with cli_activation_resolve_scope forced to always return ACTIVE, the three "must-not-drain" tests go RED at ASSERT(host_serving) and the host-namespace "must-drain" test stays GREEN. cli suite: 313 passed locally (macOS, twice); 30-round pre-fix and 8-round post-fix stress of the four tests were clean; a fault-injected never-ready child fails all four cleanly in ~37 s with no suite hang. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Vogel --- tests/test_cli.c | 89 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 7 deletions(-) diff --git a/tests/test_cli.c b/tests/test_cli.c index 3a4b4a851..43779e3bd 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -42,6 +42,7 @@ #include #include #include +#include #endif #ifdef __APPLE__ #include @@ -1510,6 +1511,16 @@ TEST(cli_install_recovers_markerless_stale_rendezvous) { * install under test and asks the daemon itself afterwards. */ #define CLI_SCOPE_HOST_DRAINED 3 #define CLI_SCOPE_TIMEOUT_MS 5000U +/* Generous, BOUNDED waits so a wedged host child (a fork-time sanitizer + * allocator stall is the classic cause — see the posix_spawn fix history) + * fails the test cleanly instead of hanging the whole suite to the CI + * wall-clock kill. Each comfortably exceeds the child's own 45 s cohort + * admission deadline plus a multi-second service start/teardown, so none trips + * for a merely slow-but-healthy runner; they only convert an otherwise + * unbounded hang into a deterministic pass/fail. */ +#define CLI_SCOPE_READY_TIMEOUT_MS 90000U +#define CLI_SCOPE_HOST_REAP_TIMEOUT_MS 60000U +#define CLI_SCOPE_HOST_SERVING_TIMEOUT_MS 15000U typedef struct { char tmpdir[256]; @@ -1598,6 +1609,68 @@ static _Noreturn void cli_scope_host_child(const cli_scope_fixture_t *fixture, i _exit(!ready_ok ? 1 : drained ? CLI_SCOPE_HOST_DRAINED : 0); } +/* Bounded read of the host child's one-byte readiness signal. A child that + * deadlocks before it can write (a fork-time allocator stall under a sanitizer + * is the classic cause) must never hang the whole suite on an unbounded read: + * poll to a generous deadline, then let the caller's ASSERT_TRUE(ready) fail + * cleanly. Returns the byte, or 0 when the child died, closed the pipe, or + * never answered in time. */ +static char cli_scope_wait_ready(int fd, uint32_t timeout_ms) { + uint64_t deadline = cbm_now_ms() + timeout_ms; + for (;;) { + int64_t remaining = (int64_t)deadline - (int64_t)cbm_now_ms(); + if (remaining <= 0) { + return 0; + } + struct pollfd pfd = {.fd = fd, .events = POLLIN, .revents = 0}; + int r = poll(&pfd, 1, (int)remaining); + if (r < 0) { + if (errno == EINTR) { + continue; + } + return 0; + } + if (r == 0) { + return 0; /* deadline reached with no signal */ + } + char ready = 0; + ssize_t got = read(fd, &ready, 1); + if (got == 1) { + return ready; + } + if (got < 0 && errno == EINTR) { + continue; + } + return 0; /* EOF (child gone) or error */ + } +} + +/* Reap the host child within a bound: the release signal makes a healthy child + * break within ~50 ms and finish teardown in a few seconds, so a child still + * alive past the deadline is wedged — SIGKILL it and reap so the suite always + * makes progress. Returns the child's exit code, or -1 when it had to be + * killed or did not exit cleanly; a -1 fails the caller's host_exit assertion + * cleanly rather than hanging. */ +static int cli_scope_reap_host(pid_t host, uint32_t timeout_ms) { + uint64_t deadline = cbm_now_ms() + timeout_ms; + int status = 0; + for (;;) { + pid_t reaped = waitpid(host, &status, WNOHANG); + if (reaped == host) { + return WIFEXITED(status) ? WEXITSTATUS(status) : -1; + } + if (reaped < 0 && errno != EINTR) { + return -1; + } + if (cbm_now_ms() >= deadline) { + (void)kill(host, SIGKILL); + (void)waitpid(host, &status, 0); + return -1; + } + cbm_usleep(2000); + } +} + static bool cli_scope_fixture_start(cli_scope_fixture_t *fixture, const char *tag) { memset(fixture, 0, sizeof(*fixture)); fixture->host = -1; @@ -1657,8 +1730,8 @@ static bool cli_scope_fixture_start(cli_scope_fixture_t *fixture, const char *ta } close(ready_pipe[1]); close(release_pipe[0]); - char ready = 0; - bool host_ready = child > 0 && read(ready_pipe[0], &ready, 1) == 1 && ready == 'R'; + char ready = child > 0 ? cli_scope_wait_ready(ready_pipe[0], CLI_SCOPE_READY_TIMEOUT_MS) : 0; + bool host_ready = ready == 'R'; close(ready_pipe[0]); fixture->host = child; fixture->release_fd = release_pipe[1]; @@ -1682,9 +1755,14 @@ static bool cli_scope_fixture_start(cli_scope_fixture_t *fixture, const char *ta * committed client still admitted. */ static bool cli_scope_host_serving(const cli_scope_fixture_t *fixture) { cbm_daemon_runtime_status_t status = {0}; + /* A generous, bounded status deadline: a foreign-namespace install leaves + * this daemon serving, so a slow response on a loaded runner must not be + * misread as "drained" (the flaky failure this fixture showed). The call + * still fails cleanly — a genuinely drained daemon is unreachable or + * reports stopping — it just no longer decides survival on a 5 s budget. */ return fixture->endpoint && cbm_daemon_runtime_request_status(fixture->endpoint, &fixture->identity, - CLI_SCOPE_TIMEOUT_MS, &status) && + CLI_SCOPE_HOST_SERVING_TIMEOUT_MS, &status) && !status.stopping && status.committed_clients == 1; } @@ -1718,10 +1796,7 @@ static int cli_scope_fixture_finish(cli_scope_fixture_t *fixture) { } int host_exit = -1; if (fixture->host > 0) { - int status = 0; - if (waitpid(fixture->host, &status, 0) == fixture->host && WIFEXITED(status)) { - host_exit = WEXITSTATUS(status); - } + host_exit = cli_scope_reap_host(fixture->host, CLI_SCOPE_HOST_REAP_TIMEOUT_MS); fixture->host = -1; } cbm_daemon_ipc_endpoint_free(fixture->endpoint); From 9a460b389ce3002ac164605c3627bb86aa3b9be3 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 12 Sep 2026 15:38:20 +0200 Subject: [PATCH 3/4] fix(sha256): keep the message schedule out of the transform's frame sha256_transform held its 64-word message schedule as a 256-byte local. That is large enough for ASan's use-after-return fake stack, so an instrumented build heap-allocated it through __asan_stack_malloc on every 64-byte block -- millions of allocations to hash one large file. Under the sanitized CI settings (detect_stack_use_after_return=1) fingerprinting a freshly installed binary therefore took minutes, and the cli suite blew its 900s wall-clock budget and was killed as hung on the Linux arm64 leg. The suite passes on macOS, where the fake stack is not engaged the same way, which is why this only ever surfaced on one leg. Move the schedule into cbm_sha256_ctx so it is allocated once per hash instead of once per block. The computed values and the resulting digest are identical; no sanitizer option is relaxed and use-after-return detection stays enabled everywhere. Verified in the Linux arm64 sanitized container under the exact CI ASAN_OPTIONS (detect_stack_use_after_return=1:strict_string_checks=1: detect_stack_use_after_scope=1): cli 316 passed in 356s, previously killed at the 900s budget; daemon_version 10 passed, covering the build-fingerprint digests that would break first on a wrong hash. Signed-off-by: Martin Vogel --- src/foundation/sha256.c | 6 +++++- src/foundation/sha256.h | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/foundation/sha256.c b/src/foundation/sha256.c index 937e363e6..84d37453e 100644 --- a/src/foundation/sha256.c +++ b/src/foundation/sha256.c @@ -25,7 +25,11 @@ static const uint32_t K[64] = { #define SIG1(x) (ROTR(x, 17) ^ ROTR(x, 19) ^ ((x) >> 10)) static void sha256_transform(cbm_sha256_ctx *c, const uint8_t *data) { - uint32_t m[64]; + /* Scratch comes from the context, not this frame: a 256-byte local here + * is fake-stacked by ASan's use-after-return mode on every 64-byte block + * (see the note on cbm_sha256_ctx::sched). Identical values, allocated + * once per hash instead of once per block. */ + uint32_t *m = c->sched; for (int i = 0, j = 0; i < 16; i++, j += 4) { m[i] = ((uint32_t)data[j] << 24) | ((uint32_t)data[j + 1] << 16) | ((uint32_t)data[j + 2] << 8) | (uint32_t)data[j + 3]; diff --git a/src/foundation/sha256.h b/src/foundation/sha256.h index a34a741d3..f3c7f9c55 100644 --- a/src/foundation/sha256.h +++ b/src/foundation/sha256.h @@ -17,6 +17,14 @@ typedef struct { uint64_t bitlen; uint8_t buf[64]; size_t buflen; + /* Message-schedule scratch for one compression round. It lives in the + * context rather than in sha256_transform's own frame because a 256-byte + * local there is large enough for ASan's use-after-return fake stack, + * which then heap-allocates it on EVERY 64-byte block — millions of + * __asan_stack_malloc calls to fingerprint one large file, turning + * sanitized binary fingerprinting into minutes. Here it is allocated once + * per hash instead of once per block. */ + uint32_t sched[64]; } cbm_sha256_ctx; void cbm_sha256_init(cbm_sha256_ctx *c); From fa99ff64eb6579fb15f3aae1aff7da349cb2999a Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 12 Sep 2026 18:01:46 +0200 Subject: [PATCH 4/4] test(cli): stop a negative probe from spending 15s proving silence The macos-15-intel leg killed the cli suite at its 900s wall clock while reporting 7537 passed, 0 failed. Two separate things were wrong. The suite was misclassified. cli spends 497s of the 900s default on macos-14, the FASTEST macOS runner, while macos-15-intel in the same matrix is 2.4-3.6x slower on comparable suites (daemon_runtime 842s vs 349s, stack_overflow_b 277s vs 76s). 497s at that ratio cannot fit. daemon_runtime, at 842s on that same runner, passes only because it was already in SLOW_SUITES. cli belongs there too -- it is deterministic and simply large, which is the only thing that list is allowed to mean. The drain test was also waiting on a clock instead of on the system. Profiling the suite per test (315 tests, 300s wall / 166s cpu) put cli_install_into_host_namespace_still_drains_host_cohort at 42s wall for 19s of cpu -- 23s idle, the largest single idle block, where the next worst was 11s. The existing cli_install_force_quiesces_active_cohort_before_replacing_binary drains a cohort too and idles 0.5s, which is what pointed at the difference. The cost was the host_serving probe. Its generous budget exists for the POSITIVE question -- is this daemon still up? -- where a slow reply on a loaded runner must not be misread as drained; that fixed a real flake and is kept. Asked in the negative it inverts: no reply is coming, so the full 15s is spent establishing silence and ASSERT_FALSE is decided by the timeout expiring rather than by the daemon's behaviour. The drain is already proven positively in that test -- install returns 0 only after the activation completed, and the host child exits CLI_SCOPE_HOST_DRAINED -- so the probe only has to confirm it. Also shortens the drained child's teardown budget, which retried service_free / lease_release / manager_free against 10s on a path where the activation had already torn the service down. Behaviour at the deadline is unchanged; it is reached sooner when nothing is wedged. Measured at only -3s on its own, kept because it is correct and free. Local, same machine, ASan+UBSan: the drain test 42.0s -> 25.8s (-38%), the suite 300s -> 279s, 315 passed before and after -- no test removed, no assertion weakened. PR #2188, which runs the same leg without these two tests, passed macos-15-intel in 37m34s, so the suite was not already over budget on main. Signed-off-by: Martin Vogel --- scripts/run-test-wave.py | 14 ++++++++++- tests/test_cli.c | 52 ++++++++++++++++++++++++++++++++++------ 2 files changed, 58 insertions(+), 8 deletions(-) diff --git a/scripts/run-test-wave.py b/scripts/run-test-wave.py index d24f80815..ce7e3d344 100755 --- a/scripts/run-test-wave.py +++ b/scripts/run-test-wave.py @@ -26,7 +26,19 @@ SUMMARY = re.compile(r"^ (?P[0-9]+) passed") FAILED = re.compile(r"(?:^|, )(?P[0-9]+) failed") SKIPPED = re.compile(r"(?:^|, )(?P[0-9]+) skipped") -SLOW_SUITES = frozenset(("incremental", "store_arch", "daemon_runtime")) +# Suites whose honest runtime does not fit the default per-suite budget, and so +# get --slow-timeout instead. This is a statement about SIZE, never about +# flakiness: every suite here is deterministic and simply long, and a racy suite +# must be made deterministic rather than given more clock. +# +# `cli` joined the list because the classification had gone stale, not because +# anything regressed. It spends 497s of the 900s default on macos-14 -- the +# FASTEST macOS runner -- while the macos-15-intel runner in the same matrix is +# 2.4-3.6x slower on comparable suites (daemon_runtime 842s vs 349s, +# stack_overflow_b 277s vs 76s). 497s at that ratio cannot fit, so the suite was +# killed at 900s and reported as hung. daemon_runtime, at 842s on that same +# runner, survives only because it was already listed here. +SLOW_SUITES = frozenset(("incremental", "store_arch", "daemon_runtime", "cli")) POLL_SECONDS = 0.05 # WHY: the Windows descendant probe below is a cold `powershell.exe` + CIM diff --git a/tests/test_cli.c b/tests/test_cli.c index 4f427be3a..4b3234ad4 100644 --- a/tests/test_cli.c +++ b/tests/test_cli.c @@ -1522,6 +1522,27 @@ TEST(cli_install_recovers_markerless_stale_rendezvous) { #define CLI_SCOPE_READY_TIMEOUT_MS 90000U #define CLI_SCOPE_HOST_REAP_TIMEOUT_MS 60000U #define CLI_SCOPE_HOST_SERVING_TIMEOUT_MS 15000U +/* Teardown budget for a child the activation ALREADY drained: its service is + * gone, so each free/release succeeds immediately and this is only the + * give-up point if one unexpectedly does not. The undrained path keeps the + * full 2 x CLI_SCOPE_TIMEOUT_MS, which is where a wedged teardown is real. */ +#define CLI_SCOPE_CLEANUP_DRAINED_MS 1000U +/* Budget for probing that an ALREADY-drained host has stopped serving. + * + * The generous CLI_SCOPE_HOST_SERVING_TIMEOUT_MS exists for the POSITIVE + * question -- "is this daemon still up?" -- where a slow reply on a loaded + * runner must not be misread as drained. Asked in the negative it inverts: + * there is no reply coming, so the whole budget is spent proving silence, and + * the assertion is decided by a timeout expiring rather than by the system's + * own behaviour. That was 15 s of the drain test's wall clock and the largest + * single idle block in the cli suite. + * + * The drain is proven POSITIVELY elsewhere in that test: install returns 0 + * only after the activation completed, and the host child exits with + * CLI_SCOPE_HOST_DRAINED. By the time this probe runs the daemon is already + * gone, so a short budget confirms the same fact a long one would -- it just + * stops charging the suite for the wait. */ +#define CLI_SCOPE_HOST_DRAINED_PROBE_MS 2000U typedef struct { char tmpdir[256]; @@ -1588,7 +1609,16 @@ static _Noreturn void cli_scope_host_child(const cli_scope_fixture_t *fixture, i break; } } - uint64_t cleanup_deadline = cbm_now_ms() + 2U * CLI_SCOPE_TIMEOUT_MS; + /* A DRAINED child has already had its service torn down by the activation, + * so every teardown below succeeds on the first attempt; the long deadline + * exists for the wedged case, where we keep retrying before giving up. On + * the drained path that budget was pure wall clock -- the parent sits in + * cli_scope_reap_host waiting for this exit, and it was the single largest + * idle block in the whole cli suite. Behaviour at the deadline is + * unchanged (give up and _exit); it is simply reached sooner when there is + * nothing wedged to wait for. */ + uint64_t cleanup_deadline = + cbm_now_ms() + (drained ? CLI_SCOPE_CLEANUP_DRAINED_MS : 2U * CLI_SCOPE_TIMEOUT_MS); if (service) { if (!drained) { (void)cbm_daemon_runtime_service_stop(service, CLI_SCOPE_TIMEOUT_MS); @@ -1752,19 +1782,24 @@ static bool cli_scope_fixture_start(cli_scope_fixture_t *fixture, const char *ta return fixture->client != NULL; } +static bool cli_scope_host_serving_within(const cli_scope_fixture_t *fixture, + uint32_t timeout_ms) { + cbm_daemon_runtime_status_t status = {0}; + return fixture->endpoint && + cbm_daemon_runtime_request_status(fixture->endpoint, &fixture->identity, timeout_ms, + &status) && + !status.stopping && status.committed_clients == 1; +} + /* Ask the host daemon itself: still running, not stopping, and the parent's * committed client still admitted. */ static bool cli_scope_host_serving(const cli_scope_fixture_t *fixture) { - cbm_daemon_runtime_status_t status = {0}; /* A generous, bounded status deadline: a foreign-namespace install leaves * this daemon serving, so a slow response on a loaded runner must not be * misread as "drained" (the flaky failure this fixture showed). The call * still fails cleanly — a genuinely drained daemon is unreachable or * reports stopping — it just no longer decides survival on a 5 s budget. */ - return fixture->endpoint && - cbm_daemon_runtime_request_status(fixture->endpoint, &fixture->identity, - CLI_SCOPE_HOST_SERVING_TIMEOUT_MS, &status) && - !status.stopping && status.committed_clients == 1; + return cli_scope_host_serving_within(fixture, CLI_SCOPE_HOST_SERVING_TIMEOUT_MS); } static int cli_scope_install(cli_scope_fixture_t *fixture, const char *home, const char *cache, @@ -1902,7 +1937,10 @@ TEST(cli_install_into_host_namespace_still_drains_host_cohort) { int install_rc = ready ? cli_scope_install(&fixture, fixture.host_home, fixture.host_cache, host_bin, false) : -1; - bool host_serving = ready && cli_scope_host_serving(&fixture); + /* Negative probe: see CLI_SCOPE_HOST_DRAINED_PROBE_MS. The drain itself is + * asserted positively below via host_exit == CLI_SCOPE_HOST_DRAINED. */ + bool host_serving = ready && cli_scope_host_serving_within(&fixture, + CLI_SCOPE_HOST_DRAINED_PROBE_MS); const char *events = read_test_file(activation_log); bool drained_in_log = events && strstr(events, "cohort drained") != NULL && strstr(events, "\"daemon_active_clients\":1") != NULL;