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

[sfmskywalker]

Fixes #263
Fixes #264
Fixes #260

Cause

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

• #263: DapperWorkflowRuntimePersistenceFeature registers IKeyValueStore against table KeyValues. Migrations only create KeyValuePairs (Runtime/V3_1 20002, TenantId in V3_3). Every Save / Find / FindMany / Delete fails (no such table: KeyValues on SQLite; 42P01 on PostgreSQL). The store is not repointed at KeyValuePairs because three layouts of that table exist in the wild.
• #264: BookmarkQueueItemRecord.SerializedOptions is written on every AddAsync / SaveAsync, but BookmarkQueueItems (Runtime/V3_3 20004) has no such column. Every bookmark-queue enqueue fails (no column named SerializedOptions on SQLite; 42703 on PostgreSQL). That includes Event publish when no bookmark matches, background-activity resume, and WaitForCompletion.

With the table in place, prefix lookups still threw on every provider (CR R1 / B1). ApplyFilter applied Is(Id) and StartsWith together, and StartsWith emitted @SearchTermLike while binding @{field}. That breaks FindManyAsync(StartsWith) and therefore KeyValueWorkflowDispatchOutboxStore.FindManyAsync.

#260: elsa-core #8539 adds IKeyValueStore.TryDeleteAsync. The default implementation finds then deletes, so two nodes adopting the same legacy pause can both get true and both write the host key. EF and Memory override it atomically; Dapper and Mongo fell back to the default.

Fix

New runtime migration 20008 (Elsa:Runtime:V3.9):

• Create table KeyValues if it does not exist: Id PK string, TenantId nullable string, Value nullable long text — matching KeyValuePairRecord.
• Add nullable long-text SerializedOptions to BookmarkQueueItems if the column is missing.

Both steps are existence-guarded. Down() is a no-op so a rollback cannot drop a hand-created KeyValues table or SerializedOptions column. KeyValuePairs is not touched.

Prefix lookups:

• DapperKeyValueStore.ApplyFilter applies either StartsWith or exact Is, then In.
• StartsWith binds @{field}StartsWith so the SQL parameter matches and does not collide with Is(@{field}). After rebasing onto fix(dapper): match PostgreSQL provider names and quote identifiers via dialect hook #258, the identifier is also QuoteIdent'd.

