Dual writer: mode 3

[marchugon]

What is this feature?

Mode 3 will work the following way:

reads (get, list) : only use unified storage
writes (create, update, delete, delete-collection) : write to unified storage and then asynchronously, write to legacy, just as a safe measure.

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes https://github.com/grafana/search-and-storage-team/issues/53

Special notes for your reviewer:

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[friendlyreviewer-staging]

Hi there 👋

🌥️ Tech	🌤️ Feat
3 high, 10 medium, 1 low	1 ok, 1 mitigated

The Mode 3 dual-writer implementation matches the documented contract structurally: reads (Get/List) are served exclusively from unified storage, and writes (Create/Update/Delete/DeleteCollection) write to unified storage synchronously and then to legacy storage in an asynchronous, best-effort goroutine. However, the async legacy path contains several genuine correctness and reliability bugs that should be fixed before merge.

The most significant issues: (1) the async goroutines derive their timeout from the incoming HTTP request context, which is cancelled as soon as the handler returns (i.e. immediately after the unified write), so the "safe-measure" legacy write races request teardown and may be cancelled before it lands — defeating the purpose of the mode; (2) multiple write paths record the wrong metric series (storage failures reported as legacy, and the DeleteCollection async path reporting legacy as storage), plus a mislabeled kind metric label; (3) Update passes objInfo directly to Legacy.Update instead of wrapping with updateWrapper, and Create writes the pre-write object rather than the storage-returned object, allowing the legacy mirror to diverge from unified storage; (4) legacy-write errors are silently dropped with no logging or timeout-cause surfacing; and (5) Delete drops the per-request log context.

Test coverage is also incomplete: unit tests never assert the async legacy path actually ran (no AssertExpectations/synchronization), the Delete/DeleteCollection tests will panic non-deterministically in the async goroutine due to missing legacy mock expectations, and no test covers async legacy-write failure. The new Mode 3 integration subtests reuse generic assertions that read from legacy immediately, which is racy under async propagation and does not explicitly verify the Mode 3 contract. A minor maintainability note: the shared fixtures/registry used by the rewritten Mode 3 tests still live in the Mode 1 test file.

Overall: the feature is directionally correct but not ready to merge given the async reliability, metric-accuracy, and test-integrity problems.

---

These might need a close look

• 🔴 pkg/apiserver/rest/dualwriter_mode3.go (L52)
  The async legacy-write goroutines derive context.WithTimeoutCause(ctx, ...) from the incoming HTTP request context, which is cancelled shortly after the handler returns — and the handler returns immediately after the unified write. The 'async' legacy write therefore races request teardown and may be cancelled before completing, defeating the mode's purpose. Derive from a request-independent context (e.g. context.Background() with a fresh timeout).
• 🔴 pkg/apiserver/rest/dualwriter_mode3_test.go (L228)
  TestMode3_Delete (~line 228) and TestMode3_DeleteCollection (~line 282) do not register any legacy mock, so the async goroutine hits d.Legacy.Delete(...)/DeleteCollection(...) with no matching expectation, causing testify's mock.Called to panic ('unexpected method call'). Because the goroutine is unsynchronized, this is a scheduling-dependent, process-killing race. Register the legacy expectations (and assert them).
• 🔴 pkg/tests/apis/playlist/playlist_test.go (L529)
  doPlaylistTests asserts that the legacy API reflects k8s writes immediately (legacy list at line 529 and getFromBothAPIs legacy GETs), but under Mode 3 writes propagate to legacy asynchronously (background goroutine, 10s timeout). In the two executed Mode 3 subtests these assertions are racy and may fail intermittently. For Mode 3, poll/retry the legacy API until the async write lands, or parametrize/disable the legacy-read assertions for async modes.

Worth checking

• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L45)
  In Create (line 45) and Update (line 129) error paths, d.recordLegacyDuration(true, ...) is called with startStorage (the storage-call duration), reporting unified-store failures under the dual_writer_legacy_duration_seconds series. Should be recordStorageDuration to match the success paths (lines 48/132).
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L166)
  The DeleteCollection async goroutine calls recordStorageDuration(err != nil, ..., startLegacy) even though startLegacy measures the Legacy.DeleteCollection call — the only one of the four async goroutines using the wrong recorder. Should be recordLegacyDuration; also inconsistent with the Create/Delete/Update goroutines.
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L106)
  The Delete success path passes name (e.g. "foo") as the 'kind' metric label value, unlike the error path (line 103) and every other call that uses options.Kind. This creates a new cardinality series per object name and mislabels data. Use options.Kind.
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L97)
  Delete resets the request logger with klog.NewContext(ctx, d.Log), dropping the name/kind/method values attached on line 96. Every sibling method uses klog.NewContext(ctx, log). Use log, not d.Log.
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L56)
  In all four async goroutines the legacy-write error is only recorded into a histogram (recordLegacyDuration(err != nil, ...)) and never logged. Since the legacy write is framed as a 'safe measure', silent failures leave the legacy store out of sync with no operator visibility; the timeout causes passed to context.WithTimeoutCause are dead code. Mode 1/2 log legacy errors. Log the error (and timeout cause) at minimum.
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L139)
  Update passes objInfo directly to Legacy.Update, which re-fetches the old object and re-runs objInfo.UpdatedObject(old). For non-idempotent/relative UpdatedObjectInfo the legacy result can diverge from what was written to unified storage. Wrap with updateWrapper{upstream: objInfo, updated: res} (as elsewhere in the package) to force the unified result into legacy.
• 🟡 pkg/apiserver/rest/dualwriter_mode3.go (L51)
  The Create async goroutine calls d.Legacy.Create(ctx, obj, ...) with the raw input object instead of the storage-returned created object, so server-assigned fields (uid, resourceVersion, generation, finalizers, defaults) are dropped from the legacy mirror. Use created so the mirror matches unified storage.
• 🟡 pkg/apiserver/rest/dualwriter_mode3_test.go (L63)
  Write tests never call m.AssertExpectations(t) and provide no synchronization (WaitGroup/channel/assert.Eventually) for the async goroutines. As written, the async legacy-write path is effectively untested — tests would pass even if the goroutine never ran. Add AssertExpectations and wait for the goroutines so the async path is deterministically exercised.
• 🟡 pkg/apiserver/rest/dualwriter_mode3_test.go (L30)
  No test covers failure of the async legacy write. There is no case where Legacy.Create/Update/Delete returns an error and the test confirms (a) the unified operation still returns success and (b) the error is tolerated (no panic, metrics recorded, ideally logged). Add a legacy-failure case per write method.
• 🟡 pkg/tests/apis/playlist/playlist_test.go (L143)
  The two executed Mode 3 subtests only add storage/feature-toggle/mode wiring and reuse the unmodified doPlaylistTests; none of the assertions actually verify the Mode 3 contract (reads from unified only, writes propagate async to legacy). Add a targeted Mode 3 assertion — e.g. create via k8s and confirm get/list come from unified and the object eventually appears in legacy — rather than relying on the racy generic assertions.

Small things (take or leave)

• 🔵 pkg/apiserver/rest/dualwriter_mode1_test.go (L27)
  The rewritten Mode 3 tests now depend on package-scope fixtures and the metrics registry defined in this Mode 1 test file (exampleObj, failingObj, exampleList, p, etc.), making Mode 1's test file the de-facto owner of shared infrastructure for the whole rest package. Relocating these to a dedicated shared test file (e.g. storage_mocks_test.go or dualwriter_fixtures_test.go) would avoid compile-time breakage if the Mode 1 file is later renamed/pruned. Maintainability concern, not a functional bug.

---

✅ Feature-level checklist

Looks good

• ✅ Mode 3 reads (get, list) must use only unified storage.
  Get and List never touch the legacy store, structurally matching the contract. Integration coverage for this is indirect, but the implementation is correct.

Partially covered

• ⚠️ Mode 3 writes (create, update, delete, delete-collection) must write to unified storage and then asynchronously write to legacy as a safe measure.
  Writes do hit unified synchronously and then attempt an async legacy write, matching the shape of the contract. However, the async legacy write derives its timeout from the request context (cancelled on handler return), so it may be cancelled before completing; legacy-write errors are silently dropped; and Create/Update can write divergent data to legacy (pre-write object / unwrapped objInfo). These issues compromise the reliability of the 'safe measure' legacy write and should be fixed.

Review time: 3m 36s