Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cached items get removed when upstream fetch is unsuccessful #4254

Closed
3 tasks done
agneevX opened this issue Feb 6, 2022 · 10 comments
Closed
3 tasks done

Cached items get removed when upstream fetch is unsuccessful #4254

agneevX opened this issue Feb 6, 2022 · 10 comments
Assignees
@EugeneOne1
Labels
bug P3: Medium
Milestone
v0.107.4

Comments

@agneevX
Copy link
Contributor

agneevX commented Feb 6, 2022

Prerequisites

  • I am running the latest version
  • I checked the documentation and found no answer
  • I checked to make sure that this issue has not already been filed

Issue Details

  • Version of AdGuard Home server:
    • 0.107.3
  • How did you install AdGuard Home:
    • Docker
  • How did you setup DNS configuration:
  • If it's a router or IoT, please write device model:
  • CPU architecture:
    • arm64
  • Operating system and version:
    • Ubuntu 20.04.3

Issue Details

It seems that cached records get deleted when an upstream query is unsuccessful, after the cached query is served.

I lost at internet at 5:16:29 PM, so when the query was made one second later, a cached query was served.
At 5:36:31, when the internet was back up already, the query was fetched from upstreams, instead of from the cache.

However I'd say this requires some more testing.

Additional Information

I remember v0.107.0 serving cached records for an infinite amount of time when upstreams were unreachable. This is why actually the network was functional even though I had misconfigured upstreams, funny story!
@EugeneOne1
Copy link
Member

@agneevX, hello. The responses are cached for a TTL value they had so that the upstream server won't be queried until the response expires. If you have the "Optimistic caching" enabled, the last response should only be returned one more time. In any case the expired cache items are removed if those couldn't be resolved again so I would say this is the intended behavior. Did my answer clarified the logic?

@EugeneOne1 EugeneOne1 added waiting for data Waiting for users to provide more data. needs investigation Needs to be reproduced reliably. labels Feb 7, 2022
@agneevX
Copy link
Contributor Author

agneevX commented Feb 7, 2022

If you have the "Optimistic caching" enabled, the last response should only be returned one more time. In any case the expired cache items are removed if those couldn't be resolved

I do have Optimistic Caching enabled. I don't think this behavior is right, it should still serve expired queries rather than deleting items it could not get a reply to.

Has this behavior changed recently? While I'm using DNS-over-HTTPS resolvers now, this was not the case before.

@EugeneOne1
Copy link
Member

@agneevX, well, you're right. This is a bug, we'll fix it soon.

@EugeneOne1 EugeneOne1 added bug external libs Issues that require changes in external libraries. and removed needs investigation Needs to be reproduced reliably. waiting for data Waiting for users to provide more data. labels Feb 7, 2022
@agneevX
Copy link
Contributor Author

agneevX commented Feb 7, 2022

Thanks @EugeneOne1 as always 👍

@EugeneOne1 EugeneOne1 self-assigned this Feb 7, 2022
@EugeneOne1 EugeneOne1 added P3: Medium and removed external libs Issues that require changes in external libraries. labels Feb 7, 2022
@EugeneOne1 EugeneOne1 added this to the v0.107.4 milestone Feb 7, 2022
adguard pushed a commit to AdguardTeam/dnsproxy that referenced this issue Feb 7, 2022
@EugeneOne1
Pull request: 4254 fix optimistic
5956b6d 
Merge in DNS/dnsproxy from 4254-fix-optimistic to master

Updates AdguardTeam/AdGuardHome#4254.

Squashed commit of the following:

commit 90f3467
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:17:21 2022 +0300

    proxy: imp docs again

commit 010a02c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:16:38 2022 +0300

    proxy: imp code

commit f6e31fa
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:08:45 2022 +0300

    proxy: imp docs

commit a50ba57
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 17:33:55 2022 +0300

    proxy: fix lint

commit 4288ccb
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 17:26:38 2022 +0300

    proxy: make optimistic actually optimistic
adguard pushed a commit that referenced this issue Feb 7, 2022
@EugeneOne1
Pull request: 4254 fix optimistic
6584c30 
Merge in DNS/adguard-home from 4254-fix-optimistic to master

Updates #4254.

Squashed commit of the following:

commit 652e2c2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:55:34 2022 +0300

    all: upd proxy
@EugeneOne1
Copy link
Member

@agneevX, latest edge build should fix the optimistic cache behavior. Could you please check if it now responds regardless to the internet connection status?

@agneevX
Copy link
Contributor Author

agneevX commented Feb 7, 2022

While I'm not able to test this currently, I'll reach out if there's an issue once I'm able to do so. Thanks again!

@agneevX agneevX closed this as completed Feb 7, 2022
adguard pushed a commit to AdguardTeam/dnsproxy that referenced this issue Feb 7, 2022
@EugeneOne1 @ainar-g
cherry-pick: 4254 fix optimistic
b3947de 
Merge in DNS/dnsproxy from 4254-fix-optimistic to master

Updates AdguardTeam/AdGuardHome#4254.

Squashed commit of the following:

commit 90f3467
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:17:21 2022 +0300

    proxy: imp docs again

commit 010a02c
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:16:38 2022 +0300

    proxy: imp code

commit f6e31fa
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:08:45 2022 +0300

    proxy: imp docs

commit a50ba57
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 17:33:55 2022 +0300

    proxy: fix lint

commit 4288ccb
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 17:26:38 2022 +0300

    proxy: make optimistic actually optimistic
@gspannu
Copy link

gspannu commented Feb 8, 2022

@agneevX

Quick question:

What tool/ app are you using that shows query logs as above? Would you mind sharing this information please?

My query log (from my Cloud hosted AdGuardHome)

Looks like this on a mobile web page (Safari on iOS)
IMG_2E1D111D4CB8-1

and like this on my desktop macOS browser (Safari)
Screenshot

@agneevX
Copy link
Contributor Author

agneevX commented Feb 8, 2022

This is the AdGuard Home Remote app @gspannu.

@gspannu
Copy link

gspannu commented Feb 8, 2022

Is it the Rocket Science app or some other one?
https://rocketscience-it.nl/
https://apps.apple.com/gb/app/adguard-home-remote/id1543143740

@jojost1
Copy link

jojost1 commented Feb 9, 2022

Is it the Rocket Science app or some other one?
https://rocketscience-it.nl/
https://apps.apple.com/gb/app/adguard-home-remote/id1543143740

Yes, it's that one (I'm the creator 😄)

adguard pushed a commit that referenced this issue Feb 11, 2022
@EugeneOne1 @ainar-g
cherry-pick: 4254 fix optimistic
97b3ed4 
Merge in DNS/adguard-home from 4254-fix-optimistic to master

Updates #4254.

Squashed commit of the following:

commit 652e2c2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:55:34 2022 +0300

    all: upd proxy
heyxkhoa pushed a commit to heyxkhoa/AdGuardHome that referenced this issue Mar 20, 2023
@EugeneOne1 @heyxkhoa
Pull request: 4254 fix optimistic
be99552 
Merge in DNS/adguard-home from 4254-fix-optimistic to master

Updates AdguardTeam#4254.

Squashed commit of the following:

commit 652e2c2
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date:   Mon Feb 7 18:55:34 2022 +0300

    all: upd proxy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@EugeneOne1 EugeneOne1

Labels
bug P3: Medium
Projects
None yet
Milestone
v0.107.4
Development

No branches or pull requests
4 participants
@gspannu @jojost1 @agneevX @EugeneOne1