Subject: [bug#50040] [PATCH 2/2] publish: Remove cache bypass support.

From: Mathieu Othacehe <[email-masked]>
Date: Fri, 13 Aug 2021 12:30:30 +0200

Unconditionally render narinfo and nar files during baking. This avoids the
need for pre-baking nar with a CI system. It also prevents receiving 404
errors during baking.

* guix/scripts/publish.scm (%options): Remove the cache-bypass-threshold
option.
(show-help): Ditto.
(cache-bypass-threshold, bypass-cache?): Remove them.
(render-narinfo/cached): Render a temporary narinfo unconditionally during
baking.
(render-nar/cached): Render a live nar unconditionally during baking.
(guix-publish): Remove cache bypass option.
* gnu/services/base.scm (<guix-publish-configuration>)
[cache-bypass-threshold]: Remove it
(guix-publish-shepherd-service): Adapt it.
* doc/guix.texi (Base services, Invoking guix publish): Document it.
* tests/publish.scm ("with cache", "with cache, lzip + gzip", "with cache,
uncompressed"): Adapt them.
("with cache, cache bypass", "with cache, cache bypass, unmapped hash part"):
Remove them.
---
 doc/guix.texi            | 28 -------------
 gnu/services/base.scm    | 11 +----
 guix/scripts/publish.scm | 54 ++++++------------------
 tests/publish.scm        | 88 ++++------------------------------------
 4 files changed, 24 insertions(+), 157 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 78c1c09858..adb2ada4e8 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -12752,13 +12752,6 @@ archive is cached in @var{directory}, subsequent requests succeed and
 are served directly from the cache, which guarantees that clients get
 the best possible bandwidth.
 
-That first @code{.narinfo} request nonetheless returns 200, provided the
-requested store item is ``small enough'', below the cache bypass
-threshold---see @option{--cache-bypass-threshold} below.  That way,
-clients do not have to wait until the archive is baked.  For larger
-store items, the first @code{.narinfo} request returns 404, meaning that
-clients have to wait until the archive is baked.
-
 The ``baking'' process is performed by worker threads.  By default, one
 thread per CPU core is created, but this can be customized.  See
 @option{--workers} below.
@@ -12794,21 +12787,6 @@ This parameter can help adjust server load and substitute latency by
 instructing cooperating clients to be more or less patient when a store
 item is missing.
 
-@item --cache-bypass-threshold=@var{size}
-When used in conjunction with @option{--cache}, store items smaller than
-@var{size} are immediately available, even when they are not yet in
-cache.  @var{size} is a size in bytes, or it can be suffixed by @code{M}
-for megabytes and so on.  The default is @code{10M}.
-
-``Cache bypass'' allows you to reduce the publication delay for clients
-at the expense of possibly additional I/O and CPU use on the server
-side: depending on the client access patterns, those store items can end
-up being baked several times until a copy is available in cache.
-
-Increasing the threshold may be useful for sites that have few users, or
-to guarantee that users get substitutes even for store items that are
-not popular.
-
 @item --nar-path=@var{path}
 Use @var{path} as the prefix for the URLs of ``nar'' files
 (@pxref{Invoking guix archive, normalized archives}).
@@ -15818,12 +15796,6 @@ When it is an integer, this is the number of worker threads used for
 caching; when @code{#f}, the number of processors is used.
 @xref{Invoking guix publish, @option{--workers}}, for more information.
 
-@item @code{cache-bypass-threshold} (default: 10 MiB)
-When @code{cache} is true, this is the maximum size in bytes of a store
-item for which @command{guix publish} may bypass its cache in case of a
-cache miss.  @xref{Invoking guix publish,
-@option{--cache-bypass-threshold}}, for more information.
-
 @item @code{ttl} (default: @code{#f})
 When it is an integer, this denotes the @dfn{time-to-live} in seconds
 of the published archives.  @xref{Invoking guix publish, @option{--ttl}},
diff --git a/gnu/services/base.scm b/gnu/services/base.scm
index c784d312b1..ac469abf8c 100644
--- a/gnu/services/base.scm
+++ b/gnu/services/base.scm
@@ -1775,8 +1775,6 @@ proxy of 'guix-daemon'...~%")
                (default "nar"))
   (cache       guix-publish-configuration-cache   ;#f | string
                (default #f))
-  (cache-bypass-threshold guix-publish-configuration-cache-bypass-threshold
-                          (default (* 10 (expt 2 20)))) ;integer
   (workers     guix-publish-configuration-workers ;#f | integer
                (default #f))
   (ttl         guix-publish-configuration-ttl     ;#f | integer
@@ -1815,8 +1813,7 @@ raise a deprecation warning if the 'compression-level' field was used."
                    lst))))
 
   (match-record config <guix-publish-configuration>
-    (guix port host nar-path cache workers ttl cache-bypass-threshold
-          advertise?)
+    (guix port host nar-path cache workers ttl advertise?)
     (list (shepherd-service
            (provision '(guix-publish))
            (requirement `(user-processes
@@ -1843,11 +1840,7 @@ raise a deprecation warning if the 'compression-level' field was used."
                                                     "s"))
                                   #~())
                            #$@(if cache
-                                  #~((string-append "--cache=" #$cache)
-                                     #$(string-append
-                                        "--cache-bypass-threshold="
-                                        (number->string
-                                         cache-bypass-threshold)))
+                                  #~((string-append "--cache=" #$cache))
                                   #~()))
 
                      ;; Make sure we run in a UTF-8 locale so we can produce
diff --git a/guix/scripts/publish.scm b/guix/scripts/publish.scm
index 981ef8d267..64f444e901 100644
--- a/guix/scripts/publish.scm
+++ b/guix/scripts/publish.scm
@@ -97,9 +97,6 @@ Publish ~a over HTTP.\n") %store-directory)
                          compress archives with METHOD at LEVEL"))
   (display (G_ "
   -c, --cache=DIRECTORY  cache published items to DIRECTORY"))
-  (display (G_ "
-      --cache-bypass-threshold=SIZE
-                         serve store items below SIZE even when not cached"))
   (display (G_ "
       --workers=N        use N workers to bake items"))
   (display (G_ "
@@ -214,10 +211,6 @@ usage."
         (option '(#\c "cache") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'cache arg result)))
-        (option '("cache-bypass-threshold") #t #f
-                (lambda (opt name arg result)
-                  (alist-cons 'cache-bypass-threshold (size->number arg)
-                              result)))
         (option '("workers") #t #f
                 (lambda (opt name arg result)
                   (alist-cons 'workers (string->number* arg)
@@ -513,18 +506,6 @@ vanished from the store in the meantime."
                result))
             (apply throw args))))))
 
-(define cache-bypass-threshold
-  ;; Maximum size of a store item that may be served by the '/cached' handlers
-  ;; below even when not in cache.
-  (make-parameter (* 10 (expt 2 20))))
-
-(define (bypass-cache? store item)
-  "Return true if we allow ITEM to be downloaded before it is cached.  ITEM is
-interpreted as the basename of a store item."
-  (guard (c ((store-error? c) #f))
-    (< (path-info-nar-size (query-path-info store item))
-       (cache-bypass-threshold))))
-
 (define* (render-narinfo/cached store request hash
                                 #:key ttl (compressions (list %no-compression))
                                 (nar-path "nar") negative-ttl
@@ -584,19 +565,14 @@ requested using POOL."
                                                      #:delete-entry delete-entry
                                                      #:cleanup-period ttl))))
 
-           ;; If ITEM passes 'bypass-cache?', render a temporary narinfo right
-           ;; away, with a short TTL.  The narinfo is temporary because it
-           ;; lacks 'FileSize', for instance, which the cached narinfo will
-           ;; have.  Chances are that the nar will be baked by the time the
-           ;; client asks for it.
-           (if (bypass-cache? store item)
-               (render-narinfo store request hash
-                               #:ttl 300          ;temporary
-                               #:nar-path nar-path
-                               #:compressions compressions)
-               (not-found request
-                          #:phrase "We're baking it"
-                          #:ttl 300)))          ;should be available within 5m
+           ;; Render a temporary narinfo right away, with a short TTL.  The
+           ;; narinfo is temporary because it lacks 'FileSize', for instance,
+           ;; which the cached narinfo will have.  Chances are that the nar
+           ;; will be baked by the time the client asks for it.
+           (render-narinfo store request hash
+                           #:ttl 300          ;temporary
+                           #:nar-path nar-path
+                           #:compressions compressions))
           (else
            (not-found request #:phrase "" #:ttl negative-ttl)))))
 
@@ -740,9 +716,9 @@ return it; otherwise, return 404.  When TTL is true, use it as the
                            `((cache-control (max-age . ,ttl)))
                            '())
 
-                     ;; XXX: We're not returning the actual contents, deferring
-                     ;; instead to 'http-write'.  This is a hack to work around
-                     ;; <http://bugs.gnu.org/21093>.
+                     ;; XXX: We're not returning the actual contents,
+                     ;; deferring instead to 'http-write'.  This is a hack to
+                     ;; work around <http://bugs.gnu.org/21093>.
                      (x-raw-file . ,cached))
                    #f))
           ((let* ((hash (and=> (string-index store-item #\-)
@@ -750,8 +726,7 @@ return it; otherwise, return 404.  When TTL is true, use it as the
                   (item (and hash
                              (guard (c ((store-error? c) #f))
                                (hash-part->path store hash)))))
-             (and item (not (string-null? item))
-                  (bypass-cache? store item)))
+             (and item (not (string-null? item))))
            ;; Render STORE-ITEM live.  We reach this because STORE-ITEM is
            ;; being baked but clients are already asking for it.  Thus, we're
            ;; duplicating work, but doing so allows us to reduce delays.
@@ -1306,10 +1281,7 @@ headers."
 consider using the '--user' option!~%")))
 
       (parameterize ((%public-key public-key)
-                     (%private-key private-key)
-                     (cache-bypass-threshold
-                      (or (assoc-ref opts 'cache-bypass-threshold)
-                          (cache-bypass-threshold))))
+                     (%private-key private-key))
         (info (G_ "publishing ~a on ~a, port ~d~%")
               %store-directory
               (inet-ntop (sockaddr:fam address) (sockaddr:addr address))
diff --git a/tests/publish.scm b/tests/publish.scm
index c3d086995a..43871a2a93 100644
--- a/tests/publish.scm
+++ b/tests/publish.scm
@@ -422,15 +422,14 @@ References: ~%"
         200                                       ;nar/gzip/…
         #t                                        ;Content-Length
         #t                                        ;FileSize
-        404)                                      ;nar/…
+        200)                                      ;nar/…
   (call-with-temporary-directory
    (lambda (cache)
      (let ((thread (with-separate-output-ports
                     (call-with-new-thread
                      (lambda ()
                        (guix-publish "--port=6797" "-C2"
-                                     (string-append "--cache=" cache)
-                                     "--cache-bypass-threshold=0"))))))
+                                     (string-append "--cache=" cache)))))))
        (wait-until-ready 6797)
        (let* ((base     "http://localhost:6797/")
               (part     (store-path-hash-part %item))
@@ -441,9 +440,9 @@ References: ~%"
               (nar      (string-append cache "/gzip/"
                                        (basename %item) ".nar"))
               (response (http-get url)))
-         (and (= 404 (response-code response))
+         (and (= 200 (response-code response))
 
-              ;; We should get an explicitly short TTL for 404 in this case
+              ;; We should get an explicitly short TTL for 200 in this case
               ;; because it's going to become 200 shortly.
               (match (assq-ref (response-headers response) 'cache-control)
                 ((('max-age . ttl))
@@ -477,15 +476,14 @@ References: ~%"
                       (response-code uncompressed)))))))))
 
 (test-equal "with cache, lzip + gzip"
-  '(200 200 404)
+  '(200 200 200)
   (call-with-temporary-directory
    (lambda (cache)
      (let ((thread (with-separate-output-ports
                     (call-with-new-thread
                      (lambda ()
                        (guix-publish "--port=6794" "-Cgzip:2" "-Clzip:2"
-                                     (string-append "--cache=" cache)
-                                     "--cache-bypass-threshold=0"))))))
+                                     (string-append "--cache=" cache)))))))
        (wait-until-ready 6794)
        (let* ((base     "http://localhost:6794/")
               (part     (store-path-hash-part %item))
@@ -533,15 +531,14 @@ References: ~%"
           (* 42 3600)                             ;TTL on nar/…
           (path-info-nar-size
            (query-path-info %store item))         ;FileSize
-          404)                                    ;nar/gzip/…
+          200)                                    ;nar/gzip/…
     (call-with-temporary-directory
      (lambda (cache)
        (let ((thread (with-separate-output-ports
                       (call-with-new-thread
                        (lambda ()
                          (guix-publish "--port=6796" "-C2" "--ttl=42h"
-                                       (string-append "--cache=" cache)
-                                       "--cache-bypass-threshold=0"))))))
+                                       (string-append "--cache=" cache)))))))
          (wait-until-ready 6796)
          (let* ((base     "http://localhost:6796/")
                 (part     (store-path-hash-part item))
@@ -551,7 +548,7 @@ References: ~%"
                 (nar      (string-append cache "/none/"
                                          (basename item) ".nar"))
                 (response (http-get url)))
-           (and (= 404 (response-code response))
+           (and (= 200 (response-code response))
 
                 (wait-for-file cached)
                 (let* ((response     (http-get url))
@@ -611,73 +608,6 @@ References: ~%"
                 (delete-paths %store (list item))
                 (response-code (pk 'response (http-get url))))))))))
 
-(test-equal "with cache, cache bypass"
-  200
-  (call-with-temporary-directory
-   (lambda (cache)
-     (let ((thread (with-separate-output-ports
-                    (call-with-new-thread
-                     (lambda ()
-                       (guix-publish "--port=6788" "-C" "gzip"
-                                     (string-append "--cache=" cache)))))))
-       (wait-until-ready 6788)
-
-       (let* ((base     "http://localhost:6788/")
-              (item     (add-text-to-store %store "random" (random-text)))
-              (part     (store-path-hash-part item))
-              (narinfo  (string-append base part ".narinfo"))
-              (nar      (string-append base "nar/gzip/" (basename item)))
-              (cached   (string-append cache "/gzip/" (basename item)
-                                       ".narinfo")))
-         ;; We're below the default cache bypass threshold, so NAR and NARINFO
-         ;; should immediately return 200.  The NARINFO request should trigger
-         ;; caching, and the next request to NAR should return 200 as well.
-         (and (let ((response (pk 'r1 (http-get nar))))
-                (and (= 200 (response-code response))
-                     (not (response-content-length response)))) ;not known
-              (= 200 (response-code (http-get narinfo)))
-              (begin
-                (wait-for-file cached)
-                (let ((response (pk 'r2 (http-get nar))))
-                  (and (> (response-content-length response)
-                          (stat:size (stat item)))
-                       (response-code response))))))))))
-
-(test-equal "with cache, cache bypass, unmapped hash part"
-  200
-
-  ;; This test reproduces the bug described in <https://bugs.gnu.org/44442>:
-  ;; the daemon connection would be closed as a side effect of a nar request
-  ;; for a non-existing file name.
-  (call-with-temporary-directory
-   (lambda (cache)
-     (let ((thread (with-separate-output-ports
-                    (call-with-new-thread
-                     (lambda ()
-                       (guix-publish "--port=6787" "-C" "gzip"
-                                     (string-append "--cache=" cache)))))))
-       (wait-until-ready 6787)
-
-       (let* ((base     "http://localhost:6787/")
-              (item     (add-text-to-store %store "random" (random-text)))
-              (part     (store-path-hash-part item))
-              (narinfo  (string-append base part ".narinfo"))
-              (nar      (string-append base "nar/gzip/" (basename item)))
-              (cached   (string-append cache "/gzip/" (basename item)
-                                       ".narinfo")))
-         ;; The first response used to be 500 and to terminate the daemon
-         ;; connection as a side effect.
-         (and (= (response-code
-                  (http-get (string-append base "nar/gzip/"
-                                           (make-string 32 #\e)
-                                           "-does-not-exist")))
-                 404)
-              (= 200 (response-code (http-get nar)))
-              (= 200 (response-code (http-get narinfo)))
-              (begin
-                (wait-for-file cached)
-                (response-code (http-get nar)))))))))
-
 (test-equal "/log/NAME"
   `(200 #t application/x-bzip2)
   (let ((drv (run-with-store %store
-- 
2.32.0