Harden check_vendored_sync.sh curl calls against transient failures - #671
Open
realmarcin wants to merge 4 commits into
Open
Harden check_vendored_sync.sh curl calls against transient failures#671realmarcin wants to merge 4 commits into
realmarcin wants to merge 4 commits into
Conversation
Add --retry 3 --retry-delay 2 --max-time 30 to both curl calls, matching the fix propagated from culturebotai-claw's canonical shared/spoke copy (culturebotai-claw#88, CultureMech#298). Does not add the history.yaml MAPPED entry that the canonical copy now carries: scripts/.vendored_canon_ref here still pins a hub commit that predates history.yaml existing in the hub, so the entry would 404 rather than catch real drift. Tracked separately: #670.
…ttempt --max-time resets on every --retry attempt, so the prior fix (--retry 3 --retry-delay 2 --max-time 30) could take up to (1 + 3) x 30s + 3x2s ~= 126s per curl call, not the ~30s the flags suggest — empirically confirmed against a blackholed address and independently flagged by dynamic-review on TraitMech#492 and proteintraitsmech#525. With 6 sequential calls per invocation and this repo's own outer retry loop around the whole script, worst case could exceed the CI job's timeout — worse than before the retry flags were added. Replace with --max-time 10 --retry 2 --retry-delay 1 --retry-max-time 15, which bounds the whole retry sequence via --retry-max-time rather than relying on --max-time alone.
Contributor
Author
|
Round-2 fix pushed for the same nested-retry timing issue independently flagged on TraitMech#492: replaced |
… attempt dynamic-review on TraitMech#492 caught that --retry-max-time only gates the DECISION to start a new retry, not an already-in-flight attempt's duration — so --max-time 10 --retry 2 --retry-delay 1 --retry-max-time 15 still costs ~21s worst case per call (10s + 1s delay + 10s), empirically confirmed. With 6 sequential calls and this repo's own outer 3-attempt/5s-backoff loop around the whole script, worst case was still ~388s against a 5-minute job timeout. The workflow's own retry loop already exists specifically to handle transient failures across whole script invocations. Script-level --retry was redundant with it and is what caused the compounding. Drop it: --max-time 10 alone bounds each call, giving a worst case of 6 x 10s = 60s per invocation, ~190s with the outer retry loop — comfortable margin under 300s.
Contributor
Author
|
Round-3 fix pushed: dropped |
dynamic-review on MediaIngredientMech#428: the --retry decision (and why it was reverted) lived only in PR descriptions, not the script itself. Since this file is vendored byte-identical across the spokes and edited rarely, a future editor had no in-repo signal against re-adding --retry flags to "improve" resiliency and reintroducing the timeout risk already found and reverted once.
Contributor
Author
|
Propagated the same documentation comment added to MediaIngredientMech#428 (explaining why this script deliberately has no |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Propagates culturebotai-claw#88's hardening of the canonical `check_vendored_sync.sh` (CultureMech#298): adds `--max-time 10` to both `curl` calls, bounding each fetch instead of letting it hang indefinitely.
Retry history: an earlier revision of this PR (and the canonical claw source) also added `--retry`/`--retry-delay`/`--retry-max-time` flags. dynamic-review caught that curl's `--retry-max-time` only gates the decision to start a new retry, not an in-flight attempt — so the worst case still exceeded this workflow's 5-minute job timeout once combined with `vendored-sync.yaml`'s own outer 3-attempt/5s-backoff retry loop (which already exists to handle transient failures across whole script invocations). Final fix drops `--retry` entirely: `--max-time 10` alone, relying on the outer loop for retries.
Does not add the `history.yaml` MAPPED entry the canonical copy now carries — `scripts/.vendored_canon_ref` here still pins a hub commit (`6be694f3`) that predates `history.yaml` existing in the hub, so the entry would 404 rather than catch real drift. This repo's local `history.yaml` is currently byte-identical to the hub's current copy regardless. Tracked separately: #670.
Test plan