Skip to content

Fix Remove Cached Eval Scripts On Engine Unregister#3503

Open
eifrah-aws wants to merge 3 commits intovalkey-io:unstablefrom
eifrah-aws:scripting-engine-crash-unload
Open

Fix Remove Cached Eval Scripts On Engine Unregister#3503
eifrah-aws wants to merge 3 commits intovalkey-io:unstablefrom
eifrah-aws:scripting-engine-crash-unload

Conversation

@eifrah-aws
Copy link
Copy Markdown
Contributor

Remove eval script cache entries that belong to a scripting engine when that engine is unregistered. This prevents the eval cache from retaining dangling engine pointers and keeps the tracked script memory in sync after engine shutdown.

The scripting engine unregister path now invokes a new eval cleanup helper, which scans the cached scripts, drops matching entries from the LRU list and dictionary, and adjusts cache memory accounting accordingly.

  • scripting engine
  • eval cache

Generated by CodeLite

Remove eval script cache entries that belong to a scripting engine when that
engine is unregistered. This prevents the eval cache from retaining dangling
engine pointers and keeps the tracked script memory in sync after engine
shutdown.

The scripting engine unregister path now invokes a new eval cleanup helper,
which scans the cached scripts, drops matching entries from the LRU list and
dictionary, and adjusts cache memory accounting accordingly.

* scripting engine
* eval cache

**Generated by CodeLite**

Signed-off-by: Eran Ifrah <eifrah@amazon.com>
@eifrah-aws eifrah-aws marked this pull request as ready for review April 14, 2026 13:03
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.43%. Comparing base (b2e0f63) to head (8c78b64).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3503      +/-   ##
============================================
- Coverage     76.45%   76.43%   -0.02%     
============================================
  Files           159      159              
  Lines         79840    79853      +13     
============================================
- Hits          61042    61036       -6     
- Misses        18798    18817      +19     
Files with missing lines Coverage Δ
src/eval.c 91.78% <100.00%> (+0.27%) ⬆️
src/scripting_engine.c 58.27% <100.00%> (+0.60%) ⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread src/eval.c Outdated
@eifrah-aws eifrah-aws requested a review from dvkashapov April 16, 2026 05:34
@enjoy-binbin enjoy-binbin requested a review from rjd15372 April 16, 2026 11:17
Copy link
Copy Markdown
Member

@dvkashapov dvkashapov left a comment

Choose a reason for hiding this comment

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

LGTM, good catch!

Copy link
Copy Markdown
Member

@rjd15372 rjd15372 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. LGTM

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.

3 participants