From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?Q?Ludovic_Court=C3=A8s?= Subject: Re: Potential for a race condition with latest-repository-commit Date: Wed, 18 Mar 2020 17:19:05 +0100 Message-ID: <871rpp8wli.fsf@gnu.org> References: <87blphw0l1.fsf@cbaines.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:55981) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jEbPR-0002mq-2e for guix-devel@gnu.org; Wed, 18 Mar 2020 12:19:10 -0400 In-Reply-To: <87blphw0l1.fsf@cbaines.net> (Christopher Baines's message of "Sat, 29 Feb 2020 21:22:18 +0000") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane-mx.org@gnu.org Sender: "Guix-devel" To: Christopher Baines Cc: guix-devel@gnu.org Hi, Christopher Baines skribis: > So one of the things that's currently restricted to doing by one job at > a time in the Guix Data Service is running latest-repository-commit from > the (guix git) module. > > Previously this was more of a problem for the Guix Data Service, as a > large section of the work for loading information about a revision was > serialised to avoid the potential for contention over the cached > checkout this procedure uses. There's still a lock used now, but I > realised when looking in to this further that it's only necessary to > lock around this specific call, not the larger section that was > restricted previously. > > I think that add-to-store which this procedure uses isn't atomic for a > directory, so there's a risk of weird results if the repository is > changed after the required revision is checked out. While it isn't > common to run guix pull twice, I think this could happen there if you > ran guix pull concurrently for the same repository, but two different > profiles. I added a sleep call just prior to the add-to-store call in > latest-repository-commit, and tested running guix pull twice at roughly > the same time, with different branches and profiles, and I did see both > profiles then reflecting a single branch, so one profile was mistakenly > referring to the wrong branch. Yes, we=E2=80=99ve also seen this problem with =E2=80=98static-web-site-ser= vice=E2=80=99=C2=B9, whereby if several instances would try to pull from, say, guix-artwork.git, we=E2=80=99d get non-deterministic results. We worked ar= ound it by using different cache directories=E2=80=A6 =C2=B9 https://git.savannah.gnu.org/cgit/guix/maintenance.git/tree/hydra/mo= dules/sysadmin/web.scm > Is this something that is worth guarding against? Maybe > latest-repository-commit could double check the Git repo state after > add-to-store completes, and raise an error if it's different to what it > expects. Or perhaps individual worktrees could be used for each process, > which would hopefully avoid the race condition entirely. It think it=E2=80=99d be worth guarding against it, yes. What we could do = is lock the cache directory, with something like =E2=80=98with-file-lock=E2=80= =99. WDYT? Thanks, Ludo=E2=80=99.