Skip to content

test(control): pin the golden scenario clock to one instant - #801

Merged
frahlg merged 1 commit into
masterfrom
harvest-golden-clock-seam
Aug 4, 2026
Merged

test(control): pin the golden scenario clock to one instant#801
frahlg merged 1 commit into
masterfrom
harvest-golden-clock-seam

Conversation

@frahlg

@frahlg frahlg commented Aug 4, 2026

Copy link
Copy Markdown
Member

Credit

This work is not mine. It was written and verified by the Codex effort that
paused mid-flight and documented itself in
#790 (comment).
It sat finished and uncommitted in a detached worktree at 31232f25. I read
every hunk, moved it onto current master, audited the seam, re-ran the
verification and added the numbers below. The design and the code are theirs;
the only thing I added is a doc comment on State.now(). Their worktree is
untouched.

What the seam is

control.State gains one unexported field:

// clock is nil in production. Tests may set it to keep a dispatch
// scenario on one instant even when the test runner is delayed.
clock func() time.Time

and State.now() returns it when set, time.Now() when nil. All 17 ambient
time reads in internal/control now go through it.

Why

The golden recorder anchored a slot directive on time.Now() at the moment the
directive was asked for, and ComputeDispatch then read time.Now() again
further down. Energy-path targets are Wh × 3600 / seconds-remaining, so
anything that happens between those two reads shortens the slot and moves the
target. The scenario was never one instant; it was two, with a race between
them.

Measured on master, injecting a pause between the slot-directive request and
dispatch, on seeded_planner/095_planner_arbitrage:

pause target moved
none −4768.4804 W
50 ms −4768.9371 W −0.4567 W
250 ms −4770.7275 W −2.2471 W
1 s −4777.4496 W −8.9692 W

The replay tolerance is 0.01 W. A 50 ms scheduler pause — nothing on a
contended CI box — already exceeds it by 45×. That is a golden gate that fails
for reasons that have nothing to do with dispatch.

With the seam, the same three pauses:

pause target moved
50 ms −4768.4803 W 0.0000 W
250 ms −4768.4803 W 0.0000 W
1 s −4768.4803 W 0.0000 W

Bit-identical. TestGoldenPlannerScenarioIgnoresSchedulingDelay ships that as a
standing assertion, with a deliberate 50 ms sleep inside the slot-directive
closure.

What it buys the corpus

Three fresh recordings on each side, 703 targets each, split by whether the
record carries a slot directive:

master this branch
275 slot-bearing targets 1.6e-5 W drift 0 — exact
428 non-slot targets 4.5e-13 W 9.1e-13 W

All clock drift lived in the energy path, and it is now gone entirely.

The residual is not clock, and this PR does not claim to fix it. The
~1e-12 W on non-slot targets is float summation order over a Go map in the
weighted distributor. I confirmed it is pre-existing by recording master eight
times: it shows up there too (4.5e-13 W on seeded_reactive/071_weighted). It
is ten orders of magnitude under the tolerance. Recording it here so the next
person does not go hunting for it.

So the honest headline is the corpus no longer depends on the wall clock, not
the corpus is bit-reproducible. The 0.01 W tolerance stays and stays
load-bearing, and both doc comments in the diff say exactly that.

Production behaviour is unchanged

Not "unchanged as far as the tests can tell" — unchanged by construction:

  • The field is unexported. Only package control can assign it, and the
    only assignment in the package is in golden_dump_test.go.
  • go/cmd/ftw/control_state.go:9 is the only non-test caller of
    control.NewState. It is in a different package and could not set the field
    if it tried.
  • With clock nil, State.now() is time.Now(). Every converted line reads
    the wall clock exactly where it read it before.
  • time.Since(x)state.now().Sub(x) is the documented identity, monotonic
    reading included.

Completeness of the conversion

I checked whether the diff missed anything rather than assuming it had not.
Master has 17 time.Now() / time.Since() calls in the package's non-test
files, all in dispatch.go; the diff converts all 17. Afterwards the only
wall-clock read left in internal/control is State.now()'s own fallback.

Outside the package, ComputeDispatch reaches telemetry only through
Store.Get, ReadingsByType, SumOnlineEVW and DriverHealth. None reads the
clock — Get is a map read and IsOnline() tests stored fields the watchdog
sets. Store.IsStale is the one clock-reading method and dispatch never calls
it. So there is no ambient read left in the call graph, and nothing to disclaim.

Interaction with #799

#799 is open and touches both of these test files, so it has right of way. I
did not rewrite anything under it:

  • git merge-tree against 63bb04e8 auto-merges, no conflicts.
  • I built the merged tree and ran it. go test ./internal/control -count=20
    passes with all 590 records, and test(control): record the slew limiter in the golden corpus #799's own coverage line is unchanged:
    slew coverage: 123 records bind (>50 W), 123 of them at a production-realistic rate (<= 3000 W/tick); largest effect 6345 W on slew/seeded_034_priority.

#799's 155 records were recorded under the old recorder and replay clean under
the seam, which is what the 0.01 W tolerance is for. Either PR can land first.

Verification

  • go test ./internal/control -count=1 — ok, 0.744s
  • go test ./internal/control -run TestGolden -count=100 — ok, 29.6s
  • go test ./internal/control -run TestGolden -race -count=10 — ok, 6.7s
  • make verify — clean (Python 3.12.13 venv via uv)
  • gofmt clean on all three files

Changeset

No changeset — no-changeset label. .changeset/README.md gates on "every
PR that ships behavior to users", and lists "internal test plumbing" under
skipping. This touches production code, but it ships no behaviour: the field is
unexported and nil in production, and the seam exists solely so a test can pin
an instant. It is not even a perf tweak — one nil check per ambient read on a
1 Hz loop. Same call as #790 and #799 made for the corpus itself.

Flagging it as a judgement call rather than a formality, since the file is
dispatch.go. If a reviewer would rather have the seam recorded in the
changelog, a patch changeset is fine by me.

🤖 Generated with Claude Code

The golden recorder anchored each slot directive on time.Now() at the
moment the directive was asked for, then let ComputeDispatch read
time.Now() again further down. Energy-path targets are Wh x 3600 /
seconds-remaining, so any pause between those two reads shortened the
slot and moved the target. On an idle machine that was 1.5e-5 W of
run-to-run drift. Under a 50 ms scheduler pause it is 0.46 W, which is
45x the corpus replay tolerance of 0.01 W.

State gains an unexported clock func() time.Time. State.now() returns
it when set and time.Now() when nil, and every one of the 17 ambient
time reads in the package now goes through it. Production never sets
the field and cannot: it is unexported, and go/cmd/ftw is the only
non-test caller of control.NewState. Production keeps reading time.Now()
on exactly the same lines it read it before.

The golden recorder captures one instant per scenario and uses it for
both the slot anchors and the dispatch clock, so the two can no longer
disagree. Across three fresh recordings the 275 slot-bearing targets are
now bit-identical; on master the same targets drift 1.6e-5 W. The
remaining ~1e-12 W on non-slot targets is float summation order over a
Go map in the weighted distributor, present on master too, and is why
the 0.01 W tolerance stays.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@frahlg frahlg added the no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping) label Aug 4, 2026
@frahlg
frahlg merged commit 8fe82e4 into master Aug 4, 2026
13 of 14 checks passed
@frahlg
frahlg deleted the harvest-golden-clock-seam branch August 4, 2026 15:33
frahlg added a commit that referenced this pull request Aug 4, 2026
The 155 slew records were recorded before #801 pinned the golden scenario
clock, so their energy-path targets still carried the wall-clock jitter that
seam removes. Re-recorded against 8fe82e4, where the slot directive and the
dispatch calculation read one instant.

Nothing about the family's behaviour moves. Comparing the two recordings
record by record: no clamp attribution, flag or driver set differs, and the
only numeric changes are 26 values in 9 records, all of them planner modes on
the energy path, the largest 4.5e-06 W — three orders of magnitude under the
0.01 W replay tolerance. The bug records the family exists for are unchanged:
bug_no_self_charge_idle_slot_leak_500 still commands +1500 W of charging on a
tick that forbids it, and carveout_export_surplus_gate_snaps_to_zero still
lands at 0 W on the same shape.

Run to run, the re-recorded family now varies by at most 6e-14 W, from the
order the recorder sums per-driver targets. That residual is the same in the
seven older families and is not what the tolerance is for.

Also correct the corpus header: it named one ftw_commit for the whole corpus,
which stopped being true when a family was recorded later than the rest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
frahlg added a commit that referenced this pull request Aug 4, 2026
* test(control): record the slew limiter in the golden corpus

The corpus that landed in #790 runs 391 of its 435 records at SlewRateW
10 kW or 100 kW. At those rates no realistic per-tick move reaches the
bound, so the slew limiter was switched off in all but name: only 16
records change at all when it is disabled, and the gate could not see
its interactions.

Adds slew_limiter, a 155-record family recorded at rates a site runs —
500 W (control.NewState's default), 3000 W (the config default) and the
250-2000 W band in between. It covers anchoring on measured output in
both directions, reversals across zero, the charge and discharge blocks
with the battery live on the wrong side, fuse relief out-ranking the
ramp, the post-slew re-clamp, and the snap-to-zero carve-out where it
applies and where it does not.

The bug_* records capture a known bug's behaviour as of this commit, not
desired behaviour: noSelfCharge pins the fleet total to 0 W and the
limiter then walks one step back toward the battery's live charging
power, so a passive_arbitrage idle slot with the meter at -2000 W and
the battery live at +2000 W commands +1500 W of charging on a tick that
forbids charging. The fix will move these records, which is the point of
recording them.

TestGoldenCorpusCoverage now measures, for every record, how far the
targets move when the limiter is opted out, and fails when fewer than 60
records bind by more than 50 W. A re-recording that drifts back to
unreachable slew rates fails CI instead of passing quietly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(control): say precisely which stage enlarges a target

forceFuseDischarge also enlarges a target, so "the last stage that can
invent power" was wrong about the pipeline. What is true of the slew
limiter alone is that it enlarges one without a safety reason to.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(control): re-record the slew family through the golden clock seam

The 155 slew records were recorded before #801 pinned the golden scenario
clock, so their energy-path targets still carried the wall-clock jitter that
seam removes. Re-recorded against 8fe82e4, where the slot directive and the
dispatch calculation read one instant.

Nothing about the family's behaviour moves. Comparing the two recordings
record by record: no clamp attribution, flag or driver set differs, and the
only numeric changes are 26 values in 9 records, all of them planner modes on
the energy path, the largest 4.5e-06 W — three orders of magnitude under the
0.01 W replay tolerance. The bug records the family exists for are unchanged:
bug_no_self_charge_idle_slot_leak_500 still commands +1500 W of charging on a
tick that forbids it, and carveout_export_surplus_gate_snaps_to_zero still
lands at 0 W on the same shape.

Run to run, the re-recorded family now varies by at most 6e-14 W, from the
order the recorder sums per-driver targets. That residual is the same in the
seven older families and is not what the tolerance is for.

Also correct the corpus header: it named one ftw_commit for the whole corpus,
which stopped being true when a family was recorded later than the rest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
frahlg added a commit that referenced this pull request Aug 4, 2026
Rebased onto master. Three resolutions are carried by this commit.

The control tick in go/cmd/ftw/main.go: master added per-tick telemetry
persistence to the blocked-dispatch branch while this branch added the
manual-hold release. Both belong there. The release runs first because
persistTelemetryTick snapshots ctrl, so the stored tick has to show the hold
already cleared rather than one the blocked tick never executed.

Two struct conflicts, both the same shape: master's ConfigApplier landed where
this branch adds BatteryIdentity, in the api.Deps type and in main.go's
composite literal. Both fields are kept.

The golden-clock seam in go/internal/control/dispatch.go: #801 made state.now()
the only wall-clock read in the package, and it rewrote the same line this
branch extends. The manual-hold read keeps master's state.now(), and the scoped
target-validity gate is appended after it. dispatch.go holds no ambient clock
read outside now() itself.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
frahlg added a commit that referenced this pull request Aug 4, 2026
Rebased onto master. Three resolutions are carried by this commit.

The control tick in go/cmd/ftw/main.go: master added per-tick telemetry
persistence to the blocked-dispatch branch while this branch added the
manual-hold release. Both belong there. The release runs first because
persistTelemetryTick snapshots ctrl, so the stored tick has to show the hold
already cleared rather than one the blocked tick never executed.

Two struct conflicts, both the same shape: master's ConfigApplier landed where
this branch adds BatteryIdentity, in the api.Deps type and in main.go's
composite literal. Both fields are kept.

The golden-clock seam in go/internal/control/dispatch.go: #801 made state.now()
the only wall-clock read in the package, and it rewrote the same line this
branch extends. The manual-hold read keeps master's state.now(), and the scoped
target-validity gate is appended after it. dispatch.go holds no ambient clock
read outside now() itself.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changeset PR intentionally exempt from the changeset requirement (dev tooling / non-shipping)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant