diff --git a/.github/actions/setup-gusset/action.yml b/.github/actions/setup-gusset/action.yml index 8df1f023..960329f8 100644 --- a/.github/actions/setup-gusset/action.yml +++ b/.github/actions/setup-gusset/action.yml @@ -25,7 +25,7 @@ runs: # Pinned to the sibling revision this tree was linked against. # Bump deliberately; tracking main would let a Gusset push fail a # DevCouncil release without a DevCouncil commit. - ref: 7a56c3c98d026b4e445f687140c90d70755e8590 + ref: 709c2b7b85be0de7c263471d933f395bb3b1b27f path: third_party/gusset persist-credentials: false @@ -60,45 +60,25 @@ runs: test -f "$dest/go.mod" test -f "$dest/crates/gusset/Cargo.toml" - # Gusset's cgo line is -lgusset -lpthread -lm -ldl. That is a GNU link. - # windows-latest's Rust host is MSVC, which emits gusset.lib and has no - # libdl. The gnu target plus MinGW produces libgusset.a, which is what - # the #cgo -L path in gussetfn/engine.go already names. - - name: Install MinGW - if: ${{ runner.os == 'Windows' && inputs.build-engine == 'true' }} - uses: msys2/setup-msys2@v2 - with: - msystem: MINGW64 - update: false - install: mingw-w64-x86_64-gcc mingw-w64-x86_64-dlfcn - path-type: inherit - + # gusset is unix-only: its crate refuses other targets with compile_error!, + # so no Windows toolchain can build the archive. On Windows the sibling is + # still checked out (go.mod's replace must resolve) and the host builds + # without it — gussetfn is build-tagged unix and answers ErrUnsupported + # elsewhere. The MinGW/gnu-target path this replaced could never succeed. - name: Rust toolchain if: ${{ runner.os != 'Windows' && inputs.build-engine == 'true' }} uses: dtolnay/rust-toolchain@stable - - name: Rust toolchain (Windows gnu target) - if: ${{ runner.os == 'Windows' && inputs.build-engine == 'true' }} - uses: dtolnay/rust-toolchain@stable - with: - targets: x86_64-pc-windows-gnu - - name: Build the umbrella staticlib - if: ${{ inputs.build-engine == 'true' }} + if: ${{ runner.os != 'Windows' && inputs.build-engine == 'true' }} shell: bash working-directory: rust/gusset-engine run: | set -euo pipefail - if [[ "${RUNNER_OS}" == "Windows" ]]; then - export CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=gcc - cargo build --release --locked --target x86_64-pc-windows-gnu - mkdir -p target/release - cp -f target/x86_64-pc-windows-gnu/release/libgusset.a target/release/libgusset.a - # Later `go build` steps in this job must use the same gcc. cgo - # will not use the MSVC linker for Gusset's -lpthread/-ldl line. - echo "CC=gcc" >> "$GITHUB_ENV" - echo "CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=gcc" >> "$GITHUB_ENV" - else - cargo build --release --locked - fi + cargo build --release --locked test -f target/release/libgusset.a + # Key Go's build and test caches on the archive (see cgo-env.sh): + # setup-go restores GOCACHE, and without this a cached link or a + # cached "ok" from an older archive is replayed. + # The -L is already in gussetfn/engine.go; only the key is needed here. + ./cgo-env.sh --no-build | grep -v "^CGO_LDFLAGS=" >> "$GITHUB_ENV" diff --git a/.github/workflows/analysis-plane.yml b/.github/workflows/analysis-plane.yml index 7d1f383c..a4af9158 100644 --- a/.github/workflows/analysis-plane.yml +++ b/.github/workflows/analysis-plane.yml @@ -24,11 +24,13 @@ on: - "rust/**" - "backend/go_orchestrator/**" - ".github/workflows/analysis-plane.yml" + - ".github/actions/setup-gusset/**" pull_request: paths: - "rust/**" - "backend/go_orchestrator/**" - ".github/workflows/analysis-plane.yml" + - ".github/actions/setup-gusset/**" workflow_dispatch: permissions: diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 13edd060..daf98630 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -20,6 +20,9 @@ on: - "rust/Cargo.lock" - "rust/.gitignore" - ".github/workflows/rust.yml" + - ".github/actions/setup-gusset/**" + - "rust/gusset-engine/**" + - "backend/go_orchestrator/gussetfn/**" pull_request: paths: - "rust/devmap-*/**" @@ -31,6 +34,9 @@ on: - "rust/Cargo.lock" - "rust/.gitignore" - ".github/workflows/rust.yml" + - ".github/actions/setup-gusset/**" + - "rust/gusset-engine/**" + - "backend/go_orchestrator/gussetfn/**" workflow_dispatch: permissions: diff --git a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go index 0e6ee85d..79b79b9e 100644 --- a/backend/go_orchestrator/cmd/devcouncil/gusset_check.go +++ b/backend/go_orchestrator/cmd/devcouncil/gusset_check.go @@ -1,3 +1,5 @@ +//go:build unix + package main import ( @@ -6,15 +8,24 @@ import ( "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/console" "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/gussetfn" + "github.com/bharathvbcr/gusset" ) func runGussetCheck() int { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - if err := gussetfn.Check(ctx); err != nil { + err := gussetfn.SelfTest(ctx) + // The check panics on purpose; what Rust logged about it, and anything + // else, belongs on stderr rather than in a ring nobody reads. + if _, drainErr := gussetfn.DrainLogs(console.Stderr()); drainErr != nil { + console.Errorf("gusset-check: draining Rust logs: %v\n", drainErr) + } + if err != nil { console.Errorf("gusset-check: %v\n", err) return 1 } - console.Println("gusset-check: ok") + st := gusset.Stats() + console.Printf("gusset-check: ok (parity, match-any, panic firewall; rust live=%dB peak=%dB allocs=%d; go threads=%d)\n", + st.LiveBytes, st.PeakBytes, st.AllocCount, gusset.Threads()) return 0 } diff --git a/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go b/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go new file mode 100644 index 00000000..22a7b5aa --- /dev/null +++ b/backend/go_orchestrator/cmd/devcouncil/gusset_check_other.go @@ -0,0 +1,15 @@ +//go:build !unix + +package main + +import ( + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/console" + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/gussetfn" +) + +// runGussetCheck says the check could not run: gusset is unix-only. A check +// that could not run exits non-zero, never as a pass. +func runGussetCheck() int { + console.Errorf("gusset-check: %v\n", gussetfn.ErrUnsupported) + return 2 +} diff --git a/backend/go_orchestrator/gate/redirect_test.go b/backend/go_orchestrator/gate/redirect_test.go index 6b62f4bc..7749615d 100644 --- a/backend/go_orchestrator/gate/redirect_test.go +++ b/backend/go_orchestrator/gate/redirect_test.go @@ -232,7 +232,12 @@ func runInThrowawayTree(t *testing.T, command string) []string { t.Fatal(err) } before := treeSnapshot(root) - cmd := exec.Command("sh", "-c", command) + // `wait` so a backgrounded write (`echo hi > .env &`) lands before the + // tree is read. Without it sh exited first, the write was reported as + // nothing, and it landed during t.TempDir's cleanup, which then failed + // with "directory not empty" (macOS, 241b018). Every corpus entry is a + // complete command, so a trailing line cannot change how it parses. + cmd := exec.Command("sh", "-c", command+"\nwait") cmd.Dir = root // A corpus entry that hangs would wedge the suite; nothing here reads // stdin, and closing it turns any that starts to into an immediate EOF. @@ -250,6 +255,17 @@ func runInThrowawayTree(t *testing.T, command string) []string { return changed } +// A backgrounded write is a write. The corpus holds `echo hi > .env &`, and +// the invariant above only means something for it if its write is seen. +func TestThrowawayTreeSeesABackgroundedWrite(t *testing.T) { + for i := 0; i < 20; i++ { + written := runInThrowawayTree(t, "sleep 0.05; echo hi > .env &") + if len(written) != 1 || written[0] != ".env" { + t.Fatalf("run %d: saw %v, want [.env]", i, written) + } + } +} + func treeSnapshot(root string) map[string]string { out := map[string]string{} _ = filepath.Walk(root, func(p string, info os.FileInfo, err error) error { diff --git a/backend/go_orchestrator/gussetfn/boundary_test.go b/backend/go_orchestrator/gussetfn/boundary_test.go new file mode 100644 index 00000000..507fd25f --- /dev/null +++ b/backend/go_orchestrator/gussetfn/boundary_test.go @@ -0,0 +1,494 @@ +//go:build unix + +package gussetfn + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" + "time" + "unicode/utf8" + + "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/fnmatch" + "github.com/bharathvbcr/gusset" +) + +// parityRows loads the CPython fnmatch table dc-glob and fnmatch both pin. +func parityRows(t testing.TB) [][3]string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("cannot locate boundary_test.go") + } + path := filepath.Join(filepath.Dir(file), "..", "..", "..", "testdata", "fnmatch-parity.tsv") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read parity fixture: %v", err) + } + var rows [][3]string + for _, line := range strings.Split(string(raw), "\n") { + if strings.HasPrefix(line, "#") || strings.TrimSpace(line) == "" { + continue + } + cols := strings.Split(line, "\t") + if len(cols) != 3 { + t.Fatalf("malformed fixture row %q", line) + } + rows = append(rows, [3]string{cols[0], cols[1], cols[2]}) + } + if len(rows) < 800 { + t.Fatalf("only %d fixture rows loaded", len(rows)) + } + return rows +} + +// The CPython table, through the boundary. dc-glob's own test reads it +// in-process; this reads it through cgo, the ring and the frame codec, which +// is the path a host actually takes. +func TestParityFixtureAcrossTheBoundary(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + for _, row := range parityRows(t) { + pattern, name, want := row[0], row[1], row[2] == "true" + got, err := Match(ctx, pattern, name) + if err != nil { + t.Fatalf("Match(%q, %q): %v", pattern, name, err) + } + if got != want { + t.Errorf("Match(%q, %q) = %v across the boundary, CPython says %v", pattern, name, got, want) + } + if goGot := fnmatch.Match(pattern, name); goGot != got { + t.Errorf("gusset %v and Go fnmatch %v disagree on (%q, %q)", got, goGot, pattern, name) + } + } +} + +// MatchAny over the fixture's patterns agrees with the Go reference for each +// name, including lists that must be split over several frames. +func TestMatchAnyAgreesWithGoAcrossFrames(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + rows := parityRows(t) + patterns := make([]string, 0, len(rows)) + for _, r := range rows { + patterns = append(patterns, r[0]) + } + for _, list := range [][]string{patterns[:1], patterns[:40], patterns} { + if _, n := encodeAny("x", list); len(list) == len(patterns) && n >= len(list) { + t.Fatal("the full list fit one frame; the split path is not exercised") + } + for _, r := range rows[:200] { + got, err := MatchAny(ctx, list, r[1]) + if err != nil { + t.Fatalf("MatchAny(%d patterns, %q): %v", len(list), r[1], err) + } + if want := fnmatch.MatchAny(list, r[1]); got != want { + t.Errorf("MatchAny(%d patterns, %q) = %v, Go says %v", len(list), r[1], got, want) + } + } + } +} + +func TestEncodeAnyStaysUnderTheInlineLimit(t *testing.T) { + long := strings.Repeat("a", maxField) + list := []string{long, long, long, long, long} + name := strings.Repeat("b", maxField) + for rest := list; len(rest) > 0; { + frame, n := encodeAny(name, rest) + if n == 0 { + t.Fatal("encodeAny made no progress") + } + if len(frame) > maxFrame { + t.Fatalf("frame of %d bytes exceeds %d", len(frame), maxFrame) + } + rest = rest[n:] + } + many := make([]string, maxPatterns+10) + if _, n := encodeAny("", many); n > maxPatterns { + t.Fatalf("frame carries %d patterns, cap is %d", n, maxPatterns) + } +} + +func TestMatchAnyRefusesBeforeTheCall(t *testing.T) { + ctx := context.Background() + long := strings.Repeat("a", maxField+1) + if _, err := MatchAny(ctx, []string{"*", long}, "a"); err == nil { + t.Fatal("an overlong pattern late in the list was accepted") + } + if _, err := MatchAny(ctx, []string{"*"}, long); err == nil { + t.Fatal("an overlong name was accepted") + } + if _, err := MatchAny(nil, []string{"*"}, "a"); err == nil { + t.Fatal("a nil context was accepted") + } + cctx, cancel := context.WithCancel(ctx) + cancel() + if _, err := MatchAny(cctx, nil, "a"); !errors.Is(err, context.Canceled) { + t.Fatalf("empty list on a cancelled context = %v", err) + } +} + +// A malformed pattern after a matching one is still an error: the engine +// validates the frame before matching, so the answer cannot depend on order. +func TestMatchAnyInvalidUTF8IsAnErrorNotAPoison(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err := MatchAny(ctx, []string{"*.py", "\xff"}, "a.py") + if err == nil || !strings.Contains(err.Error(), "UTF-8") { + t.Fatalf("got %v, want a UTF-8 error", err) + } + if poisoned(err) { + t.Fatalf("invalid UTF-8 poisoned the handle: %v", err) + } + if got, err := MatchAny(ctx, []string{"*.py"}, "a.py"); err != nil || !got { + t.Fatalf("next MatchAny = %v, %v", got, err) + } +} + +// A poisoned shared handle is retired and the next call opens a new one. +// Before, the handle was held by a sync.Once and every later call failed. +func TestPoisonedHandleIsReplaced(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := engine(); err != nil { + t.Fatal(err) + } + bad, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + t.Fatal(err) + } + if _, err := bad.Call(ctx, nil); !errors.Is(err, gusset.ErrPanic) { + t.Fatalf("self-test panic = %v", err) + } + prev := current.Swap(bad) + if prev != nil { + defer prev.Close() + } + + // The poison was a sibling's, so the call retires the handle and answers + // from the replacement instead of returning ErrPoisoned. + got, err := Match(ctx, "*.py", "a.py") + if err != nil || !got { + t.Fatalf("Match on the poisoned handle = %v, %v; want a retried match", got, err) + } + if h := current.Load(); h == bad || h == nil { + t.Fatal("the poisoned handle was not replaced") + } +} + +// This frame's own panic is not retried: the retry would poison the +// replacement with the same input. +func TestOwnPanicIsNotRetried(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + before := opens.Load() + _, err := call(gusset.ContextWithOpcode(ctx, opcodeSelfTestPanic), nil) + if !errors.Is(err, gusset.ErrPanic) { + t.Fatalf("call = %v, want ErrPanic", err) + } + if n := opens.Load() - before; n > 1 { + t.Fatalf("%d handles opened for one panicking frame", n) + } + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("Match after a panicking frame = %v, %v", got, err) + } +} + +// Many goroutines hit a handle that becomes poisoned under them. Every call +// must still succeed — the poison was another call's, and call retries once +// on the replacement — and exactly one replacement is opened per poisoning. +func TestPoisonUnderConcurrencyRecovers(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + for round := 0; round < 5; round++ { + if _, err := engine(); err != nil { + t.Fatal(err) + } + bad, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + t.Fatal(err) + } + _, _ = bad.Call(ctx, nil) + if prev := current.Swap(bad); prev != nil { + _ = prev.Close() + } + before := opens.Load() + var wg sync.WaitGroup + errs := make(chan error, 64*10) + for g := 0; g < 64; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 10; i++ { + got, err := Match(ctx, "*.py", "a.py") + if err != nil { + errs <- err + continue + } + if !got { + errs <- errors.New("match returned false") + } + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Errorf("round %d: %v", round, err) + } + if n := opens.Load() - before; n != 1 { + t.Fatalf("round %d: %d handles opened to replace one poisoned handle", round, n) + } + } +} + +// An opcode already on the caller's context must not reach Match's frame: +// opcode 1 decoded it as a match-any frame, the self-test opcode panicked +// the shared handle. +func TestMatchIgnoresAnOpcodeOnTheContext(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + for _, op := range []uint32{opcodeMatchAny, opcodeSelfTestPanic} { + got, err := Match(gusset.ContextWithOpcode(ctx, op), "*.py", "a.py") + if err != nil || !got { + t.Fatalf("Match under opcode %#x = %v, %v", op, got, err) + } + } + if h := current.Load(); h == nil { + t.Fatal("no shared handle") + } + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("shared handle after opcode contexts: %v, %v", got, err) + } +} + +// A pattern in a later frame with bad UTF-8 is an error even when the first +// frame matches: validation is over the whole list, not per frame. +func TestMatchAnyValidatesAcrossFrames(t *testing.T) { + long := strings.Repeat("a", maxField) + list := []string{"*"} + for i := 0; i < 8; i++ { + list = append(list, long) + } + list = append(list, "\xff") + if _, n := encodeAny("x", list); n >= len(list) { + t.Fatal("the list fit one frame; the case is not exercised") + } + if _, err := MatchAny(context.Background(), list, "x"); err == nil || !strings.Contains(err.Error(), "UTF-8") { + t.Fatalf("got %v, want a UTF-8 error", err) + } +} + +func TestCancellationDoesNotPoison(t *testing.T) { + for i := 0; i < 200; i++ { + ctx, cancel := context.WithTimeout(context.Background(), time.Duration(i%7)*time.Microsecond) + _, err := MatchAny(ctx, []string{"*.rs", "*.py"}, "a.py") + cancel() + if err != nil && poisoned(err) { + t.Fatalf("iteration %d: a deadline poisoned the handle: %v", i, err) + } + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if got, err := Match(ctx, "*.py", "a.py"); err != nil || !got { + t.Fatalf("Match after cancellations = %v, %v", got, err) + } +} + +// DrainLogs terminates on an idle ring and reports the bytes it wrote. It +// does not prove anything was logged: gusset's ring receives no line for a +// caught panic (that goes to FfiStatus), so there is nothing reliable to +// drain here. +func TestDrainLogsTerminatesAndCounts(t *testing.T) { + var buf bytes.Buffer + if _, err := DrainLogs(&buf); err != nil { + t.Fatal(err) + } + buf.Reset() + n, err := DrainLogs(&buf) + if err != nil || n != buf.Len() { + t.Fatalf("DrainLogs = %d, %v with %d bytes written", n, err, buf.Len()) + } +} + +func TestSelfTestProvesTheFirewall(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := SelfTest(ctx); err != nil { + t.Fatal(err) + } +} + +// FuzzMatchParity is the differential check across the boundary: for valid +// UTF-8 the Rust engine and the Go matcher must agree, and anything else is +// an engine error that leaves the handle usable. Run with +// go test -fuzz=FuzzMatchParity ./gussetfn. +func FuzzMatchParity(f *testing.F) { + for _, seed := range [][2]string{ + {"*.py", "src/foo.py"}, {"[!a-c]*", "d"}, {"[c-a-e]", "e"}, {"[]]", "]"}, + {"[!]", "!"}, {"**?*", "ab"}, {"\xff", "a"}, {"[\\]", "\\"}, {"a*b*c*d*e", "aXbXcXdXe"}, + } { + f.Add(seed[0], seed[1]) + } + ctx := context.Background() + f.Fuzz(func(t *testing.T, pattern, name string) { + if len(pattern) > maxField || len(name) > maxField { + if _, err := Match(ctx, pattern, name); err == nil { + t.Fatal("an overlong field was accepted") + } + return + } + got, err := Match(ctx, pattern, name) + if !utf8.ValidString(pattern) || !utf8.ValidString(name) { + if err == nil { + t.Fatalf("invalid UTF-8 (%q, %q) matched instead of failing", pattern, name) + } + if poisoned(err) { + t.Fatalf("invalid UTF-8 poisoned the handle: %v", err) + } + return + } + if err != nil { + t.Fatalf("Match(%q, %q): %v", pattern, name, err) + } + if want := fnmatch.Match(pattern, name); got != want { + t.Fatalf("Match(%q, %q): gusset %v, Go %v", pattern, name, got, want) + } + any, err := MatchAny(ctx, []string{pattern, pattern + "x"}, name) + if err != nil { + t.Fatalf("MatchAny: %v", err) + } + if want := fnmatch.MatchAny([]string{pattern, pattern + "x"}, name); any != want { + t.Fatalf("MatchAny(%q, %q): gusset %v, Go %v", pattern, name, any, want) + } + }) +} + +// The umbrella installs gusset's Counting allocator. Gusset counts its own +// buffers by hand whether or not Counting is installed, so buffer bytes prove +// nothing about the allocator; AllocCount across plain Matches does — each +// frame decode allocates a Vec that only a global Counting sees. +func TestCountingIsTheGlobalAllocator(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := Match(ctx, "*", "a"); err != nil { + t.Fatal(err) + } + before := gusset.Stats().AllocCount + for i := 0; i < 50; i++ { + if _, err := Match(ctx, "*.py", "src/foo.py"); err != nil { + t.Fatal(err) + } + } + if after := gusset.Stats().AllocCount; after < before+50 { + t.Fatalf("AllocCount %d -> %d over 50 Matches: Counting is not the global allocator", before, after) + } +} + +// A Rust buffer is counted once, not by both gusset's buffer accounting and +// the global allocator. +func TestStatsCountBuffersOnce(t *testing.T) { + if _, err := engine(); err != nil { + t.Fatal(err) + } + h, err := gusset.Open(gusset.WithPoolSize(1)) + if err != nil { + t.Fatal(err) + } + defer h.Close() + const n = 8 << 20 + before := gusset.Stats().LiveBytes + buf, err := h.NewBuffer(n) + if err != nil { + t.Fatal(err) + } + during := gusset.Stats().LiveBytes + if err := buf.Free(); err != nil { + t.Fatal(err) + } + after := gusset.Stats().LiveBytes + delta := int64(during) - int64(before) + // Process-wide, so a concurrent free from an earlier test's cleanup can + // pull it slightly under n; double-counting would put it near 2n. + if delta < n-n/16 || delta > n+n/2 { + t.Fatalf("an %d-byte buffer moved LiveBytes by %d; want about %d once", n, delta, n) + } + if int64(after)-int64(before) > n/8 { + t.Fatalf("LiveBytes %d after Free, %d before", after, before) + } +} + +// Calls racing Close return a result or ErrClosed, never an unclassified +// "handle is closed". Named ZZ and reset by defer because Close is +// process-wide; Go runs tests in source order, so this stays after the rest +// of this file (match_test.go runs later and relies on the reset). +func TestZZCloseRacingCallsSeeErrClosed(t *testing.T) { + defer func() { closed.Store(false) }() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if _, err := engine(); err != nil { + t.Fatal(err) + } + var wg sync.WaitGroup + errs := make(chan error, 32*50) + for g := 0; g < 32; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < 50; i++ { + if _, err := Match(ctx, "*.py", "a.py"); err != nil && !errors.Is(err, ErrClosed) { + errs <- err + } + } + }() + } + time.Sleep(2 * time.Millisecond) + if err := Close(); err != nil { + t.Errorf("Close: %v", err) + } + wg.Wait() + close(errs) + for err := range errs { + t.Errorf("call racing Close: %v", err) + } + if _, err := Match(context.Background(), "*", "a"); !errors.Is(err, ErrClosed) { + t.Fatalf("Match after Close = %v, want ErrClosed", err) + } + if err := Close(); err != nil { + t.Fatalf("second Close: %v", err) + } +} + +// Shutdown is process-wide and one-way, so it runs in a child process. +func TestShutdownIsBoundedAndRefusesLaterCalls(t *testing.T) { + if os.Getenv("GUSSETFN_SHUTDOWN_CHILD") == "1" { + ctx := context.Background() + if _, err := Match(ctx, "*", "a"); err != nil { + fmt.Println("pre-shutdown match:", err) + os.Exit(3) + } + if err := Shutdown(time.Second); err != nil { + fmt.Println("shutdown:", err) + os.Exit(4) + } + if _, err := Match(ctx, "*", "a"); !errors.Is(err, ErrClosed) { + fmt.Println("post-shutdown match:", err) + os.Exit(5) + } + os.Exit(0) + } + cmd := exec.Command(os.Args[0], "-test.run=^TestShutdownIsBoundedAndRefusesLaterCalls$") + cmd.Env = append(os.Environ(), "GUSSETFN_SHUTDOWN_CHILD=1") + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("child: %v\n%s", err, out) + } +} diff --git a/backend/go_orchestrator/gussetfn/engine.go b/backend/go_orchestrator/gussetfn/engine.go index a81ed978..42545981 100644 --- a/backend/go_orchestrator/gussetfn/engine.go +++ b/backend/go_orchestrator/gussetfn/engine.go @@ -1,3 +1,5 @@ +//go:build unix + package gussetfn /* diff --git a/backend/go_orchestrator/gussetfn/errors.go b/backend/go_orchestrator/gussetfn/errors.go new file mode 100644 index 00000000..3df79df5 --- /dev/null +++ b/backend/go_orchestrator/gussetfn/errors.go @@ -0,0 +1,12 @@ +package gussetfn + +import "errors" + +// ErrClosed is returned after Close: the engine is shut for this process. +var ErrClosed = errors.New("gusset: engine is closed") + +// ErrUnsupported is every answer on a platform gusset does not support. +// gusset is unix-only (its Rust crate refuses other targets with +// compile_error!), so a Windows build has no engine to link; it still has to +// build, because the host is built there for dc-verify's json contract. +var ErrUnsupported = errors.New("gusset: the engine is unix-only and is not built on this platform") diff --git a/backend/go_orchestrator/gussetfn/match.go b/backend/go_orchestrator/gussetfn/match.go index 8bd8f3ca..49ecb0d9 100644 --- a/backend/go_orchestrator/gussetfn/match.go +++ b/backend/go_orchestrator/gussetfn/match.go @@ -1,11 +1,14 @@ +//go:build unix + // Package gussetfn matches paths through dc-glob on the Gusset runtime. // // The write gate does not call this. fnmatch.Match returns a bool, and a // Gusset failure has nowhere to go except "no match" or "match" — one of // those opens a path the gate meant to refuse, the other denies a write the // gate meant to allow, and a poisoned handle would do it for every later -// check. Callers that can return an error use Match. The devcouncil and -// manvi gusset-check commands are the production callers. +// check. Callers that can return an error use Match. The gusset-check +// commands of devcouncil, manvi, jarvis and GitPulse run SelfTest; Manvi's +// serve policy plane runs Check (never a deliberate panic) before answering. package gussetfn import ( @@ -13,7 +16,11 @@ import ( "encoding/binary" "errors" "fmt" + "io" + "strings" "sync" + "sync/atomic" + "time" "unicode/utf8" "github.com/bharathvbcr/DevCouncil/backend/go_orchestrator/fnmatch" @@ -25,24 +32,184 @@ import ( // copy on the cgo thread. const maxField = 1024 +// Opcodes the umbrella registers (rust/gusset-engine). Opcode 0 is Match. +const ( + opcodeMatchAny uint32 = 1 + opcodeSelfTestPanic uint32 = 0x7fff_0001 +) + +// maxFrame is Gusset's inline-copy limit. A MatchAny list is split into +// frames under it; a bigger []byte would be refused by Call. +const maxFrame = 4096 + +// maxPatterns matches MAX_PATTERNS in the umbrella. +const maxPatterns = 1024 + +// poolSize is the shared handle's worker count. +const poolSize = 4 + var ( - engineOnce sync.Once - engineH *gusset.Handle - engineErr error + registerOnce sync.Once + // engineMu serialises opening; the hot path reads current without it. + engineMu sync.Mutex + current atomic.Pointer[gusset.Handle] + closed atomic.Bool + // opens counts handles opened, so tests can tell one replacement from a + // stampede of them. + opens atomic.Int64 ) +// engine returns the shared handle, opening one if there is none. +// +// This used to be a sync.Once. A poisoned handle then stayed the only handle +// for the life of the process, and every later Match answered ErrPoisoned — +// the "close it and open a new handle" Gusset asks for had nowhere to happen. +// A failed Open is also retried by the next call instead of being latched. func engine() (*gusset.Handle, error) { - engineOnce.Do(func() { - register() - engineH, engineErr = gusset.Open(gusset.WithPoolSize(4)) - }) - if engineErr != nil { - return nil, engineErr + if h := current.Load(); h != nil { + return h, nil + } + engineMu.Lock() + defer engineMu.Unlock() + if closed.Load() { + return nil, ErrClosed + } + if h := current.Load(); h != nil { + return h, nil + } + registerOnce.Do(register) + h, err := gusset.Open(gusset.WithPoolSize(poolSize)) + if err != nil { + return nil, err + } + opens.Add(1) + current.Store(h) + return h, nil +} + +// retire drops h after it reported poison, so the next call opens a fresh +// handle. Only the first caller to see a given handle's poison closes it. +// +// Close joins h's workers. dc-glob is bounded, so that is short, but it runs +// off the caller's goroutine anyway: the caller already has its answer and +// its deadline is not Close's business. +func retire(h *gusset.Handle) { + if current.CompareAndSwap(h, nil) { + go func() { _ = h.Close() }() + } +} + +// poisoned reports an error after which h refuses all further work. +func poisoned(err error) bool { + return errors.Is(err, gusset.ErrPanic) || errors.Is(err, gusset.ErrPoisoned) +} + +// Close shuts the shared handle and refuses every later call with ErrClosed. +// +// It joins the handle's workers without a time limit, so a host that must +// not hang on a stuck engine calls Shutdown instead. +func Close() error { + engineMu.Lock() + defer engineMu.Unlock() + closed.Store(true) + h := current.Swap(nil) + if h == nil { + return nil + } + return h.Close() +} + +// Shutdown is Close with a bound: it runs gusset.Shutdown(drain) first, which +// refuses new work process-wide and cancels every job, then joins the handle. +// When the drain expires with work still running (an engine that ignores its +// JobContext) it returns that error without joining, because the join would +// wait for exactly that work: the process is exiting and the OS reclaims the +// threads. Process-wide and one-way, like gusset.Shutdown: for process exit +// only. +func Shutdown(drain time.Duration) error { + engineMu.Lock() + closed.Store(true) + engineMu.Unlock() + if err := gusset.Shutdown(drain); err != nil { + current.Store(nil) + return err } - if engineH == nil { - return nil, errors.New("gusset: engine handle is nil") + return Close() +} + +// DrainLogs writes whatever Gusset's Rust side logged (worker respawns, +// sigaltstack failures, caught panics) to w and reports how many bytes it +// wrote. Nothing reads that ring otherwise; it is bounded and evicts the +// oldest line, so a host that never drains it loses the evidence. +func DrainLogs(w io.Writer) (int, error) { + buf := make([]byte, 64<<10) + total := 0 + for { + n := gusset.DrainLogs(buf) + if n <= 0 { + return total, nil + } + if n > len(buf) { + n = len(buf) + } + m, err := w.Write(buf[:n]) + total += m + if err != nil { + return total, err + } + // Not "a short read means empty": gusset drains whole lines and never + // splits a UTF-8 character, so a short read can leave more behind. + // Only 0 means the ring is empty. } - return engineH, nil +} + +// handleGone reports an error that says h was closed under the call: +// retired after another call's panic, or shut by Close. Gusset has no +// sentinel for it; these are its two spellings. +func handleGone(err error) bool { + msg := err.Error() + return strings.Contains(msg, "gusset: handle is closed") || strings.Contains(msg, "gusset: handle closed") +} + +// call runs one frame on the shared handle and retires it on poison. +// +// A call that lands on a handle another call just poisoned or retired +// retries once on the replacement: a match is idempotent, and the failure +// was a sibling's, not this frame's. Before, those callers got ErrPoisoned or +// an unclassified "handle is closed" while a healthy handle was one call +// away. ErrPanic is never retried — this frame is what panicked, and a retry +// would poison the replacement too. After Close the retry reaches engine(), +// so a call racing Close answers ErrClosed, as Close promises. +func call(ctx context.Context, payload []byte) ([]byte, error) { + for attempt := 0; ; attempt++ { + h, err := engine() + if err != nil { + return nil, err + } + out, err := h.Call(ctx, payload) + if err == nil { + return out, nil + } + if poisoned(err) { + retire(h) + } + sibling := errors.Is(err, gusset.ErrPoisoned) || handleGone(err) + if !sibling { + return nil, err + } + // After Close the retry reaches engine(), which answers ErrClosed. + if attempt > 0 || ctx.Err() != nil { + return nil, err + } + } +} + +// decodeBool reads the engine's one-byte answer. +func decodeBool(out []byte) (bool, error) { + if len(out) != 1 || (out[0] != 0 && out[0] != 1) { + return false, fmt.Errorf("gusset: engine returned %q, want a single 0 or 1", out) + } + return out[0] == 1, nil } func encode(pattern, name string) ([]byte, error) { @@ -64,47 +231,106 @@ func encode(pattern, name string) ([]byte, error) { // // A Gusset or engine error is returned, never converted into a boolean. // The handle is not poisoned by a bad frame: the next well-formed call -// still runs. +// still runs. A handle that is poisoned anyway is retired, and the next call +// opens a new one. func Match(ctx context.Context, pattern, name string) (bool, error) { if ctx == nil { return false, errors.New("gusset: nil context") } - h, err := engine() - if err != nil { - return false, err - } payload, err := encode(pattern, name) if err != nil { return false, err } - out, err := h.Call(ctx, payload) + // Opcode 0, whatever the caller's context carries. gusset reads the + // opcode from ctx, so a ctx that had passed through MatchAny's opcode — + // or the self-test's — decoded this frame as a match-any frame, or + // panicked the shared handle. + out, err := call(gusset.ContextWithOpcode(ctx, 0), payload) if err != nil { return false, err } - if len(out) != 1 || (out[0] != 0 && out[0] != 1) { - return false, fmt.Errorf("gusset: engine returned %q, want a single 0 or 1", out) - } - return out[0] == 1, nil + return decodeBool(out) } // MatchAny reports whether name matches any pattern in patterns. -// If any Match call returns an error, matching stops and returns that error. +// +// The list crosses the boundary in as few frames as fit Gusset's 4 KiB +// inline copy, one call each, stopping at the first frame that matches. It +// used to be one call per pattern. Every field is length- and UTF-8-checked +// here before the first frame is sent, so a malformed pattern is an error +// even when an earlier one, in this frame or an earlier frame, would have +// matched; the engine re-validates each frame on its own side. func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) { + if ctx == nil { + return false, errors.New("gusset: nil context") + } + if len(name) > maxField { + return false, fmt.Errorf("gusset: name exceeds %d bytes", maxField) + } + if !utf8.ValidString(name) { + return false, errors.New("gusset: name is not valid UTF-8") + } for _, p := range patterns { - matched, err := Match(ctx, p, name) + if len(p) > maxField { + return false, fmt.Errorf("gusset: pattern exceeds %d bytes", maxField) + } + if !utf8.ValidString(p) { + return false, fmt.Errorf("gusset: pattern %q is not valid UTF-8", p) + } + } + if len(patterns) == 0 { + // Still a real answer, and still refused on a dead context. + if err := ctx.Err(); err != nil { + return false, err + } + return false, nil + } + actx := gusset.ContextWithOpcode(ctx, opcodeMatchAny) + for rest := patterns; len(rest) > 0; { + frame, used := encodeAny(name, rest) + out, err := call(actx, frame) if err != nil { return false, err } - if matched { - return true, nil + matched, err := decodeBool(out) + if err != nil || matched { + return matched, err } + rest = rest[used:] } return false, nil } -// Check runs the parity vectors the adoption measured, then a frame the -// diagnostic engine would treat as a panic. The second call must still match. +// encodeAny packs name and as many leading patterns as fit one frame, and +// reports how many it took. Fields are length-checked by the caller, so one +// pattern always fits: 2+1024+2+2+1024 is far below maxFrame. +func encodeAny(name string, patterns []string) ([]byte, int) { + size := 2 + len(name) + 2 + n := 0 + for n < len(patterns) && n < maxPatterns && size+2+len(patterns[n]) <= maxFrame { + size += 2 + len(patterns[n]) + n++ + } + buf := make([]byte, 0, size) + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(name))) + buf = append(buf, name...) + buf = binary.LittleEndian.AppendUint16(buf, uint16(n)) + for _, p := range patterns[:n] { + buf = binary.LittleEndian.AppendUint16(buf, uint16(len(p))) + buf = append(buf, p...) + } + return buf, n +} + +// Check runs the parity vectors the adoption measured, the batched MatchAny +// path, and a frame the diagnostic engine would treat as a panic. It never +// panics anything: Manvi runs it on the first policy check of a live server, +// where a deliberate "panicked at" on stderr reads as a crash. SelfTest adds +// the panic proof for the gusset-check commands. func Check(ctx context.Context) error { + if ctx == nil { + return errors.New("gusset: nil context") + } vectors := []struct { pattern, name string want bool @@ -145,5 +371,55 @@ func Check(ctx context.Context) error { if !utf8.ValidString("\x01") { return errors.New("internal: 0x01 was expected to be valid UTF-8") } + + anyPatterns := []string{"*.rs", "*.go", "src/*.py"} + if got, err := MatchAny(ctx, anyPatterns, "src/foo.py"); err != nil || !got { + return fmt.Errorf("MatchAny(%q, src/foo.py) = %v, %v; want true", anyPatterns, got, err) + } + if got, err := MatchAny(ctx, anyPatterns, "src/foo.js"); err != nil || got { + return fmt.Errorf("MatchAny(%q, src/foo.js) = %v, %v; want false", anyPatterns, got, err) + } + + return nil +} + +// SelfTest is Check plus a real panic inside this archive on a throwaway +// handle (I2), after which the shared handle must still match. Rust's panic +// hook prints the induced panic to stderr; that line is the proof running, +// not a failure. For the gusset-check commands and tests, not for a server. +func SelfTest(ctx context.Context) error { + if err := Check(ctx); err != nil { + return err + } + if err := checkPanicFirewall(ctx); err != nil { + return err + } + got, err := Match(ctx, "*.py", "src/foo.py") + if err != nil || !got { + return fmt.Errorf("shared handle after the self-test panic: Match = %v, %v", got, err) + } + return nil +} + +// checkPanicFirewall panics inside the umbrella on a handle of its own and +// requires the panic to come back as ErrPanic, the handle to refuse the next +// call as ErrPoisoned, and the process to still be here. The diagnostic +// engine proves this for Gusset; this proves it for the archive this binary +// actually links, built with this crate's panic strategy. +func checkPanicFirewall(ctx context.Context) error { + if _, err := engine(); err != nil { // registers the umbrella's engines + return err + } + h, err := gusset.Open(gusset.WithPoolSize(1), gusset.WithOpcode(opcodeSelfTestPanic)) + if err != nil { + return fmt.Errorf("self-test handle: %w", err) + } + defer h.Close() + if _, err := h.Call(ctx, nil); !errors.Is(err, gusset.ErrPanic) { + return fmt.Errorf("self-test panic returned %v, want ErrPanic", err) + } + if _, err := h.Call(ctx, nil); !errors.Is(err, gusset.ErrPoisoned) { + return fmt.Errorf("call after a panic returned %v, want ErrPoisoned", err) + } return nil } diff --git a/backend/go_orchestrator/gussetfn/match_test.go b/backend/go_orchestrator/gussetfn/match_test.go index 99eb3082..922b9449 100644 --- a/backend/go_orchestrator/gussetfn/match_test.go +++ b/backend/go_orchestrator/gussetfn/match_test.go @@ -1,3 +1,5 @@ +//go:build unix + package gussetfn import ( diff --git a/backend/go_orchestrator/gussetfn/unsupported.go b/backend/go_orchestrator/gussetfn/unsupported.go new file mode 100644 index 00000000..53b81680 --- /dev/null +++ b/backend/go_orchestrator/gussetfn/unsupported.go @@ -0,0 +1,32 @@ +//go:build !unix + +package gussetfn + +import ( + "context" + "io" + "time" +) + +// Match reports ErrUnsupported: there is no engine on this platform. +func Match(ctx context.Context, pattern, name string) (bool, error) { return false, ErrUnsupported } + +// MatchAny reports ErrUnsupported: there is no engine on this platform. +func MatchAny(ctx context.Context, patterns []string, name string) (bool, error) { + return false, ErrUnsupported +} + +// Check reports ErrUnsupported: there is no engine on this platform. +func Check(ctx context.Context) error { return ErrUnsupported } + +// SelfTest reports ErrUnsupported: there is no engine on this platform. +func SelfTest(ctx context.Context) error { return ErrUnsupported } + +// Close has nothing to release on this platform. +func Close() error { return nil } + +// Shutdown has nothing to release on this platform. +func Shutdown(time.Duration) error { return nil } + +// DrainLogs has nothing to drain on this platform. +func DrainLogs(io.Writer) (int, error) { return 0, nil } diff --git a/backend/go_orchestrator/policy/zfix_split_test.go b/backend/go_orchestrator/policy/zfix_split_test.go index 228049fb..cbcdf695 100644 --- a/backend/go_orchestrator/policy/zfix_split_test.go +++ b/backend/go_orchestrator/policy/zfix_split_test.go @@ -167,12 +167,12 @@ func diffCandidate(prefixIdx, sepIdx, payloadIdx int) string { func differentialGate() (CommandGate, *dc.Task) { return CommandGate{ - GlobalAllowedCommands: []string{"echo", "echo *", "cat *", "true", ":"}, - HardRules: true, - }, &dc.Task{ - ID: "TASK-001", - AllowedCommands: []string{"echo *", "cat *"}, - } + GlobalAllowedCommands: []string{"echo", "echo *", "cat *", "true", ":"}, + HardRules: true, + }, &dc.Task{ + ID: "TASK-001", + AllowedCommands: []string{"echo *", "cat *"}, + } } // runDifferentialCase executes one candidate under a real sh in its own diff --git a/backend/go_orchestrator/repomap/boundary_test.go b/backend/go_orchestrator/repomap/boundary_test.go index b6301bce..5ea8027f 100644 --- a/backend/go_orchestrator/repomap/boundary_test.go +++ b/backend/go_orchestrator/repomap/boundary_test.go @@ -451,11 +451,16 @@ func TestAnAreaIsFoundWithoutWalkingEveryIndexedFile(t *testing.T) { // be work rather than memory: the miss is timed against a map eight times // larger holding the same areas. A scan of the path table grows with it; a // set lookup does not. - small, large := build(256), build(2048) + // + // The gap is 32x the files against a bound of 8x. It was 8x against 3x, + // and shared macOS runners measured 3.4x and 7.4x for a lookup that never + // touches the file table: scheduler noise alone crossed a bound that left + // no room for it. A scan still reads about 32x and fails by a wide margin. + small, large := build(256), build(256*32) const probe = "elsewhere/entirely/deep/enough/x.go" ratio := pairedLookupRatio(small, large, probe) - if ratio > 3 { - t.Fatalf("resolving one path took %.1fx longer against 8x the files; the lookup is "+ + if ratio > 8 { + t.Fatalf("resolving one path took %.1fx longer against 32x the files; the lookup is "+ "walking the path table rather than consulting the set of areas", ratio) } } @@ -467,7 +472,7 @@ func TestAnAreaIsFoundWithoutWalkingEveryIndexedFile(t *testing.T) { // pause looks like the large map scanning the path table. Pairing them // measures both under the same load, and alternating which runs first // cancels a drift across the pair. A real scan of 8x the files is still -// about 8x on every pair, so the median stays over the bound. +// about 32x on every pair, so the median stays over the bound. func pairedLookupRatio(small, large *Map, probe string) float64 { const rounds = 9 ratios := make([]float64, 0, rounds) diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 726c077f..09883343 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -573,9 +573,9 @@ dependencies = [ [[package]] name = "find-msvc-tools" -version = "0.1.13" +version = "0.1.14" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ef25905e51abafe4dcea6c15fec58c57b601cdbd0ee53d22ea1d3016c587d39b" +checksum = "aedcfb3409746eddb02b9e19ebda1c3394f759a152e48ee875a0844d1b955484" [[package]] name = "foldhash" diff --git a/rust/dc-proc/src/lib.rs b/rust/dc-proc/src/lib.rs index 17a1f415..d8f16d07 100644 --- a/rust/dc-proc/src/lib.rs +++ b/rust/dc-proc/src/lib.rs @@ -320,6 +320,102 @@ pub fn run_bounded(command: &mut Command, bounds: Bounds) -> Result Option { + self.child.stdin.take() + } + + /// The child's stdout, once; `None` after the first call. + pub fn take_stdout(&mut self) -> Option { + self.child.stdout.take() + } + + /// The exit status if the child has already exited, without waiting. + pub fn try_status(&mut self) -> Option { + self.child.try_wait().ok().flatten() + } + + /// The program this session runs, for messages. + pub fn program(&self) -> &str { + &self.program + } + + /// Signal the child's process group and reap the child. Idempotent. + /// + /// The group is signalled even when the child has already exited: a + /// descendant it left behind is still in the group, and the group id + /// cannot be reused while any member lives. + pub fn kill(&mut self) { + if self.killed { + return; + } + self.killed = true; + kill_descendants(&mut self.child); + let _ = self.child.wait(); + } +} + +impl Drop for Session { + fn drop(&mut self) { + self.kill(); + } +} + +/// A command for [`spawn_session`], so a session's program is named through +/// this module like every other child the kernel starts. +pub fn session_command(program: impl AsRef) -> Command { + Command::new(program) +} + +/// Start `command` as a [`Session`]: stdin and stdout piped, stderr discarded +/// (a conversational child that fills an undrained stderr pipe blocks), and the +/// child leading its own process group. Spawning retries the same pre-exec +/// refusal [`run_bounded`] does, within `spawn_deadline`. +pub fn spawn_session(command: &mut Command, spawn_deadline: Duration) -> Result { + let program = command.get_program().to_string_lossy().into_owned(); + command + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()); + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + command.process_group(0); + } + let child = spawn_with_retry(|| command.spawn(), &program, Instant::now(), spawn_deadline)?; + Ok(Session { + child, + program, + killed: false, + }) +} + /// End the child and everything it started, then let the drain threads come /// home. /// diff --git a/rust/dc-regress/tests/owners_and_test_signal.rs b/rust/dc-regress/tests/owners_and_test_signal.rs index 5d3b010d..e2dd1e0b 100644 --- a/rust/dc-regress/tests/owners_and_test_signal.rs +++ b/rust/dc-regress/tests/owners_and_test_signal.rs @@ -108,6 +108,10 @@ fn a_repo_with_no_commits_marks_owners_unavailable() { /// An empty author is not invented into a placeholder owner. It is named as /// unavailable so completeness stays honest. +// Drives a /bin/sh stub standing in for git; there is no shell to run it on +// Windows. It was an early `return` there, which passed without testing +// anything and tripped `unreachable_code` under -D warnings. +#[cfg(unix)] #[test] fn an_empty_author_is_unavailable_not_invented() { let root = temp_root("empty-author"); @@ -150,7 +154,6 @@ fn an_empty_author_is_unavailable_not_invented() { // Drive the collector against a stub that emits one empty author line. let stub = root.join("empty-author-git"); - #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; std::fs::write( @@ -162,10 +165,6 @@ fn an_empty_author_is_unavailable_not_invented() { perms.set_mode(0o755); std::fs::set_permissions(&stub, perms).unwrap(); } - #[cfg(not(unix))] - { - return; - } let (owners, gaps) = owners_for_paths_with_program( stub.as_os_str(), @@ -191,6 +190,10 @@ fn an_empty_author_is_unavailable_not_invented() { /// A timeout after the first of many paths keeps the owners already found and /// records unavailable — it does not discard them or pretend the pass finished. +// Drives a /bin/sh stub standing in for git; there is no shell to run it on +// Windows. It was an early `return` there, which passed without testing +// anything and tripped `unreachable_code` under -D warnings. +#[cfg(unix)] #[test] fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { let root = temp_root("timeout"); @@ -205,7 +208,6 @@ fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { let counter = root.join("log-count"); let stub = root.join("slow-second-git"); - #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; let script = format!( @@ -217,10 +219,6 @@ fn a_timeout_after_one_path_keeps_partial_owners_and_is_incomplete() { perms.set_mode(0o755); std::fs::set_permissions(&stub, perms).unwrap(); } - #[cfg(not(unix))] - { - return; - } let (owners, gaps) = owners_for_paths_with_program( stub.as_os_str(), diff --git a/rust/devmap-cli/src/hook.rs b/rust/devmap-cli/src/hook.rs index 8a535dbf..d49435cd 100644 --- a/rust/devmap-cli/src/hook.rs +++ b/rust/devmap-cli/src/hook.rs @@ -4,6 +4,7 @@ //! Exit 2 is treated as "block the agent" on those hosts, so this path never //! exits 2: success and no-ops are 0, failures are 1. +use std::ffi::OsStr; use std::fs; use std::io::{self, Read}; use std::path::{Path, PathBuf}; @@ -1847,10 +1848,7 @@ fn detach_build(executable: &Path, root: &Path) -> anyhow::Result<()> { } spawn_detached_with_cleanup( executable, - &format!( - "--root {} build", - shell_single_quote(&root.display().to_string()) - ), + &[OsStr::new("--root"), root.as_os_str(), OsStr::new("build")], &lock_dir, ) } @@ -1862,10 +1860,11 @@ fn detach_session_report(executable: &Path, root: &Path) -> anyhow::Result<()> { } spawn_detached_with_cleanup( executable, - &format!( - "--root {} session-report", - shell_single_quote(&root.display().to_string()) - ), + &[ + OsStr::new("--root"), + root.as_os_str(), + OsStr::new("session-report"), + ], &lock_dir, ) } @@ -1982,44 +1981,165 @@ fn lock_dir_is_stale(lock_dir: &Path) -> bool { let rc = unsafe { libc::kill(pid, 0) }; rc != 0 } - #[cfg(not(unix))] + #[cfg(windows)] + { + !windows_process_is_running(pid as u32) + } + #[cfg(not(any(unix, windows)))] { let _ = pid; false } } +/// Keep this process's own stdin/stdout/stderr out of every child it spawns. +/// +/// Windows hands a child every inheritable handle of its parent, whatever the +/// child's own stdio is set to, and the pipes the host gave this hook are +/// inheritable. A detached build would hold the host's pipe open, and the host, +/// waiting for the hook's output to end, would wait for the build. unix never +/// had this: std marks every descriptor but 0-2 close-on-exec, and 0-2 are +/// redirected. Clearing the flag changes only what children receive; this +/// process's own reads and writes are unaffected. +#[cfg(windows)] +fn stop_standard_handles_from_being_inherited() { + const STD_INPUT_HANDLE: u32 = -10i32 as u32; + const STD_OUTPUT_HANDLE: u32 = -11i32 as u32; + const STD_ERROR_HANDLE: u32 = -12i32 as u32; + const HANDLE_FLAG_INHERIT: u32 = 0x1; + #[link(name = "kernel32")] + extern "system" { + fn GetStdHandle(which: u32) -> *mut std::ffi::c_void; + fn SetHandleInformation(handle: *mut std::ffi::c_void, mask: u32, flags: u32) -> i32; + } + for which in [STD_INPUT_HANDLE, STD_OUTPUT_HANDLE, STD_ERROR_HANDLE] { + // SAFETY: GetStdHandle returns this process's handle or null/invalid, + // and SetHandleInformation on either only fails. + unsafe { + let handle = GetStdHandle(which); + if !handle.is_null() && handle as isize != -1 { + SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 0); + } + } + } +} + +/// Whether `pid` names a running process. It was not asked at all on Windows: +/// every lock read as live until [`LOCK_MAX_AGE`], so a build that died without +/// removing its lock stopped that repository's rebuilds for half an hour. +#[cfg(windows)] +fn windows_process_is_running(pid: u32) -> bool { + const PROCESS_QUERY_LIMITED_INFORMATION: u32 = 0x1000; + const STILL_ACTIVE: u32 = 259; + const ERROR_ACCESS_DENIED: u32 = 5; + #[link(name = "kernel32")] + extern "system" { + fn OpenProcess(access: u32, inherit: i32, pid: u32) -> *mut std::ffi::c_void; + fn GetExitCodeProcess(process: *mut std::ffi::c_void, code: *mut u32) -> i32; + fn CloseHandle(handle: *mut std::ffi::c_void) -> i32; + fn GetLastError() -> u32; + } + // SAFETY: plain Win32 calls on a handle this function opens and closes. + unsafe { + let handle = OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, 0, pid); + if handle.is_null() { + // A process this user may not query still exists; anything else + // (invalid parameter) is no process with that pid. + return GetLastError() == ERROR_ACCESS_DENIED; + } + let mut code = 0u32; + let ok = GetExitCodeProcess(handle, &mut code) != 0; + CloseHandle(handle); + // Unknown is held, like every other unknown owner here. + !ok || code == STILL_ACTIVE + } +} + +/// The coalescing lock this process was started to hold, released on drop. +/// +/// Only the Windows spawn names one ([`OWNED_LOCK_ENV`]). It is removed only if +/// its `pid` file names this process, so the variable cannot direct the removal +/// of any other directory: a stale or hostile value is ignored. +pub struct OwnedLock(Option); + +impl OwnedLock { + /// Take the lock named in the environment, and unset the variable so no + /// grandchild inherits the claim. + pub fn from_env() -> Self { + let Some(dir) = std::env::var_os(OWNED_LOCK_ENV) else { + return Self(None); + }; + // Only the Windows spawn sets it, and Windows serialises environment + // access in the OS, so the runtime threads already started are safe. + std::env::remove_var(OWNED_LOCK_ENV); + Self(Some(PathBuf::from(dir))) + } +} + +impl Drop for OwnedLock { + fn drop(&mut self) { + let Some(dir) = self.0.take() else { + return; + }; + // The spawning hook writes our pid a moment after the spawn; a child + // that finishes first leaves the lock for the stale check, which now + // sees this pid dead. + let owner = devmap_query::stat_memo::read_bounded(&dir.join("pid"), MAX_PID_BYTES); + if owner.as_deref().map(str::trim) == Some(std::process::id().to_string().as_str()) { + let _ = fs::remove_dir_all(&dir); + } + } +} + +#[cfg(unix)] fn shell_single_quote(text: &str) -> String { format!("'{}'", text.replace('\'', "'\\''")) } +/// Environment variable naming the coalescing lock a detached child owns and +/// must remove when it exits. Set only on Windows, where there is no shell to +/// do it; see [`OwnedLock`]. +pub const OWNED_LOCK_ENV: &str = "DEVMAP_HOOK_OWNED_LOCK"; + +/// Spawn `executable args…` detached, holding `lock_dir` for its lifetime. +/// +/// unix runs it under `sh -c` in a new session, and the shell removes the lock +/// when the child exits. Windows has no shell to rely on — this path required +/// `sh` there, and with none on PATH no hook ever started a build — so the +/// child is started directly as a detached process and removes the lock itself +/// on exit ([`OwnedLock`]); a child that dies without doing so leaves a lock +/// [`lock_dir_is_stale`] now recognises by its dead pid. fn spawn_detached_with_cleanup( executable: &Path, - args_shell: &str, + args: &[&OsStr], lock_dir: &Path, ) -> anyhow::Result<()> { - let exe = executable - .to_str() - .ok_or_else(|| anyhow!("executable path is not UTF-8"))?; - let lock = lock_dir - .to_str() - .ok_or_else(|| anyhow!("lock path is not UTF-8"))?; - // The lock directory is held for the child's lifetime so concurrent hooks - // coalesce onto one build. The shell removes it when the child exits. - let script = format!( - "{} {}; status=$?; rm -rf {}; exit $status", - shell_single_quote(exe), - args_shell, - shell_single_quote(lock) - ); - let mut cmd = Command::new("sh"); - cmd.arg("-c") - .arg(script) - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()); #[cfg(unix)] - { + let mut cmd = { + let exe = executable + .to_str() + .ok_or_else(|| anyhow!("executable path is not UTF-8"))?; + let lock = lock_dir + .to_str() + .ok_or_else(|| anyhow!("lock path is not UTF-8"))?; + let mut quoted = Vec::with_capacity(args.len()); + for arg in args { + let arg = arg + .to_str() + .ok_or_else(|| anyhow!("argument is not UTF-8"))?; + quoted.push(shell_single_quote(arg)); + } + // The lock directory is held for the child's lifetime so concurrent + // hooks coalesce onto one build. The shell removes it when the child + // exits. + let script = format!( + "{} {}; status=$?; rm -rf {}; exit $status", + shell_single_quote(exe), + quoted.join(" "), + shell_single_quote(lock) + ); + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg(script); use std::os::unix::process::CommandExt; unsafe { cmd.pre_exec(|| { @@ -2029,7 +2149,28 @@ fn spawn_detached_with_cleanup( Ok(()) }); } - } + cmd + }; + #[cfg(windows)] + let mut cmd = { + use std::os::windows::process::CommandExt; + stop_standard_handles_from_being_inherited(); + const DETACHED_PROCESS: u32 = 0x0000_0008; + const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; + let mut cmd = Command::new(executable); + cmd.args(args) + .env(OWNED_LOCK_ENV, lock_dir) + .creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP); + cmd + }; + #[cfg(not(any(unix, windows)))] + let mut cmd: Command = { + let _ = (executable, args); + anyhow::bail!("detached builds are not supported on this platform"); + }; + cmd.stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()); let child = cmd .spawn() .with_context(|| format!("spawn detached {}", executable.display()))?; @@ -3684,7 +3825,7 @@ mod lock_tests { let lock = root.join("lock"); assert!(try_acquire_lock_dir(&lock).unwrap()); - spawn_detached_with_cleanup(Path::new("/bin/sleep"), "5", &lock).unwrap(); + spawn_detached_with_cleanup(Path::new("/bin/sleep"), &[OsStr::new("5")], &lock).unwrap(); std::thread::sleep(Duration::from_millis(200)); let recorded: i32 = fs::read_to_string(lock.join("pid")) diff --git a/rust/devmap-cli/src/main.rs b/rust/devmap-cli/src/main.rs index e4f1c2f1..55a73bd6 100644 --- a/rust/devmap-cli/src/main.rs +++ b/rust/devmap-cli/src/main.rs @@ -4988,6 +4988,9 @@ fn on_command_stack(work: impl FnOnce() -> T + Send + 'static #[tokio::main] async fn main() -> std::process::ExitCode { + // Held to the end of `main`: a detached hook build on Windows removes its + // coalescing lock when it finishes (see `hook::spawn_detached_with_cleanup`). + let _owned_lock = hook::OwnedLock::from_env(); let started = Instant::now(); let (cli, matches) = on_command_stack(|| { let matches = Cli::command().get_matches(); diff --git a/rust/devmap-cli/tests/doctor_gaps.rs b/rust/devmap-cli/tests/doctor_gaps.rs index edf929bd..47f2ca08 100644 --- a/rust/devmap-cli/tests/doctor_gaps.rs +++ b/rust/devmap-cli/tests/doctor_gaps.rs @@ -50,20 +50,23 @@ fn pinned_registrations_are_not_counted_as_global_duplicates() { // One global (no --root) and one pinned project entry. fs::write( home.join(".cursor/mcp.json"), - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{}","args":["mcp"]}}}}}}"#, - devmap_bin().display() - ), + // Built, not formatted: a Windows path in a JSON literal is an + // invalid escape, and an unparseable config registers nothing. + serde_json::json!({"mcpServers": {"devmap": { + "type": "stdio", "command": devmap_bin(), "args": ["mcp"] + }}}) + .to_string(), ) .unwrap(); fs::create_dir_all(cwd.join(".cursor")).unwrap(); fs::write( cwd.join(".cursor/mcp.json"), - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{}","args":["--root","{}","mcp"]}}}}}}"#, - devmap_bin().display(), - cwd.display() - ), + serde_json::json!({"mcpServers": {"devmap": { + "type": "stdio", + "command": devmap_bin(), + "args": ["--root", cwd, "mcp"] + }}}) + .to_string(), ) .unwrap(); let payload = doctor_with_home(&home, &cwd); diff --git a/rust/devmap-cli/tests/hook_conformance.rs b/rust/devmap-cli/tests/hook_conformance.rs index a71f379e..e699972c 100644 --- a/rust/devmap-cli/tests/hook_conformance.rs +++ b/rust/devmap-cli/tests/hook_conformance.rs @@ -11,6 +11,19 @@ use serde_json::json; const DEVMAP: &str = env!("CARGO_BIN_EXE_devmap"); +/// Whether `text` names `path` as written or canonical (a `\\?\` verbatim +/// path with long names on Windows, `/private/var` on macOS). +fn names(text: &str, path: &Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + fn scratch(tag: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!( "devmap-hook-conf-{tag}-{}-{}", @@ -520,17 +533,47 @@ fn unsafe_home_without_store_is_skipped() { /// and this file already relies on it for the sibling-union regression. fn make_undetachable(root: &Path) { let codeintel = root.join(".devcouncil/codeintel"); - let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); - perms.set_readonly(true); - std::fs::set_permissions(&codeintel, perms).unwrap(); + #[cfg(windows)] + { + // Windows ignores the read-only attribute on a directory, so the hook + // still created its lock there and this helper disabled nothing. A + // deny ACE for Everyone (S-1-1-0) on writing is what refuses the + // lock's `mkdir`, administrators included. + icacls(&codeintel, &["/deny", "*S-1-1-0:(W)"]); + } + #[cfg(not(windows))] + { + let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); + perms.set_readonly(true); + std::fs::set_permissions(&codeintel, perms).unwrap(); + } } fn make_detachable(root: &Path) { let codeintel = root.join(".devcouncil/codeintel"); - let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); - #[allow(clippy::permissions_set_readonly_false)] - perms.set_readonly(false); - std::fs::set_permissions(&codeintel, perms).unwrap(); + #[cfg(windows)] + icacls(&codeintel, &["/remove:d", "*S-1-1-0"]); + #[cfg(not(windows))] + { + let mut perms = std::fs::metadata(&codeintel).unwrap().permissions(); + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); + std::fs::set_permissions(&codeintel, perms).unwrap(); + } +} + +#[cfg(windows)] +fn icacls(path: &Path, args: &[&str]) { + let out = Command::new("icacls") + .arg(path) + .args(args) + .output() + .expect("icacls runs"); + assert!( + out.status.success(), + "icacls {args:?} failed: {}", + String::from_utf8_lossy(&out.stdout) + ); } /// `--root` must pin the repository, and the payload must not move it. @@ -589,7 +632,7 @@ fn pinned_root_disables_discovery() { "the pinned repository was never acted on, so the pin proves nothing" ); assert!( - run.stderr.contains(pinned.to_str().unwrap()), + names(&run.stderr, &pinned), "the failure must name the pinned root, not another repository: {}", run.stderr ); @@ -686,7 +729,13 @@ fn cursor_session_start_stdout_survives_hostile_briefing_characters() { let root = scratch("cursor-quotes"); seed_store(&root); // The repository *name* is interpolated into the briefing. + // `"` cannot appear in a Windows file name, so no Windows repository can + // carry it; there the name keeps every other character that is hostile to + // a hand-built JSON string. Creating it failed with os error 123. + #[cfg(not(windows))] let named = root.join("repo \"quotes\" and 日本語"); + #[cfg(windows)] + let named = root.join("repo 'quotes' {braces} and 日本語"); std::fs::create_dir_all(&named).unwrap(); seed_store(&named); let payload = json!({ diff --git a/rust/devmap-cli/tests/repo_scope_hardening.rs b/rust/devmap-cli/tests/repo_scope_hardening.rs index dd70e5f0..3e7ed36b 100644 --- a/rust/devmap-cli/tests/repo_scope_hardening.rs +++ b/rust/devmap-cli/tests/repo_scope_hardening.rs @@ -6,6 +6,21 @@ use std::time::{SystemTime, UNIX_EPOCH}; use serde_json::Value; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-cli-scope-{name}-{}-{}", @@ -56,9 +71,11 @@ fn write_mcp(path: &Path, command: &str) { } std::fs::write( path, - format!( - r#"{{"mcpServers":{{"devmap":{{"type":"stdio","command":"{command}","args":["mcp"]}}}}}}"# - ), + // Built, not formatted: a Windows path spliced into a JSON literal is + // `"C:\Users\…"`, an invalid escape, and the host config it writes + // fails to parse — doctor then correctly reports nothing registered. + serde_json::json!({"mcpServers": {"devmap": {"type": "stdio", "command": command, "args": ["mcp"]}}}) + .to_string(), ) .unwrap(); } @@ -176,7 +193,7 @@ fn build_from_home_refuses_instead_of_indexing() { String::from_utf8_lossy(&out.stderr) ); assert!( - combined.contains(&home.display().to_string()), + names(&combined, &home), "refusal must name the resolved root: {combined}" ); let store = home @@ -212,7 +229,7 @@ fn doctor_warns_when_a_host_config_names_a_missing_binary() { "a host config pointing at a path that is not a file must not look like a clean doctor: {payload}" ); assert!( - warning.contains(&missing.display().to_string()) || warning.contains("no-such-devmap"), + names(warning, &missing) || warning.contains("no-such-devmap"), "warning must name the missing path: {warning}" ); let skew = payload.get("binary_skew_warning"); diff --git a/rust/devmap-cli/tests/state_discovery_contract.rs b/rust/devmap-cli/tests/state_discovery_contract.rs index 60a0e6f3..ae209e72 100644 --- a/rust/devmap-cli/tests/state_discovery_contract.rs +++ b/rust/devmap-cli/tests/state_discovery_contract.rs @@ -5,6 +5,13 @@ use std::process::{Command, Stdio}; use std::sync::atomic::{AtomicU64, Ordering}; use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; +/// A path's text without a Windows `\\?\` verbatim prefix. `paths` reports +/// `db_path` in plain form while the fixture root is canonical (verbatim on +/// Windows); both name the same file, and elsewhere this is the identity. +fn plain(path: Option<&str>) -> Option<&str> { + path.map(|p| p.strip_prefix(r"\\?\").unwrap_or(p)) +} + struct Fixture(PathBuf); impl Fixture { @@ -108,7 +115,7 @@ fn relative_repository_paths_do_not_duplicate_the_repository_directory() { fixture.run(&fixture.0, &["build", "repo", "--manifest"], None); let report = fixture.run(&fixture.0, &["paths", "repo"], None); let expected = fixture.repo().join(".devmap/codeintel/devmap.sqlite"); - assert_eq!(report["db_path"].as_str(), expected.to_str()); + assert_eq!(plain(report["db_path"].as_str()), plain(expected.to_str())); assert_eq!(report["store_exists"], true); } @@ -126,8 +133,8 @@ fn explicit_relative_database_paths_stay_relative_to_the_invoking_directory() { None, ); assert_eq!( - report["db_path"].as_str(), - fixture.0.join("custom.sqlite").to_str() + plain(report["db_path"].as_str()), + plain(fixture.0.join("custom.sqlite").to_str()) ); assert_eq!(report["store_exists"], true); } @@ -174,11 +181,13 @@ fn relative_home_override_is_repository_relative_only_once() { fixture.run(&fixture.0, &["build", "repo", "--manifest"], Some(home)); let report = fixture.run(&fixture.0, &["paths", "repo"], Some(home)); assert_eq!( - report["db_path"].as_str(), - fixture - .repo() - .join("state/codeintel/devmap.sqlite") - .to_str() + plain(report["db_path"].as_str()), + plain( + fixture + .repo() + .join("state/codeintel/devmap.sqlite") + .to_str() + ) ); assert_eq!(report["store_exists"], true); } diff --git a/rust/devmap-cli/tests/test_fanout_metric.rs b/rust/devmap-cli/tests/test_fanout_metric.rs index 9e37a362..b7ccc3e1 100644 --- a/rust/devmap-cli/tests/test_fanout_metric.rs +++ b/rust/devmap-cli/tests/test_fanout_metric.rs @@ -154,25 +154,33 @@ fn the_fanout_metric_matches_a_hand_counted_corpus() { String::from_utf8_lossy(&out.stderr) ); - // The shell gates do not use rusqlite. They run `tools/fanout.sh`, which - // is whatever `sqlite3` is on PATH. Apple's CLI `-readonly` cannot open - // this store; the script must still return the same row. - let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("../tools/fanout.sh"); - let shell = Command::new("bash") - .arg(&script) - .arg(&db) - .output() - .expect("fanout.sh"); - assert!( - shell.status.success(), - "fanout.sh failed: {}", - String::from_utf8_lossy(&shell.stderr) - ); - let shell_line = String::from_utf8(shell.stdout).expect("fanout.sh stdout is utf-8"); - assert!( - shell_line.contains(&format!("fanout_edges={EXPECTED_EDGES}")), - "fanout.sh row did not match the hand count: {shell_line}" - ); + // The shell half runs where its consumers do: `verify.sh` and + // `memory_model_probe.sh` are unix shell gates. On a Windows runner a bare + // `bash` is the WSL launcher, which exits non-zero with nothing on stderr + // when no distribution is installed. Everything below this block checks + // the same numbers through rusqlite and runs everywhere. + #[cfg(unix)] + { + // The shell gates do not use rusqlite. They run `tools/fanout.sh`, which + // is whatever `sqlite3` is on PATH. Apple's CLI `-readonly` cannot open + // this store; the script must still return the same row. + let script = Path::new(env!("CARGO_MANIFEST_DIR")).join("../tools/fanout.sh"); + let shell = Command::new("bash") + .arg(&script) + .arg(&db) + .output() + .expect("fanout.sh"); + assert!( + shell.status.success(), + "fanout.sh failed: {}", + String::from_utf8_lossy(&shell.stderr) + ); + let shell_line = String::from_utf8(shell.stdout).expect("fanout.sh stdout is utf-8"); + assert!( + shell_line.contains(&format!("fanout_edges={EXPECTED_EDGES}")), + "fanout.sh row did not match the hand count: {shell_line}" + ); + } let f = fanout(&db); diff --git a/rust/devmap-extract/src/paths.rs b/rust/devmap-extract/src/paths.rs index 41e7ee14..8348e100 100644 --- a/rust/devmap-extract/src/paths.rs +++ b/rust/devmap-extract/src/paths.rs @@ -144,44 +144,51 @@ pub fn resolve_state_dir( standalone } +/// `base` joined with a `/`-separated relative path, one component at a time, +/// so the result uses the platform's separator throughout. A plain join kept +/// the `/` on Windows (`.\.devmap\codeintel/devmap.sqlite` in `doctor`). +fn join_rel(base: PathBuf, rel: &str) -> PathBuf { + rel.split('/').fold(base, |path, part| path.join(part)) +} + /// Absolute-or-relative path to the store for `root`. pub fn store_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(STORE_RELPATH) + join_rel(state_dir(root), STORE_RELPATH) } /// Path to `repo_map.json` for `root`. pub fn repo_map_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(REPO_MAP_RELPATH) + join_rel(state_dir(root), REPO_MAP_RELPATH) } /// Path to `code_graph.json` for `root`. pub fn code_graph_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CODE_GRAPH_RELPATH) + join_rel(state_dir(root), CODE_GRAPH_RELPATH) } /// Path to the interned `code_graph.compact.json` for `root`. pub fn compact_code_graph_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CODE_GRAPH_COMPACT_RELPATH) + join_rel(state_dir(root), CODE_GRAPH_COMPACT_RELPATH) } /// Path to the content-hash memo for `root`. pub fn content_cache_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(CONTENT_CACHE_RELPATH) + join_rel(state_dir(root), CONTENT_CACHE_RELPATH) } /// Path to the `devmap`-binary digest memo for `root`. pub fn binary_digest_cache_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(BINARY_DIGEST_CACHE_RELPATH) + join_rel(state_dir(root), BINARY_DIGEST_CACHE_RELPATH) } /// Path to the workspace registry for `root`. pub fn workspace_path(root: impl AsRef) -> PathBuf { - state_dir(root).join(WORKSPACE_RELPATH) + join_rel(state_dir(root), WORKSPACE_RELPATH) } /// Path to the emitted Claude Code plugin bundle for `root`. pub fn plugin_dir(root: impl AsRef) -> PathBuf { - state_dir(root).join(PLUGIN_RELPATH) + join_rel(state_dir(root), PLUGIN_RELPATH) } /// The repository root a store path belongs to — the inverse of [`store_path`]. diff --git a/rust/devmap-extract/src/safe_fs.rs b/rust/devmap-extract/src/safe_fs.rs index 30559636..1ad4b82a 100644 --- a/rust/devmap-extract/src/safe_fs.rs +++ b/rust/devmap-extract/src/safe_fs.rs @@ -394,15 +394,11 @@ impl SafeFile { let current = check_regular(&self.file, self.access != Access::Read)?; let named = open_regular_child(&self._parent, &self.name, Access::Read, Creation::Never)?; let named_metadata = check_regular(&named, self.access != Access::Read)?; - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if current.dev() != named_metadata.dev() || current.ino() != named_metadata.ino() { - return Err(io::Error::new( - io::ErrorKind::Interrupted, - "managed file was replaced while being examined", - )); - } + if !same_identity(&self.file, ¤t, &named, &named_metadata)? { + return Err(io::Error::new( + io::ErrorKind::Interrupted, + "managed file was replaced while being examined", + )); } if current.len() < self.initial.len() { return Err(refused("managed file was truncated while being examined")); @@ -429,15 +425,11 @@ impl SafeFile { let current = check_regular(&self.file, self.access != Access::Read)?; let named = open_regular_child(&self._parent, &self.name, Access::Read, Creation::Never)?; let named_metadata = check_regular(&named, self.access != Access::Read)?; - #[cfg(unix)] - { - use std::os::unix::fs::MetadataExt; - if current.dev() != named_metadata.dev() || current.ino() != named_metadata.ino() { - return Err(io::Error::new( - io::ErrorKind::Interrupted, - "managed file was replaced while being examined", - )); - } + if !same_identity(&self.file, ¤t, &named, &named_metadata)? { + return Err(io::Error::new( + io::ErrorKind::Interrupted, + "managed file was replaced while being examined", + )); } if named_metadata.len() != current.len() || named_metadata.modified()? != current.modified()? @@ -667,6 +659,35 @@ fn open_regular_child( } } +/// Whether the handle held and the file now at the name are one file. +/// +/// unix compares device and inode. Windows compares volume serial and file +/// index, from the handles: before this the check was unix-only, so on +/// Windows a managed file swapped for another between open and use was not +/// noticed, and the name's metadata was computed and then dropped unused. +#[cfg(unix)] +fn same_identity( + _held_file: &File, + held: &std::fs::Metadata, + _named_file: &File, + named: &std::fs::Metadata, +) -> io::Result { + use std::os::unix::fs::MetadataExt; + Ok(held.dev() == named.dev() && held.ino() == named.ino()) +} + +#[cfg(windows)] +fn same_identity( + held_file: &File, + _held: &std::fs::Metadata, + named_file: &File, + _named: &std::fs::Metadata, +) -> io::Result { + let a = windows_information(held_file)?; + let b = windows_information(named_file)?; + Ok(a.volume == b.volume && a.index_high == b.index_high && a.index_low == b.index_low) +} + #[cfg(windows)] fn windows_options() -> std::fs::OpenOptions { use std::os::windows::fs::OpenOptionsExt; diff --git a/rust/devmap-query/src/artifacts.rs b/rust/devmap-query/src/artifacts.rs index 94f0feac..8c6db863 100644 --- a/rust/devmap-query/src/artifacts.rs +++ b/rust/devmap-query/src/artifacts.rs @@ -21,7 +21,50 @@ pub struct ArtifactFingerprint { /// Distinguishes concurrent temp files written by one process. static WRITE_SEQUENCE: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// How long a Windows writer waits out another process's hold on the target. +/// +/// Windows refuses to open or replace a file another handle holds without +/// `FILE_SHARE_DELETE` (os error 32, or 5 for a pending delete), which every +/// concurrent writer here does for a moment while it compares or renames. On +/// unix the same moment is invisible. Bounded, so a file held open for good +/// still fails. +#[cfg(windows)] +const WINDOWS_SHARING_DEADLINE: std::time::Duration = std::time::Duration::from_secs(5); + +/// An error that means "another handle holds this file right now". +#[cfg(windows)] +fn is_sharing_violation(error: &std::io::Error) -> bool { + error.kind() == std::io::ErrorKind::PermissionDenied + || matches!(error.raw_os_error(), Some(32 | 33)) +} + pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { + // Every step below is safe to repeat from the top: the comparison reads, + // the temporary name is new each time, and the rename is the only write. + // On Windows a sharing violation can surface from any of them while + // another process holds the same file or directory for a moment, and + // retrying only the rename and the comparison still let concurrent + // `devmap claude` runs fail with os error 32; the whole write waits it + // out instead, within the same deadline. + #[cfg(windows)] + { + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; + loop { + match write_atomic_once(path, content) { + Err(error) + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + outcome => return outcome, + } + } + } + #[cfg(not(windows))] + write_atomic_once(path, content) +} + +fn write_atomic_once(path: &Path, content: &[u8]) -> std::io::Result { use devmap_extract::safe_fs::{Access, Creation, PinnedDir}; devmap_extract::safe_fs::preflight_write(path)?; let parent = PinnedDir::open(path.parent().unwrap_or(Path::new(".")), true)?; @@ -32,7 +75,11 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { // A concurrent atomic publisher can unlink the snapshot between open // and comparison. Re-read that snapshot within a finite retry budget; // an unsafe live file still refuses publication. - for attempt in 0..16 { + #[cfg(windows)] + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; + let mut attempt = 0; + loop { + attempt += 1; let comparison = (|| -> std::io::Result { let mut existing = parent.open_file(target, Access::Read, Creation::Never)?; existing.require_owned()?; @@ -42,9 +89,19 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { Ok(true) => return Ok(true), Ok(false) => break, Err(error) if error.kind() == std::io::ErrorKind::NotFound => break, - Err(error) if error.kind() == std::io::ErrorKind::Interrupted && attempt < 15 => { + Err(error) if error.kind() == std::io::ErrorKind::Interrupted && attempt < 16 => { continue } + // Another writer is mid-rename onto this name. It failed the + // whole emission (concurrent_writers_to_one_artifact_all_succeed + // and concurrent `devmap claude` on Windows): wait it out. + #[cfg(windows)] + Err(error) + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(5)); + continue; + } Err(error) => return Err(error), } } @@ -83,18 +140,15 @@ pub fn write_atomic(path: &Path, content: &[u8]) -> std::io::Result { } #[cfg(windows)] { - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); - let mut attempts = 0; + // One deadline, not an attempt count as well: under sixteen-way + // contention a hundred 5 ms attempts ran out long before the time. + let deadline = std::time::Instant::now() + WINDOWS_SHARING_DEADLINE; loop { match parent.rename_child(&temporary, target) { Ok(()) => break, Err(error) - if (error.kind() == std::io::ErrorKind::PermissionDenied - || matches!(error.raw_os_error(), Some(32 | 33))) - && attempts < 100 - && std::time::Instant::now() < deadline => + if is_sharing_violation(&error) && std::time::Instant::now() < deadline => { - attempts += 1; std::thread::sleep(std::time::Duration::from_millis(5)); } Err(error) => return Err(error), diff --git a/rust/devmap-resolve/src/lsp.rs b/rust/devmap-resolve/src/lsp.rs index 338a089c..6e520116 100644 --- a/rust/devmap-resolve/src/lsp.rs +++ b/rust/devmap-resolve/src/lsp.rs @@ -10,12 +10,14 @@ use std::collections::{BTreeMap, BTreeSet}; use std::io::{BufRead, BufReader, Read, Write}; use std::path::{Path, PathBuf}; -use std::process::{Child, Command, Stdio}; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::mpsc::{self, Receiver, RecvTimeoutError, SyncSender}; use std::sync::Arc; +use std::thread; use std::time::{Duration, Instant}; use devmap_extract::model::{EdgeKind, ExtractedSymbol, Extraction, Span}; +use devmap_extract::subprocess::{self, Session}; use serde_json::{json, Value}; use crate::model::{ @@ -39,6 +41,13 @@ pub const LSP_REQUEST_BUDGET: Duration = Duration::from_secs(8); /// rather than parsed into edges. pub const LSP_PAYLOAD_CAP: usize = 256 * 1024; +/// Longest header line accepted. A server streaming bytes with no newline would +/// otherwise grow one `String` without bound. +const LSP_HEADER_LINE_CAP: u64 = 8 * 1024; + +/// How long `shutdown` waits for a server to exit on its own before killing it. +const LSP_EXIT_GRACE: Duration = Duration::from_millis(500); + /// Servers this pass will start, in order, and only when the binary is on /// `PATH`. A missing binary is a status line that the pass did not run — never /// "zero new edges from a run". @@ -603,10 +612,25 @@ fn which_binary(name: &str) -> Option { None } +/// What the reader thread hands back: a decoded frame, or why it stopped. +enum Incoming { + Message(Value), + Failed(LspDidNotRun), + Eof, +} + +/// A language server session. +/// +/// Every wait here is bounded. Reads and writes happen on threads of their +/// own, and the caller only ever waits on a channel with `recv_timeout`: a +/// blocking `read_line` on the calling thread never returns from a server that +/// has gone quiet, which is how the per-request and per-pass budgets used to be +/// checked only between reads and never during one. On expiry the session is +/// killed, which closes the pipes and ends both threads. struct LspClient { - child: Child, - reader: BufReader, - writer: std::process::ChildStdin, + session: Session, + incoming: Receiver, + outgoing: Option>>, next_id: u64, server_version: Option, deadline: Instant, @@ -614,6 +638,57 @@ struct LspClient { repo_root: PathBuf, } +/// One frame from `reader`: headers, then a body of at most +/// [`LSP_PAYLOAD_CAP`] bytes. +fn read_frame(reader: &mut impl BufRead) -> Incoming { + let mut content_length: Option = None; + loop { + let mut line = Vec::new(); + let read = match reader + .by_ref() + .take(LSP_HEADER_LINE_CAP) + .read_until(b'\n', &mut line) + { + Ok(read) => read, + Err(error) => { + return Incoming::Failed(LspDidNotRun::Protocol(format!( + "read header failed: {error}" + ))) + } + }; + if read == 0 { + return Incoming::Eof; + } + if !line.ends_with(b"\n") { + return Incoming::Failed(LspDidNotRun::Protocol(format!( + "header line longer than {LSP_HEADER_LINE_CAP} bytes" + ))); + } + let text = String::from_utf8_lossy(&line); + let trimmed = text.trim_end(); + if trimmed.is_empty() { + break; + } + if let Some(value) = trimmed.strip_prefix("Content-Length:") { + content_length = value.trim().parse().ok(); + } + } + let Some(length) = content_length else { + return Incoming::Failed(LspDidNotRun::Protocol("missing Content-Length".into())); + }; + if length > LSP_PAYLOAD_CAP { + return Incoming::Failed(LspDidNotRun::OversizedPayload { bytes: length }); + } + let mut body = vec![0u8; length]; + if let Err(error) = reader.read_exact(&mut body) { + return Incoming::Failed(LspDidNotRun::Protocol(format!("read body failed: {error}"))); + } + match serde_json::from_slice(&body) { + Ok(value) => Incoming::Message(value), + Err(error) => Incoming::Failed(LspDidNotRun::Protocol(format!("json failed: {error}"))), + } +} + impl LspClient { fn spawn( spec: &LspServerSpec, @@ -621,28 +696,48 @@ impl LspClient { repo_root: &Path, deadline: Instant, ) -> Result { - let mut command = Command::new(binary); - command - .args(spec.args) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .current_dir(repo_root); - let mut child = command - .spawn() + let mut command = subprocess::session_command(binary); + command.args(spec.args).current_dir(repo_root); + let spawn_budget = deadline.saturating_duration_since(Instant::now()); + let mut session = subprocess::spawn_session(&mut command, spawn_budget) .map_err(|error| LspDidNotRun::Protocol(format!("spawn {}: {error}", spec.binary)))?; - let stdout = child - .stdout - .take() + let stdout = session + .take_stdout() .ok_or_else(|| LspDidNotRun::Protocol("missing stdout".into()))?; - let stdin = child - .stdin - .take() + let mut stdin = session + .take_stdin() .ok_or_else(|| LspDidNotRun::Protocol("missing stdin".into()))?; + + // Bounded to a few frames: a server that stops reading backs the + // writer up, and the caller's recv_timeout on the reply is what ends it. + let (incoming_tx, incoming) = mpsc::sync_channel(16); + thread::spawn(move || { + let mut reader = BufReader::new(stdout); + loop { + let frame = read_frame(&mut reader); + let last = !matches!(frame, Incoming::Message(_)); + if incoming_tx.send(frame).is_err() || last { + return; + } + } + }); + let (outgoing, outgoing_rx) = mpsc::sync_channel::>(16); + thread::spawn(move || { + for frame in outgoing_rx { + if stdin + .write_all(&frame) + .and_then(|()| stdin.flush()) + .is_err() + { + return; + } + } + }); + let mut client = Self { - child, - reader: BufReader::new(stdout), - writer: stdin, + session, + incoming, + outgoing: Some(outgoing), next_id: 1, server_version: None, deadline, @@ -715,7 +810,13 @@ impl LspClient { fn shutdown(&mut self) -> Result<(), LspDidNotRun> { let _ = self.request("shutdown", json!(null)); let _ = self.notify("exit", json!(null)); - let _ = self.child.wait(); + // A polite exit gets a grace period, never an unbounded wait: a server + // that ignores `exit` is killed with the rest of its process group. + let grace = Instant::now() + LSP_EXIT_GRACE; + while self.session.try_status().is_none() && Instant::now() < grace { + thread::sleep(Duration::from_millis(10)); + } + self.kill(); Ok(()) } @@ -769,67 +870,50 @@ impl LspClient { if body.len() > LSP_PAYLOAD_CAP { return Err(LspDidNotRun::OversizedPayload { bytes: body.len() }); } - write!(self.writer, "Content-Length: {}\r\n\r\n", body.len()) - .map_err(|error| LspDidNotRun::Protocol(format!("write header failed: {error}")))?; - self.writer - .write_all(&body) - .map_err(|error| LspDidNotRun::Protocol(format!("write body failed: {error}")))?; - self.writer - .flush() - .map_err(|error| LspDidNotRun::Protocol(format!("flush failed: {error}")))?; - Ok(()) + let mut frame = format!("Content-Length: {}\r\n\r\n", body.len()).into_bytes(); + frame.extend_from_slice(&body); + let Some(outgoing) = &self.outgoing else { + return Err(LspDidNotRun::Protocol("the session is closed".into())); + }; + // try_send, not send: a writer backed up behind a server that stopped + // reading must not block this thread past the deadline. + match outgoing.try_send(frame) { + Ok(()) => Ok(()), + Err(mpsc::TrySendError::Full(_)) => { + self.kill(); + Err(LspDidNotRun::TimedOut) + } + Err(mpsc::TrySendError::Disconnected(_)) => { + Err(LspDidNotRun::Protocol("write to server failed".into())) + } + } } fn read_message(&mut self, request_deadline: Instant) -> Result { - let mut content_length: Option = None; - loop { - if Instant::now() >= request_deadline { + let wait = request_deadline.saturating_duration_since(Instant::now()); + match self.incoming.recv_timeout(wait) { + Ok(Incoming::Message(value)) => Ok(value), + Ok(Incoming::Failed(reason)) => { self.kill(); - return Err(LspDidNotRun::TimedOut); - } - let mut line = String::new(); - let read = self - .reader - .read_line(&mut line) - .map_err(|error| LspDidNotRun::Protocol(format!("read header failed: {error}")))?; - if read == 0 { - let status = self.child.try_wait().ok().flatten(); - return Err(LspDidNotRun::NonZeroExit { - code: status.and_then(|status| status.code()), - }); + Err(reason) } - let trimmed = line.trim_end(); - if trimmed.is_empty() { - break; + Ok(Incoming::Eof) | Err(RecvTimeoutError::Disconnected) => { + let code = self.session.try_status().and_then(|status| status.code()); + self.kill(); + Err(LspDidNotRun::NonZeroExit { code }) } - if let Some(value) = trimmed.strip_prefix("Content-Length:") { - content_length = value.trim().parse().ok(); + Err(RecvTimeoutError::Timeout) => { + self.kill(); + Err(LspDidNotRun::TimedOut) } } - let length = content_length - .ok_or_else(|| LspDidNotRun::Protocol("missing Content-Length".into()))?; - if length > LSP_PAYLOAD_CAP { - self.kill(); - return Err(LspDidNotRun::OversizedPayload { bytes: length }); - } - let mut body = vec![0u8; length]; - self.reader - .read_exact(&mut body) - .map_err(|error| LspDidNotRun::Protocol(format!("read body failed: {error}")))?; - serde_json::from_slice(&body) - .map_err(|error| LspDidNotRun::Protocol(format!("json failed: {error}"))) } fn kill(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); - } -} - -impl Drop for LspClient { - fn drop(&mut self) { - let _ = self.child.kill(); - let _ = self.child.wait(); + // Dropping the sender ends the writer thread once it is unblocked; the + // kill closes the pipes, which unblocks both threads. + self.outgoing = None; + self.session.kill(); } } @@ -906,3 +990,88 @@ fn location_from_value(value: &Value, repo_root: &Path) -> Option { character, }) } + +#[cfg(all(test, unix))] +mod session_bounds { + use super::*; + + fn spec(script: String) -> LspServerSpec { + LspServerSpec { + binary: "sh", + args: Box::leak( + vec!["-c", Box::leak(script.into_boxed_str()) as &str].into_boxed_slice(), + ), + extensions: &[".rs"], + } + } + + fn alive(pid: i32) -> bool { + let mut probe = subprocess::session_command("kill"); + probe.args(["-0", &pid.to_string()]); + let bounds = subprocess::Bounds { + deadline: Duration::from_secs(5), + stdout_cap: 1024, + stderr_cap: 1024, + }; + subprocess::run_bounded(&mut probe, bounds).is_ok_and(|done| done.status.success()) + } + + /// A server that reads everything and never answers. The request budget + /// used to be checked only between blocking reads, so this hung the pass + /// forever; it must now end at the deadline, with the server's own child + /// killed along with it. + #[test] + fn a_silent_server_times_out_at_the_deadline_and_takes_its_children_with_it() { + let dir = std::env::temp_dir().join(format!("lsp-silent-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let pidfile = dir.join("child.pid"); + let script = format!( + "sleep 1000 & echo $! > {}; cat > /dev/null", + pidfile.display() + ); + let started = Instant::now(); + let deadline = started + Duration::from_secs(2); + let result = LspClient::spawn(&spec(script), Path::new("sh"), &dir, deadline); + let elapsed = started.elapsed(); + assert!( + matches!(result, Err(LspDidNotRun::TimedOut)), + "a silent server must time out, got {:?}", + result.err() + ); + assert!( + elapsed < Duration::from_secs(5), + "took {elapsed:?} against a 2s deadline" + ); + let pid: i32 = std::fs::read_to_string(&pidfile) + .unwrap() + .trim() + .parse() + .unwrap(); + let reaped_by = Instant::now() + Duration::from_secs(2); + while alive(pid) && Instant::now() < reaped_by { + thread::sleep(Duration::from_millis(20)); + } + assert!(!alive(pid), "the server's child {pid} outlived the kill"); + let _ = std::fs::remove_dir_all(&dir); + } + + /// Bytes with no newline used to grow one header String without bound. + #[test] + fn an_endless_header_line_is_refused_not_buffered() { + let script = "while :; do printf 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; done".to_string(); + let started = Instant::now(); + let result = LspClient::spawn( + &spec(script), + Path::new("sh"), + &std::env::temp_dir(), + started + Duration::from_secs(20), + ); + match result { + Err(LspDidNotRun::Protocol(reason)) => { + assert!(reason.contains("header line longer"), "{reason}") + } + other => panic!("expected a header-cap refusal, got {:?}", other.err()), + } + assert!(started.elapsed() < Duration::from_secs(10)); + } +} diff --git a/rust/devmap-serve/src/mcp.rs b/rust/devmap-serve/src/mcp.rs index 2a19601c..8a73b060 100644 --- a/rust/devmap-serve/src/mcp.rs +++ b/rust/devmap-serve/src/mcp.rs @@ -620,8 +620,18 @@ pub const MAX_FRAME_BYTES: usize = 1024 * 1024; /// only kill. const CALL_TIMEOUT: Duration = Duration::from_secs(30); -/// A response must acquire its writer and reach the peer within this bound. -/// A stopped reader must not retain every admission permit indefinitely. +/// A response's write and flush must reach the peer within this bound once it +/// holds the writer. A stopped reader must not retain every admission permit +/// indefinitely: the write that meets it fails here and marks the stream +/// failed, and every response queued behind it then fails at once. +/// +/// It does not cover the wait for the writer. It used to, and on a loaded +/// machine a reply queued behind the other in-flight replies to a peer that +/// was reading perfectly well ran out of its 5s before it was written, which +/// ended the whole session (serve_stress's ten-thousand-request flood on +/// macOS). Queueing behind a peer that is making progress is backpressure, +/// already bounded by the admission ceiling; only a peer that has stopped +/// reading is a failure. const RESPONSE_WRITE_TIMEOUT: Duration = Duration::from_secs(5); /// Requests one connection may have outstanding at once. @@ -3526,6 +3536,17 @@ async fn write_frame( writer: &tokio::sync::Mutex>, frame: &Value, ) -> anyhow::Result<()> +where + W: AsyncWrite + Unpin, +{ + write_frame_within(writer, frame, RESPONSE_WRITE_TIMEOUT).await +} + +async fn write_frame_within( + writer: &tokio::sync::Mutex>, + frame: &Value, + bound: Duration, +) -> anyhow::Result<()> where W: AsyncWrite + Unpin, { @@ -3533,27 +3554,148 @@ where payload.push(b'\n'); // The lock spans the write and the flush, so two concurrent responses cannot // interleave their bytes into one unparseable line. - tokio::time::timeout(RESPONSE_WRITE_TIMEOUT, async { - let mut writer = writer.lock().await; - anyhow::ensure!( - !writer.failed, - "MCP response stream failed on an earlier frame" - ); - // Leave this set if writing fails or this future is cancelled. No - // concurrent handler may append another reply to a partial JSON frame. - writer.failed = true; + let mut writer = writer.lock().await; + anyhow::ensure!( + !writer.failed, + "MCP response stream failed on an earlier frame" + ); + // Leave this set if writing fails or times out. No concurrent handler may + // append another reply to a partial JSON frame. + writer.failed = true; + tokio::time::timeout(bound, async { writer.stream.write_all(&payload).await?; // Buffered output can deadlock a request/response protocol, so flushing // shares the write deadline and mutex rather than extending either. writer.stream.flush().await?; - writer.failed = false; - Ok::<(), anyhow::Error>(()) + Ok::<(), std::io::Error>(()) }) .await - .map_err(|_| anyhow::anyhow!("MCP response write exceeded {RESPONSE_WRITE_TIMEOUT:?}"))??; + .map_err(|_| anyhow::anyhow!("MCP response write exceeded {bound:?}"))??; + writer.failed = false; Ok(()) } +#[cfg(test)] +mod response_write_bounds { + use super::{write_frame_within, ResponseWriter}; + use serde_json::json; + use std::pin::Pin; + use std::sync::Arc; + use std::task::{Context, Poll}; + use std::time::{Duration, Instant}; + use tokio::io::AsyncWrite; + + /// A peer that reads every byte, a little slowly: each write takes `delay`. + struct SlowSink { + delay: Duration, + sleep: Option>>, + } + + impl AsyncWrite for SlowSink { + fn poll_write( + mut self: Pin<&mut Self>, + cx: &mut Context<'_>, + buf: &[u8], + ) -> Poll> { + let delay = self.delay; + let sleep = self + .sleep + .get_or_insert_with(|| Box::pin(tokio::time::sleep(delay))); + match sleep.as_mut().poll(cx) { + Poll::Ready(()) => { + self.sleep = None; + Poll::Ready(Ok(buf.len())) + } + Poll::Pending => Poll::Pending, + } + } + fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + } + + /// A peer that has stopped reading. + struct StoppedSink; + + impl AsyncWrite for StoppedSink { + fn poll_write( + self: Pin<&mut Self>, + _: &mut Context<'_>, + _: &[u8], + ) -> Poll> { + Poll::Pending + } + fn poll_flush(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Pending + } + fn poll_shutdown(self: Pin<&mut Self>, _: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + } + + use std::future::Future; + + /// Forty replies queued behind each other to a peer that reads every one + /// within the bound. The queue as a whole takes eight bounds; no reply may + /// fail for having waited its turn. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn a_reading_peer_is_not_failed_for_the_length_of_the_queue() { + let bound = Duration::from_millis(200); + let writer = Arc::new(tokio::sync::Mutex::new(ResponseWriter { + stream: SlowSink { + delay: Duration::from_millis(40), + sleep: None, + }, + failed: false, + })); + let replies: Vec<_> = (0..40) + .map(|id| { + let writer = Arc::clone(&writer); + tokio::spawn(async move { + write_frame_within(&writer, &json!({ "id": id }), bound).await + }) + }) + .collect(); + for reply in replies { + reply + .await + .expect("task") + .expect("a reading peer must get every reply"); + } + } + + /// A stopped peer still releases everyone promptly: the write that meets + /// it fails at the bound, and the rest fail at once behind it. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn a_stopped_peer_fails_every_queued_reply_within_about_one_bound() { + let bound = Duration::from_millis(200); + let writer = Arc::new(tokio::sync::Mutex::new(ResponseWriter { + stream: StoppedSink, + failed: false, + })); + let started = Instant::now(); + let replies: Vec<_> = (0..32) + .map(|id| { + let writer = Arc::clone(&writer); + tokio::spawn(async move { + write_frame_within(&writer, &json!({ "id": id }), bound).await + }) + }) + .collect(); + for reply in replies { + assert!(reply.await.expect("task").is_err()); + } + assert!( + started.elapsed() < bound * 4, + "queued replies to a stopped peer took {:?}", + started.elapsed() + ); + } +} + #[cfg(test)] mod transport_audit_tests { use super::{read_frame, serve_streams, Frame, StoreSlot, MAX_FRAME_BYTES}; diff --git a/rust/devmap-serve/src/root_resolve.rs b/rust/devmap-serve/src/root_resolve.rs index 69109140..bd936887 100644 --- a/rust/devmap-serve/src/root_resolve.rs +++ b/rust/devmap-serve/src/root_resolve.rs @@ -177,11 +177,8 @@ pub fn validate_repo_path(raw: &str) -> Result { raw.len() )); } - if looks_like_windows_path(raw) { - return Err( - "repo_path looks like a Windows path; this server only opens unix filesystem paths" - .into(), - ); + if let Some(refusal) = foreign_path_refusal(raw) { + return Err(refusal.into()); } let path = Path::new(raw); if !path.is_absolute() { @@ -202,11 +199,58 @@ pub fn validate_repo_path(raw: &str) -> Result { }) } -/// MCP `roots/list` URI → a unix filesystem path, or `None` to skip. +/// Why `raw` is a path from the other platform, if it is. +/// +/// A path is opened only in its host's own spelling, never reinterpreted: a +/// unix server refuses `C:\…` and `/C:/…` rather than reading them as +/// relative names or a `/C:` directory, and a Windows server refuses `/Users/…` +/// rather than resolving it against the current drive. This used to be the +/// unix rule on every host, so on Windows every native path was refused with a +/// message saying the server "only opens unix filesystem paths". +#[cfg(not(windows))] +fn foreign_path_refusal(raw: &str) -> Option<&'static str> { + looks_like_windows_path(raw).then_some( + "repo_path looks like a Windows path; this server only opens unix filesystem paths", + ) +} + +#[cfg(windows)] +fn foreign_path_refusal(raw: &str) -> Option<&'static str> { + // `\\server\share`, `//server/share` and `\\?\C:\…` are native; a single + // leading slash is a unix path with no drive. + let unix_style = raw.starts_with('/') && !raw.starts_with("//"); + unix_style.then_some( + "repo_path looks like a unix path; this server only opens Windows paths with a drive or UNC share", + ) +} + +/// MCP `roots/list` URI → a path on this host, or `None` to skip. /// -/// Windows-style (`file:///C:/…`) and non-file URIs are skipped rather than -/// reinterpreted. Percent-decoding refuses invalid UTF-8 rather than using -/// lossy replacement. +/// On unix, Windows-style (`file:///C:/…`) and non-file URIs are skipped +/// rather than reinterpreted. On Windows, only `file:///:/…` is taken, +/// and a drive-less URI (`file:///Users/…`) is skipped. Percent-decoding +/// refuses invalid UTF-8 rather than using lossy replacement. +#[cfg(windows)] +pub fn file_uri_to_path(uri: &str) -> Option { + let rest = uri.strip_prefix("file://")?; + // `file://localhost/C:/…` names the same file as `file:///C:/…`; any other + // authority is a remote host. + let rest = rest.strip_prefix("localhost").unwrap_or(rest); + let path = rest.strip_prefix('/')?; + let decoded = percent_decode_strict(path)?; + let bytes = decoded.as_bytes(); + let drive_absolute = bytes.len() >= 3 + && bytes[0].is_ascii_alphabetic() + && bytes[1] == b':' + && (bytes[2] == b'/' || bytes[2] == b'\\'); + if !drive_absolute { + return None; + } + Some(PathBuf::from(decoded.replace('/', "\\"))) +} + +/// See the Windows variant above. +#[cfg(not(windows))] pub fn file_uri_to_path(uri: &str) -> Option { let path = if let Some(rest) = uri.strip_prefix("file://") { if rest.starts_with('/') { @@ -404,6 +448,21 @@ mod tests { use std::fs; use std::time::{SystemTime, UNIX_EPOCH}; + /// Whether `text` names `path` in a spelling the platform gives it: as + /// written, or canonical. Windows canonicalises to a `\\?\` verbatim path + /// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and + /// macOS to `/private/var` for `/var`; every one is the same directory. + fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) + } + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-root-resolve-{name}-{}-{}", @@ -443,11 +502,11 @@ mod tests { .resolve() .expect_err("cwd store and MCP-root store must not first-wins"); assert!( - err.contains(&via_root.display().to_string()), + names(&err, &via_root), "error must name the MCP root: {err}" ); assert!( - err.contains(&via_cwd.display().to_string()), + names(&err, &via_cwd), "error must name the cwd/--root repository: {err}" ); assert!( @@ -637,12 +696,9 @@ mod tests { } .resolve() .expect_err("two stores must not first-wins"); + assert!(names(&err, &first), "error must name the first root: {err}"); assert!( - err.contains(&first.display().to_string()), - "error must name the first root: {err}" - ); - assert!( - err.contains(&second.display().to_string()), + names(&err, &second), "error must name the second root: {err}" ); assert!( @@ -653,6 +709,7 @@ mod tests { ); } + #[cfg(not(windows))] #[test] fn windows_file_uri_is_not_reinterpreted_as_a_unix_path() { assert!(file_uri_to_path("file:///C:/Users/nobody/project").is_none()); @@ -677,4 +734,38 @@ mod tests { "a single-slash file: URI is not a unix path" ); } + + #[cfg(windows)] + #[test] + fn a_unix_file_uri_is_not_reinterpreted_on_windows() { + assert_eq!( + file_uri_to_path("file:///C:/Users/example/project").unwrap(), + PathBuf::from(r"C:\Users\example\project") + ); + assert_eq!( + file_uri_to_path("file://localhost/D:/src/a%20b").unwrap(), + PathBuf::from(r"D:\src\a b") + ); + assert!( + file_uri_to_path("file:///Users/example/project").is_none(), + "a drive-less URI must not resolve against the current drive" + ); + assert!(file_uri_to_path("file://example.com/C:/x").is_none()); + assert!(file_uri_to_path("file:///C:/tmp/foo%00bar").is_none()); + assert!(file_uri_to_path("https://example.com/repo").is_none()); + } + + #[test] + fn repo_path_is_refused_in_the_other_platform_s_spelling() { + #[cfg(not(windows))] + let (foreign, marker) = (r"C:\Users\nobody\project", "Windows path"); + #[cfg(windows)] + let (foreign, marker) = ("/Users/nobody/project", "unix path"); + let err = validate_repo_path(foreign).expect_err("a foreign path must be refused"); + assert!(err.contains(marker), "{err}"); + // And a real directory in this host's own spelling opens. + let here = scratch("native-spelling"); + let opened = validate_repo_path(here.to_str().unwrap()).expect("native path opens"); + assert_eq!(opened, here.canonicalize().unwrap()); + } } diff --git a/rust/devmap-serve/tests/mcp_repo_scope.rs b/rust/devmap-serve/tests/mcp_repo_scope.rs index 63a4cda1..3056423a 100644 --- a/rust/devmap-serve/tests/mcp_repo_scope.rs +++ b/rust/devmap-serve/tests/mcp_repo_scope.rs @@ -15,6 +15,34 @@ use devmap_serve::mcp::{ use devmap_store::Store; use serde_json::{json, Value}; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + +/// `path` as a `file://` URI in this platform's form: `file:///C:/…` on +/// Windows. `format!("file://{}", path.display())` gives `file://C:\…` there, +/// which is not a file URI at all. +fn file_uri(path: &std::path::Path) -> String { + let shown = path.display().to_string(); + if cfg!(windows) { + let bare = shown.strip_prefix(r"\\?\").unwrap_or(&shown); + format!("file:///{}", bare.replace('\\', "/")) + } else { + format!("file://{shown}") + } +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-mcp-scope-{name}-{}-{}", @@ -333,8 +361,7 @@ async fn cwd_store_and_different_mcp_root_without_repo_path_is_refused() { let response = call(&slot, "devmap_status", json!({})).await; let text = tool_error_text(&response); assert!( - text.contains(&via_roots.display().to_string()) - && text.contains(&via_cwd.display().to_string()), + names(&text, &via_roots) && names(&text, &via_cwd), "must name both the MCP root and the cwd/--root repository: {text}" ); @@ -447,7 +474,7 @@ async fn preview_without_repo_path_and_two_candidate_roots_is_refused() { .await; let text = tool_error_text(&response); assert!( - text.contains(&a.display().to_string()) && text.contains(&b.display().to_string()), + names(&text, &a) && names(&text, &b), "preview must name both candidate roots, not judge the first: {text}" ); } @@ -592,6 +619,7 @@ async fn two_spellings_of_one_case_insensitive_path_open_one_canonical_root() { ); } +#[cfg(not(windows))] #[tokio::test] async fn windows_file_uri_is_rejected_not_reinterpreted() { let unix = scratch("uri-unix"); @@ -659,8 +687,7 @@ async fn mixed_calls_cannot_first_wins_across_cwd_and_mcp_root() { 0 => { let text = tool_error_text(&call(&slot, "devmap_status", json!({})).await); assert!( - text.contains(&via_roots.display().to_string()) - && text.contains(&via_cwd.display().to_string()), + names(&text, &via_roots) && names(&text, &via_cwd), "iteration {i}: empty args must stay ambiguous: {text}" ); } @@ -809,12 +836,12 @@ async fn repo_path_against_a_foreign_root_pin_is_a_tool_error_naming_both() { .await; let text = tool_error_text(&response); assert!( - text.contains(&pinned.display().to_string()) + names(&text, &pinned) || text.contains(&pinned.canonicalize().unwrap().display().to_string()), "error must name the --root pin: {text}" ); assert!( - text.contains(&foreign.display().to_string()) + names(&text, &foreign) || text.contains(&foreign.canonicalize().unwrap().display().to_string()), "error must name the refused repo_path: {text}" ); @@ -832,7 +859,7 @@ async fn late_devmap_roots_reply_is_ignored_after_a_newer_seq() { "jsonrpc": "2.0", "id": "devmap-roots-2", "result": { - "roots": [{"uri": format!("file://{}", newer.display())}] + "roots": [{"uri": file_uri(&newer)}] } }); assert!( @@ -844,7 +871,7 @@ async fn late_devmap_roots_reply_is_ignored_after_a_newer_seq() { "jsonrpc": "2.0", "id": "devmap-roots-1", "result": { - "roots": [{"uri": format!("file://{}", older.display())}] + "roots": [{"uri": file_uri(&older)}] } }); assert!(handle_line(&slot, &apply_older.to_string()).await.is_none()); @@ -893,7 +920,9 @@ async fn roots_list_error_keeps_the_previous_good_list() { #[test] fn parse_roots_list_caps_at_max_and_reports_overflow() { let roots: Vec = (0..200) - .map(|i| json!({"uri": format!("file:///tmp/devmap-overflow-{i}")})) + // In the host's own spelling: a Windows server refuses a drive-less + // `file:///tmp/...` as a unix path, which left nothing to cap. + .map(|i| json!({"uri": file_uri(&std::env::temp_dir().join(format!("devmap-overflow-{i}")))})) .collect(); let (paths, _skipped, overflow) = devmap_serve::mcp::parse_roots_list_result(&json!({"roots": roots})); @@ -904,3 +933,28 @@ fn parse_roots_list_caps_at_max_and_reports_overflow() { ); assert_eq!(overflow, 200 - devmap_serve::root_resolve::MAX_ROOTS); } + +/// The same rule from the other side: a Windows server does not resolve a +/// drive-less unix path against its current drive. +#[cfg(windows)] +#[tokio::test] +async fn unix_file_uri_is_rejected_not_reinterpreted_on_windows() { + let native = scratch("uri-native"); + plant_store(&native); + let slot = Arc::new(StoreSlot::resolving(None, native.clone(), None)); + let response = json!({ + "jsonrpc": "2.0", + "id": ROOTS_LIST_REQUEST_ID, + "result": { + "roots": [{"uri": "file:///Users/nobody/project"}] + } + }); + assert!(handle_line(&slot, &response.to_string()).await.is_none()); + assert!( + !slot + .candidate_roots() + .iter() + .any(|root| root.to_string_lossy().contains("nobody")), + "a unix URI must not become a root on Windows" + ); +} diff --git a/rust/devmap-serve/tests/phase1_freshness_reach.rs b/rust/devmap-serve/tests/phase1_freshness_reach.rs index 51e2ceaa..eb5e79d0 100644 --- a/rust/devmap-serve/tests/phase1_freshness_reach.rs +++ b/rust/devmap-serve/tests/phase1_freshness_reach.rs @@ -12,6 +12,34 @@ use devmap_serve::RootResolveInput; use devmap_store::Store; use serde_json::json; +/// Whether `text` names `path` in a spelling the platform gives it: as +/// written, or canonical. Windows canonicalises to a `\\?\` verbatim path +/// with long names where the temp dir used 8.3 short ones (`RUNNER~1`), and +/// macOS to `/private/var` for `/var`; every one is the same directory. +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + +/// `path` as a `file://` URI in this platform's form: `file:///C:/…` on +/// Windows. `format!("file://{}", path.display())` gives `file://C:\…` there, +/// which is not a file URI at all. +fn file_uri(path: &std::path::Path) -> String { + let shown = path.display().to_string(); + if cfg!(windows) { + let bare = shown.strip_prefix(r"\\?\").unwrap_or(&shown); + format!("file:///{}", bare.replace('\\', "/")) + } else { + format!("file://{shown}") + } +} + fn scratch(name: &str) -> PathBuf { let root = std::env::temp_dir().join(format!( "devmap-p1-{name}-{}-{}", @@ -99,7 +127,7 @@ async fn mcp_roots_list_does_not_silently_replace_a_cwd_store() { "jsonrpc": "2.0", "id": ROOTS_LIST_REQUEST_ID, "result": { - "roots": [{"uri": format!("file://{}", repo_b.display())}] + "roots": [{"uri": file_uri(&repo_b)}] } }); assert!(handle_line(&slot, &response.to_string()).await.is_none()); @@ -108,7 +136,7 @@ async fn mcp_roots_list_does_not_silently_replace_a_cwd_store() { Err(err) => err, }; assert!( - err.contains(&repo_a.display().to_string()) && err.contains(&repo_b.display().to_string()), + names(&err, &repo_a) && names(&err, &repo_b), "must name both repositories: {err}" ); } @@ -124,7 +152,7 @@ async fn mcp_roots_list_opens_when_cwd_has_no_store() { "jsonrpc": "2.0", "id": ROOTS_LIST_REQUEST_ID, "result": { - "roots": [{"uri": format!("file://{}", repo.display())}] + "roots": [{"uri": file_uri(&repo)}] } }); assert!(handle_line(&slot, &response.to_string()).await.is_none()); @@ -192,7 +220,20 @@ async fn concurrent_status_while_store_deleted_mid_session_fails_closed() { let store_path = plant_store(&root); let slot = Arc::new(StoreSlot::resolving(None, root.clone(), None)); let _ = slot.get().expect("open"); - std::fs::remove_file(&store_path).unwrap(); + if let Err(error) = std::fs::remove_file(&store_path) { + // Windows refuses to delete a file an open handle holds without + // FILE_SHARE_DELETE (os error 32), so the mid-session deletion this + // test exercises cannot happen there: the store stays whole and keeps + // serving. Anything else is a real failure. + assert!( + cfg!(windows) && error.raw_os_error() == Some(32), + "removing the store failed: {error}" + ); + assert!(store_path.is_file(), "a refused delete leaves the store"); + slot.get() + .expect("the store the delete could not touch still serves"); + return; + } let err = match slot.get() { Ok(_) => panic!("deleted store must not reopen"), Err(err) => err, diff --git a/rust/devmap-store/Cargo.toml b/rust/devmap-store/Cargo.toml index d8c1ac57..e1e42df8 100644 --- a/rust/devmap-store/Cargo.toml +++ b/rust/devmap-store/Cargo.toml @@ -34,6 +34,11 @@ parse = ["devmap-extract/parse", "devmap-resolve/parse", "devmap-analyze/parse"] name = "retention_churn" required-features = ["parse"] +# Builds its generations from extracted source. +[[test]] +name = "symbols_for_file" +required-features = ["parse"] + # The carry-forward guards these tests exercise only exist when an identity can # be computed: with `parse` off, `current_payload_identity` returns `None`, no # payload is ever carried, and the partial-extraction write is refused outright diff --git a/rust/devmap-store/src/db.rs b/rust/devmap-store/src/db.rs index cc50d3f5..a6feb8c3 100644 --- a/rust/devmap-store/src/db.rs +++ b/rust/devmap-store/src/db.rs @@ -32,6 +32,19 @@ impl std::fmt::Display for StoreRefusal { impl std::error::Error for StoreRefusal {} +/// Held while a connection opens and sets itself up: for a read-only +/// connection, its open and first read, the moment it maps the WAL index; for +/// a read-write one, the whole of `open_once`, through `enable_wal` and +/// `migrate`. Concurrent connections in one process setting the WAL index up +/// together livelocked on Windows: every one of them got SQLITE_PROTOCOL for +/// as long as they kept retrying (`concurrent_readers_cannot_enqueue_writer_work` +/// for readers, `concurrent_v19_openers_keep_pending_work_and_one_durable_identity` +/// for sixteen writers, which exhausted the 20 s retry deadline). Serialised, +/// the first sets it up and the rest find it ready. Other processes still +/// coordinate through SQLite's own locks, and everything after the open runs +/// concurrently. +static CONNECTION_SETUP: std::sync::Mutex<()> = std::sync::Mutex::new(()); + fn refusal(message: impl Into) -> rusqlite::Error { rusqlite::Error::ToSqlConversionFailure(Box::new(StoreRefusal(message.into()))) } @@ -1011,6 +1024,20 @@ pub struct FileEdges { pub analysis: Option, } +/// Symbols declared in one file in the latest generation, with that +/// generation's identity so a caller can compare `head_sha` to the one they +/// asked for. +/// +/// Travels as one page for the same reason [`SearchPage`] does: the rows and +/// the generation they came from must be one snapshot. A second "latest" +/// read for the head SHA could describe a different generation. +#[derive(Debug, Clone)] +pub struct FileSymbolsPage { + pub generation: u32, + pub head_sha: String, + pub rows: Vec, +} + #[derive(Debug, Clone, PartialEq)] pub struct StoredSymbol { pub name: String, @@ -3160,7 +3187,11 @@ impl Store { /// even when the application has write access to the file. A writer must /// upgrade an older store explicitly before an advisory reader can use it. pub fn open_read_only>(db_path: P) -> Result { - let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path.as_ref()) + Self::retrying_lost_races(|| Self::open_read_only_once(db_path.as_ref())) + } + + fn open_read_only_once(db_path: &Path) -> Result { + let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path) .map_err(|error| refusal(error.to_string()))?; let path = resolved.as_path(); Self::validate_database_file(path)?; @@ -3177,12 +3208,17 @@ impl Store { ))); } let _sidecars = Self::checked_sidecars(path)?; + let setup = CONNECTION_SETUP + .lock() + .unwrap_or_else(|poison| poison.into_inner()); let mut conn = Connection::open_with_flags( path, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY | rusqlite::OpenFlags::SQLITE_OPEN_NO_MUTEX, )?; conn.busy_timeout(Self::BUSY_TIMEOUT)?; - let stamped: i32 = match conn.query_row("PRAGMA user_version", [], |row| row.get(0)) { + let first_read = conn.query_row("PRAGMA user_version", [], |row| row.get(0)); + drop(setup); + let stamped: i32 = match first_read { Ok(version) => version, Err(error) if path.is_file() && Self::directory_refused_the_wal(&error) => { conn = Self::open_immutable(path)?; @@ -3210,8 +3246,53 @@ impl Store { }) } + /// How long [`Store::open`] keeps retrying `SQLITE_PROTOCOL`. + /// + /// SQLite returns it when a WAL-index lock race outlasts its own internal + /// retries, which concurrent openers of one store can provoke; it is a lost + /// race, not a damaged file, and the docs' remedy is to try again. On + /// Windows eight threads opening one legacy store all lost it + /// (`concurrent_openers_of_a_legacy_store_all_reach_the_current_schema`). + /// Opening is safe to repeat from the top: `migrate` re-reads the schema + /// version under the write lock, so a step another opener finished is not + /// run twice. + const PROTOCOL_RETRY_DEADLINE: std::time::Duration = std::time::Duration::from_secs(20); + + fn lost_a_locking_race(error: &rusqlite::Error) -> bool { + matches!( + error, + rusqlite::Error::SqliteFailure(failure, _) + if failure.code == rusqlite::ErrorCode::FileLockingProtocolFailed + ) + } + + /// Run an open, retrying `SQLITE_PROTOCOL` within + /// [`Store::PROTOCOL_RETRY_DEADLINE`]. Shared by every entry point that + /// opens a connection: read-only opens lost the same race on Windows + /// (`concurrent_readers_cannot_enqueue_writer_work`). + fn retrying_lost_races(open: impl Fn() -> Result) -> Result { + let deadline = std::time::Instant::now() + Self::PROTOCOL_RETRY_DEADLINE; + let mut pause = std::time::Duration::from_millis(10); + loop { + match open() { + Err(error) + if Self::lost_a_locking_race(&error) + && std::time::Instant::now() + pause < deadline => + { + std::thread::sleep(pause); + pause = (pause * 2).min(std::time::Duration::from_millis(500)); + } + outcome => return outcome, + } + } + } + pub fn open>(db_path: P) -> Result { - let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path.as_ref()) + Self::retrying_lost_races(|| Self::open_once(db_path.as_ref())) + } + + fn open_once(db_path: &Path) -> Result { + let resolved = devmap_extract::safe_fs::resolve_file_alias(db_path) .map_err(|error| refusal(error.to_string()))?; let path = resolved.as_path(); Self::validate_database_file(path)?; @@ -3220,6 +3301,10 @@ impl Store { // repair after `Connection::open` is a repair the connection never // sees. See `repair_sidecar_modes`. Self::repair_sidecar_modes(path)?; + // Held to the end of this open; see `CONNECTION_SETUP`. + let _setup = CONNECTION_SETUP + .lock() + .unwrap_or_else(|poison| poison.into_inner()); let mut conn = Connection::open(path)?; let store = path.display().to_string(); @@ -6218,6 +6303,58 @@ impl Store { Ok(out) } + /// Symbols declared in `path` in the latest generation. + /// + /// `None` when the store holds no generation. An empty `rows` with `Some` + /// means the generation exists and this path has no indexed symbols (or + /// is not in the generation at all) — a completed read, not a missing map. + /// + /// Bounded to one file on purpose: callers that need the whole corpus use + /// [`Self::all_symbols`]. The generation's `head_sha` travels with the + /// rows so a mismatch against the SHA the caller asked for is visible. + pub fn latest_symbols_for_file(&self, path: &str) -> Result> { + let conn = lock_conn(&self.conn)?; + let Some((snapshot, generation)) = Self::latest_snapshot(&conn)? else { + return Ok(None); + }; + let head_sha: String = snapshot.query_row( + "SELECT head_sha FROM generations WHERE id = ?1", + params![generation], + |row| row.get(0), + )?; + let mut stmt = snapshot.prepare( + "SELECT n.name, n.qualified_name, n.kind, p.path, + n.span_start, n.span_end, n.is_exported, f.content_hash + FROM generation_nodes n + JOIN generation_files f ON f.generation_id = n.generation_id AND f.file_id = n.file_id + JOIN paths p ON p.id = n.file_id + WHERE n.generation_id = ?1 AND p.path = ?2 + ORDER BY n.span_start, n.ordinal", + )?; + let rows = stmt + .query_map(params![generation, path], |row| { + let name: String = row.get(0)?; + let path: String = row.get(3)?; + let (span_start, span_end) = checked_span(&path, &name, row.get(4)?, row.get(5)?)?; + Ok(StoredSymbol { + name, + qualified_name: row.get(1)?, + kind: row.get(2)?, + path, + span_start, + span_end, + is_exported: row.get::<_, i64>(6)? != 0, + content_hash: row.get::<_, i64>(7)? as u64, + }) + })? + .collect::>>()?; + Ok(Some(FileSymbolsPage { + generation, + head_sha, + rows, + })) + } + /// Every edge of the latest generation, rendered for comparison. /// Test-facing: proving incremental output equals cold output needs the /// whole edge set, not a count. diff --git a/rust/devmap-store/src/lib.rs b/rust/devmap-store/src/lib.rs index 1e0396dd..b05fd742 100644 --- a/rust/devmap-store/src/lib.rs +++ b/rust/devmap-store/src/lib.rs @@ -13,11 +13,11 @@ pub use coverage::{ }; pub use db::{ checked_min_confidence, current_git_head, BuildHistoryRow, CallersPage, DeadPage, FileEdges, - GenerationWriteOpts, PageSizeConversion, PendingClaim, PendingEnqueueReport, PendingReconcile, - PendingSupersede, PendingWatermark, QuerySourceFreshness, SearchPage, SourceTreeDelta, Store, - StoreStatus, StoredEdge, StoredFile, StoredSymbol, UnsupportedSchema, VacuumAction, - VacuumOutcome, WalCheckpointMode, WalCheckpointResult, WriteBreakdown, WriterLock, - MAX_PENDING_ATTEMPTS, + FileSymbolsPage, GenerationWriteOpts, PageSizeConversion, PendingClaim, PendingEnqueueReport, + PendingReconcile, PendingSupersede, PendingWatermark, QuerySourceFreshness, SearchPage, + SourceTreeDelta, Store, StoreStatus, StoredEdge, StoredFile, StoredSymbol, UnsupportedSchema, + VacuumAction, VacuumOutcome, WalCheckpointMode, WalCheckpointResult, WriteBreakdown, + WriterLock, MAX_PENDING_ATTEMPTS, }; pub use edge_index::{ edge_kind_from_stored, edge_resolution, resolution_kind_from_stored, resolution_kind_label, diff --git a/rust/devmap-store/tests/kernel_defects.rs b/rust/devmap-store/tests/kernel_defects.rs index 7ba41771..b1c59d04 100644 --- a/rust/devmap-store/tests/kernel_defects.rs +++ b/rust/devmap-store/tests/kernel_defects.rs @@ -27,6 +27,19 @@ fn tmp_dir(label: &str) -> PathBuf { dir } +/// Whether `text` names `path` as written or canonical (a `\\?\` verbatim +/// path with long names on Windows, `/private/var` on macOS). +fn names(text: &str, path: &std::path::Path) -> bool { + if text.contains(&path.display().to_string()) { + return true; + } + let Ok(canonical) = path.canonicalize() else { + return false; + }; + let shown = canonical.display().to_string(); + text.contains(shown.strip_prefix(r"\\?\").unwrap_or(&shown)) +} + /// K3: a store this binary cannot open must say what to do about it. /// /// The refusal was `unsupported future schema version 99` — no store path, no @@ -48,7 +61,7 @@ fn k3_future_schema_refusal_names_the_store_the_versions_and_the_remedy() { .to_string(); assert!( - error.contains(&db_path.display().to_string()), + names(&error, &db_path), "the refusal must name the store it refused: {error}" ); assert!( @@ -90,7 +103,7 @@ fn k3_outdated_schema_refusal_names_the_store_the_versions_and_the_remedy() { .to_string(); assert!( - error.contains(&db_path.display().to_string()), + names(&error, &db_path), "the refusal must name the store it refused: {error}" ); assert!( diff --git a/rust/devmap-store/tests/symbols_for_file.rs b/rust/devmap-store/tests/symbols_for_file.rs new file mode 100644 index 00000000..5bae815c --- /dev/null +++ b/rust/devmap-store/tests/symbols_for_file.rs @@ -0,0 +1,190 @@ +//! `Store::latest_symbols_for_file`: one file's symbols from one generation. +//! +//! GitPulse's code-intel panel lists the symbols of the file a diff touches and +//! compares the generation's `head_sha` with the commit it is showing. These +//! pin the contract that comparison relies on: the rows and the SHA come from +//! the same generation, an empty store is `None` rather than an empty page, an +//! unindexed path is an empty page rather than an error, and a corrupt span is +//! refused the way every other symbol reader refuses it. + +use std::fs; +use std::path::PathBuf; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use devmap_analyze::analyze; +use devmap_extract::{extract_file, Extraction}; +use devmap_resolve::Resolver; +use devmap_store::{GenerationWriteOpts, Store}; + +fn tmp_dir(label: &str) -> PathBuf { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let stamp = SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_nanos(); + let seq = COUNTER.fetch_add(1, Ordering::Relaxed); + let dir = std::env::temp_dir().join(format!( + "devmap-symbols-for-file-{label}-{}-{stamp}-{seq}", + std::process::id() + )); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + dir +} + +fn save(store: &Store, extractions: &[Extraction], head_sha: &str) -> u32 { + let mut resolver = Resolver::new(); + resolver.index_extractions(extractions); + let resolution = resolver.resolve_all(extractions).unwrap(); + let analysis = analyze(extractions, &resolution); + store + .save_generation_with_metadata( + extractions, + &resolution, + &analysis, + GenerationWriteOpts::default(), + head_sha, + ) + .unwrap() +} + +fn corpus(lib_source: &str) -> Vec { + vec![ + extract_file("src/lib.py", lib_source), + extract_file("src/other.py", "def elsewhere():\n return 0\n"), + ] +} + +#[test] +fn an_empty_store_has_no_page_rather_than_an_empty_one() { + let store = Store::open_in_memory().unwrap(); + assert!( + store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .is_none(), + "no generation means no map, which is not the same as a file with no symbols" + ); +} + +#[test] +fn rows_are_the_files_own_symbols_in_source_order_with_their_generation() { + let store = Store::open_in_memory().unwrap(); + let generation = save( + &store, + &corpus("def second():\n return 2\n\ndef first():\n return 1\n"), + "abc123", + ); + + let page = store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists"); + assert_eq!(page.generation, generation); + assert_eq!(page.head_sha, "abc123"); + + let names: Vec<&str> = page.rows.iter().map(|row| row.name.as_str()).collect(); + assert!( + names.contains(&"second") && names.contains(&"first"), + "both declarations must be listed: {names:?}" + ); + assert!( + !names.contains(&"elsewhere"), + "another file's symbol leaked into the page: {names:?}" + ); + assert!( + page.rows.iter().all(|row| row.path == "src/lib.py"), + "every row must belong to the requested file: {:?}", + page.rows + ); + assert!( + page.rows + .windows(2) + .all(|pair| pair[0].span_start <= pair[1].span_start), + "rows must be in source order: {:?}", + page.rows + ); + assert!( + page.rows.iter().all(|row| row.span_end > row.span_start), + "spans must be real: {:?}", + page.rows + ); + + // The same rows every other symbol reader returns for this file. + let mut from_all: Vec<_> = store + .all_symbols() + .unwrap() + .into_iter() + .filter(|row| row.path == "src/lib.py") + .collect(); + let mut from_page = page.rows.clone(); + from_all.sort_by(|a, b| a.qualified_name.cmp(&b.qualified_name)); + from_page.sort_by(|a, b| a.qualified_name.cmp(&b.qualified_name)); + assert_eq!(from_page, from_all); +} + +#[test] +fn an_unindexed_path_is_an_empty_page_not_a_missing_map() { + let store = Store::open_in_memory().unwrap(); + let generation = save(&store, &corpus("def a():\n return 1\n"), "abc123"); + + let page = store + .latest_symbols_for_file("src/never_indexed.py") + .unwrap() + .expect("the generation exists even though this path is not in it"); + assert_eq!(page.generation, generation); + assert_eq!(page.head_sha, "abc123"); + assert!(page.rows.is_empty(), "{:?}", page.rows); +} + +#[test] +fn the_page_describes_the_latest_generation_only() { + let store = Store::open_in_memory().unwrap(); + save(&store, &corpus("def old_name():\n return 1\n"), "first"); + let latest = save(&store, &corpus("def new_name():\n return 1\n"), "second"); + + let page = store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists"); + assert_eq!(page.generation, latest); + assert_eq!( + page.head_sha, "second", + "the SHA must name the generation the rows came from" + ); + let names: Vec<&str> = page.rows.iter().map(|row| row.name.as_str()).collect(); + assert!(names.contains(&"new_name"), "{names:?}"); + assert!( + !names.contains(&"old_name"), + "a superseded generation's symbol was returned: {names:?}" + ); +} + +/// Same policy as S-11 in `store_hardening.rs`: a negative stored span is +/// refused, never clamped to 0 and published as real. +#[test] +fn a_corrupt_span_is_refused_rather_than_published_as_zero() { + let dir = tmp_dir("corrupt-span"); + let db_path = dir.join("index.sqlite"); + let store = Store::open(&db_path).unwrap(); + save(&store, &corpus("def a():\n return 1\n"), "abc123"); + assert!(!store + .latest_symbols_for_file("src/lib.py") + .unwrap() + .expect("a generation exists") + .rows + .is_empty()); + + { + let conn = rusqlite::Connection::open(&db_path).unwrap(); + conn.execute("UPDATE generation_nodes SET span_start = -5", []) + .unwrap(); + } + + assert!( + store.latest_symbols_for_file("src/lib.py").is_err(), + "a corrupt span must fail the read" + ); + let _ = fs::remove_dir_all(&dir); +} diff --git a/rust/gusset-engine/Cargo.lock b/rust/gusset-engine/Cargo.lock index cf9deb99..91ea4b45 100644 --- a/rust/gusset-engine/Cargo.lock +++ b/rust/gusset-engine/Cargo.lock @@ -4,7 +4,7 @@ version = 4 [[package]] name = "dc-glob" -version = "0.2.3" +version = "1.3.5" [[package]] name = "devcouncil-gusset-engine" @@ -16,7 +16,7 @@ dependencies = [ [[package]] name = "gusset" -version = "0.0.1" +version = "0.0.2" dependencies = [ "libc", "static_assertions", diff --git a/rust/gusset-engine/cgo-env.sh b/rust/gusset-engine/cgo-env.sh new file mode 100755 index 00000000..90854c9c --- /dev/null +++ b/rust/gusset-engine/cgo-env.sh @@ -0,0 +1,70 @@ +#!/usr/bin/env bash +# Build the umbrella archive and print the cgo environment that links it. +# +# eval "$(rust/gusset-engine/cgo-env.sh --export)" # a shell +# rust/gusset-engine/cgo-env.sh >> "$GITHUB_ENV" # a CI job +# rust/gusset-engine/cgo-env.sh --no-build ... # archive already built +# +# Why CGO_CFLAGS carries the archive's hash: Go's build and test caches key a +# cgo package on the text of its flags, not on the bytes of the libraries it +# links. Rebuild libgusset.a and a plain `go build` relinks the old Rust from +# the cache, and `go test` can replay a cached "ok" measured against it. CI +# restores GOCACHE, so the same holds there. A -D naming the hash changes the +# flags whenever the archive changes, which is the only thing Go looks at. +# +# Always runs the incremental cargo build: building only when the archive is +# missing linked whatever an older checkout left behind. +set -euo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +mode=env +build=1 +for arg in "$@"; do + case "$arg" in + --export) mode=export ;; + --no-build) build=0 ;; + *) echo "cgo-env.sh: unknown argument $arg" >&2; exit 2 ;; + esac +done + +if [[ ! -f "$here/../../../gusset/crates/gusset/Cargo.toml" ]]; then + echo "cgo-env.sh: gusset is not checked out next to DevCouncil ($here/../../../gusset)" >&2 + exit 1 +fi +# Go splits CGO_LDFLAGS on spaces and gussetfn's #cgo -L uses ${SRCDIR}, which +# cgo refuses to expand to a path with spaces; fail here, by name. +if [[ "$here" == *" "* ]]; then + echo "cgo-env.sh: $here contains a space, which cgo cannot link from" >&2 + exit 1 +fi +if [[ "$build" -eq 1 ]]; then + # --target-dir pins the output where gussetfn's #cgo -L and the hash below + # look. Without it an inherited CARGO_TARGET_DIR (two lanes on one + # checkout) sent the fresh archive elsewhere and this script keyed Go on + # the stale one it then linked. + cargo build --release --locked --manifest-path "$here/Cargo.toml" --target-dir "$here/target" >&2 +fi +archive="$here/target/release/libgusset.a" +if [[ ! -f "$archive" ]]; then + echo "cgo-env.sh: $archive is missing" >&2 + exit 1 +fi +if command -v sha256sum >/dev/null 2>&1; then + sum="$(sha256sum "$archive" | cut -c1-32)" +else + sum="$(shasum -a 256 "$archive" | cut -c1-32)" +fi + +ldflags="-L$here/target/release" +# Replace, never stack: runtime/cgo builds with -Werror, and a second -D of +# the same macro from re-sourcing this in one shell is a redefinition error. +# Seed from `go env`, which reports the process env, a `go env -w` setting or +# Go's default -O2 -g, in that order; the process env alone missed -w. +current="$(go env CGO_CFLAGS 2>/dev/null || printf '%s' "${CGO_CFLAGS:--O2 -g}")" +base="$(printf '%s' "$current" | sed -E 's/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=[^ ]*//g; s/^ +//')" +cflags="${base:+$base }-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=$sum" +if [[ "$mode" == export ]]; then + printf 'export CGO_ENABLED=1\nexport CGO_LDFLAGS=%q\nexport CGO_CFLAGS=%q\n' "$ldflags" "$cflags" +else + printf 'CGO_ENABLED=1\nCGO_LDFLAGS=%s\nCGO_CFLAGS=%s\n' "$ldflags" "$cflags" +fi diff --git a/rust/gusset-engine/src/lib.rs b/rust/gusset-engine/src/lib.rs index b0e272f8..ad7fbf9a 100644 --- a/rust/gusset-engine/src/lib.rs +++ b/rust/gusset-engine/src/lib.rs @@ -4,7 +4,17 @@ //! not the package name). It is the only Rust staticlib a Go binary in this //! suite may link. A second archive defines `rust_eh_personality` twice. -use gusset_core::{set_engine_handler, JobContext}; +use gusset_core::{register_engine, set_engine_handler, Counting, JobContext}; + +/// Counts every Rust allocation in the host process. +/// +/// Gusset declares no global allocator, so without this `gusset.Stats()` +/// reads zero and `AdviseMemoryLimit` subtracts nothing: the Go side of every +/// binary that links this archive was budgeting as if Rust used no memory. +/// The umbrella is the one archive per binary (R14), so this is the one place +/// it can be declared. +#[global_allocator] +static ALLOCATOR: Counting = Counting::new(std::alloc::System); /// Largest pattern or name accepted across the boundary. /// @@ -12,6 +22,28 @@ use gusset_core::{set_engine_handler, JobContext}; /// never memcpy's a path on the cgo thread. pub const MAX_FIELD: usize = 1024; +/// Opcode for [`match_any_frame`]: one crossing for a whole pattern list. +/// +/// Opcode 0 is the global handler ([`match_frame`]). Gusset resolves a +/// registered opcode before the global handler, so this cannot shadow it. +pub const OPCODE_MATCH_ANY: u32 = 1; + +/// Largest pattern count in one match-any frame. +/// +/// The 4 KiB inline limit already bounds a frame; this bounds the loop over +/// zero-length patterns, which cost two bytes each. +pub const MAX_PATTERNS: usize = 1024; + +/// Opcode that panics on purpose, so the host can prove the panic firewall +/// (I2) against this archive rather than against Gusset's diagnostic engine. +/// +/// Reachable only through the call header, which Go sets; no payload byte +/// selects it. `gussetfn.SelfTest` sends it on a throwaway handle and requires +/// ErrPanic, then ErrPoisoned, then a clean match on the shared handle; +/// `gussetfn.Match` pins opcode 0 so a context carrying this one cannot +/// reach the shared handle. `Check` never sends it. +pub const OPCODE_SELF_TEST_PANIC: u32 = 0x7fff_0001; + /// Registers the dc-glob engine on Gusset's global handler. /// /// Call once, before the first submission. Registration is not a Gusset export; @@ -30,9 +62,70 @@ pub unsafe extern "C" fn devcouncil_gusset_init() -> i32 { match_frame(input) }, ); + register_engine( + OPCODE_MATCH_ANY, + |ctx: &JobContext, input: &[u8]| -> Result, String> { + ctx.check() + .map_err(|reason| format!("cancelled: {:?}", reason))?; + match_any_frame(input) + }, + ); + register_engine( + OPCODE_SELF_TEST_PANIC, + |_: &JobContext, _: &[u8]| -> Result, String> { + panic!("devcouncil gusset self-test panic"); + }, + ); 0 } +/// Reads one `u16le len | bytes` field at `at`, returning it and the next offset. +fn field<'a>(input: &'a [u8], at: usize, what: &str) -> Result<(&'a str, usize), String> { + let len_end = at + .checked_add(2) + .filter(|&end| end <= input.len()) + .ok_or_else(|| format!("truncated {what} length"))?; + let len = u16::from_le_bytes([input[at], input[at + 1]]) as usize; + if len > MAX_FIELD { + return Err(format!("{what} length {len} exceeds {MAX_FIELD} bytes")); + } + let end = len_end + .checked_add(len) + .filter(|&end| end <= input.len()) + .ok_or_else(|| format!("truncated {what}"))?; + let text = std::str::from_utf8(&input[len_end..end]) + .map_err(|err| format!("{what} is not valid UTF-8: {err}"))?; + Ok((text, end)) +} + +/// Decodes a match-any frame and returns a single byte, 1 when any pattern matches. +/// +/// Frame: `u16le name_len | name | u16le count | (u16le pattern_len | pattern){count}`, +/// nothing else. Every pattern is validated before any is matched, so a bad +/// field late in the list is an error rather than a result that depends on +/// whether an earlier pattern happened to match first. +pub fn match_any_frame(input: &[u8]) -> Result, String> { + let (name, mut at) = field(input, 0, "name")?; + if at + 2 > input.len() { + return Err("truncated pattern count".to_string()); + } + let count = u16::from_le_bytes([input[at], input[at + 1]]) as usize; + if count > MAX_PATTERNS { + return Err(format!("pattern count {count} exceeds {MAX_PATTERNS}")); + } + at += 2; + let mut patterns = Vec::with_capacity(count); + for _ in 0..count { + let (pattern, next) = field(input, at, "pattern")?; + patterns.push(pattern); + at = next; + } + if at != input.len() { + return Err("match-any frame length does not equal its headers".to_string()); + } + Ok(vec![u8::from(dc_glob::matches_any(&patterns, name))]) +} + /// Decodes a length-prefixed frame and returns a single byte, 1 for match. /// /// Frame: `u16le pattern_len | pattern | u16le name_len | name`, nothing else. @@ -80,6 +173,65 @@ mod tests { out } + fn any_frame(name: &[u8], patterns: &[&[u8]]) -> Vec { + let mut out = Vec::new(); + out.extend_from_slice(&(name.len() as u16).to_le_bytes()); + out.extend_from_slice(name); + out.extend_from_slice(&(patterns.len() as u16).to_le_bytes()); + for p in patterns { + out.extend_from_slice(&(p.len() as u16).to_le_bytes()); + out.extend_from_slice(p); + } + out + } + + #[test] + fn match_any_matches_one_of_the_list() { + let got = match_any_frame(&any_frame(b"src/foo.py", &[b"*.rs", b"*.py"])).unwrap(); + assert_eq!(got, vec![1]); + let got = match_any_frame(&any_frame(b"src/foo.js", &[b"*.rs", b"*.py"])).unwrap(); + assert_eq!(got, vec![0]); + } + + #[test] + fn match_any_of_nothing_is_no_match() { + assert_eq!(match_any_frame(&any_frame(b"a", &[])).unwrap(), vec![0]); + } + + #[test] + fn match_any_validates_every_pattern_before_matching() { + // The first pattern matches; the second is bad UTF-8. A short-circuit + // would answer 1 and hide the malformed field. + let err = match_any_frame(&any_frame(b"a.py", &[b"*.py", b"\xff"])).unwrap_err(); + assert!(err.contains("UTF-8"), "{err}"); + } + + #[test] + fn match_any_refuses_truncation_and_trailing_bytes() { + let framed = any_frame(b"a", &[b"a", b"b"]); + for cut in 0..framed.len() { + assert!(match_any_frame(&framed[..cut]).is_err(), "prefix of {cut} bytes"); + } + let mut long = framed.clone(); + long.push(0); + assert!(match_any_frame(&long).unwrap_err().contains("does not equal")); + } + + #[test] + fn match_any_refuses_a_count_past_the_cap() { + let mut framed = vec![1, 0, b'a']; + framed.extend_from_slice(&((MAX_PATTERNS + 1) as u16).to_le_bytes()); + assert!(match_any_frame(&framed).unwrap_err().contains("exceeds")); + } + + #[test] + fn no_prefix_of_a_match_frame_panics() { + let framed = frame(b"src/*.py", b"src/foo.py"); + for cut in 0..framed.len() { + assert!(match_frame(&framed[..cut]).is_err(), "prefix of {cut} bytes"); + } + } + #[test] fn star_crosses_a_separator() { let got = match_frame(&frame(b"*.py", b"src/foo.py")).unwrap(); @@ -113,7 +265,7 @@ mod tests { #[test] fn a_field_past_the_cap_is_refused_without_parsing_it() { let mut framed = vec![0x01, 0x04]; // 1025 - framed.extend(std::iter::repeat(b'a').take(4)); + framed.extend(std::iter::repeat_n(b'a', 4)); let err = match_frame(&framed).unwrap_err(); assert!(err.contains("exceeds"), "{err}"); } diff --git a/rust/verify.sh b/rust/verify.sh index ac1472ed..89acd086 100755 --- a/rust/verify.sh +++ b/rust/verify.sh @@ -69,15 +69,21 @@ else echo "Check out https://github.com/bharathvbcr/gusset there. CI does this in .github/actions/setup-gusset." >&2 exit 1 fi - if [[ ! -f "$archive" ]]; then - echo "building gusset-engine release staticlib (libgusset.a is not in git)" - cargo build --release --locked --manifest-path gusset-engine/Cargo.toml - fi - if [[ ! -f "$archive" ]]; then - echo "GATE FAIL: $archive is missing after cargo build --release --locked" >&2 + # Always the incremental build, never "only when missing": that linked + # whatever archive an older checkout left behind. cgo-env.sh also keys Go's + # caches on the archive's hash, without which a rebuilt archive is not + # relinked and a cached test result is replayed against the old one. + echo "building gusset-engine release staticlib (libgusset.a is not in git)" + # Captured before eval: `eval "$(failing)" || …` evaluates the empty output, + # succeeds, and the build then links whatever archive is lying around. + engine_exports="$(gusset-engine/cgo-env.sh --export)" || { + echo "GATE FAIL: gusset-engine/cgo-env.sh could not build $archive" >&2 exit 1 - fi + } + eval "$engine_exports" go -C ../backend/go_orchestrator build -o bin/ ./cmd/devcouncil + go -C ../backend/go_orchestrator test -count=1 ./gussetfn + ../backend/go_orchestrator/bin/devcouncil gusset-check fi step "3/9 tests" diff --git a/scripts/check-release.mjs b/scripts/check-release.mjs index bc4f362a..da58ba2c 100644 --- a/scripts/check-release.mjs +++ b/scripts/check-release.mjs @@ -367,6 +367,11 @@ export function defaultSources(root) { packageLockPath: path.join(root, "package-lock.json"), cargoTomlPath: path.join(root, "rust", "Cargo.toml"), cargoLockPath: path.join(root, "rust", "Cargo.lock"), + // The Gusset umbrella is its own workspace with its own lock, and dc-glob + // inherits the product version, so a version bump that skips this lock + // breaks every `cargo build --locked` of the umbrella (setup-gusset, + // verify.sh, cgo-env.sh) — which is how 0.2.3 sat in it at 1.3.5. + umbrellaLockPath: path.join(root, "rust", "gusset-engine", "Cargo.lock"), // `devcouncil/version`, not `cmd/devcouncil`: the constant moved into a // library so `devcouncil/mcp` could import it, and this gate must read // the owner rather than the alias that `package main` keeps for printing. @@ -399,6 +404,12 @@ export function inspectRelease(sources, opts = {}) { const cargoToml = read(sources.cargoTomlPath, "rust/Cargo.toml"); const cargoLock = read(sources.cargoLockPath, "rust/Cargo.lock"); const goSrc = read(sources.goVersionPath, "version.go"); + // Optional so fixtures without an umbrella still inspect; the umbrella's + // own --locked build refuses a missing lock. + const umbrellaLock = + sources.umbrellaLockPath && existsSync(sources.umbrellaLockPath) + ? readFileSync(sources.umbrellaLockPath, "utf8") + : null; const versions = { "package.json": pkg ? parseNpmVersion(pkg) : null, @@ -411,6 +422,9 @@ export function inspectRelease(sources, opts = {}) { : null, "version.go": goSrc ? parseGoVersion(goSrc) : null, }; + if (umbrellaLock != null) { + versions["rust/gusset-engine/Cargo.lock dc-glob"] = parseCargoLockVersion(umbrellaLock, "dc-glob"); + } const present = Object.values(versions).filter((v) => typeof v === "string" && v !== ""); const identity = present[0] ?? null; diff --git a/scripts/check-release.test.mjs b/scripts/check-release.test.mjs index 6d3930c0..89297e5b 100644 --- a/scripts/check-release.test.mjs +++ b/scripts/check-release.test.mjs @@ -241,6 +241,18 @@ describe("inspectRelease", () => { assert.ok(result.errors.some((e) => e.includes("0.4.2") && e.includes("0.2.0"))); }); + it("fails when the Gusset umbrella's lock names another dc-glob version", () => { + const root = scratchTree("umbrella"); + const dir = path.join(root, "rust", "gusset-engine"); + mkdirSync(dir, { recursive: true }); + const lock = (v) => `version = 4\n\n[[package]]\nname = "dc-glob"\nversion = "${v}"\n`; + writeFileSync(path.join(dir, "Cargo.lock"), lock("0.2.0")); + assert.deepEqual(inspectRelease(defaultSources(root)).errors, []); + writeFileSync(path.join(dir, "Cargo.lock"), lock("0.1.9")); + const result = inspectRelease(defaultSources(root)); + assert.ok(result.errors.some((e) => e.includes("gusset-engine/Cargo.lock") && e.includes("0.1.9"))); + }); + it("fails when the notes file for this version is missing", () => { const root = scratchTree("notes", { notes: false }); const result = inspectRelease(defaultSources(root)); diff --git a/scripts/ci-local.mjs b/scripts/ci-local.mjs index 5c44b051..617bf4a3 100644 --- a/scripts/ci-local.mjs +++ b/scripts/ci-local.mjs @@ -5,6 +5,7 @@ * as a pass. */ import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; import { existsSync, readFileSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -57,12 +58,27 @@ function ensureGussetEngine() { "--locked", "--manifest-path", "rust/gusset-engine/Cargo.toml", + // Where gussetfn's #cgo -L and the hash below look, whatever + // CARGO_TARGET_DIR says. + "--target-dir", + "rust/gusset-engine/target", ]); const archive = path.join(REPO_ROOT, "rust", "gusset-engine", "target", "release", "libgusset.a"); if (!existsSync(archive)) { console.error(`FAIL: ${archive} is missing after cargo build`); process.exit(1); } + // Key Go's caches on the archive, as rust/gusset-engine/cgo-env.sh does: + // Go hashes cgo flags, not the libraries they link, so a rebuilt archive + // is otherwise not relinked and a cached "ok" is replayed against the old one. + const sum = createHash("sha256").update(readFileSync(archive)).digest("hex").slice(0, 32); + // `go env` reports the process env, a `go env -w` setting, or Go's default. + const goEnv = spawnSync("go", ["env", "CGO_CFLAGS"], { encoding: "utf8", shell: false }); + const current = goEnv.status === 0 ? goEnv.stdout.trim() : (process.env.CGO_CFLAGS || "-O2 -g"); + const base = current + .replace(/(^| )-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=\S*/g, "") + .trim(); + process.env.CGO_CFLAGS = `${base ? `${base} ` : ""}-DDEVCOUNCIL_GUSSET_ENGINE_SHA256=${sum}`; } function gofmtClean() {