Skip to content

fix: init mirror models eagerly and recover lost writes on reconnect#1585

Merged
TheLastCicada merged 8 commits into
v2-rc2from
fix/mirror-init-startup-race
Apr 20, 2026
Merged

fix: init mirror models eagerly and recover lost writes on reconnect#1585
TheLastCicada merged 8 commits into
v2-rc2from
fix/mirror-init-startup-race

Conversation

@TheLastCicada

@TheLastCicada TheLastCicada commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a silent startup race in CADT's MySQL mirror: if MySQL wasn't reachable when CADT started, every mirror Model.init() call was permanently skipped and subsequent mirror writes threw Cannot read properties of undefined (reading 'constructor') from deep inside Sequelize. The mirror stayed non-functional for the life of the process. Observed on the chiamanaged-cadt-testneta-observer-official k8s namespace — cadt_mirror_v1 and cadt_mirror_v2 databases contained zero tables despite ~60 mirror errors per sync cycle.

This PR addresses both the immediate init race AND the broader durability gap where writes during a MySQL outage are silently dropped.

Core fix (V1 + V2)

  • safeMirrorDbHandler[V2] no longer wraps Model.init(). A new initMirrorModel[V2] helper runs init synchronously at module load — safe because Sequelize.init() is pure metadata registration and doesn't need a live connection. All 34 V1+V2 *.model.mirror.js files updated.
  • safeMirrorDbHandler[V2] now detects disconnected → connected transitions (and "setup never ran at startup") and runs prepareMysqlMirror[V2] + backfillMirror[V2] before the current callback. Writes dropped during an outage are recovered without a CADT restart.
  • backfillMirror[V2] now also sweeps orphan rows (mirror rows whose PK is no longer in source) so DELETEs issued during an outage eventually propagate. Pagination now has ORDER BY on the PK so concurrent writes don't cause skipped rows. V1 previously had no backfill at all — added in this PR.
  • validateMirrorDbNames fails loud on startup instead of being swallowed by the mirror-setup try/catch.

Tests

  • New integration tests assert: every mirror model is initialised at module load (with a single-column-PK invariant that guards the orphan sweep); orphan sweep removes source-deleted rows from the mirror; reconnect detection runs backfill; steady-state connects do NOT run backfill.
  • V1 live-api data-short.js now verifies records land in the V1 MySQL mirror (symmetric with V2). Ports mysql-mirror-helpers to V1 and fixes a date-then-numeric fallback in both helpers so '2024-01-01' no longer equals '2024-06-15' via parseFloat.

CI

  • test-v2-live-api and test-v1-live-api now stop MariaDB mid-setup and restart it ~30s after CADT starts, exercising the startup race + reconnect+backfill recovery end-to-end.
  • Both jobs hard-fail if MariaDB is still reachable after service mariadb stop so a degraded race test cannot silently pass.
  • V1 Start CADT polls the V1 API with exit-on-timeout, matching V2's /health polling.

Related fix (same test surface)

wallet-health.live.spec.js and datalayer-test-helpers.js now honour CERTIFICATE_FOLDER_PATH via a new shared live-api helper, matching src/datalayer/wallet.js. The previous hardcoded ${chiaRoot}/config/ssl gave a false-negative in deployments using a non-default Chia SSL directory.

Test plan

  • npm run test:v2 passes (1465 tests, +34 from new single-PK invariant guards)
  • npm run test:v1 passes (115 tests)
  • Targeted mirror-model-init + mirror-reconnect + mirror-backfill suites pass × 3 stability runs
  • node --check all 46 changed files
  • Zero new lint errors / warnings introduced (verified against origin/v2-rc2 baseline)
  • js-yaml validation of .github/workflows/tests.yaml
  • Two rounds of adversarial pre-push-diff-review (4 slices × 2 = 8 subagent reviews); all critical + high-impact findings fixed; info-level findings triaged and filed for follow-up where appropriate
  • test-v2-live-api CI job exercises the delayed-MariaDB race path
  • test-v1-live-api CI job exercises the V1 delayed-MariaDB race path and verifies V1 mirror record landing

Context

  • Observer impact analysis: agent transcript
  • Related fixes already on v2-rc2: c0eb64fb (handle MySQL mirror connection failure gracefully in v1 prepareDb). This PR's reconnect logic supersedes the "will be retried by safeMirrorDbHandler" assumption in that commit's message — retries now actually work because models are initialised independently of authenticate.

