elsa-workflows / elsa-workflows/elsa-foundation

[Tech debt] Expressions/Caching/Modularity: perf gaps, race conditions, and inconsistent config-reading helpers

Open
#422 2 comments 0 reactions 0 assignees View on GitHub
needs-triage tech-debt
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.