stale_ttl: fix expired time caculation

kong/dns/client.lua

@@ -518,27 +518,27 @@ local function resolve_callback(self, name, qtype, cache_only, tries)
   -- `:peek(stale=true)` verifies if the expired key remains in L2 shm, then
   -- initiates an asynchronous background updating task to refresh it.
   local ttl, _, answers = self.cache:peek(key, true)
-  if answers and not answers.errcode and ttl then
-    if not answers.expired then
-      answers.expire = now() + ttl
-      answers.expired = true
-      ttl = ttl + self.stale_ttl
 
-    else
-      ttl = ttl + (answers.expire - now())
+  if answers and not answers.errcode and self.stale_ttl and ttl then
+
+    -- `_expire_at` means the final expiration time of stale records
+    if not answers._expire_at then
+      answers._expire_at = answers.expire + self.stale_ttl
     end
 
     -- trigger the update task by the upper caller every 60 seconds
-    ttl = math_min(ttl, 60)
+    local remaining_stale_ttl = math_min(answers._expire_at - now(), 60)
 
-    if ttl > 0 then
-      log(DEBUG, PREFIX, "start stale update task ", key, " ttl:", ttl)
+    if remaining_stale_ttl > 0 then
+      log(DEBUG, PREFIX, "start stale update task ", key,
+                         " remaining_stale_ttl:", remaining_stale_ttl)
 
       -- mlcache's internal lock mechanism ensures concurrent control
       start_stale_update_task(self, key, name, qtype)
-      answers.ttl = ttl
+      answers.ttl = remaining_stale_ttl
+      answers.expire = remaining_stale_ttl + now()
 
-      return answers, nil, ttl
+      return answers, nil, remaining_stale_ttl
     end
   end
 

spec/01-unit/30-new-dns-client/02-old_client_spec.lua

