REP-6826 Fix memory corruption from pool usage without cloning (#175)

PR #169 made document comparison use a memory pool. That changeset neglected to update uses of bson.Raw.Lookup(), which does not clone the field’s raw value. Thus, when a mismatched document was “put back” into memory, another thread overwrote a part of it.

Thus we got mismatch documents (i.e., in verifier metadata) where the same document ID showed up twice. This also, critically, made the verifier skip necessary rechecks because a later mismatch overwrote the buffer that contains the document ID meant for recheck.

This changeset fixes that problem by always cloning the document ID as part of reporting a mismatch. It also beefs up testing to ensure that the expected document IDs are reported in the log and in the rechecks.

This also makes the field-order-difference mismatch text more concise by removing a redundant document ID.

Author: FGasper

internal/verifier/compare.go

@@ -721,13 +721,16 @@ func transformPipelineForToHashedIndexKey(
 func (verifier *Verifier) compareOneDocument(srcClientDoc, dstClientDoc bson.Raw, namespace string) ([]VerificationResult, error) {
 	match := bytes.Equal(srcClientDoc, dstClientDoc)
 	if match {
+		// Happy path! The documents binary-match.
 		return nil, nil
 	}
 
+	docID := getDocIdFromComparison(verifier.docCompareMethod, srcClientDoc)
+
 	if verifier.docCompareMethod == DocCompareToHashedIndexKey {
 		// With hash comparison, mismatches are opaque.
 		return []VerificationResult{{
-			ID:        getDocIdFromComparison(verifier.docCompareMethod, srcClientDoc),
+			ID:        docID,
 			Details:   Mismatch,
 			Cluster:   ClusterTarget,
 			NameSpace: namespace,
@@ -746,13 +749,15 @@ func (verifier *Verifier) compareOneDocument(srcClientDoc, dstClientDoc bson.Raw
 
 		// If we're respecting field order we have just done a binary compare so we have fields in different order.
 		return []VerificationResult{{
-			ID:        srcClientDoc.Lookup("_id"),
-			Details:   Mismatch + fmt.Sprintf(" : Document %s has fields in different order", srcClientDoc.Lookup("_id")),
+			ID:        docID,
+			Details:   Mismatch + " : only field order differs",
 			Cluster:   ClusterTarget,
 			NameSpace: namespace,
 			dataSize:  int32(dataSize),
 		}}, nil
 	}
-	results := mismatchResultsToVerificationResults(mismatch, srcClientDoc, dstClientDoc, namespace, srcClientDoc.Lookup("_id"), "" /* fieldPrefix */)
+
+	results := mismatchResultsToVerificationResults(mismatch, srcClientDoc, dstClientDoc, namespace, docID, "" /* fieldPrefix */)
+
 	return results, nil
 }

internal/verifier/migration_verifier_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/10gen/migration-verifier/internal/testutil"
 	"github.com/10gen/migration-verifier/internal/types"
 	"github.com/10gen/migration-verifier/internal/util"
+	"github.com/10gen/migration-verifier/internal/verifier/recheck"
 	"github.com/10gen/migration-verifier/mbson"
 	"github.com/10gen/migration-verifier/mslices"
 	"github.com/10gen/migration-verifier/option"
@@ -1739,6 +1740,14 @@ func (suite *IntegrationTestSuite) TestVerifierDocMismatches() {
 	_, _, err = verifier.reportDocumentMismatches(ctx, builder)
 	suite.Require().NoError(err)
 
+	for _, specimen := range mslices.Of("100000", "100001") {
+		suite.Assert().Contains(
+			builder.String(),
+			specimen,
+			"summary should show all mismatched-content doc IDs",
+		)
+	}
+
 	suite.Assert().Contains(
 		builder.String(),
 		"100009",
@@ -1751,6 +1760,12 @@ func (suite *IntegrationTestSuite) TestVerifierDocMismatches() {
 		"summary should show the # of missing docs shown",
 	)
 
+	suite.Assert().Contains(
+		builder.String(),
+		" 2 ",
+		"summary should show the total # of content-mismatched documents",
+	)
+
 	suite.Assert().Contains(
 		builder.String(),
 		" 18 ",
@@ -1762,6 +1777,22 @@ func (suite *IntegrationTestSuite) TestVerifierDocMismatches() {
 		"100019",
 		"summary should NOT show a late mismatch",
 	)
+
+	rechecks := suite.fetchRecheckDocs(ctx, verifier)
+	recheckDocIDs := lo.Map(
+		rechecks,
+		func(r recheck.Doc, _ int) int {
+			num, err := mbson.CastRawValue[int32](r.PrimaryKey.DocumentID)
+			suite.Require().NoError(err)
+			return int(num)
+		},
+	)
+
+	suite.Assert().ElementsMatch(
+		lo.RangeFrom(100000, 20),
+		recheckDocIDs,
+		"all docs should be enqueued for recheck",
+	)
 }
 
 func (suite *IntegrationTestSuite) TestVerifierCompareIndexSpecs() {