Skip to content

Rebuild well-known string indexes when reading a cached object - #13603

Open
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:claude/wks-cache-versioning-abbe91
Open

Rebuild well-known string indexes when reading a cached object#13603
bneradt wants to merge 1 commit into
apache:masterfrom
bneradt:claude/wks-cache-versioning-abbe91

Conversation

@bneradt

@bneradt bneradt commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Unblocks #9636 by decoupling the well-known string (WKS) table from the on-disk cache format.

This does not invalidate an existing cache

Upgrading to this build keeps and serves every object already on disk. Walking a pre-existing 24.2 object through the 24.3 reader:

Check Result
Stripe validation (StripeSM::handle_dir_read) Major version only, 21 <= 24 <= 24 → directory attaches, nothing cleared
Future-version check (CacheVC::handleReadDone) 24.2 > 24.3 is false → not treated as corrupt
Fragment offset reader (unmarshal_helper) 24.2 < 24.2 is false → current reader, correct layout
WKS rebuild Runs, from the object's own strings

CACHE_SHM_SCHEMA_VERSION is a separate constant, so shm fast restart is unaffected, and Stripe::_clear_init writes the version only when a stripe is created or cleared.

The minor bump to 24.3 exists for the reverse direction: an ATS older than 24.3 rejects objects this build writes and refetches them, rather than resolving their indexes against its own table. So a rollback costs a refetch of whatever was written while on 24.3, and nothing that predates the upgrade. Objects older than 24.2 keep going through unmarshal_v24_1 exactly as before.

The problem

A marshalled header stores indexes into the WKS table right beside the strings those indexes stand for: the index of every field name, of the request method, and of the request URL scheme, plus the presence bits and slot accelerators derived from them. Change the table and every stored index denotes a different string — GET reads back as PUSH, http as wais, Cache-Control becomes invisible to lookups. That has kept the table frozen at its 2000-era contents.

The indexes are a pure cache over strings the object already carries, so they can always be rebuilt. HTTPInfo::unmarshal() now does that for both headers once the heaps are swizzled. The CacheAltMagic check keeps it to once per marshalled buffer, so the work lands on a read that already paid for disk I/O or a RAM-cache decompression, and it covers every reader of a marshalled object.

Alan Carroll put half of this in place in a62d3a3 (PR-1794), rebuilding the field indexes for objects older than the running version. It never covered the method or scheme indexes — and both http_hdr_method_get() and URLImpl::get_scheme() answer from the index in preference to the stored string. Its !f.doc_from_ram_cache guard is also wrong when proxy.config.cache.ram_cache.compress is on, since the RAM cache then holds the object still marshalled.

Why a version bump used to be unsafe

unmarshal_helper() selected the 4.2-era HTTPInfo::unmarshal_v24_1() with version < CACHE_DB_VERSION, which is only correct while CACHE_DB_VERSION == 24.2. Bumping the minor version would have sent every object the previous release wrote through that reader, corrupting the fragment table of any object with more than four fragments. It now compares against CACHE_DB_FRAG_OFFSET_TABLE_VERSION, the fixed version the layout changed at.

With that fixed, the bump to 24.3 is safe, and the append-only _hdrtoken_strs_frozen ledger from #13559 has nothing left to enforce, so it is removed.

Testing

Two Catch2 tests in test_Hdrs.cc and a new cache_wks_table_change autest. Since the table is constexpr, a process cannot host two of them; a TS_HAS_TESTS-gated ATS_TEST_WKS_IDX_SHIFT hook stands in by rotating every index written into a marshalled heap.

Both were checked to fail without the fix rather than pass vacuously. A cached response prints from the stored strings and looks correct even with scrambled indexes, so the autest turns on ATS finding fields instead: without the rebuild it cannot find Etag or Last-Modified in the cached response and sends a plain GET where it owes the origin a conditional one.

test_proxy_hdrs and 16 cache autests pass.

Follow-up

The string churn from #9636 is a separate PR on top of this one, and needs no further cache work.

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings August 31, 2026 15:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bneradt
bneradt force-pushed the claude/wks-cache-versioning-abbe91 branch 2 times, most recently from f03c5b4 to de45791 Compare August 31, 2026 16:26
Copilot AI review requested due to automatic review settings August 31, 2026 16:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@bneradt bneradt added this to the 11.0.0 milestone Aug 31, 2026
@bneradt bneradt removed the 11.0.x label Aug 31, 2026
@bneradt bneradt self-assigned this Aug 31, 2026
The well-known string table has been effectively frozen for two
decades because cached objects store indexes into it, so changing the
table makes every stored index denote a different string. The only
remedy on offer was a cache version bump, and that was itself unsafe:
the fragment offset reader is selected by comparing an object's
version against the current cache version rather than against the
fixed version that layout changed at, so any bump would have routed
the previous release's objects through the wrong reader.

This rebuilds the indexes, along with the presence bits and slot
accelerators derived from them, out of the header strings the object
already stores, at unmarshal time. A build can then read objects
written against any table. This also corrects the reader selection to
compare against that fixed version, and completes an earlier partial
rebuild that missed the request method and URL scheme indexes and
skipped objects served from a compressed RAM cache.

Upgrading to this does not invalidate an existing cache. Stripe
validation looks only at the major version, so the directory is kept,
and every object already on disk is still read and served. This bumps
the minor version to 24.3 only so that the reverse holds: an ATS older
than 24.3 refetches objects this build writes rather than resolving
their indexes against its own table. Rolling back therefore costs a
refetch of whatever was written while on 24.3, and nothing that
predates the upgrade.

Related: apache#9636

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@bneradt
bneradt force-pushed the claude/wks-cache-versioning-abbe91 branch from de45791 to d16fead Compare August 31, 2026 16:42
@bryancall
bryancall requested a review from moonchen August 31, 2026 22:08
@masaori335
masaori335 requested a balanced review from Copilot September 1, 2026 00:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants