[fix](profile) Fix execution profile leak when dispatch fails before profile registration

[linrrzqqq]

What problem does this PR solve?

Issue Number: N/A

Related PR: N/A

Problem Summary:

An ExecutionProfile is registered before query dispatch. If dispatch fails before the first updateProfile(false) call, the corresponding Profile has not been added to profileIdToProfileMap.

When cleanup is invoked, the previous removeProfile implementation removed execution profiles only after finding a historical Profile entry. If no such entry existed, the ExecutionProfile remained in queryIdToExecutionProfiles and continued to retain query state in FE memory. This was reproduced with a UDF JAR download failure: three failed queries left three ExecutionProfiles and no historical Profile entries.

Pass the current Profile to removeProfile and remove its ExecutionProfiles directly, regardless of whether a historical entry exists. Add tests covering a missing historical entry, an existing entry, and multiple ExecutionProfiles.

Release note

Fix FE memory growth caused by execution profiles retained when query dispatch fails before profile registration.

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?

[linrrzqqq]

/review

[github-actions]

Static-only review complete for head c588e48817809f120a4296a2a6d5d32615df8729. Request changes: one new P1 inline finding. The new cleanup removes execution profiles for a single short query even when no historical summary was inserted, but a dispatch failure followed by a query retry can still retain the retry's execution profile. No existing P0/P1 inline comment was present or independently confirmed. Two review rounds converged, including normal whole-PR and focused lifecycle reviews; the final changed-file sweep found no other substantiated new issue.

Checkpoint conclusions:

• Goal and proof: The direct absent-summary case is covered by a new unit test, but the intended memory-leak fix is incomplete for dispatch retries. The tests do not exercise failed dispatch followed by retry.
• Scope and reuse: The map and API renames are otherwise behavior-preserving: Profile.getId() delegates to SummaryProfile.getProfileId(). The cleanup API change is focused; its caller's attempt lifecycle needs correction.
• Concurrency and locking: The two manager maps are modified under the same write lock. BE profile reports run asynchronously and can hold a captured execution-profile object; no new lock-order issue, metadata-lock IO, or heavy work under a changed lock was found. The P1 is a sequential retry lifecycle failure.
• Lifecycle: StmtExecutor reuses one Profile across attempts. Its first terminal update sets isQueryFinished; later attempts register new execution IDs but cannot reach terminal cleanup. Successful Broker Load finalization follows its task callbacks, with no additional add/remove race established.
• Configuration: No new setting or dynamic-change obligation is introduced. The P1 occurs with a positive existing auto-profile threshold and the configured query retry path; max_query_retry_time defaults to 3.
• Compatibility: No FE/BE wire value, storage format, function symbol, or rolling-upgrade behavior changed.
• Parallel paths: Normal MySQL, collected internal-query, deferred Arrow Flight, and successful Broker Load paths were traced. The retry path is the one unresolved defect; cloud replan and insert retries share that root cause rather than requiring duplicate comments.
• Conditional checks: The changed short-duration branch removes the caller's attached execution IDs. Its existing isQueryFinished guard prevents this on later attempts. Other renamed conditions retain their prior meaning.
• Tests and results: New FE unit cases check absent/present summary entries and multiple execution IDs. They do not cover a real retry or Broker Load cancellation. No test result files changed. Builds and tests were prohibited by the review instructions and were not run; conclusions are static only.
• Observability: Existing debug logs show missing/removed execution profiles. No distinct logging or metric defect was established; logging would not release the retained retry entry.
• Transactions, persistence, and data writes: The PR changes no transaction, EditLog, table-data write, or persistent profile format path; existing profile storage and Broker Load commit ordering were checked without a new defect.
• FE/BE variables and performance: No new transmitted variable exists. Cleanup is proportional to the attached execution profiles and no additional hot-path anti-pattern was substantiated.
• Authorization and other issues: The REST call still applies the same non-admin profile owner check. No other candidate remains unresolved after duplicate and risk review.

User review focus: none was supplied. A separate pre-existing Broker Load cancellation retention path was noted during review; it does not arise from this query-dispatch change and is not duplicated as an inline finding.

[github-actions]

[P1] Clean execution profiles for every dispatch retry. handleQueryWithRetry calls finalizeQuery() after each attempt, but it reuses this Profile when an RpcException from coordBase.exec() triggers a retry with a new query ID. The first short attempt marks isQueryFinished and reaches this removal; the retry then registers another ExecutionProfile, while both later updateSummary calls return immediately at the finished guard. unregisterQuery only marks that second entry finished, so it remains in queryIdToExecutionProfiles until the size-triggered fallback GC. Make each attempt eligible for cleanup and cover a failed-dispatch-then-retry case that checks both IDs.