Note

High Risk
High risk because it changes V1/V2 mirror database enablement, setup/migration, and outage recovery paths, which can affect data consistency and startup behavior under partial MySQL availability.

Overview
Fixes MySQL mirror startup and outage durability by initializing mirror models eagerly (new initMirrorModel/initMirrorModelV2) and adding reconnect-aware recovery in safeMirrorDbHandler/safeMirrorDbHandlerV2 that retries mirror setup and runs catch-up backfills before applying new writes.

Adds/expands mirror backfill logic: V1 gains a full backfillMirror implementation; V2’s backfillMirrorV2 is upgraded to keyset pagination and now also sweeps orphan rows to propagate deletes missed during outages, with guards/overrides for tests and consistent mirror enablement snapshots.

Hardens CI and tests around these scenarios: live-api workflows now deliberately start CADT with MariaDB down then restart it to assert recovery; new integration specs cover model-init, datetime serialization, reconnect backfill, and orphan sweep; V1 live-api runner now verifies MySQL-mirror contents and closes the mirror pool; live-api TLS helpers now honor CERTIFICATE_FOLDER_PATH.

Reviewed by Cursor Bugbot for commit dfab796. Bugbot is set up for automated code reviews on this repo. Configure here.

Fixes a silent startup race in CADT's MySQL mirror: if MySQL was not
yet reachable when CADT started, every mirror Model.init() call was
permanently skipped. Subsequent mirror writes threw
"Cannot read properties of undefined (reading 'constructor')" from
deep inside Sequelize and the mirror stayed non-functional for the
life of the process. Observed on the cadt-testneta-observer k8s pod.

Core fix (V1 + V2):
- safeMirrorDbHandler[V2] no longer wraps Model.init(). A new
  initMirrorModel[V2] helper initialises models synchronously at
  module-load time, which is safe because Sequelize.init is pure
  metadata registration and does not require a live connection.
  All 34 V1+V2 *.model.mirror.js files updated.
- safeMirrorDbHandler[V2] now detects disconnect->reconnect
  transitions (and "setup never ran at startup") and runs
  prepareMysqlMirror[V2] + backfillMirror[V2] before the current
  callback, so writes dropped during an outage are recovered
  without requiring a CADT restart.
- backfillMirror[V2] also sweeps orphan rows (mirror rows whose PK
  no longer exists in source) so DELETEs issued during an outage
  eventually propagate. Pagination now has an ORDER BY on the PK
  so concurrent writes don't cause skipped rows. V1 previously
  had no backfill at all - that is added.
- validateMirrorDbNames now fails loud instead of being swallowed
  by the mirror-setup try/catch, so a V1/V2 DB_NAME collision
  aborts startup the same way it did before this PR's refactor.

Tests:
- New integration tests: assert every mirror model is initialised
  at module load (with a single-column-PK invariant guarding the
  orphan sweep); orphan sweep removes source-deleted rows from
  the mirror; reconnect detection runs backfill; steady-state
  connects do NOT run backfill.
- V1 live-api data-short.js now verifies records land in the V1
  MySQL mirror (symmetric with V2). Ports mysql-mirror-helpers to
  V1 and fixes a date-then-numeric comparison fallback in both
  helpers so '2024-01-01' no longer equals '2024-06-15' by
  parseFloat.

CI:
- test-v2-live-api and test-v1-live-api now stop MariaDB mid-setup
  and restart it ~30s after CADT starts, exercising the startup
  race plus the reconnect+backfill recovery path end-to-end.
  Both jobs hard-fail if MariaDB is still reachable after
  service mariadb stop so a degraded race test cannot silently
  pass. V1 Start CADT now polls the V1 API endpoint with
  exit-on-timeout, matching V2.

Related fix (out of scope for the bug but same test surface):
- wallet-health.live.spec.js and datalayer-test-helpers.js now
  honour CERTIFICATE_FOLDER_PATH via a new shared live-api helper,
  matching src/datalayer/wallet.js. Previously hardcoded
  \${chiaRoot}/config/ssl would false-negative in deployments
  using a non-default Chia SSL directory.
