elsa-workflows / elsa-workflows/elsa-core
BackgroundActivityInvokerMiddleware couples bookmarks + jobs via dual magic property-bag side channels
- Dominant language
- C#
- Stars
- 7.9k
- Forks
- 1.5k
- Avg merge
- 15h 22m
- Merged PRs (30d)
- 114
Description
## Problem
`BackgroundActivityInvokerMiddleware` + `BackgroundActivityInvoker` implement background `Job`/`Task` activities by **overloading the bookmark resume path as an IPC bus**. Scheduling a job, creating a `BackgroundActivity` bookmark, capturing detached execution results, and rehydrating those results on resume are all tangled through **two layers of magic property bags**. That is accidental complexity: hard to reason about, easy to break when touching Complete/Schedule/bookmark APIs, and opaque compared to a typed result hand-off.
## Evidence (main / elsa-core)
### 1. Middleware dual role (schedule + resume rehydration)
`src/modules/Elsa.Workflows.Runtime/Middleware/Activities/BackgroundActivityInvokerMiddleware.cs`
- **Schedule path:** creates a `BackgroundActivity` bookmark with `BackgroundActivityStimulus.JobId`, then `DeferTask` → `IBackgroundActivityScheduler.ScheduleAsync`.
- **Resume path** (same middleware, when not background-executing and kind is Task/Job): reads workflow properties and applies output / journal / bookmarks / properties / outcomes / completed / scheduled activities, then publishes `BackgroundActivityExecutionCompleted`.
So one middleware type owns both “defer to a job” and “unpack job results after bookmark resume.”
### 2. Dual magic side channels
**Layer A — activity property bag** (`BackgroundActivityExecutionContextExtensions`):
- `"BackgroundOutcomes"`, `"BackgroundCompletion"`, `"BackgroundScheduledActivities"` (string keys; also flagged in `doc/security-assessment/elsa-core-software-quality.md` under Magic literals).
`ActivityExecutionContext.CompleteActivityAsync` and `ActivityExecutionContextSchedulerStrategy.ScheduleActivityAsync` **short-circuit** when `GetIsBackgroundExecution()` is true and write into those keys instead of completing/scheduling for real.
**Layer B — workflow / resume property bag** (`BackgroundActivityInvokerMiddleware` internal static helpers):
- `__BackgroundActivityOutput:{nodeId}`
- `__BackgroundActivityOutcomes:{nodeId}`
- `__BackgroundActivityCompleted:{nodeId}`
- `__BackgroundActivityJournalData:{nodeId}`
- `__BackgroundActivityBookmarks:{nodeId}`
- `__BackgroundActivityScheduledActivities:{nodeId}`
- `__BackgroundActivityProperties:{nodeId}`
`BackgroundActivityInvoker.ResumeWorkflowAsync` copies Layer A (+ outputs/journal/bookmarks/properties) into `ResumeBookmarkOptions.Properties` under the Layer B keys, then `IBookmarkQueue.EnqueueAsync` resumes by **bookmark id**. The middleware later pulls those keys back off `WorkflowExecutionContext` properties.
Invoker and middleware are therefore coupled through undocumented string keys (`internal static` helpers on the middleware type — a leaky shared contract).
### 3. Bookmarks-as-payload-of-bookmarks
During detached background invoke, bookmarks created on the activity context are serialized into `__BackgroundActivityBookmarks:{nodeId}` and re-`AddBookmarks`’d on resume (`CaptureBookmarkData`). Nested bookmark lifecycle rides inside the outer `BackgroundActivity` bookmark’s resume bag.
### 4. Job cancel is bookmark-driven (related, not duplicate)
`CancelBackgroundActivities` unschedules jobs when `BackgroundActivity` bookmarks are removed, using `BackgroundActivityStimulus.JobId`. That is the other half of the same bookmark↔job coupling (see also #6935 for cancel-token gaps). Obsolete `BackgroundActivityBookmark` still exists as a `[ForwardedType]` shim to `BackgroundActivityStimulus`.
### 5. elsa-extensions
Hangfire (`Elsa.Scheduling.Hangfire` … `HangfireBackgroundActivityScheduler` / `ExecuteBackgroundActivityJob`) is only the `IBackgroundActivityScheduler` implementation end of the same core contract. The property-bag / middleware smell lives in **elsa-core**; no separate extensions filing needed unless a Hangfire-specific bug appears.
## Why it matters
- **Maintainability:** Changing Complete, Schedule, bookmark create, or resume options requires understanding a shadow execution model that is not typed and spans Core + Runtime + middleware.
- **Correctness risk:** Failures or omissions in the bag (e.g. scheduled activities JSON, outcomes vs completed, nested bookmarks) surface as “activity didn’t complete / children didn’t run / hang after Hangfire” class symptoms (#4513-adjacent), not as compile-time misses.
- **Subtractive clarity:** The bookmark system is being used both as a wait handle *and* as a results mailbox. That fights a single resume story.
## Proposed subtractive direction (not a redesign epic)
Prefer **one typed hand-off** over expanding the bags:
1. Introduce a single typed capture/result object (e.g. outputs, outcomes/completed, journal, bookmarks, scheduled work, properties) owned next to `IBackgroundActivityInvoker` — not string keys on `WorkflowExecutionContext.Properties` / AEC properties.
2. Keep the outer `BackgroundActivity` bookmark (+ `JobId` stimulus) as the wait/cancel handle if needed; stop stuffing nested bookmarks and scheduled work through resume `Properties`.
3. Collapse middleware responsibilities: schedule/bookmark in one place; rehydration in the invoker or a dedicated applicator — not both halves living in `ExecuteActivityAsync` with magic keys.
4. Replace magic string constants with `const`/typed members (minimum bar already noted in the security-assessment doc).
5. Do **not** fold this into #7356 (native background execution architecture) unless Crew Lead wants one epic — #7356 is provider/runtime ownership; this issue is the **marshalling/IPC** leak in the current path.
## Not a duplicate of
- #8134 — `IncludeActivityInstanceId` default footgun (middleware is only an example caller)
- #8135 / #8136 — StimulusSender enqueue gaps / obsolete inbox DI
- #7356 — native background execution architecture (provider-level redesign)
- #6935 — cancel token / cancel background jobs on workflow cancel
- #4513 — Hangfire resume symptoms (possible downstream effect)
- #7726 / #5597 / #6523 / #7223 — bookmark persistence races
- #7735 — scheduling bookmark purge at startup
## Milestone
Unset — Elsa 3 Issue Triage / Crew Lead place. Soft proposal: **Backlog / tech-debt lane**, not Ready; not 3.9 prune unless Crew Lead wants subtractive wins there. Do not auto-assign Engineer.
Contributor guide
Research direction
Start with src/modules/Elsa.Workflows.Runtime/Middleware/Activities/BackgroundActivityInvokerMiddleware.cs and the related BackgroundActivityExecutionContextExtensions, then trace BackgroundActivityInvoker.ResumeWorkflowAsync through the bookmark queue. Map the schedule and resume paths and the named property-bag keys before changing their hand-off. Done means the background result flow has one typed capture/result contract, with middleware responsibilities and magic-key usage reduced as described.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100