Feat: Introdue cache interface. by kash2104 · Pull Request #131 · kubevela/pkg

kash2104 · 2026-07-08T03:47:46Z

This PR introduces a generic Cache interface (Put/Get/Delete) package, decoupling consumers from concrete implementations and making a reusable package across KubeVela repo. This enables swapping backends (sync.Map, hashicorp/golang-lru, etc.) without touching consumer code.

The first implementation, MemoryCache, uses sync.Map with per-entry TTL and a background eviction goroutine (migrated from the kubevela/kubevela/pkg/utils/cache.go with identical behaviour).

Summary by cubic

Introduce a generic cache.Cache and in-memory MemoryCacheStore with per-entry TTL and background eviction. Uses a simple Get/Put/Delete API with found semantics and context-aware cleanup.

New Features
- Added cache.Cache[K comparable] with Get(key) (value, found), Put(key, value, expiration), and Delete(key).
- Implemented MemoryCacheStore using sync.Map, a 3s sweep interval, and context-aware shutdown.
- Added tests with github.com/onsi/ginkgo/v2 and github.com/onsi/gomega for expiration, non-expiring entries, delete, multi-key, and overwrite.

^{Written for commit d0f4239. Summary will update on new commits.}

cubic-dev-ai

All reported issues were addressed across 2 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

codecov · 2026-07-08T03:51:47Z

Codecov Report

❌ Patch coverage is 94.44444% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (6301391) to head (d0f4239).

Files with missing lines	Patch %	Lines
cache/map.go	94.44%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   88.44%   88.53%   +0.08%     
==========================================
  Files          92       93       +1     
  Lines        4805     4841      +36     
==========================================
+ Hits         4250     4286      +36     
- Misses        366      367       +1     
+ Partials      189      188       -1

