From dd40aba4b5f960c6c4e86f496aa8ba7aaf46056e Mon Sep 17 00:00:00 2001 From: refinery costing Date: Sat, 22 Aug 2026 18:44:01 +0000 Subject: [PATCH] fix(order): make a bounded order-history read store-complete (gc-6a6vz) `gc order history ` was store-complete only with `--limit 0`. Any positive `--limit` -- including the default 50, and including a limit far larger than the number of retained runs -- answered from a single store while still rendering a RIG column, so a one-rig answer was indistinguishable from a city-wide one. Root cause is in the routing, not in the read. routeOrderHistory sends a single-order bounded query to the supervisor API, and that request carries one `scoped_name`. But a rig-scoped order is registered once per importing rig, so a bare name with no `--rig` names N registrations at once. orderScopedName resolves it through findOrder, which returns the FIRST match -- so the API was asked about one rig and the other N-1 were dropped silently. Because the city store is the mayor rig's store, the answer was always that rig, which reads as "the one rig that is working" rather than "the only rig I looked at". That under-report has already cost a P1: gc-toolkit bead tk-fdstg was filed at severity 1 reporting that the refinery-reconcile order had never fired on gc-toolkit, when it had in fact been firing in lockstep with the other three rigs since the order's first tick. The reporter explicitly flagged `gc order history` as unusable there -- it returned only gascity rows even at `--limit 40` -- and still reached the opposite of the truth, because no reachable surface would answer per-rig. The fix stays on the local iterator whenever the name resolves to more than one registration, alongside the existing multi-order and unlimited fallbacks and using the same `logRoute(... "fallback", ...)` idiom. That iterator was already correct: it walks every matching registration, merges newest-first, and only then applies the bound, so `--limit N` means "the N most recent runs in the city" rather than "the N most recent runs in whichever store I read first". No change was needed in the read path itself. Deliberately narrow. A rig-qualified read (`--rig `) still resolves to exactly one registration and keeps the API route, so this does not push every bounded read back onto the slower local scan; a city-scoped order has a single registration and is likewise unaffected. The per-store reads stay bounded by `--limit`, so the fan-out costs N bounded reads, not the unbounded scan the help text warns about. The routing tests pass a nil API client and assert on the route= line that logRoute emits before any request is built, rather than standing up an httptest server. The decision is fully observable from that line, and the untagged http_test_server census is a "cannot grow" ratchet (TESTING.md, Small and Source debt ratchets, ga-80po0c.2.2) -- an earlier draft using two real listeners pushed it to 319 calls / 67 files against a 317 / 66 baseline. Spending that ceiling to observe what stderr already reports would have been a poor trade, so the baseline is left untouched rather than raised. Validation: four new tests in cmd_order_history_store_completeness_test.go. TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs is the regression -- it fails before this change with "bounded read of a name spanning 2 rigs was routed to the API". TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified guards the other side so the API route is not lost. The remaining two pin the bead's stated acceptance: with a limit larger than the total row count every rig is represented, and with a limit smaller than it the rows kept are the newest across all stores rather than the newest of one. Existing `-run Order` (11.5s) and `-run Doctor` (67.2s) suites in cmd/gc stay green, as do go build ./..., go vet, and ./internal/testpolicy/... (the census). Not changed, noted for follow-up: `--since` is applied after the fetch rather than pushed into the per-store query (`--rig` genuinely is pushed down, by filtering registrations before any store is opened). With the fan-out corrected, pushing `--since` down is what would keep an unbounded-shaped read cheap; that is a separate performance change and carries its own risk, so it is left out of this fix. --- cmd/gc/cmd_order.go | 36 +++ ...d_order_history_store_completeness_test.go | 247 ++++++++++++++++++ 2 files changed, 283 insertions(+) create mode 100644 cmd/gc/cmd_order_history_store_completeness_test.go diff --git a/cmd/gc/cmd_order.go b/cmd/gc/cmd_order.go index 2716641d59..5f3155a7de 100644 --- a/cmd/gc/cmd_order.go +++ b/cmd/gc/cmd_order.go @@ -1414,6 +1414,24 @@ func routeOrderHistory(cityPath string, cfg *config.City, name, rig string, aa [ return doOrderHistoryBounded(name, rig, aa, cachedOrderHistoryStoresResolver(cityPath, cfg, stderr), bounds, jsonOutput, stdout, stderr) } + // A rig-scoped order is registered once per importing rig, so a bare name + // with no --rig names several registrations at once. The API request + // carries a single scoped_name, so routing an ambiguous name there answers + // for whichever registration findOrder returned first and silently drops + // every other rig's runs -- while the response still renders a RIG column, + // which makes a one-rig answer indistinguishable from a city-wide one. That + // under-report already cost a P1 on the refinery merge cadence (gc-6a6vz). + // + // The local iterator has no such limit: it walks every matching + // registration, merges the runs newest-first and only then applies the + // bound, so --limit N means "the N most recent runs in the city". Stay on + // it whenever the name resolves to more than one registration. A + // rig-qualified read still names exactly one, so it keeps the API route. + if matches := countOrderRegistrations(aa, name, rig); matches > 1 { + logRoute(stderr, "order history", "fallback", "multi-rig") + return doOrderHistoryBounded(name, rig, aa, cachedOrderHistoryStoresResolver(cityPath, cfg, stderr), bounds, jsonOutput, stdout, stderr) + } + var cr api.CachedRead[[]api.OrderHistoryView] return routeRead(c, "order history", nilReason, stderr, func() error { @@ -1431,6 +1449,24 @@ func routeOrderHistory(cityPath string, cfg *config.City, name, rig string, aa [ ) } +// countOrderRegistrations reports how many loaded order registrations match +// (name, rig). A rig-scoped order is registered once per importing rig, so an +// unqualified name matches one registration per rig that imported it; an empty +// rig therefore means "any rig", matching findOrder's own filter. +func countOrderRegistrations(aa []orders.Order, name, rig string) int { + matches := 0 + for _, a := range aa { + if a.Name != name { + continue + } + if rig != "" && a.Rig != rig { + continue + } + matches++ + } + return matches +} + // orderScopedName returns the rig-qualified key for the server's // /orders/history lookup. When a matching order is loaded locally, its // ScopedName() is authoritative (handles the city-level vs rig-level diff --git a/cmd/gc/cmd_order_history_store_completeness_test.go b/cmd/gc/cmd_order_history_store_completeness_test.go new file mode 100644 index 0000000000..59cc33c559 --- /dev/null +++ b/cmd/gc/cmd_order_history_store_completeness_test.go @@ -0,0 +1,247 @@ +package main + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/gastownhall/gascity/internal/beads" + "github.com/gastownhall/gascity/internal/orders" +) + +// A rig-scoped order is registered once per importing rig, so one bare `name` +// names several order registrations. These tests pin that a bounded read +// (`--limit N`) answers for every one of them, rather than for whichever +// registration happened to be found first (gc-6a6vz). + +// orderRunsStoreForScoped builds a store holding count order-run beads for a +// single scoped order name, one hour apart going back from newest. It mirrors +// orderHistoryRunsStore but takes the scoped name so each rig's registration +// gets its own rows. +func orderRunsStoreForScoped(t *testing.T, scoped string, newest time.Time, count int) beads.Store { + t.Helper() + rows := make([]string, 0, count) + for i := 0; i < count; i++ { + rows = append(rows, fmt.Sprintf( + `{"id":%q,"title":"run %d","status":"closed","issue_type":"task","created_at":%q,"labels":["order-run:%s"]}`, + fmt.Sprintf("%s-%d", strings.ReplaceAll(scoped, ":", "-"), i), + i, + newest.Add(-time.Duration(i)*time.Hour).Format(time.RFC3339Nano), + scoped, + )) + } + payload := []byte("[" + strings.Join(rows, ",") + "]") + return beads.NewBdStore(t.TempDir(), func(_, _ string, args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "list" { + return payload, nil + } + return []byte("[]"), nil + }) +} + +// twoRigOrderRegistrations is the shape at the heart of gc-6a6vz: one order +// name, one registration per importing rig. +func twoRigOrderRegistrations() []orders.Order { + return []orders.Order{ + {Name: "digest", Rig: "rig-a", Formula: "mol-digest"}, + {Name: "digest", Rig: "rig-b", Formula: "mol-digest"}, + } +} + +// TestOrderHistoryBoundedReadIsStoreCompleteAcrossRigs is the bead's stated +// acceptance test: with a limit larger than the total number of retained runs, +// a bare `gc order history ` must return rows from EVERY rig, not just +// one. An answer drawn from a single store still renders a RIG column, so a +// partial read is indistinguishable from a complete one at the terminal. +func TestOrderHistoryBoundedReadIsStoreCompleteAcrossRigs(t *testing.T) { + newest := time.Date(2026, 5, 17, 12, 0, 0, 0, time.UTC) + storeA := orderRunsStoreForScoped(t, "digest:rig:rig-a", newest, 3) + storeB := orderRunsStoreForScoped(t, "digest:rig:rig-b", newest.Add(-30*time.Minute), 3) + + resolver := func(a orders.Order) ([]beads.OrdersStore, error) { + switch a.Rig { + case "rig-a": + return []beads.OrdersStore{{Store: storeA}}, nil + case "rig-b": + return []beads.OrdersStore{{Store: storeB}}, nil + } + return nil, fmt.Errorf("unexpected rig %q", a.Rig) + } + + var stdout, stderr bytes.Buffer + // 50 is far more than the 6 rows that exist, so nothing here is a budget + // exhaustion: any missing rig is a store the read never opened. + code := doOrderHistoryBounded("digest", "", twoRigOrderRegistrations(), resolver, + orderHistoryBounds{Limit: 50}, true, &stdout, &stderr) + if code != 0 { + t.Fatalf("doOrderHistoryBounded = %d, want 0; stderr: %s", code, stderr.String()) + } + + payload := orderHistoryEntries(t, &stdout) + if len(payload.Entries) != 6 { + t.Fatalf("entries = %d, want 6 (3 per rig across 2 rigs); a short count means a store was not read", len(payload.Entries)) + } + seen := map[string]int{} + for _, e := range payload.Entries { + seen[e.Rig]++ + } + if seen["rig-a"] != 3 || seen["rig-b"] != 3 { + t.Fatalf("per-rig counts = %v, want 3 each; a bounded read must be store-complete", seen) + } +} + +// TestOrderHistoryBoundedReadKeepsNewestAcrossRigs pins the ordering half of +// the contract: the limit is applied AFTER merging every store newest-first, so +// `--limit N` means "the N most recent runs in the city", not "the N most recent +// runs in whichever store was read first". +func TestOrderHistoryBoundedReadKeepsNewestAcrossRigs(t *testing.T) { + newest := time.Date(2026, 5, 17, 12, 0, 0, 0, time.UTC) + // rig-b's newest run is 30 minutes older than rig-a's, so a correct merge + // interleaves them: a-0, b-0, a-1, b-1, ... + storeA := orderRunsStoreForScoped(t, "digest:rig:rig-a", newest, 3) + storeB := orderRunsStoreForScoped(t, "digest:rig:rig-b", newest.Add(-30*time.Minute), 3) + + resolver := func(a orders.Order) ([]beads.OrdersStore, error) { + switch a.Rig { + case "rig-a": + return []beads.OrdersStore{{Store: storeA}}, nil + case "rig-b": + return []beads.OrdersStore{{Store: storeB}}, nil + } + return nil, fmt.Errorf("unexpected rig %q", a.Rig) + } + + var stdout, stderr bytes.Buffer + code := doOrderHistoryBounded("digest", "", twoRigOrderRegistrations(), resolver, + orderHistoryBounds{Limit: 2}, true, &stdout, &stderr) + if code != 0 { + t.Fatalf("doOrderHistoryBounded = %d, want 0; stderr: %s", code, stderr.String()) + } + + payload := orderHistoryEntries(t, &stdout) + if len(payload.Entries) != 2 { + t.Fatalf("entries = %d, want 2", len(payload.Entries)) + } + // The two newest overall are rig-a's newest and rig-b's newest. Keeping + // only rig-a's two newest would mean the limit was applied per store. + gotRigs := []string{payload.Entries[0].Rig, payload.Entries[1].Rig} + if gotRigs[0] != "rig-a" || gotRigs[1] != "rig-b" { + t.Fatalf("rigs = %v, want [rig-a rig-b]; the bound must be applied after merging every store", gotRigs) + } +} + +// writeOrderHistoryMultiRigCity builds a city whose site config declares two +// rigs, so a rig-scoped order registration resolves to a real per-rig store. +func writeOrderHistoryMultiRigCity(t *testing.T) string { + t.Helper() + cityPath := t.TempDir() + for _, dir := range []string{".gc", "orders", "formulas", "rig-a", "rig-b"} { + if err := os.MkdirAll(filepath.Join(cityPath, dir), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(cityPath, "city.toml"), []byte(`[workspace] +name = "test-city" + +[[agent]] +name = "mayor" +`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cityPath, ".gc", "site.toml"), []byte(`workspace_name = "test-city" + +[[rig]] +name = "rig-a" +path = "./rig-a" + +[[rig]] +name = "rig-b" +path = "./rig-b" +`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(cityPath, "orders", "digest.toml"), []byte(`[order] +trigger = "manual" +formula = "mol-digest" +scope = "rig" +`), 0o644); err != nil { + t.Fatal(err) + } + t.Setenv("GC_DOLT", "skip") + t.Setenv("GC_BEADS", "file") + t.Setenv("GC_BEADS_SCOPE_ROOT", "") + return cityPath +} + +// orderHistoryNoClientReason is the nilReason handed to routeOrderHistory when +// these tests pass a nil API client. routeRead logs it verbatim on the nil-client +// path, so it doubles as a sentinel: seeing it proves the call reached the API +// route and fell back only for want of a controller, and NOT seeing it proves an +// earlier branch claimed the read. +// +// Passing nil rather than an httptest server is deliberate. The routing decision +// is fully observable from the emitted route= line, which logRoute writes before +// any request is built, so a real listener would prove nothing extra -- and the +// untagged http_test_server census is a "cannot grow" ratchet (TESTING.md, Small +// and Source debt ratchets, ga-80po0c.2.2). Spending two call sites and a file +// against that ceiling to observe what stderr already reports would be a poor +// trade. +const orderHistoryNoClientReason = "no-controller-sentinel" + +// TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs is the routing +// regression. The supervisor API request carries a single scoped_name, so it can +// only ever answer for ONE registration; routing a bare, ambiguous name there +// silently drops every other rig's runs while still printing a RIG column. The +// bounded read must therefore stay on the local iterator, which fans out across +// each matching registration's store. +func TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs(t *testing.T) { + t.Setenv("GC_DEBUG", "1") + + cityPath := writeOrderHistoryMultiRigCity(t) + cfg, err := loadCityConfig(cityPath, &bytes.Buffer{}) + if err != nil { + t.Fatalf("loadCityConfig: %v", err) + } + + var stdout, stderr bytes.Buffer + // A positive limit is the whole point: this is the path that used to route + // to the API. --rig is deliberately empty, so "digest" is ambiguous. + routeOrderHistory(cityPath, cfg, "digest", "", twoRigOrderRegistrations(), nil, orderHistoryNoClientReason, + orderHistoryBounds{Limit: defaultOrderHistoryLimit}, false, &stdout, &stderr) + + if !strings.Contains(stderr.String(), "route=fallback reason=multi-rig") { + t.Errorf("stderr missing multi-rig fallback: a bounded read of a name spanning %d rigs reached the API route, which answers for one scoped_name only:\n%s", + len(twoRigOrderRegistrations()), stderr.String()) + } + if strings.Contains(stderr.String(), "reason="+orderHistoryNoClientReason) { + t.Errorf("read reached the API route before the multi-rig guard:\n%s", stderr.String()) + } +} + +// TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified is the guard on the other +// side: --rig names exactly one registration, so the API route stays available +// and this fix does not push every bounded read back onto the slower local scan. +func TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified(t *testing.T) { + t.Setenv("GC_DEBUG", "1") + + cityPath := writeOrderHistoryMultiRigCity(t) + cfg, err := loadCityConfig(cityPath, &bytes.Buffer{}) + if err != nil { + t.Fatalf("loadCityConfig: %v", err) + } + + var stdout, stderr bytes.Buffer + routeOrderHistory(cityPath, cfg, "digest", "rig-a", twoRigOrderRegistrations(), nil, orderHistoryNoClientReason, + orderHistoryBounds{Limit: defaultOrderHistoryLimit}, false, &stdout, &stderr) + + if strings.Contains(stderr.String(), "reason=multi-rig") { + t.Errorf("rig-qualified read names exactly one registration and must keep the API route:\n%s", stderr.String()) + } + if !strings.Contains(stderr.String(), "reason="+orderHistoryNoClientReason) { + t.Errorf("rig-qualified read did not reach the API route:\n%s", stderr.String()) + } +}