Skip to content

query-frontend: accept ttl as an alias for expiration in the response cache config - #9040

Open
shoemoney wants to merge 3 commits into
thanos-io:mainfrom
shoemoney:fix/qfe-cache-ttl-alias
Open

shoemoney wants to merge 3 commits into
thanos-io:mainfrom
shoemoney:fix/qfe-cache-ttl-alias

Conversation

@shoemoney

@shoemoney shoemoney commented Sep 19, 2026

Copy link
Copy Markdown

Fixes #6996

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

The store caches spell the cache lifetime ttl (pkg/store/cache/factory.go), the query-frontend response cache spells it expiration, and the response cache parses with yaml.UnmarshalStrict. So a query-frontend config that says ttl does not quietly fall back to a default, it stops the component from starting:

err="yaml: unmarshal errors:\n  line 3: field ttl not found in type queryfrontend.RedisResponseCacheConfig\ninitializing the query range cache config"

This accepts ttl as an alias for expiration in the memcached and redis response cache configs, and errors when both are set instead of silently picking one. It is the shape @MichaHoffmann proposed on the issue: "I would accept both and then error if both were provided i think."

Presence is detected by unmarshalling the backend config into a probe struct with pointer fields, because DefaultRedisConfig already sets Expiration: 24h, so a plain zero check cannot tell "user set expiration" from "default applied". A key that is present but zero does not count as set: the cache config is built as a Go struct and marshalled in places (test/e2e/query_frontend_test.go), which emits both keys, and scripts/cfggen rejects omitempty on config fields, so the conflict check has to key off the value rather than the key.

Applied to both memcached and redis on purpose. Fixing only redis, which is what the issue reports, would leave memcached rejecting ttl and create a fresh inconsistency.

What this does not do: it does not unify the query-frontend and store cache configs. @yeya24 noted on the issue that the QFE cache config comes from Cortex code and should be unified "at some point". That is a larger refactor and this is the small backward compatible step in the meantime, not a substitute for it.

Verification

Go 1.27.1, darwin/arm64, against 050d688f (main, 2026-09-18). Dated 2026-09-19.

New table test TestNewCacheConfig_TTLAlias in pkg/queryfrontend/config_test.go, red before the change and green after.

Red, test added and config.go reverted. This reproduces the exact error from the issue report:

--- FAIL: TestNewCacheConfig_TTLAlias
    --- FAIL: TestNewCacheConfig_TTLAlias/redis_ttl_is_accepted_as_an_alias_for_expiration
        line 2: field ttl not found in type queryfrontend.RedisResponseCacheConfig
    --- PASS: TestNewCacheConfig_TTLAlias/redis_expiration_still_works
    --- FAIL: TestNewCacheConfig_TTLAlias/redis_rejects_ttl_and_expiration_together
    --- FAIL: TestNewCacheConfig_TTLAlias/memcached_ttl_is_accepted_as_an_alias_for_expiration
    --- FAIL: TestNewCacheConfig_TTLAlias/memcached_rejects_ttl_and_expiration_together
FAIL	github.com/thanos-io/thanos/pkg/queryfrontend	0.044s

The expiration case passing while the rest fail is the control: it shows the test is not failing for an unrelated reason.

Green, with the change:

--- PASS: TestNewCacheConfig_TTLAlias (0.00s)
    --- PASS: TestNewCacheConfig_TTLAlias/redis_ttl_is_accepted_as_an_alias_for_expiration
    --- PASS: TestNewCacheConfig_TTLAlias/redis_expiration_still_works
    --- PASS: TestNewCacheConfig_TTLAlias/redis_rejects_ttl_and_expiration_together
    --- PASS: TestNewCacheConfig_TTLAlias/memcached_ttl_is_accepted_as_an_alias_for_expiration
    --- PASS: TestNewCacheConfig_TTLAlias/memcached_rejects_ttl_and_expiration_together
ok  	github.com/thanos-io/thanos/pkg/queryfrontend	0.042s

Package suite, measured as a delta rather than a claim of green, because pkg/queryfrontend is not clean on main in my environment. On 050d688f with no changes, TestLabelsCodec_DecodeResponse panics (slice bounds out of range [:96] with length 1) and TestLabelsCodec_MergeResponse fails. Both reproduce identically with and without this change. Excluding the panicking test so the run completes, the counts are 8 pass / 1 fail on clean main and 9 pass / 1 fail with this change, the +1 being the new test.

go vet ./pkg/queryfrontend/... is clean. gofmt is clean.

The first push of this branch got this wrong and CI caught it: treating a present key as set made TestQueryFrontendMemcachedCache fail with docker container query-frontend-1 failed to start, because that test builds the config as a Go struct and marshals it, so both keys were emitted with zero values and the component refused to start. That path is now pinned by TestNewCacheConfig_MarshalledStructRoundTrip, which marshals the same struct for both backends and asserts NewCacheConfig accepts it. I do not have Docker locally, so that unit test is my reproduction of the e2e failure rather than a local e2e run, and CI is the confirmation.

golangci-lint v2.4.0, the version pinned in .bingo/golangci-lint.mod, produces byte identical output with and without this change (11 pre-existing typecheck findings, none in the touched files). Those findings are an artifact of my local Go 1.27 toolchain rather than the code: the first one is could not load export data ... export data version 4 is greater than maximum supported version 2 on response.pb.go, which cascades into the rest of the package. I could not get a clean lint baseline locally for that reason, so CI is the authority here.

Docs: docs/components/query-frontend.md embeds generated config via mdox-exec="go run scripts/cfggen/main.go", so both affected blocks were regenerated with that command and the new field documented in prose alongside the existing expiration sentence.

CI

All checks are green on d6ce8c9.

Documentation check failed once on this commit with:

error: fmt command failed: parsing githubPullsIssues Regex: pulls API request failed. status code: 403

That was mdox hitting the GitHub pulls API, not a docs diff. The same check passed on the previous push of this branch with the same generated blocks, and it has since been re-run on this same commit and passed, so no change was needed.

All 8 Thanos end-to-end tests shards pass, including shard 2, which is the one that caught the mistake described above, along with Thanos unit tests, both linter jobs, CodeQL and codecov.

This PR was written in part with the assistance of generative AI.

Supersedes #9039, which GitHub closed automatically after a bad push replaced the branch head with an unrelated commit. The branch has been restored to the reviewed head, unchanged from what CI verified there.

… cache config

The store caches call this setting ttl and the query-frontend response cache
calls it expiration. The response cache parses strictly, so a config that says
ttl does not quietly fall back to a default, it stops the component from
starting with "field ttl not found in type ...ResponseCacheConfig".

Accept either name for the memcached and redis response caches, and reject a
config that sets both to a non-zero value rather than silently picking one.

A key that is present but zero does not count as set. The config is also built
as a Go struct and marshalled in places, which emits both keys, and cfggen
forbids omitempty on config fields.

Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com>
…esponse cache config

Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com>
…esponse cache config

Signed-off-by: Jeremy Schoemaker <jeremy@shoemoney.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Query frontend crashing when ttl specified in cache config

1 participant