scripts/soak-test.sh claimed to isolate daemon coordination from
interactive sessions through a private CBM_CACHE_DIR, but only
CBM_RUNTIME_DIR moves the daemon rendezvous, so the soak shared the
operator's account daemon and asserted that its own shutdown stopped it.
Source scripts/test-runtime.sh for a harness-owned runtime and cache,
and let the helper stop the private daemon before the root is removed.
Add tests/test_soak_runtime_isolation_contract.sh, which fails before
this change, and wire it into scripts/test.sh.
Part of #1696.
Signed-off-by: Anton Standrik <astandrik@yandex-team.ru>
Three maintainer decisions from the challenger read on the #1830 unit.
1. DELETE THE CACHE (not just fix its comment).
posix_ancestor_overflow_uid() memoised via pthread_once behind a comment
claiming "immutable /proc state". Both halves were false: unshare(CLONE_NEWUSER)
rewrites /proc/self/uid_map, and pthread_once state survives fork() already
marked done. A process that forked and then entered a namespace kept the
parent verdict and refused a directory it should have accepted.
It was harmless only because every caller today runs in a freshly exec'd
process -- the daemon double-fork execs, workers go through
cbm_subprocess_spawn's fork+exec, worker_pool.c never forks, and src/ contains
no setns or unshare at all. That made a security decision depend on an
invariant nothing enforced. The next fork-without-exec caller would have
inherited a stale verdict silently.
Now derived fresh per call: two small /proc reads against an openat + fstat +
fchmod + ACL check per path component in the same walk.
With a test that would have caught the original bug and needs no namespace.
daemon_ipc_posix_overflow_uid_is_never_cached_issue1830 counts real
derivations through the production entry point and asserts the count rises on
every ancestor check. Re-introducing a cache fails it by name on every
platform. Proven both ways on linux/arm64 as uid 1001: cache re-introduced
49 passed 1 failed; removed 50 passed 0 skipped.
Worth knowing why that test is not redundant: with the re-exec probe in
place, the userns smoke test PASSES even with the cache restored, because the
probe is a fresh process and immune to it by construction. The two tests are
complementary -- the smoke test binds the namespace wiring, this one binds
the absence of the cache.
2. RUN THE GUARD ON THE CORE MATRIX.
Ubuntu 23.10+ ships kernel.apparmor_restrict_unprivileged_userns=1, so the
#1830 smoke test skipped on every 24.04 leg and its only real-namespace
coverage sat on the two ubuntu-22.04 legs. Those are broad-matrix only, so
the test ran in NO pull request, and GitHub retires those images on
2027-04-17 -- after which it would have become a permanent silent skip, and a
skip is not a red. A deliberate relaxation of a security check was guarded by
a test that ran nowhere a change would be reviewed, on a clock.
_test.yml now lifts the restriction with one sysctl on every ubuntu leg
(hosted runners have passwordless sudo), best-effort so a runner that refuses
simply returns to skipping. The O10 skip-whitelist comment no longer presents
the AppArmor restriction as immovable.
3. KEEP THE TOLERANCE, CORRECT THE SAFETY ARGUMENT.
The comment claimed that in a single-uid map "nobody reachable can have
created it or can mutate it". The overflow uid is what EVERY unmapped host
uid maps to, not only host root (user_namespaces(7)), so overflow-owned does
not prove root-created: on a shared host another local user's sticky
directory is indistinguishable from root-owned /tmp from inside the namespace.
The tolerance stays, because there is no in-namespace discriminator that
could tell them apart -- and narrowing it would refuse the legitimate
root-owned 01777 /tmp that #1830 exists to accept, i.e. it would revert the
fix in practice. What changes is the claim: the real guarantee is that no
principal reachable from inside the namespace can create or mutate such an
ancestor, and the leaf is never tolerated as overflow (0700, euid-rechecked),
so a hostile host-side ancestor owner is bounded to denial of service and
socket-path control.
macOS daemon_ipc 53 passed 2 skipped; linux/arm64 50 passed 0 skipped;
lint-ci clean.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Follow-up to the re-exec fix, from the challenger read the frozen scope
requires for security-boundary units. Three findings, all addressed here; two
direction questions go to the maintainer separately.
1. src/daemon/ipc.c stated an invariant that is FALSE. The comment above
posix_ancestor_overflow_uid() read "Derived once from immutable /proc
state". /proc/self/uid_map is not immutable -- unshare(CLONE_NEWUSER) is
exactly what rewrites it -- and pthread_once state survives fork() already
marked done. The hazard was written up in the tests, which is the one place
an ipc.c maintainer will never look, while production shipped the opposite
claim. The comment now states the real guarantee: the cache is safe only
because every caller runs in a freshly exec'd process, and it names what
would break it.
An audit of every process-creation site in src/ backs that: the daemon
double-fork execs (daemon/bootstrap.c), cbm_fork_with_retry execs
(foundation/subprocess.c), index workers go through cbm_subprocess_spawn,
worker_pool.c never forks, and src/ contains no setns or unshare at all.
No production path forks without exec and then reaches this check.
2. The probe now _exit()s instead of returning. The runner is built with
-fsanitize=address, so returning runs the atexit chain, and a future leak
anywhere in the prologue would exit 23 -- which the parent would have read
as a verdict on a security test that has none.
3. The parent now treats 0/1/2/3 as the closed protocol and names anything
else with the code it actually saw. A signal or a sanitizer exit is a broken
harness, not a refusal, and must not be able to look like one.
The probe guard also now matches its caller (linux AND test seams) instead of
compiling dead code in a non-seams build.
VERIFICATION. The challenger was right that the earlier proof was incomplete:
it showed the new test goes green, never that the test still binds the #1830
relaxation. Stubbing posix_compute_ancestor_overflow_uid() to
POSIX_NO_OVERFLOW_UID -- the relaxation removed -- and running as uid 1001:
tolerance off, new probe 48 passed, 1 failed (refused, exit 1)
tolerance on, new probe 49 passed, 0 skipped
so the smoke test is revert-sensitive to the production decision, not merely to
the test change. Worth recording: both deterministic #1830 tests PASSED with
the tolerance stubbed off, because they drive the test-seam override rather
than the real derivation. They bind the decision table; only this smoke test
binds the wiring.
macOS daemon_ipc 53 passed 1 skipped; linux/arm64 49 passed 0 skipped;
lint-ci clean.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The single-uid userns smoke test forked a child, unshared CLONE_NEWUSER,
wrote a 1:1 uid_map, then called cbm_daemon_ipc_private_directory_secure()
directly in that child. The call cannot give the right answer there.
posix_ancestor_overflow_uid() in src/daemon/ipc.c derives the overflow uid
once per process under pthread_once, and the comment above it calls the
/proc state "immutable". unshare(CLONE_NEWUSER) is precisely what changes
/proc/self/uid_map, and pthread_once state survives fork(). Seven earlier
call sites in this suite run the derivation first, so by the time the test
forks, the child has already inherited the HOST answer -- no overflow uid --
and refuses the directory no matter what its namespace says.
The test therefore asserted a product behaviour it never executed, and failed
as a product refusal (exit 1) while the product was correct: production
daemons start inside their namespace and derive the right answer on first use.
Re-exec /proc/self/exe into a new --userns-secure-probe mode so the decision
is made by a process that STARTED in the namespace, which is also the only
shape production ever takes. A failed exec gets its own exit code 3 and a
named FAIL, so a broken harness can never be misread as a quiet refusal.
Verified both directions on linux/arm64 in the container leg as uid 1001:
reverted, 48 passed / 1 failed on "expected WEXITSTATUS(status) == 1"; with
the fix, 49 passed, 0 skipped.
The non-root half of that recipe is now written down at the test, because it
is the part that fooled me first: as root the mapped range covers uid 0, so
root-owned /tmp stays 1:1 inside the namespace and never turns overflow. The
first run of this reproduction passed with the fix REVERTED for exactly that
reason. Only a non-root uid reaches the branch the test exists to cover.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
#2189 gave codeql-gate the security-events:read it needs to see code-scanning
alerts, and granted it at the two callers I knew about: pr.yml and release.yml.
There is a third. dry-run.yml calls the same reusable workflow and was left
granting only the workflow default, so every dispatch of it now fails at
startup -- before a single job runs, with no job output to explain why.
Observed immediately: run 34725401182 on 7d74337e, startup_failure.
The contract test did not catch it because it iterated a hardcoded
("pr.yml", "release.yml"). A test that checks only the callers you remembered
cannot catch the one you forgot, and it reports green while doing so -- the
same shape of false assurance as the 403-to-"0 alerts" bug this whole contract
exists to prevent.
So the list is now derived: every workflow containing
`uses: ./.github/workflows/_security.yml` must grant security-events:read and
actions:read, with a floor of three callers so a silent deletion is noticed
too. Reverting only dry-run.yml's grant now fails the test by name.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The codeql-gate has never checked anything. It declared no permissions block,
so it inherited the workflow's contents:read and the code-scanning alert API
answered 403. gh writes the API error BODY to stdout, so
ALERTS=$(gh api '.../code-scanning/alerts?state=open' --jq 'length' 2>/dev/null || echo "0")
left ALERTS holding
{"message":"Resource not accessible by integration",...,"status":"403"}0
every [ ... -gt ... ] then failed as a non-integer comparison, the if took its
false branch, and the step printed "CodeQL gate passed (0 alerts)" and exited 0.
Observed in the log of a GREEN run (34700514102, job 103571322660) while the
repository had an open high-severity alert. The `|| echo "0"` reads as defensive
and is the exact opposite: it converts "I cannot see the alerts" into "there are
no alerts", which is the one direction a security gate must never fail in.
Grant the job security-events:read (and actions:read, which the CodeQL-run poll
needs for the same reason), and replace the fallback with a helper that treats a
non-zero gh exit or a non-numeric body as a hard failure. The helper returns
rather than exits because it runs inside a command substitution, where an exit
would end only the subshell and let the caller continue with an empty count --
the same silent pass in a new costume; both call sites check the status.
tests/test_security_gate_fail_closed.sh pins both halves and fails with six
assertions against the previous workflow.
Also bumps graph-ui's vitest to ^4.1.11 (GHSA-82fw-gwwq-j7x9, path traversal via
@vitest/mocker, CVSS 5.9). Development scope, and graph-ui ships in no release
artifact, so nothing released was ever exposed -- but it was the sole open alert
and the gate above is worthless while it stands unresolved. Raising the floor
past the vulnerable range also stops a fresh install resolving back into it.
graph-ui: npm ci clean, 47 tests in 12 files pass on 4.1.11.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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 <martin.vogel.tech@gmail.com>
A reporter brought a full ProcMon trace to #1856 and still could not say which
check refused their install. That is not their failing: the binary discards the
answer before printing it.
cli_activation_production_context_init() validates the cache, rendezvous and
log directories -- it is the emitter MOST likely to hold a useful detail -- and
it printed CLI_ACTIVATION_REFUSED_MESSAGE bare, through the raw sink rather
than cli_activation_diagnostic(). So the reader got "Check the errors above"
with nothing above. That is the exact dead end #1416 and #1537 already fixed;
the property was repaired on the paths that had tests and left broken on this
sibling, which had none. Route it through the attributing helper: the refusal
now names the transaction refusal note or cbm_daemon_ipc_validation_detail().
The remedy was also wrong for the reported shape. `icacls <dir> /remove:g <sid>`
cannot remove an INHERITED ACE, and the stock C:\ grant for Authenticated Users
reaches every new child directory exactly that way -- so a reporter following
our advice watches the command succeed and the refusal persist. The refusal now
says whether the ACE was inherited, and the advice names
`icacls <dir> /inheritance:r /grant:r "%USERNAME%":(OI)(CI)F` for that case.
No safety check is relaxed. An install directory writable by other accounts is
still refused; it is now refused in terms the reader can act on.
The new contract test guards the CLASS rather than this one call site: any
emitter of a refusal constant that bypasses the attributing helper fails it,
which is what would have caught this. It fails on the previous tree naming
src/cli/cli.c:750, plus the missing inherited-ACE report and the missing
/inheritance:r remedy.
Local: cli 314 passed; activation_transaction daemon_ipc daemon_bootstrap
daemon_version 110 passed, 1 skipped (user namespaces are Linux-only, runs on
the Linux leg); make -f Makefile.cbm lint-ci clean.
Reported by zaferavci1.
Refs #1856
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
install.ps1 set its protocol bitmask to Tls12 -bor Tls13 unconditionally.
Windows 10's schannel has no TLS 1.3 -- it arrives with Windows 11 and Server
2022 (build 20348) -- but .NET Framework 4.8 still DEFINES the enum value, so
the assignment succeeds and nothing warns. The first HTTPS request then dies
with "The request was aborted: Could not create SSL/TLS secure channel",
because an unsupported flag in this bitmask is a hard failure rather than a
downgrade. Every Windows 10 user was blocked before a single byte downloaded,
on the documented `irm ... | iex` path, with an error that points at the
network rather than at the installer.
Compute the set instead: start at Tls12 and add Tls13 only on build 20348+.
The enum-name probe alongside the build gate keeps the script parsing on .NET
Framework 4.7, where the member does not exist at all.
Verified on the Windows VM (build 26200) with the shipped parser: PARSE_OK,
pure ASCII preserved, the guard computes and assigns "Tls12, Tls13" there,
forcing build 19045 through the same expression yields "Tls12" alone, and an
HTTPS HEAD to github.com under the assigned set returns 200.
The contract test fails on the previous installer with all four assertions --
unconditional bit, missing build gate, missing enum probe, literal bitmask --
and passes with this change.
Reported with a full ProcMon trace and a decoded SID by zaferavci1.
Refs #1856
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The per-file tree-sitter parse was bounded by a 5s WALL-CLOCK budget. Under CI
contention a worker thread can be descheduled long enough for 5s of wall time
to elapse against near-zero CPU, so a small, perfectly parseable file was
abandoned with a false "parse timeout" and yielded ZERO defs. When that file
defined a symbol other files resolve against (e.g. celery.app.Celery.send_task
backing an ASYNC_CALLS edge, or a userpb gRPC client backing GRPC_CALLS), the
cross-file service edge then failed to classify -- surfacing as the flaky
convergence_probe / edge_types_probe service-edge assertions (count 0), a
verdict decided by wall-clock scheduling rather than the input (violates O9,
"no timeout decides a test").
Gate the parse on per-thread CPU time instead, with a generous wall ceiling as
a backstop:
- New portable helper cbm_thread_cpu_time_ns() (compat.c/.h): POSIX/macOS
clock_gettime(CLOCK_THREAD_CPUTIME_ID), Windows GetThreadTimes (kernel+user);
returns 0 on clock failure, degrading safely to the wall ceiling. It is NOT
cbm_clock_gettime, which is wall on every platform (QueryPerformanceCounter
on Windows).
- The progress callback's payload becomes {cpu_deadline_ns, wall_ceiling_ns};
it trips when this thread's CPU time passes the budget OR wall time passes
t0 + budget * 12 (~60s for the 5s budget). Descheduling burns wall, not CPU,
so a starved-but-parseable file survives; a CPU-spinning parse still trips
the CPU deadline; a genuinely stuck parse still trips the wall ceiling
(bounded, no hang).
RED-repro (deterministic, CBM_ENABLE_TEST_SEAMS only): a thread-local wall-clock
offset armed by CBM_TEST_WALL_STALL_ON emulates a descheduled worker (wall
reads budget + 1s ahead while CPU is untouched -- above the old 1x wall budget,
below the ceiling). async_calls_celery_wall_stall_seam arms it for the Celery
DEFINITION file and asserts ASYNC_CALLS >= 1. With the fix the parse completes
and the edge resolves; with the budget reverted to wall-only + a 1x ceiling the
def file is dropped and the edge count is 0.
Verified (macOS, sanitized): edge_types_probe + convergence_probe 107 passed /
0 failed with the fix; 58 passed / 1 failed on revert (only the seam test, at
the ASYNC_CALLS assertion); 107 restored. Regression smoke (extraction
parse_coverage pipeline lang_contract grammar_regression) 698 passed / 0
failed. lint-ci clean (cppcheck + clang-format Homebrew LLVM + NOLINT); the
Windows GetThreadTimes path is compile-checked by CI (not runnable on macOS).
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
A burst of one-shot clients (e.g. six parallel `cli list_projects`) shares one
ephemeral daemon generation and races to connect. The generation retired the
instant its last committed client disconnected, even when another one-shot's
connection was already accepted and still mid-HELLO -- stranding that racer on
a STOPPING daemon and forcing a full cold respawn (up to the 12s daemon-claim
handoff each). Under load the retire/respawn churn could exhaust a client's
30s bootstrap budget, surfacing as the Windows guard's "cold-storm client
failed (racing daemon spawn)" regression and, on POSIX, a startup that never
reached "serving".
When the last committed client of an ephemeral, non-permanent generation
disconnects, linger a bounded window if a peer connection is already accepted
but has not yet passed HELLO admission (in_use, connected, not admitted, not
disconnecting -- runtime_has_pending_hello_peer_locked). The accept loop's
reconcile retires the generation once that racer drains without committing or
the bounded window (RUNTIME_EPHEMERAL_LINGER_MS, seam-overridable) elapses; a
racer that does commit clears the linger. With no such racer -- the common
single-client case -- the generation retires immediately, unchanged.
The linger deliberately covers ONLY a pre-HELLO racer, never an already-
admitted provisional session (HELLO done, application session opening): the
contract that a provisional coordinator client cannot keep a retiring
generation alive is preserved (daemon_runtime_final_disconnect_rejects_blocked_
provisional_session). The daemon cannot count cross-process cohort peers
(advisory locks expose presence, not a holder count, and Windows LockFileEx has
no non-owning probe), so this uses the one peer signal it has locally: accepted
connections not yet admitted. The window is bounded, so a racer that never
commits cannot wedge the generation (rules out the unbounded idle hang).
Coordinator gains a transient `linger` hold (distinct from `permanent`) so its
own last-client self-transition to STOPPING is suppressed while the racer is
outstanding; clearing it while idle retires the coordinator at once.
Verified (macOS, sanitized build): daemon suites 224 passed / 0 failed / 1
skipped (the skip is the pre-existing Linux-only userns case). New tests pin
both directions -- daemon_runtime_ephemeral_lingers_while_peer_mid_hello
(a raw accepted-but-unHELLO'd peer keeps the generation RUNNING after the
committed client leaves) and daemon_runtime_ephemeral_linger_retires_within_bound
(the bounded backstop retires even while the peer is still accepted). RED on
revert: neutering the gate flips ASSERT(lingered) at test_daemon_runtime.c and
nothing else. clang-format (Homebrew LLVM) + cppcheck + NOLINT clean.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Windows install/daemon runtime directories created by an elevated process are
owned by the built-in Administrator ACCOUNT (RID 500 under the local machine
account-domain SID), even when that account is renamed or disabled. cbm's
directory-owner/ACL trust refused it, breaking install/activation on such
machines.
#1705: resolve THIS machine's built-in Administrator SID via LSA
(LsaOpenPolicy(POLICY_VIEW_LOCAL_INFORMATION) +
LsaQueryInformationPolicy(PolicyAccountDomainInformation) ->
CreateWellKnownSid(WinAccountAdministratorSid, accountDomainSid)), cached once
per process, and accept it as a trusted owner/grantee in the daemon
(win_sid_trusted) and the CLI activation transaction. Resolution is
deliberately NOT IsWellKnownSid(WinAccountAdministratorSid) and NOT a
trailing-RID-500 match: both accept a FOREIGN S-1-5-21-*-500 (a domain
administrator, or another machine's built-in Administrator), which would be a
cross-machine trust escalation. Any LSA or synthesis failure caches "none" and
grants no tolerance (fail closed). owner-not-current-user (the exact-owner
target rule) is unchanged.
#2023/#1686: an owner-trust refusal now names the offending owner SID (the path
was already carried), so a terse "status -3, os 0" becomes actionable.
Scope notes: on a domain controller -- which has no separate local SAM, so its
account domain IS the AD domain -- this trusts the domain Administrator, by
design (on a DC that account is the machine-equivalent top admin). An
un-sysprepped cloned Windows image shares the machine account-domain SID, so a
sibling clone's built-in Administrator would be non-foreign; this is inherent
to machine-SID identity.
Verified on a native Windows (arm64) VM: compiles under the sanitized build;
the daemon_ipc SID-trust test passes (local RID-500 trusted, a fabricated
foreign S-1-5-21-*-500 rejected, BUILTIN\Administrators trusted) at 35 passed /
0 failed with the fix, and 34 passed / 1 failed (ASSERT(local_ok)) with the
trust clause reverted -- the test binds precisely to the fix. #1856 (stock
ancestor ACEs): the Windows ancestor-ACL guards already pass; no defect
reproduced, no code change.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Inside a 1:1 user namespace (podman --userns=keep-id, unshare -U
--map-current-user), the root-owned ancestors /, /tmp appear owned by the
kernel overflow uid; the daemon's ancestor-ownership walk refused every
overflow-owned ancestor, making the daemon unusable in that legitimate setup.
Tolerate an overflow-owned ancestor ONLY when /proc/self/uid_map is a
single-uid map whose sole inside id is our euid AND /proc/sys/kernel/overflowuid
is readable (never hardcoded; unreadable => no tolerance). No environment
escape hatch. Group-writable overflow ancestor refused (host group unknown);
world-writable overflow tolerated only with the sticky bit (the /tmp shape).
The created/final/snapshot gates stay strict == geteuid(), so a host-squatted
overflow-owned private directory is still refused; the tolerance only widens
which ancestors the walk traverses. macOS byte-identical (overflow=-1).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cbm_remove_indexes unlinked each *.db and *.db.tmp but never the SQLite
sidecars (<db>-wal/-shm/-journal), so `cbm update`, `cbm uninstall`, and
`install --reset-indexes` (the four callers of the single deletion
chokepoint) left orphan -wal/-shm files behind. Call the existing
wide-path-safe cbm_remove_db_sidecars() helper on both the live and staged
db paths after the .db unlink; it is idempotent and ENOENT-tolerant, so an
orphan -wal that outlived its .db is still cleaned. Sidecars are not
indexes, so the removed-count is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
The #1828 fail-fast test asserted `elapsed_ms < 5000`, a wall-clock budget
that flaked on the ASan test-diag lane under CI contention while the fail-fast
mechanism itself engaged correctly: the whole daemon_bootstrap suite ran in
11 s, so the test never reached the 30 s slow-timeout path -- it only overran
the 5 s wall-clock budget. Per O9 a gate never asserts a transient timing
window, and a rerun is not a fix.
Drop the wall-clock measurement and rely on the deterministic mechanism the
test already observes: the client surfaces the recorded ENOSPC cause verbatim
("failed to start" + errno + path), the slow "active or starting" timeout
wording is absent, and exactly one daemon is spawned. That "failed to start"
message is produced only on the fast-fail break
(cbm_daemon_bootstrap_start_failure_format), never on the 30 s deadline path,
so these assertions bind the fast-fail contract without any timing.
Verified RED-on-revert: defeating the fast-fail detection makes the client
hang to the 30 s deadline and report "active or starting", tripping
ASSERT_FALSE(stale_wording). Full daemon_bootstrap suite green (28/28) under
the ASan/UBSan build.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
cli_fd_capture_begin/end use POSIX fd redirection (dup/dup2/tmpfile) and
are called only by the uninstall tests, which are themselves #ifndef
_WIN32. Defined at top level, the helpers were compiled but unused on
Windows, tripping -Werror,-Wunused-function on the test-windows CLANG64
leg. Wrap the struct and both helpers in #ifndef _WIN32 to match their
callers. Test-only; no production change.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
create_staging_path() minted the stage's main file with mkstemp and only then
took its sidecar .lock via stage_owner_register(). A second, concurrent
cbm_pipeline_run() against the same final_path (auto_index, or the
stale-rendezvous-recovery retry path) could land its sweep_orphan_stages() in
that window: the main file was visible but not yet locked, so the sweep treated
it like a genuinely orphaned stage and deleted it out from under the live writer
(POSIX), or collided on the not-yet-created sidecar and surfaced EACCES/errno=13
(Windows). That is the #2111 red on test-windows-guards and both test-windows
CLANG64 x86_64 shards (index_format / simhash / index_resilience, with
pipeline.stage action=lock_failed errno=13).
Fix: mint the six-char alphanumeric suffix in create_staging_path() itself,
create and hold the sidecar lock FIRST, then create the main file with O_EXCL.
The stage's main file is therefore never present on disk without its lock
already held, so a racing sweep can only ever find the stage lock-held: it
reaches the stage through the .lock entry too (stage_entry_stage_length()
matches it), probes the lock, sees a live holder, and keeps it. The
create->register TOCTOU is closed by construction.
#1839 is preserved: sweep_one_stage() still removes a stage whose sidecar is
absent (ENOENT). With live stages now always carrying their sidecar, ENOENT is
only ever a genuine pre-lock-era orphan minted by an older binary, which #1839
requires be swept.
stage_owner_register() becomes stage_owner_adopt(), recording the already-held
descriptor; re-taking the lock would self-conflict, since flock() and Windows
_SH_DENYRW deny a second acquire of the same sidecar even from this process.
A new after_stage_created test seam and pipeline_concurrent_sweep_must_not_-
remove_inflight_stage drive a real concurrent sweep in that window; the test is
RED without the lock-first ordering.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
On Windows, install registers the install directory in the persistent
HKCU\Environment\Path, but uninstall never removed it — so every
install/uninstall cycle left its entry behind and the user PATH grew
without bound (a real test host reached ~8000 chars, emptying the
effective PATH and breaking tooling).
The reported non-ASCII "mojibake" is historical, not a current-code bug.
Both install paths already write the value through the UTF-16 registry
API: install.ps1 (shipped in each release archive) runs `cbm install`,
which calls RegSetValueExW(L"Path", ...) — git history shows no ANSI
(RegSetValueExA / SetEnvironmentVariableA) predecessor — and
setup-windows.ps1 writes via .NET [Environment]::SetEnvironmentVariable
(Unicode) with an ASCII-only install dir. The only path that handles a
non-ASCII directory is the wide-correct C path, so current code cannot
produce a garbled entry; the garbage the reporter saw was left by an
older binary. No code change is needed for the encoding itself.
The real defect is the missing uninstall-side removal. This adds
cli_remove_windows_user_path(), the counterpart to
cli_ensure_windows_user_path(): it opens the same key, walks the
';'-separated segments, and drops exactly the ones equal to the install
dir using the same case- and trailing-separator-insensitive comparison
the install `present` scan uses, so it removes precisely what install
would have de-duplicated. Every other segment is preserved
byte-for-byte, the value kind (REG_SZ / REG_EXPAND_SZ) is preserved, and
a registry error is a warning rather than a hard failure so a user is
never blocked from finishing an uninstall over a cosmetic PATH edit.
Removing the entry on uninstall also stops accumulation on reinstall: a
garbled historical entry defeats the install de-dup and gets re-added,
so taking our own entry back out keeps the value bounded.
Testing:
- tests/test_cli.c: a hermetic Windows unit test drives the append and
remove logic against a GUID-scoped scratch key via the existing
CBM_TEST_WINDOWS_USER_PATH_RUN_ID seam, never the developer's live
HKCU\Environment\Path. A non-ASCII install dir doubles as a mojibake
guard (a broken UTF-8->UTF-16 round-trip would fail the match and leak
a duplicate). It asserts append, idempotent reinstall, exact removal
from the tail and the middle, idempotent removal, and byte-for-byte
survival of unrelated entries.
- tests/test_smoke_fixture_contract.sh: a macOS-runnable source contract
that fails when the removal is absent and passes with it, proving the
wiring binds (verified RED-on-revert).
The Windows-only registry code is #ifdef _WIN32; its compile and the
behavioral unit test are verified by CI's Windows leg (the local Windows
VM is down). Pre-existing mojibaked entries from old binaries are a
separate one-time cleanup, out of scope here, and setup-windows.ps1's
own lack of an uninstall mode is noted as a possible follow-up.
Fixes#2117
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
install wrote only part of the OpenHands config. Register the server globally
under ~/.openhands/settings.json -> mcp_config {transport:stdio, command, enabled}
(owned-entry, unrelated content preserved), and add "codebase-memory-mcp" to
each ~/.openhands/agent-profiles/<p>.json mcp_server_refs array (null -> [it],
existing -> appended once). uninstall removes exactly what it added.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
auto_index_limit was bypassed for a non-git root because the admission guard
counted `git ls-files`, which is 0 outside a repo, so the whole tree was
walked into tens of GB of RAM. Bound the check by the discovery layer's
bounded count with the limit as the ceiling; a non-git root over the limit is
refused with the limit named, and git roots are unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
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 <noreply@anthropic.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Anton Standrik