Skip to content

fix(drivers): latch failed Modbus reprobes and keep the old VM - #984

Open
frahlg wants to merge 1 commit into
masterfrom
cursor/modbus-reprobe-review-fixes-1341
Open

fix(drivers): latch failed Modbus reprobes and keep the old VM#984
frahlg wants to merge 1 commit into
masterfrom
cursor/modbus-reprobe-review-fixes-1341

Conversation

@frahlg

@frahlg frahlg commented Aug 27, 2026

Copy link
Copy Markdown
Member

Accepted text proposal

Issue or Discussion: Follow-up to merged #982 (Fredrik merged without reading the reviews).

Maintainer comment that accepted this scope: Fredrik asked to check those reviews and fix anything that needed it.

What changed

The Modbus give-up reload from #982 now matches the safety boundaries that PR stated but did not implement:

  • A failed reload (os.ReadFile / execute / driver_init) sets skipReprobe, so a missing file is not a per-poll loop.
  • sawModbusRead is armed only on Successes > 0, not on failed attempts. A device that never answered is not reloaded once its give-up tables go quiet.
  • reprobeLocked runs driver_init on the candidate VM before closing the previous one. Init failure restores the old VM so driver_default_mode still has its locals.
  • Re-reading the driver file from disk is intentional: catalog drivers are already hot-editable, and this path is a probe retry.

A prolonged outage still retries after give-up. Codex suggested latching after every reload; that would recover a 15-second flap and miss the ~9-minute Pixii blip this path exists to recover.

Why

Reviews on #982 (Codex P1/P2, Bugbot high + two mediums). Human review by miravoss26 said safe to merge, with a non-blocking question about re-reading from disk. The bot findings contradicted #982's own stated boundaries:

  • "After a failed reload, reprobe is skipped until the process restarts" — was not implemented.
  • "A driver that never successfully read is not reloaded" — Attempts > 0 treated transport failures as success.
  • Default-mode after a stale/failed driver must still run (AGENTS.md). Swapping before init left an uninitialized VM.

Boundaries and safety

Out of scope: changing catalog pixii.lua give-up to ignore transport errors (belongs in srcfl/device-drivers).

Verification

  • go test ./internal/drivers/ -count=1 -run 'GiveUp|PixiiRecovers' — pass.
  • go test ./internal/drivers/ -count=1 — pass.
  • Rebased onto master after #981 and srcfl/ftw-webapp#56, so the catalogue drift check should now compare against app main that has driver.offline / fuse.over_limit.

Checklist

  • The diff implements one accepted scope and does not add follow-on work.
  • I checked open pull requests that touch the same files.
  • Tests cover the changed behaviour and its failure path.
  • A human reviewed every changed web/UI view in a browser, or no UI changed.
  • A Changeset is included, or the change is exempt.
  • Every commit has a DCO sign-off.
Open in Web Open in Cursor 

PR #982 reloaded a give-up driver, but a missing file retried every
poll, failed attempts armed reprobe on a never-online device, and
driver_init ran after the previous VM was already closed. Latch on
reload failure, arm only after a successful read, and initialize the
candidate before swapping so default-mode still has its locals.

A long outage still retries: latching after every reload would miss
the 9-minute Pixii blip that this path exists to recover.

Signed-off-by: Cursor Agent <cursoragent@cursor.com>

Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
@cursor
cursor Bot force-pushed the cursor/modbus-reprobe-review-fixes-1341 branch from 3387c42 to 18cf2b9 Compare August 27, 2026 12:49
@frahlg
frahlg marked this pull request as ready for review August 27, 2026 12:49

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

Not approved: Cursor Bugbot was present but still pending after the 8-minute wait, so the required review signal did not complete. Human review is needed; reviewers were assigned.

Open in Web View Automation 

Sent by Cursor Approval Agent: Pull Request Router and Approver

@cursor
cursor Bot requested review from Leitet and erikarenhill August 27, 2026 12:57

@miravoss26 miravoss26 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed. Fixes two real reload-loop bugs in the Lua Modbus give-up path.

What it does:

  1. notePollModbusActivity now keys off Successes instead of Attempts — a device that never answers no longer counts as "saw a read," so it stays given-up instead of reload-looping every poll.
  2. reprobeLocked now calls driver_init on the new VM before discarding the old one. If init fails, the old VM is kept (so default-mode keeps working) instead of leaving the driver on a half-initialized VM.
  3. A failed reprobeLocked call now latches skipReprobe = true — a missing driver file no longer gets retried every single poll.

Correctness: the four new tests exercise exactly the distinction that matters here — failed-reload latches vs. successful-reload-but-device-still-down keeps retrying (the sustained-outage test is a good guard against over-latching, given the Pixii 9-minute-flap note in the comment). Old-VM-kept-on-failed-init is directly asserted via register-read counts staying flat and DefaultMode() still working post-failure.

Security: no secrets, no new deps, no new network destinations, no injection surface change. Pure state-machine fix.

Safe to merge from my read.

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