fix: stop SCRIPT FLUSH from deadlocking against a borrowed interpreter

[vyavdoshenko]

InterpreterManager::Reset() blocked until every borrowed interpreter was returned, so SCRIPT FLUSH inherited every lock its holders were waiting on. Reset no longer blocks: idle interpreters are destroyed inline, borrowed ones are retired and destroyed by whoever returns them.

Three shapes, all reproduced on a debug build:

1. MULTI; EVAL "return 1" 0; SCRIPT FLUSH; EXEC - EXEC pre-borrows an interpreter and returns it only after the body ran, so it waits for itself. SCRIPT is NO_KEY_TRANSACTIONAL, so the EXEC runs as a global transaction: afterwards even GET hangs and only PING answers.
2. Concurrent EVAL + SCRIPT FLUSH - the flusher holds ScriptMgr::mu_ while waiting for an interpreter whose holder is blocked on that same mutex. All scripting wedges permanently.
3. No pre-borrow at all - a keyed EVAL holds an interpreter behind the global lock that a concurrent MULTI; SCRIPT FLUSH; EXEC holds.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Prevent SCRIPT FLUSH deadlocks with non-blocking interpreter reset

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

---

AI Description

• Makes interpreter resets non-blocking by retiring borrowed Lua states until their return.
• Revalidates script registration when pre-flush interpreters retain compiled functions.
• Adds transactional SCRIPT FLUSH regression coverage for deadlocks and server liveness.

Diagram

sequenceDiagram
  actor Client
  participant Exec as EXEC
  participant SM as Script Manager
  participant SS as Server State
  participant Pool as Interpreter Pool
  participant Retired as Retired Map
  Client->>Exec: Run transaction
  Exec->>Pool: Borrow interpreter
  Exec->>SM: SCRIPT FLUSH
  SM->>SS: Clear script cache
  SS->>Pool: Reset pool
  Pool->>Retired: Retain borrowed state
  Pool-->>SS: Return immediately
  Exec->>SM: EVAL after flush
  SM->>SS: Revalidate script params
  Exec->>Pool: Return interpreter
  Pool->>Retired: Destroy retired state
  Exec-->>Client: Return results

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reference-counted interpreter leases

• ➕ Encapsulates lifetime ownership in the borrow handle
• ➕ Naturally destroys retired states after the final lease ends
• ➖ Requires changing every borrow and return call site
• ➖ Adds shared-ownership overhead and broader API churn

2. Generation-based pool retirement

• ➕ Can retire an entire pool generation with an inexpensive reset
• ➕ Separates pre-flush and post-flush interpreter populations clearly
• ➖ Requires generation-aware leases or outstanding-borrow counters
• ➖ Introduces more pool state and reclamation complexity

3. Release locks before blocking reset

• ➕ Preserves the previous contiguous pool implementation
• ➕ Could avoid some cross-fiber lock cycles
• ➖ Does not solve EXEC waiting for its own borrowed interpreter
• ➖ Requires fragile lock choreography across transaction paths

Recommendation: The PR’s explicit live-versus-retired ownership model is the best fit for the bounded, thread-local pool. It fixes both self-deadlock and cross-lock deadlock without broad lease API changes; stable unique_ptr-backed addresses also avoid unsafe Interpreter moves.

Files changed (5) +92 / -42

Bug fix (3) +63 / -41

interpreter.ccRetire borrowed interpreters during non-blocking resets +42/-30

Retire borrowed interpreters during non-blocking resets

• Reworks pool allocation around stable unique_ptr ownership and allows Get() to repopulate an emptied pool. Reset() destroys idle interpreters immediately, moves borrowed instances into retired ownership, and lets Return() destroy them without blocking the flusher.

src/core/interpreter.cc

interpreter.hDefine stable interpreter ownership and retirement state +18/-10

Define stable interpreter ownership and retirement state

• Makes Interpreter non-movable because Lua retains pointers to its owning object. Updates InterpreterManager with a fixed capacity, unique ownership, retired-interpreter tracking, and non-blocking reset semantics.

src/core/interpreter.h

script_mgr.ccRevalidate scripts retained by pre-flush interpreters +3/-1

Revalidate scripts retained by pre-flush interpreters

• Requires both a compiled interpreter function and registered server-side script parameters before treating a script as already loaded. This prevents borrowed interpreters surviving SCRIPT FLUSH from bypassing script re-registration.

src/server/script_mgr.cc

Tests (1) +28 / -0

eval_test.pyCover SCRIPT FLUSH inside transactional EVAL execution +28/-0

Cover SCRIPT FLUSH inside transactional EVAL execution

• Adds an asynchronous regression test that executes EVAL, SCRIPT FLUSH, and EVAL in one transaction. Timeouts detect deadlock, force-kill wedged servers, and verify ordinary writes remain responsive afterward.

tests/dragonfly/eval_test.py

Documentation (1) +1 / -1

main_service.ccDocument concurrent script lookup invalidation +1/-1

Document concurrent script lookup invalidation

• Clarifies that SCRIPT FLUSH may remove a script body between cached-parameter validation and the subsequent script lookup.

src/server/main_service.cc

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

---

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo

[augmentcode]

🤖 Augment PR Summary

Summary: Prevent SCRIPT FLUSH from waiting on Lua interpreters borrowed by running scripts.
Changes:

• Refactor the interpreter pool to keep stable heap-owned interpreters and retire borrowed instances on reset.
• Destroy idle instances during reset and destroy retired instances when their holder returns them.
• Re-register scripts when a retained interpreter still has a compiled function after a flush.
• Handle the cache/body lookup race during concurrent script flushing.
• Add an integration regression test for `EVAL`, `SCRIPT FLUSH`, and `EXEC` deadlock behavior.

Technical Notes: Pool waiters are notified after a reset so new interpreter instances may be created immediately.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

---

🔴 High

1. Destruction leaks memory accounting 🐞 Bug ◔ Observability

Description

Retired and idle interpreters are destroyed without applying the allocator deltas produced by
lua_close(), so used_memory_lua remains overstated after every reset. The stale total can also
keep lua_mem_gc_threshold exceeded, forcing unnecessary full garbage collections on later
interpreter returns.

Code

src/core/interpreter.cc[R1539-1540]

+  if (doomed) {
+    retired_.erase(retired_it);  // lua_close runs here, on this manager's own thread.

Relevance

●●● Strong

Concrete stale exported-memory accounting after destruction; recent history accepts
allocator-accounting correctness fixes.

PR-#6285
PR-#6094

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The allocator subtracts every Lua free from the interpreter's signed counter, and the destructor
invokes lua_close(). Return() drains and records the counter before erasing a retired
interpreter, while Reset() clears idle interpreter ownership without any drain; the discarded
negative deltas therefore never reduce the exported unsigned Lua-memory total.

src/core/interpreter.cc[757-776]
src/core/interpreter.cc[842-846]
src/core/interpreter.cc[1521-1541]
src/core/interpreter.cc[1553-1560]
src/server/metrics.cc[380-398]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Interpreter destruction runs `lua_close()` after the last `TakeUsedBytes()` call, discarding the negative allocator delta generated while Lua frees its state. Idle interpreters cleared by `Reset()` bypass accounting entirely, leaving Lua memory metrics permanently inflated and triggering unnecessary GC.

## Issue Context
The Lua allocator records signed allocation deltas in `Interpreter::used_bytes_`. Destruction must close the Lua state while the object is still available, drain the resulting signed delta, and safely apply it to the unsigned thread-local total; the same operation is required for both retired and idle interpreters.

## Fix Focus Areas
- src/core/interpreter.cc[842-846]
- src/core/interpreter.cc[1521-1541]
- src/core/interpreter.cc[1553-1560]
- src/core/interpreter.h[99-101]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

🟠 Medium

2. Reset bypasses pool capacity 🐞 Bug ☼ Reliability

Description

Get() limits only storage_ while Reset() moves borrowed interpreters into retired_ and
clears storage_, allowing another full generation to be allocated although the prior generation
remains alive. Repeated SCRIPT FLUSH operations overlapping long-lived borrowers can therefore
accumulate interpreter generations and exceed the configured per-thread capacity without bound until
those borrowers return.

Code

src/core/interpreter.cc[R1494-1495]

+Interpreter* InterpreterManager::Get() {
+  bool blocked = waker_.await([this]() { return !available_.empty() || storage_.size() < num_; });

Relevance

●●● Strong

Concrete capacity invariant violation; accepted history favors fixing lifecycle and accounting bugs
in this subsystem.

PR-#7242
PR-#6094

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The wait predicate permits allocation whenever storage_.size() < num_, but it does not include
retired_.size(). Reset moves every borrowed object to retired_, clears storage_, and wakes
getters, while retired objects remain alive until their borrowers return; each overlapping flush can
thus open capacity for another generation.

src/core/interpreter.cc[1494-1501]
src/core/interpreter.cc[1513-1518]
src/core/interpreter.cc[1539-1545]
src/core/interpreter.cc[1551-1562]
src/server/script_mgr.cc[389-397]
src/server/server_state.cc[269-283]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The pool's capacity predicate ignores live interpreters held in `retired_`. Every reset consequently permits another `num_` allocations, so repeated flushes can accumulate multiple live interpreter generations.

## Issue Context
Preserve the nonblocking behavior of `Reset()`, but account for both active and retired objects when enforcing capacity. If `Get()` can wait for retired objects to be returned, the doomed-return path must notify waiters after destroying one.

## Fix Focus Areas
- src/core/interpreter.cc[1494-1500]
- src/core/interpreter.cc[1539-1545]
- src/core/interpreter.cc[1553-1562]
- src/core/interpreter.h[223-230]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

---

Context sources

✅ Cross-repo context — repo relationships

_{ⓘ  1 issues published inline · 2 in summary}

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo