Retry shared e2e cluster start on transient node-placement failures

[rugpanov]

Why

The e2e suite warms one shared cluster (TEST_DEFAULT_CLUSTER_ID) per shard in the wdio onPrepare hook, and it is cold-started on every nightly run. When the test region is capacity-tight, its node placement terminates (e.g. Timeout while placing nodes) and aborts the shard — even though a fresh start() usually succeeds. Because ~40 shards share the one cluster, a single placement blip fails a whole batch at once; this signature dominated recent red nightlies.

What

startCluster (the warm-up helper) now retries the start instead of failing on the first terminal launch:

• Re-issues start() on any post-start terminal state (TERMINATED/ERROR/UNKNOWN), bounded to 3 attempts with a jittered backoff, all within the existing single 60-minute deadline (so total wall-time stays bounded and ~40 shards don't re-start in lockstep).
• Still fails fast when the control plane rejects the start() call itself and the cluster stays terminal (auth / bad request) — retrying an already-rejected call can't help.
• Surfaces the cluster's own failure reason once the attempts are spent.

Test-harness only; no production extension code, settings, telemetry, or persisted state changes.

Testing

• startCluster.test.ts (11 cases, ts-mockito + faked timers): covers already-RUNNING, stopped→RUNNING, the concurrent-start race (initial and on a retry), TERMINATING-wait, fail-fast on a rejected start(), transient-terminal-then-RUNNING, retry-to-max-then-surface-reason, deadline-exhausted-by-backoff, stuck-PENDING timeout surfaced without retry, and deadline-already-spent-on-entry.
• CI green on this branch: Integration Tests, unit tests (linux + windows), VSIX packaging, Trigger Tests. The Integration run exercised the new path — a shard that hit a real Timeout while placing nodes recovered and completed instead of aborting warm-up.

Reviewer notes

• Retry is deliberately not gated on the failure's reason string. Matching cloud error prose is brittle (wording drift silently disables it) and only ever under-retries. The trade-off: a genuinely broken shared cluster (e.g. bad Spark config) is now retried up to 3× — bounded, ~20–60s of extra delay — before the error surfaces, versus failing on the first attempt previously.
• Out of scope: the stuck-PENDING case (cloud never terminates placement). It is not fixable by retrying and belongs to a separate warm-cluster / instance-pool change in the test environment; such a poll timeout is surfaced as-is here, not retried.

This pull request and its description were written by Isaac.

[rugpanov]

🤖 Integration tests running for cf50af3e — ⏳.
View run

[rugpanov]

🤖 Integration tests running for 8b8908e2 — ⏳.
View run

[rugpanov]

🤖 Integration tests running for 6d847f22 — ⏳.
View run

[rugpanov]

🤖 Integration tests ❌ failed for 238e063d.
View run

[github-actions]

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/vscode

Inputs:

• PR number: 2182
• Commit SHA: 4f7c68716555bff01d81999a1fb615e688af41e4

Checks will be approved automatically on success.

[rugpanov]

🤖 Integration tests ✅ passed for 4f7c6871.
View run

[anton-107]

The cap here is doing two different jobs, and it's only right for one of them:

backoffMs = min(jitter, remaining)
              |        |
              |        +-- "don't sleep past the deadline"  <- wrong response
              +-- "space out ~40 shards"

When remaining < jitter there isn't room for another attempt, so the loop sleeps out the clock and then discovers that.

The timeline from your own stops re-starting once the deadline is exhausted by backoff test (timeout = 5s, jitter in [10s, 30s]):

t=0ms     get() -> TERMINATED
          attempt 1: remaining 5000 > 0  OK
          start()  ->  poll  ->  TERMINATED  ->  ClusterStartError
          attempt 1 < MAX, so back off
          backoffMs = min(  ~18000,  5000  )  =  5000
                            ^jitter  ^remaining  <- already knows a retry is impossible
t=0ms     sleep(5000) ---------------------+
                                           |  nothing happens
t=5000ms  <--------------------------------+
          attempt 2: remaining 0  ->  break
          throw lastError

The sleep is decided after the value that makes it pointless is already in hand.

The cap can go away entirely, because the guard subsumes it: if there's room to sleep, sleeping the full jitter is safe; if there isn't, the answer is break. That also drops the now-dead backoffMs > 0 check.

Suggested change

	const backoffMs = Math.min(
	Math.round(
	START_RETRY_BACKOFF.toMillSeconds().value *
	(0.5 + Math.random())
	),
	remaining().toMillSeconds().value
	);
	if (backoffMs > 0) {
	await new Promise((resolve) => setTimeout(resolve, backoffMs));
	}
	const backoffMs = Math.round(
	START_RETRY_BACKOFF.toMillSeconds().value *
	(0.5 + Math.random())
	);
	// No room to back off and still launch, so stop rather than
	// sleeping out the remaining deadline.
	if (backoffMs >= remaining().toMillSeconds().value) {
	break;
	}
	await new Promise((resolve) => setTimeout(resolve, backoffMs));

This is free with respect to the existing test, which asserts verifyStarted(1) and a /Timeout while placing nodes/ message. Both hold either way:

	verifyStarted(1)	error message	wall clock
current (sleep 5s)	pass	pass	5000ms
with early break	pass	pass	0ms

So the test never pinned the sleep, only the clock did.

One knock-on worth noting: after this change the remaining() <= 0 guard at the top of the loop is no longer reachable via the backoff path. It stays useful only for a caller-supplied already-expired deadline, which your 0 ms test covers.

Worst case in production is one jitter window (~30s) per failing shard, so this is a nit rather than a blocker.

[rugpanov]

Thanks Anton — agreed, this is a clean simplification (break instead of sleeping out the deadline, and the backoffMs > 0 check does become dead code). Since you flagged it non-blocking and it's test-neutral, I'm intentionally leaving it as-is here to avoid re-spinning the ~45-min integration run and churning the approval — noting it as a small follow-up. Appreciate the careful read on the shared-deadline invariant.

[anton-107]

LGTM, approving.

I checked the parts most likely to go wrong here:

• The single shared deadline still holds across the new loop. remaining() is a closure re-evaluated at each retry({timeout: remaining()}), so 3 attempts plus 2 backoffs can't exceed the caller's bound. The worst case is unchanged from main, where one stuck-PENDING poll could already burn the full hour.
• Dropping the reason-string classification is the right call, for the reason you give: matching cloud error prose only ever under-retries, and it fails silently when the wording drifts.
• terminationReason() is a real improvement over the old state_message ?? termination_reason. ?? only skips null/undefined, so an empty-string state_message used to produce an empty reason; now it falls through and both fields survive.
• verifyStarted(n) on every case pins the attempt count, which is the thing most likely to regress silently in this file.

One non-blocking nit inline, on the backoff cap sleeping out the remaining deadline. Good to fix, but don't hold the PR on it.

CI green on 4f7c6871, including an Integration run that exercised the new path on a real placement timeout.