[HappenLee]

Confirmed this retry cleanup gap locally on c588e48817809f120a4296a2a6d5d32615df8729.

The change correctly cleans up the first short failed attempt, but a subsequent attempt can still retain its ExecutionProfile:

1. With profiling enabled and a positive auto-profile threshold, a fast retryable RPC failure during dispatch reaches finalizeQuery(). updateSummary(..., true, ...) sets isQueryFinished = true and removes the first execution profile.
2. handleQueryWithRetry() reuses that same Profile, changes the query ID, and registers another execution profile.
3. Both summary updates for the retry return immediately at the isQueryFinished guard, so this removal is never reached again. unregisterQuery() only marks the execution profile finished.

The fallback collector is size-triggered (size > 2 * max_query_profile_num, i.e. over 1000 entries by default), so a small number of retained entries will not simply disappear after five seconds. This is a pre-existing lifecycle gap left uncovered by this fix, rather than a new regression introduced by the diff.

Please cover failed dispatch followed by a retry, and ensure cleanup applies to the retry execution as well. The lifecycle needs to distinguish completion of one attempt from completion of the whole statement.

Validation: ran the repository's run-fe-ut.sh: ProfileTest (15), AutoProfileTest (1), and ProfileManagerTest (28) all passed. The additional test below failed on the final assertion: the first execution was removed, but the retry execution remained registered. It directly reproduces the Profile lifecycle; it does not inject an actual BE RPC failure.

Minimal reproducer: ReviewProfileRetryTest.java

package org.apache.doris.common.profile;

import org.apache.doris.common.util.DebugUtil;
import org.apache.doris.thrift.TUniqueId;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.parallel.ResourceLock;

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

@ResourceLock("global")
public class ReviewProfileRetryTest {
    @Test
    public void retryExecutionMustBeReleasedAfterFinalization() {
        ProfileManager manager = ProfileManager.getInstance();
        Profile profile = new Profile(true, 1, 60_000);
        profile.getSummaryProfile().setQueryBeginTime(System.currentTimeMillis());
        TUniqueId firstId = new TUniqueId(68658, 1);
        TUniqueId retryId = new TUniqueId(68658, 2);
        Map<String, String> summary = new HashMap<>();
        summary.put(SummaryProfile.PROFILE_ID, DebugUtil.printId(firstId));
        try {
            // registerQuery, dispatch failure, finalizeQuery: no running summary was pushed.
            ExecutionProfile first = new ExecutionProfile(firstId, Collections.emptyList());
            profile.addExecutionProfile(first);
            manager.addExecutionProfile(first);
            profile.updateSummary(summary, true, null);
            manager.markExecutionProfileFinished(firstId);
            Assertions.assertNull(manager.getExecutionProfile(firstId));
            Assertions.assertTrue(profile.isQueryFinished);

            // handleQueryWithRetry reuses Profile while replacing the query ID and Coordinator.
            ExecutionProfile retry = new ExecutionProfile(retryId, Collections.emptyList());
            profile.addExecutionProfile(retry);
            manager.addExecutionProfile(retry);
            summary.put(SummaryProfile.PROFILE_ID, DebugUtil.printId(retryId));
            profile.updateSummary(summary, false, null);
            profile.updateSummary(summary, true, null);
            manager.markExecutionProfileFinished(retryId);

            Assertions.assertNull(manager.getExecutionProfile(retryId),
                    "The retry execution remains registered after finalization");
        } finally {
            manager.removeProfile(profile);
        }
    }
}

Observed failure:

The retry execution remains registered after finalization
expected: <null> but was: <DetailProfile(10c32-2): ...>
Tests run: 45, Failures: 1, Errors: 0, Skipped: 0

[linrrzqqq]

/review

[github-actions]

Complete static review

Reviewed PR #68658 at head fbb274bf92640a6757348741f72e4d876b906dc6 against base 3e3bacd60e1fca0aef2ce7a3c83516d45521973b. All eight changed files and related retry, profile-manager, Broker Load, REST, storage, and protocol paths were covered in three convergence rounds. The review found one P1 and five P2 issues, all reported inline. M5 and M6 are explicitly labeled pre-existing defects in touched or parallel paths.

The patch repairs the original short failed-dispatch cleanup gap when no history entry was published: removeProfile(Profile) now deletes attached execution IDs directly. It also prevents the earlier inner RpcException retry from finishing the shared Profile after its first attempt. Existing inline P1 comment 4140101788 described that inner retry and no longer applies to this head, so there is no still-applicable existing blocking comment ID. The separate outer cloud replan path remains affected (new P1 inline). There was no additional user-provided focus.