Flag	Coverage Δ
unit-test	`88.53% <94.44%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

cubic-dev-ai

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="cache/map_test.go">

<violation number="1" location="cache/map_test.go:40">
P3: The 3-second `time.Sleep` at line 40 adds a hard delay to every test run. Consider reducing the TTL to e.g. 100ms and sleeping only ~150ms to test expiry, or testing the expiry behavior directly via `memoryCache.IsExpired()` without wall-clock waiting.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

cubic-dev-ai · 2026-07-11T08:15:48Z

+		store.Put("test", "test data", time.Second*2)
+		store.Put("test2", "test data", 0)
+		store.Put("test3", "test data", -1)
+		time.Sleep(3 * time.Second)


P3: The 3-second time.Sleep at line 40 adds a hard delay to every test run. Consider reducing the TTL to e.g. 100ms and sleeping only ~150ms to test expiry, or testing the expiry behavior directly via memoryCache.IsExpired() without wall-clock waiting.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At cache/map_test.go, line 40: <comment>The 3-second `time.Sleep` at line 40 adds a hard delay to every test run. Consider reducing the TTL to e.g. 100ms and sleeping only ~150ms to test expiry, or testing the expiry behavior directly via `memoryCache.IsExpired()` without wall-clock waiting.</comment> <file context> @@ -0,0 +1,78 @@ + store.Put("test", "test data", time.Second*2) + store.Put("test2", "test data", 0) + store.Put("test3", "test data", -1) + time.Sleep(3 * time.Second) + Expect(store.Get("test")).Should(BeNil()) + Expect(store.Get("test2")).Should(Equal("test data")) </file context>

The delay is intentional in this test because the goal is to validate the end-to-end cache behavior rather than just the expiration check. If we only test IsExpired() directly, we wouldn't be exercising the complete cache workflow that this test is intended to cover, so I'd prefer to keep this.

Signed-off-by: kash2104 <kparikh1104@gmail.com>

roguepikachu · 2026-07-20T17:25:53Z

Phase 1 change request

Framing this as Phase 1: move the existing cache into pkg and point every consumer at it. LRU is Phase 2, so nothing below asks for LRU here. The Phase 1 goal is one cache, in pkg, with no duplicate left behind.

What this PR needs:

Fix the Get contract. Get currently returns (value, expired). The conventional shape is (value, found), and the current one is a real trap: a consumer that reads the second bool as "found" inverts its logic. That already happened in the workflow PR (#240), where the skip optimization is silently disabled. Please switch to (value, found), or at absolute minimum document the contract loudly on the interface method.
This is the single source of truth. cache/map.go here is kubevela/pkg/utils/cache.go lifted and made generic. Once the consumers migrate, that in-repo copy gets deleted (handled in the kubevela PR). After that, pkg/cache.Cache[string] is the only cache and utils.MemoryCacheStore is gone.

Not for this PR, just so the interface does not box us in later: Phase 2 (the LRU work) will need byte accounting, an OnEvicted callback, and a max-bytes bound. Keeping the value opaque is fine for Phase 1. Just worth knowing the interface will grow.

roguepikachu · 2026-07-20T17:34:38Z

+// Cache interface defines the methods for a cache implementation.
+type Cache[K comparable] interface {
+	// Get retrieves a value from the cache by key.
+	Get(key K) (value interface{}, expired bool)


The second return being expired (not found) is a footgun. sync.Map.Load and idiomatic Go caches return (value, found), so consumers naturally write if v, ok := Get(k); ok. With expired, that reads backwards and silently disables the caller's logic. This already bit workflow #240 (line 141). Please make it (value, found):

Suggested change

Get(key K) (value interface{}, expired bool)

Get(key K) (value interface{}, found bool)

and invert the check inside MemoryCacheStore.Get (line 101) accordingly (return data, true on a live hit, return nil, false when absent or expired).

Have updated the Get function to return value, found

roguepikachu · 2026-07-20T17:35:39Z

Phase 1 wrap-up (all three PRs)

Since this package is the foundation the other two PRs build on, capturing the Phase 1 endgame in one place so the sequencing is clear.

Phase 1 is done when there is a single cache in kubevela/pkg, consumed by both KubeVela and Workflow, and the in-repo oam-dev/kubevela/pkg/utils.MemoryCacheStore is deleted. Today it is half done: this package exists and workflow adopts it, but KubeVela still ships and uses the old cache.

Merge order and remaining work across the three PRs:

pkg Feat: Introdue cache interface. #131 (this PR): land the interface with Get returning (value, found) (inline note above). This is the single source of truth.
workflow #240: fix the inverted skip-check and add the skip-path test.
kubevela #7246: migrate the two remaining utils.MemoryCacheStore consumers (pkg/cue/cuex/providers/helm/provider.go and pkg/utils/helm/helm_helper.go) to pkg/cache.Cache[string], then delete pkg/utils/cache.go and cache_test.go.

Acceptance check for the whole of Phase 1: grep -rn MemoryCacheStore in the kubevela repo returns nothing outside vendor. After that, Phase 2 (the LRU byte-store) grows this shared package: byte accounting, an OnEvicted hook, and a max-bytes bound. None of that is needed here now; keeping the value opaque for Phase 1 is correct.

roguepikachu · 2026-07-21T17:42:50Z

@cubic-dev-ai review

cubic-dev-ai · 2026-07-21T17:42:56Z

@cubic-dev-ai review

@roguepikachu I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 3 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

roguepikachu

The implementation now correctly returns a found boolean, but the interface still declares Get(key K) (value interface{}, expired bool) in cache/cache.go. That named result documents the opposite contract and leaves the original consumer trap in place. Please rename expired to found so the interface and implementation agree.

Local validation at 56785c5: go test ./cache and go vet ./cache both pass.

roguepikachu · 2026-07-21T18:23:58Z

+// Cache interface defines the methods for a cache implementation.
+type Cache[K comparable] interface {
+	// Get retrieves a value from the cache by key.
+	Get(key K) (value interface{}, expired bool)


This named return still says expired, but the implementation now returns true for a live hit. Please rename it to found so the public contract documents the corrected semantics and does not recreate the inversion trap for consumers.

Signed-off-by: kash2104 <kparikh1104@gmail.com>

anoop2811 · 2026-07-22T23:25:49Z

Thank you for your contribution @kash2104 !

cubic-dev-ai Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread cache/cache.go Outdated

Comment thread cache/map.go

kash2104 force-pushed the issue-130 branch from d142abc to fe8079d Compare July 8, 2026 03:53

cubic-dev-ai Bot reviewed Jul 11, 2026

View reviewed changes

kash2104 added 4 commits July 12, 2026 23:07

feat: Introduce cache interface to define implementation methods.

697aaa1

Signed-off-by: kash2104 <kparikh1104@gmail.com>

feat: Cache implementation using map.

ca1746f

Signed-off-by: kash2104 <kparikh1104@gmail.com>

Test: Add tests for cache implemented using map.

778fb81

Signed-off-by: kash2104 <kparikh1104@gmail.com>

Refactor: Make cache K comparable.

ac62542

Signed-off-by: kash2104 <kparikh1104@gmail.com>

kash2104 force-pushed the issue-130 branch from fc58fae to ac62542 Compare July 12, 2026 17:38

This was referenced Jul 19, 2026

Refactor: Use generalized cache for StepStatusCache. kubevela/workflow#240

Open

Refactor: Use updated StepStatusCache kubevela/kubevela#7246

Open

roguepikachu reviewed Jul 20, 2026

View reviewed changes

kash2104 force-pushed the issue-130 branch from a0bb357 to 56785c5 Compare July 21, 2026 08:48

cubic-dev-ai Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread cache/map.go Outdated

roguepikachu suggested changes Jul 21, 2026

View reviewed changes

roguepikachu reviewed Jul 21, 2026

View reviewed changes

Perf: Switch from expired to found for Get contract.

d0f4239

Signed-off-by: kash2104 <kparikh1104@gmail.com>

kash2104 force-pushed the issue-130 branch from 56785c5 to d0f4239 Compare July 22, 2026 01:52

roguepikachu approved these changes Jul 22, 2026

View reviewed changes

anoop2811 approved these changes Jul 22, 2026

View reviewed changes

anoop2811 merged commit 4558427 into kubevela:main Jul 22, 2026
11 checks passed

	Get(key K) (value interface{}, expired bool)
	Get(key K) (value interface{}, found bool)

Uh oh!

Conversation

kash2104 commented Jul 8, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kash2104 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

roguepikachu commented Jul 20, 2026

Phase 1 change request

Uh oh!

roguepikachu Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

kash2104 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

roguepikachu commented Jul 20, 2026

Phase 1 wrap-up (all three PRs)

Uh oh!

roguepikachu commented Jul 21, 2026

Uh oh!

cubic-dev-ai Bot commented Jul 21, 2026

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

roguepikachu left a comment

Choose a reason for hiding this comment

Uh oh!

roguepikachu Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anoop2811 commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kash2104 commented Jul 8, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

codecov Bot commented Jul 8, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot Jul 11, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading