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.

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

Pidlock source and artifact installation #4168

Merged

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Feb 23, 2025

Fixes #3495
Fixes #4174
Also fixes registry installation which wasn't checking after acquiring the pidlock which registries still needed installing.
Makes progress on #2219

But only if all concurrent julia process installations have this fix.. I'm not sure there's a way to protect against older versions..

Source:

  • Pidlocks the source path during check/download/installation
  • Protects against an interrupted mv from temp dir to final path by checking tree_hash before and after and deleting the destination if bad.

Artifacts:

  • Pidlocks artifact downloads and updates the progress meter to indicate another process is doing the downloading

Registries:

  • Checks after acquiring the lock whether it still needs installing
Screen.Recording.2025-02-25.at.5.20.45.PM.mov

Todo:

  • Disable test debug prints
  • Decide how much testing to keep - One test with 3 concurrent processes

@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch 2 times, most recently from 274d247 to 3db4eb8 Compare February 23, 2025 04:17
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch 7 times, most recently from bd55f9d to 7c0087b Compare February 25, 2025 16:48
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from 4f4f6a8 to a7acf16 Compare February 25, 2025 17:58
@IanButterworth

This comment was marked as resolved.

@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from adada98 to f03a16b Compare February 25, 2025 18:55
@IanButterworth IanButterworth changed the title Make concurrent-process package source installation safe Make concurrent-process package source & artifact installation safe Feb 25, 2025
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch 2 times, most recently from aa0f530 to 97c3813 Compare February 25, 2025 21:59
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from 97c3813 to 4c58479 Compare February 25, 2025 22:14
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from ef18a94 to 6fb66bd Compare February 25, 2025 22:39
@IanButterworth IanButterworth changed the title Make concurrent-process package source & artifact installation safe Make concurrent-process package source & artifact installation efficient & safe Feb 25, 2025
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from b4b16f8 to d838b3d Compare February 25, 2025 22:53
@IanButterworth
Copy link
Member Author

These tests add between 1m40s and 4m (linux git registry).

I'm thinking of merging this and trying out in a Pkg bump PR to see the impact on Julia CI. Adjusting as necessary.

@IanButterworth
Copy link
Member Author

IanButterworth commented Feb 28, 2025

I saw the same hang locally on macOS when testing two concurrent processes, so I don't think the test failures are just CI peculiarities.

https://github.com/user-attachments/assets/8764844b-cfd2-4299-9b0e-cbb0056055db

Update: Fixed. I had a design flaw not pushing the expected number of elements to a channel.
It's not clear to me exactly why it wasn't a problem on some platforms.. I guess some less powerful CI machines had processes that just didn't advance enough and avoided the logic.. Anyway it seems to have fixed it.

@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from 3b0e9e3 to 09244b4 Compare February 28, 2025 17:01
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from f9b320c to 95a3f04 Compare February 28, 2025 17:57
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch 2 times, most recently from ff78e29 to 8f2fb06 Compare February 28, 2025 20:36
@IanButterworth IanButterworth force-pushed the ib/pidlock_download_source branch from 8f2fb06 to a8f43cc Compare February 28, 2025 20:39
@IanButterworth IanButterworth removed the request for review from staticfloat February 28, 2025 22:22
@IanButterworth
Copy link
Member Author

@staticfloat this should be good to go now

@IanButterworth IanButterworth dismissed staticfloat’s stale review March 1, 2025 17:31

I want to test this on Julia CI so I'm going to dismiss these requested changes (which have been addressed).

Post merge review very much welcome!

@IanButterworth IanButterworth merged commit cf6191b into JuliaLang:master Mar 1, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants