[frenemy17]

Ⅰ. Describe what this PR does

When spec.worker.tieredStore is updated on an existing CacheRuntime, the runtime configuration ConfigMap (fluid-runtime-config-<name>) is recomputed from spec on every sync (cm.go:102), but the worker pod volumes (emptyDir.sizeLimit, emptyDir storage medium, memory tmpfs /dev/shm) cannot be updated in-place by SyncComponentSpec without pod recreation.

This caused an undetected split between the ConfigMap and the worker pod volumes:

• The ConfigMap carries the updated quotas and medium settings.
• The worker pod volumes retain their initial creation-time sizes and media.
• When worker pods restart for any reason (eviction, rolling update, node reboot), the cache process reads the updated ConfigMap quota and attempts to allocate more space than the container tmpfs or emptyDir volume permits, resulting in crashes or truncated writes with no warning or event emitted.

This PR addresses the issue by:

1. Adding worker tieredStore drift detection (checkWorkerTieredStoreDrift) in syncRuntimeSpec (pkg/ddc/cache/engine/sync.go):
   • Compares level-by-level quotas in runtime.Spec.Worker.TieredStore.Levels against the actual sizeLimit of worker pod volumes (Quantity.Cmp).
   • Compares emptyDir storage media (Medium) to detect disk $\leftrightarrow$ Memory flips.
   • Matches ConfigMap extraction precedence by evaluating HostPath before EmptyDir.
   • Checks both processMemory and emptyDir volumes by their deterministic names (getMemoryTieredStoreVolumeName, getTieredStoreVolumeName).
   • Verifies declared hostPath paths match the pod's volume configurations.
   • Detects removed or orphaned tieredStore volumes on the worker pod.
   • Modularized into low-complexity helpers (checkProcessMemoryDrift, checkEmptyDirDrift, checkHostPathDrift).
2. Emits a RuntimeTieredStoreConflict Warning event on the CacheRuntime object when drift is detected.
   • Remediation guidance directs users: "Delete and recreate the CacheRuntime to apply changes."
   • Throttles the Warning event to a 10-minute interval (e.getTieredStoreWarningInterval()) to prevent event spam while ensuring the event remains visible within the default 1-hour Kubernetes event TTL.
   • Automatically resets the cooldown when the split is resolved.
3. Ensures worker PodSpec is read during sync even when neither CacheRuntime nor CacheRuntimeClass declares resources (closing the gap where podSpec was only read inside if workerResources != nil).
4. Adds end-to-end controller loop verification in test/gha-e2e/curvine/test.sh (verify_tieredstore_drift_warning):
   • Scopes event queries by namespace (-n default), object UID, and Warning type.
   • Asserts RuntimeTieredStoreConflict Warning event emission upon patch.
   • Triggers reconcile cycles while drift persists and asserts event count remains 1, pinning the 10-minute throttle on a live controller loop.
   • Reverts the full original level (including high and low) and asserts no further conflict events while resolved.
   • Verifies cooldown reset upon reversal by re-introducing drift and asserting immediate re-warning without waiting 10 minutes.
5. Documents that spec.worker.tieredStore cannot be updated in-place and details the drift warning behavior in both English and Chinese documentation (cacheruntime_spec_update.md and cache_runtime_tieredstore.md).

Ⅱ. Does this pull request fix one issue?

fixes #6192

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Added unit test suite in pkg/ddc/cache/engine/sync_test.go (Context("tiered store drift detection and warning")):

• should not emit a warning event when tiered store matches worker pod volumes: verifies 0 warning events when volumes match spec (both processMemory and emptyDir).
• should emit a Warning event when processMemory quota in spec drifts from worker volume sizeLimit: verifies RuntimeTieredStoreConflict event and error description for processMemory quota drift.
• should emit a Warning event when emptyDir quota in spec drifts from worker volume sizeLimit: verifies Warning event for emptyDir quota drift.
• should emit a Warning event when emptyDir medium is edited from disk to Memory: verifies Warning event for emptyDir medium drift.
• should emit a Warning event when emptyDir medium is edited from Memory to disk: verifies Warning event for emptyDir medium drift.
• should emit a Warning event when hostPath paths are edited: verifies Warning event for hostPath path changes.
• should give precedence to hostPath over emptyDir when both are configured on a level matching ConfigMap extraction: verifies dispatch order matches cm.go.
• should emit a Warning event when a new level is added to spec that worker pod lacks: verifies Warning event for newly added levels.
• should emit a Warning event when a tieredStore volume exists on pod but was removed from spec: verifies Warning event for orphaned volumes.
• should throttle warning events and re-emit only after interval expires: verifies rate-limiting within 10 minutes and re-emission after expiration.
• should reset throttling cooldown when drift is resolved: verifies cooldown reset upon resolution and immediate re-triggering on subsequent drift.
• should detect drift even when neither CacheRuntime nor template declares worker resources: verifies drift detection runs when workerResources == nil.
• should return gracefully when podSpec is nil: verifies nil safety.
• should detect missing volume for processMemory level: verifies missing volume detection.
• should detect missing volume for hostPath level: verifies missing volume detection for host paths.
• should return error when reading worker pod spec fails: verifies error propagation and logging when GetPodSpec fails.
• Added pkg/ddc/cache/engine/engine_test.go to cover Build, Precheck, and getTieredStoreWarningInterval accessor.
• Integration test in test/gha-e2e/curvine/test.sh (verify_tieredstore_drift_warning): verifies emission, live throttle, full level revert, and cooldown reset.

100% statement coverage achieved across all new functions and branches.

Ⅳ. Describe how to verify it

1. Run unit tests with inlining disabled for gomonkey:

   go test -gcflags="all=-N -l" -v ./pkg/ddc/cache/engine -run TestCacheEngine -ginkgo.focus="tiered store drift"

2. Verify code coverage:

   go test -gcflags="all=-N -l" -coverprofile=coverage.out ./pkg/ddc/cache/engine -run TestCacheEngine -ginkgo.focus="tiered store drift|CacheEngine Tests"
   go tool cover -func=coverage.out | grep -E "checkWorkerTieredStoreDrift|checkEmptyDirDrift|checkProcessMemoryDrift|checkHostPathDrift|findVolume|getTieredStoreWarningInterval|Build|Precheck"

Ⅴ. Special notes for reviews

Client-side tieredStore drift detection (spec.client.tieredStore) is tracked in #6221 as a follow-up, as the client DaemonSet currently uses an OnDelete/immutable model. When the validating webhook (#6193) is disabled, controller event-based detection will provide the detection fallback.

[fluid-e2e-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cheyang for approval by writing /assign @cheyang in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fluid-e2e-bot]

Hi @frenemy17. Thanks for your PR.

I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.94%. Comparing base (a01c038) to head (7247969).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6219      +/-   ##
==========================================
+ Coverage   65.75%   65.94%   +0.18%     
==========================================
  Files         486      486              
  Lines       34459    34554      +95     
==========================================
+ Hits        22660    22787     +127     
+ Misses      10044    10010      -34     
- Partials     1755     1757       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

Same-quota changes between disk-backed and memory-backed emptyDir storage still bypass drift detection.

1 open issue

• Detect EmptyDir medium-only configuration changes · New

What changed in this PR

This PR makes CacheRuntime worker tiered-store drift visible during reconciliation, addressing the silent configuration mismatch described in #6192.

Changes:

• Detects worker quota, hostPath, and missing or orphaned volume mismatches, including when worker resources are unspecified.
• Emits throttled RuntimeTieredStoreConflict warnings and adds drift-detection and cooldown tests.
• Documents tiered-store update limitations in English and Chinese.

File	Description
pkg/​ddc/​cache/​engine/​sync.go	Adds worker volume drift detection and warning emission.
pkg/​ddc/​cache/​engine/​sync_test.go	Tests drift scenarios, warning throttling, and cooldown reset.
pkg/​ddc/​cache/​engine/​engine.go	Adds warning interval and per-engine timestamp state.
pkg/​common/​constants.go	Defines the tiered-store conflict event reason.
docs/​zh/​userguide/​cache_runtime_tieredstore.md	Adds Chinese update-limit and warning guidance.
docs/​zh/​samples/​cacheruntime/​cacheruntime_spec_update.md	Documents tiered-store update behavior in Chinese.
docs/​en/​userguide/​cache_runtime_tieredstore.md	Adds English update-limit and warning guidance.
docs/​en/​samples/​cacheruntime/​cacheruntime_spec_update.md	Documents tiered-store update behavior in English.

_{Review effort: Balanced}

[cheyang]

Premise independently reproduced by both of us on the base branch — issue #6192's split (edited tieredStore reaches the ConfigMap while worker volumes keep creation-time sizes, with nothing reporting it) — and the fix is verified on this head: the worker drift warning fires through the full Sync path with quota, path, and missing/extra-volume coverage, no false positives after a real API-server round-trip, a 10-minute throttle with cooldown reset, detection still working when no worker resources are declared, and en+zh docs. CI is green on 58afa3a. Two things gate merge:

1. The remediation guidance doesn't work as written. The event says "Recreate the worker to apply changes." and the docs say "deleting and recreating the worker workload" — but nothing ever recreates a deleted worker AdvancedStatefulSet. Verified end-to-end: sync fails NotFound every 20 s, and the failure happens before the drift check, so this PR's own warning goes silent exactly when the user needs it. Only recreating the CacheRuntime converges. The wedge itself is pre-existing, but the text pointing users into it is new — please reword the event message and the ~6 doc sites (en+zh) to "delete and recreate the CacheRuntime".

2. The PR's only user-visible behavior has no coverage above the fake-client unit layer, and no tracked deferral. The 10-minute throttle survives across reconciles only via the controller's per-runtime engine cache, and live event emission is untested — a patch-then-assert-event case fits the existing test/gha-e2e/{mooncake,curvine} pattern exactly, or file a tracking issue with a timeline.

Non-blocking, folded below: emptyDir medium-only flips escape the check (one-line compare — Copilot flagged this too), the docs promise the warning for spec.client.tieredStore while detection is worker-only, and the package-level tieredStoreWarningInterval var is an unused test seam. The one real disagreement in the debate — whether the e2e gap blocks — settled toward blocking per the repo's test policy. Reviewer C (Copilot) agreed with both debaters on its single finding; nothing rests on Copilot alone.

Next round

[MUST] Reword the remediation guidance to "delete and recreate the CacheRuntime" — event message (pkg/ddc/cache/engine/sync.go:310) plus cacheruntime_spec_update.md and cache_runtime_tieredstore.md (en+zh); the current "recreate the worker workload" reading wedges sync and silences the warning.
[MUST] Cover the warning through a real controller loop: add a patch-tieredStore-then-assert-event e2e case in test/gha-e2e/{mooncake,curvine}, or file a tracked deferral issue with a timeline and reference it in the PR.
[SHOULD] Compare emptyDir Medium in checkWorkerTieredStoreDrift (sync.go:283) so disk<->Memory flips are detected.
[SHOULD] Scope the docs' warning claim to spec.worker.tieredStore and open a follow-up issue for client-side drift detection (also answer: does the event path remain the fallback when the #6193 webhook is disabled?).
[SHOULD] Replace the package-level tieredStoreWarningInterval var with a per-engine field or a const.
[Filed] none

Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit

• pkg/ddc/cache/engine/sync.go:283 [minor] — emptyDir medium-only edits (disk <-> Memory) are invisible to the drift check
  Consensus finding, three-way agreement (Claude F2 / TS-MEDIUM-BLINDSPOT, Codex F2, Copilot C4367295361). The emptyDir branch of checkWorkerTieredStoreDrift compares only SizeLimit, never Medium. Flipping an emptyDir level's medium between disk ("") and "Memory" is not cosmetic: the creation path charges the quota to container memory only when Medium == Memory (handleEmptyDir), and the ConfigMap reports mediumType MEM vs HDD from spec (extractTieredStoreLevels). After a medium-only flip the ConfigMap and resource accounting say tmpfs while the worker volume stays disk-backed with the same name and quota — a real silent split, the class of failure this PR exists to surface. Narrow trigger (a simultaneous quota change is still caught; the processMemory branch needs no medium check since the creation path pins StorageMediumMemory). One-line fix: else if vol.EmptyDir.Medium != level.EmptyDir.Medium { ... } in the emptyDir branch, plus regression tests for both medium-only transitions.
• docs/en/samples/cacheruntime/cacheruntime_spec_update.md:145 [minor] — Docs promise the drift warning for spec.{worker,client}.tieredStore, but detection is worker-only
  Consensus finding (Claude F1 / TS-DOC-CLIENT, Codex F3; provenance adjudicated — see verdict). Section 4.1 lists the field path as spec.{worker,client}.tieredStore, the summary-table row (":163", en+zh) says a tieredStore edit "Emits RuntimeTieredStoreConflict Warning event on drift" with no worker qualifier, and the userguide note 8 (cache_runtime_tieredstore.md:229, en+zh) says "Modifying tieredStore on an existing CacheRuntime will trigger" the event. But checkWorkerTieredStoreDrift only ever receives the worker pod spec: editing spec.client.tieredStore produces the same split (the ConfigMap's client levels are rebuilt from spec every sync while the client DaemonSet is never re-synced after creation — ShouldSetupClient only runs at Status.Client.Phase == RuntimePhaseNone) and no event fires. The drift-warning bullet at :148 is already worker-scoped, so the edit is concentrated in the field-path header, the summary-table row, and userguide note 8. The underlying client silence is pre-existing (origin of the behavior); the over-promising doc wording is introduced by this PR. Fine to ship worker-only — issue #6192's asks are worker-scoped — but scope the doc lines to spec.worker.tieredStore and track the client case as a follow-up (see TS-CLIENT-FOLLOWUP).
• pkg/ddc/cache/engine/sync.go:234 [minor] — Is client-side drift detection tracked as a follow-up, and does the event path remain the fallback when the #6193 webhook is disabled?
  Merged open question (Claude TS-CLIENT-SCOPE, Codex Q1). Issue #6192 explicitly notes the client case ("spec.client.tieredStore should behave the same way, going by the code … I have only reproduced the worker case"), and the PR implements worker-only detection while the docs read as covering {worker,client} (see TS-DOC-CLIENT-SCOPE). Which is intended? If worker-only is the scope decision, the docs need scoping and a follow-up issue for the client case; if not, extending the check to the client DaemonSet pod spec is small (the same helper pattern works, and both harness canaries flip red when it lands). Also, per #6193 (proposed validating webhook for tieredStore immutability), does the event path remain the detection fallback when webhook.enabled=false?
• pkg/ddc/cache/engine/engine.go:40 [nit] — Package-level mutable tieredStoreWarningInterval var instead of a per-engine field or const
  Consensus nit (Claude F4 / TS-VAR-SCOPE, Codex N1). tieredStoreWarningInterval is a package-level mutable var while the sibling tunable syncRetryDuration is resolved per-engine from env (getSyncRetryDuration, engine.go:91-101). It works (engines are per-runtime and same-key reconciles are serialized), but the var is not even used as the test seam it appears to be: the PR's own throttle specs backdate engine.timeOfLastTieredStoreWarning (sync_test.go:1431) without ever mutating the interval. Either use the seam in the throttle test (clearer) or drop it for a plain const; a per-engine field would also work. Non-blocking.

Verified review delta

• TS-REMEDIATION-GUIDANCE-WEDGE: open (initial-pr)
• TS-MEDIUM-BLINDSPOT: open (initial-pr)
• TS-DOC-CLIENT-SCOPE: open (initial-pr)
• TS-CLIENT-FOLLOWUP: open (initial-pr)
• TS-E2E-GAP: open (initial-pr)
• TS-VAR-SCOPE: open (initial-pr)

Code: needs-work. CI: success; attribution: baseline. Required CI checks still apply.

[cheyang]

Consensus finding (Codex F1, confirmed by Claude's independent cross-critique audit). The new event message ("Recreate the worker to apply changes.", pkg/ddc/cache/engine/sync.go:310) and the new docs ("deleting and recreating the worker workload (or the CacheRuntime)") point users at a remedy that wedges the runtime. Verified: SetupWorkerComponent only runs from Setup and is gated on Status.Worker.Phase == RuntimePhaseNone, and both setup gates stay closed after worker deletion — the dataset's DatasetReady condition is never cleared (syncRuntimeSpec errors before status can update), and ConstructComponentStatusAndAffinity errors on NotFound instead of resetting the phase — so a deleted worker AdvancedStatefulSet is never recreated. syncRuntimeSpec then fails at GetPodSpec (NotFound) on every 20s reconcile ("Failed to sync the ddc" events), and because the error precedes checkWorkerTieredStoreDrift, the RuntimeTieredStoreConflict warning this PR adds goes silent exactly when the user needs it. Deleting individual worker pods also doesn't help (the ASTS controller recreates them from the unchanged template). Only recreating the CacheRuntime converges. The underlying non-recreation behavior is pre-existing; the guidance text pointing users into it is introduced by this PR. Fix: reword the event message and the ~6 introduced doc sites (cacheruntime_spec_update.md en:145/152/163 + zh, cache_runtime_tieredstore.md note 8 en+zh) to "delete and recreate the CacheRuntime", or teach the controller to recreate a missing workload.

[cheyang]

Consensus finding (Claude F3 rated major; Codex M1 initially minor and withdrawn in cross-critique — "my earlier non-gating label was the deviation, and I withdraw it" — citing the governing policy that controller behavior needs e2e coverage and deferral is acceptable only with a tracked issue and a timeline; adjudicated major, see verdict). The PR's deliverable — a controller-emitted, throttled Warning event — is validated only at the fake-client unit layer: the 12 new specs drive syncRuntimeSpec directly (Claude's contract test adds the full engine.Sync path; Codex's envtest drives the engine against a real API server but not the real Reconcile loop). Two properties are exercised by nothing in the PR or the repo: (1) the 10-minute throttle surviving across reconciles, which rests entirely on CacheRuntimeReconciler.GetOrCreateEngine caching engines per runtime — if that caching ever changes, the warning degrades to an event on every sync, the exact event flood issue #6192 warned would crowd out other warnings; (2) live event emission and visibility on a real CacheRuntime. Both reviewers marked these properties "not verified (reasoning only)". The gap is cheap to close: a patch of tieredStore after Bound plus a kubectl get events --field-selector reason=RuntimeTieredStoreConflict assertion fits the existing pattern exactly (test/gha-e2e/curvine/test.sh:299 already patches a CacheRuntime; test/gha-e2e/mooncake/test.sh:268-269 already asserts an event that way). Fix: add the e2e case, or file a tracking issue with a timeline and reference it in the PR.

[frenemy17]

Thank you for the detailed review! All items from the review have been addressed in the latest commit:

1. [MUST] Reword remediation guidance to "delete and recreate the CacheRuntime":

   • Updated event message in pkg/ddc/cache/engine/sync.go to "Worker tieredStore cannot be updated in-place after creation: %s. Delete and recreate the CacheRuntime to apply changes."
   • Updated all documentation references across docs/en/samples/cacheruntime/cacheruntime_spec_update.md, docs/zh/samples/cacheruntime/cacheruntime_spec_update.md, docs/en/userguide/cache_runtime_tieredstore.md, and docs/zh/userguide/cache_runtime_tieredstore.md.

2. [MUST] Controller loop verification in e2e:

   • Added verify_tieredstore_drift_warning() to test/gha-e2e/curvine/test.sh. The test patches cacheruntime curvine-demo emptyDir quota, waits for and asserts emission of the RuntimeTieredStoreConflict Warning event from the controller loop, and then reverts the quota.

3. [SHOULD] Compare emptyDir Medium in drift check:

   • Added vol.EmptyDir.Medium != level.EmptyDir.Medium check in checkEmptyDirDrift to detect disk $\leftrightarrow$ Memory flips.
   • Added regression unit tests for both disk $\to$ Memory and Memory $\to$ disk transitions.

4. [SHOULD] Scope documentation to spec.worker.tieredStore & follow-up tracking:

   • Scoped Section 4.1 header, summary tables, and note 8 across English and Chinese docs specifically to spec.worker.tieredStore.
   • Opened follow-up issue [FEATURE] CacheRuntime: detect client-side tieredStore drift on spec.client.tieredStore update #6221 to track client DaemonSet drift detection and controller event emission as a fallback when validating webhooks are disabled.

5. [SHOULD] Replace package-level tieredStoreWarningInterval var:

   • Converted defaultTieredStoreWarningInterval = 10 * time.Minute to a const.
   • Replaced package-level var with a per-engine field tieredStoreWarningInterval time.Duration on CacheEngine, initialized in Build() with a getTieredStoreWarningInterval() accessor.

6. SonarCloud Complexity & Codecov Patch Coverage:

   • Refactored checkWorkerTieredStoreDrift into low-complexity helpers (checkProcessMemoryDrift, checkEmptyDirDrift, checkHostPathDrift), reducing Cognitive Complexity from 42 to $\le 5$ (0 SonarCloud issues).
   • Added unit test covering GetPodSpec failure path, achieving 100.0% patch statement coverage.

[Copilot]

🟡 Changes recommended

Unresolved gaps affect drift detection, persistent warning visibility, and the reliability of the end-to-end assertion.

3 open issues

• Volume precedence differs from ConfigMap extraction · New
• Reconciliation stops while persistent drift remains · New
• Warning assertion matches stale CacheRuntime objects · New

1 resolved since last review

• Detect EmptyDir medium-only configuration changes

_{Review effort: Balanced}

[cheyang]

Round 2 on b56db670 (the author force-pushed one amended commit addressing every round-1 finding): both blockers from the last review are fixed, and each fix is verified rather than assumed.

1. Remediation guidance — the event message and all six doc sites (en+zh) now say "Delete and recreate the CacheRuntime", the only remedy our round-1 verification showed actually converges. Verified three ways: both harnesses' contract tests on head, a grep showing no leftover worker-recreation wording anywhere, and the CI kind-e2e log where the live event carries the new text. The wording is now pinned by contracts on both sides, so a regression fails the harnesses.

2. Real e2e coverage — verify_tieredstore_drift_warning patches the live curvine-demo CacheRuntime and asserts the event with a 120 s deadline and panic-on-timeout; it runs inside the required kind-e2e checks and is green on all five k8s versions (v1.22.17–v1.33.2), with the v1.22.17 log confirming the event was actually found — not a vacuous pass.

Also fixed and verified: emptyDir medium-only flips are now detected (both round-1 canaries flipped red and were inverted into bite-checked contract tests), the docs are worker-scoped everywhere with client-side detection tracked in #6221 (the PR body also answers the webhook-off fallback question), and the package-level interval var became a per-engine field with a tested fallback. Required CI is green on b56db670 (only tide pending, which is approved/lgtm label gating, not a test).

What's left is non-blocking and folded below: the e2e's revert leg asserts nothing while the comment and the PR body claim it verifies the cooldown reset (and the revert patch drops the level's high/low), nothing on a live cluster yet distinguishes a throttled warning from per-reconcile spam, and the events query could take -n default. The only disagreement in the debate was a severity label (minor vs nit on the revert leg), settled at minor because the docs affirmatively claim a verification the code doesn't perform. Reviewer C (Copilot) flagged three advisory items: the requeue-dependent re-emission mechanism is real (it reinforces the throttle assertion below), the event-selector scoping is a hardening point that can't trigger in the current CI layout, and the drift-dispatch precedence mismatch is plausible by code read but harness-unverified — none of them gate.

Next round

[MUST] none — merge-ready from the review side, once maintainers accept the #6221 client-detection deferral.
[SHOULD] In verify_tieredstore_drift_warning, assert the revert leg (no further conflict events, or a fresh drift re-warns immediately) and revert the full original level including high/low; drop the "resets upon reversal" claim from the PR body in the meantime.
[SHOULD] While drift persists, assert the RuntimeTieredStoreConflict event's count stays 1 across a couple of reconcile cycles, pinning the throttle on a real controller loop.
[SHOULD] Add -n default (and optionally a UID + Warning-type filter) to the events query at test/gha-e2e/curvine/test.sh:394.
[Filed] none — the client-side follow-up is already tracked in #6221 (author-filed).

Non-blocking observations (5) — not part of this round's ask; fix now, follow up, or file an issue as you see fit

• test/gha-e2e/curvine/test.sh:406 [minor] — e2e revert leg asserts nothing while the comment and PR body claim it verifies the cooldown reset; the revert patch also drops the level's high/low
  New this round, consensus (Claude F1 minor + Codex F7 TS-E2E-REVERT-HYGIENE nit, merged; severity adjudicated minor). The revert leg comments "Revert quota back to 1Gi to verify drift resolution and cooldown reset" but the code only patches back and sleeps 5 (test.sh:407-408) — no assertion that drift resolved, that no further event fires, or that the cooldown reset; the PR body's item 4 claims the e2e verifies the warning "resets upon reversal", an affirmatively stated verification the code never performs. Additionally the revert patch (kubectl patch --type=merge) JSON-merge-replaces the levels array, dropping the original level's high "0.8" / low "0.5" (cacheruntime.yaml:36-41), leaving the CR different from how the test found it. Harmless today — the drift check compares only Medium/SizeLimit/count, and delete_dataset_and_runtime runs immediately after (test.sh:448) with each CI run re-creating the CR — and the cooldown-reset property itself is unit-covered (PR spec + harness TS4). Fix: revert the full original level and/or assert no new RuntimeTieredStoreConflict events after the revert (or a fresh drift re-warns immediately), or drop the "resets" claim from the PR description.
• test/gha-e2e/curvine/test.sh:394 [minor] — e2e proves event emission but not bounded re-emission (throttle) on a real controller loop
  New this round, consensus (Claude F2; matches the residual Codex recorded non-blocking in its F4). The e2e breaks out of the wait loop on the first event and immediately reverts, so nothing on a live cluster distinguishes a correctly throttled warning from per-reconcile event spam — the exact flood issue #6192 warned would crowd out other warnings. The proposed assertion is mechanically sound: client-go's event correlator aggregates repeats into one event object with count++, so asserting .items[0].count stays 1 across a couple of ~20 s reconcile cycles pins the throttle end-to-end, and a broken engine cache (fresh engine per reconcile → zeroed timeOfLastTieredStoreWarning) would surface as count growth. Engine-level throttle is pinned by harness contract TS4 (3 consecutive Syncs → exactly 1 event, re-emission after interval, reset on resolution, bite-checked); what remains untested is the pre-existing GetOrCreateEngine cache handoff shared with timeOfLastSync. Copilot C4368713456 adds a real adjacent mechanism (see TS-REQUEUE-REEMISSION) that strengthens the case for landing this.
• test/gha-e2e/curvine/test.sh:394 [nit] — events query without -n default in the new e2e; selector unscoped by involved object
  New this round, consensus nit (Claude F3; overlaps Copilot C4368713479 from a different angle). 'kubectl get events --field-selector reason=...' is the only events query in the script without a namespace; the diagnostics a few lines up use -n default (test.sh:252). Functionally fine — every kubectl create in the script likewise relies on the context namespace — but the flag keeps the assertion correct if the runner's default context ever changes. Copilot additionally suggests filtering by the runtime's UID and Warning type; both cross-critiques confirmed the false-pass vector cannot trigger in this suite today (exactly one CacheRuntime, curvine-demo, per fresh kind cluster), so this is robustness hardening of the same line, not a live defect.
• pkg/ddc/cache/engine/sync.go:284 [minor] — Drift-check dispatch order (EmptyDir before HostPath) differs from the ConfigMap extraction precedence (HostPath before EmptyDir) — flagged by Reviewer C, unconfirmed by the debaters
  Copilot-only finding C4368713436, recorded per advisory policy — never a consensus, never gating. Claim: the drift switch dispatches ProcessMemory → EmptyDir → HostPath (sync.go:266-282) while extractTieredStoreLevels checks HostPath before EmptyDir (cm.go:225 vs :239), and the CRD has no one-of validation (api/v1alpha1/cacheruntime_types.go:275-299, all three mediums merely +optional), so a level edited to carry both emptyDir and hostPath would be reported as hostPath in the ConfigMap while the drift check validates the stale emptyDir volume — a spurious-warning failure mode, not silence. Both debaters independently code-read both precedence orders and call the mechanism plausible, but it is harness-unverified, requires a malformed-but-accepted CR, and the failure mode is a wrong warning rather than a missed one. Candidate for a follow-up hardening: align the dispatch with the ConfigMap precedence (or add one-of validation) with a regression test for the both-fields-set edit.
• pkg/ddc/cache/engine/sync.go:301 [minor] — With FLUID_RUNTIME_RECONCILE_DURATION=-1 the throttled warning can expire under persistent drift — Reviewer C flag, mechanism confirmed by both debaters, consequence unverified
  Copilot finding C4368713456, partially confirmed by the debaters: the interval check does not itself schedule another reconciliation, and after a successful sync the controller returns via GenerateRandomRequeueDurationFromEnv (runtime_controller.go:305) — with FLUID_RUNTIME_RECONCILE_DURATION=-1 that is NoRequeue, so once watched objects stop changing, no reconcile fires to re-emit the warning and the 1 h event TTL can expire despite persistent drift. Claude's cross-critique confirmed the mechanism (default is a 90 s requeue, crtl_utils.go:38) and Codex called it 'real mechanism, supports the F4 residual' — but the PR does not touch requeue behavior (the mechanism is pre-existing infra), the non-default env must be set explicitly, and no harness run demonstrates the expiry. Advisory only; if it matters, the remedy is to schedule a follow-up reconcile while drift persists or record a persistent status condition. Reinforces TS-E2E-THROTTLE-ASSERT as the worth-landing assertion.

Verified review delta

• TS-REMEDIATION-GUIDANCE-WEDGE: fixed (initial-pr)
• TS-MEDIUM-BLINDSPOT: fixed (initial-pr)
• TS-DOC-CLIENT-SCOPE: fixed (initial-pr)
• TS-E2E-REVERT-ASSERT: open (initial-pr)
• TS-E2E-THROTTLE-ASSERT: open (initial-pr)
• TS-E2E-NS-FLAG: open (initial-pr)
• TS-CLIENT-FOLLOWUP: fixed (initial-pr)
• TS-DISPATCH-PRECEDENCE: open (initial-pr)
• TS-REQUEUE-REEMISSION: open (initial-pr)
• TS-E2E-GAP: fixed (initial-pr)
• TS-VAR-SCOPE: fixed (initial-pr)

Code: clear. CI: success; attribution: baseline. Required CI checks still apply.

[frenemy17]

Thanks for the follow-up review! All remaining review suggestions have been implemented in the latest rebased commit:

1. e2e Revert Leg & Full Level Restore (test/gha-e2e/curvine/test.sh):

   • Updated the revert patch to restore the complete original level including high: "0.8" and low: "0.5".
   • Added assertions after revert triggering a reconcile to verify no further conflict events are emitted while resolved.
   • Added verification of cooldown reset upon reversal by re-introducing drift and asserting immediate re-warning without waiting 10 minutes.

2. e2e Throttle Assertion on Controller Loop:

   • While drift persists, triggered reconcile cycles via annotations and asserted that event count remains 1, pinning the 10-minute throttle on a live controller loop.

3. Event Query Selector Scoping:

   • Scoped the event query with -n default and filtered by involvedObject.uid, type=Warning, and reason=RuntimeTieredStoreConflict.

4. Dispatch Order Alignment (pkg/ddc/cache/engine/sync.go):

   • Reordered level type checking in checkWorkerTieredStoreDrift so HostPath precedes EmptyDir, matching the ConfigMap extraction precedence in cm.go:225.
   • Added regression unit test in sync_test.go verifying that configuring both fields evaluates as HostPath.

[Copilot]

🟡 Changes recommended

The unsupported JSONPath expression len(.items) fails silently, leaving items_len empty and preventing detection of fresh warnings stored as separate Event objects with count: 1.

2 open issues

• Use supported JSONPath item counting to detect fresh warnings · New
• Reconciliation stops while persistent drift remains

2 resolved since last review

• Warning assertion matches stale CacheRuntime objects
• Volume precedence differs from ConfigMap extraction

_{Review effort: Balanced}

[cheyang]

Round 3 on 6809f62f — the author rebased onto the moved base (fe074af, which brought #6186/#6214/#6215/#6217 into the same syncRuntimeSpec region) and amended the commit to address every open round-2 item. All of them are closed, and each closure is verified rather than assumed:

1. Dispatch precedence (the round-2 Copilot question): checkWorkerTieredStoreDrift now evaluates HostPath before EmptyDir, matching extractTieredStoreLevels — so a level carrying both fields (the CRD accepts it; the creation path doesn't) is validated against the medium the ConfigMap actually reports. Both harnesses added a contract, and both were bite-checked: restoring the old order makes the warning vanish on exactly that input.

2. The e2e now proves what it claims. verify_tieredstore_drift_warning gained three assertion phases, all visible in the CI kind-e2e log on all five k8s versions: under persistent drift, annotation-forced reconciles keep the event count at 1 (the live throttle, including the engine-cache handoff unit tests can't reach); the revert restores the full original level including high/low; and a re-patch re-warns within 60 s, proving the cooldown reset. Every events query is now namespace-, UID-, and Warning-type-scoped.

3. The rebase is clean. All five harness contracts plus the P0 premise test pass on the merged head, and the full pkg/ddc/cache/engine package (the PR's 51 sync specs coexisting with #6186's) is green. Remediation wording, medium-flip detection, and doc scoping all survive at the new line numbers; required CI is fully green on the head (only tide pending, which is label gating, not a test).

What's left is small and folded below: a new runs-on: ubuntu-24.04 pin on the kind-e2e workflow that no round asked for (benign and CI-green, but unexplained and it splits the runner fleet three ways — worth one sentence in the description), the revert leg's middle assertion ("count stays 1") is vacuous given the throttle (the reset leg is what actually proves resolution — a comment tweak), and a dead len(.items) JSONPath fallback Copilot spotted in the cooldown-reset poll (kubectl's jsonpath has no len; the count branch is what carried the CI proof — replace it with {{len .items}} or drop it). The only disagreement in the debate was a provenance label on the requeue advisory (PR-introduced vs pre-existing infra), settled as PR-introduced-but-advisory: under the non-default FLUID_RUNTIME_RECONCILE_DURATION=-1, the throttled warning can expire while drift persists — the new e2e works around exactly that mechanism by forcing reconciles, which is fine for the test but leaves the production property as a maintainer decision. Reviewer C's advisory position: one new flag (the dead jsonpath branch, mechanism since verified by both of us); its round-2 precedence and selector items are fixed, the requeue item remains open advisory.

Next round

[MUST] none — merge-ready from the review side, once maintainers accept the #6221 client-detection deferral.
[SHOULD] Add a one-line rationale for the kind-e2e runs-on: ubuntu-24.04 pin (or move it to a separate workflow PR) so the runner fleet's split is deliberate.
[SHOULD] Replace the dead len(.items) JSONPath at test/gha-e2e/curvine/test.sh:458 with a Go template {{len .items}} (or drop the branch), and reword the revert leg's log line so the vacuous middle assertion isn't read as the resolution check.
[SHOULD] If warning persistence under FLUID_RUNTIME_RECONCILE_DURATION=-1 matters, requeue while drift persists or record a status condition — the new e2e already demonstrates reconciles are what drive re-emission.
[Filed] none — client-side detection remains tracked in #6221 (author-filed).

Non-blocking observations (3) — not part of this round's ask; fix now, follow up, or file an issue as you see fit

• .github/workflows/kind-e2e.yml:25 [nit] — kind-e2e.yml runs-on pinned ubuntu-latest -> ubuntu-24.04, unexplained CI-infra scope creep in a functional PR — new this round (consensus, independently co-found)
  New in this round's diff and not asked for by any review round; independently co-found by both reviewers (Claude F12, Codex F2) and mutually confirmed: the required kind-e2e gate's runs-on changed from ubuntu-latest to ubuntu-24.04. Benign — CI is green on all five k8s versions with the pin, and kind-e2e passed on ubuntu-latest as recently as round 2 (2026-10-25), so the pin is a deliberate stability choice rather than a masked failure; neither reviewer asks for a split. But it is CI-infra scope creep unexplained in the PR body (zero mention of runner/ubuntu/24.04), and it makes kind-e2e the only workflow on 24.04 while build/CodeQL/backward-compat/pr-quota-limit/project-check ride ubuntu-latest and sonarcloud pins 22.04 — a three-way runner split. Ask: one sentence of rationale in the PR body (or a separate workflow PR). Cannot gate.
• test/gha-e2e/curvine/test.sh:437 [nit] — Post-revert 'count stays 1' assertion is vacuous given the throttle; the reset leg carries the real proof — new this round (consensus, confirmed in cross-critique)
  New this round (Codex F1, independently confirmed by Claude's cross-critique): the revert leg asserts items[0].count == 1 with the log line "no new conflict events emitted after drift resolved", but the 10-minute throttle alone guarantees count stays 1 in that window — an emission requires timeOfLastTieredStoreWarning to be zero or older than the interval (sync.go:301-311), and the first emission happened ~15-30 s earlier — so the assertion passes whether or not resolution was observed, and even a completely failed revert patch would not grow the count. It is also redundant with the preceding throttle leg. The real proof of resolution is the subsequent cooldown-reset leg (re-patch -> fresh emission within 60 s), which has teeth: it requires a reconcile to have observed the resolved state and reset the timer. Cosmetic only — suggest rewording the comment/log line ("throttle guarantees count stays 1; resolution is proven by the reset leg below") so future maintainers do not read the middle assertion as the resolution check. Same nit covers the ~10 s revert->re-patch window assuming a completed reconcile: comfortable on kind (both the patch and the annotate trigger reconciles via the watch predicate; CI green 5/5), but under pathological reconcile latency the reset leg would fail as a flake rather than a product bug.
• test/gha-e2e/curvine/test.sh:458 [nit] — Dead len(.items) JSONPath fallback in the cooldown-reset poll — flagged by Reviewer C (C4420411622), mechanism verified by both debaters, non-gating
  Reviewer C (Copilot) finding C4420411622, new this round; the mechanism was then verified by both debaters — Claude empirically (kubectl create configmap ... -o jsonpath='{len(.items)}' -> 'unrecognized identifier len', exit 1) and against client-go's jsonpath source (evalIdentifier only recognizes range/end), Codex against the repo's pinned client-go v0.29.15 and cli-runtime's JSONPathPrinter error path. In the test the error is swallowed by 2>/dev/null || true, so items_len is always empty -> ${items_len:-0} is always 0 -> the items_len >= 2 disjunct at test.sh:459 is dead code. Does not reopen TS-E2E-REVERT-ASSERT or TS-E2E-GAP: in the observed CI runs the count >= 2 branch necessarily carried the proof (the re-drift message is byte-identical to the first emission from the same 1Gi->2Gi patch, so client-go's event logger merges it into the existing event object via PATCH with Count = last + 1, inside the correlator's 600 s window). But the belt-and-suspenders design has only one working suspender — had the message ever differed or the correlator cache been evicted, the leg would false-fail with both branches inert on one side. Fix: replace with a Go template {{len .items}} or jq, or delete the dead branch. Per advisory policy this stays Reviewer-C-origin and never gates.

Verified review delta

• TS-E2E-REVERT-ASSERT: fixed (initial-pr)
• TS-E2E-THROTTLE-ASSERT: fixed (initial-pr)
• TS-E2E-NS-FLAG: fixed (initial-pr)
• TS-DISPATCH-PRECEDENCE: fixed (initial-pr)
• TS-E2E-RUNNER-PIN: open (initial-pr)
• TS-E2E-REVERT-VACUOUS-LEG: open (initial-pr)
• TS-E2E-LEN-JSONPATH: open (initial-pr)

Code: clear. CI: success; attribution: baseline. Required CI checks still apply.

[cheyang]

Hi @frenemy17, this branch now conflicts with master (GitHub reports DIRTY). The collision is in .github/workflows/kind-e2e.yml: master merged #6225, which pins the kind-e2e runner to ubuntu-24.04 in the same job block this PR modifies — that pin also fixes the repo-wide JindoRuntime warmup flake, so it's worth keeping on your side when resolving.

Could you update the branch (git fetch origin && git rebase origin/master, or merge master into fix/cacheruntime-tieredstore-drift-warning) and resolve the workflow conflict? The review side is already clear otherwise — once CI re-runs green on the updated head, we're good to merge. Thanks!

[frenemy17]

Hi @cheyang, thanks for the heads-up!

I've rebased onto the latest master and resolved the conflict by keeping master's runner pin from #6225 in .github/workflows/kind-e2e.yml. The PR diff is now clean and strictly contains the tieredStore drift detection logic, tests, and documentation updates.

CI is currently running on the new head. Really appreciate your guidance and reviews across this PR!!

[Copilot]

🟡 Changes recommended

Persistent drift warnings are not rescheduled when periodic reconciliation is disabled, and the end-to-end checks can mask query failures or miss additional events due to unsupported JSONPath syntax.

3 open issues

• Require successful event-count queries before verification · New
• Use supported JSONPath item counting to detect fresh warnings
• Reconciliation stops while persistent drift remains

_{Review effort: Balanced}

[cheyang]

Round 4 on 133accac is the quietest round so far, in the good way. The author rebased onto the moved base (96a3765 — exactly one commit: #6225, the kind-e2e ubuntu-24.04 runner pin this PR used to carry in-line) and dropped the PR's own workflow hunk. The pin now lives in its own maintainer workflow PR with the rationale round 3 asked for (ubuntu-latest resolves to the ubuntu-26.04 image, whose AppArmor fusermount3 profile breaks the JindoRuntime FUSE mounts; un-pin tracked in #6224) — so the round-3 runner-pin nit is closed, and this PR no longer touches CI at all. git range-diff confirms that is the only content change: every runtime, test, and doc byte is identical to the round-3 head.

Nothing was assumed: both harnesses re-ran on the new head (Claude's five contracts plus the client-silence canary and the P0 premise test; Codex's full re-verify, 8/8 fixed including a genuinely executed envtest), the PR's own 16 drift specs and the full engine package (341 passed / 0 failed; the 16 skips are pre-existing env-dependent specs from #6186's base files) are green, and the required CI gate is fully green on 133accac with all four phases of verify_tieredstore_drift_warning — emission, throttle count-stays-1, revert, cooldown reset — visible in the kind-e2e v1.22.17 job log, still carrying the "Delete and recreate the CacheRuntime" remediation text.

One new item, and it's small: Copilot flagged that the throttle and revert legs read the event count with 2>/dev/null || true and assert ${count:-1} == "1" (test.sh:424, :440) — a failed kubectl query defaults to the passing value, so a query flake would be logged as "Throttling verified"/"Revert verified". We both verified the mechanism (the emission and cooldown-reset legs fail safe; only these two legs mask). One remedy caveat from the debate: dropping || true alone won't do anything — the script has no set -e, so the empty string still defaults to 1; make the empty default a failing value (or assert non-empty) instead. No disagreement this round — the round-3 provenance question on the requeue advisory stays settled (PR-introduced-but-advisory) — and Copilot's other two items are the already-tracked dead len(.items) jsonpath and the requeue-under--1 advisory, both still open and non-gating.

Next round

[MUST] none — merge-ready from the review side, once maintainers accept the #6221 client-detection deferral.
[SHOULD] In test/gha-e2e/curvine/test.sh, make the two count == 1 assertions fail safe on an empty query result (e.g. ${count:-2} or an explicit non-empty check with panic) — dropping || true alone is inert without set -e — and replace the dead len(.items) jsonpath at :459 with a Go template {{len .items}} (or drop the branch); reword the revert leg's log line so the vacuous middle assertion isn't read as the resolution check.
[SHOULD] Maintainer decision on the carried advisory: under FLUID_RUNTIME_RECONCILE_DURATION=-1, requeue while drift persists or record a status condition so the throttled warning cannot expire with the 1 h event TTL — the e2e already demonstrates reconciles are what drive re-emission.
[Filed] none — client-side detection remains tracked in #6221 (author-filed); the runner-pin rationale now lives in #6225/#6224.

Non-blocking observations (1) — not part of this round's ask; fix now, follow up, or file an issue as you see fit

• test/gha-e2e/curvine/test.sh:424 [nit] — Throttle/revert event-count queries mask kubectl failures as success (|| true + ${count:-1} defaults to the passing value) — NEW this round (Reviewer C origin r4448145736), mechanism verified by both debaters, non-gating
  New this round: flagged by Reviewer C (Copilot, discussion r4448145736), carried by Codex as F1, and independently verified by both debaters — Codex with a shell-level mechanism demo and Claude by re-checking the assertion polarity at head, so the mechanism is settled even though the origin is Reviewer C. The throttle leg (test.sh:421-424) and revert leg (test.sh:437-440) fetch the event count as $(kubectl get events … -o jsonpath='{.items[0].count}' 2&gt;/dev/null || true) and assert "${count:-1}" == "1". A failed (or empty-result) kubectl query yields an empty value that defaults to the PASSING value, so a query flake is logged as 'Throttling verified' / 'Revert verified' — the assertion passes vacuously. The fail-safe contrast: the emission leg (:400-411) requires non-empty output under a 120 s deadline with panic, and the cooldown-reset leg (:454-459) uses ${fresh_count:-0} so an empty result keeps polling and panics at 60 s — only the two count==1 legs default to the passing value. Masked mode requires an independent kubectl/apiserver failure after the emission leg already proved the event exists under the same UID-scoped selector ~30-60 s earlier, inside the 1 h event TTL — a query flake, not a plausible steady-state gap; test hygiene debt of the same class as TS-E2E-LEN-JSONPATH, not a product risk. Remedy caveat from Claude's cross-critique: dropping || true ALONE is inert — test.sh runs #!/bin/bash with no set -e, so the assignment still captures an empty string and the :-1 default still masks it; the actionable fix is to make the empty default a failing value (e.g. ${count:-2} != 1 -> panic) or assert non-empty ([[ -n "$count" ]] || panic). The :-1 defaults have existed since the assertion was added in round 3 (code unchanged on the round-4 head), so this is a late discovery on unchanged code — an initial-PR miss, not a revision regression, per round-policy rule 1; firstSeenSha records the head at which the finding was first observed. Nit, non-gating.

Verified review delta

• TS-E2E-RUNNER-PIN: fixed (initial-pr)
• TS-E2E-COUNT-QUERY-MASK: open (initial-pr)

Code: clear. CI: success; attribution: baseline. Required CI checks still apply.

[frenemy17]

Hi @cheyang,

All three points from the review have been addressed in the latest commit:

1. Made both the throttle and revert event count assertions fail-safe against empty/failed kubectl queries ([[ -z "$count" || "$count" != "1" ]]).
2. Updated the revert leg log to reflect event count stability (count remained 1).
3. Replaced jsonpath='{len(.items)}' with -o go-template='{{len .items}}'.

I've also rebased onto the latest master to incorporate #6223 without conflicts. All unit tests pass locally and the branch is clean. Thanks!

[Copilot]

🔵 Needs a closer look

Persistent drift does not schedule a bounded requeue, so when normal polling is disabled or slower than the warning interval, the initial warning can expire without recurring and leave the drift invisible.

1 open issue

• Reconciliation stops while persistent drift remains

2 resolved since last review

• Require successful event-count queries before verification
• Use supported JSONPath item counting to detect fresh warnings

_{Review effort: Balanced}

[cheyang]

Round 5 on a8f4e558 closes out the review's own remediation list. The author rebased onto the moved base (7f27b7e — one commit, #6223's HPA scale subresource, which touches the same package's status/worker/component-manager files but nothing on the drift-check path: sync.go, cm.go, and GetPodSpec are untouched) and applied exactly the three round-4 e2e-hygiene SHOULDs, nothing else: every runtime, doc, and unit-test file is byte-identical to round 4; the only changed file is test/gha-e2e/curvine/test.sh, and the only changed function is verify_tieredstore_drift_warning.

Each fix was verified independently, not taken on the author's word: the two count == 1 assertions now read [[ -z "$count" || "$count" != "1" ]], so a failed/empty kubectl query panics instead of defaulting to the passing value (shell-level demo on both sides: empty → panic, 1 → pass, 2 → panic — and the fix correctly avoids the inert "drop || true" remedy, which does nothing without set -e); the dead jsonpath='{len(.items)}' fallback is now -o go-template='{{len .items}}', verified empirically on kubectl v1.37.0 by both of us (go-template renders len; jsonpath errors "unrecognized identifier len") — both suspenders of the reset assertion are live for the first time; the revert leg's log now says "Revert stability verified: event count remained 1" instead of overclaiming resolution, with the real resolution proof still carried by the cooldown-reset leg one step later. Both harnesses re-ran everything on the new head (Claude: TS1/TS3/TS4/TS5 + the P0 premise test green, TS2 client-silence canary green as the expected #6221 record; Codex: 8/8 FIXED including a genuinely executed envtest, plus a premise re-run against the new base 7f27b7e confirming the silent split still reproduces there — the attribution control), the full engine package is green (343 passed / 0 failed / 16 pre-existing env skips; the +2 vs round 4 are #6223's own new specs), and the required CI gate is fully green on a8f4e558 with all four drift phases log-confirmed in the kind-e2e v1.22.17 job, now carrying the new wording and the fail-safe assertions.

No new findings, and no disagreement this round — the two reviews converged completely (4/15 confirmed, 0 rebutted in both cross-critiques); the round-3 provenance adjudication on the requeue advisory is carried settled. Reviewer C (Copilot) concurs: its fresh review on this head lists both nit origins as resolved and carries exactly one open item — the same requeue advisory we both carry. What remains is unchanged and advisory: the FLUID_RUNTIME_RECONCILE_DURATION=-1 requeue decision (maintainer call; the consequence is still unverified by execution, as in every round — stated honestly) and the #6221 client-side-detection deferral (still OPEN, author-filed).

Next round

[MUST] none — merge-ready from the review side, once maintainers accept the #6221 client-detection deferral and make the -1 requeue call.
[SHOULD] none on the code — all three round-4 e2e-hygiene items are applied and verified. Optional cosmetic residue: the revert leg's inline comment (test.sh:522, "assert no further conflict events while resolved") still reads resolution-flavored; one comment line would finish the job the log line already did.
[SHOULD] Maintainer decision on the carried advisory: under FLUID_RUNTIME_RECONCILE_DURATION=-1, requeue while drift persists or record a status condition so the throttled warning cannot expire with the 1 h event TTL — the e2e already demonstrates reconciles are what drive re-emission.
[Filed] none — client-side detection remains tracked in #6221 (author-filed); the runner-pin rationale lives in #6225/#6224.

Verified review delta

• TS-E2E-REVERT-VACUOUS-LEG: fixed (initial-pr)
• TS-E2E-LEN-JSONPATH: fixed (initial-pr)
• TS-E2E-COUNT-QUERY-MASK: fixed (initial-pr)

Code: clear. CI: success; attribution: baseline. Required CI checks still apply.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
24.0% Duplication on New Code

See analysis details on SonarQube Cloud

[frenemy17]

Hi @cheyang,

All CI checks and E2E matrix tests are green on the latest commit. Whenever you have a moment, could you please dismiss the earlier "Changes requested" review and apply /lgtm and /approve so Tide can merge? Thank you