Dead code removal - #5695
Merged
Merged
Conversation
The requestId null checks in RetryingManager were added in 1.30 (fb328c1) for RetryBatch documents written by 1.29 and earlier, before retry operations existed. Every requestId reaching the manager today is non-null: the batch-reading paths in RetryProcessor and RetryDocumentManager read RequestId from documents that CreateBatch always writes, and CreateBatch is only ever fed a route-constrained value, a computed deterministic guid, or a constant. RetryDocumentManager already null-guarded before calling PreparedAdoptedBatch, and the EF persister models RequestId as required and non-nullable.
rbev
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two unrelated pieces of long dead code, both on the Raven/recoverability side.
FindClrTypeDatabaseConfiguration.FindClrType(audit) andEmbeddedDatabaseConfiguration.FindClrType(sharedServiceControl.RavenDB)were never assigned anywhere. Neither had a constructor parameter feeding it,
and no caller set the
initproperty, so the delegate was permanently null andall three consumers were no-op null checks.
Removed:
Sparrow.Jsonusingsif (configuration.FindClrType != null)blocks inRavenExternalPersistenceLifecycle(audit) andEmbeddedDatabase.ConnectRavenEmbeddedPersistenceLifecycleThe error instance had no copy of its own but shares
EmbeddedDatabaseandEmbeddedDatabaseConfiguration, so it is covered by the same change. A repowide grep for
FindClrTypeandBlittableJsonReaderObjectnow returns nothing.Legacy retry batch guards
RetryingManagerhad eight copies of:added in fb328c1 (Dec 2016, shipped in 1.30.0) for
RetryBatchdocumentswritten by 1.29 and earlier, before retry operations existed. For one to fire
today, a database would need an in flight batch document written by 1.29 that
survived to 6.19 and the RavenDB 3.5 to 5 to 6 migrations without ever being
staged and forwarded.
Every path into the manager supplies a non-null
requestId:StartRetryForSingleMessageuniqueMessageId{failedMessageId:required:minlength(1)}StartRetryForMessageSelectionDeterministicGuid.MakeId(...)RetryForAllMessages"All"RetryForEndpointendpoint!string.IsNullOrWhiteSpace(message.Endpoint)RetryForFailedQueueAddressqueueAddress{queueAddress:required:minlength(1)}RetryForFailureGroupgroupId{groupId:required:minlength(1)}The paths that read
RequestIdback out of persistence (Skip,Forwarding,ForwardedBatchinRetryProcessor,FailinRetryDocumentManager) only seevalues that
CreateBatchwrote from that same set. Two existing signals agree:RetryDocumentManager.RebuildRetryOperationStatealready wrapped its call in a!string.IsNullOrWhiteSpacecheck, and the EF persister declaresrequired string RequestIdwith.IsRequired()andnullable: false.Why a throw replaces them
Deleting the guards with nothing in their place would not surface a null.
MakeOperationIdinterpolates, sonullbecomes the key"<RetryType>/",GetOrCreatecreates anInMemoryRetrywith a nullRequestId, and every suchbatch collides on that one key while raising domain events with
RequestId = null. That is quieter and worse than the old no-op, soGetOrCreatenow callsArgumentException.ThrowIfNullOrWhiteSpace(requestId). All eight methods routethrough it, so one line replaces eight guards at the exact point a null would
otherwise become a bogus dictionary key.
IsOperationInProgressForandGetStatusForRetryOperationare unchanged: theyare read only
TryGetValuelookups that never had a guard.Behaviour change
Previously a legacy batch would still be forwarded, just without progress
tracking. It will now throw. Given the reachability analysis above this is the
intended trade: fail loudly on a state that cannot legitimately occur.