server: Shut down faster by releasing db slices instead of reset

[abhijat]

When we are shutting down we do not need to walk every single object in the db and run its destructor, the OS will reclaim the heap memory anyway. To avoid wasting time on that a special flag is added. It releases the pointer instead, leaking memory, but it will be reclaimed soon by the OS.

Recently slow shutdown watchdog triggers have increased. That might be caused due to many different issues, but releasing values will buy more time in a shutdown situation especially for large data sets.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

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

PR Summary by Qodo

Accelerate server shutdown by releasing database slices

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

---

AI Description

• Adds an opt-in fast-clear path that releases database slices during process shutdown.
• Preserves destructor-based cleanup by default for tests and non-terminal callers.
• Avoids shutdown watchdog pressure by leaving bulk heap reclamation to the OS.

Diagram

sequenceDiagram
  participant Main as Server Main
  participant Service
  participant Shards as Shard Set
  participant NS as Namespaces
  participant Async as Async Deleter
  participant DB as DB Slices
  participant OS as Operating System
  Main->>Service: Shutdown with fast clear
  Service->>Shards: Propagate fast clear
  Shards->>NS: Clear namespaces
  NS->>Async: Discard delete queue
  loop Every shard slice
    NS->>DB: Release ownership
  end
  Shards-->>Service: Shutdown complete
  Service-->>Main: Shutdown complete
  Main->>OS: Exit process
  OS-->>DB: Reclaim heap

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Bounded incremental destruction

• ➕ Retains normal destructor semantics.
• ➕ Avoids intentional leaks in embedded or restartable deployments.
• ➖ Cleanup remains proportional to dataset size.
• ➖ Large datasets could still trigger the shutdown watchdog.

2. Explicit allocator heap teardown

• ➕ Can reclaim shard allocations in bulk.
• ➕ Avoids walking individual database objects.
• ➖ Couples shutdown behavior to the configured allocator.
• ➖ Introduces strict ordering requirements for remaining thread-local objects.

3. Abandon only large containers

• ➕ Allows DbSlice destructors to run required non-memory cleanup.
• ➕ Limits intentional leakage to known expensive structures.
• ➖ Requires deeper knowledge of every owned data structure.
• ➖ May miss other expensive destructor paths and provide less predictable latency.

Recommendation: The PR's opt-in ownership-release approach is appropriate for terminal process shutdown because it provides predictable latency while retaining normal cleanup as the default. Reviewers should confirm that all non-memory resources are closed before fast clearing and that no DbSlice destructor side effects are required after namespace cleanup.

Files changed (9) +31 / -11

Enhancement (6) +11 / -7

db_slice.hDeclare DbSlice thread-local shutdown API +3/-0

Declare DbSlice thread-local shutdown API

• Adds a static API for discarding pending shard-local asynchronous deletions without destroying their queued objects.

src/server/db_slice.h

engine_shard_set.ccPropagate fast clearing through shard shutdown +2/-2

Propagate fast clearing through shard shutdown

• Accepts the fast-clear setting and forwards it to namespace cleanup before destroying engine-shard thread-local state.

src/server/engine_shard_set.cc

engine_shard_set.hAdd optional fast-clear shard shutdown parameter +1/-1

Add optional fast-clear shard shutdown parameter

• Extends the shard-set shutdown interface with a default-disabled fast-clear option, preserving existing caller behavior.

src/server/engine_shard_set.h

main_service.ccForward cleanup mode during service shutdown +2/-2

Forward cleanup mode during service shutdown

• Extends service shutdown to propagate the selected cleanup mode into the engine shard set.

src/server/main_service.cc

main_service.hExpose configurable service shutdown cleanup +1/-1

Expose configurable service shutdown cleanup

• Adds a default-disabled fast-clear parameter to the public service shutdown API.

src/server/main_service.h

namespaces.hAdd optional fast namespace cleanup mode +2/-1

Add optional fast namespace cleanup mode

• Extends 'Namespaces::Clear' with a default-disabled fast-clear parameter so tests and ordinary teardown retain destructor-based cleanup.

src/server/namespaces.h

Bug fix (2) +16 / -4

dfly_main.ccEnable fast clearing for terminal server shutdown +3/-1

Enable fast clearing for terminal server shutdown

