Skip to content

feat: saveUserHistory refactor - #3582

Open
stevekaplan123 wants to merge 4 commits into
masterfrom
bug/sc-40653/improve-user-history-cleanup-mechanism
Open

feat: saveUserHistory refactor#3582
stevekaplan123 wants to merge 4 commits into
masterfrom
bug/sc-40653/improve-user-history-cleanup-mechanism

Conversation

@stevekaplan123

@stevekaplan123 stevekaplan123 commented Aug 4, 2026

Copy link
Copy Markdown
Member

Description

When you read while logged in, your reading history is saved in the
database. When you're logged out there's no account to attach it to, so it's
stored in a browser cookie named user_history instead. The browser sends that
cookie back to us on every single request to the domain.

Two separate size limits apply, and it matters that they're different:

  1. Per cookie — browsers cap any one cookie at about 4kB.
  2. All cookies combined — nginx caps it at 8kb.

Overrunning limit 2 returns 400 Request Header Or Cookie Too Large from nginx before hitting Django.

The old code tried to keep the cookie small like this: write the entire history,
immediately read the cookie back, and if what came back was shorter than what was
written, conclude the browser had rejected it and retry with fewer entries.

That only ever detects limit 1. user_history can sit comfortably under 4kB —
so the browser accepts it and the read-back looks fine — while still pushing the
combined header over nginx's 8kB limit. The check is blind to the failure we
actually hit.

Code Changes

Make the value small before writing it, instead of writing and then reacting.

A new helper walks the history newest-first and keeps the longest run that fits
inside a byte budget: _trimUserHistoryForCookie. The write path in saveUserHistory becomes three lines, with no read-back and no
retry. Since the value can no longer exceed the budget, the browser never rejects it and
there's nothing left to detect afterward.

Notes for reviewers

Why a byte budget instead of "keep the last 20 refs". The ticket suggested
capping at a fixed number of refs, but history entries aren't a uniform size. Twenty entries is already ~8kB — double the browser's per-cookie limit and at
nginx's header limit. So a count doesn't bound the cookie at all; it only bounds
it if every entry happens to be small. A byte budget bounds it directly. At
3000 bytes you get roughly 7–11 entries in practice.

Why 3000 specifically. It leaves margin under the browser's ~4kB per-cookie
limit, and leaves about 5kB of nginx's 8kB header for every other cookie plus
User-Agent, Referer, and the request line. It's a single constant if we want
to tune it later.

This is really tricky to test in a browser so I wrote a Playwright test to save 40 history items and then checked that only the last few items were still in the history cookie and that the size of the cookie was roughly 3KB.

@stevekaplan123
stevekaplan123 marked this pull request as draft August 4, 2026 18:45
@gitvelocity-reviewer

gitvelocity-reviewer Bot commented Aug 4, 2026

Copy link
Copy Markdown

📊 Code Quality Score: 16/100

Base Score 41 × ESF 0.4 = 16.4, rounded to 16

Category Score Factors
🔭 Scope 7/20 Two files, single subsystem (anonymous user cookie management). Fixes a real production bug causing nginx 400 errors. No new endpoints or external integrations.
🏗️ Architecture 6/20 Extracts trimming logic into a named, testable function. Removes flawed write-then-check pattern. Clean separation of concerns within existing module.
⚙️ Implementation 8/20 Correct URL-encoded byte budget approach handles Hebrew character expansion (6 bytes/letter). Fixes subtle race condition in old code. O(n²) re-serialization acceptable given bounded input size (~20-30 items at budget).
⚠️ Risk 6/20 Fixes production nginx 400 errors for anonymous users with large history. Straightforward and easily reversible. No database, auth, or external service changes.
✅ Quality 12/15 8 test cases covering empty input, fits-within-budget, short refs, long Hebrew refs with version titles, ordering, tight budget boundary, and oversized single item. Good inline documentation explaining nginx header buffer constraint and budget rationale.
🔒 Perf / Security 2/5 Comment explicitly documents nginx large_client_header_buffers constraint and budget derivation. No security concerns introduced.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

Scored by GitVelocity · How are scores calculated?

@stevekaplan123
stevekaplan123 marked this pull request as ready for review August 4, 2026 19:02
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.

1 participant