Improve selection of cache RPP fallback

Corner case. Suppose the cache has (for whatever reason) downloaded the
two following URLs separately:

	rsync://a.b.c/d
	rsync://a.b.c/d/e/f

(This might happen if the latter is downloaded in one iteration, then
the former is downloaded the next, and the cleanup timer hasn't kicked
in yet.)

This commit extends the existing priority selection algorithm to this
situation.

(The old way was to choose the most specific URL, which would go on to
lose to a different URL which might have lost to the less specific
version.)

Honestly, this is a micro-correction. It hopefully slightly increases
the chances of the cache fallback being useful in very specific unusual
situations, rather than guarantee it. But it's more consistent, future-
proof and looks more sensible in the tests.

diff --git a/src/cache/local_cache.c b/src/cache/local_cache.c
index 2780c618cccfdf4af50fc751c47921d9a7c28f57..06d3665c88cbf382a6efd17c12978ff94d704817 100644 (file)
--- a/src/cache/local_cache.c
+++ b/src/cache/local_cache.c
@@ -80,7 +80,6 @@ static struct cache_node *https;
 
 static time_t startup_time; /* When we started the last validation */
 
-/* Minimizes multiple evaluation */
 static struct cache_node *
 add_child(struct cache_node *parent, char const *basename)
 {
@@ -494,8 +493,41 @@ end:
        return error;
 }
 
+/*
+ * Highest to lowest priority:
+ *
+ * 1. Recent Success: !error, CNF_SUCCESS, high ts_success.
+ * 2. Old Success: !error, CNF_SUCCESS, low ts_success.
+ * 3. Previous Recent Success: error, CNF_SUCCESS, high ts_success.
+ * 4. Previous Old Success: error, CNF_SUCCESS, old ts_success.
+ * 5. No Success: !CNF_SUCCESS (completely unviable)
+ */
+static struct cache_node *
+choose_better(struct cache_node *old, struct cache_node *new)
+{
+       if (!(new->flags & CNF_SUCCESS))
+               return old;
+       if (old == NULL)
+               return new;
+
+       /*
+        * We're gonna have to get subjective here.
+        * Should we prioritize a candidate that was successfully downloaded a
+        * long time ago (with no retries since), or one that failed recently?
+        * Both are terrible, but returning something is still better than
+        * returning nothing, because the validator might manage to salvage
+        * remnant cached ROAs that haven't expired yet.
+        */
+
+       if (old->error && !new->error)
+               return new;
+       if (!old->error && new->error)
+               return old;
+       return (difftime(old->ts_success, new->ts_success) < 0) ? new : old;
+}
+
 static struct cache_node *
-find_uri(struct rpki_uri *uri)
+find_node(struct rpki_uri *uri)
 {
        char *luri, *token, *saveptr;
        struct cache_node *parent, *node;
@@ -528,79 +560,45 @@ find_uri(struct rpki_uri *uri)
                if (node == NULL)
                        goto end;
                if (recursive && (node->flags & CNF_DIRECT))
-                       result = node;
+                       result = choose_better(result, node);
                parent = node;
        }
 
-       if ((node != NULL) && (node->flags & CNF_DIRECT))
-               result = node;
+       if (!recursive && (node != NULL) && (node->flags & CNF_DIRECT))
+               result = choose_better(result, node);
 
 end:
        free(luri);
        return result;
 }
 
-static unsigned int
-get_score(struct cache_node *node)
-{
-       unsigned int score;
-
-       /*
-        * Highest to lowest priority:
-        *
-        * 1. Recent Success: !error, CNF_SUCCESS, high ts_success.
-        * 2. Old Success: !error, CNF_SUCCESS, low ts_success.
-        * 3. Previous Recent Success: error, CNF_SUCCESS, high ts_success.
-        * 4. Previous Old Success: error, CNF_SUCCESS, old ts_success.
-        * 5. No Success: error, !CNF_SUCCESS (completely unviable)
-        */
-
-       if (node == NULL)
-               return 0;
-
-       score = 0;
-       if (!node->error)
-               score |= (1 << 1);
-       if (node->flags & CNF_SUCCESS)
-               score |= (1 << 0);
-       return score;
-}
-
-/*
- * Returns true if @n1's success happened earlier than n2's.
- */
-static bool
-earlier_success(struct cache_node *n1, struct cache_node *n2)
-{
-       return difftime(n1->ts_success, n2->ts_success) < 0;
-}
+struct uri_and_node {
+       struct rpki_uri *uri;
+       struct cache_node *node;
+};
 
-struct rpki_uri *
-cache_recover(struct uri_list *uris, bool use_rrdp)
+/* Separated because of unit tests. */
+static void
+__cache_recover(struct uri_list *uris, bool use_rrdp, struct uri_and_node *best)
 {
-       struct scr {
-               struct rpki_uri *uri;
-               struct cache_node *node;
-               unsigned int score;
-       };
-
        struct rpki_uri **uri;
-       struct scr cursor;
-       struct scr best = { 0 };
+       struct uri_and_node cursor;
 
        ARRAYLIST_FOREACH(uris, uri) {
                cursor.uri = *uri;
-               cursor.node = find_uri(cursor.uri);
-               cursor.score = get_score(cursor.node);
-               if (cursor.score == 0)
+               cursor.node = find_node(cursor.uri);
+               if (cursor.node == NULL)
                        continue;
-               if (cursor.score > best.score)
-                       best = cursor;
-               else if (cursor.score == best.score
-                     && earlier_success(best.node, cursor.node))
-                       best = cursor;
+               if (choose_better(best->node, cursor.node) == cursor.node)
+                       *best = cursor;
        }
+}
 
+struct rpki_uri *
+cache_recover(struct uri_list *uris, bool use_rrdp)
+{
+       struct uri_and_node best = { 0 };
+       __cache_recover(uris, use_rrdp, &best);
        return best.uri;
 }
 

diff --git a/test/cache/local_cache_test.c b/test/cache/local_cache_test.c
index f497cdcbbbe23198cc346a127b4a7ba0cf535542..5908cce007e2b8e2a83f3ec85c3bcc1e287e9285 100644 (file)
--- a/test/cache/local_cache_test.c
+++ b/test/cache/local_cache_test.c
@@ -1336,7 +1336,29 @@ START_TEST(test_recover)
        ck_assert_ptr_eq(uris.array[14], cache_recover(&uris, false));
        uris_cleanup(&uris);
 
-       /* FIXME test HTTP (non-recursive) */
+       cache_teardown();
+
+
+       struct uri_and_node un = { 0 };
+
+       rsync = NODE("rsync", 0, 0,
+               TNODE("1", CNF_SUCCESS, 200, 200, 0,
+                       TNODE("2", CNF_DIRECT, 200, 200, 1,
+                               TNODE("3", CNF_DIRECT | CNF_SUCCESS, 100, 100, 1,
+                                       TNODE("4", CNF_DIRECT | CNF_SUCCESS, 200, 200, 1,
+                                               TNODE("5", CNF_DIRECT | CNF_SUCCESS, 100, 100, 0,
+                                                       TNODE("6", CNF_DIRECT | CNF_SUCCESS, 200, 200, 0)))))));
+
+       /* Try them all at the same time */
+       PREPARE_URI_LIST(&uris, "rsync://1/2/3/4/5/6");
+       __cache_recover(&uris, false, &un);
+       ck_assert_ptr_eq(uris.array[0], un.uri);
+       ck_assert_str_eq("6", un.node->basename);
+       uris_cleanup(&uris);
+
+       /* TODO (test) HTTP (non-recursive) */
+       /* TODO (test) more variations */
+       /* TODO (test) node with DIRECT, then not direct, then DIRECT */
 
        cache_teardown();
 }