• Passes an explicit fast-clear setting when the main server process shuts down, activating the optimized ownership-release path.

src/server/dfly_main.cc

namespaces.ccRelease database slices during fast namespace clearing +13/-3

Release database slices during fast namespace clearing

• Adds a fast cleanup branch that discards each shard's pending asynchronous deletions and releases DbSlice unique pointers instead of resetting them. Normal cleanup continues to invoke destructors when fast clearing is disabled.

src/server/namespaces.cc

Refactor (1) +4 / -0

db_slice.ccExpose shard-local asynchronous deletion shutdown +4/-0

Expose shard-local asynchronous deletion shutdown

• Moves asynchronous deleter shutdown behind 'DbSlice::ShutdownThreadLocal()' and continues invoking it from the destructor. This lets namespace teardown discard queued deletions even when DbSlice destructors are intentionally skipped.

src/server/db_slice.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 route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo

[Copilot]

🟢 Approval recommended

The fast path is confined to final process shutdown while existing test teardown retains deterministic destruction.

Pull request overview

Accelerates production shutdown by intentionally leaking database slices for OS reclamation while retaining normal cleanup for tests.

Changes:

• Adds optional fast-clear propagation through shutdown APIs.
• Releases DbSlice ownership and clears pending asynchronous deletions.
• Enables fast clearing in the production shutdown path.

File summaries

File	Description
src/server/namespaces.h	Adds the fast-clear option.
src/server/namespaces.cc	Implements slice release during fast shutdown.
src/server/main_service.h	Extends the service shutdown API.
src/server/main_service.cc	Propagates fast clearing to shards.
src/server/engine_shard_set.h	Extends shard-set shutdown API.
src/server/engine_shard_set.cc	Passes fast clearing to namespaces.
src/server/dfly_main.cc	Enables fast production shutdown.
src/server/db_slice.h	Exposes thread-local deleter shutdown.
src/server/db_slice.cc	Reuses asynchronous-deleter cleanup.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 0
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[augmentcode]

🤖 Augment PR Summary

Summary: This PR speeds normal server shutdown by intentionally bypassing destruction of per-shard database slices.
Changes:

• Adds a `fast_clear` shutdown path from `RunEngine` through `Service`, `EngineShardSet`, and `Namespaces`.
• Releases `DbSlice` ownership instead of resetting each slice, leaving process-exit cleanup to the OS.
• Explicitly discards shard-local asynchronous-deletion queue nodes before slices are released.
• Preserves the existing destructor-based cleanup path for tests and other non-fast shutdown callers.

Technical Notes: Tiered storage and periodic shard work are stopped before namespace cleanup, maintaining the established teardown ordering.

