[Bug] storeEventListenStatus leaks shared store listener when a non-owner CachedGraphTransaction closes first

[dpol1]

Bug Type (问题类型)

logic (逻辑设计问题)

Before submit

• 我已经确认现有的 Issues 与 FAQ 中没有相同 / 重复问题 (I have confirmed and searched that there are no similar problems in the historical issue and documents)

Environment (环境信息)

• Server Version: 1.7.0 (Apache Release Version)
• Backend: RocksDB x nodes, HDD or SSD
• OS: xx CPUs, xx G RAM, Ubuntu 2x.x / CentOS 7.x
• Data Size: xx vertices, xx edges

Expected & Actual behavior (期望与实际表现)

Expected

When several CachedGraphTransaction instances share the same graph, closing a non‑owner transaction must not touch the shared store listener registered by the first (owner) transaction. The store listener should only be unregistered when the owner closes.

Actual

unlistenChanges() calls storeEventListenStatus.remove(graphName) unconditionally. The first transaction to close — owner or not — pops the tracking entry. If it is a non‑owner, it then calls provider().unlisten(this.storeEventListener) on its own per‑instance listener, which was never registered with the provider, so the unlisten is a no‑op. The owner's real store listener stays registered but is now untracked. When the owner closes later, remove(graphName) returns null, the real unlisten(...) call is skipped, and the listener leaks for the rest of the JVM lifetime.

The same pattern is present in CachedSchemaTransaction.java.

Reproduction

1. Open two CachedGraphTransaction instances T1 (owner) and T2 (non‑owner) for the same spaceGraphName.
2. T1.listenChanges() registers T1.storeEventListener via store().provider().listen(...) and inserts the tracking entry into storeEventListenStatus.
3. T2.listenChanges() sees the entry already there and skips registration.
4. Close T2 first. storeEventListenStatus.remove(graphName) returns true. provider().unlisten(T2.storeEventListener) runs as a no‑op because that instance was never registered.
5. Close T1. storeEventListenStatus.remove(graphName) returns null. The real provider().unlisten(T1.storeEventListener) is skipped.
6. T1.storeEventListener stays registered with the provider indefinitely.

Root cause

Same owner‑first‑close shape that PR #3017 fixed for graphCacheEventListeners via the CacheListenerHolder ref‑count. storeEventListenStatus was left as‑is, with the gap flagged in the in‑code TODO:

• CachedGraphTransaction
• mirror in CachedSchemaTransaction

The unit test CachedGraphTransactionTest papers over it with storeListeners.putIfAbsent(graphName, true) after closing the secondary transaction, so the test teardown can still unlisten the primary listener.

Suggested fix

Two options surfaced in the PR #3017 review:

1. Apply the same CacheListenerHolder ref‑counted holder pattern used for the cache listener to the store listener.
2. Stop registering and unregistering on transaction lifecycle. The cache is already a CacheManager singleton per spaceGraphName, so the store listener lifetime is the graph, not the transaction. Move register/unregister to graph open/dispose.

Option 2 is preferred: it removes the owner concept and the whole race class. Option 1 is the smaller mechanical change.

Scope

• Apply the chosen fix to CachedGraphTransaction and CachedSchemaTransaction.
• Remove the putIfAbsent workaround from CachedGraphTransactionTest.
• Add a regression test that exercises non‑owner‑first‑close ordering and asserts the owner's listener is still unregistered when the owner closes.

References

• PR fix(server): align cache event actions in legacy EventHub path #3017 review thread on CachedGraphTransactionTest.java:236 (TODO follow‑up note).
• PR fix(server): align cache event actions in legacy EventHub path #3017 review thread on CachedGraphTransaction.java:210 (owner‑first‑close discussion).
• Introduced as a non‑blocking TODO in PR fix(server): align cache event actions in legacy EventHub path #3017 (cache event alignment).

Vertex/Edge example (问题点 / 边数据举例)

Schema [VertexLabel, EdgeLabel, IndexLabel] (元数据结构)

[dpol1]

@dosu please act as an issue clarity editor for this ticket.
I already wrote a detailed technical description with links to the relevant code (CachedGraphTransaction, CachedSchemaTransaction, tests, and PR #3017).
Your job is:

Add a short TL;DR at the top (3–4 lines) that explains in plain language:

• what component is affected

• what the bug is (one‑sentence summary)

• why it matters for HugeGraph (impact on long‑running servers).

• Add a Context section that briefly explains what store listeners are and how they are supposed to behave, referencing the links already present in the issue.

• Keep the existing Expected, Actual, Root cause, Suggested fix, and Scope sections, but lightly rephrase sentences where needed to improve readability without removing technical details or changing the meaning.

[dosubot]

Here's the edited version of your issue with the requested additions and light rephrasing:

---

TL;DR

Component: CachedGraphTransaction and CachedSchemaTransaction (cache layer, store event listener lifecycle)

Bug: When multiple CachedGraphTransaction instances share a graph and a non-owner transaction closes first, the shared store event listener is orphaned — it remains registered with the backend provider but becomes untracked, so it is never cleaned up.

Impact: On long-running HugeGraph servers, each leaked listener holds references to cache structures and continues receiving store events indefinitely, resulting in a slow memory leak and unnecessary event processing that accumulates over the JVM lifetime.

---

Context

HugeGraph's cached transaction layer registers store event listeners with the backend provider to perform bulk cache invalidation when critical store lifecycle events occur (STORE_INIT, STORE_CLEAR, STORE_TRUNCATE) [1]. These listeners ensure that in-memory vertex/edge/schema caches are wiped when the underlying store state is fundamentally altered.

Because the cache is a singleton per spaceGraphName, the code uses a static storeEventListenStatus map to ensure only one listener is registered per graph, regardless of how many transaction instances exist [2]. The first transaction to call listenChanges() becomes the implicit "owner" — it registers the listener with store().provider().listen(...). Subsequent transactions see the map entry and skip registration. The bug is in the teardown path: any transaction — owner or not — can remove the tracking entry and break the unregistration contract.

---

Expected behavior

When several CachedGraphTransaction instances share the same graph, closing a non-owner transaction must not disturb the shared store listener registered by the owner. The store listener should only be unregistered when the last interested transaction closes (or, ideally, when the graph itself is disposed).

Actual behavior

unlistenChanges() calls storeEventListenStatus.remove(graphName) unconditionally [3]. Whichever transaction closes first wins the remove. If it is a non-owner:

1. It pops the tracking entry successfully.
2. It calls provider().unlisten(this.storeEventListener) on its own per-instance listener — which was never registered — so the call is a no-op.
3. The owner's real store listener stays registered but is now untracked.
4. When the owner closes later, remove(graphName) returns null, the real unlisten(...) call is skipped, and the listener leaks for the rest of the JVM lifetime.

The same pattern is present in CachedSchemaTransaction [4].

Reproduction steps

1. Open two CachedGraphTransaction instances — T1 (owner) and T2 (non-owner) — for the same spaceGraphName.
2. T1.listenChanges() registers T1.storeEventListener via store().provider().listen(...) and inserts the tracking entry.
3. T2.listenChanges() sees the entry already present and skips registration.
4. Close T2 first — storeEventListenStatus.remove(graphName) returns true; provider().unlisten(T2.storeEventListener) runs as a no-op (never registered).
5. Close T1 — storeEventListenStatus.remove(graphName) returns null; the real provider().unlisten(T1.storeEventListener) is skipped.
6. T1.storeEventListener stays registered with the provider indefinitely.

Root cause

This is the same owner-first-close shape that PR #3017 fixed for graphCacheEventListeners via the ref-counted CacheListenerHolder [5]. storeEventListenStatus was left as-is, with the gap explicitly flagged in a TODO comment added by that PR:

"storeEventListenStatus has the same owner-first close bug this PR fixes for GRAPH_CACHE_EVENT_LISTENERS."

Relevant code locations:

• CachedGraphTransaction unlistenChanges
• Mirror in CachedSchemaTransaction

Suggested fix

Two options surfaced in the PR #3017 review:

1. Ref-counted holder — Apply the same CacheListenerHolder pattern used for the cache listener to the store listener. Smaller mechanical change; proven pattern already in-tree.
2. Move lifetime to graph open/dispose — The cache is already a CacheManager singleton per spaceGraphName, so the store listener's natural lifetime is the graph, not the transaction. Register on graph open, unregister on graph dispose. This eliminates the owner concept entirely and removes the whole race class.

Option 2 is preferred for long-term cleanliness; Option 1 is the safer incremental fix.

Scope

• Apply the chosen fix to both CachedGraphTransaction and CachedSchemaTransaction.
• Remove any test workarounds (e.g. putIfAbsent re-insertion after closing a secondary transaction) from CachedGraphTransactionTest.
• Add a regression test exercising non-owner-first-close ordering, asserting the owner's listener is properly unregistered when the owner closes.

References

• PR fix(server): align cache event actions in legacy EventHub path #3017 review thread on CachedGraphTransactionTest.java:236 (TODO follow-up note)
• PR fix(server): align cache event actions in legacy EventHub path #3017 review thread on CachedGraphTransaction.java:210 (owner-first-close discussion)
• Introduced as a non-blocking TODO in PR fix(server): align cache event actions in legacy EventHub path #3017 (cache event alignment)

To reply, just mention @dosu.

---

Share context across your team and agents. Try Dosu.

[dpol1]

@dosu Where is the per-graph CacheManager singleton instantiated and disposed today? List the call sites that create the per-spaceGraphName cache instances and any hook (graph open/close/dispose) where store listener register/unregister could naturally live if we move them out of listenChanges/unlistenChanges. Does CachedSchemaTransactionV2 have an equivalent storeEventListenStatus-style tracking field for its store listener? If yes, does it share the close of T1+T2 possible? This affects whether the fix needs synchronized or whether ConcurrentHashMap.compute() (like the CacheListenerHolder pattern) is sufficient.

[dosubot]

Here's what I found across all your questions:

---

CacheManager is a JVM-wide singleton, not per-graph. Per-graph isolation is achieved through naming: "{prefix}-{spaceGraphName}" passed to CacheManager.instance().cache(name, capacity) [1].

Cache creation call sites (all go through the singleton):

Component	Call Site	Pattern
CachedGraphTransaction	Constructor	"vertex-" + spaceGraphName, "edge-" + spaceGraphName [2]
CachedSchemaTransaction	cache() helper	"{prefix}-" + graph().spaceGraphName() [3]
CachedSchemaTransactionV2	Same pattern	[4]
StandardAuthManager	Constructor	"{prefix}-" + graph.spaceGraphName() [5]

---

Natural lifecycle hooks for Option 2 — StandardHugeGraph has these methods where listener registration could live [6]:

• Register at: initBackend() [7] — runs once when the graph store is first initialized. Alternatively, inside the constructor/open() path where EventHub instances are already created [8].
• Unregister at: close() [9] — synchronized, runs once during shutdown. It already calls closeTx() → destroys transactions, and then closes the store provider. Inserting provider().unlisten(...) before storeProvider.close() would be the natural spot.
• drop() → calls clearBackend() then close(), so close covers it [10].

The key insight: closeTx() [11] destroys the per-thread transaction instances, but the store provider outlives them until close() finishes. Placing listener management at the graph level removes the multi-transaction race entirely.

---

CachedSchemaTransactionV2 — yes, it exists [12], used for HStore backends. The selection is in StandardHugeGraph.openSchemaTransaction() [13]:

if (isHstore()) {
    return new CachedSchemaTransactionV2(...);
}
return new CachedSchemaTransaction(...);

It does NOT have a storeEventListenStatus field. It stores listener refs directly as instance fields [14]:

private EventListener storeEventListener;
private EventListener cacheEventListener;

Because there's no shared static tracking map, V2 doesn't suffer the same T1+T2 race — but it does call unlisten() unconditionally on every transaction close, mirroring the CachedSchemaTransaction behavior. If the underlying AbstractBackendStoreProvider.unlisten() is idempotent (which it is — it just removes from an EventHub), the V2 path is safe for its own close ordering, but it registers on every instance construction, which could mean redundant registrations rather than leaks.

---

Thread safety: is ConcurrentHashMap.compute() sufficient?

The storeEventListenStatus is a static ConcurrentHashMap<String, Boolean> in GraphTransaction [15]. Multiple concurrent transaction instances for the same graph absolutely exist — each thread gets its own via ThreadLocal [16].

ConcurrentHashMap.compute() is sufficient for the ref-counted holder pattern (Option 1), because:

• compute() guarantees atomicity for the lambda — no two threads execute the lambda for the same key concurrently.
• The CacheListenerHolder pattern from PR fix(server): align cache event actions in legacy EventHub path #3017 already does this successfully: it puts the listen()/unlisten() calls inside the compute() lambda, making the check-and-act atomic.
• Since provider().listen() and unlisten() just delegate to EventHub.listen()/EventHub.unlisten() [17], which are lightweight map operations, executing them inside compute() doesn't create deadlock risk.

You do not need an external synchronized block — compute() handles per-key exclusion. The proven pattern from PR #3017's GRAPH_CACHE_EVENT_LISTENERS demonstrates this works correctly.

---

Summary for your fix scope:

Class	Has tracking field?	Affected by T1+T2 race?	Fix needed?
CachedGraphTransaction	Yes (storeEventListenStatus static)	✅ Yes	✅
CachedSchemaTransaction	No (unconditional unlisten)	Different bug shape (redundant register/no-op unlisten)	✅
CachedSchemaTransactionV2	No (instance fields, no static map)	Redundant registration, not a leak	Low priority

If you go with Option 2 (graph-level lifecycle), you eliminate the problem in all three classes at once — the listener lives in StandardHugeGraph, tied to initBackend()/close(), and the transactions simply use the cache without managing listeners at all.

To reply, just mention @dosu.

---

Docs are dead. Just use Dosu.