From 5a9c72d8f8c65b38c66a29a34184b49edf3378ff Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sat, 26 Sep 2026 00:27:53 +0200 Subject: [PATCH 1/2] test(watcher): pin the pending-free drain on a held state, not a git probe watcher_unwatch_drains_pending_free failed three times on macos-14 CI with `index_call_count == 0, expected 1` at its dirty-poll precondition. That precondition needs every git subprocess of one poll (rev-parse HEAD, status, submodule foreach) to succeed. A transient probe failure (spawn refused under runner load, a non-zero exit) makes that poll a fail-closed no-op by design: #937 leaves the baseline uncommitted and the next poll retries. Several of those paths return without a log line, which matches the CI output exactly: watch, baseline, FAIL, with no watcher.changed and no watcher.git.failed. Holding a spawn failure with cbm_subprocess_force_spawn_eagain_for_testing reproduces that output deterministically. This is a test race, not a production defect. The precondition was also unrelated to the contract under test. A poll that stopped draining would neither crash nor leak, because cbm_watcher_free frees the same deferred list. So the old test could not see a broken drain at all. Add a test-only seam, cbm_watcher_test_pending_free_count, and assert the stable states the watcher holds: 0 parked before unwatch, 1 after, 0 after the next poll with no index work, and still 0 after a further poll. No assertion depends on a git probe succeeding. Dirty-poll reindexing stays covered by watcher_detects_dirty_worktree and watcher_modify_tracked_file. Signed-off-by: Martin Vogel --- src/watcher/watcher.c | 12 ++++++++++++ src/watcher/watcher.h | 8 ++++++++ tests/test_watcher.c | 43 ++++++++++++++++++++++++++++++++----------- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/src/watcher/watcher.c b/src/watcher/watcher.c index 95dfc1725..dca47cb20 100644 --- a/src/watcher/watcher.c +++ b/src/watcher/watcher.c @@ -1653,3 +1653,15 @@ int cbm_watcher_run(cbm_watcher_t *w, int base_interval_ms) { cbm_log_info("watcher.stop"); return 0; } + +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS +int cbm_watcher_test_pending_free_count(cbm_watcher_t *w) { + if (!w) { + return -1; + } + cbm_mutex_lock(&w->projects_lock); + int count = w->pending_free_count; + cbm_mutex_unlock(&w->projects_lock); + return count; +} +#endif diff --git a/src/watcher/watcher.h b/src/watcher/watcher.h index 013e1356a..2b33d78f4 100644 --- a/src/watcher/watcher.h +++ b/src/watcher/watcher.h @@ -110,6 +110,14 @@ int cbm_watcher_watch_count(cbm_watcher_t *w); * without first giving the counter atomic accessors. */ int cbm_watcher_index_failure_count(cbm_watcher_t *w, const char *project_name); +#if defined(CBM_ENABLE_TEST_SEAMS) && CBM_ENABLE_TEST_SEAMS +/* Test seam: number of unwatched/replaced project states parked on the + * deferred-free list, read under projects_lock. The drain in poll_once is + * otherwise unobservable — cbm_watcher_free frees the same list, so a poll + * that stopped draining would neither crash nor leak. Test builds only. */ +int cbm_watcher_test_pending_free_count(cbm_watcher_t *w); +#endif + /* Return the adaptive poll interval (ms) for a given file count. */ int cbm_watcher_poll_interval_ms(int file_count); diff --git a/tests/test_watcher.c b/tests/test_watcher.c index 5ca2d95e8..65c86f61c 100644 --- a/tests/test_watcher.c +++ b/tests/test_watcher.c @@ -3361,8 +3361,22 @@ TEST(watcher_callback_data_passed) { } TEST(watcher_unwatch_drains_pending_free) { - /* Unwatch moves project_state to pending_free; the next poll_once - * must drain it without crash or leak. */ + /* Contract: unwatch parks the project's state on the deferred-free list + * (an in-flight poll snapshot may still hold it), and the NEXT poll_once + * frees it, exactly once, without admitting index work for the unwatched + * project. + * + * No assertion here depends on a git probe succeeding. The earlier + * version asserted as a precondition that one dirty poll reindexed + * exactly once, which needs every git subprocess of that poll to + * succeed. A transient probe failure (spawn refused under runner load, a + * non-zero exit) makes that poll a fail-closed no-op by design (#937: + * the baseline stays uncommitted and the next poll retries), so the + * precondition read 0 on loaded macos-14 runners. Holding a spawn + * failure with cbm_subprocess_force_spawn_eagain_for_testing reproduces + * that CI log exactly. Dirty-poll-reindexes stays covered by + * watcher_detects_dirty_worktree and watcher_modify_tracked_file; this + * test pins the drain itself, through the pending-free seam. */ char tmpdir[256]; snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_df_XXXXXX"); if (!cbm_mkdtemp(tmpdir)) @@ -3383,28 +3397,35 @@ TEST(watcher_unwatch_drains_pending_free) { cbm_watcher_t *w = cbm_watcher_new(store, index_callback, NULL); cbm_watcher_watch(w, "df-repo", tmpdir); ASSERT_EQ(cbm_watcher_watch_count(w), 1); + ASSERT_EQ(cbm_watcher_test_pending_free_count(w), 0); index_call_count = 0; - /* Baseline */ + /* Baseline: the state goes through a poll snapshot. Its outcome is not + * asserted; a baseline poll never indexes, whatever git answers. */ cbm_watcher_poll_once(w); - ASSERT_EQ(index_call_count, 0); - /* Make dirty + detect change */ + /* Leave a new dirty state behind: if the unwatched state were still + * polled, the next poll would index it. */ { char p[300]; th_append_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "dirty\n"); } cbm_watcher_touch(w, "df-repo"); - cbm_watcher_poll_once(w); - ASSERT_EQ(index_call_count, 1); - /* Unwatch — state moves to pending_free */ + /* Unwatch: the state leaves the table and is parked, not freed. */ cbm_watcher_unwatch(w, "df-repo"); ASSERT_EQ(cbm_watcher_watch_count(w), 0); + ASSERT_EQ(cbm_watcher_test_pending_free_count(w), 1); - /* Next poll drains pending_free — no crash, no double-free */ - cbm_watcher_poll_once(w); - ASSERT_EQ(index_call_count, 1); + /* The next poll drains it and admits no index work. */ + ASSERT_EQ(cbm_watcher_poll_once(w), 0); + ASSERT_EQ(cbm_watcher_test_pending_free_count(w), 0); + ASSERT_EQ(index_call_count, 0); + + /* A further poll finds nothing left to free (no double free). */ + ASSERT_EQ(cbm_watcher_poll_once(w), 0); + ASSERT_EQ(cbm_watcher_test_pending_free_count(w), 0); + ASSERT_EQ(index_call_count, 0); cbm_watcher_free(w); cbm_store_close(store); From 07eb426c95386e299a6aa6adbf2b4eb9f272ff3b Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 27 Sep 2026 16:06:29 +0200 Subject: [PATCH 2/2] test(watcher): wait for the 14th reindex attempt in the sustained-failure test watcher_sustained_failure_logs_once_issue2015 failed on macos-14 CI with `failing_index_calls == 13, expected 14`. Its loop ran exactly 14 touch+poll rounds and assumed each one reached the index callback. A poll whose git probe fails transiently (spawn refused under runner load) is a fail-closed no-op by design (#937): the baseline stays uncommitted and the next poll retries. So one refused spawn costs one attempt, and the count ends at 13. This is the same test race as watcher_unwatch_drains_pending_free, not a production defect. Holding one exhausted spawn mid-loop with cbm_subprocess_force_spawn_eagain_for_testing reproduces the CI line exactly. The loop now polls until the 14th failed attempt instead of counting polls. Each poll runs at most one attempt for the single project, so it stops at exactly 14. A watcher that never reindexes hangs, and the runner reports that as a failure, never a pass. The log-once contract means the same thing as before: watcher.index.sustained_failure is emitted only on a failed index attempt when the streak equals the threshold, so extra no-op polls cannot add lines. An `==` to `>=` mutation in watcher.c still turns it red (`sustained_log_hits == 5, expected 1`). The change is now a dirty worktree instead of a commit. If the baseline poll itself failed, the old commit would be adopted as the baseline HEAD and never reindexed, so the wait would never end. The baseline keeps the dirty signature at "clean known", so a dirty change is seen whenever the baseline lands. With spawns forced to fail at the baseline poll and again mid-loop, the test passes after 16 polls for 14 attempts. Signed-off-by: Martin Vogel --- tests/test_watcher.c | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/test_watcher.c b/tests/test_watcher.c index 65c86f61c..3ff0c81b4 100644 --- a/tests/test_watcher.c +++ b/tests/test_watcher.c @@ -2121,21 +2121,34 @@ TEST(watcher_sustained_failure_logs_once_issue2015) { failing_index_calls = 0; failing_index_fail_first_n = 100; /* never succeeds */ - cbm_watcher_poll_once(w); /* baseline */ + /* Baseline poll. Its outcome is not asserted: if a git probe fails here, + * the first loop poll takes the baseline instead. */ + cbm_watcher_poll_once(w); + /* The change is a DIRTY worktree, not a commit. The baseline leaves the + * dirty signature at "clean known", so this change is seen whenever the + * baseline lands. A commit made before a late (or HEAD-less) baseline + * would be adopted as the baseline HEAD and never reindexed. */ { char p[300]; th_append_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "world\n"); } - wt_git(tmpdir, "add file.txt"); - wt_git(tmpdir, "commit -q -m add-world"); sustained_log_hits = 0; cbm_log_set_sink(sustained_log_sink); - /* Drive well past the threshold (10). touch clears the deadline each time - * so every iteration actually attempts a reindex. */ - for (int i = 0; i < 14; i++) { + /* Drive well past the threshold (10): poll until the 14th failed reindex. + * A poll does not always reach the index callback: when one of its git + * probes fails transiently (spawn refused under runner load) it is a + * fail-closed no-op by design (#937) and the next poll retries. So the + * loop waits for the attempt count instead of assuming one per poll; + * touch clears the backoff deadline each time. Each poll runs at most one + * attempt for this single project, so the loop stops at exactly 14. A + * watcher that never reindexes hangs here, which the runner reports as a + * failure, never a pass. No-op polls cannot add log lines to the count: + * sustained_failure is emitted only on a failed index attempt, when the + * streak EQUALS the threshold. */ + while (failing_index_calls < 14) { cbm_watcher_touch(w, "sust-repo"); cbm_watcher_poll_once(w); }