Skip to content

fix(db): resolve resource leakage in database close() #6688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
warku123 wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

fix(db): resolve resource leakage in database close() #6688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cea8bd9
Fix resource leakage
warku123 Feb 6, 2026
75af270
Merge branch 'tronprotocol:develop' into 481_database_leakage_fix
warku123 Mar 20, 2026
ab078f6
fix(db): resolve resource leakage in CheckPointV2Store.close()
warku123 Mar 20, 2026
57949ca
Fix review issues: deduplicate logs and simplify exception handling
warku123 Apr 1, 2026
7142e2e
fix(db): split try-catch in TronDatabase.close() and improve tests
warku123 Apr 16, 2026
bcb7c0b
fix(db): restore finally block for close log in TronDatabase
warku123 Apr 16, 2026
70add8b
ci: retry
warku123 Apr 17, 2026
6bf1267
test(db): add real-resource close test to restore coverage
warku123 Apr 17, 2026
39cddb5
fix(db): extract doClose() hook to keep CheckPointV2Store logs at deb…
warku123 Apr 17, 2026
a099357
test(db): cover dbSource exception path in doClose()
warku123 Apr 17, 2026
22dd4a1
ci: retry
warku123 Apr 17, 2026
10bd532
test(db): add doClose() test to verify dbSource closes despite writeO…
warku123 Apr 20, 2026
267e2ab
fix(style): expand anonymous subclass methods to pass checkstyle
warku123 Apr 20, 2026
12ab2c2
test(db): cover stub methods in CheckPointV2Store to improve changed-…
warku123 Apr 20, 2026
ab0db5f
test(event): fix EventPluginLoader singleton pollution between tests
warku123 Apr 22, 2026
cc57817
refactor(db): document doClose() contract and simplify CheckPointV2St…
warku123 Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions chainbase/src/main/java/org/tron/core/db/TronDatabase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,13 +76,28 @@ public void reset() {
@Override
public void close() {
logger.info("******** Begin to close {}. ********", getName());
try {
doClose();
} finally {
logger.info("******** End to close {}. ********", getName());
}
}

/**
* Releases writeOptions and dbSource (best-effort, exceptions logged at WARN).
* Subclasses with extra resources should override {@link #close()} and call
* {@code doClose()} directly — not {@code super.close()} — to avoid duplicated logs.
*/
protected void doClose() {
try {
writeOptions.close();
} catch (Exception e) {
logger.warn("Failed to close writeOptions in {}.", getName(), e);
}
try {
dbSource.closeDB();
} catch (Exception e) {
logger.warn("Failed to close {}.", getName(), e);
} finally {
logger.info("******** End to close {}. ********", getName());
logger.warn("Failed to close dbSource in {}.", getName(), e);
}
}

Expand Down
10 changes: 3 additions & 7 deletions chainbase/src/main/java/org/tron/core/store/CheckPointV2Store.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,20 +62,16 @@ public void updateByBatch(Map<byte[], byte[]> rows) {
this.dbSource.updateByBatch(rows, writeOptions);
}

/**
* close the database.
*/
@Override
public void close() {
logger.debug("******** Begin to close {}. ********", getName());
try {
writeOptions.close();
dbSource.closeDB();
} catch (Exception e) {
Copy link
Copy Markdown
Collaborator

@xxo1shine xxo1shine Apr 23, 2026

Choose a reason for hiding this comment

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

@warku123, [SHOULD] The fact that testCloseDbSourceWhenWriteOptionsThrows has to reflectively set BOTH CheckPointV2Store.writeOptions and TronDatabase.writeOptions proves the subclass has its own writeOptions field shadowing the parent's — two WriteOptionsWrapper instances live side by side. This is pre-existing, but the new close() structure makes the implicit contract "child closes its own writeOptions first, then delegates to parent" very fragile. Consider either promoting the parent field to protected and removing the subclass field, or adding a class-level comment explaining why two writeOptions instances are intentional.

Copy link
Copy Markdown
Author

@warku123 warku123 Apr 23, 2026

Choose a reason for hiding this comment

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

Thanks for catching this. The two WriteOptionsWrapper instances are intentional rather than accidental shadowing:

  • Parent TronDatabase.writeOptions is configured with isDbSync()
  • CheckPointV2Store.writeOptions is configured with isCheckpointSync() (a dedicated checkpoint-specific sync flag)

Collapsing them into a single protected field would change the sync semantics for either the parent or the checkpoint store, so it's not a zero-risk refactor. Since this shadowing pre-dates the PR and the scope here is the resource-leak fix, I'd prefer to leave the structure as-is and file a follow-up issue if you'd like the shadowing formally addressed. Happy to add a class-level comment documenting the intent if that would help.

logger.warn("Failed to close {}.", getName(), e);
} finally {
logger.debug("******** End to close {}. ********", getName());
logger.warn("Failed to close writeOptions in {}.", getName(), e);
}
doClose();
logger.debug("******** End to close {}. ********", getName());
}

}
6 changes: 6 additions & 0 deletions framework/src/test/java/org/tron/common/logsfilter/FilterQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.List;
import java.util.Map;
import lombok.extern.slf4j.Slf4j;
import org.junit.After;
import org.junit.Assert;
import org.junit.Test;
import org.tron.common.logsfilter.capsule.ContractEventTriggerCapsule;
Expand All @@ -28,6 +29,11 @@
@Slf4j
public class FilterQueryTest {

@After
Copy link
Copy Markdown
Collaborator

@xxo1shine xxo1shine Apr 23, 2026

Choose a reason for hiding this comment

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

@warku123, [SHOULD] This @After cleanup of EventPluginLoader.getInstance().setFilterQuery(null) and the similar change in BlockEventGetTest.before() are test-pollution fixes unrelated to the TronDatabase.close() resource leak this PR targets. Mixing them into the same PR makes git blame and review harder. Please split the test-pollution fixes into a separate PR, or add a note in the PR description explaining why they must ship together (e.g. the new tests in this PR surface the existing pollution).

Copy link
Copy Markdown
Author

@warku123 warku123 Apr 23, 2026

Choose a reason for hiding this comment

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

The background and rationale are already written up in an earlier inline comment: #6688 (comment)

TL;DR: the new tests in this PR shifted Gradle's forkEvery=100 batch boundary, which exposed a pre-existing singleton-pollution bug in FilterQueryTest — EventPluginLoader.filterQuery was never reset in @After, so BlockEventGetTest landing in the same JVM read a stale {fromBlock=100, toBlock=190} and matchFilter() returned false for block 1, causing processTrigger to go from 277/293 to 0/293 instructions and failing the overall-delta gate.

So these @After / @Before resets aren't ornamental test-pollution fixes — they are a key finding of this PR: without them the CI coverage gate fails (evidence links in the inline comment). I kept them here so the PR lands green.

That said, you're right that the root fix belongs outside this PR's scope. My plan is to open a follow-up issue after this merges, covering both:

  1. The FilterQueryTest singleton cleanup (done here defensively, but the pattern likely exists elsewhere — any test writing to a static singleton without @After reset is a latent coverage-gate hazard).
  2. Auditing other singletons (e.g. Args, EventPluginLoader, ConfigLoader) for similar cross-test leakage.

I'll also update the PR description with a short pointer to the inline comment so reviewers don't have to dig for it.

public void tearDown() {
EventPluginLoader.getInstance().setFilterQuery(null);
}

@Test
public synchronized void testParseFilterQueryBlockNumber() {
assertEquals(LATEST_BLOCK_NUM, parseToBlockNumber(EMPTY));
Expand Down
161 changes: 161 additions & 0 deletions framework/src/test/java/org/tron/core/db/CheckPointV2StoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
package org.tron.core.db;

import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;

import java.io.IOException;
import java.lang.reflect.Field;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
import org.rocksdb.RocksDB;
import org.tron.common.TestConstants;
import org.tron.common.storage.WriteOptionsWrapper;
import org.tron.core.config.args.Args;
import org.tron.core.db.common.DbSourceInter;
import org.tron.core.store.CheckPointV2Store;

public class CheckPointV2StoreTest {

@ClassRule
public static final TemporaryFolder temporaryFolder = new TemporaryFolder();

static {
RocksDB.loadLibrary();
}

@BeforeClass
public static void initArgs() throws IOException {
Args.setParam(
new String[]{"-d", temporaryFolder.newFolder().toString()},
TestConstants.TEST_CONF
);
}

@AfterClass
public static void destroy() {
Args.clearParam();
}

@Test
public void testStubMethods() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-stubs");
try {
byte[] key = "key".getBytes();

store.put(key, new byte[]{});
Assert.assertNull(store.get(key));
Assert.assertFalse(store.has(key));
store.forEach(item -> {
});
Assert.assertNull(store.spliterator());

Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource =
(DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(store, mockDbSource);
store.delete(key);
dbSourceField.set(store, originalDbSource);

java.lang.reflect.Method initMethod =
CheckPointV2Store.class.getDeclaredMethod("init");
initMethod.setAccessible(true);
initMethod.invoke(store);
} finally {
store.close();
}
}

@Test
public void testCloseWithRealResources() {
CheckPointV2Store store = new CheckPointV2Store("test-close-real");
// Exercises the real writeOptions.close() and dbSource.closeDB() code paths
store.close();
}

@Test
public void testCloseReleasesAllResources() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-close");

// Replace dbSource with a mock so we can verify closeDB()
Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(store, mockDbSource);

try {
store.close();

verify(mockDbSource).closeDB();
} finally {
originalDbSource.closeDB();
}
}

@Test
public void testCloseWhenDbSourceThrows() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-close-dbsource-throws");

Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
doThrow(new RuntimeException("simulated dbSource failure")).when(mockDbSource).closeDB();
dbSourceField.set(store, mockDbSource);

try {
store.close();
} finally {
originalDbSource.closeDB();
}
}

@Test
public void testCloseDbSourceWhenWriteOptionsThrows() throws Exception {
CheckPointV2Store store = new CheckPointV2Store("test-close-exception");

// Replace child writeOptions with a spy that throws on close
Field childWriteOptionsField = CheckPointV2Store.class.getDeclaredField("writeOptions");
childWriteOptionsField.setAccessible(true);
WriteOptionsWrapper childWriteOptions =
(WriteOptionsWrapper) childWriteOptionsField.get(store);
WriteOptionsWrapper spyChildWriteOptions = spy(childWriteOptions);
doThrow(new RuntimeException("simulated writeOptions failure"))
.when(spyChildWriteOptions).close();
childWriteOptionsField.set(store, spyChildWriteOptions);

// Replace parent writeOptions with a spy that throws on close
Field parentWriteOptionsField = TronDatabase.class.getDeclaredField("writeOptions");
parentWriteOptionsField.setAccessible(true);
WriteOptionsWrapper parentWriteOptions =
(WriteOptionsWrapper) parentWriteOptionsField.get(store);
WriteOptionsWrapper spyParentWriteOptions = spy(parentWriteOptions);
doThrow(new RuntimeException("simulated parent writeOptions failure"))
.when(spyParentWriteOptions).close();
parentWriteOptionsField.set(store, spyParentWriteOptions);

// Replace dbSource with a mock
Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(store);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(store, mockDbSource);

try {
store.close();

// dbSource.closeDB() must be called even though both writeOptions threw
verify(mockDbSource).closeDB();
} finally {
originalDbSource.closeDB();
}
}
}
52 changes: 52 additions & 0 deletions framework/src/test/java/org/tron/core/db/TronDatabaseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
package org.tron.core.db;

import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;

import com.google.protobuf.InvalidProtocolBufferException;
import java.io.IOException;
import java.lang.reflect.Field;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
Expand All @@ -12,7 +18,9 @@
import org.junit.rules.TemporaryFolder;
import org.rocksdb.RocksDB;
import org.tron.common.TestConstants;
import org.tron.common.storage.WriteOptionsWrapper;
import org.tron.core.config.args.Args;
import org.tron.core.db.common.DbSourceInter;
import org.tron.core.exception.BadItemException;
import org.tron.core.exception.ItemNotFoundException;

Expand Down Expand Up @@ -99,4 +107,48 @@ public void TestGetFromRoot() throws
Assert.assertEquals(db.getFromRoot("test".getBytes()),
"test");
}

@Test
public void testDoCloseDbSourceCalledWhenWriteOptionsThrows() throws Exception {
TronDatabase<String> db = new TronDatabase<String>("test-do-close") {

@Override
public void put(byte[] key, String item) {
}

@Override
public void delete(byte[] key) {
}

@Override
public String get(byte[] key) {
return null;
}

@Override
public boolean has(byte[] key) {
return false;
}
};

Field writeOptionsField = TronDatabase.class.getDeclaredField("writeOptions");
writeOptionsField.setAccessible(true);
WriteOptionsWrapper spyWriteOptions = spy((WriteOptionsWrapper) writeOptionsField.get(db));
doThrow(new RuntimeException("simulated writeOptions failure")).when(spyWriteOptions).close();
writeOptionsField.set(db, spyWriteOptions);

Field dbSourceField = TronDatabase.class.getDeclaredField("dbSource");
dbSourceField.setAccessible(true);
DbSourceInter<byte[]> originalDbSource = (DbSourceInter<byte[]>) dbSourceField.get(db);
DbSourceInter<byte[]> mockDbSource = mock(DbSourceInter.class);
dbSourceField.set(db, mockDbSource);

try {
db.doClose();
verify(spyWriteOptions).close();
verify(mockDbSource).closeDB();
} finally {
originalDbSource.closeDB();
}
}
}
1 change: 1 addition & 0 deletions framework/src/test/java/org/tron/core/event/BlockEventGetTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ public static void init() {

@Before
public void before() throws IOException {
EventPluginLoader.getInstance().setFilterQuery(null);
Args.getInstance().setNodeListenPort(10000 + port.incrementAndGet());
Copy link
Copy Markdown
Author

@warku123 warku123 Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Defensive reset unrelated to this PR's resource-leakage fix, but needed to stabilize the JaCoCo overall-delta gate.

Root cause: FilterQueryTest (in org.tron.common) sets EventPluginLoader.filterQuery = {fromBlock=100, toBlock=190} at the end of testMatchFilter() with no cleanup. With forkEvery=100, this PR's new test methods shifted the JVM batch boundary, causing FilterQueryTest and BlockEventGetTest to land in the same JVM. Since start(config) does not reset filterQuery, the stale range filter caused matchFilter() to return false for block 1, skipping processTrigger entirely — 0/293 instructions covered in the PR run vs 277/293 on base.

Evidence:

Fix: FilterQueryTest now resets filterQuery to null in @After, eliminating the pollution at its source. The @Before reset here is an additional guard against any other test leaving dirty singleton state.

Note: This singleton pollution pattern may exist elsewhere in the codebase — any test that writes to a static singleton (e.g. Args, EventPluginLoader, ConfigLoader) without resetting it in @After is a potential source of similar unexpected JaCoCo coverage drops when batch boundaries shift.


dbManager = context.getBean(Manager.class);
Expand Down
Loading