@@ -1292,7 +1292,7 @@ describe("[DNS client]", function()
     assert.are.equal(NOT_FOUND_ERROR, err2)
     answers2 = assert(cli.cache:get(qname .. ":" .. resolver.TYPE_A))
     assert.equal(answers1, answers2)
-    assert.falsy(answers2.expired)
+    assert.falsy(answers2._expire_at)
 
     -- wait for expiry of ttl and retry, it will not use the cached one
     -- because the cached one contains no avaible IP addresses
@@ -1303,7 +1303,7 @@ describe("[DNS client]", function()
     assert.are.equal(NOT_FOUND_ERROR, err2)
 
     answers2 = assert(cli.cache:get(qname .. ":" .. resolver.TYPE_A))
-    assert.falsy(answers2.expired)  -- refreshed record
+    assert.falsy(answers2._expire_at)  -- refreshed record
 
     -- wait for expiry of stale_ttl and retry, should be called twice now
     ngx.sleep(0.75 * stale_ttl)
@@ -1315,7 +1315,7 @@ describe("[DNS client]", function()
 
     answers2 = assert(cli.cache:get(qname .. ":" .. resolver.TYPE_A))
     assert.not_equal(answers1, answers2)
-    assert.falsy(answers2.expired)  -- new answers, not expired
+    assert.falsy(answers2._expire_at)  -- new answers, not expired
   end)
 
   it("verifies stale_ttl for available records", function()
@@ -1345,7 +1345,7 @@ describe("[DNS client]", function()
     answers1 = cli:resolve(qname, { qtype = resolver.TYPE_A })
     assert.same(answers1[1].address, "1.1.1.1")
     assert.are.equal(call_count, 1)
-    assert.falsy(answers1.expired)
+    assert.falsy(answers1._expire_at)
 
     -- try again, HIT from cache, not stale
     answers2 = cli:resolve(qname, { qtype = resolver.TYPE_A })
@@ -1359,10 +1359,9 @@ describe("[DNS client]", function()
     assert.are.equal(call_count, 1) -- todo: flakiness
 
     answers2 = assert(cli.cache:get(qname .. ":" .. resolver.TYPE_A))
-    assert.is_true(answers2.expired)
-    answers2.expired = nil  -- clear to be same with answers1
+    assert(answers2._expire_at)
+    answers2._expire_at = nil  -- clear to be same with answers1
     assert_same_answers(answers1, answers2)
-    answers2.expired = true
 
     -- async stale updating task
     ngx.sleep(0.1 * stale_ttl)
@@ -1372,15 +1371,15 @@ describe("[DNS client]", function()
     answers2 = cli:resolve(qname, { qtype = resolver.TYPE_A })
     assert.same(answers2[1].address, "1.1.1.1")
     assert.are.equal(call_count, 2)
-    assert.falsy(answers2.expired)
+    assert.falsy(answers2._expire_at)
 
     -- The stale one will be completely eliminated from the cache.
     ngx.sleep(ttl + stale_ttl)
 
     answers2 = cli:resolve(qname, { qtype = resolver.TYPE_A })
     assert.same(answers2[1].address, "1.1.1.1")
     assert.are.equal(call_count, 3)
-    assert.falsy(answers2.expired)
+    assert.falsy(answers2._expire_at)
   end)
 
   describe("verifies the polling of dns queries, retries, and wait times", function()

spec/01-unit/30-new-dns-client/03-old_client_cache_spec.lua

@@ -232,10 +232,9 @@ describe("[DNS client cache]", function()
       -- background refresh query
       local answers2 = cli:resolve("myhost6")
       assert.falsy(answers2[1].tag)
-      assert.is_true(answers2.expired)  -- stale; marked as expired
-      answers2.expired = nil
+      assert.is_number(answers2._expire_at)  -- stale; marked as expired
+      answers2._expire_at = nil
       assert_same_answers(answers2, answers)
-      answers2.expired = true
 
       -- wait for the refresh to complete. Ensure that the sleeping time is less
       -- than ttl, avoiding the updated record from becoming stale again.
@@ -244,7 +243,7 @@ describe("[DNS client cache]", function()
       -- resolve and check whether we got the new record from the mock copy
       local answers3 = cli:resolve("myhost6")
       assert.equal(answers3[1].tag, "new")
-      assert.falsy(answers3.expired)
+      assert.falsy(answers3._expired_at)
       assert.not_equal(answers, answers3)  -- must be a different record now
       assert_same_answers(answers3, mock_records["myhost6.domain.test:"..resolver.TYPE_A])
 
@@ -345,15 +344,15 @@ describe("[DNS client cache]", function()
       }
       -- doing a resolve will trigger the background query now
       answers = cli:resolve("myhost9", { qtype = resolver.TYPE_A })
-      assert.is_true(answers.expired)  -- we get the stale record, now marked as expired
+      assert.is_number(answers._expire_at)  -- we get the stale record, now marked as expired
       -- wait again for the background query to complete
       sleep(0.1)
       -- background resolve is now complete, check the cache, it should still have the
       -- stale record, and it should not have been replaced by the error
       --
       answers = cli.cache:get("myhost9:" .. resolver.TYPE_A)
-      assert.is_true(answers.expired)
-      answers.expired = nil
+      assert.is_number(answers._expire_at)
+      answers._expire_at = nil
       assert_same_answers(rec1, answers)
     end)
 
@@ -383,14 +382,14 @@ describe("[DNS client cache]", function()
       }
       -- doing a resolve will trigger the background query now
       answers = cli:resolve("myhost9", { qtype = resolver.TYPE_A })
-      assert.is_true(answers.expired)  -- we get the stale record, now marked as expired
+      assert.is_number(answers._expire_at)  -- we get the stale record, now marked as expired
       -- wait again for the background query to complete
       sleep(0.1)
       -- background resolve is now complete, check the cache, it should still have the
       -- stale record, and it should not have been replaced by the empty record
       answers = cli.cache:get("myhost9:" .. resolver.TYPE_A)
-      assert.is_true(answers.expired)  -- we get the stale record, now marked as expired
-      answers.expired = nil
+      assert.is_number(answers._expire_at)  -- we get the stale record, now marked as expired
+      answers._expire_at = nil
       assert_same_answers(rec1, answers)
     end)