Quiescence adoption (#260):

• Pin ElsaVersion to 3.9.0-preview.5726 (core release/3.9.0 @ fa68369a, includes #8539). ElsaStudioVersion stays 3.9.0-preview.1757. This is the final 3.9 cut pin.
• Dapper TryDeleteAsync is a tenant-scoped DELETE that returns Store.DeleteAsync row count > 0.
• Mongo TryDeleteAsync is DeleteOneAsync(ApplyTenantScope(Eq(Key))).DeletedCount > 0. It does not use DeleteWhereAsync.
• Default-tenant Dapper reads/deletes match TenantId IS NULL OR TenantId = '' so a legacy NULL row is visible to Tenant.Default (narrow Dapper Store: SetTenantId overwrites every TenantId (incl. "*"), key-only upserts allow cross-tenant overwrite, and reads have no "*" visibility #245 gap for adoption). The rest of Dapper Store: SetTenantId overwrites every TenantId (incl. "*"), key-only upserts allow cross-tenant overwrite, and reads have no "*" visibility #245 stays on 3.10.

Tests

Fresh DBs are built only with MigrateUp. Then:

• #263: IKeyValueStore Save / Find / FindMany / Delete; prefix FindManyAsync; outbox Save / FindMany.
• #264: bookmark-queue Options round-trip; Event start + resume to Finished.
• A DB stopped at 20007 that already has KeyValues and SerializedOptions migrates as a no-op.
• #260 Dapper (SQLite): two stores race TryDeleteAsync and exactly one wins; not-found returns false; the default DIM (find then delete) lets both racers return true; a NULL TenantId row is found and TryDeleted by the default tenant, and is invisible to a named tenant.
• #260 Dapper (PostgreSQL via Testcontainers postgres:16, now that fix(dapper): match PostgreSQL provider names and quote identifiers via dialect hook #258 is on the branch): the same race, not-found, and NULL-tenant adoption cases.
• #260 Mongo (Testcontainers mongo:7.0.24): the same race, not-found, DIM both-win, and NULL-tenant adoption cases.

Release note

Existing Dapper databases heal on their next migrate. 20008 creates KeyValues and adds BookmarkQueueItems.SerializedOptions only when they are missing. Down() does not drop them. KeyValuePairs is unchanged.

Dapper and Mongo IKeyValueStore.TryDeleteAsync are now atomic (row-count / DeletedCount). Legacy-pause adoption is safe on those providers. Default-tenant Dapper queries treat NULL and '' TenantId as the same stamp so leftover 3.8 pause rows are adopted.

Notes

Rebased onto origin/release/3.9.0 after #258 merged (9d9049e). StartsWith conflict resolved as QuoteIdent + @{field}StartsWith. Main port is #267.

  

[sfmskywalker]

Code Review, Round 1/4: REQUEST_CHANGES + MEDIUM-HIGH @ 360f2fa

Scope: the single commit 360f2fa on release/3.9.0 (base dc5b341). It adds a guarded runtime migration 20008 (Runtime/V3_9) and 4 SQLite tests. #260's future delta is out of scope, apart from the notes at the end.

Summary: The migration is correct:

• The schema matches the store.
• Provider types are right, with no truncation.
• 20008 is the right number and doesn't collide with anything.
• The guards work on SQLite and PostgreSQL.
• Upgrades from 20007 work.
• Quiescence and the bookmark-queue paths now work end to end on fresh, migration-built databases.

There is one blocker. With the table in place, IKeyValueStore's prefix query (StartsWith) still throws on every provider. #263 lists that path as failing (FindManyAsync with StartsWith, and the transactional outbox). So Fixes #263 is incomplete, and the outbox and the clustering heartbeat monitor are still unusable on Dapper.

---

Blocker

B1. DapperKeyValueStore prefix queries throw: Must add values for the following parameters: @TenantId, @Id, @SearchTermLike (PG: 42703 column "searchtermlike" does not exist)

Repro on a fresh, migration-built SQLite DB (and on PostgreSQL 17 with #258 merged): save app:1, app:2 and other:1, then call:

await kv.FindManyAsync(new KeyValueFilter { Key = "app:", StartsWith = true });

It throws. KeyValueWorkflowDispatchOutboxStore.FindManyAsync fails the same way (its SaveAsync works).

The affected callers:

• the outbox store: 4 StartsWith filters in core release/3.9.0, used when UseTransactionalOutbox is enabled
• InstanceHeartbeatMonitorService in clustering

Quiescence only uses the exact-key FindAsync, so it is fine.

There are two defects:

1. Modules/Runtime/Stores/KeyValueStore.cs, ApplyFilter (lines ~45-50), applies Is(Id, filter.Key) and StartsWith(Id, filter.StartsWith, filter.Key). Even with correct binding, that means Id = 'app:' AND Id LIKE 'app:%', which never matches a prefix. It should be one or the other, as in the EF store.
2. Extensions/ParameterizedQueryBuilderExtensions.cs, StartsWith (lines ~255-265), writes like @SearchTermLike into the SQL but binds the value as @{field} (@Id). That leaves @SearchTermLike unbound and collides with Is's @Id.

I validated this fix locally. With it, the prefix query returns the 2 app: rows and the outbox FindManyAsync returns its entry:

// KeyValueStore.ApplyFilter
if (filter.StartsWith) query.StartsWith(nameof(KeyValuePairRecord.Id), true, filter.Key);
else query.Is(nameof(KeyValuePairRecord.Id), filter.Key);
query.In(nameof(KeyValuePairRecord.Id), filter.Keys);

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

Please add tests on the migration-built DB:

• FindManyAsync(StartsWith) on IKeyValueStore, which should return only the matching prefix
• a Save/FindMany round trip through the KV outbox store, or the heartbeat path

Ordering: #258 edits the same line in StartsWith (it becomes {query.QuoteIdent(field)} like @SearchTermLike) but does not fix the binding. Whichever PR lands second gets a small textual conflict; keep the quoting and the new parameter name together.

---

Verified (no action needed)

1. Schema matches the store and records

• KeyValues(Id PK, TenantId NULL, Value) matches KeyValuePairRecord and the store SQL: Id/TenantId/Value, with SerializedKeyValuePair.Key/SerializedValue mapped onto them in the store.
• SerializedOptions is a nullable long string, matching the record's string?.
• All store queries filter on Id, which is the PK, plus the usual tenant filter. The table is small, so no extra index is needed.

2. Per-provider DDL. Checked with FluentMigrator 7.2's generators, and on real SQLite and PostgreSQL 17:

Provider	KeyValues	SerializedOptions
SQL Server 2016+	[dbo].[KeyValues] ([Id] NVARCHAR(255) NOT NULL, [TenantId] NVARCHAR(255), [Value] NVARCHAR(MAX), PRIMARY KEY ([Id]))	NVARCHAR(MAX)
PostgreSQL	"public"."KeyValues" ("Id" text NOT NULL, "TenantId" text, "Value" text, PRIMARY KEY ("Id"))	text
SQLite	"KeyValues" ("Id" TEXT NOT NULL, "TenantId" TEXT, "Value" TEXT, PRIMARY KEY ("Id"))	TEXT
MySQL 8 / Oracle 12c	LONGTEXT / NCLOB for Value	same

Dapper has no MySQL/Oracle dialect, so those two rows are informational only.

• On PostgreSQL with #258 merged, a 100 KB Value and a 100 KB Options payload round-trip intact.
• The quoted mixed-case "KeyValues" and "SerializedOptions" match #258's identifier quoting.
• 20008 has no IfDatabase branch, so it runs on every processor, including SqlServer2016 and the other aliases. #258's alias-aware provider list doesn't affect it.

3. Version number.

• On release/3.9.0, the runtime migrations are 20001-20004 and 20006-20007 (V3_7, from #251/#252, already merged). 20008 is next.
• Management uses 10001+ and Alterations 30001+.
• Neither #258 nor #259 adds a migration: they edit existing ones and add MigrationDatabases.cs.
• A fresh PostgreSQL run applied 10001-10005, 20001-20004, 20006-20008 and 30001-30004.

4. Guards.

• FluentMigrator's TableExists/ColumnExists use sqlite_master/pragma_table_info on SQLite, information_schema + public on PostgreSQL, and INFORMATION_SCHEMA + dbo on SQL Server.
• On SQLite and PostgreSQL, a hand-created KeyValues table and SerializedOptions column make Up a no-op, and the existing rows are kept.

5. Existing databases, upgraded from 20007 on SQLite:

• An existing BookmarkQueueItems row survives with SerializedOptions = NULL.
• KeyValues is created, and KV upserts work.
• A legacy KeyValuePairs table (the Key/Value/TenantId layout, with a row) is left untouched: same schema, same row.

6. End-to-end runtime checks on fresh migration-built SQLite DBs (and PostgreSQL where noted):

• AcrossReactivations pause:
  • Node 1 starts with no warnings or errors, and its later startup and background tasks run.
  • The pause is persisted as a KeyValues row.
  • A restarted node comes up as AdministrativePause, with the reason kept. This also works on PostgreSQL with #258.
  • After Resume and another restart, the node starts as None and the row is gone.
• Events:
  • Publishing an event, with or without listeners, no longer throws.
  • Queued items carry SerializedOptions.
  • An event published before its bookmark exists is queued and later resumes the workflow to Finished.
• ExecuteWorkflow with WaitForCompletion: the parent resumes through the bookmark queue, and both workflows finish.
• Non-null Options round-trip: after the upgrade, Input[x] = 42 comes back intact.
• Down: MigrateDown(20007) followed by Up works on both SQLite and PostgreSQL.

7. Tests. The 4 new tests are readable and deterministic: AAA layout, a temp file per test, and Pooling=false. They use FluentMigrator-built databases; the no-op test hand-creates objects on purpose, which is legitimate for that scenario.

• Revert-proven: without V3_9 the 3 functional tests fail (no such table: KeyValues / no column named SerializedOptions).
• Without the guards, the no-op test fails (table "KeyValues" already exists).

8. CI (all green):

• ubuntu-latest passes: Elsa.Persistence.Dapper.UnitTests reports 25/25 passed, up from 21, so the 4 new tests ran.
• submit-nuget, GitGuardian and the CLA check all pass.
• The NU1903 SQLitePCLRaw warning appears in the untouched test projects too, so it predates this PR.

---

Non-blocking notes

1. Down() can destroy user data. Up skips a KeyValues table or SerializedOptions column that already existed (hand-created workarounds), but Down drops them unconditionally. I confirmed this: a hand-created, populated KeyValues survives Up and is dropped by MigrateDown(20007). Either document it in the XML doc and release note ("rolling back 20008 drops KeyValues/SerializedOptions even if they pre-date it") or make Down a no-op. Rollbacks are rare, so documenting it is enough.

2. Value is nullable, while the record (string Value = default!), SerializedKeyValuePair.SerializedValue and the EF model all treat it as required. Consider .NotNullable(), or add a comment explaining why it's nullable.

3. The guards are shape-blind.

   • A hand-created KeyValues table with the wrong columns (e.g. no TenantId) is silently accepted, and the store keeps failing.
   • On PostgreSQL, an unquoted, hand-created keyvalues table isn't detected, because the check is exact-case under force-quoting, so a second "KeyValues" table gets created.
   • On SQL Server, the check assumes the dbo schema, which is the existing pattern.

   Worth one line in the release note ("drop or rename any hand-made KeyValues table before upgrading, unless it matches Id/TenantId/Value").

4. Id length. Id is NVARCHAR(255) on SQL Server. That is fine for the current keys (quiescence, outbox and heartbeat keys are short). Just noting the limit.

5. LIKE wildcards. After B1 is fixed, prefix values containing % or _ are still not escaped, and SQLite LIKE is case-insensitive, so prefixes can over-match. The outbox re-filters in C#; this is follow-up material, not for this PR.

6. Test gaps worth closing, cheap on the existing fixture:

   • a plain upgrade from 20007 with existing rows and no hand-made objects
   • a legacy KeyValuePairs table staying untouched
   • a Down/Up round trip
   • a column-guard-only no-op (only the table guard is exercised on its own today)

7. Ordering with #258:

   • The test csproj conflicts trivially: #258 adds the FluentMigrator runners, Npgsql and Testcontainers; this PR adds Elsa. Keep both.
   • The StartsWith line conflicts, see B1.
   • Otherwise independent. Verifying 20008 on PostgreSQL needs #258, because before #258 the PostgreSQL migrations fail earlier (#255).

8. For the #260 delta: now that KeyValues exists, an atomic TryDeleteAsync (DELETE ... WHERE Id = @key with a rows-affected check) is straightforward. Please give it migration-built-DB tests as well. Nothing here makes that delta harder, but B1 needs to be fixed first, or the outbox/heartbeat callers that #260 is meant to help still fail on the prefix query.

Main port

There is no port to main yet. main has the same two bugs: no migration creates KeyValues, and there is no SerializedOptions column. It also has the same StartsWith defect. Please open a port PR to main with the next free runtime migration number on main and the B1 fix, so 3.9 and main don't diverge.

What was not run

• SQL Server: not executed, since no container runtime was available. Its DDL comes from FluentMigrator's SqlServer2016Generator, and the dbo-scoped guard behaviour comes from reading FluentMigrator's source. Both are inferred, not observed.
• PostgreSQL: verified on a local PostgreSQL 17 with #258 merged into this PR. Before #258, the Dapper PostgreSQL path is broken anyway.
• DispatchWorkflow resume: a variant driven through the background command processor failed with WorkflowDefinitionNotFoundException. That comes from the background tenant context (a known separate issue family), not from this PR; the ExecuteWorkflow/bookmark-queue path is what this PR fixes, and it works.

[cursor]

Addressed CR R1 B1 in 5736349.

B1 — prefix lookups. ApplyFilter now applies either StartsWith or exact Is, then In (same shape as the EF store). StartsWith binds @{field}StartsWith so it matches the SQL and does not collide with Is(@{field}). The identifier is still unquoted on this branch; if #258 lands first, keep its QuoteIdent and this parameter name together.

On a fresh migration-built SQLite DB:

• FindManyAsync(Key = "app:", StartsWith = true) returns app:1 / app:2 and excludes other:1; exact Find("app:1") still works.
• IWorkflowDispatchOutboxStore Save / FindMany returns the saved item.

Those two tests failed first with Must add values for the following parameters: @TenantId, @Id, @SearchTermLike, then passed.

Down(). 20008 Down() is now a no-op, with a comment: rolling back must not drop a hand-created KeyValues table or SerializedOptions column.

[sfmskywalker]

Code Review, Round 2/4: REQUEST_CHANGES + HIGH @ 5736349

Scope: this round covers only the R1 fix commit, 360f2fa..5736349. The branch has since moved to d8b3285 with two #260 commits: 340114d pins ElsaVersion and d8b3285 adds TryDeleteAsync. Those are not reviewed here. They are the next round's delta. The one exception is a PostgreSQL heads-up below, because it affects the same merge.

Summary: The R1 blocker is fixed, and the fix is correct and minimal:

• Prefix lookups now match the EF store.
• The StartsWith binding is fixed.
• Each cause has its own test, and each test fails when its cause is reverted.
• Down() is now a documented no-op.

I would approve this delta as it stands. The change I'm requesting is not in this code. #258 merged into release/3.9.0 at 10:59 CEST, so the branch now conflicts with its base (GitHub shows it as not mergeable, dirty). Because of the conflict, ubuntu-latest has not run on the new head.

The conflict needs a particular resolution. The obvious "keep one side" choices are both wrong, and one of them breaks PostgreSQL without failing any test in the repo. The recipe below is verified, and I expect to approve the resolved branch quickly.

---

Required: merge release/3.9.0 (conflicts with #258)

There are two textual conflicts and one semantic one:

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

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

The parameter is @{field}StartsWith (so @IdStartsWith), not @SearchTermLike. Here is what each wrong resolution does:

• Taking #258's side brings back the unbound @SearchTermLike. This PR's tests catch that.
• Taking this PR's side drops the quoting. On PostgreSQL, every prefix query then fails with 42703: column "id" does not exist, and no test in the repo catches it. I tried it on the merged tree: the PR's tests and #258's tests (including its PG end-to-end test) all pass, and only my local PG harness fails.

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

3. Semantic conflict. #258's NonPgQuerySqlSnapshotTests pins ["starts-with"] = "and Name like @SearchTermLike". That value has to become "and Name like @NameStartsWith", or the SQLite and SQL Server snapshots fail. This is the intended SQL change and the only non-PG SQL change in this PR.

4. Recommended. Add one line to PostgreSqlDialectTests.QueryBuilder_QuotesInlinedIdentifiers, so that dropping the quoting fails a test:

Assert.Contains("and \"Name\" like @NameStartsWith", queries["starts-with"], StringComparison.Ordinal);

With exactly items 1–3 applied on top of release/3.9.0 @ 9d9049e:

• Elsa.Persistence.Dapper.UnitTests: 53/53. This includes #258's PG end-to-end test, run against a local PostgreSQL 17 instead of Testcontainers.
• Elsa.Dapper.UnitTests: 17/17.
• My SQLite and PG harness: green.

Heads-up for the #260 delta (not reviewed, not part of this verdict). The same merge breaks d8b3285 on PostgreSQL:

• The new IsNullOrEmpty helper appends {field} unquoted.
• Store.ApplyTenantFilter now uses that helper for every default-tenant query.
• So after the merge, every default-tenant Dapper read or delete on PG fails with 42703: column "tenantid" does not exist.

I confirmed this by merging d8b3285 with the base: #258's own PG end-to-end test fails with that error. The fix is to use query.QuoteIdent(field) there as well. I'll cover it properly in Round 3.

---

Verified

1. Prefix semantics match the EF store. EF's KeyValueFilter.Apply (core release/3.9.0 and main) works like this: when Key != null, it filters on StartsWith ? Id.StartsWith(Key) : Id == Key, then on Keys.Contains(Id). ApplyFilter now does the same: prefix or exact, then In. Results on fresh, migration-built SQLite DBs:

• Prefix app: returns app:a and app:b, and not other or apple.
• Exact app: returns null. Exact app:a returns its value.
• Prefix combined with Keys = [app:b, other] returns app:b, the intersection, as in EF.
• StartsWith = true with Key = null returns all rows, as in EF, where there's no key filter.
• Tenant filtering is intact. Store applies the tenant filter before calling ApplyFilter. A row saved under tenant t1 is invisible to the default tenant's prefix query and returned for t1's.
• On PG17 (merged tree), prefix app: returns app:a and app:b, and not APP:upper, because PG's LIKE is case-sensitive.

2. SQL for other filters is unchanged. With StartsWith = false, the emitted SQL is byte-identical to before: Is then In. Only the StartsWith = true path changes, and that path never worked.

3. Other callers of the helper. DapperKeyValueStore is the only caller of StartsWith in the repo.

• The workflow definition and instance stores use WorkflowDefinitionSearchTerm and AndWorkflowInstanceSearchTerm. Those bind their own @SearchTerm/@SearchTermLike and are untouched. #258's snapshots for them still pass after the merge.
• The helper is public. For an external caller, the old version either threw (unbound @SearchTermLike) or, when combined with a search-term helper, silently used the search term's value. So the rename is purely a fix.

4. 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 the following parameters: @TenantId, @SearchTermLike); binding unit test

The outbox test goes through FindManyAsync(), so it exercises the index, recovery and legacy prefix scans.

5. Down() as a no-op is safe and documented. A <remarks> explains why it leaves both objects in place. On SQLite and PG17:

• After MigrateDown(20007), KeyValues, SerializedOptions and their rows remain, and VersionInfo no longer lists 20008.
• A second MigrateUp records 20008 again as a guarded no-op.
• A hand-created, populated KeyValues table survives Up followed by Down. R1 note 1 is resolved.

6. Runtime harness on fresh migrated SQLite, re-run at 5736349: 9/9. Every R1 scenario still passes:

• 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.
• ExecuteWorkflow with WaitForCompletion resumes the parent.
• A queued stimulus resumes its workflow to Finished.
• SerializedOptions round-trips.
• A plain upgrade from 20007 keeps existing rows and leaves a legacy KeyValuePairs table untouched.

New in this round:

• Outbox: Save o3, o1, o2, then:
  • FindManyAsync() returns o1, o2, o3 in CreatedAt order.
  • FindManyAsync(1) returns o1.
  • After DeleteAsync(o1), it returns o2, o3.
• Column guard on its own: with SerializedOptions added by hand and a row in the table, Up creates KeyValues and leaves the column and row untouched.

7. PostgreSQL 17 (local cluster). I used the merged tree, because the Dapper PG path only works with #258:

• A fresh migrate applies 10001–10005, 20001–20004, 20006–20008 and 30001–30004.
• It creates "KeyValues" ("Id" text PK, "TenantId" text, "Value" text) and a nullable text "SerializedOptions".
• KV store: a 100 KB upsert, FindMany(Keys), FindMany(StartsWith) and Delete all work.
• Outbox: Save, FindMany, FindMany(1) and Delete all work.
• Bookmark queue: a 100 KB Options payload round-trips.
• Quiescence: an AcrossReactivations pause survives a restart.
• Migration Down/Up: behaves as in item 5.
• Guards: a quoted, hand-created table is detected (no-op). A lowercase unquoted one is not (R1 note 3).

8. CI at 5736349: all green. It ran against the old base, dc5b341, before #258 merged.

• ubuntu-latest:
  • Elsa.Persistence.Dapper.UnitTests: 27/27, up from 25 at R1, so the 2 new tests ran.
  • Elsa.Dapper.UnitTests: 17/17, up from 16, so the binding test ran.
• GitGuardian, CLA and submit-nuget pass.
• The NU1903 warning predates this PR.
• The new head d8b3285 has no ubuntu-latest run, because GitHub doesn't run pull_request workflows while a PR has conflicts.

---

Non-blocking notes

1. The binding test's last line is hard to read. Assert.DoesNotContain("Id", query.Parameters.ParameterNames.Where(name => name == "Id")) filters the list down to "Id" and then asserts "Id" isn't in it. It's correct, but it reads like a puzzle. Assert.DoesNotContain("Id", query.Parameters.ParameterNames) says the same thing plainly. Two other asserts are redundant: DoesNotContain("@SearchTermLike", sql) next to the exact Contains, and DoesNotContain(prefixed, x => x.Key == "other:1") after the exact Assert.Equal. The other new tests are clear AAA and deterministic.
2. Pin the no-op Down() with a test. It departs from the usual pattern on purpose. A ~10-line test (Up, write a row, Down(20007), assert the table, column and row survive, Up again) would stop someone from "fixing" the empty method and bringing back the data loss. The other R1 test gaps are still open: a plain upgrade with rows, a legacy KeyValuePairs left untouched, and the column guard on its own. I covered all of them locally and they pass, so this is cheap regression coverage, not a correctness concern.
3. The Dapper KV store ignores KeyValueFilter.Take and OrderByKey. This predates the PR. The outbox sets both on every prefix scan.
   • Results are still correct, because the outbox re-sorts by CreatedAt and truncates in memory. FindManyAsync(1) returns the oldest item, as shown above.
   • The cost: each poll reads every outbox row, and the follow-up Keys lookup expands into one parameter per index row. That will hit SQL Server's 2,100-parameter limit once roughly 2,100 items are pending.
   • For comparison: prefix + OrderByKey + Take = 1 returns 2 rows on Dapper, where EF returns 1.
   • Suggest a follow-up issue: ORDER BY Id plus dialect paging in DapperKeyValueStore when those options are set.
4. Value is nullable in the table while the record treats it as required (R1 note 2). Unchanged, and fine to leave, because the store never writes null.
5. R1 notes 3–5 still apply: shape-blind guards, the Id length, and LIKE wildcards and case. SQLite's LIKE is case-insensitive where PG's is not. These belong in the release note.

What was not run

• SQL Server: no container runtime is available here. The change is dialect-neutral: a renamed parameter and different control flow.
• The #260 commits after 5736349: not reviewed, apart from the PG heads-up above.

[cursor]

Re: Round 2 — the required release/3.9.0 rebase is done, with the recipe you specified.

Conflicts (items 1–3) at e996fe1 on top of 9d9049e (#258):

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

Recommended item 4 is on the new tip 8b59c59: PostgreSqlDialectTests.QueryBuilder_QuotesInlinedIdentifiers asserts and "Name" like @NameStartsWith, so dropping the quoting fails in-repo.

#260 PG heads-up: IsNullOrEmpty already uses query.QuoteIdent(field). Default-tenant reads/deletes on PG emit "TenantId". There are also PG Testcontainers TryDelete tests (race / not-found / NULL TenantId adoption).

CI at e996fe1 was already green (ubuntu-latest 5m21s: Persistence.Dapper 61/61 including PG, Mongo 67/67 in 44s). 8b59c59 is the QuoteIdent assert only; pr.yml is running on it now.

Non-blocking notes 1–5 acknowledged; left for a follow-up so this stays the schema / prefix / atomic-TryDelete cut.

[sfmskywalker]

Code Review, Round 3/4: APPROVE + HIGH @ 8b59c59

Scope: the delta since the Round 2 head 57363491: the rebase onto release/3.9.0 @ 9d9049e (#258), plus the #260 atomic TryDeleteAsync work (93c301e..8b59c59). Everything was re-verified on a fresh migration-built SQLite DB, local PostgreSQL 17 and MongoDB 7.0, with independent harness tests plus revert-the-fix experiments.

Merge condition (blocks a merge-commit merge, not the code): commit 93c301e contains unresolved conflict markers (<<<<<<< HEAD … >>>>>>> 360f2fa) in test/modules/persistence/Elsa.Persistence.Dapper.UnitTests/Elsa.Persistence.Dapper.UnitTests.csproj. They are only resolved later in e51ea14. With a merge commit, an unbuildable commit lands on release/3.9.0 and breaks git bisect. The repo allows squash merges, so squash-merge this PR. Otherwise, rewrite 93c301e before merging. The squash also folds away the noise commits: 27e9518 is an empty retrigger, 7c51e4a only touches a comment, and e996fe1 re-adds QuoteIdent that an earlier commit in the range dropped.

1. Merge resolution: #258 survives intact ✅

• Store.cs differs from base only in ApplyTenantFilter. The Store.DeleteAsync body is byte-identical to base (same hash).
• These files are untouched by the PR: the migration database lists, ISqlDialect, the dialects, the Runtime Initial and V3_3 migrations, Management, DapperPostgreSqlMigrationTests and DapperPostgresProcessorNameTests.
• Quoting is preserved:
  • QuoteIdent call sites rise by exactly one (the new IsNullOrEmpty).
  • StartsWith is now and {QuoteIdent(field)} like @{field}StartsWith.
  • The snapshots were updated consistently: the default dialect gives and Name like @NameStartsWith, PostgreSQL gives and "Name" like @NameStartsWith.
  • A new PG unit test pins and "Id" like @IdStartsWith.
• The #258 PG E2E ("A fresh PostgreSQL DB migrates, persists records, and runs a WriteLine workflow to completion through Dapper stores") passes on this head against local PG17.

2. #260 atomic TryDeleteAsync ✅

• Dapper: TryDeleteAsync is await store.DeleteAsync(q => q.Is(Id, key)) > 0.

  • It runs one conditional DELETE and decides on rows affected.
  • The tenant filter is applied inside Store.DeleteAsync, so the statement is tenant-scoped and quoted on PostgreSQL.
  • There is no read-then-delete window.

• Mongo: it calls DeleteOneAsync(ApplyTenantScope(Eq(Key, key))) and returns DeletedCount > 0. Single-document delete is atomic on the server, and the tenant scope is the same strict scope the other deletes use.

• The tests prove one winner, and fail when the fix is reverted. I reverted each fix in turn and ran only the PR's own tests:

  Revert	Failing tests
  Dapper atomic delete → find-then-delete	#260 PG: two Dapper stores racing TryDeleteAsync: exactly one returns true
  Mongo DeleteOne/DeletedCount → find-then-delete	#260: two Mongo stores racing TryDeleteAsync: exactly one returns true

  Earlier runs on this same head failed 6 of 6 times for each revert. The barrier tests ("the default TryDeleteAsync lets both racers win") also show that the unfixed default path really does produce two winners.

• An independent PostgreSQL row-lock check: I held a FOR UPDATE lock on the row, started two TryDeleteAsync calls (both blocked on the lock), then released it. Exactly one returned true in each of 3 rounds. With the fix reverted, both return true.

• Caveat: the SQLite race test still passes with the atomic fix reverted, because SQLite serializes writers. Atomicity is proven by the PG and Mongo tests, not the SQLite one (see §6).

3. Default-tenant NULL vs empty, and IsNullOrEmpty quoting ✅

• Behaviour matches EF. For the default (or absent) tenant context, Dapper now filters (TenantId is null or TenantId = ''). For a named tenant it filters TenantId = @tenant. Core EF's SetTenantIdFilter behaves the same way for these cases.

• Harness results on fresh migration-built SQLite and PG17 (identical):

  • Tenant.Default and "no tenant context" both see the NULL and '' rows, in both KV and bookmark-queue;
  • tenant t1 sees only t1;
  • default-tenant writes stamp '';
  • a * row is not visible to, and can't be TryDeleted by, the default tenant.

  The only remaining difference from EF is * visibility, the known #245 remainder that was scoped to 3.10.

• There is no index on TenantId, so the OR doesn't change any query plan.

• Quoting: IsNullOrEmpty uses QuoteIdent(field).

  • The unit test asserts the default dialect's unquoted form only.
  • The quoted PG form is pinned by the integration tests. Reverting the quoting fails 4 PG tests: the NULL-tenant TryDelete, not-found, the race, and the #258 PG E2E (42703: column "tenantid" does not exist).
  • #245 / #260 PG: a NULL TenantId legacy row is found and TryDeleted by the default tenant ran (not skipped) in CI and passes locally on PG17.

• Reverting the tenant change fails both NULL-tenant tests, on SQLite and on PG.

• Reverting the StartsWith quoting fails StartsWith quotes the identifier on PostgreSQL… and Shared query-builder inlines go through QuoteIdentifier on PostgreSQL.

4. Core pin ✅

ElsaVersion is 3.9.0-preview.5726, built from core fa68369a. It ships IKeyValueStore.TryDeleteAsync and the host-pause/legacy-adoption code that uses it, so the fix is live on this branch.

Locally on this head, these runtime checks against a migration-built DB all pass:

• quiescence persists across restart;
• a legacy NULL-tenant pause row is adopted on startup and removed;
• event publish, WaitForCompletion and a queued stimulus work;
• an upgrade from 20007 with outbox, prefix and Down works.

Moving to a newer 3.9 preview is not required for this PR.

5. CI ✅

The pr workflow (run 36403248563, pull_request event, success, Sep 28 11:24 CEST) ran Restore, Compile, Pack (81 packages) and Test: 494 passed, 2 skipped. Both skips are the existing Slack and Azure Service Bus tests.

• Elsa.Persistence.Dapper.UnitTests: 61/61, 0 skipped. Its 12 s duration fits the PG Testcontainers tests actually running.
• Elsa.Dapper.UnitTests: 19/19.
• Elsa.MongoDb.UnitTests: 67/67, 0 skipped.

pr.yml triggers on pull_request to main/release/* with paths **/*, so the run was not short-circuited. The local counts reconcile with CI.

6. Maintainability (HIGH bar)

All of these are non-blocking and can be follow-ups.

1. SQLite race test: add a comment that it can't detect a non-atomic implementation (SQLite serializes writers), or drop the "exactly one winner" claim from its name. The PG and Mongo tests carry the proof.
2. Stale comment: the test doc comment "There is no PostgreSQL Testcontainers setup on this 3.9 branch" is now false.
3. PG IsNullOrEmpty assertion: a one-line PostgreSqlDialectTests assertion of and ("TenantId" is null or "TenantId" = '') would pin the quoted form directly, instead of only through integration tests.
4. Duplicated helper: TestTenantAccessor is copied across several test files; one shared helper would do.
5. Mongo tenant test: add a named-tenant-cannot-TryDelete-a-NULL-row test for Mongo, to mirror the Dapper one.
6. Carried over:
   • the no-op Down() pinning test;
   • Dapper ignoring Take/OrderByKey (needs a follow-up issue);
   • * tenant visibility (#245 remainder, 3.10).

Verdict

APPROVE + HIGH, on condition that it is squash-merged (or 93c301e is rewritten first). The code at this head is correct, #258 is intact, and the #260 fix is atomic and tenant-scoped on both providers. The tests demonstrably fail when the fix is reverted, and CI is genuinely green with the PG tests executed.