_{🤖 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.

[kostasrim]

I wrote this on the issue but this seems like it's the actual root cause for the "deadlock".

It still to me a mystery why we didn't have a CPU spike in some cases or even perf files but the later incidents actually do have those 😮‍💨

Even though in principle we are correct to exit and let the os reclaim the memory, why not instead use mi_heap_destroy ? It would stop iterating over the whole object space and free all the pages handled by the shard heap. So for O(total_objects) which is potentially huge we would drop down to O(total_mimalloc_pages) which should be handful.

And writing this we can make the same optmization for flushall! Instead of walking down every object destroy the heap in one go and create a new one. This will actually make flushall fast as well

I guess for flushall we should be careful to reinitialize the threadlocals so it might actually be harder in practice but maybe worth looking into ?

P.s. I am not necessarily in favour of using mi_heap_destroy over release && leak memory because we shutdown the instance afterwords. However that might not be true for all environments and I really don't know which one is better because in principle leaking memory to the os and have it reclaiming it is "ok" 🤷 Obviously the downside of what I write here is O(pages) destruct vs O(1). The question though is can O(pages) be slow such that we end up with the same issue ? If the answer is no then it might be worth following to be "in principle correct"

P.s. I am happy to approve as is, just thinking out loud here

[kostasrim]

I wrote this on the issue but this seems like it's the actual root cause for the "deadlock".

It still to me a mystery why we didn't have a CPU spike in some cases or even perf files but the later incidents actually do have those 😮‍💨

Even though in principle we are correct to exit and let the os reclaim the memory, why not instead use mi_heap_destroy ? It would stop iterating over the whole object space and free all the pages handled by the shard heap. So for O(total_objects) which is potentially huge we would drop down to O(total_mimalloc_pages) which should be handful.

And writing this we can make the same optmization for flushall! Instead of walking down every object destroy the heap in one go and create a new one. This will actually make flushall fast as well

I guess for flushall we should be careful to reinitialize the threadlocals so it might actually be harder in practice but maybe worth looking into ?

P.s. I am not necessarily in favour of using mi_heap_destroy over release && leak memory because we shutdown the instance afterwords. However that might not be true for all environments and I really don't know which one is better because in principle leaking memory to the os and have it reclaiming it is "ok" 🤷 Obviously the downside of what I write here is O(pages) destruct vs O(1). The question though is can O(pages) be slow such that we end up with the same issue ? If the answer is no then it might be worth following to be "in principle correct"

P.s. I am happy to approve as is, just thinking out loud here

Hmm it should be ok even for large datastores, a 500gb datastore with only MI_SMALL_PAGES==64kb should take milliseconds. I am happy to try this if you want me to, then we can be sure and that way we can have one clean path for cleaning up backing heaps

[abhijat]

Even though in principle we are correct to exit and let the os reclaim the memory, why not instead use mi_heap_destroy ?

yes sure, if you can try it out and it works in terms of the time taken, I think it should be fine to go that way.

[kostasrim]

I prototyped it. The test I added times out when it tries to shut down dragonfly. It's instance (a few ms) to shutdown by reaping the heap pages instead

[kostasrim]

@abhijat if you want you can try it out/stress it more 😄

I guess we won't commit this test, it takes ages but I am leaving it here for reference for now

[Copilot]

🟡 Changes recommended

The resource-intensive manual benchmark should not run automatically in the large test suite.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Bulk heap destruction intentionally bypasses a broad object graph’s destructors and requires final human validation of allocator ownership and shutdown ordering.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The bulk allocator reset leaves stale zmalloc memory accounting when reinitialized.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Shutdown crashes, allocator correctness gaps, disabled tests, and unsafe CI privileges remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (5)

src/server/engine_shard.cc:577

• mi_heap_destroy is still the crash point in the replica-shutdown paths being debugged here: the latest run reports repeated failures in mi_free_generic_mt. Keep the destructor-skipping behavior, but free shard_ and use mi_heap_delete so mimalloc can safely transfer pages that still participate in ownership bookkeeping.

  mi_heap_destroy(tlh);

src/redis/zmalloc_mi.c:233

• Bulk heap teardown bypasses zfree, so nulling only the heap leaves zmalloc_used_memory_tl carrying the discarded database usage. A later lifecycle on the same thread then starts with inflated memory/OOM accounting; reset the counter here too.

void reset_zmalloc_threadlocal(void) {
  zmalloc_heap = NULL;
}

src/redis/zmalloc.h:153

• REDIS_ZMALLOC_MI=OFF selects zmalloc.c, which has no definition for this new API, while EngineShard calls it unconditionally. That supported configuration will fail to link; add the no-op fallback implementation there as well.

void reset_zmalloc_threadlocal(void);

.github/workflows/ci.yml:154

• The added && false disables every C++ unit-test variant, and the regression step is also narrowed to five cases. Restore the normal conditions and remove the temporary test filter before merging this allocator/shutdown change.

        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

src/core/topk.cc:87

• With unequal memory resources, this vector move constructs new HeapItems via the implicit move constructor, so their PMR strings retain other's resource. The destination can then hold dangling allocator pointers; rebuild items with the destination resource or make HeapItem allocator-aware.

    custom_decay_table_ = std::move(other.custom_decay_table_);
    counters_ = std::move(other.counters_);
    min_heap_ = std::move(other.min_heap_);

• Files reviewed: 30/30 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The heap-destruction path remains crash-prone, standard C++ tests are disabled, and the regression invocation exits before pytest.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 31/31 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

The shutdown path still crashes replica tests, has allocator lifecycle defects, and disables standard CI coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

src/server/engine_shard.cc:577

• The focused regression run at this head's shutdown implementation fails multiple replica tests with SIGFPE immediately after mi_heap_destroy starts. Restore freeing shard_ and use mi_heap_delete(tlh) so pages that still participate in mimalloc ownership bookkeeping are transferred safely.

  mi_heap_destroy(tlh);

src/redis/zmalloc_mi.c:233

• Bulk heap destruction bypasses zfree, so this leaves zmalloc_used_memory_tl carrying usage from the discarded heap. A later lifecycle on the same thread starts with stale memory/OOM accounting; reset the counter here too.

void reset_zmalloc_threadlocal(void) {
  zmalloc_heap = NULL;
}

src/redis/zmalloc.h:153

• REDIS_ZMALLOC_MI=OFF selects zmalloc.c, which has no definition for this new API, while EngineShard::DestroyThreadLocal calls it unconditionally. Add a no-op fallback implementation so that supported configuration still links.

// Drops the thread-local heap pointer. Call before the underlying heap is destroyed
// and before this thread's zmalloc/zfree could be called again (a following
// init_zmalloc_threadlocal() re-arms it for reuse, e.g. across test cycles).
void reset_zmalloc_threadlocal(void);

.github/workflows/ci.yml:87

• This temporary workflow rewrite disables all four C++ unit-test variants and narrows regression coverage to five tests, so the allocator/shutdown changes no longer receive the repository's standard validation. Revert the experimental CI configuration before merge.

    # TEMPORARY EXPERIMENT (fast-shutdown SIGFPE investigation): no job-level container:.
    # kernel.core_pattern is host-kernel-wide; Docker containers can't write it themselves
    # even with extra capabilities (verified: --cap-add SYS_ADMIN alone did not work). So
    # this sets it on the bare runner VM, then docker run's the build/test container
    # manually so the setting is already in effect before anything inside it can crash.
    # Drops ccache/deps-cache (those composite-action steps only work under job-level
    # container:) -- intentional, to be reverted after this experiment.

• Files reviewed: 31/31 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The shutdown path remains crash-prone, breaks the fallback allocator build, retains stale accounting, and disables C++ CI coverage.

Review details

Suppressed comments (4)

src/redis/zmalloc.h:153

• reset_zmalloc_threadlocal() is called unconditionally, but REDIS_ZMALLOC_MI=OFF selects zmalloc.c, which has no definition for this new API, so that supported configuration will fail to link. Add the corresponding fallback implementation in zmalloc.c.

void reset_zmalloc_threadlocal(void);

src/redis/zmalloc_mi.c:233

• Bulk heap destruction bypasses every zfree, so this thread-local counter retains the bytes from the destroyed heap. Reset it here as well; otherwise a later same-thread initialization reports stale memory usage and may make OOM decisions from the previous server instance.

void reset_zmalloc_threadlocal(void) {
  zmalloc_heap = NULL;
}

.github/workflows/ci.yml:184

• The forced && false disables the entire C++ test suite for every PR matrix entry; the same guard also disables the epoll and both cluster variants below. Remove all four forced-false guards before merge so these allocator and shutdown changes retain required regression coverage.

        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

src/server/engine_shard.cc:577

• This switch to mi_heap_destroy is the fast-shutdown path already producing replica-shutdown/SIGFPE failures, as reflected by the temporary crash-instrumentation changes in this PR. Keep the stable mi_free(shard_) plus mi_heap_delete(tlh) teardown until the allocator crash is fixed rather than merging a shutdown path known to crash.

  mi_heap_destroy(tlh);

• Files reviewed: 31/31 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Heap teardown has correctness and build issues, and the normal CI coverage is disabled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

src/server/engine_shard.cc:577

• mi_heap_destroy invalidates every outstanding allocation immediately, but shutdown still performs thread-local and connection cleanup after shard_set->Shutdown(); this fast path already correlates with the replica shutdown crashes called out in the workflow. Keep the pointer-release optimization, but free shard_ and use mi_heap_delete(tlh) so late owners can safely release their allocations.

  mi_heap_destroy(tlh);

src/redis/zmalloc_mi.c:233

• Bulk heap destruction bypasses every zfree, so leaving zmalloc_used_memory_tl unchanged carries the destroyed heap's usage into a later initialization on the same thread and corrupts memory accounting. Reset the counter together with the heap pointer.

void reset_zmalloc_threadlocal(void) {
  zmalloc_heap = NULL;
}

src/redis/zmalloc.h:153

• This new API is called unconditionally, but the REDIS_ZMALLOC_MI=OFF build selects zmalloc.c, which has no reset_zmalloc_threadlocal definition, causing a link failure. Add the corresponding fallback implementation in zmalloc.c.

// Drops the thread-local heap pointer. Call before the underlying heap is destroyed
// and before this thread's zmalloc/zfree could be called again (a following
// init_zmalloc_threadlocal() re-arms it for reuse, e.g. across test cycles).
void reset_zmalloc_threadlocal(void);

.github/workflows/ci.yml:154

• The hard-coded && false disables all four C++ test suites for every PR matrix entry, while the regression job below is also restricted to five cases. Restore the original guards and full regression selection before merging so this allocator and teardown rewrite receives the repository's normal validation.

      - name: C++ Unit Tests - IoUring
        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

• Files reviewed: 37/37 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Heap teardown and accounting defects remain, and multiple CI test suites are disabled.

Review details

Suppressed comments (8)

Previously missed (1) — in code that hasn't changed since the last review.

.github/workflows/ci.yml:185

• This condition permanently disables the epoll C++ test suite, including shutdown coverage. Restore the original condition before merging.

This issue also appears in the following locations of the same file:

• line 213
• line 225

src/server/engine_shard.cc:577

• mi_heap_destroy force-reclaims allocations that may still be released by longer-lived shutdown owners; this teardown path is the one associated with the replica shutdown crashes called out by this branch's CI changes. Keep the safe mi_heap_delete(tlh) behavior and explicitly free shard_ rather than invalidating the heap eagerly.

  mi_heap_destroy(tlh);

src/redis/zmalloc_mi.c:233

• This drops the heap pointer but leaves zmalloc_used_memory_tl accounting allocations from the destroyed heap. Tests recreate ServerState on the same worker threads, so the next instance starts with stale memory usage and can make incorrect OOM/usage decisions; reset the counter here too.

void reset_zmalloc_threadlocal(void) {
  zmalloc_heap = NULL;
}

src/redis/zmalloc.h:153

• REDIS_ZMALLOC_MI=OFF selects zmalloc.c, but that implementation does not define this new function while EngineShard::DestroyThreadLocal() calls it unconditionally, causing the fallback build to fail at link time. Add the corresponding no-op/reset implementation to zmalloc.c.

void reset_zmalloc_threadlocal(void);

.github/workflows/ci.yml:255

• This temporary filter replaces the full regression suite with five tests, leaving unrelated shutdown and allocation lifetimes unvalidated across the build matrix. Remove test-cases so the normal suite runs before merge.

          # debugging fast-shutdown crash: only the tests that actually crashed in CI, so
          # this step doesn't wait on the full ~700-test suite while iterating on a fix.
          test-cases: "test_cascaded_partial_sync|test_partial_sync|test_partial_replication_on_same_source_master_with_replica_lsn_inc|test_bgsave_during_stable_sync|test_tls_replication"

.github/workflows/ci.yml:213

• This condition permanently disables the cluster-mode C++ test suite, so the new shutdown path is not validated in cluster mode. Restore the original condition before merging.

        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

.github/workflows/ci.yml:225

• This condition permanently disables the hashtag-locking cluster test suite, so the new shutdown path is not validated in this configuration. Restore the original condition before merging.

        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

.github/workflows/ci.yml:154

• This condition permanently disables the primary IoUring C++ test suite, so the allocator and shutdown rewrite can merge without unit-test coverage. Restore the original condition before merging.

        if: github.event_name != 'push' && false # debugging fast-shutdown crash: skip unit tests

• Files reviewed: 39/39 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The shutdown allocator regressions and unsafe CI container privileges must be resolved before approval.

Review details

Suppressed comments (1)

.github/workflows/ci.yml:87

• Granting SYS_ADMIN to a PR-controlled build container that also bind-mounts the host root (/:/hostroot) breaks the runner's isolation; the new helper then explicitly remounts /proc/sys writable. Remove this capability and rely on the unprivileged core-file watcher (or isolate core collection in a trusted job).

      options: --security-opt seccomp=unconfined --sysctl "net.ipv6.conf.all.disable_ipv6=0" --cap-add SYS_ADMIN

• Files reviewed: 39/39 changed files
• Comments generated: 0 new
• Review effort level: Balanced