atoi and atol answer 0 for text they cannot read, and 0 is a real setting at
three places in this project. So a typo, a trailing unit such as "30s", or a
stray space silently chose a value nobody asked for, and nothing on screen
said the setting had been dropped.
src/mcp/index_supervisor.c CBM_INDEX_WORKER_TIMEOUT_S
src/cli/hook_augment.c CBM_HOOK_DEADLINE_MS
src/mcp/mcp.c CBM_INDEX_MAX_RESTARTS
CBM_HOOK_DEADLINE_MS was the worst of the three. atoi answered 0, 0 is below
HA_DEADLINE_MIN_MS, and the clamp then handed back 50 ms -- the SHORTEST
deadline the setting allows, for a setting whose only purpose is to give the
hook more room. The comment above that function records a hunt for hook runs
that never finished, 0 of 24 real sessions, which is the exact symptom a
silently-shortened deadline produces.
CBM_INDEX_MAX_RESTARTS lost twice. A typo kept the default of 100, and
CBM_INDEX_MAX_RESTARTS=0 -- which reads as "do not restart" to anybody who
sets it -- also kept 100. The setting did the opposite of the request.
CBM_INDEX_WORKER_TIMEOUT_S fell through to the 15-minute default, so a test
set to give up after 30 seconds hung for 15 minutes with nothing to explain
why.
The fix adds one helper rather than three copies of the same check:
bool cbm_env_long(const char *name, long *out);
It answers true only when the variable is set, is not empty, and reads
cleanly from its first character to its last. It holds no policy -- no
minimum, no maximum, no default -- because the three sites disagree on all
three, and a helper that guessed would be wrong at two of them. The shape is
the one src/main.c:1104 already uses: an end pointer, errno, and a check that
nothing was left over. It also refuses a leading blank, which strtol would
otherwise step over, so " 5" is a slip rather than the number 5.
Each site keeps its own rule:
worker timeout an unreadable value keeps the 15-minute default AND logs
the value it dropped
restart cap 0 now means no restarts; an unreadable value keeps 100
AND logs the value it dropped
hook deadline an unreadable value now yields HA_DEADLINE_DEFAULT_MS,
not the floor. This one stays silent on purpose: the file
includes no log header and writes no stderr, because its
output is hook protocol.
The restart-cap parse was lifted out of a very large function into a named
index_restart_cap(), so it can be read and reached on its own.
Four tests come with the change. The two that pin the user-visible behaviour
were seen failing before the fix:
FAIL tests/test_cli.c:402: ms == 50, expected HOOK_DEADLINE_DEFAULT == 2000
(with "unreadable value \"abc\" gave 50 ms" printed above it)
FAIL tests/test_cli.c:434: cbm_index_restart_cap_for_testing() == 100,
expected 0 == 0
After the fix, TEST_SUITES="platform cli mcp" reports 518 passed, 2 failed.
The full suite reports 7635 passed, 2 failed. Both failures are in
tests/test_cli.c (lines 1826 and 6802), print "error: one or more agent
cleanup operations failed", and reproduce on a clean tree without this
change -- they depend on the coding agents installed on the machine.
make -f Makefile.cbm lint-ci passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
