Skip to content

Stabilize Nacos retry timing assertion - #272

Merged
Telli merged 1 commit into
mainfrom
codex/fix-nacos-retry-timing-flake
Oct 1, 2026
Merged

Telli merged 1 commit into
mainfrom
codex/fix-nacos-retry-timing-flake

Conversation

@Telli

@Telli Telli commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • record retry attempts with monotonic Stopwatch timestamps
  • allow 10 ms of host timer tolerance around the configured 100 ms delay
  • keep the assertion strict enough to detect a missing retry backoff

Context

Merged-main CI run 36889089486 repeated a boundary flake previously seen on main: DynamicSlot_UseToolFailureRetries_ThenFallsBack reported a value that formatted as 100 ms but was fractionally below the exact 100 ms threshold.

Validation

  • focused failing case: passed
  • focused case repeated 10 times: 10/10 passed
  • NacosRouterIntegrationTests: 30 passed, 5 live-only skipped
  • full OpenClaw.Tests suite: 3,194 passed, 11 integration-only skipped
  • git diff --check: passed

Summary by CodeRabbit

  • Tests
    • Improved validation of router retry timing by checking elapsed intervals between consecutive attempts and allowing a small tolerance. Failure messages now identify the attempt pair and measured interval, making timing issues easier to diagnose.
    • Updated timestamp tracking to use monotonic elapsed-time measurements, reducing sensitivity to system clock changes. These changes affect test coverage and diagnostics; no end-user functionality changes.

Copilot AI balanced review requested due to automatic review settings October 1, 2026 16:16
@coderabbitai

coderabbitai Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
.github/copilot-instructions.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d7b0e512-0720-4876-b235-b54cf2b821d1

📥 Commits

Reviewing files that changed from the base of the PR and between e33beba and 62080c5.

📒 Files selected for processing (2)
  • src/OpenClaw.Tests/FakeNacosRouterMcpTools.cs
  • src/OpenClaw.Tests/NacosRouterIntegrationTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The test fixture now records use timestamps with Stopwatch. The integration test measures elapsed time between consecutive timestamps and accepts intervals of at least 90 ms.

Changes

Retry timing test updates

Layer / File(s) Summary
Timestamp capture and retry interval assertion
src/OpenClaw.Tests/FakeNacosRouterMcpTools.cs, src/OpenClaw.Tests/NacosRouterIntegrationTests.cs
The fixture stores Stopwatch timestamps as long values. The integration test checks each consecutive timestamp interval against a 90 ms minimum and reports the attempt pair and measured interval if a check fails.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: tellikoroma

Merge Risk: ⚪ Minimal · up to 62080

This test-only change makes retry timing checks less sensitive to small timer variations while still checking both configured retry delays. No concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: stabilizing the Nacos retry timing assertion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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

Copilot review overview

🟢 Approval recommended

The focused test-only changes correctly address timer-boundary flakiness.

Review effort: Balanced
Findings: None

What changed in this PR

Stabilizes the Nacos retry timing test without affecting runtime behavior.

Changes:

  • Uses monotonic Stopwatch timestamps.
  • Adds a 10 ms tolerance while preserving backoff detection.
File Description
NacosRouterIntegrationTests.cs Updates retry timing assertions.
FakeNacosRouterMcpTools.cs Records monotonic timestamps.

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

@Telli
Telli merged commit 31dd333 into main Oct 1, 2026
22 checks passed
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.

2 participants