Doris-Breakwater commented on issue #66001:
URL: https://github.com/apache/doris/issues/66001#issuecomment-5067346833
## Preliminary maintainer assessment
**Verdict: confirmed concurrency correctness bug, with high-confidence
code-level root cause.** The issue's low-frequency/P2-style impact assessment
is reasonable, but the invariant violation is real and should be fixed rather
than treated as an observability-only problem. The issue is currently open with
no labels, assignee, milestone, or linked development item.
### Code-path confirmation
I checked PR #65126 at head `19b7bdd5f538d2e8f398cfca79d8a6fbb5c46802` and
its merge-base baseline `805b746addd7496e2df520d85b8fc6fe006f8636`.
- In the PR, `ExternalDatabase.getTableNullable(String)` calls
`tables.get(finalName)` and then independently calls `tableIdToName.put(...)`.
`ExternalCatalog.getDbNullable(String)` has the identical `databases.get(...)`
/ `dbIdToName.put(...)` sequence. A hot read can therefore observe an object,
lose the race to explicit invalidation, and restore only the ID mapping
afterward.
- The miss path is also confirmed. `MetaCacheEntry.getWithManualLoad()`
returns the loaded value even when its captured generation changed and cache
publication was rejected. The caller cannot distinguish that result from a
successfully published object and consequently republishes the ID mapping.
- Incremental updates have the same split commit: `updateTableCache()` /
`updateDatabaseCache()` perform object `put` and then ID-map `put`, while
`invalidateTableCache()` / `invalidateDatabaseCache()` invalidate the object
and then remove matching ID entries. Full invalidation similarly invalidates
objects before clearing the ID map. None of these pairs share one linearization
point.
- This class of problem predates the refactor. Baseline
`MetaCache.getMetaObj()` publishes to `metaObjCache` before `idToName.put`,
`updateCache()` also updates the object before the ID map, and `invalidate()`
removes them in separate operations. PR #65126 adds the new hot-hit late-write
window but is not the sole origin.
The resulting stale entry is persistent because the ID maps have no TTL.
Normal by-ID lookup then resolves the stale name and calls
`databases.get(name)` / `tables.get(name)`. After a drop or rename, the
names/existence check will usually reject the reload, so the most likely
symptom is repeated load work or failed by-ID resolution rather than a stale
object on common name-based query paths. Replay by-ID lookup remains cache-only
and normally returns empty once the object entry is gone. This supports the
issue's current impact assessment without weakening the correctness finding.
### Required fix invariant
The fix should provide one per-object-key publication protocol covering the
object entry and its ID navigation entry:
1. Add the ID mapping only when object publication is still valid under the
same publication/generation fence.
2. Remove the ID mapping during explicit key/full invalidation under that
same fence, so an earlier lookup or update cannot write it back afterward.
3. Do not publish an ID mapping for a generation-rejected load, even though
the current in-flight caller may still receive the loaded object.
4. Route hot named lookup, manual miss load, explicit/incremental `put`,
unregister/drop/rename, and full reset through the same primitive; removing
only the two late `put` calls is insufficient.
A coordinator or a `MetaCacheEntry` atomic callback/result API would both be
viable. The important property is that “object is current” validation, ID-map
mutation, and explicit invalidation share the entry's short publication lock;
slow remote loading must remain outside that lock.
One compatibility detail must be preserved: the ID map intentionally
outlives ordinary TTL/capacity eviction so normal by-ID lookup can reload a
cold object. Cleanup should therefore be coupled to explicit
invalidation/unregister/reset, not indiscriminately to every Caffeine removal
cause.
### Recommended deterministic validation
- Add latch/hook-based tests at both catalog and table levels for:
- hot named hit paused before ID publication vs unregister/drop and rename;
- miss loader paused while explicit invalidation advances the generation;
- generation-rejected publication returning an object to the in-flight
caller without leaving either an object entry or ID mapping;
- incremental object update vs key invalidation, plus full
invalidation/reset.
- After each invalidation wins, assert all of the following: the object
cache is empty for the old key, the old ID mapping is absent, replay-by-ID is
empty, and normal by-ID does not restore the mapping or repeatedly invoke the
loader.
- For rename, assert the old ID/name pair cannot reappear and only the
successfully published new object owns the new mapping.
- Keep focused coverage in `ExternalCatalogTest` and `ExternalDatabaseTest`;
extend `MetaCacheEntryTest` if the chosen atomic API lives there. Run the
corresponding FE unit-test suites.
### Missing information
No runtime logs, profile, or production reproduction are needed to confirm
this code-level race. The only non-blocking triage gaps are the intended target
base (stacked on #65126 versus follow-up after merge) and which public release
branches containing the baseline implementation should receive a backport.
Please also add the repository-standard bug and external-catalog/meta-cache
labels and assign an owner.
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]
