fix(subprocess): bound the spawn backoff by the wait, not by the budget

`cbm_spawn_backoff` clamped its shift with

    attempt < CBM_SPAWN_RETRY_ATTEMPTS ? attempt : CBM_SPAWN_RETRY_ATTEMPTS

and its comment said "the budget is the only bound needed — callers never
exceed it, and a second clamp would be dead code". The clamp that stayed is the
dead one: both retry loops return before `attempt` reaches the budget
(cbm_fork_with_retry, and the posix_spawn loop on Apple), so it can never fire.

What it leaves unbounded is the case it was written for. Nothing stops the
doubling, so the schedule explodes with the budget rather than flattening:

    budget  6   10 20 40 80 160 320                                 630ms
    budget  9   10 20 40 80 160 320 640 1280 2560                  5110ms
    budget 12   10 20 40 80 160 320 640 1280 2560 5120 10240 20480 40950ms

40s of waiting is exactly the "hang instead of fail fast" this retry exists to
avoid, and it is one edit to the budget away.

Bound a single wait instead, at 2560ms — the largest wait either shipping
budget already produces. Measured old vs new across both:

    budget 6   old 630ms   new 630ms    identical
    budget 9   old 5110ms  new 5110ms   identical
    budget 12  old 40950ms new 12790ms  flattens at the ceiling

So nothing changes today; a future budget increase now costs 2560ms per extra
attempt instead of doubling. The ceiling is spelled as a shift so the value
cannot overflow `long` on the way to being clamped, and the doubling comment
above now names both bounds instead of only the budget.

clang-format clean; subprocess suite green (14 passed, 17 skipped — the POSIX
retry probes skip on this Windows box, so the loop bounds themselves are CI's to
confirm).

Signed-off-by: Mauricio Offermann <mauricio.offermann@gocode.cl>
This commit is contained in:
Mauricio Offermann
2026-08-14 08:49:50 -04:00
parent 973cde86a3
commit e1921bc212
+22 -8
View File
@@ -912,8 +912,9 @@ enum { CBM_SPAWN_RETRY = 2, CBM_SPAWN_RETRY_ATTEMPTS = 6 };
/* Exponential backoff, doubling from 10ms: 10, 20, 40, 80, 160, 320 — ~630ms of
* total patience on an ordinary build, and three further doublings (640, 1280,
* 2560) to roughly 5s on a sanitized one. The waits follow from
* CBM_SPAWN_RETRY_ATTEMPTS above rather than being listed separately here, so
* changing the budget cannot leave this description behind.
* CBM_SPAWN_RETRY_ATTEMPTS above and from the per-wait ceiling at
* cbm_spawn_backoff, rather than being listed separately here, so changing the
* budget cannot leave this description behind.
*
* The first version waited a flat 3 x 10ms, which was enough for a momentary
* dip and NOT enough for the real thing: a CI runner building and testing in
@@ -948,13 +949,26 @@ static bool cbm_spawn_eagain_injected(void) {
}
#endif
/* The bound is on a single WAIT, and 2560ms (10ms << 8) is the largest wait
* either budget produces today: 10..320 on an ordinary build, 10..2560 on a
* sanitized one. So this changes nothing now — it changes what happens next.
*
* The clamp it replaces bounded `attempt` by CBM_SPAWN_RETRY_ATTEMPTS, which no
* caller can reach: both retry loops return before passing the budget, so the
* clamp never fired and the comment claiming "the budget is the only bound
* needed" described a bound that did not exist. Doubling with nothing to stop
* it is the actual risk, and it grows fast — a budget of 12 would make the last
* wait ~20s and the total ~41s, which is precisely the "hang instead of fail
* fast" the retry was written to avoid. Flattening at the ceiling keeps a
* budget increase linear.
*
* Spelled as a shift so the value cannot overflow `long` on the way to being
* clamped. */
enum { CBM_SPAWN_BACKOFF_BASE_MS = 10, CBM_SPAWN_BACKOFF_MAX_SHIFT = 8 };
static void cbm_spawn_backoff(int attempt) {
/* Cap the shift at the budget, not at a constant: with the sanitized budget
* of 9 a hard-coded 6 would flatten the last three waits to 320ms each
* instead of continuing to double. The budget is the only bound needed —
* callers never exceed it, and a second clamp would be dead code. */
int shift = attempt < CBM_SPAWN_RETRY_ATTEMPTS ? attempt : CBM_SPAWN_RETRY_ATTEMPTS;
long ms = 10L << shift;
int shift = attempt < CBM_SPAWN_BACKOFF_MAX_SHIFT ? attempt : CBM_SPAWN_BACKOFF_MAX_SHIFT;
long ms = (long)CBM_SPAWN_BACKOFF_BASE_MS << shift;
struct timespec delay = {ms / 1000L, (ms % 1000L) * 1000L * 1000L};
(void)cbm_nanosleep(&delay, NULL);
}