Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean stale git temp files #11308

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Clean stale git temp files #11308

merged 1 commit into from
Nov 1, 2022

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 28, 2022

What does this PR try to resolve?

When cargo is interrupted while libgit2 is indexing the pack file, it will leave behind a temp file that never gets deleted. These files can be very large. This checks for these stale temp files and deletes them.

How should we test and review this PR?

There is a simulated test here. To test with the actual behavior:

  1. Run CARGO_HOME=chome cargo fetch
  2. While it is "resolving deltas", hit Ctrl-C.
  3. Notice that there is a 200MB file in chome/registry/index/git.luolix.top-1ecc6299db9ec823/.git/objects/pack/
  4. Do that several times if you want, each time adds another 200MB file.
  5. Build this PR: cargo b -r
  6. Run CARGO_HOME=chome CARGO_LOG=cargo::sources::git::utils=debug ./target/release/cargo fetch
  7. Notice that it deletes the pack_git2_* files.

@rustbot
Copy link
Collaborator

rustbot commented Oct 28, 2022

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 28, 2022
weihanglo
weihanglo approved these changes Oct 30, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This patch does its job well!

Have you played with libgit2's git_fetch_prune_t? It removes old temp files sometimes, but I have yet figured out when.

@ehuss
Copy link
Contributor Author

ehuss commented Oct 30, 2022

I'm not aware of the fetch pruning affecting stale temporary pack files. My understanding of the prune option is that it will remove refs from the remote that don't exist anymore (such as a deleted branch).

If you want to follow, it starts in git_remote_prune which generally collects the refs to delete, then calls refdb_fs_backend__delete to delete the ref from the refdb.

Temp files, for the purpose of downloading are constructed via git_futils_mktmp which uses a specific template pattern. The indexer creates the temp files in the pack directory. I don't see anything elsewhere that knows about git_futils_mktmp's filename patterns (specifically the _git2_ part).

If you find something to the contrary, please let me know.

@weihanglo
Copy link
Member

I cannot find either.

Anyway, this patch is good to merge. Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 31, 2022

📌 Commit 754de17 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2022
@bors
Copy link
Contributor

bors commented Oct 31, 2022

⌛ Testing commit 754de17 with merge 1a0d5e3...

bors added a commit that referenced this pull request Oct 31, 2022
@bors
Auto merge of #11308 - ehuss:clean-tmp-libgit2, r=weihanglo
1a0d5e3 
Clean stale git temp files

### What does this PR try to resolve?

When cargo is interrupted while libgit2 is indexing the pack file, it will leave behind a temp file that never gets deleted. These files can be very large. This checks for these stale temp files and deletes them.

### How should we test and review this PR?

There is a simulated test here. To test with the actual behavior:

1. Run `CARGO_HOME=chome cargo fetch`
2. While it is "resolving deltas", hit Ctrl-C.
3. Notice that there is a 200MB file in `chome/registry/index/git.luolix.top-1ecc6299db9ec823/.git/objects/pack/`
4. Do that several times if you want, each time adds another 200MB file.
5. Build this PR: `cargo b -r`
6. Run `CARGO_HOME=chome CARGO_LOG=cargo::sources::git::utils=debug ./target/release/cargo fetch`
7. Notice that it deletes the `pack_git2_*` files.
@bors
Copy link
Contributor

bors commented Oct 31, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 31, 2022
@weihanglo
Copy link
Member

CI has been fixed. REF

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 31, 2022
@bors
Copy link
Contributor

bors commented Oct 31, 2022

⌛ Testing commit 754de17 with merge 605a356...

bors added a commit that referenced this pull request Oct 31, 2022
@bors
Auto merge of #11308 - ehuss:clean-tmp-libgit2, r=weihanglo
605a356 
Clean stale git temp files

### What does this PR try to resolve?

When cargo is interrupted while libgit2 is indexing the pack file, it will leave behind a temp file that never gets deleted. These files can be very large. This checks for these stale temp files and deletes them.

### How should we test and review this PR?

There is a simulated test here. To test with the actual behavior:

