Skip to content

fix(storage): correct OnCacheRemovalCallback to actually delete LevelDB entries on cache eviction#1344

Open
usernametooshort wants to merge 9 commits intoprojectdiscovery:devfrom
usernametooshort:fix/cache-eviction-stale-leveldb-data
Open

fix(storage): correct OnCacheRemovalCallback to actually delete LevelDB entries on cache eviction#1344
usernametooshort wants to merge 9 commits intoprojectdiscovery:devfrom
usernametooshort:fix/cache-eviction-stale-leveldb-data

Conversation

@usernametooshort
Copy link

@usernametooshort usernametooshort commented Mar 5, 2026

Problem

Fixes #1340

Clients receive repeated Could not unmarshal interaction errors after a session
restores with the same correlation ID but gets a new AES key — because stale data
encrypted with the old AES key is still in LevelDB.

Root Cause

OnCacheRemovalCallback has two bugs that together mean the LevelDB entry is
never cleaned up when the in-memory cache evicts a correlation ID:

Bug 1 — wrong type assertion

// Before (always fails — value is *CorrelationData, not []byte)
if key, ok := value.([]byte); ok {
    _ = s.db.Delete(key, &opt.WriteOptions{})
}

value is *CorrelationData (see AddInteraction line 146). The .([]byte)
assertion always returns ok = false, so db.Delete is unreachable.

Bug 2 — variable shadowing

The shadow variable key inside the if block would hold the (wrong) value
bytes even if the assertion succeeded. The actual correlation-ID string lives
in the outer key parameter.

Fix

func (s *StorageDB) OnCacheRemovalCallback(key cache.Key, value cache.Value) {
    if !s.Options.UseDisk() {
        return
    }
    correlationID, ok := key.(string)
    if !ok {
        return
    }
    _ = s.db.Delete([]byte(correlationID), &opt.WriteOptions{})
}

Now when the cache evicts a session, the LevelDB row is deleted. A subsequent
re-registration generates a fresh AES key and starts with a clean slate — no
stale ciphertext is ever returned to the client.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed cache eviction behavior for disk-backed storage, ensuring entries are properly removed when evicted from memory rather than remaining on disk.

orrk-litt and others added 9 commits November 10, 2025 15:25
…ion-strategy

feat(server) added eviction strategy
…-1275-feature/eviction-strategy

Revert "feat(server) added eviction strategy"
Bumps [github.com/refraction-networking/utls](https://github.com/refraction-networking/utls) from 1.8.0 to 1.8.2.
- [Release notes](https://github.com/refraction-networking/utls/releases)
- [Commits](refraction-networking/utls@v1.8.0...v1.8.2)

---
updated-dependencies:
- dependency-name: github.com/refraction-networking/utls
  dependency-version: 1.8.2
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
…abot/go_modules/github.com/refraction-networking/utls-1.8.2

chore(deps): bump github.com/refraction-networking/utls from 1.8.0 to 1.8.2
…DB entries

Two bugs in OnCacheRemovalCallback prevented evicted correlation IDs from
being removed from LevelDB:

Bug 1 — wrong type assertion
  The callback received (key cache.Key, value cache.Value).  The code did
  'if key, ok := value.([]byte)' — asserting the *value* as []byte.
  Cache values are stored as *CorrelationData, so this assertion always
  fails and the db.Delete call was unreachable.

Bug 2 — wrong variable shadowing
  Even if the assertion had succeeded, the shadow variable 'key' would
  hold the value bytes, not the correlation-ID string, so the wrong key
  would have been deleted.

Effect: when a correlation ID was evicted from the in-memory cache (e.g.
after TTL expiry), its LevelDB row was never cleaned up.  If the client
then re-registered the same correlation ID, a fresh AES key was generated
and stored in the cache, but the old LevelDB row — encrypted with the
previous AES key — was returned alongside new interactions.  Decryption
with the new key produced corrupted JSON, causing the unmarshal error:
  'server.Interaction.Protocol: ReadString: invalid control character found'

Fix: assert key.(string) (correlation IDs are stored as plain strings),
guard with UseDisk(), then delete the correct row.

Fixes projectdiscovery#1340
@neo-by-projectdiscovery-dev
Copy link

neo-by-projectdiscovery-dev bot commented Mar 5, 2026

Neo - PR Security Review

No security issues found

Highlights

  • Fixes cache eviction callback to properly delete LevelDB entries when correlation IDs expire
  • Corrects type assertion from value.([]byte) to key.(string) to extract correlation ID
  • Prevents stale encrypted data from causing client-side unmarshal errors on session restoration
Hardening Notes
  • Consider logging when OnCacheRemovalCallback fails to delete a LevelDB entry (currently error is silently discarded with _ = s.db.Delete(...) at line 95) to aid debugging of storage issues
  • Add a metric counter for successful/failed cache eviction deletions to monitor LevelDB cleanup health

Comment @neo help for available commands. · Open in Neo

@coderabbitai
Copy link

coderabbitai bot commented Mar 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb08edd3-2243-4fa0-a876-ce783b35a77a

📥 Commits

Reviewing files that changed from the base of the PR and between 307efeb and 38bd467.

📒 Files selected for processing (1)
  • pkg/storage/storagedb.go

Walkthrough

The OnCacheRemovalCallback function in disk-backed storage is corrected to properly handle cache eviction. The function now validates disk usage, extracts the correlation ID from keys, and deletes corresponding LevelDB entries, replacing previous logic that incorrectly attempted to cast values and lacked proper deletion handling.

Changes

Cohort / File(s) Summary
Cache Eviction Fix
pkg/storage/storagedb.go
Reworks OnCacheRemovalCallback to properly handle disk-backed cache eviction by validating disk usage, extracting correlation ID from keys, and correctly deleting LevelDB entries. Removes faulty type assertion logic that prevented actual deletion.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A callback once broken, now healed with care,
No more stale data floating in storage air,
The correlation dances with proper ID,
LevelDB entries now deleted as they should be,
Cache eviction flows clean, the system's now fair! 📚✨

🚥 Pre-merge checks | ✅ 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 accurately describes the main fix: correcting OnCacheRemovalCallback to properly delete LevelDB entries during cache eviction, which is the primary change in the PR.
Linked Issues check ✅ Passed The PR successfully addresses the core coding requirement from issue #1340: fixing OnCacheRemovalCallback to correctly remove LevelDB entries when in-memory cache entries are evicted.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the OnCacheRemovalCallback logic in storagedb.go; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

The PR should point to the dev branch

@usernametooshort usernametooshort changed the base branch from main to dev March 9, 2026 07:01
@usernametooshort
Copy link
Author

Thanks for the feedback! I've updated the base branch to dev as requested.

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.

Interaction data corruption causes unmarshal errors on client

4 participants