Skip to content

proxy: preserve zero TTL in respectTTLOverrides - #491

Open
hengwu0 wants to merge 1 commit into
AdguardTeam:masterfrom
hengwu0:master
Open

proxy: preserve zero TTL in respectTTLOverrides#491
hengwu0 wants to merge 1 commit into
AdguardTeam:masterfrom
hengwu0:master

Conversation

@hengwu0

@hengwu0 hengwu0 commented Mar 26, 2026

Copy link
Copy Markdown

TTL=0 means the response must not be cached and should only be used for the current transaction.

Before this change, respectTTLOverrides() promoted TTL=0 to cacheMinTTL, which could turn a non-cacheable response into a cacheable one.

Skip the minimum TTL override when ttl is zero so zero-TTL responses preserve their original semantics.

@hengwu0

hengwu0 commented Mar 26, 2026

Copy link
Copy Markdown
Author

fixed #490

@Sil3ntVip3r Sil3ntVip3r left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The production change is correct. I independently added an integration regression with CacheMinTTL=20 and an upstream A record at TTL=0. On current master it fails because the response TTL becomes 20 and a cache item is created. On this PR current merge ref it preserves TTL=0, creates no cache item, passes 20 repetitions under -race, and the full proxy package also passes under -race. Please add an equivalent regression test to this PR so this standards-sensitive behavior remains protected.

TTL=0 means the response must not be cached and should only be used
for the current transaction.

Before this change, respectTTLOverrides() promoted TTL=0 to
cacheMinTTL, which could turn a non-cacheable response into a
cacheable one.

Skip the minimum TTL override when ttl is zero so zero-TTL responses
preserve their original semantics.

Signed-off-by: hengwu0 <17380582683@163.com>
@hengwu0

hengwu0 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Added an integration regression test with CacheMinTTL set to 20 and an upstream A record with TTL 0. The test verifies that the response TTL remains zero and that no cache entry is created.

@Sil3ntVip3r Sil3ntVip3r left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 3cfeeffe630de523034077d27b7fc96ad9d4b8cd.

The added preserve_zero integration case covers the gap from my earlier
review through the real Proxy.Resolve and cache path: with
CacheMinTTL=20, an upstream A response at TTL 0 remains TTL 0 in the
returned message and produces no cache entry. The existing minimum- and
maximum-override cases remain intact.

Fresh validation in a clean detached checkout:

go test -race -count=20 -run '^TestCacheExpirationWithTTLOverride$/preserve_zero$' ./proxy
PASS

go test -race -count=1 ./proxy
PASS

go vet ./proxy/...
PASS

git diff --check acf2b30e05202e171bbbaf4a3176ea07edb964b3...HEAD
PASS

The checkout remained clean at the exact head. I found no remaining
correctness or regression-coverage blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants