Fix TCP console restore status wait timeout by jay23606 · Pull Request #530 · microsoft/nvx · GitHub
Skip to content

Fix TCP console restore status wait timeout - #530

Merged
Pedro Henrique Penna (ppenna) merged 3 commits into
microsoft:devfrom
jay23606:pr-scout/504-track-tcp-console-bugs-status-query-time
Oct 14, 2026
Merged

Pedro Henrique Penna (ppenna) merged 3 commits into
microsoft:devfrom
jay23606:pr-scout/504-track-tcp-console-bugs-status-query-time

Conversation

@jay23606

Copy link
Copy Markdown
Contributor

Bug

A restore status query can be received in the same TCP read as an earlier snapshot milestone. The monitor validates it, but the subsequent wait used the current query count as its baseline and timed out waiting for a second query.

Change

Return immediately when the monitor has already observed a status query. Added a regression test covering a restore response consumed during milestone waiting.

Testing

  • git diff --check

Regression test: fails on base twice (/TimeoutError: nvx-time status did not exit within 0.2s on the TCP console/), passes on fix twice (python -m unittest scripts.test_time_abi.RunnerWiringTests.test_tcp_console_status_query_consumed_with_restore_milestone)

Fixes #504

Copilot AI balanced review requested due to automatic review settings October 13, 2026 19:35

Copilot AI 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.

🟡 Changes recommended

The new logic breaks repeated waits, and the regression test does not reliably enforce the triggering receive layout.

2 open findings
What changed in this PR

Fixes TCP console restore-status waits when the response was consumed alongside a snapshot milestone.

Changes:

  • Returns when a status query was previously observed.
  • Adds a regression test for coalesced milestone/status output.
File Description
scripts/​nvx_tools/​openvmm_process.py Adjusts status-query waiting logic.
scripts/​test_time_abi.py Adds the coalesced-response regression test.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/nvx_tools/openvmm_process.py Outdated
Comment thread scripts/test_time_abi.py Outdated
Copilot AI balanced review requested due to automatic review settings October 13, 2026 21:45

Copilot AI 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.

🔵 Needs a closer look

The implementation breaks repeated status waits and an existing test that validates them.

2 open findings

🧠 Review effort: Balanced

wait_for_time_abi_status returned on any status query ever observed, so
the next-query contract was lost. The snapshot-tier caller now captures
the status count before its milestone reads and waits since that
baseline. The regression test reads its bytes in one scripted chunk so
the query is always consumed by the milestone read.

Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings October 13, 2026 22:14

Copilot AI 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.

🟢 Approval recommended

The focused implementation resolves the reported TCP buffering race and includes appropriate regression coverage.

0 open findings

2 resolved since last review

🧠 Review effort: Balanced

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.

Track TCP console bugs: status-query timeouts, unbounded cleanup

3 participants