Codex automated review

The fix resolves the reported typo, but raw substring matching causes CDC omissions and still misses foreign keys added outside the original CREATE SQL.

P1 - Do not classify raw SQL literals as foreign-key constraints (pkg/cdc/table_scanner.go:757)

rel_createsql is the original CREATE statement (pkg/sql/plan/build_ddl.go:1099-1110), so a valid non-FK table such as CREATE TABLE t (note VARCHAR(32) DEFAULT 'foreign key') is incorrectly skipped. This silently removes the table from the scanner map and stops CDC for it. Detect the parsed constraint or use FK metadata rather than an unquoted substring search.

P1 - Foreign keys added with ALTER TABLE remain scannable (pkg/cdc/table_scanner.go:757)

The predicate only inspects the original rel_createsql. The supported in-place ALTER path creates an AddFk action and updates mo_foreign_keys (pkg/sql/plan/build_ddl.go:3536-3608; pkg/sql/compile/ddl.go:1440-1447), but does not rewrite the table's original CREATE SQL. Therefore ALTER TABLE t ADD FOREIGN KEY ... leaves no matching text and the table continues into CDC despite having a foreign key.

Codex automated review

The raw-SQL false-positive concern from the prior review is resolved, but the replacement metadata lookup cannot identify production foreign keys. Focused tests could not start because cgo/libmo.dylib is absent; the added tests mock the computed boolean and therefore do not exercise either failure.

P1 - Match foreign keys using metadata that is actually populated (pkg/cdc/sql_builder.go:257)

The FK DDL path writes literal zeroes into mo_foreign_keys.db_id and table_id for both CREATE and ALTER (getSqlForAddFk sets row[3] and row[5] to "0"), and no later code populates those fields. User tables have nonzero mo_tables.reldatabase_id/rel_id values, so this EXISTS predicate remains false and the scanner still admits every FK table. Match the populated name fields or use the serialized constraint metadata in mo_tables instead.

P1 - Read foreign-key metadata in the owning tenant (pkg/cdc/sql_builder.go:257)

scanTable executes this query without an account override, so the internal executor defaults to system account 0. mo_foreign_keys is an ordinary predefined table created separately in every account and has no account_id column, whereas mo_tables is cluster-visible. Consequently, this subquery can only see account 0's FK rows and cannot classify CDC tables belonging to tenant accounts. Query each owning tenant or use cluster-visible metadata from mo_tables.

Codex automated review

The current head resolves the prior review findings, but the new reader-reconciliation path introduces three blocking concurrency/lifecycle defects. Current-head CI is green; focused local tests could not start because cgo/libmo.dylib is absent.

P1 - Do not mutate a live reader's table info while matching patterns (pkg/frontend/cdc_exector.go:862)

GetTableInfo returns the same DbTableInfo pointer held by TableChangeStream and its sinker. matchAnyPattern writes SinkDbName and SinkTblName before Close, while active reader/sinker goroutines read that object without synchronization. This is a production data race even when the assigned values are unchanged. Use a clone or separate pure matching from sink-field population; the mock-reader tests cannot expose this race.

P1 - Include Close in the removed-reader shutdown timeout (pkg/frontend/cdc_exector.go:873)

The 10-second timeout starts only after reader.Close returns. TableChangeStream.Close synchronously calls mysqlSinker2.Close, which waits for the consumer; an in-flight Exec uses the background sinker context and can block until the default 10-minute MySQL timeout. Meanwhile the shared TableDetector remains in handling state, delaying discovery callbacks for every CDC task on the CN. Make the complete Close-and-Wait operation bounded/cancelable; the added mock has a nonblocking Close and misses this path.

P1 - Make timed-out reader shutdown single-flight (pkg/frontend/cdc_exector.go:874)

After Wait times out, the reader intentionally remains in runningReaders. Every later scan where the table is still absent therefore starts another goroutine waiting on the same reader. If shutdown remains stuck, these goroutines grow without bound and every scan also spends another 10 seconds in this callback. Track one in-progress shutdown/wait per reader until it exits, and cover repeated missing scans with a blocked Wait.

Codex automated review

The latest update resolves the previously reported ownership race, live-table mutation, unbounded Close call, and repeated shutdown goroutines. However, shutdown timeouts are applied serially per removed reader, so one scan can still block global CDC discovery for an arbitrarily long time.

P1 - Bound the entire removed-reader reconciliation, not each reader serially (pkg/frontend/cdc_exector.go:872)

sync.Map.Range invokes stopRemovedReader serially, and each invocation waits up to 10 seconds at lines 917-924. If a wildcard CDC task has N readers disappear in one scan—such as adding foreign keys to many tables while their sinkers are blocked—the callback takes up to N×10 seconds. During that time it holds the executor lock and TableDetector.handling remains true, so discovery callbacks for every CDC task on this CN are stalled; 100 affected tables can block scans for roughly 17 minutes. Start all shutdowns without per-reader synchronous waits, or use one aggregate deadline for the reconciliation batch, and add a multi-reader blocked-Close regression.

Codex automated review

The scanner metadata fix is sound, but two blocking lifecycle issues remain in reader reconciliation.

