Skip to content

Refactor: Migrate optional in-memory context storage to pkg/cache - #262

Open
NachiketPatil26 wants to merge 2 commits into
kubevela:mainfrom
NachiketPatil26:refactor/in-memory-context-cache
Open

NachiketPatil26 wants to merge 2 commits into
kubevela:mainfrom
NachiketPatil26:refactor/in-memory-context-cache

Conversation

@NachiketPatil26

@NachiketPatil26 NachiketPatil26 commented Oct 1, 2026 •

Copy link
Copy Markdown

Description of your changes

Moves MemStore onto github.com/kubevela/pkg/cache, same approach as the email change in #259. Entries go in with no expiry since this is live workflow context, and the exported methods stay the same because vela-core calls them.

GetOrCreateInMemoryContext now holds the lock for the whole lookup + create, and reads use the same namespace defaulting as writes (empty namespace -> default).

Fixes #254

I have:

  • Read and followed KubeVela's contribution process.
  • Related Docs updated properly. (internal refactor, no docs change)
  • Run make reviewable to ensure this PR is ready for review. (fmt, vet, golangci-lint and staticcheck; no manifest or chart changes)
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Added pkg/context/storage_test.go: create/get/update, empty namespace, get-or-create returning a copy, concurrent access, and NewContext -> Commit -> LoadContext with EnableInMemoryContext on. go test -race ./pkg/context/... passes.

Special notes for your reviewer

Two existing bugs I left out to keep this a straight refactor:

I can send follow-ups for either if you want.


Summary by cubic

Migrates in-memory workflow context storage from a plain map to github.com/kubevela/pkg/cache without changing exported methods for vela-core callers. Entries are stored without expiry since they hold live workflow context. Fixes #254.

  • GetOrCreateInMemoryContext now holds the lock across the whole lookup and create, so concurrent calls can't race.
  • Reads now apply the same namespace defaulting as writes (empty namespace -> default).
  • Adds pkg/context/storage_test.go covering create/update/get, empty namespace handling, concurrent get-or-create asserting every caller ends up with the stored context, and NewContext/Commit/LoadContext with in-memory storage enabled.
  • Known bugs left out of scope: DeleteInMemoryContext builds its key without the namespace, and sync() still falls through to cli.Patch when in-memory context is on. Can be sent as follow-ups.

Written for commit 76bfa23. Summary will update on new commits.

Review in cubic

Back MemStore with the shared kubevela/pkg cache instead of a plain map.
Entries are stored without expiry since this holds live workflow context.

Also take the lock across GetOrCreateInMemoryContext so the lookup and
create can't race, and use the same namespace defaulting for reads and
writes.

Signed-off-by: Nachiket <nachiketpatil2604@gmail.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

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

Fix all with cubic | Re-trigger cubic

Comment thread pkg/context/storage_test.go Resolved
Comment thread pkg/context/storage_test.go Resolved
@codecov

codecov Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.48%. Comparing base (d7db9c4) to head (76bfa23).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
pkg/context/storage.go 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #262      +/-   ##
==========================================
+ Coverage   62.49%   70.48%   +7.99%     
==========================================
  Files          62       73      +11     
  Lines        4415     5852    +1437     
==========================================
+ Hits         2759     4125    +1366     
- Misses       1324     1341      +17     
- Partials      332      386      +54     
Flag Coverage Δ
e2etests 24.69% <0.00%> (?)
unit-test 64.91% <94.11%> (+2.42%) ⬆️

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.

The previous concurrency test only checked that a key existed afterwards, so it would pass even if GetOrCreateInMemoryContext were not atomic. Its DeleteInMemoryContext calls also never matched the stored keys. Race workers on a single key and assert every caller ends up with the stored context.

Signed-off-by: Nachiket <nachiketpatil2604@gmail.com>
@NachiketPatil26

Copy link
Copy Markdown
Author

Hi, CI is green and I tightened the concurrency test based on the bot review. Would someone have time to take a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: Migrate optional in-memory context storage to pkg/cache

1 participant