Skip to content

Cap the liquid-test polling interval at 5 seconds - #263

Merged
michieldegezelle merged 2 commits into
mainfrom
cap-test-polling-interval
Jul 30, 2026
Merged

Cap the liquid-test polling interval at 5 seconds#263
michieldegezelle merged 2 commits into
mainfrom
cap-test-polling-interval

Conversation

@Benjvandam

Copy link
Copy Markdown
Contributor

Why

fetchResult grows the poll delay 5% per poll with no cap. For long test runs that means:

elapsed poll interval
60s ~4s
5 min ~15s
10 min ~30s
20 min ~60s
~33 min (waitingLimit) ~100s

A liquid test that takes ~10 minutes server-side (observed in be_market CI, e.g. run 28656835005: a single test, 9m51s) is only checked every ~30s by then, so a finished run sits unnoticed for up to a minute. In CI batches that detection lag multiplies.

What

Cap the delay at 5 seconds (reached after ~70s of polling). Detection lag is now bounded at 5s; short runs poll exactly as before, and the steady-state request rate is at most one readTestRun per 5s per run.

Validation

  • New unit test drives a 60-poll run through fake timers and asserts the maximum setTimeout delay is exactly 5000ms (cap reached, never exceeded).
  • Full suite: 566/566 pass.

Context: follow-up from silverfin/bso_github_actions#28 (CI-side retries + parallel firms) — this is the CLI-side piece of the latency work.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR caps liquid-test polling backoff at 5000ms, adds a unit test and test-catalogue entry, updates release notes for versions 1.57.0–1.57.2, bumps the package to 1.57.2, and updates adm-zip.

Changes

Polling Delay Cap

Layer / File(s) Summary
Cap polling delay implementation
lib/liquidTestRunner.js
Adds a 5000ms maximum and caps pollingDelay growth with Math.min during test-result polling.
Test coverage and release documentation
tests/lib/liquidTestRunner.test.js, tests/TESTS.md, CHANGELOG.md
Tests the 5000ms timeout cap, records it in the test catalogue, and adds release notes for polling and run-sampler changes.
Release metadata update
package.json
Bumps the package version to 1.57.2 and updates adm-zip from ^0.5.18 to ^0.6.0.

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

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is missing the required template sections, including Fixes #, Testing Instructions, and the checklist items. Rewrite it to match the template and add an issue link, a concise change summary, step-by-step testing instructions, and both checklists.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: capping the liquid-test polling interval at 5 seconds.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cap-test-polling-interval

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

@michieldegezelle michieldegezelle self-assigned this Jul 6, 2026
@michieldegezelle
michieldegezelle self-requested a review July 6, 2026 12:06
Benjvandam and others added 2 commits July 30, 2026 10:10
The delay between result polls grows 5% per poll with no upper bound, so a
long test run (~10 minutes server-side) ends up being checked only every
30-60 seconds - a finished run can sit unnoticed for up to a minute, and
in CI that lag multiplies across batches. Cap the delay at 5s: at most one
poll per 5 seconds per run (reached after ~70s), bounding the detection
lag without meaningfully increasing request volume for short runs.
@michieldegezelle
michieldegezelle force-pushed the cap-test-polling-interval branch from 5228b01 to 5a493c2 Compare July 30, 2026 08:15

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/liquidTestRunner.js`:
- Around line 314-316: Clarify the polling-rate comment near pollingDelay to
state that the 5-second maximum interval applies only after the delay reaches
its cap; acknowledge that the initial 1000ms delay allows multiple polls within
five seconds.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 19e0b708-bda4-4965-8e23-3fb4b4ef1eb0

📥 Commits

Reviewing files that changed from the base of the PR and between 5228b01 and 5a493c2.

⛔ Files ignored due to path filters (2)
  • package-lock.json is excluded by !**/package-lock.json
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • CHANGELOG.md
  • lib/liquidTestRunner.js
  • package.json
  • tests/TESTS.md
  • tests/lib/liquidTestRunner.test.js

Comment thread lib/liquidTestRunner.js
@michieldegezelle
michieldegezelle merged commit 6666187 into main Jul 30, 2026
3 checks passed
@michieldegezelle
michieldegezelle deleted the cap-test-polling-interval branch July 30, 2026 08:21
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