fix(#6014,#6015): real rebuild in-flight across the process boundary; dashboard stops serving a stale graph.json - #6036
Merged
Conversation
`grafel status` reported both `in_flight=` and `rebuild: in_flight=` as a structural zero in split mode (the default since ADR-0024). Three defects on one path, all of which had to go for the number to reach a human. 1. THE WORK MOVED PROCESSES. Service.Rebuild's rebuildInFlight / groupsActiveCount increments sit below the split-mode early return, so in the default mode they can never be non-zero: the rebuild runs in the ENGINE. Serve has no local information from which to answer "is a rebuild running", so the fix has to be cross-process -- there is no honest serve-local number here, and a fabricated plausible one would be no better than the wrong zero. The engine now maintains engineRebuildInFlight, incremented in lockstep with rebuildWorker.active (the per-group guard that is the real single-flight), and publishes it on the liveness sidecar it already heartbeats. RebuildInFlightFromStatusFile reads it back in serve. This is the same route #5954 established for the stage gate and #6002 for GroupAlgoInFlight, and it deliberately shares their freshness rule: a stale heartbeat yields 0, never a dead engine's last-known count reported as live. It is NOT sourced from indexstate (EngineInFlight): the engine's direct group-rebuild path bypasses the scheduler -- cmd/grafel/daemon.go already documents that it "never calls scheduler.Enqueue, so s.inflight stays empty" -- so indexstate.InFlight reads 0 for the entire multi-minute rebuild. 2. THE PRINT GATE MADE IT UNREADABLE ANYWAY. `grafel status` emitted its `rebuild:` line inside a block gated on StatusReply.RSSBudgetMB, which Service.Status populates only from an in-process scheduler. In split mode that is 0, so the block -- and the rebuild line with it -- never printed at all. The count crossed the process boundary correctly and was then discarded by the renderer: the same bug one layer up. RebuildConcurrencyCap is set unconditionally, so the line is now gated on that alone. To make the gate testable at all, the daemon detail block moved out of runStatus (which needs a live client.Dial) into printDaemonDetail. An unreachable print gate is exactly what this issue is about, so it gets a test rather than a comment. 3. THE RPC ITSELF WAS UNCOUNTED. s.inFlight -- serve's own count of executing RPCs, the `in_flight=` field -- was also incremented below the split-mode branch. Harmless for the fire-and-forget path (nothing is in flight once the request file lands) but a lie for WaitForCompletion, which blocks the handler in awaitRebuildCompletion for the entire rebuild. The increment moves to the top of the handler, where it matches the field's definition in every mode. Known latency, documented at RebuildInFlightFromStatusFile: the sidecar is written by a plain 5s ticker with no state-change coalescing, so a just-started rebuild still reads 0 for up to one heartbeat. Rebuilds are multi-minute, so this is acceptable -- but it is this same bug in miniature, and the note says to add a notify on rebuildWorker.submit rather than shorten the tick. Mutation-tested, 6 of 6 caught: dropping the split-branch read; dropping the heartbeat publication; dropping the worker increment; moving the s.inFlight increment back below the branch; re-nesting the `rebuild:` line under RSSBudgetMB; and dropping the reader's freshness guard -- that last one initially slipped through, because the Status RPC enters its split-mode branch via StageGateFromStatusFile's own freshness check, which shadowed it. TestRebuildInFlightFromStatusFile_FreshVsStale binds the accessor directly so the guard is real rather than decorative. Closes #6014
repoGraphBytes read graph.json UNCONDITIONALLY and only fell back to the FB
graph when the JSON was absent. Any repo that ever produced one -- via
`--export-json`, or by predating the ADR-0016 layout -- therefore pinned the
dashboard's RepoGraph / GroupGraph endpoints to that snapshot across every
subsequent reindex, with nothing on the surface saying it was stale. The
sibling sidecar readers (graph/descriptions, graph/flows) already stat-compare
before use; store.go was the odd one out.
The read is now gated on graphJSONUsable:
- Freshness is measured against graph.CurrentGraphDescriptor, not a
hardcoded graph.fb path, so a SEGMENT-SET repo is compared against its
manifest.json commit point rather than being mistaken for "no graph
exists" and pinned to the JSON -- the same defect one layout over, and the
layout #5915 is making the default.
- The comparison is `After`, never `!Before` -- for the opposite of the
obvious reason. An equal mtime is not filesystem luck, it is deliberately
engineered: cmd/grafel/index.go stamps both artifacts with one timestamp
on every export (#1626) so a drift check cannot fire on two encodings of
one pass. A tie therefore reads either as "co-written and current" or as
"an older export a later graph write landed on", and mtime alone cannot
separate them. Only the second is a correctness question, and the costs
are asymmetric, so ties resolve to regenerate.
- CONSEQUENCE, stated in the code because it is a real cost: that same
Chtimes makes the mtimes equal every time for a SINGLE-FILE repo that also
exports graph.json, so the raw-read fast path is effectively disabled
there and every request re-marshals. SEGMENT-SET repos are unaffected --
there the stamped path is the gen DIRECTORY while the gate reads
manifest.json, written earlier, so the JSON stays newer and served. The
asymmetry is real, is now documented accurately, and is pinned by a test
so the comment cannot quietly drift. It is accepted only because the blast
radius is narrow (`--export-json`, `grafel quality`, xrepo_verify -- never
the 427k corpus); removing it properly needs a generation identity stamped
INTO the JSON, which is a larger change than this fix should carry.
- A fresh graph.json is still served by raw read, byte-for-byte. The
verbatim assertions use an indented fixture so they cannot be satisfied by
an unmarshal/re-marshal round trip.
- A graph that exists but cannot be read no longer falls back to the JSON --
that fallback IS this bug. But format-version skew is transient-by-reindex,
not corruption, and a bare loader error behind an empty panel says nothing
about the one command that fixes it. graphUnreadableError surfaces
graph.ReindexRequiredReason' shared "reindex required" wording instead,
while any other load failure passes through unchanged.
- A legacy JSON-only repo (GraphAbsent) keeps the raw-read fast path.
Mutation-tested, 11 of 12 caught: removing the gate; `!Before` for the tie;
hardcoding graph.fb over the descriptor; always-regenerate; dropping the
GraphAbsent fast path; dating a segment set by its gen dir instead of its
manifest (twice over, via both the asymmetry and the helper test); serving the
JSON on a corrupt descriptor; serving the JSON on an FB load error; rendering
version skew as a bare error; and dressing every load failure up as
reindex-required. The survivor -- dropping the `ji.IsDir()` arm -- is benign
and now says so in the code: os.ReadFile on a directory fails and falls through
to the loader regardless.
Closes #6015
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two release blockers in the same class as #5991/#6013: the tool reported a state that was not true.
#6014 —
in_flight=0always in split modeThe issue proposed moving the counter to the branch split mode takes. There is no such branch. Two separate defects produced the symptom.
1. The work moved processes.
Service.rebuildInFlight/groupsActiveCountare incremented belowService.Rebuild's split-mode early return, and in split mode the rebuild runs in the engine. Serve cannot derive the number locally, so the fix has to be cross-process — anything else would be a fabricated plausible-looking number.The obvious shortcut was checked and rejected:
statusfile.EngineInFlightmirrorsindexstate.InFlight, whose only writer isindexstate.Set(len(s.inflight))in the scheduler — andcmd/grafel/daemon.go:1541-1543already documents that the group rebuild "indexes every repo DIRECTLY … it never callsscheduler.Enqueue, sos.inflightstays empty". It would have read 0 for the entire 10–12 minute rebuild while looking correct.The real engine-side truth is
rebuildWorker.active, the per-group single-flight guard.engineRebuildInFlightis incremented in lockstep with it, published by the existing liveness heartbeat, and read back viaRebuildInFlightFromStatusFile()— the same route #5954's stage gate and #6002'sGroupAlgoInFlightalready use, sharing their freshness rule (stale heartbeat ⇒ 0, never a dead engine's last-known count).2. The RPC itself was uncounted.
s.inFlight— the literalin_flight=field — was also below the split branch. Harmless for fire-and-forget, but a lie forWaitForCompletion, which blocks the handler for the entire rebuild. Moved to the top of the handler.The first fix was correct on the wire and dead on the screen
Review caught this on the live daemon: the count crossed the process boundary correctly and the renderer threw it away.
StatusReply.RebuildInFlighthas one consumer,internal/cli/status.go:205, nested insideif st.RSSBudgetMB > 0.reply.RSSBudgetMBis set atservice.go:399— insideif s.scheduler != nil. In split mode serve's scheduler is nil, so that whole block never prints.grafel statusagainst the running daemon showedin_flight=0and norebuild:line at all.That is the same defect class as the bug being fixed, one layer up — and the code asserted otherwise, claiming the count reached
grafel status.Hoisting the conditional alone would not have been testable: the block lives inside
runStatus, which requires a liveclient.Dial, so every conditional there was unreachable from a test. That is how a dead print gate survived. The detail block is now extracted intoprintDaemonDetail(following the existingprintAnnotationStatusprecedent) and the rebuild line hoisted out of theRSSBudgetMBgate there. Independently verified: re-nesting it failsTestPrintDaemonDetail_RebuildLinePrintsWithNoRSSBudget.Both false comments corrected.
#6015 — dashboard served a stale
graph.jsonforeverAs filed, plus a layout the issue does not mention: a gate hardcoded to
graph.fbfinds nothing on a segment-set repo, concludes "no graph", and serves the stale JSON anyway. Freshness is measured againstgraph.CurrentGraphDescriptor—manifest.jsonfor a segment set (its atomic commit point, written after every segment flush and before thecurrentpointer flip), the resolved.fbotherwise.On equal mtimes:
cmd/grafel/index.go:828-830doesnow := time.Now(); os.Chtimes(fbPath, now, now); os.Chtimes(outPath, now, now). An equal mtime is deliberately engineered, not filesystem-resolution luck, so!Beforewould have been actively wrong. Comparison isAfter; ties regenerate.Known cost, documented and pinned rather than described: because those mtimes are forced equal, the JSON fast path is now dead for single-file-gen repos that opted into
--export-json— every graph request re-marshals. Blast radius is narrow (FB-only is the default; this affects--export-json,grafel quality,xrepo_verify) and specifically not the 427k-entity corpus. Segment sets keep the fast path, becausewriteSegmentsreturns the gen dir while the gate reads the earlier-writtenmanifest.json. That asymmetry is pinned byTestRepoGraphBytes_SegmentSetKeepsJSONFastPathWhenManifestOlder, so the comment is a checked claim.The content-identity fix was considered and deferred:
descriptions.CurrentSourceKeycomputes a key for the current graph, but nothing stores a comparable key ongraph.json, so using it properly means stamping a generation identity into the JSON — a new sidecar contract, well outside this fix.Behaviour change, decided deliberately: an unreadable
.fbnow errors instead of falling back tograph.json. That is right for a silently-stale snapshot, but a bare error is wrong for format-version skew, which is transient-by-reindex.graphUnreadableErrorrunsgraph.ReindexRequiredReason(segment-set aware) so the panel names the fixing command. Two tests: version skew must say "reindex required"; any other load failure must not — dressing every failure as a reindex prompt would send users to reindex something a reindex cannot fix.Verification
#6014: 6/6 mutations killed (including the re-nested gate). #6015: 11/12. The original sweep was re-run after the refactor and all still die.
The sole survivor is dropping
ji.IsDir()— benign, sinceos.ReadFileon a directory fails and falls through anyway. It is kept with a comment saying explicitly that it is belt-and-braces and that no test binds it, so nobody later mistakes it for load-bearing.One earlier escape, disclosed and closed: the
RebuildInFlightFromStatusFilefreshness guard was shadowed byStageGateFromStatusFile()'s own check, so it could be deleted entirely with the RPC test green.TestRebuildInFlightFromStatusFile_FreshVsStalebinds the accessor directly.Also verified during review and requiring no change: no counter leak (the
Add(-1)defer covers panic, early return, backoff, retry and dead-letter, all insiderunGroup); genuine lockstep withw.active;manifest.jsonreally is the segment-set commit point, so the gate cannot report fresh mid-write.go build ./... && go vet ./... && gofmt -l .clean.internal/cli,internal/daemon,internal/dashboard,internal/statusfileall pass post-rebase with raw exit codes viartk proxy.Known and disclosed
RebuildInFlightFromStatusFile, noting the right tightening is a notify onrebuildWorker.submitrather than a shorter tick. Not done here — it touches the heartbeat's construction seam.StatusReply.GroupAlgoRunning(group names) is still empty in split mode; the sidecar carries counts only, as group-algo child: 29.5h elapsed for 116min CPU (~6.5% duty) — the pass is blocked, not slow #6002 left it. The same is now true of rebuilds.s.rebuilddirectly still reports via the serve-local atomics, so the two modes report through different mechanisms rather than fully converging.Independently adversarially reviewed.
Closes #6014
Closes #6015