Comment thread src/database/v2/index.js
Comment thread src/database/index.js
Sequelize's default datetime format "YYYY-MM-DD HH:mm:ss.SSS +00:00"
includes a trailing "+00:00" offset that recent MariaDB strict mode
rejects with:

  Incorrect datetime value: '2026-04-16 22:41:27.490 +00:00'
    for column `cadt_mirror_test`.`audit`.`createdAt` at row 1

Every mirror write touching a DATE/DATETIME column was silently dropped
by the fire-and-forget safeMirrorDbHandler[V2]. This affected both V1
and V2 mirrors in production - the v2 live-api CI run exposed 59 such
errors but still passed because V2 test assertions only check
project/methodology-level records, not audit. V1 live-api verification
added in this PR surfaced the bug directly on the verified records
(projects.projectStatusDate, labels.creditingPeriodStartDate, etc.).

The fix is to set `timezone: '+00:00'` at the Sequelize constructor
level for both `mirror` and `v2Mirror` configs. At the constructor
level (unlike Model.init, where the option is silently ignored),
Sequelize converts Date values to UTC and emits the MariaDB-safe
"YYYY-MM-DD HH:MM:SS[.SSS]" format.

Also updates prepareMysqlMirrorV2's JSDoc to reflect that it does
throw (intentionally) when V1/V2 share a MIRROR_DB name - the earlier
"Never throws" claim was left over from before validateMirrorDbNames
was moved to fail-loud. Both callers (prepareV2Db and
startV2ReconnectBackfill) are wrapped in try/catch at the right level.
Comment thread src/database/index.js
Two addressable bugbot + CI findings on the mirror fixes branch:

1. MariaDB "Incorrect datetime value" errors continued after the prior
   `timezone: "+00:00"` config tweak. Root cause: Sequelize 6's base
   `DATE._stringify` emits "YYYY-MM-DD HH:mm:ss.SSS Z" (e.g.
   "2026-04-17 17:07:13.399 +00:00") which MariaDB strict mode rejects,
   and some runtime paths reach the base serialiser even when a dialect-
   specific MySQL class is wired up. Patch the base DATE._stringify to
   emit the MariaDB-safe "YYYY-MM-DD HH:mm:ss" format; shadow an own-
   property `_stringify` on `sqlite.DATE.prototype` that preserves the
   original offset-bearing format (SQLite's parse() round-trip depends
   on it). Add `tests/v2/integration/mirror-datetime-format.spec.js`
   to guard the patch against future Sequelize upgrades.

2. Backfill used offset-based pagination, which can silently skip rows
   if a row is deleted concurrently (the next OFFSET lands one row
   later than intended). Switch to keyset pagination
   (WHERE pk > :lastSeenPk ORDER BY pk ASC LIMIT N) in both V1 and V2.
   Composite-PK mirrors now throw explicitly instead of silently
   loading the whole table; the single-PK invariant is still asserted
   at load time by mirror-model-init.spec.js. Add a defensive break
   if a terminal row returns a null PK. Tighten V2's outer-catch log
   formatting to match V1's (defensive `error?.message` fallback plus
   stack on debug).
@TheLastCicada

Copy link
Copy Markdown
Contributor Author

Addressed the Bugbot pagination finding in 5d23732:

Root fix: Switched both V1 (src/database/index.js) and V2 (src/database/v2/index.js) mirror backfill from offset-based to keyset pagination (WHERE pk > :lastSeenPk ORDER BY pk ASC LIMIT N). This eliminates the offset-shift-on-delete hazard.

Documented caveat: Keyset pagination is still not immune to concurrent INSERTs with a PK less than lastPk (plausible with UUID PKs, which most V2 mirrors use). Such rows are missed by the current pass but picked up by the next reconnect backfill, and by normal safeMirrorDbHandler[V2] writes in steady state — so the miss window is bounded.

Hardening: Composite-PK mirrors now throw explicitly instead of silently loading the whole source table into memory. mirror-model-init.spec.js asserts every current mirror has a single-column PK; this is a defensive rail for future schema changes.

Also bundled in the same commit (same CI failure, different root cause):

  • MariaDB Incorrect datetime value errors continued after the prior timezone config tweak. Root cause: Sequelize 6s base DATE._stringify emits "YYYY-MM-DD HH:mm:ss.SSS Z" which MariaDB strict mode rejects, and some runtime paths reach the base serialiser even when a dialect-specific class is wired up. Patch both the base serialiser (MariaDB-safe format) and the sqlite subclass (shadows the original format so SQLite parse round-trips). Added tests/v2/integration/mirror-datetime-format.spec.js as a regression guard.

Comment thread src/database/v2/index.js Outdated
The prior revision patched Sequelize's base DATE._stringify to force
a MariaDB-safe format, on the hypothesis that some steady-state write
paths bypassed the mysql.DATE subclass. Empirical tracing (all three
write paths - single INSERT, bulk INSERT, INSERT ... ON DUPLICATE KEY
UPDATE - instrumented against the query generator) shows that is NOT
the case: every MySQL write dispatches through mysql.DATE.stringify,
which already emits "YYYY-MM-DD HH:mm:ss".

The actual source of the residual "Incorrect datetime value" CI errors
was the reconnect-backfill path. `source.findAll({ raw: true })`
returns SQLite DATE columns as raw strings ("YYYY-MM-DD HH:mm:ss.SSS
+00:00") without running sqlite.DATE.parse. The strings were forwarded
verbatim into `mirror.bulkCreate`, which MariaDB strict mode rejected.

Fix: drop `raw: true` from backfill's findAll calls. Model instances
run sqlite.DATE.parse during construction, so DATE columns become Date
objects on dataValues; mysql.DATE then serialises them safely.

Use `.get({ plain: true, raw: true })` to extract the plain payload -
plain:true alone runs attribute-level `get()` accessors, which would
corrupt columns like ProjectV2.projectSector (stored as JSON, returned
as Array by the getter) when bulkCreate'd into the mirror's STRING/TEXT
column.

The sqlite PK-only orphan sweep keeps `raw: true` - its projection
excludes DATE and custom-getter columns, and a comment now documents
that invariant.

Revert the previous base-class `_stringify` monkey-patch and the
no-op `timezone: "+00:00"` constructor options (that value is already
Sequelize's default). Simplify the datetime regression test to pin
only the mysql subclass's output contract - the one Sequelize API we
actually depend on.
@TheLastCicada

Copy link
Copy Markdown
Contributor Author

Root-cause fix pushed as 4aa768b.

What the prior CI run revealed
The 34 residual Incorrect datetime value errors on 5d237321 (down from 266 pre-fix) had the same .SSS +00:00 signature even though the base _stringify patch was active. That was the clue that the bad value was not being produced by _stringify at all - it was arriving pre-serialized.

Actual root cause
source.findAll({ raw: true }) in the reconnect-backfill path does not run sqlite.DATE.parse. DATE columns come back as the raw SQLite-stored strings like "2026-04-17 17:07:13.399 +00:00". Those strings were forwarded verbatim to mirror.bulkCreate, and MariaDB strict mode rejected them. Verified empirically with a minimal Sequelize repro.

What the prior patch actually did
The base _stringify monkey-patch reduced the error count from 266 → 34 because it affected the local-to-local SQLite write path (meaning subsequent raw reads produced a safer string). But it still left .SSS +00:00 in the persisted SQLite files, so the raw-backfill replay still failed for any row whose SQLite text happened to still contain the offset form (e.g. rows written in the same process but by a different code path, or simply not fully ingested yet).

What 4aa768b does
Drop the monkey-patch entirely (plus the no-op timezone: "+00:00" constructor options that were already the Sequelize default). Instead, drop raw: true from backfill findAll so model instances are built - which runs sqlite.DATE.parse and yields real Date objects. bulkCreate then serializes them through mysql.DATE._stringify, which already produces the MariaDB-safe YYYY-MM-DD HH:mm:ss format.

Empirical verification of the steady-state path
Instrumented Sequelize.DataTypes.DATE.prototype._stringify and Sequelize.DataTypes.mysql.DATE.prototype._stringify and ran single INSERT, bulk INSERT, and INSERT ... ON DUPLICATE KEY UPDATE through the query generator. Base was called 0 times; mysql was called for every DATE field. So steady-state mirror writes never needed the patch.

Bonus fix caught by pre-push review
Initial commit used .get({ plain: true }) which runs attribute-level get() accessors. That would have corrupted ProjectV2.projectSector / projectType (stored as JSON string, but the getter returns a parsed Array). Switched to .get({ plain: true, raw: true }) which skips getters but keeps Date typing (the sqlite parser runs during instance construction, not in get()).

Comment thread src/database/index.js
V1 live-api CI on 4aa768b still reproduced 33 "Incorrect datetime
value: '... +00:00'" errors on Audit mirror writes - the exact base-
DATE._stringify signature. Local instrumentation shows every MySQL
INSERT path (bulkInsert, insertQuery, upsert) dispatches through
mysql.DATE._stringify, so the CI observation points to some
Sequelize-internal path we can't isolate from here that prototype-
invokes the base method on a mysql attribute type.

Rather than continue bisecting Sequelize internals, restore a
targeted patch on Sequelize.DataTypes.DATE.prototype._stringify that
strips the " +00:00" offset suffix (MariaDB strict mode rejects it)
while keeping "YYYY-MM-DD HH:mm:ss.SSS" fractional seconds so local
SQLite round-trips preserve millisecond precision. Legacy SQLite
rows still round-trip via sqlite.DATE.parse's `date.includes("+")`
branch. Uses a static ESM `import moment from 'moment'` (the prior
attempt with `require('moment')` threw ReferenceError under ESM and
was silently swallowed by safeMirrorDbHandler's catch).

Also addresses the two open Bugbot findings:

- V1 `mirrorDBEnabled()` always returned true in test mode. The
  in-process cost was the on-startup 11-table backfill iterating
  every source/mirror pair. Gate the startup backfill on
  isMysqlMirrorConfiguredForReconnect() instead of mirrorDBEnabled()
  so SQLite-fallback test runs skip it. Keep mirrorDBEnabled() true
  in mirrorTest mode - V1 integration tests rely on that implicit
  test enablement for SQLite-fallback mirror writes to land.

- sweepMirrorOrphans[V2] loaded every PK from both mirror and source
  tables in a single unbounded findAll. Replace with keyset-paginated
  scans. Peak memory is now O(|mirror|) for the orphan-candidate set
  rather than O(|mirror|+|source|); the source side is streamed.

Restore the original mirror-first scan order (a pre-push review
caught that an earlier refactor inverted it, which allowed false-
positive orphan deletes for rows inserted concurrently between the
source scan and the mirror scan).
@TheLastCicada

Copy link
Copy Markdown
Contributor Author

Addressed the two outstanding Bugbot findings in 9571d6e:

Medium: V1 backfillMirror always runs in test environment
The cost of running the 11-table on-startup backfill against the SQLite fallback was the real concern, not mirrorDBEnabled() per se. Gated the startup backfill in prepareDb on isMysqlMirrorConfiguredForReconnect() so SQLite-fallback runs skip it entirely. Kept mirrorDBEnabled() returning true in mirrorTest mode because V1 integration tests rely on that implicit test enablement for their SQLite-fallback mirror writes to land (changing it to match V2 broke 6 Project/Unit integration tests). Documented the V1/V2 asymmetry in-code.

Low: Orphan sweep loads all PKs into memory unbounded
Both sweepMirrorOrphans and sweepMirrorOrphansV2 now keyset-paginate the mirror and source PK scans. Peak memory is O(|mirror|) for the orphan-candidate set; the source side is streamed in pages and used to remove non-orphans from that set, then remaining orphans are deleted in BACKFILL_BATCH_SIZE batches.

Plus one pre-push-review catch that would have regressed: the first refactor inverted the scan order (source-first, mirror-second). That violates the documented concurrency-safety invariant — a row inserted concurrently between the source scan and the mirror scan would have been misclassified as an orphan and deleted. Restored the original mirror-first order; the orphan-candidate set pattern preserves the streaming memory win without losing the safety property.

Also in the same commit: restored a targeted Sequelize.DataTypes.DATE.prototype._stringify patch (strip the " +00:00" offset suffix, keep .SSS ms precision). V1 live-api CI on 4aa768b2 reproduced the exact same 33 Incorrect datetime value: ...SSS +00:00`` errors I thought had been fixed — despite local instrumentation showing every MySQL write path dispatches through mysql.DATE._stringify. Rather than continue bisecting Sequelize internals, the patch treats this as defense-in-depth for any path that does prototype-invoke the base method on a mysql attribute type. Uses a static ESM `import moment from "moment"` this time (the earlier `require("moment")` threw `ReferenceError` in ESM and was silently swallowed by `safeMirrorDbHandler`).

Comment thread src/database/index.js
Adds a gated (CADT_DATE_STRINGIFY_DIAG=1) stack-trace logger on the
patched base _stringify, capped at 8 invocations per process so it
can't spam logs or disk. Enabled in the V1 and V2 live-api Start-CADT
steps so one CI run captures the full call stack for whatever
Sequelize internal path is reaching the base serialiser on a mysql
attribute type despite the mysql.DATE subclass being wired up.

Remove this commit after the root cause is understood and the
explanatory comment in src/config/config.js is updated with the
concrete Sequelize call site.
Comment thread src/config/config.js Outdated
CI stack-trace diagnostic (enabled in 6088b1b) captured 12 base
DATE._stringify invocations in the V1 live-api run. All 12 came from
SQLiteQueryGenerator / SQLiteQueryInterface frames - i.e. the SQLite
source writes. Zero came from a MySQL path. The run produced 0
MariaDB datetime rejections, confirming that:

1. Every MySQL mirror write correctly dispatches through
   mysql.DATE._stringify (which emits the safe "YYYY-MM-DD HH:mm:ss"
   form). The base class is not reached on a MySQL write.

2. The base _stringify IS called for every SQLite source write.
   Before this patch, SQLite therefore persisted
   "YYYY-MM-DD HH:mm:ss.SSS +00:00" - the exact offset-bearing form
   MariaDB strict mode rejects.

3. Some code path (backfill was one; empirical comparison between
   4aa768b and 9571d6e shows at least one more that I was unable to
   isolate) forwards those SQLite-stored strings to MariaDB verbatim.
   The backfill fix (drop raw:true, use `.get({plain:true,raw:true})`)
   closed the backfill path. The base-class patch closes the unknown
   remaining path by making SQLite never persist the " +00:00" form
   in the first place.

Remove the diagnostic counter / env-gated stack logger and the
CADT_DATE_STRINGIFY_DIAG wiring in the V1/V2 live-api CI jobs.
Rewrote the src/config/config.js comment to state the concrete
empirical finding instead of the previous "presumably some internal
path" speculation.
@TheLastCicada

Copy link
Copy Markdown
Contributor Author

Diagnostic stack-trace logger (6088b1b) captured the empirical root cause in V1 live-api CI. Removed the diagnostic in 7b059e2 and rewrote the src/config/config.js comment with concrete findings instead of speculation.

What the diagnostic showed

12 base DATE._stringify invocations in the V1 live-api run. Every single stack trace was a SQLiteQueryGenerator / SQLiteQueryInterface frame — SQLite source writes, never MySQL. Call sites: organization.create, organization.update, audit.create. Sample stack:

at DATE._stringify (src/config/config.js:61)
at DATE.stringify (sequelize/lib/data-types.js:22)
at DATE.bindParam (sequelize/lib/data-types.js:30)
at SQLiteQueryGenerator.format (sequelize/lib/dialects/abstract/query-generator.js:807)
at SQLiteQueryGenerator.insertQuery (sequelize/lib/dialects/abstract/query-generator.js:183)
at SQLiteQueryInterface.insert (sequelize/lib/dialects/abstract/query-interface.js:305)
at Audit.save (sequelize/lib/model.js:2490)
at async audit.create (sequelize/lib/model.js:1362)
at async syncOrganizationAudit (src/tasks/sync-registries.js:335)

Conclusion

  1. Every MySQL mirror write correctly dispatches through mysql.DATE._stringify (safe "YYYY-MM-DD HH:mm:ss" format). The base class is never reached on a MySQL write — my earlier theory about a "Sequelize internal path prototype-invoking the base method" was wrong.

  2. The base _stringify IS called for SQLite source writes (sqlite.DATE inherits, no own override). Before this patch, SQLite therefore persisted "YYYY-MM-DD HH:mm:ss.SSS +00:00" — the exact offset-bearing form MariaDB strict mode rejects.

  3. The rejection errors MariaDB reported were SQLite-stored strings being forwarded to MariaDB verbatim. The reconnect-backfill path was one such forwarder (fixed by dropping raw: true from source.findAll). Empirical comparison of commits 4aa768b2 (backfill fix only, no patch → 33 errors) vs 9571d6e1 (both → 0 errors) proves at least one OTHER forwarding path exists that I was unable to isolate from the CI log.

  4. The base-class patch closes that unknown remaining path by making SQLite never persist the " +00:00" form in the first place. Any downstream consumer that forwards a SQLite-stored date string to MariaDB now sees a safe format, regardless of whether they re-parse it or not.

The patch is therefore necessary, not just defense-in-depth.

Comment thread src/database/v2/index.js
Three unresolved bugbot findings on PR #1585:

1. Skip mirror callback when MySQL setup has not completed yet (V1+V2).
   In safeMirrorDbHandler[V2], after the post-reconnect await, the
   callback previously ran unconditionally. If MySQL setup was still
   failing (mirrorSetupSucceeded=false), the write hit tables that
   did not exist and the error was swallowed by the outer catch. Add a
   guard that returns early when MySQL is configured but setup has not
   succeeded. The next authenticate() success re-enters the reconnect
   path and retries setup.

2. Prevent mirrorDBEnabledV2 from desyncing from sequelizeV2Mirror.
   sequelizeV2Mirror is built once at module load from the memoized
   config. mirrorDBEnabledV2() previously re-read the live config on
   every call, which could flip the enablement true while writes still
   go to the originally-selected SQLite fallback. Snapshot the
   enablement at module load (v2MirrorTargetsMysql) so the check stays
   consistent with the Sequelize instance.

3. Apply the same snapshot pattern to V1 for symmetry
   (v1MirrorConfiguredAtLoad + mirrorDBEnabled).

Add test-only overrides (__setMysqlConfiguredForReconnect*,
__resetMirrorAuthState*) and regression tests for the new skip-callback
guard in both the V1 and V2 reconnect specs. The auth-state reset is
required to prevent the new test from leaving mirrorAuthState in
'disconnected' and leaking into downstream integration specs.
@TheLastCicada

Copy link
Copy Markdown
Contributor Author

Addressed remaining bugbot findings in dfab7961.

Resolved

1. Reconnect backfill runs callback even after setup failure (Low, V1+V2)

After await startReconnectBackfill[V2]() returns, safeMirrorDbHandler[V2] previously fell through to await callback() unconditionally. If setup was still failing (mirrorSetupSucceeded=false), the mirror tables did not exist yet and the callback produced a swallowed "table doesn't exist" error. Added a guard that returns early when isMysqlMirrorConfiguredForReconnect[V2]() && !mirrorSetupSucceeded[V2]. The next authenticate success re-enters the reconnect path and retries setup.

2. Mirror enable check desynchronizes mirror target (Medium, V2)

sequelizeV2Mirror is built once at module load from the memoized config. mirrorDBEnabledV2() previously re-read live config on every call. If the memoize cache is cleared and config swapped at runtime (some test helpers do this), enablement could flip true while writes still go to the originally-selected SQLite fallback. Fixed by snapshotting enablement alongside sequelizeV2Mirror construction (v2MirrorTargetsMysql). Applied the same snapshot pattern to V1 for symmetry (v1MirrorConfiguredAtLoad).

3. V1/V2 sweep and backfill logic fully duplicated (Low, code style)

Replied on the thread — duplication is intentional for this PR; a shared helper would need to parameterise six distinct things (Sequelize instances, loggers, model barrels, state variables, Op imports, and asymmetric test-mode behaviour) and would obscure the code more than inlined copies do. Follow-up refactor issue will be opened separately.

Regression tests

Added a regression test to both tests/integration/mirror-reconnect.spec.js and tests/v2/integration/mirror-reconnect-v2.spec.js that exercises the new skip-callback guard end-to-end (forces isMysqlMirrorConfiguredForReconnect[V2]() true via a new test-only override, leaves setup at false, calls safeMirrorDbHandler[V2], asserts the callback does not run). Confirmed via the logged line Reconnect recovery: mirror setup still failing, backfill skipped that the guard is exercised on the intended path.

Also added __resetMirrorAuthStateForTests[V2] to prevent the new test from leaking mirrorAuthState='disconnected' into downstream integration specs running in the same mocha session.

Test results (local)

  • V1: 116 passing (115 previous + 1 new regression test)
  • V2: 1471 passing (1470 previous + 1 new regression test)

Pre-push diff review: 2 warnings, both addressed (V1/V2 symmetry + test coverage for the new guard).

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit dfab796. Configure here.

Comment thread src/database/v2/index.js
@TheLastCicada TheLastCicada merged commit f9fbe55 into v2-rc2 Apr 20, 2026
26 checks passed
@TheLastCicada TheLastCicada deleted the fix/mirror-init-startup-race branch April 20, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant