elsa-core/doc/agent-logs/cshells-changes-summary.md
Sipke Schoorstra 3decb12680
feat: extend shells integration and modular server support (#7399)
* refactor(deps): use local CShells project refs

Replace CShells NuGet package references with direct project references to the local CShells source to enable developing and testing against local changes and simplify build integration across modules.

* Handle assembly load errors in feature discovery

Added error handling for assembly load failures in feature discovery to improve resilience. Also updated configuration for identity token options and removed unused service bus consumer dependencies. Simplified project structure by moving and cleaning up `Directory.Build.targets` files.

* Refactor configuration and service extension methods.

Moved `ShellSettingsExtensions` and `ShellConfiguration` to `CShells.Abstractions` for better modularity. Added new `ServiceCollectionFeatureExtensions` to improve options registration. Updated appsettings and references to support these changes.

* Introduce ManagementServiceCollectionExtensions to streamline activity and variable registration

Added `ManagementServiceCollectionExtensions` for registering Elsa activity types and variable descriptors, providing a modular and shell-feature-compatible approach to configuration. Updated relevant features to utilize these new extension methods, enhancing code modularity and reducing redundancy.

* Add resilience strategy registration to HTTP feature

Introduced `ResilienceServiceCollectionExtensions` to register resilience strategies within the `Elsa.Resilience.Core` module. Updated `HttpFeature` to incorporate resilience strategies, enhancing HTTP-related resilience configuration leveraging the new extension methods.

* Add new configuration options to JavaScriptFeature

Implemented multiple properties in `JavaScriptFeature` to enhance JavaScript execution: `AllowClrAccess`, `AllowConfigurationAccess`, `ScriptCacheTimeout`, `DisableWrappers`, and `DisableVariableCopying`. These additions enable more flexible and secure configuration of the Jint JavaScript engine.

* refactor(workflows): unify graph caching

Resolve workflow definitions first and store graphs under stable per-version-ID cache keys so different lookup paths share entries.
Centralize cache creation and change-token registration to remove duplicated caching logic.
Skip materializer-unavailable definitions to avoid caching null graphs and simplify flow.

* refactor(tests): centralize default IDs and materializer setup

Introduce constants for default definition and version IDs, and materializer name. Refactor tests to use these constants, streamline graph and definition resolution, and improve cache key creation by sharing logic across tests. Extend tests to check scenarios with unavailable materializers, ensuring caching only occurs for valid cases.

* extend(tests): enhance cache key verification in AutoUpdateTests

Added checks for both workflow definition and version cache keys in AutoUpdateTests to ensure comprehensive cache validation, improving test reliability and coverage.

* refactor(projects): update CShells project paths and solution configuration

Revised project reference paths in `Elsa.ModularServer.Web.csproj` for CShells projects and updated `Elsa.sln` to include new CShells projects, streamlining project organization and build configuration.

* Add `IWorkflowReferenceGraphBuilder` to `WorkflowManagementFeature`; rename `ResilienceShellFeature` to `ResilienceFeature`.

* Refactor `HttpFeature` to use `IMiddlewareShellFeature`, include `HttpWorkflowsMiddleware`, and update `HttpActivityOptions` defaults.

* Add `AddTypeAlias` and `AddVariableTypeAndAlias` extension methods to service collections

- Introduced `AddTypeAlias<T>` method in `ServiceCollectionExtensions.cs` for adding type aliases.
- Added `AddVariableTypeAndAlias<T>` method in `ManagementServiceCollectionExtensions.cs` to add variable types with aliases.

* Remove shell reload API endpoints, orchestrator, and associated tests from the codebase.

* Introduce `DefaultAdminUser` options and refactor `AdminUserInitializer` to use `IOptions`.

* Add user management endpoints: Delete, List, Update with enhanced user store functionality

* Implement `DefaultAdminUser` feature for initial admin bootstrap, decouple `SecurityRoot` from user management endpoints, update related documentation and permissions.

* Add role management endpoints: Delete, List, and Update, including role data models and handle obsolete SecurityRoot policy.

* Update CShells package references to version 0.0.12-preview.66 and refactor `TenantTaskManager` for improved task lifecycle management.

* Replace project references with package references in csproj files and remove unused folders.

* Integrate Nuplane features, add sample packages, and update dependency handling within ModularServer Web.

* Improve `CShells` startup endpoint registration and resolver handling

- Address duplicate endpoint registration by adding state-aware tracking and deduplication
- Resolve `WebRoutingShellResolver` constructor ambiguity by switching to factory-based registration
- Implement a startup-specific filter to prevent redundant endpoint remapping during `ShellsReloaded`
- Update project to use project references for `CShells` and `Nuplane` components in csproj files.

* Update logging configuration in appsettings for Development and Production

- Change default log level to 'Warning' in Development settings
- Adjust Microsoft.Hosting and Elsa.SamplePackage log levels to 'Information'
- Remove Microsoft.EntityFrameworkCore log level entry from Production settings

* Refactor assembly retrieval methods and update endpoint calls for consistency.

* Add `SampleEndpointFeature` and enhance logging and service integration

- Implement `SampleEndpointFeature` with a new endpoint for handling requests.
- Log endpoint access and integrate `ISampleService` with method `DoSomething`.
- Update logging configuration to include `CShells` and `Nuplane` log levels in Development settings.
- Update `Elsa.SamplePackage` to version 1.0.1 and manage dependencies with project and assembly references.
- Modify JSON configuration for `SampleEndpoint`.

* Update package versions for `CShells` to 0.0.13 and `Nuplane` to 0.0.1-preview.15 in props file.

* Refactor `DefaultAdminUserFeature` by renaming `ConfigureServices` to `Apply` and adjusting service registration method.

* Replace project references with package references across multiple projects and remove obsolete cshells-related solution entries.

* Remove `SampleCatalogEndpointExtensions.cs` and related endpoint mappings.

* Improve `TenantTaskManager` by using `TryRemove` for state clean-up and clarify `SemaphoreSlim` disposal behavior.

* Remove hardcoded default admin credentials and add warning for unconfigured AdminRoleName in admin user setup.

* Address unresolved review comments: fix doc comments, security defaults, compilation issue, and restore reload response contracts

Agent-Logs-Url: https://github.com/elsa-workflows/elsa-core/sessions/34eb1e13-833f-4b3c-9db6-2e9221d221b9

Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>

* Refine reload endpoints: use specific exceptions, add error messages, rename ReloadedAt to Timestamp, remove unused model

Agent-Logs-Url: https://github.com/elsa-workflows/elsa-core/sessions/34eb1e13-833f-4b3c-9db6-2e9221d221b9

Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>

* Potential fix for pull request finding 'Generic catch clause'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* Potential fix for pull request finding 'Generic catch clause'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* Potential fix for pull request finding 'Generic catch clause'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* Add `ExceptionExtensions` with `IsFatal` method and simplify exception handling in `TenantTaskManager`. Remove unused properties from `Directory.Build.props`.

* Add unit tests for `TenantTaskManager` and fix potential state orphaning issue.

* Potential fix for pull request finding 'Generic catch clause'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

* Fix logger dependency in `SampleEndpointFeature` constructor to use correct type.

* Add unit tests for Elsa Shells API endpoints and update solution configuration.

* Refactor ShellReload models: remove ShellReloadItemResult, update ShellReloadResponse properties.

* Potential fix for pull request finding 'Generic catch clause'

Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
2026-04-18 14:33:34 +02:00

8.7 KiB

Summary of CShells Changes

There were two separate runtime problems I was addressing in CShells:

  1. Duplicate endpoint registration during startup

    • This surfaced as: System.InvalidOperationException: Not allowed to configure endpoints after startup! Culprit: [Verbs()]
    • The root cause was that shell endpoints were getting mapped once during per-shell activation and then mapped again during the startup-wide ShellsReloaded flow.
  2. Ambiguous activation of WebRoutingShellResolver

    • This surfaced as: Unable to activate type 'CShells.AspNetCore.Resolution.WebRoutingShellResolver'. The following constructors are ambiguous: ...
    • The root cause was that dependency injection could satisfy more than one constructor, so type-based activation was ambiguous.

src/CShells.AspNetCore/Configuration/CShellsBuilderExtensions.cs

What changed

I made two important changes here.

1. Switched web-routing resolver registration from type-based to factory-based creation

Instead of registering WebRoutingShellResolver by type alone, I changed the resolver pipeline registration to explicitly construct it with:

  • IShellHost
  • WebRoutingShellResolverOptions

2. Changed endpoint-registration notification wiring to one shared concrete handler plus per-notification forwarders

Previously, ShellEndpointRegistrationHandler was registered separately as:

  • INotificationHandler<ShellActivated>
  • INotificationHandler<ShellDeactivating>
  • INotificationHandler<ShellRemoved>
  • INotificationHandler<ShellsReloaded>

I changed that to:

  • register one singleton ShellEndpointRegistrationHandler
  • register small forwarders for each notification type:
    • ShellActivatedEndpointRegistrationForwarder
    • ShellDeactivatingEndpointRegistrationForwarder
    • ShellRemovedEndpointRegistrationForwarder
    • ShellsReloadedEndpointRegistrationForwarder

Why I did that

The factory-based resolver registration was to fix the WebRoutingShellResolver constructor ambiguity. Since DI could resolve multiple constructors, it was not safe to rely on type activation. Explicit construction forces one intended activation path.

The forwarder-based notification registration was needed because the endpoint deduplication logic depends on shared in-memory state. Registering the same implementation separately for multiple closed generic interfaces can lead to different singleton instances being used per interface type. That breaks state sharing.

By routing all notifications through one shared concrete ShellEndpointRegistrationHandler, the deduplication state is preserved across ShellActivated, ShellDeactivating, ShellRemoved, and ShellsReloaded.


src/CShells.AspNetCore/Notifications/ShellEndpointRegistrationHandler.cs

What changed

This file contains the main startup-endpoint fix.

I added:

  • an optional IShellRuntimeStateAccessor
  • a lock-protected dictionary of tracked applied generations
  • helper methods to:
    • track generations
    • forget generations
    • reset tracked generations
  • an IsEndpointGraphCurrent(...) guard

I also changed notification handling so that:

  • ShellActivated registers endpoints and tracks the applied generation
  • ShellDeactivating removes endpoints and forgets the generation
  • ShellRemoved removes endpoints and forgets the generation
  • ShellsReloaded now:
    • filters to routable shells
    • checks whether the current endpoint graph is already current
    • skips rebuild if nothing changed
    • otherwise clears and rebuilds the full endpoint graph and retracks generations

Finally, I added the forwarder classes at the bottom of the file.

Why I did that

This was the core fix for the duplicate startup endpoint-registration path.

The failure mode was effectively:

  1. A shell becomes active
  2. Its endpoints are mapped
  3. A startup-wide ShellsReloaded is published
  4. All shell endpoints are rebuilt again

That second pass was a problem for FastEndpoints, which rejects endpoint configuration after startup has progressed beyond its expected window.

To fix that, I made ShellEndpointRegistrationHandler state-aware. It now checks whether the currently mapped shell IDs and applied generations already match runtime state. If so, it skips the aggregate rebuild instead of remapping the same endpoints again.

That preserves the ability to rebuild endpoints when shells truly change, while avoiding no-op startup remaps.


src/CShells/Hosting/ShellStartupHostedService.cs

What changed

I changed startup publication of ShellsReloaded to use a custom notification strategy:

  • startupShellsReloadedStrategy

I also added a private StartupShellsReloadedNotificationStrategy implementation that filters out ASP.NET Core notification handlers during the startup ShellsReloaded publication.

Why I did that

This was a startup-specific safety measure.

Even after adding deduplication logic, I wanted to prevent the startup-wide ShellsReloaded from immediately driving the ASP.NET Core endpoint-remapping path in the same startup cycle.

The intent was:

  • keep publishing ShellsReloaded as a meaningful system event
  • avoid redundant ASP.NET Core endpoint registration work during startup
  • preserve other non-ASP.NET-Core ShellsReloaded observers

So this was not a general suppression of ShellsReloaded; it was a targeted startup filter to avoid duplicate endpoint mapping during application bootstrap.


src/CShells/Resolution/ResolverPipelineBuilder.cs

What changed

I added support for registering resolver strategies with a factory:

  • Use<TStrategy>(Func<IServiceProvider, TStrategy> factory, int? order = null)

I also updated internal strategy registration so each strategy registration can now hold:

  • a type
  • an instance
  • or a factory

During pipeline build, the resolver strategy can now be registered using that explicit factory.

Why I did that

This was necessary to support the explicit-construction fix for WebRoutingShellResolver in CShellsBuilderExtensions.

Once I decided not to rely on DI constructor selection for that resolver, the pipeline needed a way to say:

register this resolver strategy, but instantiate it exactly this way

Without factory support in ResolverPipelineBuilder, there was no clean way to do that.


Supporting Test Changes

These test changes matter because they encode the design intent behind the production changes.

tests/CShells.Tests/Integration/AspNetCore/ApplicationBuilderExtensionsTests.cs

What changed

I added a regression test that verifies:

  • after a shell is activated and endpoints are mapped,
  • a subsequent ShellsReloaded with the same applied generation state
  • does not remap the endpoints again

I also added:

  • a counting web feature
  • a simple endpoint mapping counter

Why I did that

Because the production bug was about duplicate endpoint registration, I wanted a test that directly proves the bad sequence no longer happens.

The test protects the scenario that previously triggered the FastEndpoints startup exception.


tests/CShells.Tests/Integration/AspNetCore/ServiceCollectionExtensionsTests.cs

What changed

I added a regression test that verifies the DI registration shape now consists of:

  • one concrete ShellEndpointRegistrationHandler
  • forwarders registered for each notification interface

Why I did that

The shared-state design only works if all notification types ultimately delegate to the same concrete handler instance.

This test protects that registration shape so it is not accidentally simplified back into a broken form later.


Overall Rationale

The CShells changes were meant to make startup behavior more correct and deterministic.

For endpoint registration

I was trying to prevent this sequence:

  • shell activates
  • endpoints are mapped
  • startup-wide reload fires
  • endpoints are rebuilt again
  • FastEndpoints rejects the second configuration pass

So I:

  • added endpoint graph/generation tracking
  • ensured the tracking state is actually shared across notification types
  • skipped unnecessary aggregate rebuilds
  • added a startup-specific filter as a safety measure

For web routing

I was trying to prevent DI from making an ambiguous constructor choice when building WebRoutingShellResolver.

So I:

  • stopped relying on type-based activation
  • added factory-based resolver registration support
  • explicitly constructed the resolver with the intended dependencies

Net Effect

These changes were intended to move CShells away from startup behavior that depended on:

  • duplicate lifecycle-triggered endpoint registration
  • DI constructor-selection heuristics

and toward behavior that is:

  • state-aware
  • deduplicated
  • explicitly constructed where necessary

That is why each of those CShells files changed the way they did.