PCSM-399: Fix dropDatabase replication to Atlas Flex by chupe · Pull Request #286 · percona/percona-clustersync-mongodb

chupe · 2026-10-19T12:41:59Z

Problem

Replaying a dropDatabase event on an Atlas Flex target kills the run:

change replication: dropDatabase: list collection names: (AtlasError) can't get regex from filter doc not a regex

catalog.DropDatabase lists the target collections through mdb.ListCollectionNames, which filters with {name: {$not: {$regex: "^system\\."}}}. Plain mongod (6.0 through 8.0 in CI) accepts that, the Atlas Flex/M0 proxy does not. It rejects any regex under $not, the native BSON regex form included, which I verified on a Flex 8.0.34 cluster. Once the run is in failed, resume --from-failure replays the same event and fails again, so there is no way out without a new binary.

mdb.ListDatabaseNames has the same class of bug on the source side. The proxy accepts only a string or a regex for name, so the {$nin: ["admin","config","local"]} filter fails at clone start with Expected user provided "name" field in filter to be regex or string was bson.D instead. Nobody has hit that yet because Atlas has only been a target so far.

Solution

Send an empty filter to listCollections and listDatabases and exclude system.* collections and admin/config/local in Go. The result is the same list on every server that accepted the old filters, and the proxy has nothing left to reject (on Flex it hides system.views on its own anyway).

The integration tests record the outgoing command with a CommandMonitor and assert the filter is empty. That is the only assertion that can guard this on a mongod-backed CI, since mongod happily runs the old filters.

Verified against real Atlas clusters. On Flex 8.0.34 the old mdb helpers fail with the exact customer error and the patched ones return the expected names. On a dedicated M10 8.0.34 both old and new pass, so only the proxy tiers were affected.

chupe · 2026-10-19T13:34:26Z

90313c6 - https://psmdb.cd.percona.com/job/hetzner-pcsm-functional-tests/263/

chupe · 2026-10-19T14:36:59Z

/agentic_review

qodo-code-review · 2026-10-19T14:38:59Z

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🟠 Medium

1. ~~The wire test accepts missing filters~~ ✓ Resolved

Description

requireNoServerSideFilter records a missing filter as nil and then skips that entry instead of
verifying it is an empty document. A malformed or changed command that omits the filter entirely can
therefore satisfy the retry-aware assertion without proving the required empty-filter request shape.

Code

mdb/schema_integration_test.go[R65-76]

+	// At least, not exactly: a transient error retried by RunWithRetryVal adds
+	// Started events while the helper still succeeds.
+	require.GreaterOrEqual(t, len(r.filters[command]), minCalls, "%s calls", command)
+
+	for i, filter := range r.filters[command] {
+		if filter == nil {
+			continue
+		}
+
+		elems, err := filter.Elements()
+		require.NoError(t, err)
+		assert.Empty(t, elems, "%s #%d must not filter server-side", command, i+1)

Relevance

●●● Strong
Accepted test-hardening patterns require assertions to detect malformed or missing command data; nil
entries currently bypass the wire-shape check.
PR-#282
PR-#254

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The retry-tolerant helper appends nil when filter is absent, then continues past nil entries,
so the test can pass without checking the filter on those commands. This weakens the wire-level
protection for the empty-filter behavior required by the dropDatabase replication fix.

Replicate dropDatabase without collection-name filter errors
mdb/schema_integration_test.go[59-76]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The integration assertion skips recorded commands whose `filter` field is absent, so it does not verify that every list command carries the intended empty filter shape.

## Fix Focus Areas
- mdb/schema_integration_test.go[65-76]

## Recommended Fix
Treat a missing `filter` field as an assertion failure, or normalize it explicitly to an empty BSON document only if the MongoDB driver guarantees that representation is equivalent for this test. Continue allowing additional captured calls from retries while validating every captured command's filter.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~New lint suppressions lack reasons~~ ✓ Resolved

Description

The changed code adds //nolint:wrapcheck and //nolint:testpackage directives without
human-readable explanations on the same lines. These suppressions cover error returns and package
declarations in the new code, so a later maintainer cannot tell why each exception is considered
safe.

Code

mdb/schema.go[85]
+		return nil, err //nolint:wrapcheck

Relevance

●●● Strong
Recent reviews accepted same-line explanations for testpackage and repeated nolint suppressions.
PR-#282
PR-#254

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed lines add several bare //nolint:wrapcheck directives and a bare //nolint:testpackage
directive; none has a same-line rationale beyond the rule identifier.
Rule 2904353: Require justification comments for each nolint directive
mdb/schema.go[82-85]
mdb/schema.go[108-111]
mdb/schema.go[127-130]
mdb/schema_test.go[1-1]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New lint suppression directives in the changed files contain only the rule name and no same-line explanation of why the suppression is safe.

## Fix Focus Areas
- mdb/schema.go[82-85]
- mdb/schema.go[108-111]
- mdb/schema.go[127-130]
- mdb/schema_test.go[1-1]

## Recommended Fix
Add concise, human-readable rationale comments after every `//nolint` directive, explaining why the raw error return or internal test package is intentional and safe.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Integration tests bypass shared timeouts~~ ✓ Resolved

Description

TestListCollections_ExcludeSystemCollectionsClientSide and
TestListDatabaseNames_ExcludeInternalClientSide create deadlines with context.WithTimeout
instead of the shared timeout helper. Running either integration test therefore bypasses the
repository-wide timeout policy for its database setup and MongoDB operations.

Code

mdb/schema_integration_test.go[94]
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

Relevance

●●● Strong
Recent integration-test reviews accepted replacing direct context.WithTimeout with
util.CtxWithTimeout.
PR-#282
PR-#284

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed integration tests directly call context.WithTimeout at both test setup sites, while
the checklist requires application and reviewed Go code to use util.CtxWithTimeout except when
explicitly testing that utility.
Rule 2904342: Use util.CtxWithTimeout for all context timeouts
mdb/schema_integration_test.go[94-95]
mdb/schema_integration_test.go[134-135]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new integration tests call `context.WithTimeout` directly, contrary to the repository requirement that timeout contexts use `util.CtxWithTimeout`.

## Fix Focus Areas
- mdb/schema_integration_test.go[94-94]
- mdb/schema_integration_test.go[134-134]

## Recommended Fix
Import the project `util` package and replace both direct `context.WithTimeout` calls with `util.CtxWithTimeout`, preserving the 30-second duration and cancellation handling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

✅ Tickets: 🎫 PCSM Failure during dropDatabase

✅ Compliance rules (platform): 21 rules

✅ Cross-repo context — repo relationships

Explored: repo: percona/percona-server-mongodb (sha: bf41dbe6) — View relationship

Review mode: Auto: 🚀 Fast: Localized test assertion change with no production behavior impact.

✅ Fix-loop round: fix loop

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit c981c0e

Results up to commit 90313c6 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🟠 Medium

1. ~~New lint suppressions lack reasons~~ ✓ Resolved

Description

The changed code adds //nolint:wrapcheck and //nolint:testpackage directives without
human-readable explanations on the same lines. These suppressions cover error returns and package
declarations in the new code, so a later maintainer cannot tell why each exception is considered
safe.

Code

mdb/schema.go[85]
+		return nil, err //nolint:wrapcheck

Relevance

●●● Strong
Recent reviews accepted same-line explanations for testpackage and repeated nolint suppressions.
PR-#282
PR-#254

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed lines add several bare //nolint:wrapcheck directives and a bare //nolint:testpackage
directive; none has a same-line rationale beyond the rule identifier.
Rule 2904353: Require justification comments for each nolint directive
mdb/schema.go[82-85]
mdb/schema.go[108-111]
mdb/schema.go[127-130]
mdb/schema_test.go[1-1]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New lint suppression directives in the changed files contain only the rule name and no same-line explanation of why the suppression is safe.

## Fix Focus Areas
- mdb/schema.go[82-85]
- mdb/schema.go[108-111]
- mdb/schema.go[127-130]
- mdb/schema_test.go[1-1]

## Recommended Fix
Add concise, human-readable rationale comments after every `//nolint` directive, explaining why the raw error return or internal test package is intentional and safe.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Integration tests bypass shared timeouts~~ ✓ Resolved

Description

TestListCollections_ExcludeSystemCollectionsClientSide and
TestListDatabaseNames_ExcludeInternalClientSide create deadlines with context.WithTimeout
instead of the shared timeout helper. Running either integration test therefore bypasses the
repository-wide timeout policy for its database setup and MongoDB operations.

Code

mdb/schema_integration_test.go[94]
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

Relevance

●●● Strong
Recent integration-test reviews accepted replacing direct context.WithTimeout with
util.CtxWithTimeout.
PR-#282
PR-#284

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed integration tests directly call context.WithTimeout at both test setup sites, while
the checklist requires application and reviewed Go code to use util.CtxWithTimeout except when
explicitly testing that utility.
Rule 2904342: Use util.CtxWithTimeout for all context timeouts
mdb/schema_integration_test.go[94-95]
mdb/schema_integration_test.go[134-135]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new integration tests call `context.WithTimeout` directly, contrary to the repository requirement that timeout contexts use `util.CtxWithTimeout`.

## Fix Focus Areas
- mdb/schema_integration_test.go[94-94]
- mdb/schema_integration_test.go[134-134]

## Recommended Fix
Import the project `util` package and replace both direct `context.WithTimeout` calls with `util.CtxWithTimeout`, preserving the 30-second duration and cancellation handling.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit e4b9c9c 🚀 Fast

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🟠 Medium

1. ~~The wire test accepts missing filters~~ ✓ Resolved

Description

requireNoServerSideFilter records a missing filter as nil and then skips that entry instead of
verifying it is an empty document. A malformed or changed command that omits the filter entirely can
therefore satisfy the retry-aware assertion without proving the required empty-filter request shape.

Code

mdb/schema_integration_test.go[R65-76]

+	// At least, not exactly: a transient error retried by RunWithRetryVal adds
+	// Started events while the helper still succeeds.
+	require.GreaterOrEqual(t, len(r.filters[command]), minCalls, "%s calls", command)
+
+	for i, filter := range r.filters[command] {
+		if filter == nil {
+			continue
+		}
+
+		elems, err := filter.Elements()
+		require.NoError(t, err)
+		assert.Empty(t, elems, "%s #%d must not filter server-side", command, i+1)

Relevance

●●● Strong
Accepted test-hardening patterns require assertions to detect malformed or missing command data; nil
entries currently bypass the wire-shape check.
PR-#282
PR-#254

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The retry-tolerant helper appends nil when filter is absent, then continues past nil entries,
so the test can pass without checking the filter on those commands. This weakens the wire-level
protection for the empty-filter behavior required by the dropDatabase replication fix.

Replicate dropDatabase without collection-name filter errors
mdb/schema_integration_test.go[59-76]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The integration assertion skips recorded commands whose `filter` field is absent, so it does not verify that every list command carries the intended empty filter shape.

## Fix Focus Areas
- mdb/schema_integration_test.go[65-76]

## Recommended Fix
Treat a missing `filter` field as an assertion failure, or normalize it explicitly to an empty BSON document only if the MongoDB driver guarantees that representation is equivalent for this test. Continue allowing additional captured calls from retries while validating every captured command's filter.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Powered by Qodo

chupe · 2026-10-19T14:42:56Z

/agentic_review

qodo-code-review · 2026-10-19T14:44:04Z

Code review by qodo was updated up to the latest commit d353d02

chupe · 2026-10-19T14:46:07Z

/agentic_review

qodo-code-review · 2026-10-19T14:47:37Z

Code review by qodo was updated up to the latest commit e4b9c9c

chupe · 2026-10-19T14:49:34Z

/agentic_review

qodo-code-review · 2026-10-19T14:50:52Z

Code review by qodo was updated up to the latest commit 5406fbd

Replaying a dropDatabase event lists the target collections with {name: {$not: {$regex: "^system\\."}}}. Plain mongod accepts it, the Atlas Flex/M0 proxy rejects it with "(AtlasError) can't get regex from filter doc not a regex" and the run ends in the failed state, with resume --from-failure hitting the same event again. Native regex under $not is rejected too, verified on a Flex 8.0.34 cluster. ListDatabaseNames has the same problem on the source side: the proxy only takes a string or regex for name, so {$nin: [...]} fails at clone start. Send an empty filter to listCollections and listDatabases and drop system.* collections and admin/config/local in Go. The integration tests record the command filter with a CommandMonitor, since mongod cannot reproduce the rejection.

Integration tests bound their MongoDB work with util.CtxWithTimeout like the rest of the suite, and clean up with their own bounded contexts instead of the already-cancelled test context. Every new nolint directive carries its reason.

A transient error retried by RunWithRetryVal adds Started events while the helper still succeeds, so the recorder now requires at least the expected number of calls and still checks every captured filter.

The driver always encodes filter: {} for an empty document, so a missing field is a changed command shape, not a valid no-predicate request. The recorder now fails on it instead of skipping it.

qodo-code-review Bot reviewed Oct 19, 2026

View reviewed changes

Comment thread mdb/schema_integration_test.go Resolved

Comment thread mdb/schema.go Resolved

qodo-code-review Bot reviewed Oct 19, 2026

View reviewed changes

Comment thread mdb/schema_integration_test.go Resolved

chupe added 4 commits October 19, 2026 18:16

PCSM-399: Address Qodo review

c6e7e59

Integration tests bound their MongoDB work with util.CtxWithTimeout like the rest of the suite, and clean up with their own bounded contexts instead of the already-cancelled test context. Every new nolint directive carries its reason.

PCSM-399: Tolerate retried list commands in the wire assertion

acbe037

A transient error retried by RunWithRetryVal adds Started events while the helper still succeeds, so the recorder now requires at least the expected number of calls and still checks every captured filter.

PCSM-399: Require the filter field in the wire assertion

ad832de

The driver always encodes filter: {} for an empty document, so a missing field is a changed command shape, not a valid no-predicate request. The recorder now fails on it instead of skipping it.

chupe force-pushed the PCSM-399 branch from 5406fbd to ad832de Compare October 19, 2026 16:16

Merge branch 'main' into PCSM-399

c981c0e

Navigation Menu

Repository navigation

PCSM-399: Fix dropDatabase replication to Atlas Flex - #286

PCSM-399: Fix dropDatabase replication to Atlas Flex#286
chupe wants to merge 5 commits into
mainfrom
PCSM-399

chupe commented Oct 19, 2026 •

edited by atlassian Bot

Loading

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

chupe commented Oct 19, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

chupe commented Oct 19, 2026

Uh oh!

qodo-code-review Bot commented Oct 19, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chupe commented Oct 19, 2026 •

edited by atlassian Bot

Loading

qodo-code-review Bot commented Oct 19, 2026 •

edited

Loading