fix: reply NOSCRIPT for a sha that only has SCRIPT FLAGS set

[vyavdoshenko]

SCRIPT FLAGS <sha> default-inserts a body-less entry into ScriptMgr::db_ and publishes its params to every thread's cache. That cache is EVALSHA's only existence guard, while ScriptMgr::Find requires a non-null body - so EVALSHA passed the guard and aborted in LoadScript (LOG(DFATAL); in release it falls through to CHECK(result == RUN_OK) and aborts too). Reachable from two clients commands, and replicated, so it also killed replicas.

Fixes #8103

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

PR Summary by Qodo

Return NOSCRIPT for SHA entries containing only SCRIPT FLAGS

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

---

AI Description

• Prevent flags-only script entries from making EVALSHA pass existence checks and abort.
• Exclude unloaded scripts from listings, snapshots, and automatic flag correction.
• Cover unknown, flushed, pre-load flags, persistence, and replication scenarios.

Diagram

graph TD
  A["SCRIPT FLAGS"] --> B{"Body loaded?"}
  B -- Yes --> C["Thread cache"] --> E{"EVALSHA cache?"}
  B -- No --> D["Pending flags"] -. No cache .-> E
  E -- Found --> F{"Body available?"}
  E -- Missing --> H["NOSCRIPT reply"]
  F -- Yes --> G["Execute script"]
  F -- No --> H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Separate pending-flags map

• ➕ Makes loaded scripts and preconfigured flags distinct states by construction.
• ➕ Prevents body-less entries from affecting future ScriptMgr consumers.
• ➖ Adds synchronization and merge logic across two maps.
• ➖ Broadens a targeted production fix and increases migration risk.

2. Check ScriptMgr on every EVALSHA

• ➕ Uses the authoritative body store for all existence checks.
• ➕ Eliminates reliance on cache presence as an existence signal.
• ➖ Introduces shared ScriptMgr locking into the EVALSHA hot path.
• ➖ Duplicates parameter lookup already optimized through thread-local caches.

Recommendation: Keep the PR’s targeted approach: publish cache entries only after a body exists, filter body-less metadata from enumeration, and retain LoadScript as a defensive failure boundary. A separate pending-flags map would provide a stronger long-term type invariant, but its added synchronization complexity is unnecessary for this focused fix.

Files changed (4) +123 / -9

Bug fix (2) +22 / -9

main_service.ccHandle missing script bodies safely during EVALSHA +12/-5

Handle missing script bodies safely during EVALSHA

• Changes LoadScript to report whether the interpreter already contains or successfully loads the requested script. EvalInternal converts missing-body and loading failures into a NOSCRIPT response instead of reaching fatal assertions.

src/server/main_service.cc

script_mgr.ccKeep flags-only entries out of executable script paths +10/-4

Keep flags-only entries out of executable script paths

• Publishes script parameters to thread caches only when a body is loaded, preserving pending flags in ScriptMgr until insertion. Excludes body-less entries from error auto-correction, SCRIPT LIST results, and snapshot enumeration.

src/server/script_mgr.cc

Tests (2) +101 / -0

dragonfly_test.ccAdd regression coverage for flags-only script entries +80/-0

Add regression coverage for flags-only script entries

• Adds tests ensuring unknown and flushed SHAs return NOSCRIPT without terminating the server. Also verifies pre-load flags are preserved after loading and body-less entries remain absent from SCRIPT LIST and persisted snapshots.

src/server/dragonfly_test.cc

replication_test.pyVerify flags-only SHAs remain safe after replication +21/-0

Verify flags-only SHAs remain safe after replication

• Adds a master-replica regression test confirming replicated SCRIPT FLAGS metadata does not make an unknown SHA executable. Both nodes must return NOSCRIPT, remain responsive, and report no listed or existing script.

tests/dragonfly/replication_test.py

[augmentcode]

🤖 Augment PR Summary

Summary: Prevents EVALSHA from treating a SCRIPT FLAGS-only SHA as a loaded script.

Changes:

• Publishes script parameters to per-thread caches only after a script body is available.
• Makes `LoadScript` report a missing body so the caller returns the standard `NOSCRIPT` response.
• Excludes flags-only entries from script listing and snapshot serialization.
• Adds unit and replication regressions for unknown SHAs, flushes, pre-load flags, and reloads.

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

[augmentcode]

Review completed. No suggestions at this time.

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

[qodo-code-review]

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

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

Code Review by Qodo

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

---

🔴 High

1. Preload flags drop defaults 🐞 Bug ≡ Correctness

Description

When SCRIPT FLAGS is issued for an unloaded sha, params are stored in db_ but not published; later,
ScriptMgr::Insert() uses db_.emplace() and does not update params if the flags-only entry already
exists, so params deduced from the body (and default_lua_flags) can be ignored. This can run scripts
with incorrect atomic/undeclared/float_as_int behavior compared to what the loaded body and defaults
specify.

