Skip to content

Commit 10e2f53

Browse files
authored
[CRCR] Add SHA validator for nightly/periodic callbacks (Phase 1.2) (#8304)
## Summary Phase 1.2 of the CRCR nightly/periodic self-report implementation ([RFC 98](pytorch/rfcs#98)). Adds `utils/sha_validator.py` — validates that a self-reported commit SHA exists on `pytorch/pytorch` via the GitHub API before accepting nightly/periodic callback results. **Key features:** - `validate_sha(upstream_repo, sha)` → `True` if SHA exists, `False` on 404, raises on other errors - Module-level TTL cache (1 hour) avoids redundant API calls when multiple downstream repos report against the same nightly SHA - Expired entries are evicted lazily on each call **Depends on:** Phase 1 (#8302) — will be called from `_handle_nightly_callback()` **Changes:** | File | Change | |------|--------| | `utils/sha_validator.py` | New module: SHA validation with TTL cache | | `tests/test_sha_validator.py` | 7 tests: valid/invalid SHA, cache hit/miss, TTL expiry, error propagation | ## Test plan - [x] 7 unit tests covering all paths - [ ] CI passes
1 parent c0b0155 commit 10e2f53

2 files changed

Lines changed: 250 additions & 0 deletions

File tree

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
import time
2+
import unittest
3+
from unittest.mock import MagicMock, patch
4+
5+
import github
6+
from utils.sha_validator import _CacheEntry, _REPO_CACHE, _SHA_CACHE, validate_sha
7+
8+
9+
class TestShaValidator(unittest.TestCase):
10+
def setUp(self):
11+
_SHA_CACHE.clear()
12+
_REPO_CACHE.clear()
13+
14+
def tearDown(self):
15+
_SHA_CACHE.clear()
16+
_REPO_CACHE.clear()
17+
18+
def test_valid_sha_returns_true(self):
19+
mock_gh = MagicMock()
20+
mock_gh.get_repo.return_value.get_commit.return_value = MagicMock()
21+
22+
result = validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
23+
24+
self.assertTrue(result)
25+
mock_gh.get_repo.assert_called_once_with("pytorch/pytorch")
26+
27+
def test_invalid_sha_returns_false(self):
28+
mock_gh = MagicMock()
29+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
30+
404, {"message": "Not Found"}, None
31+
)
32+
33+
result = validate_sha("pytorch/pytorch", "bad_sha", gh_client=mock_gh)
34+
35+
self.assertFalse(result)
36+
37+
def test_cache_hit_skips_api_call(self):
38+
mock_gh = MagicMock()
39+
mock_gh.get_repo.return_value.get_commit.return_value = MagicMock()
40+
41+
validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
42+
self.assertEqual(mock_gh.get_repo.call_count, 1)
43+
44+
validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
45+
# repo handle is cached, so get_repo is only called once
46+
self.assertEqual(mock_gh.get_repo.call_count, 1)
47+
48+
def test_different_sha_not_cached(self):
49+
mock_gh = MagicMock()
50+
mock_repo = MagicMock()
51+
mock_gh.get_repo.return_value = mock_repo
52+
53+
validate_sha("pytorch/pytorch", "sha1", gh_client=mock_gh)
54+
validate_sha("pytorch/pytorch", "sha2", gh_client=mock_gh)
55+
56+
# repo handle cached, but get_commit called twice (different SHAs)
57+
self.assertEqual(mock_repo.get_commit.call_count, 2)
58+
59+
@patch("utils.sha_validator.time")
60+
def test_expired_cache_entry_evicted(self, mock_time):
61+
mock_time.monotonic.return_value = 1000.0
62+
_SHA_CACHE["pytorch/pytorch:old_sha"] = _CacheEntry(
63+
exists=True, timestamp=1000.0
64+
)
65+
66+
mock_time.monotonic.return_value = 1000.0 + 3601
67+
mock_gh = MagicMock()
68+
mock_gh.get_repo.return_value.get_commit.return_value = MagicMock()
69+
70+
validate_sha("pytorch/pytorch", "new_sha", gh_client=mock_gh)
71+
72+
self.assertNotIn("pytorch/pytorch:old_sha", _SHA_CACHE)
73+
74+
def test_transient_error_fails_open(self):
75+
mock_gh = MagicMock()
76+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
77+
500, {"message": "Server Error"}, None
78+
)
79+
80+
result = validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
81+
82+
self.assertTrue(result)
83+
84+
def test_rate_limit_403_fails_open(self):
85+
mock_gh = MagicMock()
86+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
87+
403, {"message": "rate limit exceeded"}, None
88+
)
89+
90+
result = validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
91+
92+
self.assertTrue(result)
93+
94+
def test_negative_cache_hit_returns_false(self):
95+
mock_gh = MagicMock()
96+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
97+
404, {"message": "Not Found"}, None
98+
)
99+
100+
validate_sha("pytorch/pytorch", "bad_sha", gh_client=mock_gh)
101+
self.assertIn("pytorch/pytorch:bad_sha", _SHA_CACHE)
102+
self.assertFalse(_SHA_CACHE["pytorch/pytorch:bad_sha"].exists)
103+
104+
# Second call hits the negative cache — no additional API call
105+
mock_gh.get_repo.return_value.get_commit.reset_mock()
106+
result = validate_sha("pytorch/pytorch", "bad_sha", gh_client=mock_gh)
107+
self.assertFalse(result)
108+
mock_gh.get_repo.return_value.get_commit.assert_not_called()
109+
110+
@patch("utils.sha_validator.time")
111+
def test_negative_cache_expires_sooner(self, mock_time):
112+
mock_time.monotonic.return_value = 1000.0
113+
_SHA_CACHE["pytorch/pytorch:bad_sha"] = _CacheEntry(
114+
exists=False, timestamp=1000.0
115+
)
116+
117+
# After 301 seconds (> 300s negative TTL), entry should be evicted
118+
mock_time.monotonic.return_value = 1000.0 + 301
119+
mock_gh = MagicMock()
120+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
121+
404, {"message": "Not Found"}, None
122+
)
123+
124+
result = validate_sha("pytorch/pytorch", "bad_sha", gh_client=mock_gh)
125+
self.assertFalse(result)
126+
# API was called again because cache was evicted
127+
mock_gh.get_repo.return_value.get_commit.assert_called_once()
128+
129+
def test_cached_valid_sha_short_circuits_even_if_api_would_404(self):
130+
"""A previously cached valid SHA returns True without hitting the API,
131+
even if the API would now return 404."""
132+
_SHA_CACHE["pytorch/pytorch:abc123"] = _CacheEntry(
133+
exists=True, timestamp=time.monotonic()
134+
)
135+
136+
mock_gh = MagicMock()
137+
mock_gh.get_repo.return_value.get_commit.side_effect = github.GithubException(
138+
404, {"message": "Not Found"}, None
139+
)
140+
141+
result = validate_sha("pytorch/pytorch", "abc123", gh_client=mock_gh)
142+
143+
self.assertTrue(result)
144+
mock_gh.get_repo.return_value.get_commit.assert_not_called()
145+
146+
def test_repo_handle_cached(self):
147+
mock_gh = MagicMock()
148+
mock_repo = MagicMock()
149+
mock_gh.get_repo.return_value = mock_repo
150+
151+
validate_sha("pytorch/pytorch", "sha1", gh_client=mock_gh)
152+
validate_sha("pytorch/pytorch", "sha2", gh_client=mock_gh)
153+
154+
# get_repo only called once due to repo handle caching
155+
mock_gh.get_repo.assert_called_once_with("pytorch/pytorch")
156+
157+
158+
if __name__ == "__main__":
159+
unittest.main()
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
"""Validate that a commit SHA exists on pytorch/pytorch via GitHub API.
2+
3+
Used by the nightly/periodic callback path to verify that the self-reported
4+
dispatch_id (commit SHA) is real before accepting the result. A TTL cache
5+
avoids redundant API calls when multiple downstream repos report against the
6+
same SHA.
7+
"""
8+
9+
from __future__ import annotations
10+
11+
import logging
12+
import time
13+
from dataclasses import dataclass
14+
15+
import github
16+
17+
18+
logger = logging.getLogger(__name__)
19+
20+
_CACHE_TTL_SECONDS = 3600 # 1 hour
21+
_NEGATIVE_CACHE_TTL_SECONDS = 300 # 5 minutes for 404s
22+
23+
24+
@dataclass
25+
class _CacheEntry:
26+
exists: bool
27+
timestamp: float
28+
29+
30+
_SHA_CACHE: dict[str, _CacheEntry] = {}
31+
_REPO_CACHE: dict[str, object] = {}
32+
33+
34+
def _evict_expired() -> None:
35+
now = time.monotonic()
36+
expired = []
37+
for k, entry in _SHA_CACHE.items():
38+
ttl = _CACHE_TTL_SECONDS if entry.exists else _NEGATIVE_CACHE_TTL_SECONDS
39+
if now - entry.timestamp > ttl:
40+
expired.append(k)
41+
for k in expired:
42+
del _SHA_CACHE[k]
43+
44+
45+
def _get_repo(gh_client: github.Github, upstream_repo: str):
46+
"""Return a cached repo handle to avoid redundant API calls."""
47+
if upstream_repo not in _REPO_CACHE:
48+
_REPO_CACHE[upstream_repo] = gh_client.get_repo(upstream_repo)
49+
return _REPO_CACHE[upstream_repo]
50+
51+
52+
def validate_sha(
53+
upstream_repo: str,
54+
sha: str,
55+
gh_client: github.Github,
56+
) -> bool:
57+
"""Return True if ``sha`` exists on ``upstream_repo``, False otherwise.
58+
59+
Results are cached: valid SHAs for 1 hour, invalid (404) SHAs for 5 minutes.
60+
The repo handle is also cached to halve API calls per cache miss.
61+
62+
Transient API errors (500, 403 rate-limit) fail open — since nightly results
63+
are informational, a GitHub outage should not reject valid callbacks.
64+
"""
65+
_evict_expired()
66+
67+
cache_key = f"{upstream_repo}:{sha}"
68+
if cache_key in _SHA_CACHE:
69+
return _SHA_CACHE[cache_key].exists
70+
71+
try:
72+
repo = _get_repo(gh_client, upstream_repo)
73+
repo.get_commit(sha)
74+
_SHA_CACHE[cache_key] = _CacheEntry(exists=True, timestamp=time.monotonic())
75+
return True
76+
except github.GithubException as exc:
77+
if exc.status == 404:
78+
logger.warning("SHA %s does not exist on %s", sha, upstream_repo)
79+
_SHA_CACHE[cache_key] = _CacheEntry(
80+
exists=False, timestamp=time.monotonic()
81+
)
82+
return False
83+
# Transient errors (500, 403 rate-limit, etc.) — fail open since
84+
# nightly is informational and should not be blocked by GitHub outages.
85+
logger.warning(
86+
"GitHub API error (%d) validating SHA %s on %s, failing open",
87+
exc.status,
88+
sha,
89+
upstream_repo,
90+
)
91+
return True

0 commit comments

Comments
 (0)