Repository navigation
fix(security-tracker-stats): retry 5xx and default cache to temp root #1752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,49 +21,109 @@ | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||
| import re | ||||||||||||||||||||||||||||
| import subprocess | ||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||
| import tempfile | ||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ROOT = os.environ.get("TRACKER_STATS_CACHE", "/tmp/tracker-stats-cache") | ||||||||||||||||||||||||||||
| # The cache defaults under the platform temp root rather than a hardcoded `/tmp`: | ||||||||||||||||||||||||||||
| # a sandboxed run is typically granted its own scratch directory and cannot write | ||||||||||||||||||||||||||||
| # `/tmp`, so `/tmp/tracker-stats-cache` made the first fetch step fail outright. | ||||||||||||||||||||||||||||
| # `tempfile.gettempdir()` honours `$TMPDIR` (POSIX) and `%TEMP%` (Windows). | ||||||||||||||||||||||||||||
| ROOT = os.environ.get("TRACKER_STATS_CACHE") or os.path.join(tempfile.gettempdir(), "tracker-stats-cache") | ||||||||||||||||||||||||||||
| REPO = os.environ.get("TRACKER_STATS_REPO", "airflow-s/airflow-s") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| os.makedirs(ROOT, exist_ok=True) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| print(f"Fetching issue list from {REPO} (state=all, limit 1000) ...") | ||||||||||||||||||||||||||||
| r = subprocess.run( | ||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||
| "gh", | ||||||||||||||||||||||||||||
| "issue", | ||||||||||||||||||||||||||||
| "list", | ||||||||||||||||||||||||||||
| "--repo", | ||||||||||||||||||||||||||||
| REPO, | ||||||||||||||||||||||||||||
| "--state", | ||||||||||||||||||||||||||||
| "all", | ||||||||||||||||||||||||||||
| "--limit", | ||||||||||||||||||||||||||||
| "1000", | ||||||||||||||||||||||||||||
| "--json", | ||||||||||||||||||||||||||||
| # `updatedAt` is consumed by fetch_events.py, which refuses to trust a cached | ||||||||||||||||||||||||||||
| # events file older than the issue's last update. Dropping it here silently | ||||||||||||||||||||||||||||
| # disables that guard, because the check falls back to "no timestamp -> trust | ||||||||||||||||||||||||||||
| # the cache" and every relabelled issue keeps its stale event history. | ||||||||||||||||||||||||||||
| # `body` and `closedByPullRequestsReferences` ride on this one paginated call | ||||||||||||||||||||||||||||
| # so fetch_bodies.py can build issue_extra.json without a `gh issue view` | ||||||||||||||||||||||||||||
| # per issue. | ||||||||||||||||||||||||||||
| "number,title,state,stateReason,createdAt,closedAt,updatedAt,labels,comments,body,closedByPullRequestsReferences", | ||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||
| capture_output=True, | ||||||||||||||||||||||||||||
| text=True, | ||||||||||||||||||||||||||||
| timeout=300, | ||||||||||||||||||||||||||||
| # `gh` reports a server-side hiccup as a non-zero exit with the status in stderr. | ||||||||||||||||||||||||||||
| # A single transient `HTTP 502` on the first call used to abort the entire | ||||||||||||||||||||||||||||
| # dashboard run, so retry the failures that are worth retrying and fail fast on | ||||||||||||||||||||||||||||
| # everything else (auth errors, a bad repo slug, an unknown flag). | ||||||||||||||||||||||||||||
| _TRANSIENT_FAILURE = re.compile( | ||||||||||||||||||||||||||||
| r"HTTP\s+5\d\d" | ||||||||||||||||||||||||||||
| r"|Bad Gateway" | ||||||||||||||||||||||||||||
| r"|Service Unavailable" | ||||||||||||||||||||||||||||
| r"|Gateway Time-?out" | ||||||||||||||||||||||||||||
| r"|Internal Server Error" | ||||||||||||||||||||||||||||
| r"|connection reset" | ||||||||||||||||||||||||||||
| r"|timed out", | ||||||||||||||||||||||||||||
| re.IGNORECASE, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| if r.returncode != 0: | ||||||||||||||||||||||||||||
| raise SystemExit(f"gh failed: {r.stderr}") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| issues = json.loads(r.stdout) | ||||||||||||||||||||||||||||
| with open(f"{ROOT}/issues.json", "w") as f: | ||||||||||||||||||||||||||||
| json.dump(issues, f) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| print(f"Wrote {len(issues)} issues to {ROOT}/issues.json") | ||||||||||||||||||||||||||||
| if len(issues) >= 1000: | ||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||
| "WARNING: the issue list hit the 1000-issue cap; older issues are missing " | ||||||||||||||||||||||||||||
| "and every count in the dashboard is a floor. Raise --limit in fetch_issues.py." | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ATTEMPTS = 3 | ||||||||||||||||||||||||||||
| BASE_DELAY_SECONDS = 2.0 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def is_transient_failure(stderr: str) -> bool: | ||||||||||||||||||||||||||||
| """True when a failed `gh` call looks like a retryable server-side error.""" | ||||||||||||||||||||||||||||
| return bool(_TRANSIENT_FAILURE.search(stderr or "")) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def run_gh( | ||||||||||||||||||||||||||||
| args: list[str], | ||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||
| attempts: int = ATTEMPTS, | ||||||||||||||||||||||||||||
| base_delay: float = BASE_DELAY_SECONDS, | ||||||||||||||||||||||||||||
| timeout: int = 300, | ||||||||||||||||||||||||||||
| ) -> subprocess.CompletedProcess: | ||||||||||||||||||||||||||||
| """Run `gh`, retrying transient failures with exponential backoff. | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Raises `SystemExit` with the last stderr once the attempts are exhausted, or | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Consider wrapping
Suggested change
|
||||||||||||||||||||||||||||
| immediately for a failure that retrying cannot fix. | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
| for attempt in range(1, attempts + 1): | ||||||||||||||||||||||||||||
| r = subprocess.run(args, capture_output=True, text=True, timeout=timeout) | ||||||||||||||||||||||||||||
| if r.returncode == 0: | ||||||||||||||||||||||||||||
| return r | ||||||||||||||||||||||||||||
| if attempt == attempts or not is_transient_failure(r.stderr): | ||||||||||||||||||||||||||||
| raise SystemExit(f"gh failed: {r.stderr}") | ||||||||||||||||||||||||||||
| delay = base_delay * 2 ** (attempt - 1) | ||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||
| f"gh failed (attempt {attempt}/{attempts}), retrying in {delay:g}s: " | ||||||||||||||||||||||||||||
| f"{(r.stderr or '').strip()[:200]}", | ||||||||||||||||||||||||||||
| file=sys.stderr, | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| time.sleep(delay) | ||||||||||||||||||||||||||||
| raise AssertionError("unreachable: run_gh returns or raises on every attempt") | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def main() -> None: | ||||||||||||||||||||||||||||
| os.makedirs(ROOT, exist_ok=True) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| print(f"Fetching issue list from {REPO} (state=all, limit 1000) ...") | ||||||||||||||||||||||||||||
| r = run_gh( | ||||||||||||||||||||||||||||
| [ | ||||||||||||||||||||||||||||
| "gh", | ||||||||||||||||||||||||||||
| "issue", | ||||||||||||||||||||||||||||
| "list", | ||||||||||||||||||||||||||||
| "--repo", | ||||||||||||||||||||||||||||
| REPO, | ||||||||||||||||||||||||||||
| "--state", | ||||||||||||||||||||||||||||
| "all", | ||||||||||||||||||||||||||||
| "--limit", | ||||||||||||||||||||||||||||
| "1000", | ||||||||||||||||||||||||||||
| "--json", | ||||||||||||||||||||||||||||
| # `updatedAt` is consumed by fetch_events.py, which refuses to trust a cached | ||||||||||||||||||||||||||||
| # events file older than the issue's last update. Dropping it here silently | ||||||||||||||||||||||||||||
| # disables that guard, because the check falls back to "no timestamp -> trust | ||||||||||||||||||||||||||||
| # the cache" and every relabelled issue keeps its stale event history. | ||||||||||||||||||||||||||||
| # `body` and `closedByPullRequestsReferences` ride on this one paginated call | ||||||||||||||||||||||||||||
| # so fetch_bodies.py can build issue_extra.json without a `gh issue view` | ||||||||||||||||||||||||||||
| # per issue. | ||||||||||||||||||||||||||||
| "number,title,state,stateReason,createdAt,closedAt,updatedAt,labels,comments,body,closedByPullRequestsReferences", | ||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| issues = json.loads(r.stdout) | ||||||||||||||||||||||||||||
| with open(f"{ROOT}/issues.json", "w") as f: | ||||||||||||||||||||||||||||
| json.dump(issues, f) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| print(f"Wrote {len(issues)} issues to {ROOT}/issues.json") | ||||||||||||||||||||||||||||
| if len(issues) >= 1000: | ||||||||||||||||||||||||||||
| print( | ||||||||||||||||||||||||||||
| "WARNING: the issue list hit the 1000-issue cap; older issues are missing " | ||||||||||||||||||||||||||||
| "and every count in the dashboard is a floor. Raise --limit in fetch_issues.py." | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if __name__ == "__main__": | ||||||||||||||||||||||||||||
| main() | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,8 @@ | |
| # Usage: run.sh [output-path] | ||
| # | ||
| # Env overrides: | ||
| # TRACKER_STATS_CACHE (default: /tmp/tracker-stats-cache) | ||
| # TRACKER_STATS_OUT (default: /tmp/airflow_s_monthly.html - or arg $1) | ||
| # TRACKER_STATS_CACHE (default: $TMPDIR/tracker-stats-cache) | ||
| # TRACKER_STATS_OUT (default: $TMPDIR/airflow_s_monthly.html - or arg $1) | ||
| # TRACKER_STATS_REPO tracker repo (default: airflow-s/airflow-s) | ||
| # TRACKER_STATS_BUCKETS monthly | quarterly (overlay) | ||
| # TRACKER_STATS_START "YYYY-MM" / "YYYY-Qn" (overlay) | ||
|
|
@@ -55,4 +55,4 @@ echo "-> fetch_prs" | |
| echo "-> render" | ||
| "${PY_CMD[@]}" "$HERE/render.py" | ||
|
|
||
| echo "done: ${TRACKER_STATS_OUT:-/tmp/airflow_s_monthly.html}" | ||
| echo "done: ${TRACKER_STATS_OUT:-${TMPDIR:-/tmp}/airflow_s_monthly.html}" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small consistency nit: Not a blocker on POSIX, but worth keeping in mind or letting |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because prose in Step 2 was modified here, the frontmatter
surface_hashandmeasured_tokens(lines 19–21) are now out of date.Running
prek run --all-files(orpython tools/dev/check-measured-tokens.py --fix) will recalculate and update these stamps so the CIskill-surface-hashhook passes.