-
Notifications
You must be signed in to change notification settings - Fork 1
Repo audit: dead code, test guards, and the two ways data left the machine #146
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
b75543c
407c039
eb9ab18
f89ab3d
fb8c1c5
dc25606
51c2024
8db7ef1
db781ce
ba98bea
38f7da6
7d366e4
2e3e185
9070dce
6812918
7319d0f
37a4dee
1268c17
090ea51
d0eac76
bcd7a06
ec600a3
46d0170
733275e
d853af8
f22fff9
3189645
892b6f5
f78482a
c8d5206
b7fa019
7fd5815
a2cb0eb
fa21ee8
6ff0ecf
5434807
49e417d
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 |
|---|---|---|
|
|
@@ -393,10 +393,9 @@ def _manifest_cache_path(abs_root: Path, content_signature: str) -> Path: | |
|
|
||
|
|
||
| def _ref_manifest_cache_path(abs_root: Path, ref_sha: str) -> Path: | ||
| # `__ref-` (not `__`) so cache_clear_manifests's `{repo_key}__*.json.gz` | ||
| # glob still sweeps these alongside content-signature entries, while the | ||
| # prefix keeps a ref-sha visually distinct from a content signature in | ||
| # directory listings. | ||
| # `__ref-` prefix keeps a ref-sha visually distinct from a content | ||
| # signature in directory listings, while staying inside the | ||
| # `{repo_key}__*.json.gz` shape the other per-root globs match. | ||
| return CACHE_ROOT / "manifests" / f"{repo_key(abs_root)}__ref-{ref_sha}.json.gz" | ||
|
|
||
|
|
||
|
|
@@ -462,6 +461,83 @@ def _save_gz_manifest(path: Path, manifest: "Manifest") -> None: | |
| ) | ||
|
|
||
|
|
||
| # Entries are keyed by repo CONTENT, so without a cap this directory grows for | ||
| # the life of the install. Sized by how far back a re-read is plausible; these | ||
| # are pure performance caches, so evicting costs a rescan, never correctness. | ||
| _KEEP_CONTENT_MANIFESTS = 5 | ||
| _KEEP_REF_MANIFESTS = 20 | ||
| _KEEP_TIMELINE_BUNDLES = 3 | ||
|
|
||
|
|
||
| def _entry_family(name_rest: str) -> str: | ||
| """Which retention pool a `{repo_key}__<rest>` cache file belongs to.""" | ||
| if name_rest.startswith("ref-"): | ||
| return "ref" | ||
| if name_rest.startswith("timeline-"): | ||
| return "timeline" | ||
| return "content" | ||
|
|
||
|
|
||
| _FAMILY_KEEP = { | ||
| "content": _KEEP_CONTENT_MANIFESTS, | ||
| "ref": _KEEP_REF_MANIFESTS, | ||
| "timeline": _KEEP_TIMELINE_BUNDLES, | ||
|
Owner
Author
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. enum, that's also ideally used here. |
||
| } | ||
|
|
||
|
|
||
| def prune_manifest_cache(abs_root: Path, *, protect: Path | None = None) -> int: | ||
| """Drop this root's oldest manifest-dir entries, per family. Returns the | ||
| count deleted. | ||
|
|
||
| `protect` is never evicted. Callers pass the path they just wrote: mtime | ||
| resolution is only one second on some filesystems, so a burst of saves can | ||
| tie and sort the newest entry into the tail — which would delete the very | ||
| manifest the caller is about to read back. | ||
|
|
||
| Ordering is by mtime rather than access time, which macOS and most Linux | ||
| mounts do not track (relatime), so this is write-recency, not LRU: an old | ||
| signature that keeps being re-read still ages out. That is the right trade | ||
| here, since re-reading it only ever saved a rescan. | ||
|
|
||
| Best-effort, like the rest of this module: a failed unlink (or a file a | ||
| concurrent request removed first) must never break the response.""" | ||
|
Owner
Author
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. make this more concise |
||
| manifests_dir = CACHE_ROOT / "manifests" | ||
| if not manifests_dir.exists(): | ||
| return 0 | ||
|
|
||
| prefix = f"{repo_key(abs_root)}__" | ||
|
Owner
Author
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. if this is the cache prefix, this should be a general shared constant, so that it doesn't become out of date w/ the the cache naming. |
||
| families: dict[str, list[tuple[float, Path]]] = { | ||
| "content": [], | ||
| "ref": [], | ||
| "timeline": [], | ||
| } | ||
| for path in manifests_dir.glob(f"{prefix}*.json.gz"): | ||
|
Owner
Author
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. same thing w/ the file ending, there could be a shared helper function with shared constnats. |
||
| if protect is not None and path == protect: | ||
| continue # counts against nothing; it always stays | ||
| try: | ||
| mtime = path.stat().st_mtime | ||
| except OSError: | ||
| continue # vanished under us; nothing to prune | ||
| families[_entry_family(path.name[len(prefix) :])].append((mtime, path)) | ||
|
|
||
| deleted = 0 | ||
| for family, entries in families.items(): | ||
| # The protected entry is out of `entries`, so leave room for it. | ||
| keep = _FAMILY_KEEP[family] | ||
| if protect is not None and _entry_family(protect.name[len(prefix) :]) == family: | ||
| keep -= 1 | ||
| if len(entries) <= keep: | ||
| continue | ||
| entries.sort(key=lambda e: e[0], reverse=True) # newest first | ||
| for _, path in entries[keep:]: | ||
| try: | ||
| path.unlink() | ||
| deleted += 1 | ||
| except OSError: | ||
| pass | ||
| return deleted | ||
|
|
||
|
|
||
| def cache_load_manifest( | ||
| abs_root: Path, | ||
| content_signature: str, | ||
|
|
@@ -476,20 +552,23 @@ def cache_save_manifest( | |
| manifest: "Manifest", | ||
| ) -> None: | ||
| """Atomically write the manifest cache for this (root, content_signature).""" | ||
| _save_gz_manifest(_manifest_cache_path(abs_root, content_signature), manifest) | ||
| path = _manifest_cache_path(abs_root, content_signature) | ||
| _save_gz_manifest(path, manifest) | ||
| prune_manifest_cache(abs_root, protect=path) | ||
|
|
||
|
|
||
| def cache_load_ref_manifest(abs_root: Path, ref_sha: str) -> "Manifest | None": | ||
| """Load the cached manifest for this (root, ref_sha). A resolved commit | ||
| sha's manifest is immutable (the commit's content never changes), so | ||
| unlike the content-signature cache this key never needs invalidating — | ||
| only `cache_clear_manifests`/`cache_clear_all` remove it.""" | ||
| unlike the content-signature cache this key never needs invalidating.""" | ||
| return _load_gz_manifest(_ref_manifest_cache_path(abs_root, ref_sha)) | ||
|
|
||
|
|
||
| def cache_save_ref_manifest(abs_root: Path, ref_sha: str, manifest: "Manifest") -> None: | ||
| """Atomically write the ref-keyed manifest cache for (root, ref_sha).""" | ||
| _save_gz_manifest(_ref_manifest_cache_path(abs_root, ref_sha), manifest) | ||
| path = _ref_manifest_cache_path(abs_root, ref_sha) | ||
| _save_gz_manifest(path, manifest) | ||
| prune_manifest_cache(abs_root, protect=path) | ||
|
|
||
|
|
||
| def _excludes_key(excludes: frozenset[str]) -> str: | ||
|
|
@@ -531,12 +610,14 @@ def cache_save_timeline( | |
| excludes: frozenset[str] = frozenset(), | ||
| ) -> None: | ||
| """Atomically write the timeline bundle cache for (root, head_sha, excludes).""" | ||
| path = _timeline_cache_path(abs_root, head_sha, excludes) | ||
| _save_gz_envelope( | ||
| _timeline_cache_path(abs_root, head_sha, excludes), | ||
| path, | ||
| envelope_key="bundle", | ||
| version=_TIMELINE_CACHE_VERSION, | ||
| payload=cast("dict[str, object]", bundle), | ||
| ) | ||
| prune_manifest_cache(abs_root, protect=path) | ||
|
|
||
|
|
||
| def cache_clear_timeline(abs_root: Path) -> int: | ||
|
|
@@ -556,50 +637,3 @@ def cache_clear_timeline(abs_root: Path) -> int: | |
| except OSError: | ||
| pass | ||
| return count | ||
|
|
||
|
|
||
| def cache_clear_manifests(abs_root: Path) -> int: | ||
| """Delete every cached manifest file for this root, across all | ||
| signatures, every ref-keyed manifest, AND every timeline bundle (the | ||
| `__*.json.gz` glob below matches `__<signature>.json.gz`, | ||
| `__ref-<sha>.json.gz`, and `__timeline-<sha>.json.gz`). | ||
| Returns the count deleted. | ||
|
|
||
| Silently ignores I/O errors per the rest of this module's hygiene — | ||
| cache cleanup failures must never break the response.""" | ||
| manifests_dir = CACHE_ROOT / "manifests" | ||
| if not manifests_dir.exists(): | ||
| return 0 | ||
| pattern = f"{repo_key(abs_root)}__*.json.gz" | ||
| count = 0 | ||
| for path in manifests_dir.glob(pattern): | ||
| try: | ||
| path.unlink() | ||
| count += 1 | ||
| except OSError: | ||
| pass | ||
| return count | ||
|
|
||
|
|
||
| def cache_clear_all(abs_root: Path) -> int: | ||
| """Delete EVERY per-root cache for this root — manifest (all | ||
| signatures), file-stat, git-history, and blob-stats. Returns the count | ||
| deleted. | ||
|
|
||
| Backs the "clear cache" flow's clean-slate guarantee for a source. | ||
| The git clone working tree lives outside CACHE_ROOT, so the caller | ||
| removes it separately (see clone.remove_clone). Same swallow-errors | ||
| hygiene as the rest of this module — cleanup failures must never | ||
| break the response.""" | ||
| count = cache_clear_manifests(abs_root) | ||
| for path in ( | ||
| _file_cache_path(abs_root), | ||
| _git_history_cache_path(abs_root), | ||
| _blob_cache_path(abs_root), | ||
| ): | ||
| try: | ||
| path.unlink() | ||
| count += 1 | ||
| except OSError: | ||
| pass # missing file or I/O error — best-effort cleanup | ||
| return count | ||
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.
these should be a proper enum