Skip to content

test(providerio): widen the idle window in the content-reset test - #1172

Open
ShryukGrandhi wants to merge 1 commit into
Twigpine:mainfrom
ShryukGrandhi:fix/issue-1140-macos-smoke-testscanssedatawithcontextco
Open

ShryukGrandhi wants to merge 1 commit into
Twigpine:mainfrom
ShryukGrandhi:fix/issue-1140-macos-smoke-testscanssedatawithcontextco

Conversation

@ShryukGrandhi

@ShryukGrandhi ShryukGrandhi commented Oct 21, 2026 •

Copy link
Copy Markdown

Summary

TestScanSSEDataWithContextContentResetsOnData flaked once on Smoke (macos-latest). It wrote a data line every 30 ms against a 100 ms idle timeout, which leaves about 70 ms of slack per gap. One scheduling stall that long on a shared runner is enough to fire the idle watchdog.

This change scales the timing as suggested on the issue:

  • idle timeout 100 ms -> 500 ms, so the content window (ContentStallTimeout, 1.2x) is 600 ms
  • 10 writes -> 25 writes at 30 ms, so the run lasts about 750 ms and still outlasts the content window
  • the n check moves from 10 to 25

Each gap now has about 470 ms of slack, and the test still finishes in under a second. Only the test changes.

Linked issue

Fixes #1140

Testing

All on macOS arm64 with go1.26.9:

  • Reproduced the flake: with 16 busy-loop processes and -test.cpu 1, the old test failed 37 of 200 runs with idle timeout (upstream stopped sending data).
  • Simulated a stall deterministically by temporarily adding one extra 120 ms sleep mid-stream (a 150 ms gap). The old test fails every time. The new test passed 20 of 20.
  • Checked that the test still covers its claim: with resetContent() temporarily removed from the data path, the new test fails with stream stalled (upstream kept the connection alive but produced no output).
  • Limitation: if the machine is oversubscribed (40 busy processes on 28 cores), I still saw rare failures (3/200) with real stalls over 500 ms. No wall-clock window survives that much starvation, but stalls that large should be far rarer on CI than the ~70 ms ones that hit the old margin.
  • make fmt-check, go vet ./..., go test -race ./internal/providers/providerio/, go run ./cmd/zero-release build, go run ./cmd/zero-release smoke, and git diff HEAD --check pass.
  • go test ./...: three failures outside this package.
    • internal/sandbox TestPermissionProfileDropsAutomaticMasksCoveredByUserDeny and internal/tui TestRelativeAgeFormatsLastActivity also fail on unmodified main in my environment (the sandbox one depends on GH_CONFIG_DIR, the tui one on today's date).
    • internal/config TestLoadProviderCommandSuccess hit its 5 s timeout during the full run and passes on its own.
  • make lint-static reports four existing findings in other files. make vulncheck reports GO-2026-6809 in the go1.26.9 standard library (fixed in go1.26.10). Neither is related to this change.

Checklist

  • The linked issue already has the issue-approved label.
  • go build ./..., go vet ./..., and go test ./... pass locally (apart from the three unrelated failures above).
  • gofmt clean.
  • Tests added/updated for the change (and run under -race where relevant).
  • UI changes include screenshots or a short recording where possible. (n/a)

Summary by CodeRabbit

  • Tests
    • Updated content-reset test timing and input volume.
    • Adjusted expected results to reflect handling 25 data lines.

TestScanSSEDataWithContextContentResetsOnData wrote a data line every 30ms against a 100ms idle timeout, so a ~70ms scheduling stall on a shared runner tripped the idle watchdog. Use a 500ms idle timeout (600ms content window) and 25 writes (~750ms), which keeps every gap far under the idle timeout while the run still outlasts the content window.

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 21, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: Twigpine/zero/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8d3392c7-4591-4d3c-938c-69ea88abdeca
📥 Commits

Reviewing files that changed from the base of the PR and between 1d233c0 and 795d873.

📒 Files selected for processing: 1
  • internal/providers/providerio/providerio_test.go

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

The content-reset test now emits 25 data lines over approximately 750 ms with a 500 ms idle timeout. It expects 25 handled lines and still checks for clean completion.

Changes

Content-reset test

Layer / File(s) Summary
Stream test timing
internal/providers/providerio/providerio_test.go
The test now uses 25 data events and a 500 ms idle timeout. It expects 25 handled lines and still requires clean completion.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Other · Severity of issue fixed: Low

Suggested reviewers: gnanam1990

Merge Risk: ⚪ Minimal · up to 795d8

The test-only timing adjustment has no identified merge-blocking risk.

Pre-merge checks: Passed 5
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: widening the idle window in the content-reset test to reduce timing-related flakiness.
Linked Issues check Passed Issue #1140 requires reliable completion of TestScanSSEDataWithContextContentResetsOnData for a 30 ms producing stream. The whole-PR diff updates that test only: it uses a 500 ms idle timeout, emits…
Out of Scope Changes check Passed The whole-PR diff contains changes only in internal/providers/providerio/providerio_test.go, within TestScanSSEDataWithContextContentResetsOnData. The timeout, stream length, comments, and expecte…
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Full details: Linked Issues check

Explanation

Issue #1140 requires reliable completion of TestScanSSEDataWithContextContentResetsOnData for a 30 ms producing stream. The whole-PR diff updates that test only: it uses a 500 ms idle timeout, emits 25 data lines at 30 ms intervals, and expects 25 handled lines. The 500 ms timeout gives about 470 ms between the write interval and the idle limit. The test runs beyond the 600 ms content window and still verifies that data resets the content watchdog. These changes address the linked issue and include the required automated test update.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff contains changes only in internal/providers/providerio/providerio_test.go, within TestScanSSEDataWithContextContentResetsOnData. The timeout, stream length, comments, and expected count directly support issue #1140. No unrelated production code, test, or repository changes are present.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving. This is the scaling from #1140, and the numbers hold: each 30 ms gap sits far under the 500 ms idle timeout, and 25 writes still outlast the 600 ms content window, so the content watchdog is still exercised.

Checked on Windows at 795d873:

  • the test passes 5 of 5;
  • with resetContent() taken out of the data path it fails with "stream stalled", so it still tests what it names;
  • with a 120 ms pause injected mid-stream it passes at 500 ms, and at the old 100 ms it fails with the same "idle timeout" error the macOS run hit.

I also approved the CI runs for the fork.

This branch has not been deployed

No deployments
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.

macOS smoke: TestScanSSEDataWithContextContentResetsOnData hit the idle timeout once

2 participants