Doris-Breakwater commented on issue #66001: URL: https://github.com/apache/doris/issues/66001#issuecomment-5067342646
## Breakwater initial analysis **Triage:** Confirmed FE external-metadata-cache correctness bug. Recommended priority is **P2 / medium**: the race requires concurrent lookup and drop/rename/invalidation, but it can leave state that explicit cleanup has already removed. The issue is currently open with no labels, assignee, milestone, project, or linked implementation. ### Code-level finding I verified the behavior against PR #65126 at head `19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802`. The root cause is that the object cache has a key-scoped publication protocol, but the corresponding ID-to-name map does not participate in that protocol: - [`ExternalCatalog.getDbNullable(String)`](https://github.com/apache/doris/blob/19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java#L749-L779) calls `databases.get(dbName)` and then performs `dbIdToName.put(...)` after `MetaCacheEntry.get()` has returned. - [`ExternalDatabase.getTableNullable(String)`](https://github.com/apache/doris/blob/19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java#L535-L549) has the identical late `tableIdToName.put(...)`. - Table invalidation removes the object and then removes matching ID bindings in [`invalidateTableCache`](https://github.com/apache/doris/blob/19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalDatabase.java#L717-L727); database invalidation has the same split sequence in [`invalidateDatabaseCache`](https://github.com/apache/doris/blob/19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802/fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java#L1410-L1420). `ConcurrentHashMap` makes each map operation safe, but it does not make the combined object/ID transition atomic. - On a miss, [`MetaCacheEntry.getWithManualLoad()`](https://github.com/apache/doris/blob/19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCacheEntry.java#L306-L347) intentionally returns the loaded object when its captured generation was invalidated, while refusing to cache it. The upper-layer late `put` cannot distinguish that rejected publication and therefore creates an ID binding for an object that was never committed. - The baseline claim is also correct: the legacy [`MetaCache`](https://github.com/apache/doris/blob/27cb451229c1893fbf182851a202379e26dadc76/fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/MetaCache.java#L95-L155) writes `idToName` after miss loading or object update, while `invalidate()` is not synchronized with that combined sequence. PR #65126 adds the hot-hit form of the race but does not introduce the entire bug class. The immediate consequence is a stale ID-to-name binding after explicit cleanup. A later by-ID lookup can follow that binding and attempt another object load. This does not by itself demonstrate incorrect query results on ordinary named lookup paths, so the issue's limited-impact assessment is reasonable; however, the stale navigation is a definite violated cache invariant rather than a speculative risk. ### Recommended fix contract The durable fix should make object publication and ID binding one key-scoped commit: 1. Remove the read-side ID-map repair from the two named lookup methods. A hot read should not mutate navigation state. 2. Publish the ID binding only as part of a successfully committed object load or explicit object publication. A generation-rejected load may still be returned to its initiating caller under the current API contract, but it must not populate either the object cache or the ID map. 3. Run explicit invalidation/unregister and ID-binding removal under the same key publication lock/generation boundary. A post-invalidation writer from an older operation must be unable to restore the binding. 4. Route incremental register/update through that same key-scoped mutation API, including the intentional cold-object case where only navigation state is retained. Otherwise register/update can still race with drop/rename even after the named-lookup writes are removed. 5. Preserve the documented distinction between passive TTL/capacity eviction and explicit cleanup: an ID binding may intentionally outlive passive object eviction so by-ID lookup can reload, but it must not outlive drop, rename of the old key, unregister, reset, or explicit invalidation. This is best encapsulated in a small coordinator or in outcome-aware `MetaCacheEntry` mutation APIs (for example, a successful-publication callback and an invalidation callback executed while holding the entry's existing publication lock). A separate map write followed by a cache-presence recheck would remain a TOCTOU race and is not sufficient. ### Tests required Please use latches/test hooks rather than sleeps and cover both database and table coordinators: - hot named hit paused at the publication/binding boundary versus drop and versus rename; - slow miss load versus drop/rename, with invalidation occurring while the loader is blocked; - generation-rejected load: the racing named caller may receive its loaded value, but the object is absent from cache and the ID binding is absent afterward; - incremental register/update versus invalidation, including the cold-object/hot-ID behavior; - full reset/invalidate-all if it uses a different generation path. Each test should assert the object entry, ID binding, and by-ID behavior together. For drop, by-ID lookup must return null without triggering a fresh object load. For rename, the old name must not be restored and the ID must resolve only according to the final rename state. ### Missing decisions / next steps No production log, profile, version string, or user reproduction is needed to establish this bug; the race is confirmed statically. Before implementation, maintainers should record: - whether the current behavior of returning a generation-rejected loaded object to the initiating named caller remains intentional; - the exact passive-eviction versus explicit-invalidation ID-retention contract; - the target base and backport branches after PR #65126 is finalized. Suggested next step: assign the FE/external-catalog cache owner, add the coordinator-level deterministic tests first, then implement the shared publication/invalidation API and apply it symmetrically to database and table paths. Breakwater-GitHub-Analysis-Slot: slot_c0caa06b02a5 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
