feat(mongodb): make transaction read concern configurable - #314
Conversation
The MongoDB storage backend hardcodes readConcern: snapshot on every multi-document transaction (conditional writes, TransactWriteItems, TransactGetItems, idempotency tokens). Real MongoDB (7.0+) supports this, but some MongoDB-wire-compatible servers do not. Concretely, DocumentDB (github.com/documentdb/documentdb) rejects transactions started with readConcern: snapshot with 'Error code 115 (CommandNotSupported)', which made the mongodb backend unusable against it beyond CreateTable. Add storage.mongodb.transaction_read_concern (default: "snapshot", preserving current behavior) so operators can set "majority" or "local" to run against backends that don't implement snapshot reads, trading off snapshot isolation for compatibility. Validated end-to-end against DocumentDB 0.116.0 (PutItem/GetItem/CreateTable/DeleteTable all succeed with transaction_read_concern = "majority"). Also widens the StorageConfig trait-object bound used by ServerComponentsFactory from an elided reference lifetime to 'static, since StorageConfig::as_any()'s downcast (already documented as intended for backend-specific settings like this one) requires it -- without it, Any::downcast_ref cannot be proven sound against an unannotated &dyn StorageConfig. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1de8a53e-4145-4deb-8d82-294a4879046e Signed-off-by: German <geeichbe@microsoft.com>
Updates the MongoDB backend design doc (13-storage-mongodb.md) to reflect that transaction read concern is now configurable via storage.mongodb.transaction_read_concern, rather than unconditionally snapshot. Left the RFC (docs/rfcs/0000-mongodb-backend.md) as a point-in-time design record, matching this repo's convention for RFCs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1de8a53e-4145-4deb-8d82-294a4879046e Signed-off-by: German <geeichbe@microsoft.com>
LeeroyHannigan
left a comment
There was a problem hiding this comment.
Thanks for this, the plumbing is thorough: all five transaction sites covered, catalog reads untouched, and the 'static widening checks out. A few things before it lands:
| "snapshot" => Ok(mongodb::options::ReadConcern::snapshot()), | ||
| "majority" => Ok(mongodb::options::ReadConcern::majority()), | ||
| "local" => Ok(mongodb::options::ReadConcern::local()), | ||
| "linearizable" => Ok(mongodb::options::ReadConcern::linearizable()), |
There was a problem hiding this comment.
Blocking linearizable and available pass startup validation here but MongoDB rejects both inside multi-document transactions, so an operator who sets them gets exactly the opaque runtime failure this gate exists to prevent. Shrinking the allowlist to snapshot | majority | local (or rejecting the two with a message saying they're invalid in transactions) keeps the fail-at-startup promise.
There was a problem hiding this comment.
Fixed in 91af441. Startup validation now accepts only snapshot, majority, and local; linearizable, available, and unknown values fail with a clear error listing the transaction-safe choices.
| /// change this if the target deployment's MongoDB-compatible server | ||
| /// does not support snapshot reads. | ||
| #[serde(default = "default_transaction_read_concern")] | ||
| pub transaction_read_concern: String, |
There was a problem hiding this comment.
Blocking There's no test coverage on this: nothing pins the parser (accept/reject/case) and nothing asserts the configured value actually reaches TransactionOptions, so a refactor reverting one of the five sites back to the literal stays green. A parser unit test plus one majority-mode transaction test would cover it.
There was a problem hiding this comment.
Fixed in 91af441. Added parser/default/unknown-field tests and centralized read/write TransactionOptions construction across all five transaction sites. The focused test asserts configured majority reaches both option shapes. The MongoDB crate suite passes (103 tests total).
| that request it with `CommandNotSupported` (error code 115); set this to | ||
| `"majority"` or `"local"` to run against such targets. Doing so weakens | ||
| isolation between concurrent transactions relative to `"snapshot"` — see | ||
| §5.1 for what that trades away. |
There was a problem hiding this comment.
Should-fix Off-default values change externally observable semantics (TransactGetItems stops being a point-in-time snapshot, and under local a condition can be evaluated against data that rolls back on failover), which belongs in differences-from-dynamodb.md. Also this pointer lands on session mechanics, and 5.1 still promises WriteConflict-not-stale-reads unconditionally, which stops being true off snapshot.
There was a problem hiding this comment.
Fixed in 91af441. Added the externally observable divergence to differences-from-dynamodb.md and revised the MongoDB design so point-in-time/WriteConflict guarantees are explicitly limited to snapshot; majority/local weakening and local failover rollback risk are documented.
| /// view of the data than they would under snapshot isolation. Only | ||
| /// change this if the target deployment's MongoDB-compatible server | ||
| /// does not support snapshot reads. | ||
| #[serde(default = "default_transaction_read_concern")] |
There was a problem hiding this comment.
Should-fix The postgres config has deny_unknown_fields but this one doesn't, so a typo'd transaction_read_concern key silently defaults to snapshot, which for a DocumentDB user is the exact failure this knob exists to avoid.
There was a problem hiding this comment.
Fixed in 91af441. MongoStorageConfig now uses #[serde(deny_unknown_fields)], with a regression test proving a typoed transaction_read_concern key fails configuration loading instead of falling back to snapshot.
Restrict transaction read concerns to transaction-safe values, centralize transaction options, reject configuration mistakes, and document weaker non-snapshot semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c9b74922-c628-425e-bf1b-484b223c52b5 Signed-off-by: German <geeichbe@microsoft.com>
…n-read-concern' into geeichbe/configurable-transaction-read-concern
|
Addressed all review feedback in 91af441 and pushed the branch after integrating current Validation completed:
The playground documentation and smoke test were also updated in the sibling operator working tree; those changes are not part of this PR. |
|
Should-fix (and the final ask, promise): a loud startup warning when a weaker read concern is configured. Thanks for the quick iterations on the earlier rounds, the fixes all landed clean. One last thing before this merges: when Heads up that the startup banner is about to change (#329 adds a wordmark), so here's roughly what I have in mind once that lands: Exact wording is yours to shape; the points that matter are naming the setting, naming what's weakened, being clear that writes stay atomic and durable, and saying how to get back to full semantics. |
…le-transaction-read-concern
Show backend startup warnings in the interactive banner and tracing logs when MongoDB transactions use majority or local read concern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c9b74922-c628-425e-bf1b-484b223c52b5 Signed-off-by: German <geeichbe@microsoft.com>
|
Addressed the final startup-warning request in
Affected crate tests and Clippy pass. |
Problem
The MongoDB storage backend hardcodes
readConcern: snapshoton every multi-document transaction it opens (conditional writes,TransactWriteItems,TransactGetItems, idempotency-token checks). Real MongoDB (7.0+) supports this and it's the strongest available isolation level, but some MongoDB-wire-compatible servers do not implement it.Concretely, DocumentDB (a Postgres-based MongoDB-API-compatible database) rejects transactions started with
readConcern: snapshotwithError code 115 (CommandNotSupported), which makes the mongodb backend unusable against it beyondCreateTable.Change
Adds
storage.mongodb.transaction_read_concern(default:"snapshot", preserving current behavior) so operators can set"majority"or"local"to run against backends that don't implement snapshot reads, trading off snapshot isolation for compatibility. Also updates the MongoDB backend design doc (docs/design/13-storage-mongodb.md) to describe the new setting.As a side effect, this widens
ServerComponentsFactory'sStorageConfigtrait-object bound from an elided reference lifetime to'static.StorageConfig::as_any()'s downcast is already documented as the intended mechanism for backend-specific settings like this one, butAny::downcast_refrequires the pointee to be provably'static— an unannotated&dyn StorageConfigdoesn't satisfy that, so the downcast wouldn't compile without this change. All existing callers already own'staticdata (Box<dyn StorageConfig>), so this is not a breaking change for callers.Validation
cargo build --workspace,cargo test(all touched crates pass),cargo clippy --workspace -- -D warningsclean.transaction_read_concern = "majority":CreateTable,PutItem,GetItem, andDeleteTableall succeeded via the DynamoDB API surface.transaction_read_concernunset) is unchanged — stillsnapshot.Trade-offs
Running with
majority/localinstead ofsnapshotweakens isolation between concurrent transactions on backends that use this setting — concurrent transactions may observe a less consistent view of the data than under true snapshot reads. This is called out in both the sample config and the design doc.