[bug#66387] [PATCH] home: services: Fix race condition when detecting first login

[bug#66387] [PATCH] home: services: Fix race condition when detecting first login

[Carlo Zancanaro]

* gnu/home/services.scm (compute-on-first-login-script): Use open to
atomically check whether a file exists and create it if not.
---

I run Guix Home on NixOS with SDDM as my display manager. When I log in, I find that there are two shepherd processes running. Looking at the on-first-login script I noticed a race condition that I suspect was causing the issue.

I believe the "open" incantation I have used is atomic, except on NFS, so this should ensure that the gexps are only run once. I have confirmed that this patch fixes my problem. With this patch, when I login I only have a single shepherd process.

 gnu/home/services.scm | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 8d53f2f4d3..95ef66e091 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2021-2023 Andrew Tropin <andrew@trop.in>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;; Copyright © 2022-2023 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2023 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ gexps)
      #~(begin
          (use-modules (guix i18n)
                       (guix diagnostics))
+
+       (define (claim-first-run file-name)
+         (catch #t
+           (lambda ()
+             ;; This incantation will raise an error if the file at
+             ;; flag-file-path already exists, and will create it otherwise.
+             (close (open file-name (logior O_CREAT O_EXCL)))
+             #t)
+           (lambda (e)
+             #f)))
+
        #$%initialize-gettext
 
        (let* ((xdg-runtime-dir (or (getenv "XDG_RUNTIME_DIR")
                                    (format #f "/run/user/~a" (getuid))))
               (flag-file-path (string-append
-                               xdg-runtime-dir "/on-first-login-executed"))
-              (touch (lambda (file-name)
-                       (call-with-output-file file-name (const #t)))))
+                               xdg-runtime-dir "/on-first-login-executed")))
          ;; XDG_RUNTIME_DIR dissapears on logout, that means such trick
          ;; allows to launch on-first-login script on first login only
          ;; after complete logout/reboot.
          (if (file-exists? xdg-runtime-dir)
-             (unless (file-exists? flag-file-path)
-               (begin #$@gexps (touch flag-file-path)))
+             (when (claim-first-run flag-file-path)
+               #$@gexps)
              ;; TRANSLATORS: 'on-first-login' is the name of a service and
              ;; shouldn't be translated
              (warning (G_ "XDG_RUNTIME_DIR doesn't exists, on-first-login script

base-commit: b566e1a98a74d84d3978cffefd05295602c9445d
-- 
2.41.0

[bug#66387] [PATCH] home: services: Fix race condition when detecting first login

[Carlo Zancanaro]

On Sat, Oct 07 2023, Carlo Zancanaro wrote:
> @@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ 
> gexps)
>       #~(begin
>           (use-modules (guix i18n)
>                        (guix diagnostics))
> +
> +       (define (claim-first-run file-name)
> +         (catch #t
> +           (lambda ()
> +             ;; This incantation will raise an error if the 
> file at
> +             ;; flag-file-path already exists, and will create 
> it otherwise.
> +             (close (open file-name (logior O_CREAT O_EXCL)))
> +             #t)
> +           (lambda (e)

Whoops, just noticed this should be _, not (e). It still works, 
because it crashes the script which stops the gexps from running, 
but writes a message to the console. I'll send an updated patch 
tomorrow.

[bug#66387] [PATCH] home: services: Fix race condition when detecting first login

[Carlo Zancanaro]

* gnu/home/services.scm (compute-on-first-login-script): Use open to
atomically check whether a file exists and create it if not.
---
 gnu/home/services.scm | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 8d53f2f4d3..7137925b30 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2021-2023 Andrew Tropin <andrew@trop.in>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;; Copyright © 2022-2023 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2023 Carlo Zancanaro <carlo@zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ gexps)
      #~(begin
          (use-modules (guix i18n)
                       (guix diagnostics))
+
+       (define (claim-first-run file-name)
+         (catch #t
+           (lambda ()
+             ;; This incantation will raise an error if the file at
+             ;; flag-file-path already exists, and will create it otherwise.
+             (close (open file-name (logior O_CREAT O_EXCL)))
+             #t)
+           (lambda _
+             #f)))
+
        #$%initialize-gettext
 
        (let* ((xdg-runtime-dir (or (getenv "XDG_RUNTIME_DIR")
                                    (format #f "/run/user/~a" (getuid))))
               (flag-file-path (string-append
-                               xdg-runtime-dir "/on-first-login-executed"))
-              (touch (lambda (file-name)
-                       (call-with-output-file file-name (const #t)))))
+                               xdg-runtime-dir "/on-first-login-executed")))
          ;; XDG_RUNTIME_DIR dissapears on logout, that means such trick
          ;; allows to launch on-first-login script on first login only
          ;; after complete logout/reboot.
          (if (file-exists? xdg-runtime-dir)
-             (unless (file-exists? flag-file-path)
-               (begin #$@gexps (touch flag-file-path)))
+             (when (claim-first-run flag-file-path)
+               #$@gexps)
              ;; TRANSLATORS: 'on-first-login' is the name of a service and
              ;; shouldn't be translated
              (warning (G_ "XDG_RUNTIME_DIR doesn't exists, on-first-login script

base-commit: 9ad9113fc238ee8de5191a5e15b5153fd149e9fa
-- 
2.41.0

bug#66387: [PATCH] home: services: Fix race condition when detecting first login

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

Hi Carlo,

Carlo Zancanaro <carlo@zancanaro.id.au> skribis:

> * gnu/home/services.scm (compute-on-first-login-script): Use open to
> atomically check whether a file exists and create it if not.

Good catch!

I made the following cosmetic changes (‘open-fdes’ is cheaper than
‘open’) and applied it.

Thanks!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 966 bytes --]

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 7137925b30..651c068f79 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -414,15 +414,15 @@ (define (compute-on-first-login-script _ gexps)
          (use-modules (guix i18n)
                       (guix diagnostics))
 
-       (define (claim-first-run file-name)
+       (define (claim-first-run file)
          (catch #t
            (lambda ()
-             ;; This incantation will raise an error if the file at
-             ;; flag-file-path already exists, and will create it otherwise.
-             (close (open file-name (logior O_CREAT O_EXCL)))
+             ;; This incantation raises an error if FILE already exists, and
+             ;; creates it otherwise.
+             (close-fdes
+              (open-fdes file (logior O_CREAT O_EXCL O_CLOEXEC)))
              #t)
-           (lambda _
-             #f)))
+           (const #f)))
 
        #$%initialize-gettext
 

[bug#66387] [PATCH] home: services: Fix race condition when detecting first login

[Andrew Tropin]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On 2023-10-19 22:29, Ludovic Courtès wrote:

> Hi Carlo,
>
> Carlo Zancanaro <carlo@zancanaro.id.au> skribis:
>
>> * gnu/home/services.scm (compute-on-first-login-script): Use open to
>> atomically check whether a file exists and create it if not.
>
> Good catch!
>
> I made the following cosmetic changes (‘open-fdes’ is cheaper than
> ‘open’) and applied it.
>
> Thanks!
>
> Ludo’.
>
> diff --git a/gnu/home/services.scm b/gnu/home/services.scm
> index 7137925b30..651c068f79 100644
> --- a/gnu/home/services.scm
> +++ b/gnu/home/services.scm
> @@ -414,15 +414,15 @@ (define (compute-on-first-login-script _ gexps)
>           (use-modules (guix i18n)
>                        (guix diagnostics))
>  
> -       (define (claim-first-run file-name)
> +       (define (claim-first-run file)

Very nice trick!  Carol, Ludo, Thank you for the fix!

>           (catch #t
>             (lambda ()
> -             ;; This incantation will raise an error if the file at
> -             ;; flag-file-path already exists, and will create it otherwise.
> -             (close (open file-name (logior O_CREAT O_EXCL)))
> +             ;; This incantation raises an error if FILE already exists, and
> +             ;; creates it otherwise.
> +             (close-fdes
> +              (open-fdes file (logior O_CREAT O_EXCL O_CLOEXEC)))
>               #t)
> -           (lambda _
> -             #f)))
> +           (const #f)))
>  
>         #$%initialize-gettext
>  

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]