Skip to content

fix(control): bound every driver command with a deadline - #791

Merged
frahlg merged 1 commit into
masterfrom
harvest-bound-driver-command-deadline
Aug 4, 2026
Merged

fix(control): bound every driver command with a deadline#791
frahlg merged 1 commit into
masterfrom
harvest-bound-driver-command-deadline

Conversation

@frahlg

@frahlg frahlg commented Aug 4, 2026

Copy link
Copy Markdown
Member

The hazard

The control tick sent every driver command with the process-lifetime
context built in main():

if err := reg.Send(ctx, t.Driver, payload); err != nil {   // main.go:2652

Registry.Send honours the context on both the channel push and the
result wait, and the doc comment on the sibling SendDefault names the
consequence outright: "an unblocked send into a wedged driver deadlocks
the entire control loop". But a cancel-only context never fires on its
own. The driver goroutine runs driver_command inline with the device
I/O, so a cloud driver waiting on an HTTP or OAuth request that never
answers held the tick for as long as its socket did — and the tick is
shared. Every other battery on the site waited behind it, including the
reactive fuse guard that protects the main breaker, and the site sat at
whatever output it last commanded.

The asymmetry reads as an oversight rather than a decision:
sendDriverDefault already bounds its call with
context.WithTimeout(ctx, driverDefaultTimeout). The bound landed on the
path that runs rarely and missed the one that runs every control
interval.

The fix

sendDriverCommand in a new go/cmd/ftw/driver_command_deadline.go,
mirroring sendDriverDefault's shape, used at both dispatch sends. The
delta in main.go is three small hunks: one line to derive the timeout
next to controlInterval, and one call in place of each inline
reg.Send.

Choosing the timeout

Derived from the operator's site.control_interval_s, not hardcoded — a
site on a 1 s tick and a site on 30 s should not share a number:

timeout = control_interval / 2, capped at driverDefaultTimeout (2 s),
                                floored at 250 ms
  • Half the interval — a wedged driver then costs its own command and
    still leaves the rest of the tick (the remaining drivers, the fuse
    guard, the state save) inside the interval. At the 2 s default that is
    1 s; at 1 s it is 500 ms.
  • Capped at driverDefaultTimeout — a dispatch command must never be
    given longer to give up than the safety default that has to reach the
    same driver afterwards.
  • Floored at 250 ms — a deadline shorter than a LAN Modbus write
    would fail every command and freeze the site at its last output, which
    is worse than the stall being fixed. The floor cannot bind at any
    currently valid config (integer seconds, minimum 1 s); it guards a
    future sub-second tick.

Health semantics: unchanged, deliberately

A timed-out command is logged at Warn with the driver name and the
timeout, and nothing else. I did not make it count as a driver failure,
for a physical reason: the driver goroutine serialises polls and
commands, so a driver wedged inside driver_command also stops polling
and stops emitting. Its telemetry goes stale, and the existing watchdog
already walks it to its autonomous default mode within the watchdog
window. Counting the timeout separately would double-book one fault, and
would let a single bad round trip on an otherwise healthy cloud driver
knock it out of control. If the Warn line turns out to fire on real
sites in a pattern the staleness path misses, a threshold can be added
then — with evidence.

Tests

Both new test files fail without the production change. With the
deadline stripped out of sendDriverCommand, three of them fail:

--- FAIL: TestSendDriverCommandStopsWaitingOnWedgedDriver (5.00s)
    sendDriverCommand never returned: a wedged driver is holding the control tick
--- FAIL: TestWedgedDriverDoesNotStarveLaterDispatchTargets (5.00s)
    dispatch never finished: the wedged driver blocked the drivers behind it
--- FAIL: TestSendDriverCommandLeavesHealthyDriverUntouched (0.00s)
    driver received a context with no deadline
  • go/cmd/ftw/driver_command_deadline_test.go — the derived timeout as a
    table (1 s / 2 s / 5 s / 30 s / 0), the wedged driver releasing its
    caller at the deadline and naming itself in the log, a wedged driver
    not starving the drivers dispatched behind it, a healthy command
    unaffected and silent, and a driver refusal still reported as an error
    rather than a timeout.
  • go/internal/drivers/registry_command_deadline_test.go — the twin of
    the existing TestSendDefaultPassesCallerContextToRuntime for the
    command path. The fake runtime ignores the context completely, which is
    the honest model of a Lua driver sitting in a socket read: Send must
    return to its caller while the driver is still inside Command.

make verify clean (it also runs as this repo's pre-commit hook).

Known residual, not fixed here

A driver wedged across several ticks still accumulates commands in its
cmdCh (buffer 8) and replays them on recovery. That predates this
change and applies equally to SendDefault; the queue is FIFO, so the
last state applied is the newest one. Worth its own change if it shows up
in the field — a different question from the one this PR answers.

Likewise, N wedged drivers still cost N × timeout in one tick, because
the sends are sequential. Bounding the whole fan-out (parallel sends, or
a shared per-tick budget) is a larger change to the dispatch block and
would collide with the open work on main.go.

Contention

go/cmd/ftw/main.go is claimed by #732, #734, #735, #736, #741 and #746.
None of them touch this block: their hunks are at lines 24–53 (imports),
992–1103, 1504, 2081, 2442, 2488 and 2967. The new helper lives in its
own file precisely so that #741 — which rewrites sendDriverDefault to
route through the API server — does not have to merge around it.

🤖 Generated with Claude Code

The control tick handed Registry.Send the process-lifetime context. That
context has no deadline, and the driver goroutine runs the device call
inline, so a driver wedged in an unanswered HTTP or OAuth request held
the tick for the whole site — every other battery, and the reactive fuse
guard with them.

Each dispatch command now carries its own deadline, derived from
site.control_interval_s: half the interval, capped at driverDefaultTimeout
so a command never gets longer to give up than the safety default that has
to reach the same driver, floored at 250 ms so a sub-second interval cannot
starve a healthy LAN write. A command that runs out of time is logged at
Warn with the driver name.

sendDriverDefault already bounded its call for exactly this reason; the
fix had landed on the rare path and missed the one that runs every tick.

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 23587f3fb3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

func sendDriverCommand(ctx context.Context, reg driverCommandSender, kind, name string, payload []byte, timeout time.Duration) {
cmdCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
err := reg.Send(cmdCtx, name, payload)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Discard expired commands before dequeuing them

When a legacy Lua driver_command ignores cancellation and remains blocked past this timeout, Registry.Send returns even though the registry has already accepted the command; subsequent ticks can then fill the eight-entry cmdCh. runLoop executes each queued command without checking cmd.ctx.Err(), and LuaDriver.Command ignores the context, so recovery replays obsolete battery or PV setpoints. If the queue is full when WatchdogScan makes its one-shot offline transition, SendDefault also times out before enqueueing and is not retried, leaving the stale driver controlled instead of autonomous. Expired controls should be discarded or coalesced, and default mode must supersede them.

AGENTS.md reference: AGENTS.md:L35-L36

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant