[Bug] MetaLockUtils.commitLockTables leaks acquired commit locks on rollback (unlocks wrong index)

[LuciferYang]

Search before asking

• I had searched in the issues and found no similar issues.

Version

master (MetaLockUtils.commitLockTables).

What's Wrong?

MetaLockUtils.commitLockTables has a rollback bug: when commitLock() throws for the table at index i, the rollback loop iterates j from i-1 down to 0 but unlocks tableList.get(i) (the table that failed to lock) instead of tableList.get(j) (the previously locked tables):

public static void commitLockTables(List<Table> tableList) {
    for (int i = 0; i < tableList.size(); i++) {
        try {
            tableList.get(i).commitLock();
        } catch (Exception e) {
            for (int j = i - 1; j >= 0; j--) {
                tableList.get(i).commitUnlock();   // should be get(j)
            }
        }
    }
}

Consequences on the failure path:

• Commit locks already acquired for tables 0..i-1 are never released (leak).
• The failing table (i) is commitUnlock()-ed repeatedly on a lock it does not hold.
• The exception is swallowed and the outer loop keeps trying to lock the remaining tables.

The correct siblings tryCommitLockTables and writeLockTablesOrMetaException use get(j) and propagate the failure.

Note: in normal operation commitLock() → MonitoredReentrantLock.lock() does not throw, so this catch is defensive and the bug is latent; the fix makes the rare failure path correct.

What You Expected?

On a commit-lock acquisition failure, the locks already acquired should be released (get(j)) and the failure propagated, instead of leaking locks and silently continuing.

How to Reproduce?

Code inspection: commitLockTables unlocks get(i) instead of get(j) in its rollback loop, diverging from every other lock helper in the class. A unit test that injects a table whose commitLock() throws shows the previously-locked table is left locked.

Anything Else?

Fix proposed in PR #64676.

Are you willing to submit PR?

• Yes I am willing to submit a PR!

[zclllyybb]

Breakwater-GitHub-Analysis-Slot: slot_a0ba233667ad
This content is generated by AI for reference only.

Initial assessment: this looks like a valid latent bug in MetaLockUtils.commitLockTables on current master.

I checked upstream master at 90db340515e88e177bc4e6151ba0b9a84cf04d8c. In fe/fe-core/src/main/java/org/apache/doris/common/util/MetaLockUtils.java, the failure rollback path for commitLockTables() iterates j = i - 1 ... 0, but still calls tableList.get(i).commitUnlock(). That diverges from the nearby table-lock helpers, which unlock the already acquired entries with get(j) and then propagate the failure.

The consequence is not only stylistic. The main caller I found is the cloud-mode branch of BrokerLoadJob, which calls MetaLockUtils.commitLockTables(tableList) before entering the later transaction try/finally that unlocks all tables. Therefore, if commit-lock acquisition fails partway through the list, commitLockTables() itself is responsible for releasing the earlier locks. With the current code, tables 0..i-1 are not released on that path.

One nuance: the issue says the original exception is swallowed and the loop keeps going. The method indeed does not explicitly rethrow e; however, with the current Table.commitUnlock() implementation, commitUnlock() delegates to MonitoredReentrantLock.unlock(), which delegates to ReentrantLock.unlock(). If the failed table's lock is not owned by the current thread, the wrong rollback call can raise an unlock-side runtime exception instead. Either way, the already acquired commit locks remain unreleased, and the original acquisition failure is not handled correctly.

PR #64676 appears to address the two important parts of the fix:

• change the rollback target from tableList.get(i) to tableList.get(j);
• rethrow the acquisition failure after rollback;
• add a focused FE unit test covering rollback and failure propagation.

Suggested maintainer next step: review PR #64676 as the direct fix. I do not think extra runtime logs are required to validate the bug because the affected branch is visible by code inspection. If maintainers want to assess whether this has already occurred in a running deployment, the useful evidence would be FE logs around cloud-mode broker-load transaction commit failures plus a thread dump or lock-owner evidence showing a table commit lock held after a failed commit-lock acquisition.