Skip to content

test(harvest): drop the wall-clock upper bound that made BackoffPositive flaky in real CI - #952

Merged
jsboige merged 1 commit into
masterfrom
fix/flaky-backoff-upper-bound
Jul 27, 2026
Merged

test(harvest): drop the wall-clock upper bound that made BackoffPositive flaky in real CI#952
jsboige merged 1 commit into
masterfrom
fix/flaky-backoff-upper-bound

Conversation

@jsboige

@jsboige jsboige commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Constat

Depuis #911, l'étape Test de la CI exécute réellement les tests. Un test est apparu intermittent : HarvestManagerRetryAsyncTests.BackoffPositive_AppliesDelayBetweenAttempts.

Mesure (2026-07-26) — sur les 26 PRs npm groupées ouvertes par dependabot en 5 minutes :

Runs Résultat Test en échec
24 ✅ vert
#928, #935 build (Debug) BackoffPositive_AppliesDelayBetweenAttempts [3 s], Total tests: 643 / Passed: 637 / Failed: 1 / Skipped: 5

Les diffs de #928 et #935 ne touchent que des lockfiles npm vendorés sous DNNPlatform/Portals/1/2sxc/**aucun code .NET ne peut être en cause. La variable réelle est la contention du runner : 26 jobs déclenchés dans la même fenêtre de 5 minutes.

L'assertion qui casse :

sw.Elapsed.Should().BeLessThanOrEqualTo(TimeSpan.FromMilliseconds(2 * 120 + 2000));  // ≤ 2 240 ms

Le test a mis 3 s pour une opération dont le délai attendu est 240 ms.

Correctif : soustraire, pas élargir

La borne supérieure n'est pas le contrat d'un backoff — « le délai a bien été appliqué » l'est, et c'est la borne inférieure, qui reste dure :

sw.Elapsed.Should().BeGreaterThanOrEqualTo(TimeSpan.FromMilliseconds(2 * 120 - 30),);  // conservée

Passer 2000 à 10000 aurait été le réflexe de contrepoids : ça ne fait que déplacer le niveau de charge auquel la borne ment, et ça ne teste toujours rien. Le commentaire du fichier enregistre la mesure pour que la borne ne soit pas « restaurée » plus tard.

Rien n'est affaibli : si le backoff cessait d'être appliqué, la borne inférieure échoue. C'est bien la seule chose que ce test peut observer de l'extérieur.

Traitement asymétrique de son voisin, délibéré

BackoffZero_DoesNotDelay (même fichier) garde sa borne supérieure < 500 mselle est son contrat : « TimeSpan.Zero n'introduit aucun délai » ne s'observe que par le haut. Elle est du même genre de fragilité en principe, elle n'a pas été observée en échec, et la retirer supprimerait la seule garde du chemin rapide. Noté plutôt que masqué.

Vérification

643 total / 638 réussis / 0 échec / 5 ignorés en 24 s, local (dotnet test, Debug). Aucun continue-on-error, aucun test désactivé, aucun Skip.

Ce que ça ne corrige pas

Le rouge de #949 est un autre défaut, sur d'autres tests, et il est réel — voir la correction postée sur #949 et l'issue #951 (Humanizer v3 renomme les IRI publiées).

Refs #911, #928, #935

🤖 Coordinator ai-01

…ive flaky in real CI

The upper bound (elapsed <= 2*backoff + 2000ms) is not the contract of a backoff;
"the delay was applied" is, and that is the lower bound, which stays hard.

Measured 2026-07-26: the test took 3 s and failed the 2 240 ms bound on dependabot
PRs #928 and #935 while 24 sibling npm runs passed. Those two diffs touch ONLY
vendored npm lockfiles under DNNPlatform/Portals/1/2sxc/**, so no production code
could be implicated -- 26 dependabot runs firing inside 5 minutes contend for the
runner. The flakiness became visible only because #911 made the CI Test step real.

Subtracting rather than widening, deliberately: a bigger number only moves the load
level at which the bound lies, and it still would not be testing anything. The note
in the file records the measurement so the bound is not "restored" later.

Local: 643 total / 638 passed / 0 failed / 5 skipped (24 s).

Refs #911, #928, #935

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jsboige
jsboige merged commit 00d2af5 into master Jul 27, 2026
3 checks passed
@jsboige
jsboige deleted the fix/flaky-backoff-upper-bound branch July 27, 2026 03:29
@jsboige

jsboige commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Preuve empirique de la flakiness (post-merge)

Le diagnostic posé dans le corps de cette PR était : « la borne wall-clock supérieure ment sous charge, ce n'est pas un défaut de RetryAsync ». J'ai voulu le prouver plutôt que de le laisser en diagnostic, avec une expérience qui ne peut donner qu'une réponse : re-lancer les deux runs échoués sur un diff strictement inchangé, hors de la fenêtre de contention.

Si le code était en cause, un re-run sur le même diff échoue de la même façon. Résultat :

run PR diff conclusion du re-run
30225172979 #928 inchangé (npm-067f763e28) success
30225384170 #935 inchangé (npm-6b79b1f9df) success

Deux verts sur deux diffs identiques à ceux qui avaient rougi ⇒ la variable explicative n'est pas dans le diff, elle est dans la charge du runner (26 runs dependabot déclenchés en 5 minutes ; 24/26 verts au premier passage). La borne de 2 240 ms sur une opération de 240 ms mesurait la contention de l'agent CI, pas le backoff.

Ce que ça change concrètement :

  • le retrait de la borne supérieure est justifié par mesure, plus seulement par raisonnement ;
  • et la conséquence inverse est vraie aussi : élargir la borne (2 000 → 10 000 ms) n'aurait fait que déplacer le niveau de charge auquel elle mente. La borne inférieure, elle, échoue toujours dur si le backoff est sauté — c'est elle qui est le contrat.

L'asymétrie avec BackoffZero_DoesNotDelay (dont la borne supérieure est délibérément conservée) tient toujours : là, « aucun délai » est le contrat.

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