Add support to Oracle Database for Inbox/Outbox/Locking

[lillo42]

Description

Adding support for Oracle Database for Inbox/Outbox/Locking

Related Issues

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the Contributing Guide
• I have checked the documentation for relevant guidance
• I have added/updated XML documentation for any public API changes
• I have added/updated tests as appropriate
• My changes follow the existing code style and conventions

Additional Notes

[claude]

Code Review — Add support for Oracle (#4232)

Thanks for this — a large, well-structured contribution. The XML documentation is excellent, the NETFRAMEWORK vs. modern await using handling is consistent, and the BoxProvisioning.Oracle advisory-lock code is notably careful (good comments on why RollbackAsync must not throw, the null-_lockResource short-circuit, and correct DBMS_LOCK.REQUEST return-code handling). Feedback below, ordered by severity.

Bugs

1. OracleLockingProvider treats a lock timeout as a successful acquisition — src/Paramore.Brighter.Locking.Oracle/OracleLockingProvider.cs:82

var requestLock = await RequestLock(connection, lockHandler, 1);
if (requestLock > 1)          // treats 0 (success) AND 1 (timeout) as "granted"
{
    Log.LockNotGranted(_logger, resource, requestLock);
    return null;
}

DBMS_LOCK.REQUEST returns 0 = success, 1 = timeout, 2 = deadlock, 3 = parameter error, 4 = already held (same session), 5 = illegal handle. With > 1, a timeout (1) — which means the lock was not acquired — falls through, the connection is added to _connections, and the handle is returned as if the lock were held. This defeats the purpose of a distributed lock: two different holders can both believe they own the same resource.

It also contradicts the base contract test When_Trying_To_Obtain_Same_Lock_With_Different_Instances_It_Should_Fail_Second_Time — the second instance times out (return code 1) and this code returns a non-null handle, so that test should be failing. Note your own OracleAdvisoryLock.AcquireAsync (in BoxProvisioning.Oracle) handles the same codes correctly (0/4 = success, 1 = timeout -> throw).

Fix: if (requestLock != 0) (each ObtainLockAsync uses a fresh session, so code 4 cannot legitimately occur here).

2. OracleOutboxBuilder.GetExistsQuery passes owner/table in the wrong order — src/Paramore.Brighter.Outbox.Oracle/OracleOutboxBuilder.cs

The template is WHERE OWNER = '{0}' AND TABLE_NAME = '{1}', but the method is GetExistsQuery(string outboxTable, string ownerName) and calls string.Format(OutboxExistsSql, outboxTable, ownerName) — so {0} (OWNER) gets the table name and {1} (TABLE_NAME) gets the owner. The query can never match. Compare OracleInboxBuilder.GetExistsQuery(ownerName, inboxTableName), which is ordered correctly. Appears to have no callers yet, so latent, but worth fixing before it is wired up.

Please verify

3. Trailing semicolons in the builder DDL / exists-query templates — OracleOutboxBuilder.cs, OracleInboxBuilder.cs

The CREATE TABLE (...); DDL strings and the *ExistsQuery templates end with a trailing ;. Oracle.ManagedDataAccess typically raises ORA-00911: invalid character for a trailing ; on a single, non-PL/SQL statement executed via OracleCommand.ExecuteNonQuery. These GetDDL(...) results are executed directly by both the test CreateStore() and OracleBoxMigrationRunner.ExecuteDdlAsync. (The history-table DDL correctly avoids this by wrapping in EXECUTE IMMEDIATE '...' with no trailing ;.) Please confirm the Oracle CI test job is green — if it is red, this is a likely cause.

Minor

• Connection leak under a race in ObtainLockAsync (OracleLockingProvider.cs:63,88): the _connections.ContainsKey(resource) check and the later _connections.TryAdd(resource, connection) are not atomic, and the TryAdd return value is ignored. If two concurrent calls race for the same resource, the losing TryAdd returns false and that opened OracleConnection is never closed/disposed. Consider checking the TryAdd result and releasing the connection on failure.
• Unused constant MIGRATION_HISTORY_TABLE in OracleBoxMigrationRunner.cs:45 — the DDL/inserts use the literal table name instead.
• InvalidEnumArgumentException is used for a null/empty string guard in OracleInboxBuilder.GetExistsQuery — ArgumentException would fit better.

Test coverage

Good breadth (inbox text/binary/json, outbox text/binary sync+async, locking, box provisioning) with CI wired up via gvenzl/oracle-free. Bug #1 suggests the contention scenarios may not be exercising as intended — worth confirming those assertions actually run against a live Oracle instance rather than being skipped.

Automated review — please treat as suggestions and verify against the live Oracle CI run where noted.

[claude]

Claude Code Review — PR #4232: Oracle support for Inbox/Outbox/Locking

Thanks for this substantial contribution! The overall structure closely mirrors the existing relational backends, the XML docs are excellent, and the DBMS_LOCK/DUAL UNION ALL/OFFSET…FETCH choices show real Oracle-specific care. I focused on correctness, since the integration tests only run against a fresh database and won't exercise several of the paths below.

🔴 High — Migration history table name mismatch (silently breaks re-provisioning)

OracleBoxMigrationRunner creates and writes to BRIGHTER_MIGRATION_HISTORY:

• OracleBoxMigrationRunner.cs:120 — CREATE TABLE BRIGHTER_MIGRATION_HISTORY (...)
• OracleBoxMigrationRunner.cs:243 — INSERT INTO BRIGHTER_MIGRATION_HISTORY ...

but OracleBoxDetectionHelper reads a different table, __BRIGHTERMIGRATIONHISTORY:

• OracleBoxDetectionHelper.cs:90 — DoesTableExistAsync(connection, "__BRIGHTERMIGRATIONHISTORY", ...)
• OracleBoxDetectionHelper.cs:99 — SELECT COUNT(1) FROM __BRIGHTERMIGRATIONHISTORY ...
• OracleBoxDetectionHelper.cs:126 — SELECT COALESCE(MAX(MigrationVersion),0) FROM __BRIGHTERMIGRATIONHISTORY ...

Consequences:

• DoesHistoryExistAsync always returns false (it checks for a table that is never created), so provisioning never recognises an existing install → a second ProvisionAsync re-enters the fresh path and re-runs CREATE TABLE <box> → ORA-00955 (name already used).
• RunNormalPathAsync → GetMaxVersionAsync selects from __BRIGHTERMIGRATIONHISTORY → ORA-00942 (table or view does not exist) whenever a real migration/bootstrap path is taken.

The fresh-database tests pass only because on a truly empty schema "history doesn't exist" happens to be the correct answer. Please pick one name (the test helper at OracleBoxProvisioningTestHelper.cs:35 also hard-codes BRIGHTER_MIGRATION_HISTORY) and use it in all three places, and consider adding an idempotency test (ProvisionAsync twice) to lock this down. Note the unused constant MIGRATION_HISTORY_TABLE at OracleBoxMigrationRunner.cs:45 — routing all names through it would prevent this class of drift.

🔴 High — Connection leak in OracleLockingProvider.ObtainLockAsync

OracleLockingProvider.cs:61-108: the connection is opened up front, but the two early return null paths never close/dispose it and never add it to _connections:

• :75-78 — lock handle empty → return null;
• :82-86 — requestLock > 0 (lock not granted) → return null;

"Lock not granted" is a normal, expected outcome under contention for a distributed lock, so every contended acquisition leaks a pooled OracleConnection (and holds a server session) until finalization — this will exhaust the connection pool in a busy sweeper/scheduler. Only the catch block disposes. Recommend a finally that disposes the connection unless it was successfully handed to _connections (e.g. track a tracked bool and dispose when !tracked).

Related: _connections.TryAdd(resource, connection) at :88 ignores its return value. Combined with the ContainsKey check at :63 this is a check-then-act race — if another thread wins, TryAdd returns false, the connection is neither tracked nor disposed, yet the method still returns the handle. Same leak.

🟠 Medium — Trailing semicolons in builder DDL

OracleOutboxBuilder.cs (:59, :89) and OracleInboxBuilder.cs (:43, :57, :72) end each CREATE TABLE with ;. These strings become the migration UpScript/FreshInstallDdl and are executed directly via ExecuteNonQueryAsync in OracleBoxMigrationRunner.ExecuteDdlAsync (:230-234) — not wrapped in a PL/SQL block. ODP.NET typically rejects a trailing ; on a single statement with ORA-00911: invalid character (unlike Npgsql/SqlClient, which is why the other backends get away with it). Worth confirming against the Oracle CI job; if it's currently green I'd double-check the driver/version behaviour, otherwise strip the trailing ;.

🟡 Low — OracleOutboxBuilder.GetExistsQuery arguments swapped + SQL injection surface

OracleOutboxBuilder.cs:106-109: signature is GetExistsQuery(string outboxTable, string ownerName) but the template (:96-97) is WHERE OWNER = '{0}' AND TABLE_NAME = '{1}', so outboxTable lands in OWNER and ownerName in TABLE_NAME — reversed relative to the inbox equivalent (OracleInboxBuilder.cs:120). It also interpolates directly into SQL. This appears to be dead code (no callers found), but it's a latent bug if wired up later — either fix the ordering or remove it.

🟡 Low — Misc

• OracleTransactionProvider.GetConnection/GetConnectionAsync XML docs (:73, :89) say "This is not a shared connection; you should manage its lifetime" — but this provider deliberately shares Connection (??=). Looks copy-pasted from OracleConnectionProvider; the doc contract is misleading.
• RequestLock/ReleaseLockByHandleAsync use Convert.ToInt32(resultParam.Value?.ToString()) (OracleAdvisoryLock.cs:167,192; OracleLockingProvider.cs:213). If the OUT value were ever NULL, Convert.ToInt32(null) returns 0, which reads as "success/granted." DBMS_LOCK always returns a code so this is theoretical, but round-tripping a number through .ToString() and back is fragile — prefer reading the numeric value directly.

✅ Nice work

• Correct handling of Oracle's lack of multi-row VALUES via SELECT … FROM DUAL UNION ALL (OracleOutbox.GenerateBulkInsert).
• BindByName = true set consistently, and the @→: parameter remapping is clean.
• Thorough, honest XML docs (e.g. the transactionless-DDL rationale in OracleProvisioningUnitOfWork).
• Parameterised detection queries with UPPER() folding and OrdinalIgnoreCase column sets.

The history-table naming (🔴) and the locking connection leak (🔴) are the two I'd consider blocking. Happy to help verify once addressed.

Reviewed against the repo's CLAUDE.md conventions. Automated review — please treat as advisory.