feat(facade): check repo size and enforce clone limit before cloning (#458) - #459
Dipro-cyber wants to merge 5 commits into
Conversation
9ab30b8 to
b0cb8fe
Compare
|
im a little confused on the theory of how your safety margin system is meant to work. The original idea for that is because the size as github reports it is very likely not accurate to the actual size of the whole directory that you get after running I have other branches that are very work-in-progress for refactoring a lot of CollectOSS to use Happy to chat on slack about this too, im very curious what you find |
|
@MoralCode thank u for the context sir, sorry i was a bit inactive. the github api size field is from a bare repo so it can differ significantly from the actual clone size. happy to revisit the safety margin approach once the pygit2 refactor lands, since that library likely has a way to get the actual disk usage after clone. for now, should i simplify this to just use the raw github-reported size without a safety margin, and let users set the limit knowing it's an approximation? that would make the behavior more predictable while still solving the core disk space problem. |
|
I think the main idea is to figure out the disk space before the clone so we can avoid cloning if its too big. Maybe thats where the conversation in #49 could help (I.e. if we migrated CollectOSS to use bare clones) |
…haoss#458) Signed-off-by: Diptesh Roy <droy88333@gmail.com>
…ent E2E worker timeout Signed-off-by: Diptesh Roy <droy88333@gmail.com>
Per MoralCode's review feedback, the safety margin concept was confusing because the GitHub/GitLab API size field is already an approximation (measured from a bare repo, not an actual clone). Adding a multiplier on top of an already-inaccurate value made the limit unpredictable. Simplified to compare the raw reported size directly against max_clone_size_kb. Users set the limit knowing it's an approximation of the bare repo size, which is the most honest and predictable behavior. Also removes clone_size_safety_margin from config and FacadeHelper. Signed-off-by: Diptesh Roy <droy88333@gmail.com>
4af2829 to
4cc4d18
Compare
|
@MoralCode sir can u check this once, the new feature by @drkrillo has been a help here! |
|
The underlying issue has had additional notes added since this PR was filed. The additional notes Include a way to estimate the size of a checkout (rather than using a fudge factor). This, combined with the bare size of a git repo should give a decent estimate of repo size Could you update this PR to account for that more precise method of checking the size? |
Oh yes sure sir, I will check out the notes. |
Per MoralCode's research, the GitHub API 'size' field alone underestimates by ~4% because it only measures the bare repo. Adding the working tree file size (sum of all blob sizes from /git/trees/HEAD?recursive=1) gives a much more accurate estimate of actual on-disk clone size. If the tree is truncated (very large repo), falls back to bare size only. Updated tests to cover the combined estimation logic. Signed-off-by: Diptesh Roy <droy88333@gmail.com>
|
@MoralCode the new commit now uses the combined bare repo size + file tree blob sizes for github repos. falls back to bare size only if the tree is truncated. also updated tests to cover the combined logic. |
|
Wdym if the tree is truncated? |
sir the github git tree API (/git/trees/HEAD?recursive=1) returns truncated: true for very large repos where the tree has too many entries to return in a single response. in that case we can't sum all blob sizes, so we fall back to just the bare repo size from the metadata api. for most repos this won't happen, github truncates at ~100,000 tree entries. |
- Default max_clone_size_kb changed from 0 (disabled) to 5242880 KB (5 GB) as suggested by MoralCode — provides a sensible out-of-the-box limit - Reverted contributor_interface.py and tasks.py to upstream — those changes are unrelated to the repo size limit feature Signed-off-by: Diptesh Roy <droy88333@gmail.com>
| def test_is_valid_searchable_email(self): | ||
| from collectoss.tasks.github.facade_github.contributor_interfaceable.contributor_interface import is_valid_searchable_email | ||
| self.assertTrue(is_valid_searchable_email("user@example.org")) | ||
| self.assertTrue(is_valid_searchable_email("john.doe@company.co.uk")) | ||
|
|
||
| self.assertFalse(is_valid_searchable_email("root@augur")) | ||
| self.assertFalse(is_valid_searchable_email("michaelwoodruff@mwc-021001.dhcp.missouri.edu")) | ||
| self.assertFalse(is_valid_searchable_email("user@localhost")) | ||
| self.assertFalse(is_valid_searchable_email("invalid_email")) | ||
| self.assertFalse(is_valid_searchable_email("")) | ||
| self.assertFalse(is_valid_searchable_email(None)) |
2 participants
Description
Some git repositories are exceptionally large, which can rapidly exhaust local disk space when cloned. This PR introduces a configurable maximum repository clone size limit (
max_clone_size_kb) and safety margin (clone_size_safety_margin) in CollectOSS.Before running
git clone, CollectOSS queries platform APIs (GitHub REST API or GitLab API) to obtain reported repository size stats and computesestimated_size_kb = reported_size_kb * (1 + safety_margin). If the estimated clone size exceedsmax_clone_size_kb, cloning is skipped and the repository collection status is set toFailed Clone.This PR fixes #458
Notes for Reviewers
max_clone_size_kb(default0, disabled) andclone_size_safety_margin(default0.5, 50% margin) toFacadeconfig incollectoss/application/config.pyandFacadeHelper.check_repo_size_limit()and pre-clone enforcement logic incollectoss/tasks/git/util/facade_worker/facade_worker/repofetch.py.tests/test_tasks/test_git/test_repo_size_limit.py.Signed commits
Generative AI disclosure