fix: strip mirror file contents in MinerEvaluationCache - #824
Conversation
7cd1d27 to
2989ad6
Compare
|
@anderdc Please review this PR. I'd appreciate any feedbacks. Thanks |
anderdc
left a comment
There was a problem hiding this comment.
Scope this down to the actual leak. The file-strip change in _build_cache_entry plus a _scored_mirror_pr_for_cache helper that nulls files is all #823 asks for.
Drop _scored_mirror_pr_with_fresh_pr, _mirror_pr_with_fresh_metadata, _mirror_linked_issue_with_fresh_labels, the _isolate_for_downstream mirror branch, and the two metadata-isolation tests (test_store_isolates_cached_mirror_metadata_from_source_mutations, test_get_returns_isolated_mirror_metadata) — those guard against mutations no caller on the cache-fallback path actually makes. The legacy _pr_with_fresh_issues deliberately only deep-copies issues for the same reason; see the comment at classes.py:723-725 ("PR metadata is read-only on the cache-fallback path, so we can share it"). The mirror path feeds the same downstream (issue competitions + issue discovery), so the same logic applies.
Keep test_cache_drops_mirror_files_without_mutating_source — that one matches the actual issue invariant.
If aliasing on metadata ever becomes a real concern, land it in its own PR with the offending caller cited.
4334a38 to
2dd3fad
Compare
|
@anderdc pushed the update. plz review again. |
Summary
MinerEvaluationCachegot shallow-copy helpers in #786, but they only cover the legacyPullRequestlists. The three mirror lists (mirror_merged_prs,mirror_open_prs,mirror_closed_prs) still get shared by reference, which causes two real problems:ScoredMirrorPR.files[*].head_contentandbase_content(whole head/base file blobs). Cache memory grows with every mirror-scored PR and never shrinks ([Bug]:MinerEvaluationCachedoesn't strip mirror file contents — unbounded memory growth #823).store()leak into the cache, and back-to-backget()calls hand back the sameScoredMirrorPR/MirrorPullRequest/MirrorLinkedIssue/MirrorLabelobjects.This PR extends the same shallow-copy strategy from #786 to mirror PRs, via four small private helpers that mirror the existing
_pr_for_cache/_pr_with_fresh_issuespair:_build_cache_entry: dropScoredMirrorPR.filesand shallow-copy the nestedMirrorPullRequest(review_summary,labels,linked_issues+ each linked issue'slabels)._isolate_for_downstream: same nested-metadata copy on the way out, so successiveget()calls return fully independent snapshots.The cache-fallback path only runs issue competitions and issue discovery scoring, neither of which reads file contents — so dropping
filesoutright is enough, no need to keep strippedMirrorFileplaceholders.Related Issues
Closes #823
Type of Change
Testing
Three new tests in
tests/validator/test_validator_cache_fallback.py::TestMirrorCacheIsolation, one per invariant:test_cache_drops_mirror_files_without_mutating_source—filesisNoneon the cached copy across all three mirror buckets, and the source eval is untouched.test_store_isolates_cached_mirror_metadata_from_source_mutations— mutatingsource.mirror_merged_prs[i].pr.{title, review_summary, labels, linked_issues}afterstore()does not affect the cached copy.test_get_returns_isolated_mirror_metadata— two consecutiveget()calls return independent snapshots.Checklist