Doris-Breakwater commented on issue #69861: URL: https://github.com/apache/doris/issues/69861#issuecomment-6503376824
The reported interleaving is supported by the code and warrants high-priority cloud/recycler correctness triage because it can make retained data eligible for premature physical deletion. This is a source-level finding; I have not run the proposed race reproduction or verified an actual missing-data incident. The issue currently has no labels. I checked the public `4.1.4` tag (`ad35a140c7fd`) and upstream `master` at `2a4191c1dc94`. Both contain the split reads. In particular: - `check_recycle_and_tmp_rowset_exists()` creates its own transaction. `check_rowset_exists()` scans formal rowsets through `txn_get()`, which creates another transaction **for each scan page**. The outer transaction in `process_single_packed_file()` is not passed into either helper. See the [4.1.4 helpers](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/recycler/recycler.cpp#L1240) and [current master correction path](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/cloud/src/recycler/recycler.cpp#L1487). - For the non-versioned metadata write path, compaction puts the input rowset's recycle record and removes its formal metadata in the same transaction, committed by `_finish_tablet_job()`. Thus the writer is atomic, but the correction reader can combine observations from opposite sides of that commit. See [compaction input recycling](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/meta-service/meta_service_job.cpp#L1173) and [commit](https://github.com/apache/doris/blob/ad35a140c7fd0b842f18c23300bac581f7d04326/cloud/src/meta-service/meta_service_job.cpp#L2082). The failing schedule is: 1. An eligible, uncorrected slice belongs to a formal rowset; its recycle and tmp keys are absent. 2. Correction reads recycle/tmp absence at snapshot A. Strictly speaking, this first helper does **not** read the formal key; that key merely still exists at this point. 3. Compaction atomically removes the formal rowset and creates its recycle record. 4. The subsequent formal-rowset scan at snapshot B cannot find that rowset ID. Correction marks the slice `deleted=true`, `corrected=true`, and excludes it from the recomputed reference count and remaining bytes. Physical deletion is conditional: correction must be due, the slice must not already be corrected/deleted, the packed-metadata update must commit, and the resulting packed-file reference count must reach zero. Other live slices can postpone object deletion, but the incorrect slice accounting remains. With zero references, correction sets `RECYCLING`, and `process_single_packed_file()` can delete the object in the same call. Its later KV reread checks packed-file state/count **after** object deletion; it does not recheck rowset retention. The packed-key conflict retry is not sufficient because the input rowset's formal-to-recycle transition does not itself update that packed key. See the [correction and deletion sequence](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/cloud/src/recycler/recycler.cpp#L1507). The ordinary rowset expiration calculation applies the compacted-rowset retention interval, but this orphan-classification path bypasses that chec k. See [retention calculation](https://github.com/apache/doris/blob/2a4191c1dc94e789e1725d6cb345f46c31a17272/cloud/src/recycler/recycler.cpp#L1863). The proposed shared-transaction approach addresses this interleaving. It must cover recycle/tmp reads **and every page of the formal-rowset scan**, with the whole existence decision restarted on transaction expiry/read failure. Merely putting the existing helpers behind one wrapper, or setting `snapshot=true` on independent transactions, would preserve the problem. Keep the packed-metadata write conflict/retry protection and the existing semantics that preserve recycle references without marking them corrected. [PR #68153](https://github.com/apache/doris/pull/68153) is still open/unmerged at review time; its checker changes do not modify this recycler correction implementation. Suggested validation and remaining evidence: - Add a deterministic regression beside `recycle_packed_files_respect_recycle_and_tmp_refs` in `cloud/test/recycler_test.cpp`. Pause after recycle/tmp absence, commit the formal-to-recycle transition, then resume correction. Use a packed file whose only remaining reference is the target slice; keep `force_immediate_recycle=false`, positive rowset retention, and an old enough packed-file creation time to make correction due. Assert that the unexpired recycle key, slice reference/count/bytes, and object survive. Exercise both commit orderings and a multi-page formal scan; also verify genuine orphans still recycle. - Please provide the exact deployed MetaService/recycler build SHA and whether versioned metadata reads/writes are enabled. The atomic transition traced above is specifically the non-versioned write path; versioned operation-log handling needs separate coverage before extending the conclusion to that mode. - To establish observed impact, attach a sanitized reproducer/test result, before/after `PackedFileInfoPB`, the matching formal/tmp/recycle metadata with recycle creation time, and effective `packed_file_correction_delay_seconds`, `compacted_rowset_retention_seconds`, `retention_seconds`, and `force_immediate_recycle`. Correlate compaction commit with `packed slice correction status`, `corrected packed file ref count`, and `deleted packed file` logs. Any claimed reader failure additionally needs the corresponding BE missing-object error and timestamp. A query profile is not needed to establish this metadata race. No source code was modified and no fix or regression-test pass is claimed. Breakwater-GitHub-Analysis-Slot: slot_3c21b68379d1 -- 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]
