gc order history: a bounded read is not store-complete — any positive --limit answers for the city store alone while printing a RIG column, and it cost a P1 on the refinery merge cadence (gc-6a6vz) - #155
Merged
Conversation
`gc order history <name>` 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 <name>`) 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.
Author
|
Codex signoff (pre-open, comment-only — not an approval): VERDICT: COMMENT Scope checked: Full diff for cmd/gc/cmd_order.go and cmd/gc/cmd_order_history_store_completeness_test.go, plus surrounding order-history routing, bounded local iterator behavior, route logging, existing route-history tests, and anchor gc-6a6vz intent. Findings: None. Verification: go test ./cmd/gc -run 'OrderHistory' -count=1 -> PASS; go test ./cmd/gc -run 'Order' -count=1 -> PASS; go test ./internal/testpolicy/... -count=1 -> PASS; go vet ./cmd/gc ./internal/orders ./internal/api -> PASS; git diff --check origin/main...dd40aba -> PASS. Not checked: full make test-fast-parallel was not rerun in this review pass. |
johnzook
approved these changes
Aug 22, 2026
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…z) (#155) `gc order history <name>` 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 <name>`) 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. Co-authored-by: refinery costing <refinery@local>
zook-bot
added a commit
that referenced
this pull request
Aug 25, 2026
…z) (#155) `gc order history <name>` 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 <name>`) 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. Co-authored-by: refinery costing <refinery@local>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defect
gc order history <name>is store-complete only when the read is unbounded(
--limit 0). Any positive--limit— including the default 50, and includinga limit far larger than the number of rows that exist — returns runs from the
city store alone, while printing them under a
RIGcolumn. The outputtherefore looks city-wide and is not.
Measured (loomington, 2026-08-20, 43 retained
refinery-reconcileruns across 4 rigs)--limit 100truncating to one rig with only 43 rows in existence rules out anexhausted budget. The bounded and unbounded reads are different code paths, and
only the unbounded one fans out across the per-rig stores.
Because the city store is the mayor rig's store, the bounded read always answers
"gascity". To a reader that looks like the one rig that is working, not the
only rig I looked at.
Why it matters
This is the triage surface for
scope = "rig"orders, whose entire point is oneregistration per importing rig. The help text steers straight into the trap —
"On a city with a long order-run history an unbounded read costs tens of
seconds, so prefer keeping a bound when triaging" — so the documented triage
path is the one that under-reports.
It has already cost a P1. gc-toolkit bead tk-fdstg was filed at severity 1
reporting that the
refinery-reconcileorder (the refinery merge cadence, i.e.the merge queue's clock) "has NEVER fired on gc-toolkit" and that the rig was
running only on a retired
/tmpdaemon. It had in fact been firing in lockstepwith the other three rigs since the order's first tick — 19 passes, first one at
the same second as gascity's. The reporter checked
gc order list, ruled outcity.toml overrides, ruled out suspension, and explicitly flagged
gc order historyas "NOT a usable check here: it returned only gascity rowseven at --limit 40" — and still concluded the opposite of the truth, because no
reachable surface would answer per-rig. (
bd listdoes not enumerate thetracking beads at all: they are wisps.)
Patch site
gc order history— the history read path behindcmd/gc order history. Theunbounded branch already enumerates every rig store; the bounded branch does
not. Fix by making the bounded path fan out over the same store set, merging by
executeddescending before applying the limit, so--limit Nmeans "the Nmost recent runs in the city" rather than "the N most recent runs in whichever
store I read first".
Worth checking whether
--sinceand--rigare pushed into the per-store queryor applied after the fetch — with the fan-out fixed,
--sinceis what keeps anunbounded-shaped read cheap (measured ~3s here for
--since 20m --limit 0).Test
A city with ≥2 rigs, each having run the same rig-scoped order. Assert that
gc order history <name> --limit N(for N greater than the total row count)returns rows from every rig, and that its rows are the N most recent by
executedacross all stores.Downstream
gc-toolkit's
doctor/check-refinery-merge-cadencereads history with--since <window> --limit 0and its hermetic test asserts the--limit 0ispresent, so it stays correct either way; nothing there needs to change when this
lands.
docs/refinery-merge-cadence.mdin that pack documents the trap.Filed from gc-toolkit tk-fdstg; determination in that repo at
specs/tk-fdstg/merge-cadence-false-alarm.md.Implementation notes
Correction to "Patch site" — converse sitting 2026-08-22T14:2xZ (visit gc-qcvca). Defect RE-CONFIRMED live; the named fix location is wrong.
Still reproduces, today, on the running binary (loomington, 44 retained
refinery-reconcile runs, 11 per rig across 4 rigs):
--limit 100 returning 11 of 44 existing rows rules out an exhausted budget, as
the original report argued.
The mechanism is one level up from where this bead points. The bead says
"the bounded branch does not fan out ... fix by making the bounded path fan out
over the same store set". But doOrderHistoryBounded ALREADY fans out — it takes
an orderStoresResolver and is the same function the unbounded path calls.
Patching it changes nothing, because a bounded named read never reaches it.
The split is in routeOrderHistory (cmd/gc/cmd_order.go, ~1398-1430). GC_DEBUG=1
prints the decision:
Three branches fall back to the local multi-store iterator — name == ""
(multi-order) and bounds.Limit <= 0 (unlimited) — while a NAMED read with a
POSITIVE limit is routed to the server: c.GetOrderHistory(orderScopedName(...),
bounds.Limit, ""). That server read answers from the city store alone, and the
renderer still prints a RIG column. The comment above the unlimited branch
states the reason the fallback exists ("The API has no wire representation for
an unlimited read"), which is why only that branch was ever made store-complete.
So the fix is one of two places, neither of them doOrderHistoryBounded:
(a) make the API's /orders/history handler store-complete — merge across every
rig store by executed-desc BEFORE applying the row bound; or
(b) stop routing rig-scoped order reads to the API and always use the local
fan-out iterator, taking the latency cost the help text already warns about.
(a) keeps the API fast path; (b) is smaller and needs no server change.
Fork/upstream classification (the FIRST method, blob hash + per-file log):
upstream/main carries the IDENTICAL routeOrderHistory — same three branches,
same GetOrderHistory call at the same place. This is a GENUINE UPSTREAM DEFECT,
not fork-local debt. Fix in fork first, tag upstream_pr_candidate.
Rebase risk is real but small and unchanged: cmd/gc/cmd_order.go is
fork-touched (1 commit ours: 3d65376, refuse unbound city registration of a
rig-scoped order, gc-g3pgp) AND upstream-moving (1 commit theirs: b3f125f,
route session-/graph-/convergence-class reads to their owning store, gastownhall#5209).
Three distinct blobs at merge-base / origin/main / upstream/main. Flag this to
whoever takes it. Note b3f125f is the same CLASS of fix as this bead
(routing a read to its owning store) but does not touch order history.
Implemented 2026-08-22 (polecat gc-toolkit.nux, branch polecat/gc-6a6vz)
Commit dd40aba — fix(order): make a bounded order-history read store-complete.
ROOT CAUSE (differs from the "Patch site" guess in the filing, which pointed at
the read): the read was never the problem. doOrderHistoryBounded ALREADY fans
out across every matching registration, merges newest-first, and only then
applies the bound — exactly the semantics the bead asks for. The defect is one
level up, in ROUTING.
routeOrderHistory (cmd/gc/cmd_order.go) sends a single-order bounded query to
the supervisor API, and that request carries ONE
scoped_name. A rig-scopedorder is registered once per importing rig, so a bare name with no --rig names
N registrations. orderScopedName resolves it via findOrder, which returns the
FIRST match — so the API was asked about one rig and the other N-1 were dropped
silently, while the response still rendered a RIG column. Because the city store
is the mayor rig's store, that first match is always gascity, which is why the
symptom read as "only gascity ever runs".
This also explains the two facts in the filing that a read-side bug would not:
why --limit 100 truncated to one rig with only 43 rows in existence (not budget
exhaustion — a different store set), and why --rig gc-toolkit worked (it names
exactly one registration, so the single scoped_name is correct).
FIX: stay 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. +36 lines in cmd_order.go,
no deletions, no change to the read path. Deliberately narrow: a rig-qualified
read and a city-scoped order each resolve to exactly one registration and keep
the API route, so this does not push every bounded read onto the slower local
scan. Per-store reads stay bounded by --limit, so the fan-out is N bounded
reads, not the unbounded scan the help text warns about.
ACCEPTANCE MET — 4 new tests in cmd/gc/cmd_order_history_store_completeness_test.go,
verified by inverted-fix run (source reverted to base, tests kept):
TestRouteOrderHistoryBoundedStaysLocalWhenNameSpansRigs FAILS without fix
TestRouteOrderHistoryBoundedUsesAPIWhenRigQualified guard (passes both)
TestOrderHistoryBoundedReadIsStoreCompleteAcrossRigs bead's stated test:
limit 50 > 6 total rows -> 3 rows from EACH of 2 rigs
TestOrderHistoryBoundedReadKeepsNewestAcrossRigs limit 2 -> newest
across ALL stores (rig-a, rig-b), not the newest of the first store
The routing tests pass a nil API client and assert the route= line logRoute
emits before any request is built, rather than standing up httptest servers.
An earlier draft used two real listeners and pushed the untagged
http_test_server census to 319 calls / 67 files against a 317 / 66 baseline —
and that ratchet's invariant is "cannot grow" (TESTING.md, Small + Source debt
ratchets, ga-80po0c.2.2). The baseline is therefore left untouched, not raised;
the decision is fully observable from stderr either way.
GATES (this rig configures no test/lint/build commands; run by hand):
go build ./... PASS
go vet ./cmd/gc ./internal/orders ./internal/api PASS
go test ./cmd/gc -run Order PASS (11.5s)
go test ./cmd/gc -run Doctor PASS (67.2s)
go test ./internal/testpolicy/... PASS (census green, no ratchet)
make test-fast-parallel (full push gate) PASS — sentinel RC=0, 10/10 jobs
including unit-core. Run by hand because the shared rig repo sets
core.hooksPath=/dev/null, so no git hook fires for any agent here
(filed separately as gc-w89av, P1).
NOT CHANGED, noted for follow-up: the filing asks to check whether --since and
--rig are pushed into the per-store query. --rig IS pushed down (registrations
are filtered before any store is opened). --since is NOT — it is applied after
the fetch, in the row loop. With the fan-out corrected, pushing --since down is
what would keep an unbounded-shaped read cheap; that is a separate performance
change with its own risk, so it is deliberately out of scope here.
2026-08-22 REFINERY GATE (gascity/gc-toolkit.refinery, wisp gc-wisp-jl9).
make test-fast-parallel EXTRA_TEST_ENV='GC_TEST_INNER_P=2', TMPDIR=/var/tmp/rp-6a6vz
HEAD dd40aba, 19:25:44Z -> 19:38:54Z (13m10s), sentinel RC=0.
Header 'Running 10 fast job(s) with LOCAL_TEST_JOBS=2 inner_p=2'; 10/10 jobs ok
(unit-core + all six cmd/gc shards); runner printed "All fast jobs passed".
Run ran for real: fresh fixtures under the run's own TMPDIR (gc-home-,
gascity-subprocess, beads-circuit, TestCustomTypesCheck_).
Ran the gate explicitly because core.hooksPath=/dev/null in the shared rig
.git/config disables the pre-push hook (a push returns RC=0 in seconds with zero
job lines — the documented silent-no-op signature), so a green push here is not
evidence of a green gate.
Rebase onto origin/main was a no-op (branch already contained main tip 9e89da7);
push was a fast-forward no-op; remote confirmed at dd40aba.
Parked as a pre-open gating anchor (merge_result=pre_open_gate, check_set=codex).
Codex signoff dispatched as gc-oejd5 against origin/main...origin/polecat/gc-6a6vz.
pre-open-resolve.sh opens the PR once check.codex=green@dd40aba.
Refinery handoff
gc-6a6vz(bug, P1)polecat/gc-6a6vzmaindd40aba4; PR opened codex-green.