diff --git a/scanner/cargofallback.go b/scanner/cargofallback.go index b20a939..0888ffa 100644 --- a/scanner/cargofallback.go +++ b/scanner/cargofallback.go @@ -34,6 +34,7 @@ func buildFileGraphFromOutcomeWithCargoMetadataAndFilters(ctx context.Context, r return nil, err } applyPrecomputedFileEdges(fg, outcome.precomputedEdges) + fg.sortEdges() return fg, nil } diff --git a/scanner/deterministic_edges_test.go b/scanner/deterministic_edges_test.go new file mode 100644 index 0000000..c3ee3a8 --- /dev/null +++ b/scanner/deterministic_edges_test.go @@ -0,0 +1,53 @@ +package scanner + +import ( + "context" + "reflect" + "sort" + "testing" +) + +// Edges are appended in analysis order, which the scanner does not fix, so the +// same repository scanned twice produced the same edges in a different +// sequence. That made --importers output shift between identical runs and made +// an exact-list assertion impossible for any caller. +func TestFileGraphEdgeOrderIsDeterministic(t *testing.T) { + const runs = 8 + var first *FileGraph + for run := 0; run < runs; run++ { + graph, err := BuildFileGraph(context.Background(), "../testdata/deterministic-edges", Filters{}) + if err != nil { + t.Fatalf("run %d: build graph: %v", run, err) + } + if first == nil { + first = graph + continue + } + if !reflect.DeepEqual(graph.Importers, first.Importers) { + t.Fatalf("run %d importers = %v, want the same order as run 0 %v", run, graph.Importers, first.Importers) + } + if !reflect.DeepEqual(graph.Imports, first.Imports) { + t.Fatalf("run %d imports = %v, want the same order as run 0 %v", run, graph.Imports, first.Imports) + } + // Imports keep resolution order, so they must stay stable without + // being sorted — the CUE resolver's selected-package-first ordering + // depends on it. + } + + // Sorted, not merely stable: a caller asserting an exact list needs to + // know which order it will get. + got := first.Importers["lib/shared.ts"] + want := append([]string(nil), got...) + sort.Strings(want) + if !reflect.DeepEqual(got, want) { + t.Fatalf("importers = %v, want them sorted %v", got, want) + } + if len(got) != 4 { + t.Fatalf("importers = %v, want all four importers of lib/shared.ts", got) + } +} + +func TestSortEdgesHandlesNilGraph(t *testing.T) { + var graph *FileGraph + graph.sortEdges() // must not panic +} diff --git a/scanner/filegraph.go b/scanner/filegraph.go index bd6c4f5..90f611a 100644 --- a/scanner/filegraph.go +++ b/scanner/filegraph.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "path/filepath" + "sort" "strings" "codemap/analysis" @@ -248,9 +249,30 @@ func buildFileGraphFromAnalysesWithCargoMetadataAndFilters(ctx context.Context, if err := ctx.Err(); err != nil { return nil, err } + fg.sortEdges() return fg, nil } +// sortEdges orders the reverse edge lists. Importers are appended while +// iterating analyses, whose order the scanner does not fix, so the same +// repository scanned twice produced the same importers in a different +// sequence: --importers output shifted between identical runs, diffs of +// codemap output showed changes that were not changes, and no caller could +// assert an exact list. +// +// Imports are deliberately left alone. They are appended per file in +// resolution order, which is already stable and which callers rely on: the +// CUE resolver returns a selected package before the package it falls back +// to, and DepsProject sorts its own copy for JSON output anyway. +func (fg *FileGraph) sortEdges() { + if fg == nil { + return + } + for file := range fg.Importers { + sort.Strings(fg.Importers[file]) + } +} + func applyPrecomputedFileEdges(fg *FileGraph, edges []fileEdge) { for _, edge := range edges { if edge.from == "" || edge.to == "" || edge.from == edge.to { @@ -713,7 +735,10 @@ func (fg *FileGraph) IsHub(path string) bool { return CountHubImporters(fg.Importers[path]) >= HubThreshold } -// HubFiles returns all files that qualify as hubs under IsHub. +// HubFiles returns all files that qualify as hubs under IsHub, ordered by +// non-test importer count descending and then by path. Map iteration order is +// random, so callers that truncate or display the head of this list would +// otherwise show a different set of hubs on every run over the same graph. func (fg *FileGraph) HubFiles() []string { var hubs []string for path := range fg.Importers { @@ -721,6 +746,16 @@ func (fg *FileGraph) HubFiles() []string { hubs = append(hubs, path) } } + counts := make(map[string]int, len(hubs)) + for _, path := range hubs { + counts[path] = CountHubImporters(fg.Importers[path]) + } + sort.Slice(hubs, func(i, j int) bool { + if counts[hubs[i]] != counts[hubs[j]] { + return counts[hubs[i]] > counts[hubs[j]] + } + return hubs[i] < hubs[j] + }) return hubs } diff --git a/scanner/filegraph_test.go b/scanner/filegraph_test.go index 2690f10..9f31c13 100644 --- a/scanner/filegraph_test.go +++ b/scanner/filegraph_test.go @@ -792,6 +792,31 @@ func TestDetectModule(t *testing.T) { } } +func TestHubFilesOrderIsDeterministic(t *testing.T) { + fg := &FileGraph{ + Importers: map[string][]string{ + "core.go": {"a.go", "b.go", "c.go", "d.go", "e.go"}, + "util.go": {"a.go", "b.go", "c.go", "d.go"}, + "alpha.go": {"a.go", "b.go", "c.go"}, + "beta.go": {"a.go", "b.go", "c.go"}, + "gamma.go": {"a.go", "b.go", "c.go"}, + "delta.go": {"a.go", "b.go", "c.go"}, + "lonely.go": {"a.go"}, + // Test importers never count toward hub status, so this file must + // not appear no matter how many test files import it. + "testonly.go": {"a_test.go", "b_test.go", "c_test.go", "d_test.go"}, + }, + } + + want := []string{"core.go", "util.go", "alpha.go", "beta.go", "delta.go", "gamma.go"} + for i := 0; i < 12; i++ { + got := fg.HubFiles() + if !reflect.DeepEqual(got, want) { + t.Fatalf("HubFiles() call %d = %v, want %v", i+1, got, want) + } + } +} + func TestFileGraphHubAndConnectedFiles(t *testing.T) { fg := &FileGraph{ Imports: map[string][]string{ diff --git a/scanner/rustcargo_test.go b/scanner/rustcargo_test.go index 8e6368a..603684f 100644 --- a/scanner/rustcargo_test.go +++ b/scanner/rustcargo_test.go @@ -118,6 +118,49 @@ func TestCargoMetadataSharesOneScanDeadline(t *testing.T) { } } +func TestCargoMetadataDeadlinePreservesFallbackTopology(t *testing.T) { + root := t.TempDir() + writeRustCargoFixture(t, root, map[string]string{ + "Cargo.toml": "[workspace]\nmembers = [\"one\", \"two\"]\n", + "one/Cargo.toml": cargoTestManifest("one"), + "one/src/lib.rs": "mod local;\n", + "one/src/local.rs": "", + "two/Cargo.toml": cargoTestManifest("two"), + "two/src/lib.rs": "", + }) + analyses := []FileAnalysis{ + {Path: "one/src/lib.rs", Language: "rust", Imports: []string{"local"}}, + {Path: "one/src/local.rs", Language: "rust"}, + {Path: "two/src/lib.rs", Language: "rust"}, + } + files := []FileInfo{ + {Path: "one/src/lib.rs"}, + {Path: "one/src/local.rs"}, + {Path: "two/src/lib.rs"}, + } + + index, outcome, err := buildRustWorkspaceIndexWithTimeout( + context.Background(), root, analyses, files, + func(ctx context.Context, _ string) ([]byte, error) { + <-ctx.Done() + return nil, ctx.Err() + }, + time.Millisecond, + ) + if err != nil { + t.Fatalf("buildRustWorkspaceIndexWithTimeout() error: %v", err) + } + if outcome == nil || outcome.Status != ScanSourceFallback { + t.Fatalf("metadata outcome = %#v, want fallback", outcome) + } + if pkg, ok := index.packageForFile("one/src/local.rs"); !ok || pkg.root != "one" { + t.Fatalf("fallback package = %#v, ok %v, want one", pkg, ok) + } + if pkg, ok := index.packageForFile("two/src/lib.rs"); !ok || pkg.root != "two" { + t.Fatalf("fallback package = %#v, ok %v, want two", pkg, ok) + } +} + func TestCargoMetadataRecoversLocalCargoTopology(t *testing.T) { tests := []struct { name string diff --git a/testdata/deterministic-edges/app/alpha.ts b/testdata/deterministic-edges/app/alpha.ts new file mode 100644 index 0000000..21a11ac --- /dev/null +++ b/testdata/deterministic-edges/app/alpha.ts @@ -0,0 +1,3 @@ +import { shared } from "../lib/shared"; + +export function alpha(): string { return shared(); } diff --git a/testdata/deterministic-edges/app/bravo.ts b/testdata/deterministic-edges/app/bravo.ts new file mode 100644 index 0000000..8cded45 --- /dev/null +++ b/testdata/deterministic-edges/app/bravo.ts @@ -0,0 +1,3 @@ +import { shared } from "../lib/shared"; + +export function bravo(): string { return shared(); } diff --git a/testdata/deterministic-edges/app/charlie.ts b/testdata/deterministic-edges/app/charlie.ts new file mode 100644 index 0000000..c384837 --- /dev/null +++ b/testdata/deterministic-edges/app/charlie.ts @@ -0,0 +1,3 @@ +import { shared } from "../lib/shared"; + +export function charlie(): string { return shared(); } diff --git a/testdata/deterministic-edges/app/delta.ts b/testdata/deterministic-edges/app/delta.ts new file mode 100644 index 0000000..8afaca9 --- /dev/null +++ b/testdata/deterministic-edges/app/delta.ts @@ -0,0 +1,3 @@ +import { shared } from "../lib/shared"; + +export function delta(): string { return shared(); } diff --git a/testdata/deterministic-edges/lib/shared.ts b/testdata/deterministic-edges/lib/shared.ts new file mode 100644 index 0000000..69ed7cd --- /dev/null +++ b/testdata/deterministic-edges/lib/shared.ts @@ -0,0 +1 @@ +export function shared(): string { return "s"; } diff --git a/watch/graph_state.go b/watch/graph_state.go index bc0e00a..52ed57f 100644 --- a/watch/graph_state.go +++ b/watch/graph_state.go @@ -29,7 +29,7 @@ const ( graphLifecycleSkippedSize = GraphLifecycleSkippedSize graphLifecycleFailed = GraphLifecycleFailed - graphBuilderRevision = "filegraph-v1" + graphBuilderRevision = "filegraph-v2" graphCacheLegacy = "legacy" graphCacheRootMismatch = "root_mismatch" graphCacheFilterMismatch = "filter_mismatch" diff --git a/watch/graph_state_test.go b/watch/graph_state_test.go index 45ed698..b0200b4 100644 --- a/watch/graph_state_test.go +++ b/watch/graph_state_test.go @@ -2,6 +2,7 @@ package watch import ( "context" + "encoding/json" "errors" "os" "path/filepath" @@ -16,6 +17,47 @@ import ( "github.com/fsnotify/fsnotify" ) +// A state file written by a binary from before importers were sorted carries +// builder revision "filegraph-v1". Its edge lists are in map-iteration order, +// so reusing it would keep serving non-deterministic answers; the revision bump +// must force a rebuild. +func TestPreSortStateFileIsNotReused(t *testing.T) { + if graphBuilderRevision == "filegraph-v1" { + t.Fatalf("graphBuilderRevision is still %q; bump it so pre-sort caches are rebuilt", graphBuilderRevision) + } + + root := t.TempDir() + cfg := config.ProjectConfig{} + current := newGraphState(root, cfg, graphLifecycleAvailable, time.Unix(10, 0), []string{"dep.go", "main.go"}) + fresh := State{ + Graph: ¤t, + Imports: map[string][]string{"main.go": {"dep.go"}}, + Importers: map[string][]string{"dep.go": {"main.go"}}, + } + configuredCount := 2 + fresh.ConfiguredFileCount = &configuredCount + fresh.Coverage.Status = analysis.CoverageComplete + + if graph, reason := ValidateCachedGraph(&fresh, root, cfg); graph == nil || reason != "" { + t.Fatalf("current-revision cache = %#v, %q; want it reused", graph, reason) + } + + payload, err := json.Marshal(fresh) + if err != nil { + t.Fatal(err) + } + var onDisk State + if err := json.Unmarshal(payload, &onDisk); err != nil { + t.Fatal(err) + } + onDisk.Graph.BuilderRevision = "filegraph-v1" + + graph, reason := ValidateCachedGraph(&onDisk, root, cfg) + if graph != nil || reason != graphCacheRevisionMismatch { + t.Fatalf("filegraph-v1 state file = %#v, %q; want nil, %q", graph, reason, graphCacheRevisionMismatch) + } +} + func TestGraphProvenanceValidation(t *testing.T) { root := t.TempDir() cfg := config.ProjectConfig{Only: []string{"go", "rust"}, Exclude: []string{"vendor", "generated"}}