Archive backend: fetch repos as tarballs for remote drift runs#2570
Merged
Conversation
Seven remote source functions (read, read_optional, list_tree,
list_dir, list_dir_at_ref, ref_exists, read_at_ref) each repeated the
same GitHub-request skeleton: a requests.get with auth headers and a
30s timeout, a try/except wrapping RequestException into a SourceError,
and a "raise _http_error on non-ok" guard. The only per-site variation
was whether the REST Accept header is sent and which status codes are
treated as a valid outcome rather than a failure (a 404 meaning
"absent", the commits API's 404/422 meaning "ref does not resolve").
Collapse that skeleton into one helper:
_get(action, url, *, json_api=False, ok=())
json_api toggles the GitHub REST Accept header; ok lists the status
codes the caller handles itself, which _get returns without raising
(everything else non-ok still goes through _http_error and keeps the
rate-limit hint). Each caller now keeps only its genuine difference --
its own branch on the whitelisted 404/422 -- so the transport,
timeout, auth, and error wrapping live in a single place.
Behavior-preserving; no call site changes what it returns or raises.
All 281 tests pass.
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Roger Luethi <luethi@osism.tech>
A remote drift run reads every repo through the GitHub API: dozens of requests spread over several minutes, with no output between the source listing and the final report. To a user that looks like a hang, and the common reaction is an impatient Ctrl-C that aborts the run. Give the wait a shape. source.py gains an opt-in progress sink: when a sink is installed, each remote read prints one short line naming the kind of call (raw/tree/list/ref?), the repo and path, and the ref, with a running counter that doubles as a rough rate-limit gauge. The default sink is a no-op, so library and test callers stay silent; only the remote code paths call _note(), so local runs are unaffected. The driver installs the sink (to stderr) only when the run is not --quiet and at least one source actually resolves remote, and prints a one-line "this can take several minutes" heads-up first so the reason for the wait is stated before it begins. A fully local run prints neither the warning nor the per-call lines. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Roger Luethi <luethi@osism.tech>
The rate-limit hint added to remote reads only fires when the response carries the GitHub REST API markers (X-RateLimit-Remaining: 0 or a Retry-After header). Those come from api.github.com. The bulk of a remote drift run, however, fetches file bytes from raw.githubusercontent.com -- a Fastly CDN that throttles per-IP and returns 429 with none of those headers. So the read most likely to be throttled (over a thousand raw GETs in a full run) was the one that produced a bare "HTTP 429 fetching <url>" with no explanation, which is what a coworker reported. Treat a markerless 429 as CDN throttling and emit its own hint: the raw host is rate-limited per-IP, intermittently, and separately from the GitHub API budget, so a token does not draw it from the 5000/hr pool and is no guaranteed fix. The advice points instead at --base-dir (read from local checkouts and skip the remote fetch entirely), retrying later, or switching networks. A markerless 403 is left untouched: that is an ordinary auth/permission refusal, not throttling, and must stay a plain error -- only a markerless 429 flips to the new hint. The api.github.com path (header-bearing 403 or 429) keeps its existing quota/reset message unchanged. Verified end-to-end: read() against a header-less 429 now renders the raw hint, and a header-less 403 still renders a bare HTTP 403. Assisted-by: Claude:claude-opus-4-8 Signed-off-by: Roger Luethi <luethi@osism.tech>
Move SourceError, _GITHUB_HOSTS, _auth_headers, _rate_limit_hint, _http_error, _GH_JSON, and _get from source.py into a new leaf module http.py. Re-export all seven names from source.py so existing callers resolve source.SourceError, source._get, etc. without change. Add a timeout keyword parameter (default 30 s) to _get so the new archive backend can pass a longer timeout without touching existing callers. Full test suite (304 tests) passes unchanged. Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Roger Luethi <luethi@osism.tech>
Introduce _dir_read, _dir_read_optional, _dir_list_tree, and _dir_list
in source.py. These take a where context string used in error messages
so callers control the wording.
Refactor the unpinned-local branches of read, read_optional, list_tree,
and list_dir to call the matching helper, preserving the existing error
messages byte-for-byte (e.g. "not found in local {repo} ({d})").
The pinned-git branches (_git_show / _git_ls_tree) and list_dir_at_ref /
read_at_ref are not touched. Task 5 reuses these helpers for the archive
backend without duplication.
Add tests/kolla_drift/test_dir_helpers.py exercising all four helpers
against real tmp_path trees (no mocking).
Full suite (314 tests) passes unchanged.
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Roger Luethi <luethi@osism.tech>
Introduce src/osism_drift/archive.py, a leaf module that depends only on http.py. snapshot_dir(owner, repo, ref, config) fetches a GitHub tarball at the archive endpoint, extracts it to a tmp dir (cleaned up at exit), and returns the Path to the extracted tree. Results are memoized on config.snapshot_cache by (github_api, owner, slug, ref) so repeated reads within one run collapse to one HTTP request. _fetch_archive_bytes retries once on transport/server errors before re-raising. _safe_extract uses the data filter (Python 3.12+) or falls back to a strict manual check that rejects any member resolving outside the extraction root or with a link/special type. Tests in test_archive.py cover: slug/URL construction, happy-path extract+serve, memoization (one HTTP call for two reads), 404 raises, retry-then-success, double-failure raises, unsafe traversal member rejected, symlink rejected on the fallback path. Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Roger Luethi <luethi@osism.tech>
Add two fields to the Config dataclass: archive (bool, default False)
and snapshot_cache (dict, default {}). The driver sets archive=True in
_load_runtime so real runs use the archive backend by default; the
dataclass default False leaves all existing test Configs on the per-file
path with zero edits.
Add --use-raw-get to the driver's argument parser. Passing it keeps
archive=False (the old per-file path) as an escape hatch when the
archive backend misbehaves.
Do not add archive/snapshot_cache to load_config YAML parsing; they are
runtime/CLI state like base_dirs and ref_cache.
Test: load_config-built Config has archive=False and snapshot_cache={};
dataclasses.replace(cfg, archive=True) flips the flag correctly.
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Roger Luethi <luethi@osism.tech>
Wire the archive backend into all six remote read/list functions: read, read_optional, list_tree, list_dir, list_dir_at_ref, and read_at_ref. Each remote section calls _note first (preserving progress output for both paths), then checks config.archive: if True, fetches the tarball via archive.snapshot_dir and serves from the extracted tree; otherwise falls through to the existing per-file URL path (the --use-raw-get escape hatch). Because Config.archive defaults to False (Task 4), all existing remote tests continue to mock per-file raw/contents URLs and pass unchanged. The driver sets archive=True so real CLI runs use the archive backend. Add two integration tests to test_source.py with archive=True: one verifying a file read from the tarball, one verifying that two reads from the same (repo, ref) produce exactly one HTTP call (memoization). All 325 tests pass. Assisted-by: Claude:claude-sonnet-4-6 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
archive-snapshot
branch
from
7843476 to
e58ad99
Compare
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
3 participants
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.
What
Speeds up and stabilizes remote drift runs by fetching each repo once as a
tarball instead of per-file over the GitHub API, plus the transport-layer refactor
it builds on.
Archive backend
archive.py— fetches a(repo, ref)tarball once, memoizes it, and serves allreads/listings from the extracted tree. On by default;
--use-raw-getselects theold per-file raw-GET path.
repo — far less rate-limit exposure.
Transport refactor it builds on
_get— one shared helper for the repeated GitHub-request skeleton (auth, timeout,error wrapping, rate-limit hint).
http.py— the transport/error layer extracted out ofsource.py.raw.githubusercontent.com;shared dir-tree helpers for local branches.
Behavior-preserving for existing checks; 303 tests pass.
Merge order
This PR carries the shared transport layer (
_get,http.py, progress) that thedrift-tooling PR also currently contains. Merge this before drift-tooling; that PR
is then rebased to drop the two now-shared commits.
Related