Skip to content

MILAB-5933: honour an explicit request-timeout in test configs - #1785

Open
xnacly wants to merge 1 commit into
MILAB-5933_hz-e2e-runner-fixfrom
MILAB-5933_e2e-honour-request-timeout
Open

MILAB-5933: honour an explicit request-timeout in test configs#1785
xnacly wants to merge 1 commit into
MILAB-5933_hz-e2e-runner-fixfrom
MILAB-5933_e2e-honour-request-timeout

Conversation

@xnacly

@xnacly xnacly commented Aug 18, 2026

Copy link
Copy Markdown
Member

plAddressToTestConfig parsed the address and then overwrote defaultRequestTimeout with a flat 500ms, so the request-timeout=3000 the pl e2e job puts in PL_ADDRESS never took effect.

Invisible against localhost, fatal against the k8s service DNS name: a cold gRPC channel cannot resolve and get an LB pick inside 500ms, so whichever test opened it failed with Deadline exceeded after 0.498s ... Waiting for LB pick against a healthy backend. Addresses without request-timeout keep the short default.

Greptile Summary

The PR preserves an explicitly parsed request-timeout when creating test client configuration while retaining the short 500ms default for addresses without that parameter.

  • Updates plAddressToTestConfig so an address-level timeout takes precedence over TEST_REQUEST_TIMEOUT.
  • Documents why Kubernetes cold-channel setup requires a longer explicit deadline.
  • PlClientConfig: The parsed client connection configuration; its defaultRequestTimeout is no longer unconditionally overwritten by the test helper.
  • plAddressToTestConfig: The test configuration adapter; it now applies the 500ms test timeout only when the address does not specify request-timeout.
  • request-timeout: An address query parameter defining the default request deadline; explicit values such as 3000ms are now honored in tests.
  • TEST_REQUEST_TIMEOUT: The 500ms fail-fast default for tests; it remains active for addresses without an explicit timeout.

Confidence Score: 5/5

The PR appears safe to merge, with the intended explicit Kubernetes request timeout now preserved and no actionable regression established.

The changed helper continues applying the 500ms test default when no timeout parameter is present and retains the parsed numeric timeout for the supported CI address form.

Important Files Changed

Filename Overview
lib/node/pl-client/src/test/test_config.ts Correctly gives an explicit address-level request timeout precedence over the short test default without changing ordinary local test behavior.

Reviews (1): Last reviewed commit: "MILAB-5933: honour an explicit request-t..." | Re-trigger Greptile

Context used:

…t configs

plAddressToTestConfig overwrote the parsed defaultRequestTimeout with a flat
500ms, discarding the request-timeout=3000 the e2e CI puts in PL_ADDRESS. Against
a cluster service DNS name the first call on a cold channel has to resolve DNS and
get an LB pick inside that budget, so whichever test opened the cold channel failed
with DEADLINE_EXCEEDED against a healthy backend - a different test each run, which
read as flakiness. Addresses without request-timeout keep the 500ms default.
@notion-workspace

Copy link
Copy Markdown

@changeset-bot

changeset-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 8579f52

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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