Critical checkpoints

• Goal and tests: The new ProfileTest cases verify map cleanup with and without a history entry and with multiple execution IDs. The StmtExecutorTest cases manually finalize mocked attempts and use empty fragment lists; they do not prove real dispatch/replan behavior, nonempty-fragment completion, configured threshold retention, or rendered profile content. The goal is only partially achieved because the inline findings remain.
• Scope and lifecycle: The ProfileManager map rename follows the same SummaryProfile ID; QueryProfileAction changes a method name only. The central lifecycle is registration before dispatch, running summary after dispatch, and terminal update before unregister. Inner retry now defers terminal update, but outer replan still finishes too early (P1), and abandoned attempts affect completion, retention, and rendering (P2 findings).
• Concurrency and locking: ProfileManager map operations use its read/write lock; Profile.updateSummary is synchronized. Late BE reports can hold an already retrieved execution object after map removal, but no new lock-order or report-thread race was substantiated. No cross-translation-unit initialization is involved.
• Configuration and parallel paths: No new configuration, transmitted FE/BE variable, protocol symbol, or format was added. The existing auto_profile_threshold_ms is applied incorrectly after a retry. MySQL local-result, Flight backend-result, outer replan, Broker Load, memory eviction, and storage paths were traced; the pre-existing Flight orphan registration and Broker Load retention defect are inline with their provenance stated.
• Tests, results, and observability: No result file changed. The review was static only: no builds or tests were run, as required by the review prompt; author or prior-thread test claims were not treated as independent validation. Existing logging and profile status expose some failures, but a successful retry can show an incomplete profile or lose its merged runtime view and actual row counts.
• Persistence, data, and performance: No EditLog, transaction, table-data write, or crash-recovery protocol changes were made. Profile file retention is covered by the Broker Load inline finding. The outer retry can retain execution state; abandoned attempts can delay spill and suppress a profile that exceeds its configured threshold. No other material hot-path cost was substantiated.

Disposition: Request changes for the P1 outer-replan lifecycle issue. The remaining accepted points are P2. No unresolved candidate remained after the final changed-file sweep.

[github-actions]

[P1] Keep the Profile open for the outer cloud replan retry too. A post-registration dispatch error containing NO_SCAN_NODE_BACKEND_AVAILABLE_MSG goes through the early throw in handleQueryWithRetry with willRetry=false, so this branch marks the shared Profile finished. queryRetry then starts a new query ID on the same StmtExecutor; both of its summary updates return at Profile.isQueryFinished, and unregister only marks its ExecutionProfile finished. That entry remains until size-triggered GC, while a retained history entry still describes the failed ID. Carry the outer retry decision into finalization or reset the Profile before the next attempt, and test a dispatch-time replan error followed by success.

[github-actions]

[P2] Do not let an abandoned dispatch keep the successful retry's profile incomplete. Real ExecutionProfiles start with nonempty fragment IDs and zero expected backend reports; if dispatch fails before fragments are sent, isCompleted() stays false. Retaining that attempt here makes Profile.shouldStoreToStorage wait the spill timeout and then mark the final successful profile INCOMPLETE even when its own backend reports finished. The new retry tests use empty fragment lists, for which isCompleted() is always true. Exclude the abandoned attempt from completion accounting or mark it terminal, and cover a nonempty failed attempt.

[github-actions]

[P2] Apply the configured query duration threshold after the final retry without multiplying it by retry count. This deferred finish keeps two ExecutionProfiles; Profile.updateSummary compares a 12-second query (11-second failed dispatch plus 1-second successful retry) against 2 * auto_profile_threshold_ms. At a 10-second setting the new path drops the whole profile, whereas the base retained a profile after the first 11-second attempt. Keep any Broker Load per-task policy separate and test a retry whose total falls between one and two thresholds.

[HappenLee]

Please fix this before merging. I rechecked the current head fbb274bf92640a6757348741f72e4d876b906dc6; the deferred retry finalization still applies the existing multi-execution threshold to attempts of a single query.

For example, with enable_profile=true and auto_profile_threshold_ms=5000, a retryable dispatch failure after about 100 ms followed by a successful 6-second attempt leaves executionProfiles=[E1,E2]. The inner retry does not reset the query start time, so finalization compares approximately 6100 ms against 2 * 5000 and removes the entire profile. Even the successful attempt alone exceeds the configured threshold.

