mirror of
https://github.com/DeusData/codebase-memory-mcp.git
synced 2026-10-21 05:59:35 +08:00
fix: address QA round 2
Retracts an incorrect justification from the round-1 commit (4fc5999a) and
closes the coverage it was used to excuse.
MAJOR — the delay wire-up had no coverage, and the reason given for that was
wrong. Round 1 claimed the gating half was "structurally unobservable without
an injectable clock". It is not. cbm_watcher_touch zeroes next_poll_ns, so a
wire-up that never sets a deadline leaves it at zero and the callback fires on
every poll: simply NOT calling touch between polls observes gating
deterministically, no clock involved. Only the delay's MAGNITUDE needs one.
watcher_index_failure_backoff_gates_repolling_issue2015 covers it — one hard
failure, then five polls without touch asserting no re-fork, then a touch
proving the retry still gets through (so the test cannot pass by observing a
dead watcher). Mutation-verified: deleting the wire-up yields
"failing_index_calls == 6, expected 1".
MINOR — round 1's F1 was half-fixed. Of the two single-token mutations that
finding named, only the streak reset was covered; widening the
sustained-failure "==" to ">=" still passed. watcher_sustained_failure_logs_
once_issue2015 drives 14 consecutive failures through a cbm_log_set_sink and
asserts exactly one emission. Mutation-verified: ">=" yields
"sustained_log_hits == 5, expected 1".
All three mutations F1 named are now demonstrated caught rather than argued
about: reset removal (round 1), wire-up deletion, and the "==" widening.
MINOR — cbm_watcher_index_failure_count reads index_failures under
projects_lock while poll_project writes it lock-free from a state snapshot.
That is the discipline every other per-project field here already follows, so
this documents the visibility contract rather than making one field atomic in
isolation: the accessor is a diagnostic and test seam, not a synchronisation
point, and must not carry scheduling decisions without atomic accessors first.
MINOR — watcher.h still described the pre-F3 backoff contract. It now states
the clamp is a floor as well as a cap, why the floor exists for an exported
function whose reachable inputs cannot currently hit it, and that negative
inputs are treated as zero.
Still uncovered, now stated precisely rather than hand-waved: the delay's
MAGNITUDE. Nothing asserts that ten failures produce a five-minute delay
rather than a five-second one. Gating is proven; the numbers are not, and
that does need a controllable clock.
Refs #2015
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FbrM52pmB1m2SR1vRRoNnf
Signed-off-by: Shane McCarron <shane.mccarron@corvexconnect.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
4fc5999a85
commit
61ccd90511
+24
-5
@@ -92,10 +92,19 @@ void cbm_watcher_stop(cbm_watcher_t *w);
|
||||
int cbm_watcher_watch_count(cbm_watcher_t *w);
|
||||
|
||||
/* Return a watched project's consecutive hard-index-failure count, or -1 when
|
||||
* it is not watched. Exposed because the failure state machine (increment on
|
||||
* a hard error, reset on success) is otherwise unobservable from a test: the
|
||||
* only other thing it feeds is a poll deadline, and gating on that deadline
|
||||
* needs a controllable clock the watcher does not have. */
|
||||
* it is not watched. Exposed so the failure state machine (increment on a
|
||||
* hard error, reset on success) can be asserted directly rather than inferred
|
||||
* from the poll deadline it feeds.
|
||||
*
|
||||
* Memory visibility: this reads under projects_lock, but poll_project WRITES
|
||||
* the counter outside that lock — it runs against a state snapshot taken
|
||||
* while the lock was held, which is the same discipline every other
|
||||
* per-project field in this struct already follows. Reading concurrently with
|
||||
* a live poll is therefore formally a data race and may observe a stale
|
||||
* value; it is a diagnostic and test accessor, not a synchronisation point.
|
||||
* Single-threaded callers (the tests, and any caller between poll cycles)
|
||||
* always see the current value. Do not build scheduling decisions on it
|
||||
* without first giving the counter atomic accessors. */
|
||||
int cbm_watcher_index_failure_count(cbm_watcher_t *w, const char *project_name);
|
||||
|
||||
/* Return the adaptive poll interval (ms) for a given file count. */
|
||||
@@ -105,7 +114,17 @@ int cbm_watcher_poll_interval_ms(int file_count);
|
||||
* `consecutive_failures` consecutive hard index failures. Zero failures
|
||||
* yields `interval_ms` unchanged; each further failure doubles the delay up
|
||||
* to a fixed ceiling, so a permanently failing project stops re-forking a
|
||||
* worker at the poll cadence without ever being abandoned. */
|
||||
* worker at the poll cadence without ever being abandoned.
|
||||
*
|
||||
* The result is clamped at BOTH ends: never above the ceiling, and never
|
||||
* below `interval_ms`, so backing off can only ever delay the next attempt,
|
||||
* never bring it forward. That lower clamp matters only for an `interval_ms`
|
||||
* already above the ceiling, which the current constants cannot produce
|
||||
* (POLL_MAX_MS < the ceiling) — it is stated because this is an exported
|
||||
* function and the guarantee should hold for any argument a caller passes,
|
||||
* not only for the ones today's constants generate.
|
||||
*
|
||||
* Negative inputs are treated as zero. */
|
||||
int cbm_watcher_index_backoff_ms(int interval_ms, int consecutive_failures);
|
||||
|
||||
/* Classify a stat() errno observed on a watched project root: returns true
|
||||
|
||||
@@ -7,6 +7,7 @@
|
||||
#include "../src/foundation/compat.h"
|
||||
#include "../src/foundation/compat_thread.h"
|
||||
#include "../src/foundation/constants.h"
|
||||
#include "../src/foundation/log.h"
|
||||
#include "../src/foundation/platform.h"
|
||||
#include "test_framework.h"
|
||||
#include "test_helpers.h"
|
||||
@@ -1873,6 +1874,143 @@ TEST(watcher_index_failure_streak_increments_and_resets_issue2015) {
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* The delay WIRE-UP (#2015): a hard failure must actually push the project's
|
||||
* next poll out, so a permanently failing project stops re-forking a worker
|
||||
* on every cycle. That is this PR's headline behaviour and it is observable
|
||||
* without a clock, by simply NOT calling cbm_watcher_touch between polls:
|
||||
* touch zeroes next_poll_ns, so a wire-up that never sets a deadline leaves
|
||||
* it at 0 and the callback fires on every poll. What still needs a clock is
|
||||
* only the delay's MAGNITUDE, not whether gating happens at all. */
|
||||
TEST(watcher_index_failure_backoff_gates_repolling_issue2015) {
|
||||
char tmpdir[256];
|
||||
snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_gate_XXXXXX");
|
||||
if (!cbm_mkdtemp(tmpdir))
|
||||
FAIL("cbm_mkdtemp failed");
|
||||
|
||||
if (wt_git(tmpdir, "init -q") != 0) {
|
||||
th_rmtree(tmpdir);
|
||||
FAIL("git init failed");
|
||||
}
|
||||
{
|
||||
char p[300];
|
||||
th_write_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "hello\n");
|
||||
}
|
||||
wt_git(tmpdir, "add file.txt");
|
||||
wt_git(tmpdir, "commit -q -m init");
|
||||
|
||||
cbm_store_t *store = cbm_store_open_memory();
|
||||
cbm_watcher_t *w = cbm_watcher_new(store, failing_index_callback, NULL);
|
||||
|
||||
cbm_watcher_watch(w, "gate-repo", tmpdir);
|
||||
failing_index_calls = 0;
|
||||
failing_index_fail_first_n = 100; /* never succeeds */
|
||||
|
||||
cbm_watcher_poll_once(w); /* baseline */
|
||||
|
||||
{
|
||||
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");
|
||||
|
||||
/* One failure, forced by touch. */
|
||||
cbm_watcher_touch(w, "gate-repo");
|
||||
cbm_watcher_poll_once(w);
|
||||
ASSERT_EQ(failing_index_calls, 1);
|
||||
ASSERT_EQ(cbm_watcher_index_failure_count(w, "gate-repo"), 1);
|
||||
|
||||
/* Now poll repeatedly WITHOUT touching. The change is still pending (the
|
||||
* baseline stays uncommitted per #937), so the ONLY thing that can stop a
|
||||
* re-fork is the deadline the failure just scheduled. Before the backoff
|
||||
* wire-up existed this project was re-forked at the poll cadence; with no
|
||||
* deadline set at all it would fire on every one of these iterations. */
|
||||
for (int i = 0; i < 5; i++) {
|
||||
cbm_watcher_poll_once(w);
|
||||
}
|
||||
ASSERT_EQ(failing_index_calls, 1);
|
||||
ASSERT_EQ(cbm_watcher_index_failure_count(w, "gate-repo"), 1);
|
||||
|
||||
/* Control: the watcher is not simply dead — clearing the deadline lets the
|
||||
* retry through, which is what keeps #937's at-least-once guarantee. */
|
||||
cbm_watcher_touch(w, "gate-repo");
|
||||
cbm_watcher_poll_once(w);
|
||||
ASSERT_EQ(failing_index_calls, 2);
|
||||
ASSERT_EQ(cbm_watcher_index_failure_count(w, "gate-repo"), 2);
|
||||
|
||||
cbm_watcher_free(w);
|
||||
cbm_store_close(store);
|
||||
th_rmtree(tmpdir);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* The sustained-failure line must fire EXACTLY once, at the threshold — the
|
||||
* other single-token mutation round 1 named (`==` widened to `>=`) would emit
|
||||
* it on every failure from the threshold onward and turn a one-shot signal
|
||||
* into the log spam the whole change is meant to avoid. */
|
||||
static int sustained_log_hits = 0;
|
||||
static void sustained_log_sink(const char *line) {
|
||||
if (line && strstr(line, "watcher.index.sustained_failure") != NULL) {
|
||||
sustained_log_hits++;
|
||||
}
|
||||
}
|
||||
|
||||
TEST(watcher_sustained_failure_logs_once_issue2015) {
|
||||
char tmpdir[256];
|
||||
snprintf(tmpdir, sizeof(tmpdir), "/tmp/cbm_watcher_sust_XXXXXX");
|
||||
if (!cbm_mkdtemp(tmpdir))
|
||||
FAIL("cbm_mkdtemp failed");
|
||||
|
||||
if (wt_git(tmpdir, "init -q") != 0) {
|
||||
th_rmtree(tmpdir);
|
||||
FAIL("git init failed");
|
||||
}
|
||||
{
|
||||
char p[300];
|
||||
th_write_file(wt_path(p, sizeof(p), tmpdir, "file.txt"), "hello\n");
|
||||
}
|
||||
wt_git(tmpdir, "add file.txt");
|
||||
wt_git(tmpdir, "commit -q -m init");
|
||||
|
||||
cbm_store_t *store = cbm_store_open_memory();
|
||||
cbm_watcher_t *w = cbm_watcher_new(store, failing_index_callback, NULL);
|
||||
|
||||
cbm_watcher_watch(w, "sust-repo", tmpdir);
|
||||
failing_index_calls = 0;
|
||||
failing_index_fail_first_n = 100; /* never succeeds */
|
||||
|
||||
cbm_watcher_poll_once(w); /* baseline */
|
||||
|
||||
{
|
||||
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++) {
|
||||
cbm_watcher_touch(w, "sust-repo");
|
||||
cbm_watcher_poll_once(w);
|
||||
}
|
||||
|
||||
cbm_log_set_sink(NULL);
|
||||
|
||||
ASSERT_EQ(failing_index_calls, 14);
|
||||
ASSERT_EQ(cbm_watcher_index_failure_count(w, "sust-repo"), 14);
|
||||
/* Exactly one, not four (11..14) and not zero. */
|
||||
ASSERT_EQ(sustained_log_hits, 1);
|
||||
|
||||
cbm_watcher_free(w);
|
||||
cbm_store_close(store);
|
||||
th_rmtree(tmpdir);
|
||||
PASS();
|
||||
}
|
||||
|
||||
TEST(watcher_index_failure_count_unknown_project) {
|
||||
cbm_store_t *store = cbm_store_open_memory();
|
||||
cbm_watcher_t *w = cbm_watcher_new(store, index_callback, NULL);
|
||||
@@ -3302,6 +3440,8 @@ SUITE(watcher) {
|
||||
RUN_TEST(index_backoff_degenerate_inputs);
|
||||
RUN_TEST(index_backoff_never_schedules_sooner_than_the_interval);
|
||||
RUN_TEST(watcher_index_failure_streak_increments_and_resets_issue2015);
|
||||
RUN_TEST(watcher_index_failure_backoff_gates_repolling_issue2015);
|
||||
RUN_TEST(watcher_sustained_failure_logs_once_issue2015);
|
||||
RUN_TEST(watcher_index_failure_count_unknown_project);
|
||||
|
||||
/* Lifecycle */
|
||||
|
||||
Reference in New Issue
Block a user