fix(persistence): KeyValues, SerializedOptions, and atomic TryDeleteAsync

[sfmskywalker]

Refs #263, Refs #264, Refs #260

Main port of #266. main has the same Dapper schema, prefix-lookup, and non-atomic TryDelete bugs.

Cause

On a database created by the Dapper FluentMigrator assembly, two store mappings never matched the schema.

• #263: IKeyValueStore is registered against table KeyValues. Migrations only create KeyValuePairs.
• #264: BookmarkQueueItemRecord.SerializedOptions is written but BookmarkQueueItems has no such column.

Prefix lookups threw because ApplyFilter applied exact key AND prefix, and StartsWith bound @{field} while emitting @SearchTermLike.

#260: elsa-core #8538 adds IKeyValueStore.TryDeleteAsync. The default is find-then-delete (not atomic). Dapper and Mongo need count-checked overrides.

Fix

Guarded runtime migration 20008 (Elsa:Runtime:V3.9): create KeyValues if missing; add BookmarkQueueItems.SerializedOptions if missing. Down() is a no-op.

Prefix lookups: ApplyFilter is XOR; StartsWith binds @{field}StartsWith and, after rebasing onto #259, quotes the identifier.

Quiescence adoption (#260):

• Dapper TryDeleteAsync returns Store.DeleteAsync row count > 0.
• Mongo TryDeleteAsync is DeleteOneAsync(ApplyTenantScope(Eq(Key))).DeletedCount > 0. ApplyTenantScope is public so the store can use it. Does not use DeleteWhereAsync.
• Default-tenant Dapper reads/deletes match TenantId IS NULL OR TenantId = '' (narrow Dapper Store: SetTenantId overwrites every TenantId (incl. "*"), key-only upserts allow cross-tenant overwrite, and reads have no "*" visibility #245 gap for adoption).

Pin: ElsaVersion stays 3.10.0-preview.5722. Newest published 3.10.0-preview.* on feedz is still 5722 (commit=24f2ed1), which is before core main c1c935ce (#8538). No 3.10 package yet includes TryDeleteAsync on the interface. The overrides live on the concrete Dapper/Mongo stores and will implement the interface method once that pin exists.

Tests

Same suite as #266: migration-built SQLite KV/bookmark/outbox/Event resume; Dapper two-node race / not-found / NULL-tenant adoption (SQLite + PostgreSQL Testcontainers postgres:16 after #259); Mongo Testcontainers race / not-found / NULL-tenant. The default find-then-delete both-win case is exercised with a local stand-in until the interface method is in the pin.

Release note

Existing Dapper databases heal on their next migrate. Dapper and Mongo leftover-pause adoption is a count-checked delete. Default-tenant Dapper treats NULL and '' TenantId as the same stamp.

Notes

Rebased onto origin/main after #259 merged (212dc55). StartsWith conflict resolved as QuoteIdent + @{field}StartsWith. The 3.9 train is #266.

  

[sfmskywalker]

Code Review, Round 1/4: REQUEST_CHANGES + HIGH @ f2230a0

Scope: the single commit f2230a0 on main (base 90c95b1). It ports #266 up to 5736349: migration 20008 and the prefix-lookup fix. The head has since moved to ba604bc, which adds the #260 TryDeleteAsync port. That commit is not reviewed here. It is next round's delta. The same PostgreSQL heads-up as in the #266 Round 2 review applies to it; see below.

Summary: The port is faithful and correct:

• Every file matches #266 @ 5736349, apart from one test's location, which main requires.
• 20008 is the correct next migration on main, with no collisions.
• The prefix fix matches the EF store.
• Each cause has its own test, and each test fails when its cause is reverted.
• The runtime and PostgreSQL checks pass.

I would approve the code as it stands. As with #266, the change I'm requesting is the base merge. #259 merged into main at 10:59 CEST, so this branch now conflicts with its base (GitHub shows it as not mergeable, dirty). The resolution needs the same care as #266's.

---

Required: merge main (conflicts with #259)

The recipe is identical to the one in the #266 Round 2 review.

1. ParameterizedQueryBuilderExtensions.StartsWith. Keep the quoting and this PR's parameter:

var parameterName = $"@{field}StartsWith";
query.Sql.AppendLine($"and {query.QuoteIdent(field)} like {parameterName}");
query.Parameters.Add(parameterName, $"{value}%");

Here is what each wrong resolution does:

• Taking #259's side brings back the unbound @SearchTermLike. This PR's tests catch that.
• Taking this PR's side drops the quoting, and PG prefix queries fail with 42703: column "id" does not exist. No test in the repo catches this.

2. The test csproj. Keep both sides: Elsa, plus #259's FluentMigrator runners, Npgsql and Testcontainers.PostgreSql.

3. Semantic conflict. In #259's NonPgQuerySqlSnapshotTests, ["starts-with"] must become "and Name like @NameStartsWith".

4. Recommended. Add Assert.Contains("and \"Name\" like @NameStartsWith", queries["starts-with"], StringComparison.Ordinal); to PostgreSqlDialectTests.QueryBuilder_QuotesInlinedIdentifiers.

With items 1–3 applied on top of main @ 212dc55:

• Elsa.Persistence.Dapper.UnitTests: 54/54, including #259's PG end-to-end test run against a local PostgreSQL 17.
• Elsa.Dapper.UnitTests: 11/11.
• My SQLite and PG harness: green.

Heads-up for the #260 port (ba604bc, not reviewed): it has the same problem as the 3.9 branch.

• IsNullOrEmpty appends {field} unquoted.
• Store.ApplyTenantFilter now uses it for every default-tenant query.
• So once main is merged in, every default-tenant Dapper query on PG fails with 42703: column "tenantid" does not exist.

I reproduced this on the 3.9 branch, and the code on main is identical. Use query.QuoteIdent(field).

---

Port fidelity and main-specific handling

• Same files as #266. Runtime/V3_9.cs, DapperKeyValueStore, DapperKeyValuesAndBookmarkQueueTests.cs and the test csproj are byte-identical to #266 @ 5736349. The StartsWith change is the same three lines, at line ~241 on main instead of ~257.
• 20008 is the right number. The migration number, namespace (Elsa.Persistence.Dapper.Migrations.Runtime) and description (Elsa:Runtime:V3.9) match #266, and that is correct:
  • Main's Dapper migrations are the same set as release/3.9.0: 10001–10005, 20001–20004, 20006–20007 and 30001–30004. V3_7 is on both branches, and nothing on main has claimed 20008.
  • Sharing the number keeps VersionInfo consistent across the 3.9 and 3.10 trains: a database migrated by 3.9.x already has 20008 recorded when it moves to 3.10.
  • It also means a later merge of release/3.9.0 into main adds identical files, so the migration won't conflict.
• No collisions.
  • #259 is merged. It adds no migration: it edits existing ones and adds MigrationDatabases.cs.
  • I checked every open PR against main. Only #132 (split project) and #150 (TenantId) touch Dapper, and neither adds a migration.
  • There is no other V3_9 on main.
• Main-specific difference, handled. Main has no Elsa.Dapper.UnitTests/ParameterizedQueryBuilderExtensionsTests.cs, because the LessThan tests only exist on 3.9. So the binding test lives in the existing Elsa.Persistence.Dapper.UnitTests/ParameterizedQueryBuilderExtensionsTests.cs. It is the same test.
  • If release/3.9.0 is later merged into main, this test will exist in two assemblies. That's harmless, but worth knowing for whoever does that merge.
• Core dependency. Main builds against Elsa 3.10.0-preview.5722. On core main, KeyValueFilter.Apply and KeyValueWorkflowDispatchOutboxStore are the same as on release/3.9.0, so the EF-parity reasoning and the outbox coverage carry over.

Verified

1. Migration (the file is identical to #266's, so its per-provider DDL is too):

Provider	KeyValues	SerializedOptions
SQL Server	NVARCHAR(255) PK / NVARCHAR(255) / NVARCHAR(MAX)	NVARCHAR(MAX)
PostgreSQL	text	text
SQLite	TEXT	TEXT

On this branch:

• Fresh migrate: SQLite and PG17 both apply 20008.
• Upgrade from 20007: existing BookmarkQueueItems rows are kept, with SerializedOptions = NULL. A legacy KeyValuePairs table is left untouched.
• Guards: a quoted, hand-created table and column make Up a no-op, and their rows are kept. With only the column added by hand, Up still creates KeyValues.
• Down: Down() is a documented no-op. Objects and data survive MigrateDown(20007), and Up again works, on both SQLite and PG17.

2. Prefix semantics match EF. Prefix or exact, then In. On fresh migrated SQLite:

• Prefix app: returns only app:*. Exact app: returns null. Prefix combined with Keys returns the intersection.
• StartsWith with Key = null returns all rows.
• Tenant filtering is intact: a t1 row is not visible to the default tenant.

The SQL for StartsWith = false is unchanged. DapperKeyValueStore is the only caller of the helper, and the definition and instance search helpers are untouched.

3. The tests fail when their cause is reverted. All of them build fresh DBs with MigrateUp only.

Reverted	Tests that fail
Cause 1 only (Is + In + StartsWith together)	prefix test (Collections differ); outbox test (Assert.Single(): the collection was empty)
Cause 2 only (@SearchTermLike in the SQL, bound as @Id)	prefix and outbox tests (Must add values for ... @SearchTermLike); binding unit test

The outbox FindManyAsync is covered, through the index, recovery and legacy prefix scans.

4. Runtime harness on fresh migrated SQLite at f2230a0: 9/9.

• An AcrossReactivations pause persists across restart and clears after Resume. Startup and background tasks run, with 0 warnings or errors.
• Event publish works, including with no listener.
• WaitForCompletion resumes the parent.
• A queued stimulus resumes its workflow to Finished.
• SerializedOptions round-trips.
• Outbox: Save followed by FindMany returns items in CreatedAt order. FindMany(1) returns the oldest, and Delete works.

5. PostgreSQL 17 (merged tree, since the Dapper PG path only works with #259):

• The DDL is as above.
• KV store: a 100 KB upsert, FindMany(Keys), FindMany(StartsWith) and Delete work. APP:upper doesn't match app:.
• Outbox: Save, FindMany, FindMany(1) and Delete work.
• Bookmark queue: a 100 KB Options payload round-trips.
• Quiescence: a pause survives a restart.
• Migration Down/Up: works.

6. CI at f2230a0: all green. It ran against 90c95b1, before #259 merged.

• ubuntu-latest:
  • Elsa.Persistence.Dapper.UnitTests: 28/28, up from 21 on main before this PR, so all 7 new tests ran (6 store/migration tests and the binding test).
  • Elsa.Dapper.UnitTests: 11/11, unchanged.
• CodeQL (actions and C#), GitGuardian, CLA and submit-nuget pass.
• The new head ba604bc can't get a pull_request run until the conflict is resolved.

---

Non-blocking notes

These are the same as in the #266 Round 2 review:

1. Simplify the binding test's last assert. Assert.DoesNotContain("Id", query.Parameters.ParameterNames) says the same as the current Where(name => name == "Id") form, and reads plainly. Also drop the redundant DoesNotContain checks.
2. Pin the no-op Down() with a small Up → Down → Up test, so nobody "fixes" the empty method. The remaining upgrade, legacy-layout and column-guard tests are cheap to add. They pass locally.
3. The Dapper KV store ignores KeyValueFilter.Take and OrderByKey. This predates the PR. Outbox results are still correct, but each poll reads every outbox row. A Keys lookup with one parameter per pending item hits SQL Server's 2,100-parameter limit at roughly 2,100 pending items. This deserves a follow-up issue.
4. Carried over from the #266 reviews: the nullable Value column, the shape-blind and case-sensitive guards, the Id length, and LIKE wildcards and case. These belong in the release note.
5. The PR body says Refs #263, Refs #264. #266 says Fixes, but it targets a non-default branch, so GitHub closes neither issue automatically. Either switch this PR to Fixes, or close the issues by hand once both PRs land.

What was not run

• SQL Server: no container runtime is available here. The change is dialect-neutral.
• The #260 port (ba604bc): not reviewed, apart from the PG heads-up above.

[cursor]

Re: Round 1 — the required main rebase is done, with the same recipe as #266 R2.

Conflicts (items 1–3) at 81f9fb6 on top of 212dc55 (#259):

1. StartsWith is QuoteIdent + @{field}StartsWith.
2. Test csproj keeps Elsa plus fix(dapper): match PostgreSQL provider names and quote identifiers via dialect hook #259's runners / Npgsql / Testcontainers.PostgreSql.
3. NonPgQuerySqlSnapshotTests ["starts-with"] is and Name like @NameStartsWith.

Recommended item 4 is on the new tip 3f110a2: PostgreSqlDialectTests.QueryBuilder_QuotesInlinedIdentifiers asserts and "Name" like @NameStartsWith.

#260 PG heads-up: IsNullOrEmpty uses query.QuoteIdent(field). PG Testcontainers TryDelete coverage is in DapperKeyValueStoreTryDeletePostgreSqlTests.

ElsaVersion stays 3.10.0-preview.5722 — still no published 3.10.0-preview.* whose nuspec commit= is at or after core main c1c935ce (#8538). Concrete-store TryDeleteAsync methods are in place until that pin lands.

CI at 81f9fb6 was green (ubuntu-latest 4m16s: Persistence.Dapper 64/64 including PG, Mongo 45/45 in 38s). 3f110a2 is the QuoteIdent assert only.

Non-blocking notes acknowledged; left for a follow-up. On note 5: #266 stays Fixes for the 3.9 train; this PR stays Refs plus “main port of #266”.

[sfmskywalker]

Code Review, Round 2/4: APPROVE + HIGH @ 3f110a2

Scope: the delta since the Round 1 head f2230a03: the rebase onto main @ 212dc55 (#259), plus the #260 port (d70549a..3f110a2). The src delta is identical to #266 except for doc comments, so the shared analysis is in the #266 Round 3 review. This review covers what differs on main. Everything was re-verified on a fresh migration-built SQLite DB, local PostgreSQL 17 and MongoDB 7.0.

History: no intermediate commit contains conflict markers (checked every commit in the range). 6ef1f5f (empty retrigger) and 95c7047 (comment-only) are noise, so a squash merge is preferred but not required.

1. Merge resolution: #259 survives intact ✅

The result is the same as described in the #266 Round 3 review:

• Store.DeleteAsync is byte-identical to base, and the #259 files (dialects, migration lists, PG migration and processor-name tests) are untouched.
• QuoteIdent call sites rise by exactly one.
• StartsWith is quoted and bound as @{field}StartsWith. Here the new PG binding test lives in Elsa.Persistence.Dapper.UnitTests.
• The #259 PG E2E passes on this head against PG17.

2. #260 atomic TryDeleteAsync ✅

The code is identical to #266: a single tenant-scoped conditional DELETE on rows affected (quoted on PG), and Mongo DeleteOneAsync/DeletedCount. ApplyTenantScope became public here, with a doc comment.

I reverted each fix in turn on this head:

Revert	Failing tests
Dapper atomic delete	#260 PG: two Dapper stores racing…
Mongo atomic delete	#260: two Mongo stores racing…
Tenant change	both NULL-tenant tests (SQLite + PG)
IsNullOrEmpty quoting	4 PG tests, including the #259 PG E2E
StartsWith quoting	both PG quoting unit tests

The PostgreSQL row-lock check gave exactly one winner in each of 3 rounds. As on #266, the SQLite race test can't detect a revert.

3. Default-tenant NULL vs empty ✅

The results are identical to the #266 Round 3 review. On SQLite and PG17, default and no-context see the NULL and '' rows, t1 sees only t1, writes stamp '', and * rows stay invisible (#245 remainder). This matches EF apart from that known gap.

4. The main pin (3.10.0-preview.5722): safe to merge, but bump before release ⚠️ (non-blocking for this PR)

• What 5722 contains: it is built from core 24f2ed1f, which is 18 commits behind c1c935ce.
  • Its Elsa.KeyValues has no IKeyValueStore.TryDeleteAsync.
  • Its runtime has only the elsa.quiescence.pause. key: no host-pause key and no legacy-adoption sweep. So no core code calls TryDeleteAsync.
  • As of Oct 2 there is no 3.10 preview newer than 5722.
• Safe to merge now: on this pin the ext TryDeleteAsync methods are plain public methods. They compile and are correct, and the doc comment says so accurately ("becomes IKeyValueStore.TryDeleteAsync once ElsaVersion includes elsa-core#8538"). Green CI is not an accident.
• But the fix is inert on main until ElsaVersion moves to a build that includes c1c935ce or later. After that bump, a public method with a matching signature implicitly implements the new interface member, so no ext code change is needed. I confirmed with a minimal repro that an interface call dispatches to the class method rather than the default implementation.
• Upgrade path: my local legacy-pause adoption check fails on this head only because 5722 has no adoption logic. That is a pin consequence, not an ext defect. The 3.8 → 3.10 upgrade behaviour therefore can't be validated until the pin bump.
• Required before the 3.10 release: bump ElsaVersion to a c1c935ce-or-later 3.10 build, and rerun the PG/Mongo race and NULL-tenant tests on it.

5. CI ✅

The pr workflow (run 36403231686, pull_request event, success, Sep 28 11:23 CEST): 467 passed, 2 skipped (the existing Slack and Azure Service Bus tests).

• Elsa.Persistence.Dapper.UnitTests: 64/64, 0 skipped. The PG tests executed.
• Elsa.Dapper.UnitTests: 11/11.
• Elsa.MongoDb.UnitTests: 45/45.

CodeQL, GitGuardian, CLA and submit-nuget all pass. pr.yml paths are **/*, so the run was not short-circuited.

6. Maintainability (HIGH bar)

Same items as the #266 Round 3 review: the SQLite race test caveat, the PG IsNullOrEmpty quoted-form assertion, the duplicated TestTenantAccessor, the Mongo named-tenant test and the carried-over items.

On main the barrier helper is the simpler BarrierFindThenDelete; aligning the name with the 3.9 branch would ease future ports. All of these are non-blocking.

Verdict

APPROVE + HIGH. The code is correct, #259 is intact, the tests fail when each fix is reverted, and CI is genuinely green with PG executed. Merging on the 5722 pin is safe. A pin bump to a c1c935ce-or-later 3.10 build is required before the 3.10 release, because until then the #260 fix is not exercised by core on main.