Fix the riscv64 test sweep: hermetic collapse tests, unique sandbox names - #172
Open
congwang-mk wants to merge 5 commits into
Open
Fix the riscv64 test sweep: hermetic collapse tests, unique sandbox names#172congwang-mk wants to merge 5 commits into
congwang-mk wants to merge 5 commits into
Conversation
The collapse test assumed the four /usr/bin files it cats share a parent directory after learn canonicalizes them. On hosts where coreutils are symlinks into a multicall link farm (Ubuntu rust-coreutils on riscv64, issue #170), cat/ls/env resolve into /usr/lib/cargo/bin/coreutils/ while sh resolves to dash, so no directory reaches the N=4 threshold, the profile keeps individual file grants, and run's cat /usr/bin/true is denied because its canonical target was never observed. Collapse over files the test creates itself so the observed paths canonicalize to the same parent on every filesystem layout; reading a fifth, unobserved file still proves the directory grant is what admits it. Signed-off-by: Cong Wang <cwang@multikernel.io>
Contributor
Author
|
@ghazariann Please take a look. Thanks! |
test_config_toml_flag_produces_toml timed out on riscv64 waiting for its sandbox to show up in ps, while identical spawns in sibling tests passed the same run. The harness gave no way to tell why: spawn_sandbox discarded the child's stderr and wait_for_sandbox polled ps blindly, so a sandbox that died at startup and one that was merely slow produced the same bare timeout message. Capture stderr, fail fast with the exit status and stderr when the child dies before appearing, and raise the poll ceiling from 5s to 15s so parallel sandbox startups on slow boards are not misreported as failures. If the riscv64 failure recurs, the panic now carries the evidence needed to diagnose it. Signed-off-by: Cong Wang <cwang@multikernel.io>
test_collapse_threshold_reads fails on the rust-coreutils riscv64 host for the same reason as the integration collapse test: learn canonicalizes observed paths, so the four /usr/bin names scatter across /usr/bin (raw exec paths), /usr/bin/dash, and the multicall link farm, and no directory reaches the N=4 threshold. test_collapse_prefix_forces_collapse passes there only because a raw exec path happens to remain under /usr/bin; if exec recording ever canonicalizes fully, it breaks the same way. Point both tests at a tempdir the test populates itself, so observed paths share a canonical parent on every filesystem layout. The prefix test keeps its file count below the threshold so only --collapse-prefix can produce the directory grant. Signed-off-by: Cong Wang <cwang@multikernel.io>
Since names started claiming a per-UID runtime dir with a hard failure on live collision, every internal fixed name became a global mutex: two concurrent pipelines collide on 'pipeline-stage-0', transactions on 'txn-stage-0', gathers on 'gather-consumer', and single-stage runs on 'stage'. A parallel test run surfaces this immediately (36 core integration tests fail), but any library embedder running two pipelines at once hits the same wall. Factor the auto-name generator's <pid>-<counter> suffix into unique_instance_id() and stamp it into every internally assigned name; one run id is shared across a pipeline, gather, or transaction so its stages remain recognizable as one unit in ps. Signed-off-by: Cong Wang <cwang@multikernel.io>
The 'test', 't', and 'mybox' names are used by several integration tests each; with names now claiming a per-UID runtime dir, any two of those tests running in parallel collide. Drop the name where it carried no meaning (the auto-generated name is unique), and give the hostname-virtualization tests, which must know their name to assert the virtual hostname, names that are distinct per test and suffixed with the process id so concurrent test binaries cannot collide either. Signed-off-by: Cong Wang <cwang@multikernel.io>
congwang-mk
force-pushed
the
fix/learn-collapse-portability
branch
from
July 28, 2026 23:21
bb45e33 to
036d86a
Compare
Contributor
Learn test changes look reasonable to me, private tempdirs over hardcoded |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes for every failure from the riscv64 (rust-coreutils, uutils multicall) test sweep. One is a real product bug the sweep exposed; the rest are test portability issues.
1. Product bug: internal fixed sandbox names are a global mutex
Since the control-socket work made every sandbox name claim a runtime dir under
/dev/shm/sandlock-$UID/with a hard failure on live collision, every internally assigned fixed name became a process-wide (in fact UID-wide) mutex: two concurrent pipelines collide onpipeline-stage-0, transactions ontxn-stage-0, gathers ongather-consumer, single-stage runs onstage. A parallel test run surfaces this immediately (36 core integration tests fail, reproduced on x86_64 with default test parallelism), but any embedder running two pipelines at once hits the same wall.Fix: factor the auto-name generator's
<pid>-<counter>suffix intounique_instance_id()and stamp it into every internally assigned name. One run id is shared across a pipeline / gather / transaction so its stages remain recognizable as one unit inps(e.g.pipeline-<pid>-<n>-stage-0).Core integration tests also stopped sharing the fixed names
test,t, andmyboxacross parallel tests: dropped where meaningless, made per-test and pid-suffixed where the hostname assertion needs a known name.2.
test_learn_then_run_collapse(issue #170)The test assumed the four
/usr/binfiles it cats share a parent directory aftersandlock learncanonicalizes observed paths. On the rust-coreutils host,cat/ls/envresolve into/usr/lib/cargo/bin/coreutils/whileshresolves to dash, so no directory reaches the N=4 collapse threshold, the profile keeps individual file grants, andrun'scat /usr/bin/trueis denied because its canonical target was never observed. Sandlock behaves as designed (the--mergehint is the intended UX); the bug is the test's dependence on host layout.Fix: the test creates its own tempdir with four observed files plus one unobserved file, learns with a pinned
--collapse=4, and runscaton the unobserved file, which is only readable through the collapsed directory grant.3.
test_collapse_threshold_readsandtest_collapse_prefix_forces_collapseUnit-level twins of the same root cause, made hermetic the same way. The prefix test passed on riscv64 only because a raw exec path happens to remain under
/usr/bin; it now keeps its file count below the threshold so only--collapse-prefixcan produce the directory grant.4.
test_config_toml_flag_produces_tomltimeoutTimed out waiting for its sandbox to appear in
pswhile identical sibling spawns passed. The harness discarded the child's stderr and polled blindly with a hard 5s budget, so a startup death and slowness were indistinguishable. Fix: capture stderr, fail fast with exit status and stderr if the child dies, raise the ceiling to 15s. Instrumentation plus margin, not a root-cause fix; a recurrence now produces discriminating evidence.The SIGPIPE and transaction stderr-tee fixes were split into #173.
Fixes #170
Test plan
cargo test --releasefor sandlock-core (lib 675 + integration 389, default parallelism), sandlock-cli (82), sandlock-ffi: all pass🤖 Generated with Claude Code