🟡 Changes recommended

Critical cache consistency and version-publication issues remain unresolved, along with a partial-refresh failure path.

Get a fresh assessment by requesting another Copilot review.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Requesting changes after a static-only exact-head review of dd23972f9672d90d2b2097cace05cb2a8fc4b307.

Checkpoint conclusions:

• Goal/correctness: the staged daemon path improves failure atomicity and epoch preservation, but SQL-cache token/snapshot coherence, forced publication, and disable/re-enable transitions still have stale or mixed-cache gaps. The first two are already covered by live same-head threads; the attached comments add only distinct issues.
• Scope/tests: the production changes are focused and the added tests are broad, but they miss the attached cases; the unchanged finite-TTL regression directly conflicts with the new disabled-syncer behavior.
• Concurrency/lifecycle: metadata/version-lock ordering was traced and no inverse-lock deadlock was found. The forced path nevertheless waits for healthy lazy commits while holding metadata read locks, and the mutable syncer flag has no enable-generation recovery hook.
• Compatibility/data paths: there is no new persisted format, FE/BE field, or RPC symbol; wait_for_pending_txn is already optional. Commit/push, scalar/batch refresh, SQL-cache, forced SHOW, DDL/topology, and add/drop/restore paths were checked.
• Errors/observability/performance: staged daemon failures remain retryable and logs identify failed tables, but hot-write non-convergence and catalog-wide snapshot retention create avoidable RPC, cache-miss, heap, and GC pressure.

No additional user focus was supplied. Per the review contract, I did not build or run tests; git diff --check passed. Four same-head Copilot threads were present at the final recheck, so their SQL-cache, forced-sync, and foreground-snapshot issues are not duplicated here.

Requesting changes after a static-only exact-head review of 7bae5545b7d6394df738fbeb726c578680ac4033.

Checkpoint conclusions:

• Goal/correctness: the new-FE/new-MS pending marker correctly keeps pending partition versions from validating the cache, but the two rolling-upgrade directions remain unsafe. A new FE trusts omitted state from an old MS, while an old FE ignores the new response-only field from a new MS.
• Scope/reuse: the homogeneous-version implementation is focused and reuses the existing cache epoch mechanism; current-MS scalar and batch producers preserve response order and cardinality.
• Concurrency/lifecycle: foreground, daemon, push, invalidation, retry, and lock orderings were traced. The captured epoch cannot acknowledge a later invalidation, monotonic publication prevents downgrade, and no new deadlock or runtime-field lifecycle defect was found. The previously reported mixed-snapshot and SQL-cache races were not duplicated.
• Compatibility/parallel paths: additive protobuf encoding is wire-compatible, but semantic capability negotiation is missing in both upgrade directions. Scalar, batch, legacy, versioned, missing, first-write, waiting, and non-waiting paths were checked.
• Configuration: no new setting is introduced, but the existing infinite default table/partition TTLs make best-effort push and daemon repair insufficient as correctness fences.
• Transactions/persistence/errors: lazy-commit table/partition timing and wait success/failure were traced. No new EditLog, persisted format, FE/BE data path, or partial-error publication issue was found.
• Observability/performance: the legacy-response debug log is useful, and pending periods intentionally refetch; no distinct new logging, allocation, lock-hold, or RPC-amplification issue survived review.
• Tests/results: the added tests cover the homogeneous new/new protocol well, but the legacy-response assertion encodes unsafe caching and there is no old-client capability/rollout-convergence coverage. No result-file change applies. Per the review contract, I did not build or run tests.

No additional user focus was supplied. The nine existing inline discussions were treated as duplicate fences; the two attached P1 comments are distinct compatibility defects.

Static review of exact head adc99c7. The review converged after three rounds over all 12 changed files and the relevant FE/MS call chains. I found two P1 correctness/upgrade issues and three P2 lock/scaling issues that are distinct from the existing inline threads.

Checkpoint summary: the same-binary pending-state protocol is internally cardinality-consistent, but a pending response can still publish future metadata into shared FE state, and upgraded FE images retain the former infinite TTL. Scalar/batch, legacy/versioned storage, first-write, retry/error cleanup, clone routing, lazy-commit persistence, delete-bitmap, proc, daemon, planner, session/global forwarding, image replay, DDL/failure, and direct time/TSO consumers were traced. Existing mixed-binary, cross-partition publication, proc, daemon recovery, staged-memory, and SQL-cache findings were treated as hard duplicate fences. The new wait flag is forwarded consistently; no new FE-BE field is required. Existing observability is adequate, but the new daemon critical section and finite-default foreground refresh are unbounded in partition count.

Validation was static only, as required: I did not run builds, unit tests, regression tests, or runtime validation, and author/CI claims were not treated as independent proof.

Reviewed the complete supplied 12-file diff at head 344f3dad7e362553401395d4bd54fe145bfdf208. Static review converged in 1 round: both complete-review agents and the separate cache-risk reviewer returned NO_NEW_VALUABLE_FINDINGS; all candidates, existing threads and R1-R3 risks have evidence-based dispositions. No new inline findings are needed. Changes are still requested for five independently revalidated existing P1s:

• 4032914010: forced SHOW waits for lazy transactions while holding table/MTMV metadata read locks. The configured retry envelope applies; the current default is 200 attempts with a 30-second attempt timeout, rather than the historical comment's three-attempt example.
• 4035088962: absent old-MS pending state still validates the cache; legacy first-write batch version 0 is not normalized and reaches the scan's nonpositive-version error. The fresh default is now finite, but persisted/explicit infinite TTL cases remain.
• 4035088973: old FE clients ignore field 9 and can cache pending old/initial partition versions together with an advanced table token; request capability negotiation and upgrade cache recovery remain absent.
• 4045487571: pending responses invalidate the epoch but still install a mismatched visible-version/time/TSO tuple on a lagging FE. Direct metadata consumers bypass epoch checks; equal-version pushes cannot repair it.
• 4045487595: image replay restores the persisted infinite TTL over the new initializer; no versioned migration covers the old default.

The disabled-syncer TTL-bypass thread 4032914017 and dirty-marker/re-enable thread 4032914020 no longer describe this head: the bypass is absent, the table TTL remains infinite, and daemon selection polls managed tables by interval. Historical staged-map/reject-on-token-race branches are also absent. Existing foreground/proc snapshot, wait-under-lock, full invalidation sweep and foreground batching concerns were checked and retained as duplicate fences; no copies are posted.

Individual critical-checkpoint conclusions:

1. Goal and proof: upgraded MS/FE scalar and batch paths propagate pending state and leave pending responses uncacheable; unit cases cover policy and first-write normalization. The five known P1s prevent accepting the complete upgrade/correctness goal.
2. Scope and reuse: changes stay within version reads/cache policy, reuse existing waiting/RPC machinery, and centralize implicit wait selection and publication.
3. Concurrency and locking: query, daemon, commit, push and DDL interactions were traced. Epoch acknowledgement preserves later invalidations and monotonic setters prevent lower-version replacement. No distinct new lock inversion was found; heavy locked waits/sweeps and snapshot gaps remain known concerns.
4. Lifecycle: cache epochs and flags are transient; existing daemon pools and deduplicated lazy tasks retain their cleanup owners. No new resource lifetime or static initialization issue was found.
5. Configuration: the new session boolean supports standard SET/global fallback and forwarding; uncached reads observe it immediately. Explicit proc/daemon/MoW policies remain authoritative. Persisted TTL migration remains the known P1.
6. Compatibility: new-to-new behavior is consistent; old-MS/new-FE and new-MS/old-FE behavior remains blocked as above. No persistent schema or function-symbol removal was introduced.
7. Shared definitions: complete GetVersionResponse definitions were compared on HEAD and origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2. Field 9 is unused on every release ref; no reused/colliding ID or new required field was found.
8. Parallel paths: scalar/batch, legacy/versioned layouts, missing/first-write values, waiting/nonwaiting, clone-source instances, foreground/forced/daemon/proc and MoW paths were covered. The cloud/shared-nothing split remains intact.
9. Conditions: has_version normalization handles the real pending-first-write producer, pending flags are aligned with versions, and malformed flag cardinality leaves caches invalid. Omitted flags are the already-reported unsafe exception.
10. Test coverage: all five changed test files were inspected. They cover both layouts, scalar/batch pending and first-write responses, invalidation, explicit/session/default waits, and scalar MoW cache bypass. Mixed-version descriptors, persisted-image migration, real lock contention and large-table behavior lack corresponding added end-to-end proof.
11. Expected results: inspected mock/version expectations match the intended new-MS behavior; the legacy-cache test preserves the known compatibility issue. No regression output files changed.
12. Observability: existing RPC latency/profile, retry, lazy-wait and daemon failure logs remain available. No distinct new observability defect was found.
13. Persistence and failover: no new durable transaction state or EditLog record is required; MS pending state remains durable and lazy work can be reconstructed. Default-session image replay is the known migration gap.
14. Writes and atomicity: scalar MoW reads now match batch's forced MS read and configured waiting policy after bitmap-lock acquisition, then use visible version +1; callers retain failure cleanup. No distinct new delete-bitmap/sentinel, crash-leak or self-wait defect was found. Known query/proc publication gaps remain.
15. Variable propagation: the new boolean uses FE session forwarding and the existing FE-to-MS wait field; no new BE query-option plumbing is required.
16. Performance and memory: pending flags add linear work to existing partition loops. The new full-table invalidation under locks and periodic unbounded foreground refresh remain existing P2s; scalar MoW deliberately trades cache hits for current MS versions. No distinct new growth mechanism was verified.
17. Final sweep: all 12 paths, 16 existing comments, five carried blockers and R1-R3 dispositions were rechecked; no unresolved static candidate or uncovered assigned path remains.

No additional user focus was supplied. Validation was static only, as required: no builds, tests, source changes or runtime-performance claims. Convergence is complete; the review verdict remains blocking because the five existing P1s still apply.

Review converged after 1 round(s).

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

Request changes at head 344f3dad7e362553401395d4bd54fe145bfdf208. No additional distinct inline finding was verified. Five previously reported P1 findings still apply and are retained as blocking comments rather than reposted:

• 4045487595 — persisted old TTL: image replay restores the annotated Long.MAX_VALUE over the new ten-minute initializer; no versioned migration changes that value. Existing untouched clusters retain the infinite partition TTL.
• 4045487571 — pending metadata tuple: pending answers invalidate the epoch but still install an old visible version with the pending transaction's timestamp/TSO on a lagging FE. Direct metadata accessors ignore validity, and equal-version updates cannot repair the tuple.
• 4035088973 — old FE compatibility: old FEs ignore the new response field and can cache pending versions alongside the advanced table token. There is no capability negotiation or upgrade-completion cache recovery.
• 4035088962 — old MS compatibility: omitted pending state is still accepted as cacheable. An old MS also returns zero for a pending first-write batch entry, which this FE does not normalize and ScanNode rejects. The finite initializer does not fix existing infinite-TTL sessions or the zero-version error.
• 4032914010 — waiting under metadata locks: forced SHOW/proc refresh retains table and related MTMV metadata read locks across sequential waiting RPCs. The configurable VersionHelper retry envelope can block metadata writers; its current defaults are 200 attempts and 30 seconds per attempt, broader than the earlier comment's example.

The inspected interaction is lazy commit -> pending VersionPB plus advanced table token -> scalar/batch get-version -> FE epoch/tuple publication -> planner/ScanNode snapshot -> BE visible-version bound. Concurrent daemon workers and FE commit/push workers update the same caches. Forced proc refresh holds metadata read locks during its RPCs; daemon invalidation takes metadata read lock then version write lock; batch and push publication use sorted table version locks.

Critical checkpoint conclusions:

1. Goal and correctness: explicit pending answers on upgraded MS/FE pairs remain epoch-invalid, including pending first writes. Full correctness is blocked by the tuple and rolling-upgrade findings above. Added unit tests inspect the intended local behavior, without proving upgrade safety.
2. Scope and reuse: the twelve changed paths are focused on pending-state propagation, shared cache policy, wait selection, scalar MoW cache bypass, and daemon invalidation. Common cacheability and implicit-wait helpers avoid scalar/batch policy duplication.
3. Concurrency and locks: captured epoch acknowledgment preserves invalidations after request start. Existing metadata-lock waits, full-table locked sweeps, and cached/fetched snapshot races remain. No additional lock-order deadlock was verified.
4. Lifecycle and initialization: no new worker lifecycle, static initialization dependency, or ownership cycle was introduced. Response metadata and session variables follow existing ownership; cache epochs remain transient.
5. Configuration: the new wait boolean defaults false, supports SET/forwarding, and is read at cache misses. Explicit daemon, proc, incremental-read and bitmap policies retain precedence. The partition TTL changes to ten minutes for fresh defaults; persisted old values are not migrated. Table TTL remains infinite by default.
6. Compatibility: no durable storage representation changes. Semantic mixed-MS/FE compatibility remains blocked by the two existing upgrade findings.
7. Protobuf: complete GetVersionResponse definitions on HEAD and origin/branch-3.1, branch-4.0, branch-4.1 and branch-4.2 were compared. Existing ids retain meaning; field 9 is unused on those releases, repeated and optional on the wire. No field-id collision, reuse, or new required field was found.
8. Parallel paths: scalar/batch, legacy/versioned MS storage, missing/nonpending/pending-first-write responses, foreground/daemon/proc/MoW consumers, and shared-nothing fallbacks were checked. Forced scalar bitmap reads now follow the existing batch policy.
9. Special conditions and errors: explicit pending state overrides waiting policy for cacheability; absent state still takes the unsafe legacy branch. Backend read/parse/wait failures propagate status and clear added flags; scalar/batch FE failures retain invalid epochs. No additional distinct error path was verified.
10. Test coverage: C++ tests cover layouts, ordering, waiting, missing and first-write responses. Java tests cover epochs, scalar/batch cacheability, explicit/session/global waits, forwarding, scalar bitmap cache bypass, and daemon preparation. Missing cases include old images/descriptors, lagging pending metadata tuples, real proc/planner lock scopes, full daemon cycles and failed later batches. The legacy-MS test currently endorses the unsafe caching policy.
11. Test results: no regression output files changed. New expected values match the inspected implementation; that does not establish the correctness of the legacy policy. No builds or tests were run, as required by this static-only review contract.
12. Observability: existing get-version RPC metrics, query profile counters, daemon logs and failure warnings remain. No additional distinct observability defect was verified.
13. Persistence and failover: no new EditLog or KV write protocol was introduced. Persisted TTL replay and upgrade-seeded volatile caches are the lifecycle concerns retained above.
14. Writes and atomicity: rowset/delete-bitmap publication protocols are unchanged. Scalar bitmap version reads run after the existing bitmap update lock and use existing failure cleanup in ordinary and subtransaction callers. Known shared-cache publication concerns remain duplicate review context.
15. FE/BE variable passing: no new FE/BE plan or thrift variable is added. The new boolean is FE-side and forwarded between FEs; every MS partition response producer is updated.
16. Performance: the producer adds one boolean per partition without another metadata read. Existing P2 comments cover unbounded foreground expiry refresh, O(partition count) invalidation under metadata locks, and planner waiting RPCs. These were revalidated and not reposted.
17. Additional concerns: existing partial-proc-batch publication, table-token-before-wait, and mixed cached/fetched foreground snapshot threads remain duplicate fences. The older disabled-syncer TTL-bypass and dirty/staged-cache reports describe mechanisms absent from this head; P1 ids 4032914017 and 4032914020 are not retained as confirmed blockers.

No additional user focus was supplied. All changed paths and risk focuses received main-agent and independent review. The remaining limitations are static validation and the test gaps stated above; no unresolved distinct candidate or unreviewed changed scope remains. Convergence describes completion of the review search, while the five confirmed P1 findings still require changes.

Review converged after 2 round(s).

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