Skip to content

[FLINK-39923][state] Fix RocksDB Statistics native memory leak#28489

Open
leekeiabstraction wants to merge 1 commit into
apache:masterfrom
leekeiabstraction:FLINK-39923-rocksdb-statistics-leak
Open

[FLINK-39923][state] Fix RocksDB Statistics native memory leak#28489
leekeiabstraction wants to merge 1 commit into
apache:masterfrom
leekeiabstraction:FLINK-39923-rocksdb-statistics-leak

Conversation

@leekeiabstraction

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Fix RocksDBNativeMetricMonitor.close() to invoke Statistics.close() instead of just nulling. Also hoist dbOptions.statistics() into a local in RocksDBHandle.loadDb() so a monitor-constructor failure doesn't orphan the wrapper.

Why are the changes needed?

Fix memory leak. When any of the 11 RocksDB ticker metrics is enabled, the TaskManager leaks native memory in proportion to the number of keyed state backend rebuilds e.g. during continuously failing and restarting job.

  1. Latent Flink-side bug was introduced in FLINK-24786 when Statistics object was added without explicit close() on the object. This was latent as it relied on finalize() running to call dispose() and close().
  2. rocksdb side finalizer was removed in facebook/rocksdb@99d8625
  3. Flink 2.0+ uses frocksdb 8.10.0. Leak started occurring as close is no longer called.

Verifying this change

Reproduced on flink:2.2.2, TaskManager container limited to 4 GB cgroup matching taskmanager.memory.process.size. Jobs that throw NumberFormatException on every row is submitted with restart-strategy.fixed-delay.delay=100ms and all 11 ticker metrics enabled.

Run Stats Outcome
Unpatched ON TM OOMKilled at 4 GB in ~80 s
Patched ON Ran for 4 minutes, no OOM, anon RSS stayed at around 2GB

See here for reproduction steps: https://github.com/leekeiabstraction/flink/tree/reproduce-rocksdb-statistics-leak/reproduce-rocksdb-statistics-leak

Does this PR introduce any user-facing change?

None.

Close the JNI Statistics wrapper in `RocksDBNativeMetricMonitor.close()`
and on partial init in `RocksDBHandle.loadDb()`.
@flinkbot

flinkbot commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

Comment on lines +316 to +319
if (nativeMetricMonitor != null) {
// Monitor owns the statistics wrapper.
IOUtils.closeQuietly(nativeMetricMonitor);
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

closeQuietly has it's own internal non null check
given that, can it be simplified to just

        IOUtils.closeQuietly(nativeMetricMonitor);
        IOUtils.closeQuietly(statistics);

?

@rkhachatryan rkhachatryan Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC that might cause double close of statistics (here and in nativeMetricMonitor).

@snuyanzin snuyanzin Jun 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will it be an issue (double closing), asking since closeQuietly should catch all the exceptions?
Or we should keep unclosed in some cases?

anyway it is just a nit pick, so feel free to merge

@rkhachatryan rkhachatryan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

FYI: @Zakelly, @fredia, @zoltar9264

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.

4 participants