Skip to content

fix(control): bound control-loop driver sends with a per-command deadline - #841

Closed
Sanjin-Maker wants to merge 1 commit into
srcfl:masterfrom
Sanjin-Maker:agent/dispatch-send-timeout
Closed

fix(control): bound control-loop driver sends with a per-command deadline#841
Sanjin-Maker wants to merge 1 commit into
srcfl:masterfrom
Sanjin-Maker:agent/dispatch-send-timeout

Conversation

@Sanjin-Maker

Copy link
Copy Markdown
Contributor

Summary

  • Battery dispatch, PV curtailment and loadpoint sends waited on Registry.Send with the long-lived loop context; one driver wedged mid-poll (cmdCh full, runLoop busy) stalled dispatch to every other driver for the rest of the tick and beyond.
  • Extract sendDriverCommand (new go/cmd/ftw/dispatch_send.go) applying the same 2 s deadline the watchdog default path already uses (sendDriverDefault, driverDefaultTimeout), and route the battery + curtail loops through it.
  • Wrap the send function injected into loadpoint.NewController with the same deadline — the controller passes long-lived (or context.Background()) contexts in several paths.
  • Failures (including deadline expiry) are logged, not propagated: recovery stays with the existing watchdog/staleness paths.

Relationship to #798: no file overlap (that PR reworks registry.go's default-command queueing; this one bounds the callers in cmd/ftw). The two compose — with #798's priority slot, a timed-out normal command can never starve a safety default. Happy to rebase if #798 lands first.

Verification

  • go test ./cmd/ftw/ -count=1 including new dispatch_send_test.go: blocked-driver send returns at the deadline with a deadline-carrying context; payload pass-through; errors swallowed-and-logged.
  • go build ./..., go vet ./cmd/ftw/.
  • Patch changeset included.

🤖 Generated with Claude Code

…line

Battery dispatch, PV curtailment and loadpoint sends waited on
Registry.Send with the long-lived loop context, so one wedged driver
stalled dispatch to every other driver. Wrap each send in the same 2 s
deadline the watchdog default path already uses; failures are logged and
recovery stays with the watchdog/staleness paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sanjin Naidu <sanjin@sanrowconsulting.com>
@Sanjin-Maker
Sanjin-Maker force-pushed the agent/dispatch-send-timeout branch from 707a5aa to 173e3bc Compare August 6, 2026 20:09

@miravoss26 miravoss26 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.

Bounds every control-loop driver send (battery, PV curtail, loadpoint) with a 2s deadline via a new sendDriverCommand helper, so a driver wedged mid-poll can no longer stall dispatch to every other driver for the rest of the tick.

  • Correctness: matches the existing driverDefaultTimeout pattern, errors are logged not propagated (recovery stays with watchdog/staleness, as stated). dispatch_send_test.go covers the blocked-driver case, pass-through, and swallowed errors — good coverage for a concurrency-adjacent fix.
  • The loadpoint wrapper (main.go lpSend) correctly derives its deadline from the parent ctx via context.WithTimeout, so an already-shorter parent deadline still wins.
  • No secrets, no new deps, no network/authz surface.
  • ⚠️ GitHub reports this PR as CONFLICTING against master and shows no CI check runs yet — worth confirming CI actually ran and rebasing before merge.

Safe to merge from my read once it's rebased and green.

@frahlg

frahlg commented Aug 7, 2026

Copy link
Copy Markdown
Member

Queue-order note: this PR changes go/cmd/ftw/main.go, which the older #701 already owns. #701 is waiting for the required human browser review. Under the repository file-order rule, #701 keeps right of way, so I am not rebasing or merging #841 yet. The current branch is also conflicting and has no checks on this head. Once #701 clears, rebase, run the race test and full make verify, then reassess #841 before #842.

frahlg commented Aug 8, 2026

Copy link
Copy Markdown
Member

Thank you @Sanjin-Maker for digging into a real control-loop liveness risk and for adding focused tests.

We reviewed this stack against current master. Since this branch started, #791, #800 and #805 have landed a broader command-deadline, actuation-result and autonomous-default path. That now covers most of this PR's production behavior. Because #842 and #843 also carry this older commit while targeting master, rebasing the current stack would preserve duplicate and stale code.

We are closing this PR as superseded, not rejecting the problem you identified. If you want to continue in this area, please start with a written issue rather than a replacement code PR. The issue should describe the remaining behavior on current master, the failure order and the test that proves it. We can then agree on one small implementation.

Thank you for the time and care you put into this.

@frahlg frahlg closed this Aug 8, 2026
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.

3 participants