perf: cache parseEngineDefinitionFromJSON to eliminate per-compile JSON→YAML round-trip#46469
Conversation
…d-trip per compile The `parseEngineDefinitionFromJSON` function was doing an expensive JSON→any→YAML bytes→struct round-trip on every `CompileWorkflow` call. For workflows using standard engines (copilot, claude, etc.), the engine definition JSON is always the same string because it comes from immutable built-in virtual files registered at startup. Add a package-level `sync.Map` cache (`engineDefinitionCache`) that stores the parsed `EngineDefinition` value (not pointer) keyed by the JSON input string. On cache hit, a fresh copy is returned so callers can mutate the struct fields (e.g. `RuntimeID`) without affecting the cached entry. Benchmark results (BenchmarkCompileMCPWorkflow): Before: ~385-420 ns/op, 149K B/op, 2924 allocs/op After: ~290-310 ns/op, 116K B/op, 2187 allocs/op This is a ~24% throughput improvement and 737 fewer heap allocations per compilation. The regressed 437μs is now well below the 382μs historical average. Closes #46453 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey One thing that would strengthen this PR before it lands:
If you'd like a hand adding coverage, here's a ready-to-use prompt:
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46469 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (26 additions detected, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Caches parsed engine definitions to improve repeated workflow compilation performance.
Changes:
- Adds a concurrent engine-definition cache.
- Refreshes the generated auto-upgrade schedule.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/behavior_defined_engine.go |
Caches parsed engine definitions. |
.github/workflows/agentic-auto-upgrade.yml |
Changes the weekly schedule. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
| // | ||
| // Stored values are EngineDefinition values (not pointers) so cached entries are | ||
| // never mutated by callers that modify the returned pointer's fields. | ||
| var engineDefinitionCache sync.Map // map[string]EngineDefinition |
There was a problem hiding this comment.
Fixed in 54898e7. Added engineDefinitionBuiltinKeys sync.Map pre-seeded at startup by loadBuiltinEngineDefinitions. parseEngineDefinitionFromJSON now only stores to the cache when the incoming JSON key is present in that set, so custom engine edits in --watch sessions never add entries.
| // Store a value copy so the cache entry is never mutated by callers. | ||
| engineDefinitionCache.Store(engineJSON, def) | ||
| defCopy := def | ||
| return &defCopy, nil |
There was a problem hiding this comment.
Fixed in 54898e7. Added deepCopyEngineDefinition and deepCopyEngineBehaviorDefinition that recursively copy all reference types (slices, maps, pointers). Also added deepCopyAny for Options map[string]any values which can contain nested maps/slices from YAML unmarshal. Both cache-hit and cache-miss paths now store and return fully independent deep copies. Added a test (TestParseEngineDefinitionFromJSON_CacheHitReturnsDeepCopy) that caught a real bug: the original miss path shared the Options map between the cache entry and the returned pointer.
| on: | ||
| schedule: | ||
| - cron: "11 4 * * 6" # Weekly (auto-upgrade) | ||
| - cron: "21 3 * * 5" # Weekly (auto-upgrade) |
There was a problem hiding this comment.
The schedule drift was already reverted by the merge commit from main — the current HEAD has "11 4 * * 6" (Saturday 04:11 UTC) unchanged from before this PR.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The caching implementation is correct and well-structured. Values are stored rather than pointers (preventing caller mutation from corrupting the cache), a fresh copy is returned on every hit, type-assertion failure is handled with logging + eviction, and sync.Map provides thread safety. The cache is unbounded but that is fine given the small fixed set of built-in engines. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.5 AIC · ⌖ 4.26 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with two targeted suggestions.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Excellent root-cause analysis and clear PR description with before/after benchmark data
- ✅ Storing values (not pointers) in the
sync.Mapcorrectly prevents cache corruption from caller mutations - ✅ Defensive eviction + re-parse on type-assertion failure is a good resilience pattern
- ✅ The cache comment is thorough and explains why the cache is safe
Suggestions
/tdd— No unit tests for cache behaviour (mutation isolation, empty/invalid input). The copy-on-return contract is the most important correctness guarantee here and it's currently untested./codebase-design— Redundant double-copy on the slow path:Store(engineJSON, def)followed immediately bydefCopy := def; return &defCopyis one copy too many —return &defsuffices after the Store.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 31.2 AIC · ⌖ 4.68 AIC · ⊞ 6.7K
Comment /matt to run again
| engineDefinitionCache.Store(engineJSON, def) | ||
| defCopy := def | ||
| return &defCopy, nil | ||
| } |
There was a problem hiding this comment.
[/tdd] No unit tests cover the new cache behaviour — the mutation-isolation guarantee and the eviction/re-parse path are untested.
💡 Suggested test cases
Without tests, a future refactor could silently break the copy-on-return contract or leave the eviction path as dead code.
Three cases worth adding to behavior_defined_engine_test.go:
func TestParseEngineDefinitionFromJSON_CacheHitReturnsCopy(t *testing.T) {
engineJSON := `{"id":"test-engine"}`
first, _ := parseEngineDefinitionFromJSON(engineJSON)
first.RuntimeID = "mutated"
second, _ := parseEngineDefinitionFromJSON(engineJSON)
if second.RuntimeID == "mutated" {
t.Fatal("cache hit returned the same object; mutations leaked into cache")
}
}
func TestParseEngineDefinitionFromJSON_EmptyJSON(t *testing.T) {
def, err := parseEngineDefinitionFromJSON("")
if def != nil || err != nil {
t.Fatalf("expected nil, nil; got %v, %v", def, err)
}
}
func TestParseEngineDefinitionFromJSON_InvalidJSON(t *testing.T) {
_, err := parseEngineDefinitionFromJSON("{bad json")
if err == nil {
t.Fatal("expected error for invalid JSON")
}
}The eviction path (type-assertion failure) is harder to unit-test through the public API; a brief comment acknowledging it's defensive code is sufficient if full coverage isn't feasible.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 54898e7. Added behavior_defined_engine_cache_test.go with 10 tests covering all three of the suggested cases plus additional isolation cases:
TestParseEngineDefinitionFromJSON_EmptyJSONTestParseEngineDefinitionFromJSON_InvalidJSONTestParseEngineDefinitionFromJSON_CacheHitReturnsCopy(scalar mutation isolation)TestParseEngineDefinitionFromJSON_CacheHitReturnsDeepCopy(map mutation isolation)TestParseEngineDefinitionFromJSON_NonBuiltinNotCachedTestDeepCopyEngineDefinition_SlicesAreIndependentTestDeepCopyEngineDefinition_BehaviorsAreIndependentTestDeepCopyAny_NestedMapAndSlice
Added resetEngineDefinitionCacheForTest to ensure hermetic per-test state.
| return &def, nil | ||
| // Store a value copy so the cache entry is never mutated by callers. | ||
| engineDefinitionCache.Store(engineJSON, def) | ||
| defCopy := def |
There was a problem hiding this comment.
[/codebase-design] Redundant copy on the slow path — defCopy := def (line 551) is an extra allocation that adds no safety because Store on line 550 already received a copy.
💡 Suggested simplification
engineDefinitionCache.Store(engineJSON, def)
return &def, nil // def is a local value; its address is safe to returnOr keep a named copy if clarity is preferred, but remove the extra variable:
engineDefinitionCache.Store(engineJSON, def)
result := def
return &result, nilThe current pattern stores one copy and then immediately creates a second named copy before returning, which is confusing and allocates unnecessarily on every slow-path call.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 54898e7. The miss path now returns &def directly — no intermediate defCopy variable. The cache stores a separate deepCopyEngineDefinition(def) to maintain isolation from the returned pointer.
There was a problem hiding this comment.
REQUEST_CHANGES — cache correctness bug must be fixed before merge
The performance optimization is well-motivated and the benchmarks look real, but the implementation has a critical correctness bug and two medium concerns.
### Issues found
Critical — shallow copy does not protect the cache (lines 523, 551)
EngineDefinition contains reference fields: Options map[string]any, Behaviors *EngineBehaviorDefinition, and Auth []AuthBinding. The defCopy := def struct copy is shallow — all three fields share the underlying data with the cached entry. Any caller that mutates Options entries or follows Behaviors to modify nested fields will silently corrupt the cache for every subsequent caller. The comment explicitly claims cached entries are "never mutated by callers," but the copy does not enforce that. A deep clone (e.g. JSON round-trip) or a documented and enforced immutability contract at the call sites is required.
Medium — unbounded cache is a latent memory leak (line 514)
Keys are full JSON strings; there is no size cap, no TTL, and no eviction. The PR justification correctly states that built-in engine definitions are a small, fixed set — but that invariant is not enforced in code. A comment documenting the cardinality assumption and a size check would prevent a future regression.
Medium — package-level cache makes tests non-hermetic (line 514)
No reset/flush mechanism exists. Tests that exercise parseEngineDefinitionFromJSON will bleed cached state into one another, and a future bug fix that changes parse behavior for the same JSON input will be invisible in tests until process restart.
🔎 Code quality review by PR Code Quality Reviewer · 35.8 AIC · ⌖ 4.77 AIC · ⊞ 5.6K
Comment /review to run again
| if cached, ok := engineDefinitionCache.Load(engineJSON); ok { | ||
| if def, ok := cached.(EngineDefinition); ok { | ||
| defCopy := def | ||
| return &defCopy, nil |
There was a problem hiding this comment.
Shallow copy does not protect the cache from mutation: defCopy := def is a struct copy, but EngineDefinition contains reference fields (Options map[string]any, Behaviors *EngineBehaviorDefinition, Auth []AuthBinding) that are shared with the cached entry. Any caller that mutates Options or follows and modifies Behaviors will silently corrupt the cached value, affecting every subsequent caller — directly contradicting the comment's safety claim.
💡 Suggested fix
A true defensive copy requires a deep clone. The simplest correct approach is to re-encode to JSON and decode into a fresh struct, accepting that cache hits are slightly more expensive than a struct copy:
func cloneEngineDefinition(src EngineDefinition) (EngineDefinition, error) {
b, err := json.Marshal(src)
if err != nil {
return EngineDefinition{}, err
}
var dst EngineDefinition
return dst, json.Unmarshal(b, &dst)
}Or, if callers are known not to mutate returned fields, document that contract explicitly and enforce it with a lint rule rather than relying on a copy that only looks protective.
The bug is present in both the cache-hit path (line 523) and the cache-miss path (line 551).
There was a problem hiding this comment.
Fixed in 54898e7. Replaced the shallow struct copy with deepCopyEngineDefinition which recursively copies all reference types. The test TestParseEngineDefinitionFromJSON_CacheHitReturnsDeepCopy specifically validates that mutating Options on a returned definition does not affect subsequent cache hits.
| // | ||
| // Stored values are EngineDefinition values (not pointers) so cached entries are | ||
| // never mutated by callers that modify the returned pointer's fields. | ||
| var engineDefinitionCache sync.Map // map[string]EngineDefinition |
There was a problem hiding this comment.
Unbounded cache with no eviction grows forever on diverse inputs: engineDefinitionCache is a package-level sync.Map with no size cap, no TTL, and no eviction. Keys are full JSON strings; values are EngineDefinition structs (which contain maps and slices). If parseEngineDefinitionFromJSON is ever called with user-supplied or dynamically-varied JSON (not just the small set of built-in engines), the cache is a memory leak for the process lifetime.
💡 Suggested fix
If the intent is strictly to cache built-in engine definitions (a small, bounded set), enforce that at the call site or document the cardinality assumption in a //nolint comment and add a test assertion:
// engineDefinitionCache is bounded: only built-in engine JSON strings (of which
// there are O(10)) are ever passed to parseEngineDefinitionFromJSON.
// If that invariant changes, replace this with an LRU cache.
var engineDefinitionCache sync.MapAlternatively, add a max-size guard at store time and log a warning when the threshold is exceeded.
There was a problem hiding this comment.
Fixed in 54898e7. The cache is now strictly bounded to built-in engine JSON strings via engineDefinitionBuiltinKeys (pre-seeded from the embedded data/engines/*.md files at startup). Non-builtin JSON is never stored; TestParseEngineDefinitionFromJSON_NonBuiltinNotCached enforces this invariant.
| // | ||
| // Stored values are EngineDefinition values (not pointers) so cached entries are | ||
| // never mutated by callers that modify the returned pointer's fields. | ||
| var engineDefinitionCache sync.Map // map[string]EngineDefinition |
There was a problem hiding this comment.
No cache reset mechanism makes tests non-hermetic: The package-level engineDefinitionCache persists across all test runs in the same process. If parseEngineDefinitionFromJSON is called in one test with a given JSON string, subsequent tests — or a future bug fix that changes parse behavior for the same input — will silently receive the stale cached result.
💡 Suggested fix
Add an unexported reset helper usable in tests:
// resetEngineDefinitionCacheForTest clears the cache. Call in TestMain or individual test setup.
func resetEngineDefinitionCacheForTest() {
engineDefinitionCache.Range(func(k, _ any) bool {
engineDefinitionCache.Delete(k)
return true
})
}Or inject the cache as a dependency rather than relying on global state.
There was a problem hiding this comment.
Fixed in 54898e7. Added resetEngineDefinitionCacheForTest which clears both engineDefinitionCache and engineDefinitionBuiltinKeys. All new cache tests call it via defer to ensure hermetic isolation between test runs.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…omJSON - Add engineDefinitionBuiltinKeys sync.Map to track built-in engine JSON strings, pre-seeded by loadBuiltinEngineDefinitions via registerBuiltinEngineDefinitionJSON. parseEngineDefinitionFromJSON now only caches when the JSON key is in this set, preventing unbounded growth in compile --watch sessions with custom engine edits. - Implement deepCopyEngineDefinition / deepCopyEngineBehaviorDefinition to recursively copy all reference types (pointers, slices, maps). Cache-hit path returns a deep copy so callers cannot corrupt cached state through mutations. On the miss path the freshly-parsed value is returned directly (no shared references with the newly-stored cache entry). Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
- Replace maps.Copy with deepCopyAny for Options map[string]any values since
YAML unmarshaling can produce nested map[string]any or []any values that
maps.Copy would leave as shared references. deepCopyAny handles the full set
of types produced by yaml.Unmarshal into interface{}.
- Add explicit warning logs in loadBuiltinEngineDefinitions when YAML unmarshal
or JSON marshal fails during cache-key registration to aid debugging.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot resolve the merge conflicts on this branch. |
- Store a deep copy on cache miss so caller mutations cannot corrupt cached entries (the test suite caught this: Options map was shared between the returned &def and the cache entry). - Add behavior_defined_engine_cache_test.go with 10 tests covering: empty/invalid/non-object JSON, valid parse, cache-hit scalar isolation, cache-hit map deep copy, non-builtin not cached, and deepCopyEngineDefinition slice/map/behavior isolation. - Add resetEngineDefinitionCacheForTest helper for hermetic per-test cache state. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved threads addressed in commits Unbounded cache (3 threads) — Added Shallow/missing deep copy (3 threads) — Implemented Redundant miss-path copy (1 thread) — Miss path now returns No tests (1 thread) — Added Auto-upgrade schedule (1 thread) — Schedule was already restored to Local |
…ce-regression-compilemcworkflow # Conflicts: # .github/workflows/agentic-auto-upgrade.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
PR Triage\n\n| Field | Value |\n|---|---|\n| Category | feature (perf) |\n| Risk | medium |\n| Score | 58/100 (impact:28, urgency:15, quality:15) |\n| Action | fast_track |\n\nCaches parseEngineDefinitionFromJSON — 24% throughput improvement, 737 fewer allocs/compile. Correctness risk: sync.Map key is JSON string, copy-on-return is sound. Needs CI pass.
|
BenchmarkCompileMCPWorkflowregressed +14.5% (437μs vs 382μs historical average). Profiling pinned 24% ofCompileWorkflowwall-clock time toparseEngineDefinitionFromJSONdoing a repeated JSON→any→YAML bytes→struct round-trip.Root cause
injectBuiltinEngineImportIfNeededinjects the engine's built-in virtual file as an import on everyCompileWorkflowcall. The import processor extracts the engine config as a JSON string and passes it toregisterNamedEngineDefinitionFromJSON→parseEngineDefinitionFromJSON. Built-in engine files are registered once at startup and never mutated, so this JSON string is always identical for a given engine — yet the full marshal/unmarshal pipeline ran on every iteration.Fix
Add a package-level
engineDefinitionCache sync.Mapkeyed by the JSON input string, storingEngineDefinitionvalues (not pointers) so callers that mutate fields likeRuntimeIDdon't corrupt the cache. On cache hit, a fresh copy is returned. Unexpected type assertion failures are logged and recovered via eviction + re-parse.Results
~24% throughput improvement, 737 fewer allocations per compile. The regressed 437 μs is now well below the 382 μs historical average.