Code

src/server/script_mgr.cc[R188-192]

+  // The per-thread params cache doubles as the EVALSHA existence check, so publish only scripts
+  // that are actually loaded. Flags of a not yet loaded script stay in db_ until Insert() runs.
+  if (data.body) {
+    UpdateScriptCaches(key, data);
+  }

Evidence

The PR change makes flags-only entries persist in db_ without publishing caches. Insert()
computes params from deduced/default flags but uses db_.emplace(...) and does not overwrite an
existing entry, so the pre-created entry’s params remain authoritative and the computed params are
effectively discarded.

src/server/script_mgr.cc[173-193]
src/server/script_mgr.cc[273-349]
src/server/script_mgr.cc[56-65]
src/server/dragonfly_test.cc[336-357]

Agent prompt

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

### Issue description
`SCRIPT FLAGS <sha>` on an unloaded sha creates a body-less `db_` entry. After this PR, that entry is intentionally *not* published to per-thread caches (good), which makes `Insert()` the point where the final params must be established.

However, `ScriptMgr::Insert()` currently does `db_.emplace(sha, InternalScriptData{params, nullptr});` and ignores the `inserted` boolean, so if a flags-only entry already exists, the newly computed `params` (from `default_params_` / `--!df flags=` / per-sha allowlists) are not applied.

### Issue Context
This leads to scripts loaded after pre-configuring flags potentially dropping:
- `default_lua_flags` defaults
- in-body `--!df flags=...` deductions
- other per-sha deductions done in `Insert()`

### Fix Focus Areas
- src/server/script_mgr.cc[173-198]
- src/server/script_mgr.cc[273-349]
- src/server/script_mgr.cc[56-65]

### Suggested implementation direction
1. In `ConfigCmd`, when inserting a new (previously unseen) sha, initialize the new entry’s params from `default_params_` (not `ScriptParams{}`), then apply requested flags.
2. In `Insert`, if an entry already exists and has no body, merge the newly computed `params` with the preconfigured flags instead of ignoring `params`. Given current flags are monotonic, a practical merge is:
  - `params.atomic = params.atomic && existing.atomic` (disable-atomicity should win)
  - `params.undeclared_keys |= existing.undeclared_keys`
  - `params.float_as_int |= existing.float_as_int`
  Then store merged params back into `it->second` before `UpdateScriptCaches(...)`.
3. Add/extend a unit test that covers: `default_lua_flags` set + pre-load `SCRIPT FLAGS` + `SCRIPT LOAD` and verifies the merged behavior.

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

---

🟡 Low

2. Unreachable warning misleading 🐞 Bug ◔ Observability

Description

LoadScript() logs a rate-limited WARNING claiming “cached params but no body” is unreachable, but
this state can still occur transiently (e.g. due to timing between reading the per-thread params
cache and a concurrent FlushAllScript clearing the script registry/caches). This risks confusing
operators by warning about an expected race rather than a true invariant violation.

Code

src/server/main_service.cc[R2242-2244]

+    // Unreachable: params are cached only for scripts that have a body.
+    LOG_EVERY_T(WARNING, 1) << "Script " << sha << " has cached params but no body";
+    return false;

Evidence

EvalInternal uses ServerState::GetScriptParams() (thread-local) before calling LoadScript(),
while FlushAllScript clears db_ then triggers thread-local cache clearing; that ordering can
temporarily desynchronize cache vs registry, making this condition reachable.

src/server/main_service.cc[2316-2329]
src/server/main_service.cc[2235-2245]
src/server/script_mgr.cc[387-395]
src/server/server_state.cc[280-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
`LoadScript()` logs `"cached params but no body"` with a comment calling it unreachable. Because `EvalInternal` consults the thread-local params cache before `script_mgr->Find()`, administrative operations like `SCRIPT FLUSH` can create a short window where params were read but the body is already removed.

### Issue Context
`FlushAllScript()` clears `db_` and then clears per-thread caches via `ServerState::FlushScriptCache()` on all threads; the cross-thread invalidation timing can make this state observable.

### Fix Focus Areas
- src/server/main_service.cc[2235-2245]
- src/server/main_service.cc[2316-2329]
- src/server/script_mgr.cc[387-395]
- src/server/server_state.cc[280-283]

### Suggested change
- Remove the “Unreachable” assertion/comment.
- Consider lowering severity (e.g. `VLOG(1)`/`DVLOG`) or making the message explicitly describe a stale-cache race (e.g. “stale script params cache; likely concurrent SCRIPT FLUSH”).

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

---

---

Context

✅ Tickets: 🎫 crash: main_service.cc:2241] Script <sha> not found in script mgr — SCRIPT FLAGS on an unknown sha then EVALSHA

✅ 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