Investigate: some transactions complete without emitting TransactionCompleted/TransactionTimedOut events

[iduartgomez]

Context

Discovered during investigation of #2981 (pending_op_results memory leak). The fix adds cleanup on TransactionCompleted/TransactionTimedOut events plus a periodic safety-net sweep for entries where the sender is closed.

The reviewer asked: why do some transactions complete without emitting these events?

Root Cause Analysis

Several code paths in handle_op_result() (crates/core/src/operations/mod.rs) reach terminal states without emitting TransactionCompleted:

1. Local state queueing (UpdateBroadcasting)

When OperationResult has return_msg: None and state: Some(updated_state), the state is pushed to OpManager but no completion event is emitted (line ~258-266).

2. StatePushed error path

Err(OpError::StatePushed) returns Ok(None) — the operation continues locally without any event (line ~113-117).

3. Duplicate message for completed op

Err(OpError::OpNotPresent(tx)) returns Ok(None) — the original transaction's callback was already inserted but if the original completion path didn't emit an event, the entry leaks (line ~118-127).

4. Root operation waiting for sub-operations

When a finalized operation has sub-operations that haven't completed, it's stored in root_ops_awaiting_sub_ops and returns Ok(None) — no event emitted until all children complete (line ~180-213).

5. Callback invocation guard

In process_message_decoupled() (crates/core/src/node/mod.rs), the callback sender is only invoked when is_operation_completed(&op_result) returns true. The pending_op_results entry is inserted regardless, but the callback may never fire for the paths above.

Impact

The periodic safety-net cleanup in #2981 handles all these cases by checking sender.is_closed(). The event-driven cleanup handles the common case. Together they prevent the leak.

Recommendation

Consider auditing whether TransactionCompleted should be emitted in more of these terminal paths, particularly:

• When StatePushed operations actually finish their local processing
• When root operations' sub-operations all complete (verify the parent event fires)

This would make the system more consistent, though the safety-net cleanup makes it non-critical.

Related

• fix: clean up pending_op_results to prevent unbounded memory growth #2981 — fix: clean up pending_op_results to prevent unbounded memory growth
• Wasmer Engine code_memory grows unboundedly #2941 — referenced in cleanup comments (Wasmer memory, different root cause)

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-bug (95% confidence)
• P-medium (80% confidence)
• E-medium (75% confidence)

Reasoning: The report describes code paths where terminal operation states are reached without emitting TransactionCompleted/TransactionTimedOut events, causing pending_op_results entries to leak. This is a correctness/bug in operation lifecycle handling (T-bug). It can cause unbounded memory growth without the safety-net and warrants normal priority (P-medium). Fixing requires auditing multiple code paths and adjusting event emission/cleanup, which is moderate effort (E-medium). The issue sits in core operation/message handling in the node/operations code, related to networking/operation lifecycle (A-networking). The recommendation to audit whether events should be emitted suggests an architectural decision, so a design discussion is appropriate (S-needs-design). Confidence scores reflect certainty about label applicability.

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.

[sanity]

Issue Priority Assessment

Generated: 2026-06-16T14:01:54.052598040+00:00
Issue hash: 111b2fc2

Value Assessment

Relevance Check: This issue is still relevant. The @iduartgomez re-investigation was accurate — the original three paths were indeed eliminated by the #1454 op refactor, but the LocalSubscribeComplete bypass in p2p_protoc.rs persists in current code. At line ~2519, the handler calls op_manager.result_router_tx.try_send() directly without ever calling send_client_result() or emitting NodeEvent::TransactionCompleted. In contrast, sub-op and renewal completions in the same handler (lines ~2494 and ~2507) do call op_manager.completed(tx). The periodic 60-second sweep added in #2981 masks the symptom, and a comment at line ~1373 explicitly acknowledges that it's a "safety net for transactions that complete without emitting TransactionCompleted/TimedOut events" — meaning the technical debt is baked into the codebase's own commentary.

User Impact: The practical impact is a 60-second transient memory leak per local subscription completion (when the contract is already cached). Every client that subscribes to a locally-cached contract contributes one pending_op_results entry that lingers rather than being freed immediately. This is bounded by the periodic sweep so it won't grow unboundedly, but in busy nodes with high subscription rates to popular contracts, this could accumulate meaningfully within that window. The impact is moderate — not a crash or correctness bug, but wasted memory in a system where the issue tracker shows memory growth as an already-known pain point (#2981).

Strategic Alignment: The fix aligns directly with an ongoing effort to make operation lifecycle management consistent and auditable. The entire #1454 op refactor was motivated by exactly this kind of invariant enforcement — funneling all terminal paths through deliver_outcome() → send_client_result(). The LocalSubscribeComplete path is a holdout from that refactor. Cleaning it up completes the refactor's intent and removes a known gap from the "periodic cleanup as safety net" comment, which otherwise signals to future contributors that this inconsistency is accepted design rather than technical debt.

Blocking Factor and Urgency: This doesn't block other work directly, and the safety net prevents it from causing real resource exhaustion. However, the absence of a regression test for the LocalSubscribeComplete → TransactionCompleted invariant is notable — the existing test_pending_op_results_bounded test covers the general property but not this specific path. The fix is small: route the standalone parent path through op_manager.completed(tx) or emit TransactionCompleted explicitly, consistent with what sub-op and renewal paths already do. Given its low complexity and the fact that it closes a real (if mild) leak while restoring invariant consistency, this is a reasonable P-medium issue — worth fixing in a focused PR but not urgent enough to block other work.

Cost Assessment

Now I have a clear picture of the issue. Let me write my assessment.

Relevance: The issue is still actionable and the comment from @iduartgomez accurately describes the current state. The original multi-path problem described in the issue body is fully fixed by the post-#1454 op refactor. The one remaining violation is in the LocalSubscribeComplete arm of p2p_protoc.rs (~line 2466–2568): after routing the response through result_router_tx.try_send(), the handler manually removes tx_to_client but never removes pending_op_results[tx] and never calls live_tx_tracker.remove_finished_transaction(tx). The TransactionCompleted event that every other terminal path emits is simply absent here. This is a narrow, well-scoped problem in one code path.

Scope and Complexity: The fix touches a single file (crates/core/src/node/network_bridge/p2p_protoc.rs) in one event arm. The cleanest approach is to emit NodeEvent::TransactionCompleted(tx) after the try_send in the success path — mirroring what every op driver already does via send_client_result. The existing TransactionCompleted handler already covers pending_op_results.remove and live_tx_tracker.remove_finished_transaction, so no new cleanup logic is needed. The error path (try_send fails) needs a parallel decision: should it emit TransactionCompleted anyway to force cleanup, or let the 60s sweep handle it? The safe answer is yes — emit it regardless, since the client has already been abandoned. There are no tricky edge cases: sub-operations and renewals already call op_manager.completed(tx) and continue, so they're unaffected.

External Dependencies and Risk: No external dependencies. The risk is low: this is an additive cleanup path. The only regression risk is if something downstream depends on the leaked pending_op_results entry surviving past the response send, which would be a latent bug in its own right, not a feature. The TransactionCompleted handler is idempotent (remove on a DashMap returns None if already absent), so double-emission is safe.

Testing Effort: This is where most of the work will be. There are currently no tests that guard this invariant, as @iduartgomez noted. A proper regression test would need to verify that after a LocalSubscribeComplete event, pending_op_results is empty for that transaction — which requires either a simulation test that exercises the standalone-subscribe path (contract already cached locally, no remote peers) and then inspects internal node state, or a unit test that directly drives the event-loop arm. The simulation test approach is more realistic but takes more setup. Overall this is a 1–2 hour fix with maybe 2–4 hours of test authoring to do it properly, making it genuinely low cost by this codebase's standards.

---

Generated by issue-prioritizer

[iduartgomez]

Re-investigated against current code (post #1454 op refactor).

The three paths in the original report no longer exist: `handle_op_result()` and `operations/mod.rs` were replaced by the per-tx `op_ctx_task` drivers. All four op drivers now funnel terminal state through `deliver_outcome()` → `OpManager::send_client_result()` (`node/op_state_manager.rs`), which always emits `NodeEvent::TransactionCompleted(tx)`. So the originally-described paths are fixed.

However, the invariant is still violated by ONE remaining path:

`LocalSubscribeComplete` bypasses `send_client_result`. In `node/network_bridge/p2p_protoc.rs` (~line 2120), when a SUBSCRIBE completes locally (contract already cached, no remote peer), the handler does `op_manager.result_router_tx.try_send((tx, response))` directly and removes the client-waiting bookkeeping, but never calls `send_client_result` and never emits `TransactionCompleted`/`TransactionTimedOut`. The `pending_op_results[tx]` entry then lingers until the 60s periodic sweep — a transient leak rather than the unbounded one originally feared.

Keeping this open, re-scoped to the single remaining path. Fix: route `LocalSubscribeComplete` through `send_client_result` (or otherwise emit `TransactionCompleted`) so local-subscribe completions clean up immediately like every other terminal path. No existing test guards this invariant.

[AI-assisted - Claude]