Skip to content

Tep's gzip cache answers a repeated body before hashing it - #741

Merged
eddygarcas merged 1 commit into
rubys:mainfrom
namespaceMarcello:fix/tep-gzip-last-hit
Oct 11, 2026
Merged

eddygarcas merged 1 commit into
rubys:mainfrom
namespaceMarcello:fix/tep-gzip-last-hit

Conversation

@namespaceMarcello

@namespaceMarcello namespaceMarcello commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

What

Tep.gzip_cached, which gzips identity HTML on the Spinel lane, looks up the gzip by the SHA-256 of the body (#432: Spinel's Hash reads the whole key, and with no GVL the lock had to stay short). It computed that digest on every request, hits included. In a perf profile of the Spinel binary serving Campfire's older-messages page (408,174 bytes, gzip, one client; binary from adee8959, same gzip_cached), 40% of the samples were in sp_crypto_sha256_block.

The CRuby overlay's GzipCache (#488) compares the last body served with == before any key. Tep now does the same:

  • the pair (last body, its gzip) is read under GZIP_LOCK and compared outside it;
  • it is stored on a digest hit, that is, for a body seen before, and not on a miss. A page with a per-request CSRF token (the room page, search, the sidebar) is never seen twice, so storing it on a miss would copy the whole page on every request for nothing (1 MB for search, up to 7.4 MB for the sidebar in our seed); an earlier version of this branch did that, and search's median dropped about 2.5%, inside overlapping ranges. A repeated body now pays the digest once more than it would with the pair stored on a miss: miss, digest hit, then last-hit;
  • the stored body is a dup. On Spinel raw can be a shared mutable string handle (SP_BUILTIN_STRBUF), and a caller that mutated it afterwards would make == match bytes whose gzip this is not. The overlay snapshots for the same reason (last_hit_does_not_follow_a_mutated_source_string);
  • a miss runs as before, and the digest table is unchanged.

Cost: the last repeated body stays in memory (up to the largest gzipped page; 7.4 MB for a sidebar with 10,000 direct rooms in our seed), and a digest hit copies the body once. Two repeated pages that alternate (A, B, A, B) both go through the digest every time, as before, plus that copy.

Pages with a per-request token never repeat byte for byte, so they still go through the digest. That is what the overlay's splicing (85c9f67d, then #526 and #546) solves on CRuby; it is not part of this change.

Repro and test

  • Repro: the profile above, and the messages page under wrk on main (below).
  • Test: gzip_cache::tep_gzip_cached_repeats_the_last_body_without_a_digest loads tep_core.rb on CRuby and counts Digest::SHA256.hexdigest and Zlib.gzip calls. A miss keeps no copy; the same bytes again are a digest hit and become the last body; a fresh String with those bytes then runs neither; a body of the same length with one byte changed misses and leaves the last body in place; a caller that mutates its String after the call misses; the pair survives the table emptying at GZIP_CACHE_MAX; an empty body with nothing served yet is gzipped; four threads alternating three bodies of one length always get the gzip of their own body. Every answer is checked with Zlib.gunzip. On main it fails at last-hit ran the digest (3). Five wrong versions fail it too: storing raw without dup, comparing bytesize only, dropping the empty-pair guard, storing the pair on a miss, and not storing it on a digest hit.

Verification

Run locally:

  • cargo test --test gzip_cache: 14 passed, 1 ignored (JRuby).
  • The Ruby floor jobs, replicated in a Linux container on this branch: build, store-check, unit (4,870 passed, 217 ignored), compare-ruby, the Campfire suite (417 tests, 69 files), campfire-compare and db-differential on the Ruby target. All green.
  • Measured on 91233b2d; the branch is the same patch rebased on 0ffebca2.
  • Campfire at the CI pin (32b4144), built by scripts/build-campfire-archive from main (91233b2d) and from 91233b2d with this change, with the same Spinel (2afc69c54). Both binaries ran in the same image on 4 CPUs (AMD Ryzen 9 7940HX, Docker Desktop on Windows 11), one at a time, against the same database (2M messages, 1M of them in room 1).
    • Same identity bodies: room 454,655 bytes, older messages 408,174, search 1,046,233, sidebar 1,498,697 and 7,377,805; same md5 once the per-session CSRF values are masked.
    • Every gzip response inflates to the identity body. Three gzip requests of the messages page in one session return the same bytes (39,008, one md5): the miss, the digest hit and the last-hit answer are one gzip.

On our machine, wrk -t2 -c16 on 4 other CPUs, gzip, 4 s discarded and 10 s measured, 3 rounds in rotated order, median (min–max):

main this PR
older messages (no token, repeats) 2,355.7 req/s (2,345.1–2,365.3) 7,030.7 req/s (6,915.6–7,116.7) ×2.98
room (CSRF token, never repeats) 704.5 req/s (701.8–708.9) 703.2 req/s (694.0–708.4) within noise
search (CSRF token, never repeats) 374.6 req/s (371.2–375.7) 373.7 req/s (362.5–377.2) within noise

CPU per request on the messages page goes from 1.70 ms to 0.57 ms (4 CPUs / req/s). The earlier version that also stored the pair on a miss was measured in a separate series, with another job on the machine's other CPUs: messages ×2.88, search 355.0 → 346.1 req/s, room unchanged.

Not run: hosted CI; compare-ruby-next; the Spinel and Campfire CI jobs, apart from the Campfire build above. The thread test runs on CRuby, under the GVL: on Spinel the pair is still written and read under one lock and the snapshot is never mutated, but no Spinel run exercised the race.

🤖 Generated with Claude Code

Written by Claude Code (AI assistant) on behalf of @namespaceMarcello, who directs this work.

Summary by CodeRabbit

  • Performance
    • Repeated requests to compress identical content can reuse the existing compressed result more quickly.
    • Compression continues to produce correct results when content changes, including when source text is modified or requests run concurrently.

Tep.gzip_cached computed SHA-256 of the whole identity body on every
request, hits included, to find the cached gzip. In a perf profile of
the Spinel binary serving Campfire's older-messages page (408 KB, gzip),
40% of the samples were in that digest.

The CRuby overlay's GzipCache (rubys#488) compares the last body served with
`==` before any key, and Tep now does the same. The pair (last body,
its gzip) is stored on a digest hit, a body seen before, and not on a
miss: a page with a per-request CSRF token never repeats, so copying it
would be wasted. It is read under the lock and compared outside it.
The stored body is a `dup`: on Spinel a String can be a shared mutable
buffer, and a caller that mutated it afterwards would otherwise make
`==` match bytes whose gzip this is not. A miss runs as before, and the
digest-keyed table is unchanged (SHA-256, rubys#432).

Test: tests/gzip_cache.rs
tep_gzip_cached_repeats_the_last_body_without_a_digest counts digests
and gzips: a miss keeps no copy, a digest hit becomes the last body, a
repeat of it runs neither, a same-length body misses and leaves the last
body in place, a mutated source misses, and the pair survives the table
emptying at GZIP_CACHE_MAX.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FpReMQmF4AACf5cmD9N6Cq
@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8037866d-1914-478f-92dc-92ea2fdde135

📥 Commits

Reviewing files that changed from the base of the PR and between 0ffebca and 005c2c6.


📒 Files selected for processing (2)
  • runtime/spinel/tep/tep_core.rb
  • tests/gzip_cache.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.



📝 Walkthrough

Walkthrough

Tep.gzip_cached now checks its most recently matched raw body and gzip result before computing a digest. Ruby integration tests cover cache paths, body mutation, cache capacity, and concurrent requests.

Changes

Gzip cache fast path

Layer / File(s) Summary
Track and reuse the last matched body
runtime/spinel/tep/tep_core.rb
gzip_cached checks the saved body before hashing. On a digest-cache hit, it saves a duplicate of the body with the cached gzip result.
Validate cache paths and concurrent requests
tests/gzip_cache.rs
Ruby integration tests check cache misses and hits, same-length different bodies, mutated source strings, a full digest table, and concurrent requests.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: rubys


Merge Risk

Merge Risk: ⚪ Minimal · up to 005c2

No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 005c2

The optimization preserves the existing response-compression controls, and no introduced attack path was established. It retains one additional response-body snapshot, while independent-copy and concurrent-mutation behavior in the production runtime remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected security scope is eligible inline responses sharing Tep's module-level cache within a process. Request Accept-Encoding participates in eligibility, while reuse is selected by response content rather than tenant identity. The new path requires equality with the saved body; the inspected code does not establish reuse of another request's different body or increased service privileges.

Security Findings and Attack Paths

  • inferred — Returning shared mutable gzip objects is pre-existing behavior. The new last-body reference can keep such an object reusable after digest-table eviction, but the inspected production writers only measure and pass the compressed body to socket output. Without an established mutating caller or verified native mutation, this lifetime extension is not retained as an introduced cache-poisoning finding.

Trust Boundaries and Controls

  • observed — The existing caller excludes streaming, WebSocket upgrades, files, HEAD, bodyless statuses, small bodies, already encoded responses, and specified compressed or binary media types. Accept-Encoding negotiation still precedes gzip_cached. Bypassing SHA-256 on a last-body match therefore does not bypass these response controls.

Hardening Proposals

  • proposed — Establish an explicit ownership contract for input and returned gzip strings, supported by production-runtime coverage of independent duplication, concurrent input mutation, and returned-value mutation across eviction. This would resolve the remaining response-identity uncertainty rather than presume a vulnerability.



Pre-merge checks | Passed 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: serving a repeated body from the gzip cache before hashing.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.


✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR


  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@eddygarcas
eddygarcas merged commit 822a0b7 into rubys:main Oct 11, 2026
39 checks passed
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