Fix MSP status polls leaking into CLI session on tab entry - #2688
Merged
sensei-hacker merged 2 commits intoJul 20, 2026
Merged
Conversation
cliTab.initialize() flushed the MSP queue synchronously but only set CONFIGURATOR.cliActive true inside the async cli.html import callback, leaving a window where periodicStatusUpdater's 300ms poll still saw cliActive as false and queued status requests that never got flushed again. Those requests went out over the wire and corrupted the FC's CLI output (visible as leaked MSP2 frame bytes). Move the flag to the first line of initialize() so no interval tick can land in the gap. Also guard the tab-click handler's cliTab.exit() call with cliValid in addition to cliActive, since cliActive can now be true before the FC has confirmed CLI mode was actually entered.
MSP.send_message()'s queue-put can be rejected as a duplicate when the same MSP code is still awaiting a response (routine on a slow/loaded serial link where round-trip exceeds the poll interval). The rejected message retried every 150ms for up to 25 attempts without ever re-checking CONFIGURATOR.cliActive, so a status poll deferred right before switching to the CLI tab could still land several seconds later, once the FC was already echoing raw bytes in CLI mode. Found by reproducing the original bug's exact symptom live after the first fix landed. Check cliActive before every attempt, not just the one made at send_message() time.
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
|
Configurator test build ready — commit Download build artifacts for PR #2688 Available platforms (scroll to the Artifacts section at the bottom of the run page):
|
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
1 participant
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.
Summary
Fixes the CLI tab intermittently corrupting its own session on entry, visible as
$X<$X<...garbage in the CLI output/input area (a leaked MSP2 request frame —<is the MSPTO_MWCdirection byte).Two independent bugs contributed to this, both closed here:
cliTab.initialize()flush-then-flag race (tabs/cli.js): the MSP queue was flushed synchronously, butCONFIGURATOR.cliActivewasn't settrueuntil inside the asynccli.htmlimport callback.periodicStatusUpdater's 300ms status-poll interval (kept alive across tab switches for the header status bar) checkscliActivebefore sending, but during that async gap it still sawfalseand queued sends that were never flushed again. Fix: set the flag as the very first statement ininitialize(), before the flush and before the async import, closing the race entirely (JS is single-threaded).This also widens the window where
cliActivecan betruebefore the CLI DOM/session is confirmed ready. Re-audited every othercliActiveconsumer (CliAutoComplete.js,serial_backend.js,configurator_main.js) for timing assumptions; the tab-click handler'scliTab.exit()call needed an addedcliValidcheck (it only becomes true once the FC has actually echoed CLI-mode confirmation) to avoid sending a strayexit\rbefore the FC has even been told to enter CLI mode.MSP._enqueue()'s duplicate-retry loop bypassing CLI mode (js/msp.js): found while live-testing fix LTM - Add telemetry support #1 against real hardware — the garbage still reproduced, well after CLI mode was fully confirmed. Root cause:mspQueue.put()rejects a message as a duplicate if the same MSP code is still awaiting a response (routine when round-trip time exceeds the poll interval, e.g. slower serial links). The rejected message retries every 150ms for up to 25 attempts, entirely independent of whoever originally calledsend_message()— and never re-checkedCONFIGURATOR.cliActiveon each retry. A poll deferred right before switching to CLI could still land several seconds into the session. Fix: checkcliActivebefore everyput()attempt, not just when deciding whether to schedule the next retry.Testing
tests/cli-tab-msp-polling.test.mjs(4 tests): executes the realcliTab.initialize(),periodicStatusUpdater.run(), andMSP._enqueue()/mspQueueproduction code (via mechanical import-specifier substitution, not mocks/reimplementations) to reproduce and verify both races, plus two positive-control tests proving the mechanisms aren't vacuously passing.npm test).Code Review
Reviewed with the
inav-code-reviewagent in two passes (one per commit) — no CRITICAL or IMPORTANT issues outstanding; one MINOR comment-length nitpick addressed.