Skip to content

test: stabilize watchdog rotating-log test and promote Windows lane to blocking - #10070

Open
fszcd wants to merge 3 commits into
AstrBotDevs:masterfrom
fszcd:fix/windows-watchdog-flake
Open

test: stabilize watchdog rotating-log test and promote Windows lane to blocking#10070
fszcd wants to merge 3 commits into
AstrBotDevs:masterfrom
fszcd:fix/windows-watchdog-flake

Conversation

@fszcd

@fszcd fszcd commented Sep 13, 2026

Copy link
Copy Markdown

The unit-tests workflow keeps the Windows lane best-effort via continue-on-error, with a comment saying to promote it once the pre-existing Windows failures are fixed. The suite has actually been green on the Windows lane for a while: I scanned the last ~40 workflow runs and every completed Windows lane passed, with one exception — a flaky failure of tests/unit/test_event_loop_diagnostics.py::test_event_loop_watchdog_writes_rotating_log (example run). This PR deflakes that test and promotes the Windows lane to blocking, per the workflow's own TODO comment.

unit-tests 工作流目前用 continue-on-error 让 Windows lane 保持非阻塞,注释写明等既有 Windows 失败修复后应提升为阻塞。我扫描了最近约 40 次 workflow 运行:已完成的 Windows lane 全部通过,唯一例外是 test_event_loop_watchdog_writes_rotating_log 的一次偶发失败(见上方链接)。本 PR 消除该测试的偶发因素,并按注释意图将 Windows lane 提升为阻塞。

Root cause of the flake / 偶发原因

The test asserts that the watchdog's stack dump contains the test file's frame. Two Windows-flavoured races break that:

  1. The watchdog keeps only one dump per stall (deduplicated by heartbeat). If the dump fires before the loop reaches time.sleep() (the heartbeat starts when the watchdog task starts, not when the stall starts) or only after the stall has ended (the watchdog thread is starved through the whole 50 ms stall on a loaded 2-core runner), the dump catches the loop inside pytest machinery instead of the test frame.

  2. On Windows, virus scanners briefly hold freshly written files open, so the prefill write_text() followed ~20 ms later by the dump-time Path.replace() rotation can fail with WinError 32, leaving no .1 file at all (the failure is only logged as a warning).

  3. 每次停滞只保留一份 dump(按 heartbeat 去重)。若 dump 在循环进入 time.sleep() 前触发(heartbeat 在 watchdog 任务启动时记录,而非停滞开始时),或在停滞结束后才触发(高负载双核 runner 上看门狗线程被调度饿着、错过整个 50ms 停滞窗口),dump 抓到的是 pytest 框架栈而非测试栈。

  4. Windows 上杀毒软件会短暂占用刚写入的文件,预填 write_text() 约 20ms 后 dump 时的 Path.replace() 轮转会以 WinError 32 失败,导致根本不生成 .1 文件(仅以 warning 形式记录)。

Modifications / 改动点

  • tests/unit/test_event_loop_diagnostics.py — split the two concerns inside test_event_loop_watchdog_writes_rotating_log:

    • Stack dumping is verified through the watchdog's dump_file parameter (an io.StringIO), retrying the intentional stall until the dump lands inside this test (bounded at 5 attempts).
    • Log rotation is verified synchronously against _open_watchdog_log_file, with a short settle delay after the prefill write so transient file locks (AV scanners) cannot race the Path.replace, plus a bounded retry.
    • No production code is changed; coverage of both behaviours is preserved.
  • .github/workflows/unit_tests.yml — removed continue-on-error for windows-latest and the now-stale comment above it.

  • tests/unit/test_event_loop_diagnostics.py — 在 test_event_loop_watchdog_writes_rotating_log 内拆分的两个关注点:栈 dump 通过 dump_fileio.StringIO)验证,有界重试直至 dump 落入本测试;日志轮转改为直接同步验证 _open_watchdog_log_file,预填写入后留出短暂延迟避开杀毒软件的瞬时文件锁,并加有界重试。不改动任何生产代码,两项行为的覆盖保持不变。

  • .github/workflows/unit_tests.yml — 移除 windows-latestcontinue-on-error 及其上方已过时的注释。

Verification / 验证

  • Full suite on a Windows 11 host (Python 3.13, uv run python -m pytest tests): 2686 passed, 5 skipped (the 5 skips are symlink tests requiring Developer Mode, unrelated to this change).

  • The restructured test file under deliberate CPU saturation (8 busy-loop workers): 40/40 iterations green. For comparison, a naive timing-only fix still failed 5/40 iterations under the same load, and the previous version of the test was observed failing on the Windows CI lane (run).

  • 本机 Windows 11 全量套件(Python 3.13):2686 passed, 5 skipped(5 个跳过为需要开发者模式的 symlink 测试,与本改动无关)。重构后的测试文件在 8 个满载 CPU worker 下连续 40 次全绿;作为对照,仅调整时序的朴素修法在同等负载下仍有 5/40 失败。

  • This is NOT a breaking change. / 这不是一个破坏性变更。


Checklist / 检查清单

  • 😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc. — no new features; test/CI-only change.
  • 👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
  • 🤓 I have ensured that no new dependencies are introduced.
  • 😮 My changes do not introduce malicious code.

Summary by Sourcery

Stabilize event-loop watchdog diagnostics coverage and make the Windows unit-test lane blocking.

Bug Fixes:

  • Stabilize the Windows watchdog rotating-log test by independently validating stack dumps and log rotation with bounded retries and tolerance for transient file locks.

Enhancements:

  • Promote the Windows unit-test workflow lane from best-effort to blocking.

CI:

  • Make Windows unit-test failures block the workflow by removing its continue-on-error setting.

Tests:

  • Refactor the event-loop watchdog test to reliably cover stalled-loop stack dumps and oversized log rotation under Windows scheduling and file-lock conditions.

@sourcery-ai sourcery-ai Bot 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.

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/unit/test_event_loop_diagnostics.py" line_range="57" />
<code_context>
 @pytest.mark.asyncio
 async def test_event_loop_watchdog_writes_rotating_log(tmp_path):
     """The watchdog should write to and rotate its log file."""
-    log_path = tmp_path / "logs" / "event_loop_watchdog.log"
-    log_path.parent.mkdir()
</code_context>
<issue_to_address>
**nitpick:** The test docstring says the watchdog writes to and rotates its log file, but the changed test only makes the watchdog write to `io.StringIO` and tests rotation by calling the private helper directly. The documentation therefore describes an integration behavior that this test no longer exercises.

**Suggested fix:** Update the docstring to describe the two focused checks, or retain an explicit watchdog-to-disk integration assertion.

```suggestion
    """The watchdog should dump stalled-loop stacks and rotate oversized log files."""
```
</issue_to_address>

Sourcery assessment

Approved.


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread tests/unit/test_event_loop_diagnostics.py Outdated
@fszcd

fszcd commented Sep 14, 2026

Copy link
Copy Markdown
Author

Note for maintainers: the CI workflows on this PR are awaiting approval (first-time contributor). Everything is ready on our side — could someone approve the workflow runs when convenient? Happy to address any feedback.

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