1. Run `CARGO_HOME=chome cargo fetch`
2. While it is "resolving deltas", hit Ctrl-C.
3. Notice that there is a 200MB file in `chome/registry/index/git.luolix.top-1ecc6299db9ec823/.git/objects/pack/`
4. Do that several times if you want, each time adds another 200MB file.
5. Build this PR: `cargo b -r`
6. Run `CARGO_HOME=chome CARGO_LOG=cargo::sources::git::utils=debug ./target/release/cargo fetch`
7. Notice that it deletes the `pack_git2_*` files.
@bors
Copy link
Contributor

bors commented Oct 31, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 31, 2022
@weihanglo
Copy link
Member

CI should be fixed this time. REF

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2022
@bors
Copy link
Contributor

bors commented Nov 1, 2022

⌛ Testing commit 754de17 with merge 37cad5b...

@bors
Copy link
Contributor

bors commented Nov 1, 2022

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 37cad5b to master...

@bors bors merged commit 37cad5b into rust-lang:master Nov 1, 2022
@weihanglo weihanglo mentioned this pull request Nov 2, 2022
Merged
weihanglo added a commit to weihanglo/rust that referenced this pull request Nov 2, 2022
@weihanglo
Update cargo
331aa45 
14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03
2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 3, 2022
@bors
Auto merge of rust-lang#103860 - weihanglo:update-cargo, r=ehuss
432b1a4 
Update cargo

14 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..810cbad9a123ad4ee0a55a96171b8f8478ff1c03
2022-10-27 15:20:57 +0000 to 2022-11-02 21:04:31 +0000
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)

r? `@ghost`
@weihanglo weihanglo mentioned this pull request Nov 4, 2022
Merged
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2022
@weihanglo
Update cargo
bfce227 
20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b
2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000
- chore: Upgrade dependencies (rust-lang/cargo#11328)
- Clean more aggressively in CI (rust-lang/cargo#11335)
- Remove remove_dir_all (rust-lang/cargo#11333)
- test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327)
- Revert rust-lang/cargo#11183 (rust-lang/cargo#11331)
- fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332)
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 5, 2022
@bors
Auto merge of rust-lang#103992 - weihanglo:update-cargo, r=weihanglo
5b3e909 
Update cargo

20 commits in 7e484fc1a766f56dbc95380f45719698e0c82749..9286a1beba5b28b115bad67de2ae91fb1c61eb0b 2022-10-27 15:20:57 +0000 to 2022-11-04 06:41:49 +0000
- chore: Upgrade dependencies (rust-lang/cargo#11328)
- Clean more aggressively in CI (rust-lang/cargo#11335)
- Remove remove_dir_all (rust-lang/cargo#11333)
- test(publish): Cover more wait-for-publish cases (rust-lang/cargo#11327)
- Revert rust-lang/cargo#11183 (rust-lang/cargo#11331)
- fix(semver-check): adapt to a different error for variant not covered (rust-lang/cargo#11332)
- Update curl-sys (rust-lang/cargo#11326)
- Mention fix on build script deadlock (rust-lang/cargo#11325)
- Make cargo forward pre-existing CARGO if set (rust-lang/cargo#11285)
- Clean up workspace dependencies after cargo remove (rust-lang/cargo#11242)
- Update the outdated link for rust-semverver (rust-lang/cargo#11322)
- Fix broken link to compilation entry point (rust-lang/cargo#11317)
- Only remove fingerprints and build script artifacts of the requested package (rust-lang/cargo#10621)
- Newer anyhow features are required (rust-lang/cargo#11316)
- Clean stale git temp files (rust-lang/cargo#11308)
- Report crate size on package and publish (rust-lang/cargo#11270)
- add a note that some warnings (and/or errors) can be auto-fixed (rust-lang/cargo#10989)
- Update libcurl (rust-lang/cargo#11307)
- artifact deps shoud works when target field specified coexists with `optional = true` (rust-lang/cargo#11183)
- Fix singular verb in tests page (rust-lang/cargo#11300)

r? `@ghost`
@ehuss ehuss added this to the 1.67.0 milestone Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@weihanglo weihanglo

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ehuss @rustbot @weihanglo @bors