Complete static review of PR #68610 at head 7e6d7e5764fa24c2fe7c74a9607a8ee2c02b010e. Request changes: two new P1 findings below; no existing P0/P1 inline finding applies. The focus file contains only -light and specifies no additional technical point, so I reviewed all four changed files and their relevant call paths.

Goal, scope, and tests. The two configuration defaults select ThreadPool schedulers for newly created local and remote workload-group scans, and the admission guard tries to avoid opening scanners after a shared LIMIT is exhausted. The patch is small, but the default switch exposes all ordinary scans and direct remote scheduler callers. The new unit tests assert registered defaults, scheduler types, and admission states; they do not execute a remote RPC under worker-creation failure or produce projected rows with buffered padding. No test result files changed. This review did not build or run tests, as the review contract prohibits both; author-reported test runs were not independent validation.

Correctness, error handling, and memory safety. The guard can end a projected parallel scan with rows already charged to the LIMIT still held in a scanner's private padding buffer (P1 inline finding). The remote ThreadPool can report submission failure after retaining a multiget_data_v2 callback, so the RPC may complete and later run a callback using released request/response/completion pointers (P1 inline finding). Other changed-path statuses and the wakeup/EOS handoff were traced; no separate lost wakeup or silent error was substantiated. The suggested nested RowId stack-reference variant was rejected because it runs from an already active worker.

Concurrency and lifecycle. Scanner workers, Context runnables, and the pipeline operator share ScannerContext; its pending/completed/in-flight transitions and dependency wakeups use _transfer_lock, and the shared LIMIT is atomic. The row-loss issue is an ordering/accounting error rather than a data race. Workload-group scheduler creation, resizing, and stopping use its scheduler lock; ThreadPool protects its queue. The RPC finding is a callback ownership failure after a zero-worker submission error. No new cross-translation-unit static initialization dependency, ownership cycle, or independent lock-order issue was found.

Configuration, parallel paths, and compatibility. These are process-start DEFINE_Bool defaults; an explicit true still selects TaskExecutor, and no dynamic-update behavior is promised. I traced local scans, file scans, direct remote submissions, and both scheduler branches. The projected LIMIT example requires a nonserial scan, reachable with enable_adaptive_pipeline_task_serial_read_on_limit=false or eligible predicates. The existing TaskExecutor pending-scanner check does not justify the new ThreadPool guard when a scanner holds counted padding. The patch adds no FE-BE variable, wire or storage format, glibc ABI requirement, transaction/write/persistence path, or visible-version/delete-bitmap change. It changes no BE hub header, gate table, PCH, or explicit template list; the added header include is confined to a test source file.

Performance and observability. Avoiding unnecessary scanner setup is useful, and the new guard is constant time, but it must preserve buffered rows. Existing worker/queue/submission metrics and scan profiles offer operational context; neither prevents the retained callback or makes a successful short LIMIT result safe. No additional distinct resource-bound or logging defect was substantiated. Three review rounds completed, with the final normal and risk-focused reviews returning NO_NEW_VALUABLE_FINDINGS; all candidates were accepted, deduplicated, or dismissed with code evidence.

Complete static review of PR #68610 at 30617c27748ff789ca6adbee4fd0af7a7f8c305b

Two new P1 findings are inline: a successful ThreadPool submission can be stranded after overlapping worker-start failures, and projected small-LIMIT scans with residual filters can read complete ranges before buffered qualifying rows are charged. The two existing P1 threads (4129581696 and 4129581700) are resolved for their exact reported error-return/later-callback and counted-padding row-loss scenarios; these new findings have different outcomes and are not duplicate reports.

• Goal and proof: The PR makes ThreadPool the default for new internal and external workload-group scan schedulers, aligns pending-task LIMIT admission, charges only emitted projection rows, and withdraws a single failed ThreadPool submission. The added tests cover scheduler selection, admission states, emitted-row accounting, and isolated failure/recovery. They do not cover overlapping failed starts or a small LIMIT with sparse residual predicates. I performed static inspection only; the review contract prohibits builds and tests, so the PR author's test claims are not independent validation here.
• Scope, conditions, and parallel paths: Both defaults are existing non-dynamic flags; explicit true still selects TaskExecutor. Local and external scanners share ScannerScheduler::_scanner_scan(), while the remote scheduler also accepts multiget_data_v2 work. The new admission guard preserves its one-task wakeup escape. The shared ThreadPool helper affects all callers; M1 applies to accepted work. The projection wrapper is common to scanner types; M2 is reachable with an AGG-key scan, a residual non-key value predicate, and parallel scanners.
• Concurrency and lifecycle: ThreadPool _lock protects queue, token, worker counts, and take markers, but worker creation occurs outside it; M1 is the resulting all-starts-fail interleaving. Context _transfer_lock protects admission and completion, and shared LIMIT progress is atomic. Cached scan results are published and drained before exhausted-counter EOS. No separate lock inversion, double release, ownership cycle, or cross-translation-unit static initialization issue was substantiated.
• Errors, memory, performance, and observability: An isolated failed submit now removes its callback before returning an error, and read/projection errors propagate. In M1 an accepted callback has no worker or error path. No new large untracked allocation was identified. Per-output-block atomic accounting and rare queue scanning are bounded; M2 instead permits full-range read amplification for a tiny LIMIT. Existing warnings, metrics, and scanner counters do not prevent these failures.
• Compatibility and data paths: No FE/BE wire field, function symbol, storage format, EditLog, write, transaction, visible-version, or MoW delete-bitmap path changes in this diff. The earlier counted-padding data-loss path is repaired for the reported case; operator reached_limit() trims overproduction. Header hygiene checkpoints found no new hub include, gate-table change, PCH edit, explicit-instantiation mismatch, or source-.cpp test include.
• Tests and focus: No generated result files changed. No build or test was run during this review. The focus file contains only -light and names no additional area; all 11 changed files were reviewed.

Review completion: two full subagent rounds and the additional risk scans converged with NO_NEW_VALUABLE_FINDINGS from every subagent in round 2. I validated and deduplicated every candidate, swept all 11 changed files, refreshed the live base/head and inline comments, and found no unresolved suspicious point. These two P1 findings request changes.

Review complete for d29c213. The existing P1 padding read-amplification thread still applies; request changes on that basis. With a projected, filtered LIMIT 5 and batch size 8, scanner A can buffer 1 row and emit 4, leaving one row both buffered and needed. The thread pool can then run scanner B before A is requeued. B has no local padding, so the new guard in Scanner::_get_block_after_projects() never checks that A's buffered row covers the remaining LIMIT. B may repeatedly read an all-filtered tail until range EOS. This is the same issue as the existing thread, so I am not posting a duplicate inline comment. The other three existing P1 sequences are addressed on this head by emitted-row charging, failed-submit withdrawal, pending-start waiting, and last-worker queue draining.

Critical checkpoints: The goal is to default both internal and external scans to the thread pool and preserve LIMIT behavior; both defaults and workload-group selections are wired, but the remaining padding case means the LIMIT performance goal is incomplete. The change is focused on those scheduler and scanner paths. Concurrency was traced from Context admission through worker execution and result delivery: scanner counters are atomic, task queues use the transfer lock, and thread-pool tasks use the pool lock; no separate lock-order or lifecycle defect was substantiated. The new shared counter belongs to the scan operator and no cross-translation-unit static dependency was added. These are startup configuration defaults, with no new dynamic configuration, wire or storage format, FE/BE variable, transaction, or data-write path. Both local and remote scheduler paths, conditional admission, failure wake-up, and final operator LIMIT cut were reviewed. The added unit cases cover defaults, emitted rows, buffered rows, admission, and worker-start failures, but do not cover the zero-local-padding case above; no new end-to-end test was added. Existing status and metrics paths provide failure observability, and no other concrete performance or correctness issue was confirmed.

User focus -light: the full 13-file PR was reviewed. This was static inspection only, as required by the review prompt; no builds or tests were run, and author-reported test results were not treated as independent validation. One round of full-review and risk-focused subagents converged with no new distinct findings, followed by a final changed-file and candidate sweep.

Existing P0/P1 findings confirmed for this head: #68610 (comment)

Review complete for head 3abe4030fe5f4b1dd6993d05338e194112512dd4 by static inspection. Request changes: the existing P1 buffered-row read-amplification thread still applies; I am reusing its inline comment rather than posting a duplicate. There are no distinct new inline findings.

With projected LIMIT 5 and batch size 8, scanner A can emit four rows while holding one padding row. B is already in flight, and a busy scan pool caps this Context to one outstanding scanner. Consuming A queues it but cannot admit it while B runs. B reads filtered rows, returns an empty non-EOS block because A's buffered row covers the remaining LIMIT, and is then pushed above A on the LIFO pending stack. The next Context runnable selects B again. Under sustained scheduler pressure this repeats through B's filtered tail before A can emit the row already available. The latest reply on that thread assumes A runs after B's first empty block; the new scanner-level test manually makes that call and does not test Context scheduling order. This is the same performance defect reported in the existing P1, confirmed on the current head.

Critical checkpoints

• Goal and proof: Both BE defaults now select ThreadPoolSimplifiedScanScheduler; the ThreadPool admission guard mirrors the TaskExecutor exhausted-LIMIT rule. Added unit tests cover default selection, admission, emitted-row accounting, buffered-row checks, and failed worker starts. They do not prove the scheduler-pressure sequence above.
• Scope and clarity: The changed production code is confined to scheduler defaults, scan LIMIT accounting/admission, and ThreadPool failed-start handling. The added counters and token bookkeeping have local explanations; no unrelated source changes were found.
• Concurrency and locks: Scanner tasks run on pool workers, while the pipeline task consumes completed results. _transfer_lock protects pending/completed/in-flight Context state, _lock protects ThreadPool queues and worker counts, and the shared LIMIT/buffered counters are atomic. Worker creation is outside the pool lock. No new lock-order deadlock was substantiated; the remaining concurrency defect is LIFO starvation under the one-scanner cap.
• Lifecycle and ownership: Emitted rows, padding rows, scanner EOS, Context completion, and failed task withdrawal were traced to their release/wake paths. The prior missing-row P1 and both failed-worker-start P1s appear fixed at this head. No separate lifetime or memory-accounting issue was substantiated.
• Configuration: Both existing static flags default to false and are read when workload groups construct local and remote scan schedulers. An explicit config value still chooses TaskExecutor. Already-constructed schedulers keep their selected type until reconstructed; no dynamic-change contract was added.
• Compatibility and parallel paths: No storage format, FE-BE protocol, persistence, or externally versioned interface changes were found. Internal/external scheduler branches and projected/non-projected scan paths were inspected; TopN has no ordinary shared LIMIT.
• Conditions and errors: The new exhausted-LIMIT admission guard leaves a completed/in-flight wake source, or admits one task when neither exists. Scanner projection and ThreadPool errors propagate through existing status paths. The buffered-row condition itself returns B after one read, but does not ensure A is scheduled next.
• Tests and results: New assertions were inspected, including negative failed-start cases. No build, test, or generated result was run or modified in this review, as required by the review prompt; author-reported local passes are not independent verification. A Context-level regression with A pending, B returning empty, and effective concurrency one is still needed.
• Observability and performance: The failed-start path logs a warning and uses existing pool metrics. The unresolved P1 can scan an entire filtered tail and delay a satisfied LIMIT; no other substantiated performance regression was found.
• Transactions, writes, and variable transport: This PR changes neither transaction/persistence behavior nor data writes nor FE-to-BE variable transport; those checkpoints are not applicable.

User focus: -light named no additional technical area. I still reviewed all 13 changed paths, related call chains, existing threads, and the final candidate list. The one-round normal and risk-focused reviews converged with no new valuable findings; the sole still-applicable blocker is the existing P1 above.

Existing P0/P1 findings confirmed for this head: #68610 (comment)

Review opinion

Complete static review of PR #68610 at 4dcf5397fe9831d1828081916466e99cdac6d74e: one new P2 performance finding is inline. No previously reported P0/P1 remains applicable on this head. The two normal full-review passes and separate risk scans converged in round 2; all 14 changed files and every candidate received a final disposition.

Critical checkpoints

• Goal and proof: Both existing config defaults now select the ThreadPool scheduler for newly created internal and external workload-group scanners. The code also applies the shared-LIMIT admission guard, charges emitted rows after projection, and withdraws a failed ThreadPool submit. New unit tests exercise these paths; their runtime results were not independently verified here.
• Scope and clarity: The changes stay within the affected BE scan, scheduler, pool, and test paths. The redundant zero-delta atomic operation in the inline finding is the remaining simplification.
• Concurrency and locks: Scanner tasks publish results under _transfer_lock; pending-task collections are changed together under that lock. The LIMIT and buffered-row counters are atomic. ThreadPool start, withdrawal, token queues, and shutdown are serialized by _lock; a start outcome wakes waiting submitters. I found no remaining lock inversion or stranded accepted task in the traced paths.
• Lifecycle and initialization: Context, scanner, callback, worker-start, and shutdown lifetimes were traced. Failed submits remove their runnable, while accepted work retains a worker. No new cross-translation-unit static initialization dependency was found.
• Configuration: No new setting was introduced. Both existing defaults become false; explicit true still chooses TaskExecutor when a workload-group scheduler is created.
• Compatibility: No wire, storage, function-symbol, or FE/BE variable format changes appear in the diff. No release ELF was built or audited for the glibc baseline under the review-only instructions.
• Parallel paths and conditions: Local and remote workload-group selection, ThreadPool and TaskExecutor admission, projected and ordinary scan reads, buffered-row retirement, exhausted-LIMIT escape, and worker-start failure paths were checked. The special branches have concrete progress or cleanup paths.
• Test coverage and results: The new BE tests cover scheduler defaults, LIMIT accounting, padding and holder scheduling, failed and overlapping worker starts, and max-thread shrink. No regression output file changed. This invocation ran no build, test, or style gate, as the review prompt forbids builds; author-reported runs are not independent validation.
• Observability: Existing scan counters and the new worker-creation warning support diagnosis; no separate observability defect was substantiated.
• Transactions, persistence, and writes: The PR changes read scheduling only; it adds no transaction, EditLog, persistent metadata, or data-write path.
• Performance and other issues: The inline P2 identifies the redundant shared atomic RMW on ordinary LIMIT reads. The projected empty-read variant uses the same helper and fix, so it is one finding. No other distinct correctness, memory-safety, or performance issue survived validation.

Existing threads and focus

The four existing P1 threads were checked against this head: emitted-only LIMIT charging and buffered-row scheduling address the scan concerns; pending-start waiting and failed-task withdrawal address the ThreadPool concerns. None is repeated or included as an existing blocking ID. The focus file contains only -light; the full PR was reviewed and no additional focus-specific issue was found.

Complete static review of PR #68610 at head 2d3f59f. The main review, two complete independent passes, and the focused counter and pool/default passes converged in one round. All 14 changed files and their related call paths were reviewed. No new substantiated finding remains. The five existing inline concerns were checked against this head; their fixes are present, and none of the earlier P1 findings still applies.

Critical checkpoints:

• Goal and proof: Both scheduler defaults are false, so new workload groups select the thread pool for internal and external scans. The scan path limits admission after shared LIMIT exhaustion, charges only emitted rows, and schedules a scanner holding needed padding rows. Failed worker starts withdraw rejected tasks. Added unit tests exercise these paths; the PR reports existing regression suites. This review did not run builds or tests, as instructed.
• Scope and reuse: The changes are confined to scheduler defaults, shared scan accounting/admission, thread pool submission guarantees, and their tests. Both TaskExecutor and ThreadPool paths use the same pending-scanner selection helper.
• Concurrency: Scanner workers publish progress with acquire/release atomics; the operator and Context workers serialize pending/completed task state under the transfer lock. Thread pool task pickup, withdrawal, and queue counters use the pool lock; pending-start waiters are notified on start, failure, and shutdown. No distinct lock-order or stranded accepted-task case was substantiated.
• Lifecycle and errors: Accepted Context runnables retain their Context until execution; rejected submissions mark it failed and wake its dependency. Completed scan blocks drain before Context EOS. No new ownership cycle, cross-translation-unit initialization dependency, or silently ignored new error path was found.
• Configuration and compatibility: The two existing BE configs change only their defaults; explicit configuration remains available. No new wire format, storage format, FE-to-BE variable, or rolling-upgrade protocol is introduced.
• Parallel paths and conditions: Internal and external scheduler selection, projected and unprojected scans, both admission paths, empty blocks, cancellation, and exhausted-LIMIT wakeups were traced. The one-pending-task escape allows EOS publication when no other task can wake the operator; its condition is documented.
• Tests and observability: New unit tests cover default selection, buffered-row scheduling, failed thread starts, withdrawal, and the last-worker case. No test output files changed. Existing counters and the failed-start warning cover the new failure path; no further metric need was identified. Author-reported test results were not treated as independent validation.
• Data correctness and performance: The operator retains the final LIMIT cut; no transaction, persistence, or data-write path changes. Zero-delta padding publication is skipped and unused scanners are not opened after LIMIT exhaustion. No additional supported correctness or performance issue was found.
• User focus: The focus file contains only “-light”; it names no additional code area. The full PR review is complete.

Request changes: two new P2 findings are inline, and three previously reported P1 findings still apply at e275f85. Review converged after 2 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final set; the final sweep covered all 14 supplied changed files and disposed of every candidate/risk.

The new findings concern repeated empty non-EOS scanner resubmission after shared LIMIT exhaustion, and RPC-test timeout teardown destroying captured objects before joining an active worker.

Existing blockers were independently confirmed and are carried without duplicate comments:

• 4129581696: minimum-worker creation can fail without failing pool initialization; a failed first-worker submit retains the RPC callback even after its error path completes done. A later submission can execute released request/response/done captures. This generic ThreadPool fix was intentionally left outside the diff, but the mechanism remains reachable with the new default.
• 4130378110: overlapping zero-worker start failures can leave an accepted task queued with no worker, stranding its completion. Positive min_threads does not exclude this state because initialization ignores thread-creation failure.
• 4130378115: for projected LIMIT 2, two scanners holding one matching row each still see a remaining counter of 2 and can read long filtered tails. The per-call target clamp does not combine peer-held rows; the author's latest reply explicitly leaves this amplification unfixed.

Critical checkpoint conclusions:

1. Goal/proof: both defaults select ThreadPool; emitted-only charging fixes the prior padding row-loss issue, and the RPC helper converts Doris exceptions before completion. Unit coverage was inspected; M1/M2 and the carried blockers limit the result.
2. Scope/reuse: focused changes reuse the existing exception macro, shared counter, and scheduler interfaces.
3. Concurrency/locks: workers and pipeline consumers share transfer_lock-protected pending/completed/in-flight state and an atomic remaining-row counter. Scans run outside that lock; no new lock inversion found. The new resubmission finding concerns progress.
4. Lifecycle/static state: the new test timeout order is unsafe; known pool submission ownership/liveness failures remain. No new production reference cycle or cross-translation-unit initializer introduced.
5. Configuration: existing immutable startup defaults change to false; explicit true still selects TaskExecutor separately for each table kind. No dynamic update is promised.
6. Compatibility: no wire/storage format changed; the helper is internal C++. No new libc API requirement apparent from source; no release binary audit performed.
7. Thrift/protobuf IDs: no gensrc changes.
8. Parallel paths: both schedulers, projected/nonprojected scans, disabled padding, no SQL LIMIT, key TopN, late filters, and external row-ID children were examined. Nested same-pool saturation also exists under the prior TaskExecutor and is not a new finding.
9. Conditions/comments: new conditions are explained; SQL-LIMIT presence correctly determines pointer presence even with a negative live counter. Empty non-EOS behavior still needs the scheduling fix.
10. Tests: new default-selection, scanner admission, emitted/padded accounting and real-ThreadPool exception cases cover intended success paths. Integrated exhausted-counter resubmission, safe timeout teardown, startup-failure recovery and split-padding coverage remain inadequate. No tests were executed.
11. Expected results: no golden output changed; unit assertions match intended successful behavior on inspection.
12. Observability: existing scanner profiles/queue counters, pool metrics and RPC query-id timing logs remain available; no additional distinct gap found.
13. Persistence/transactions: no edit-log, commit/version or failover changes.
14. Writes/atomicity: no data-write path changes; read-side correctness, errors and lifetime were reviewed as above.
15. FE-BE variables: none added or changed.
16. Performance: new empty-task churn and existing parallel-padding read amplification remain; large-LIMIT coalescing is preserved and obsolete buffered-row zero-delta atomic work is gone.
17. Other concerns/focus: supplied focus is only -light, with no extra functional question. Full changed-file coverage and missed-issue rechecks were retained; no unresolved candidate or uncovered source scope remains.

Existing findings 4129581700 (lost counted padding) and 4140089770 (uncaught row-ID Doris exception) are fixed; 4132871043 is obsolete, and 4226650611/4227165026 are addressed. Human EOS and BE_TEST-setter requests were checked. Static review only: no builds, tests, benchmarks or source modifications; author-reported runs were not treated as executed evidence. No measured spin rate or inevitable permanent hang is claimed.

Review converged after 2 round(s).

Existing P0/P1 findings confirmed for this head: #68610 (comment), #68610 (comment), #68610 (comment)

Static review of PR #68610 at head 62afc6b03b7e55ba938eec1a5e13c2cad1cd8844 against base 4ffe6632b8cf75a0d647719c5f7a5f77308f0e5f. The review covered all 14 changed paths, the complete authoritative diff, required AGENTS.md guidance, upstream/downstream scanner and workload-group paths, and all existing review threads. No new inline finding survived verification or deduplication; the three existing P1 findings below remain applicable and are carried without reposting.

Existing blocking findings:

• 4129581696: ThreadPool can retain an accepted remote RPC callback after a failed first-worker submission; multiget_data_v2 completes the RPC on the error path, and a later worker can execute the stale callback. ThreadPool initialization still ignores minimum-worker creation failures, and this PR changes the defaults to select that pool.
• 4130378110: overlapping worker-start failures can leave accepted tasks queued with no running or pending worker. This is distinct from the retained callback case and remains in unchanged generic ThreadPool code.
• 4130378115: the pending-holder projected LIMIT follow-up can make a scanner with no local padding read a long filtered tail while another scanner holds the last needed row. Results remain complete, but this is a real read-amplification issue intentionally left in the current head.

Critical checkpoint conclusions:

1. Goal and proof: both existing scheduler flags now default to ThreadPool for newly constructed internal and external workload-group schedulers. Shared LIMIT charging is performed for emitted rows after projection, the pointer is selected from immutable SQL LIMIT state, exhausted scanners are parked while outstanding work drains, and row-ID RPC exceptions are serialized as response status. Added tests cover scheduler selection, LIMIT accounting/padding/EOS and late initialization, and an unknown row-ID type; this invocation is static only and ran no build or test.
2. Scope and reuse: production changes remain focused on scheduler defaults, scanner LIMIT accounting/admission, and the RPC exception boundary, using existing scanner/context, ThreadPool, Status, and exception mechanisms.
3. Concurrency and locks: scanner workers and pipeline consumers coordinate pending/completed/in-flight state under ScannerContext::_transfer_lock; the shared LIMIT is atomic; scan/projection work runs outside that lock. No new lock inversion was substantiated. The generic ThreadPool worker-start failures above remain.
4. Lifecycle and memory: emitted rows are charged only after they are placed in the output block; task completion publishes before waking the dependency. RPC callbacks attach the row-ID tracker and serialize status before ClosureGuard completes. No new ownership cycle, static-initialization dependency, untracked large allocation, or nullable/COW defect was found.
5. Configuration and compatibility: no new setting, wire field, storage format, persistence, transaction, or FE-BE variable was introduced. Explicit true values still select TaskExecutor; flags are sampled at scheduler construction.
6. Parallel paths and conditions: local/remote schedulers, TaskExecutor/ThreadPool routes, projected/non-projected and nonmergeable scans, no-LIMIT/key-TopN, adaptive/low-memory admission, late scanner initialization, EOS, cancellation, and RPC submit/error paths were traced.
7. Tests and observability: new BE unit-test source meaningfully exercises the changed invariants, but no runtime validation or generated result file was run or changed here. Existing context diagnostics, scanner counters/profiles, pool metrics/warnings, and RPC status/timing logs remain available.

User focus -light: the focus file names no additional technical area; the complete changed-file review found no focus-specific issue. Convergence is true after round 1: the normal scanner reviewer, normal RPC/default-scheduler reviewer, and separate risk-focused reviewer all returned no new valuable findings, and every candidate/risk has an evidence-based disposition. The review remains blocking only because the three confirmed existing P1 findings are carried in existing_blocking_comment_ids.

Review converged after 1 round(s).

Existing P0/P1 findings confirmed for this head: #68610 (comment), #68610 (comment), #68610 (comment)