elsa-workflows / elsa-workflows/elsa-foundation
[Tech debt] Expressions/Caching/Modularity: perf gaps, race conditions, and inconsistent config-reading helpers
- Dominant language
- C#
- Stars
- 5
- Forks
- 1
- Avg merge
- 3h 52m
- Merged PRs (30d)
- 212
Description
**Severity: medium | Ease: easy-medium** — batched findings from `src/Elsa/Expressions/**`, `src/Elsa/Caching/**`, `src/Elsa/Locking/**`, `src/Elsa/Modularity/**`, `src/Elsa/Foundation/**`.
1. **`LibraryResourcePreProcessor.cs:11-21`:** re-reads and re-decodes an embedded JS library resource from the assembly manifest on every single JS expression evaluation, even though it never changes for the process lifetime — a hot-path allocation cost.
2. **`LiquidTemplateManager.cs:35-36`:** the Liquid template cache uses the raw template source text as the `IMemoryCache` key with no namespace prefix, unlike the sibling Jint `PreparedScriptFactory` (`"jint:script:"` prefix) — collision risk with any other feature caching by literal string in the same shared app-wide `IMemoryCache`.
3. **`LiquidTemplateManager.cs:33-53,30`:** if a malformed template's fallback `{% raw %}`-wrapped re-parse *also* fails, `null` is cached and returned as the template, and `RenderAsync` calls `.RenderAsync` on it with no null check — unhandled `NullReferenceException` instead of a clear "template invalid" error.
4. **`IChangeTokenSignaler`/`IChangeTokenSignalInvoker`** are a redundant pass-through layer with byte-for-byte identical method signatures and a pure-forwarding implementation; the `CancellationToken` threaded through all three layers is never actually consulted anywhere.
5. **`DistributedLockProviderAdaptor.cs:10-33`:** timeout-resolution (`timeout ?? options.Value.LockAcquisitionTimeout`) and handle-wrapping logic duplicated across three adaptor methods.
6. **`JsonShellFeatureConfigurationStore.cs` (`SaveAsync`):** read-check-write race with no lock serializing concurrent calls, defeating its own optimistic-concurrency revision check.
7. **`OpenIddictTokenService.cs`:** in-memory token dictionaries never evict expired/revoked entries — unbounded memory growth in long-running hosts.
8. **`SecurityContracts.cs`:** `KnownWeakKeys` blocklist uses `StringComparer.Ordinal`, so case variants like `"Secret"`/`"ChangeMe"` bypass the weak-signing-key guard.
**Proposed fix:** Cache the decoded library-resource string in (1); prefix the Liquid cache key in (2); guard against a null cached template in (3); collapse the redundant pass-through layer (or document its purpose) and drop the unused token in (4); extract shared helpers in (5); add a lock around (6)'s read-check-write; add eviction to (7); use case-insensitive comparison for (8).
_Found via automated codebase quality audit._
Contributor guide
No contributing guide indexed for this repository
Research direction
Split the audit by the named files and inspect the listed methods and interfaces under src/Elsa/Expressions, Caching, Locking, Modularity, and Foundation. Start with one concern, such as LibraryResourcePreProcessor.cs or LiquidTemplateManager.cs, and review nearby tests and call sites before changing behavior. Done means each selected finding has a focused fix and the relevant existing checks pass without regressions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend, infrastructure, performance, security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 28/100