To be precise about provenance: the multiplication formula predates this PR. This PR defers finalization until the last attempt, making that formula operate on all retained retry attempts. The base already had a premature-finalization defect, so this is not a claim that the base correctly retained the final successful attempt.

A simpler fix for ordinary query retries would be to discard the previous attempt's profile data before starting the next attempt and keep only the final attempt's execution profile:

• Remove the previous history entry and its execution-profile registration from ProfileManager.
• Also remove the previous execution profile from the shared Profile's executionProfiles list. Calling the current removeProfile(profile) alone does not clear that list.
• Keep the statement-level Profile open while retrying; finalize it only after the last attempt, whether that attempt succeeds or fails.
• Keep Broker Load's multi-task behavior separate. For query retention, preserve the statement's cumulative elapsed-time semantics rather than multiplying the threshold by retry count.

This also avoids making the successful query lose its merged profile merely because a failed attempt remains in the list.

Please add a regression unit test with a positive threshold and total duration between one and two thresholds, asserting that the final profile is retained and the abandoned attempt is removed from both the manager and the execution list. The current tests using a zero threshold or a large threshold do not cover this boundary.

Validation for this follow-up: static code-path review only; no additional tests were run.

[github-actions]

[P2] Render the successful retry's merged query profile. Keeping the failed attempt here makes executionProfiles.size() equal two, while Profile.getExecutionProfileContent only builds MergedProfile, fills rowsProducedMap, and annotates PhysicalPlan actual rows when the size is one. Thus a retained successful retry loses its normal merged runtime view and actual row counts in both REST and stored profiles. Show the last successful attempt's merged view while preserving earlier attempts separately, and assert profile text in a retry test.

[HappenLee]

Please fix this before merging as well. At the current head fbb274bf92640a6757348741f72e4d876b906dc6, retaining failed attempts in the shared Profile prevents a successful retry from producing its normal merged profile.

Example: enable profiling and set auto_profile_threshold_ms=0 to exclude the threshold issue. For an ordinary query such as SELECT SUM(v) FROM t, let the first dispatch fail with a retryable RPC error and the second attempt succeed with complete reports from two BEs. The shared list is then executionProfiles=[E1_failed, E2_successful].

Profile.getExecutionProfileContent() only calls getAggregatedFragmentsProfile(planNodeMap) when executionProfiles.size() == 1. With two attempts, it skips:

• Building and printing MergedProfile for the successful execution.
• Populating rowsProducedMap from that merged execution.
• Calling updateActualRowCountOnPhysicalPlan() to annotate actual rows through this path.

The raw attempt profiles are still printed and the SQL result is unaffected, but the successful execution loses its merged diagnostic view even when all of its BE reports are complete.

The size-one restriction already exists in the base; the retry-retention design needs to accommodate it. This is not a claim that the base's premature-finalization behavior correctly handled retries.

For ordinary query retries, the simpler fix discussed in the threshold thread is to discard the previous attempt's profile data before registering the next one, leaving only the final attempt in the execution list. Cleanup must remove both the manager registrations/history and the old list entry, while keeping the statement-level Profile open until the last attempt finishes. Preserve Broker Load's multi-task behavior. If retaining failed attempts is required, select the final attempt explicitly for the merged view and display earlier attempts separately; do not combine failed and successful executions' counters.

Please add a test with nonempty fragment profiles and backend reports that renders the retained profile after a successful retry, asserts that MergedProfile is present, and verifies actual-row propagation for a known plan node. Merely checking map membership or using empty fragment lists does not cover this behavior.

Validation for this follow-up: static code-path review only; no additional tests were run.

[github-actions]

[P2] Clean the failed Flight SQL attempt on an outer cloud replan retry. This is pre-existing: Flight beforeQuery switches results to the backends before registration, so a post-registration replan error makes isReturnResultFromLocal false and skips this finalization call. The failed attempt never reaches deferForArrowFlight, while beforeAttempt and request close act on the later query ID; its QeProcessor registration, execution profile, and finish callbacks remain. This differs from the local-result outer retry, which finalizes too early. Unregister the abandoned ID before replanning and cover this Flight path.

[github-actions]

[P2] Handle Broker Load IDs when pruning stored profiles. This is pre-existing: a Broker Load profile uses its decimal job ID, but after deleting or archiving the selected files this loop parses that ID as a TUniqueId on the next line. The parser throws for a decimal ID, so the outer catch stops cleanup and later selected profiles remain indexed although their files were already removed or moved. Remove each attached execution by its own query ID, as memory eviction does, and cover mixed query/load profiles.