P1 - Bound reconciliation across all task callbacks, not per task (pkg/frontend/cdc_exector.go:880)

The latest change bounds all removed readers within one executor to 10 seconds, but TableDetector invokes every task callback sequentially while its global handling flag remains true. If M CDC tasks each have one blocked removed reader, every callback waits here for its own deadline, producing M×10 seconds of blocked discovery for every task on the CN. Make shutdown initiation nonblocking for callbacks or enforce one detector-wide deadline.

P1 - Keep reader ownership until cleanup actually finishes (pkg/cdc/table_change_stream.go:607)

stopRemovedReader calls Close, which cancels Run before potentially blocking in sinker.Close. The canceled stream enters cleanup and deletes its ownership entry here even though cleanup later calls Close again and Wait is still pending. If the foreign key is dropped before that sink shutdown completes, the next scan starts a replacement reader while the old sink may still have an in-flight downstream operation, allowing overlapping pipelines and shared-watermark races. The mock shutdown tests miss this real-stream self-deletion; retain ownership until the complete cleanup has finished.

Reviewed latest head 2f6bd73. The prior scanner correctness and reader-lifecycle findings are now resolved, but the newly added frontend regression test is not synchronized with the asynchronous shutdown it is testing and fails on the current head.

P1 - Wait for async shutdown completion before asserting ownership removal (pkg/frontend/cdc_test.go:4337)

stopRemovedReader deliberately performs Close -> Wait -> CompareAndDelete in a goroutine. Receiving from closeCh only proves that Close reached its notification; it does not establish that Wait and the subsequent map deletion have completed. The test immediately calls runningReaders.Load, so it races the shutdown goroutine and intermittently observes the old reader.

This fails locally on the current head:

.agents/skills/mo-dev/scripts/mo-cgo-test -count=20 -timeout=180s -run TestCdcTask_handleNewTablesStopsReaderRemovedFromScan ./pkg/frontend
--- FAIL: TestCdcTask_handleNewTablesStopsReaderRemovedFromScan
    cdc_test.go:4338: Should be false

Synchronize on completion of the whole shutdown (or use require.Eventually for the ownership deletion) before asserting. The other focused production-path tests and the CDC -race -count=3 set pass.

Codex automated review

The FK metadata and reader-lifecycle fixes address the prior production findings, but one new regression test remains flaky on the current head.

P1 - Synchronize the async shutdown before asserting reader removal (pkg/frontend/cdc_test.go:4337)

stopRemovedReader performs Close, Wait, and CompareAndDelete in a goroutine (cdc_exector.go:907-913). Receiving closeCh only establishes that mockChangeReader.Close closed that channel; it occurs before Wait and map deletion. The test can therefore resume at line 4337 and observe the old reader before the shutdown goroutine deletes it, producing an intermittent failure. Wait for completion of the full shutdown or use require.Eventually for the map assertion.

Deep re-review completed on 782e62b. No blocking findings.

• Correctness: FK detection now uses the canonical mo_tables.constraint / engine.ConstraintDef encoding, covers CREATE and ALTER ADD/DROP FK, avoids SQL-text false positives, and preserves the previous complete scan snapshot on malformed metadata.
• Lifecycle/concurrency: readers removed from scan results are closed asynchronously and single-flighted; ownership remains in runningReaders until full cleanup, and value-aware deletion cannot remove a replacement reader.
• Performance/risk: detector callbacks no longer wait serially for reader shutdown; work is bounded to one in-flight shutdown per removed reader. The scan adds one catalog blob column and remains linear in scanned tables/constraint bytes.
• Validation: focused tests passed; CDC race tests passed 10x, frontend shutdown/replacement race tests passed 20x, full pkg/cdc passed under -race, go vet ./pkg/cdc ./pkg/frontend passed, and the focused tests passed on a synthetic merge with latest main. Current CI is green.

Codex automated review

The scanner fix and latest test synchronization are sound, and earlier FK metadata/ownership findings are resolved. One blocking reader-reappearance path can still stall global CDC discovery.

P1 - Avoid synchronously waiting for an in-flight removed reader (pkg/frontend/cdc_exector.go:1219)

When a scan omits a table, stopReadersMissingFromScan starts Close/Wait asynchronously but deliberately retains the old reader in runningReaders until cleanup. If a later scan contains the same name with a new table ID before cleanup finishes—for example, the table is dropped and recreated with identical CREATE SQL and no FK—OnlyDiffinTblId is true here and line 1231 waits synchronously for that already-stuck reader. TableChangeStream.Wait cannot finish until cleanup completes, while mysqlSinker2.Close may wait for in-flight sender/consumer work for the default 10-minute SQL timeout. Because this callback holds the executor lock and TableDetector runs callbacks serially with handling=true, one such transition stalls discovery for every CDC task on the CN. The new blocked-reader test only reintroduces the same ID and therefore misses this branch. Skip the table-ID wait while removedReaderShutdowns owns this reader, and cover different-ID reappearance.

Codex automated review

No actionable findings. The follow-up correctly avoids waiting on an in-progress removed-reader shutdown while preserving single-flight ownership and cleanup semantics.