Skip to content

Commit dfab796

Browse files
TheLastCicadaTheLastCicada
committed
fix: address bugbot review for mirror handler callback + enable check
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.
1 parent 7b059e2 commit dfab796

4 files changed

Lines changed: 217 additions & 12 deletions

File tree

‎src/database/index.js‎

Lines changed: 62 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,24 @@ const mirrorConfig =
4545
(process.env.NODE_ENV || 'local') === 'local' ? 'mirror' : 'mirrorTest';
4646
export const sequelizeMirror = new Sequelize(config[mirrorConfig]);
4747

48+
// Snapshot of whether V1 MIRROR_DB was fully configured at module-load time.
49+
// Captured alongside sequelizeMirror construction so mirrorDBEnabled() stays
50+
// consistent with the backend sequelizeMirror was actually built against.
51+
// Reading the live config on every call would let tests that clear the
52+
// getConfig() memoize cache (or any future runtime-reload path) drift the
53+
// enablement check away from the already-constructed Sequelize instance.
54+
// Symmetric with v2MirrorTargetsMysql in src/database/v2/index.js.
55+
const v1MirrorConfiguredAtLoad = (() => {
56+
const CONFIG = getConfig();
57+
return !!(
58+
CONFIG?.MIRROR_DB?.DB_HOST &&
59+
CONFIG.MIRROR_DB.DB_HOST !== '' &&
60+
CONFIG.MIRROR_DB.DB_NAME &&
61+
CONFIG.MIRROR_DB.DB_USERNAME &&
62+
CONFIG.MIRROR_DB.DB_PASSWORD
63+
);
64+
})();
65+
4866
const logDebounce = _.debounce(() => {
4967
console.log('Mirror DB not connected');
5068
logger.info('Mirror DB not connected');
@@ -56,6 +74,11 @@ const logDebounce = _.debounce(() => {
5674
// real config value".
5775
let mirrorEnabledTestOverride = null;
5876

77+
// Test-only override for isMysqlMirrorConfiguredForReconnect(). Tests set
78+
// this to simulate "MySQL is configured" (or not) independently of the
79+
// actual config.yaml. null means "use the real config value".
80+
let mysqlConfiguredForReconnectOverride = null;
81+
5982
// Track the last observed auth outcome so we can detect a transition from
6083
// "disconnected" back to "connected" and trigger a catch-up backfill. This
6184
// recovers writes (INSERT/UPDATE) and deletes that were dropped during an
@@ -82,7 +105,6 @@ export const mirrorDBEnabled = () => {
82105
if (mirrorEnabledTestOverride !== null) {
83106
return mirrorEnabledTestOverride;
84107
}
85-
const CONFIG = getConfig();
86108
// In production-like mode ('local' env), require full MySQL config.
87109
// In SQLite-fallback test mode ('mirrorTest' env), leave the mirror
88110
// enabled unconditionally so integration tests that rely on the
@@ -97,13 +119,11 @@ export const mirrorDBEnabled = () => {
97119
// is gated separately in prepareDb() via
98120
// isMysqlMirrorConfiguredForReconnect(), not via mirrorDBEnabled(),
99121
// so the backfill only fires when MySQL is actually configured.
100-
if (
101-
mirrorConfig === 'mirror' &&
102-
(!CONFIG?.MIRROR_DB?.DB_HOST ||
103-
!CONFIG?.MIRROR_DB?.DB_NAME ||
104-
!CONFIG?.MIRROR_DB?.DB_USERNAME ||
105-
!CONFIG?.MIRROR_DB?.DB_PASSWORD)
106-
) {
122+
//
123+
// Uses v1MirrorConfiguredAtLoad (module-load snapshot) so this check
124+
// stays consistent with the backend sequelizeMirror was built against.
125+
// Symmetric with mirrorDBEnabledV2.
126+
if (mirrorConfig === 'mirror' && !v1MirrorConfiguredAtLoad) {
107127
return false;
108128
}
109129
return true;
@@ -122,6 +142,23 @@ export const __setMirrorSetupSucceededForTests = (value) => {
122142
mirrorSetupSucceeded = value;
123143
};
124144

145+
// Test-only. DO NOT call from production code. Forces
146+
// isMysqlMirrorConfiguredForReconnect() to return a specific boolean so
147+
// tests can exercise the reconnect setup / skip-callback guards without a
148+
// live MySQL instance or a real MIRROR_DB config. null restores live behaviour.
149+
export const __setMysqlConfiguredForReconnectForTests = (value) => {
150+
mysqlConfiguredForReconnectOverride = value;
151+
};
152+
153+
// Test-only. DO NOT call from production code. Resets the internal
154+
// auth-state machine back to 'unknown' so tests that intentionally leave
155+
// the state in 'disconnected' can reset it in an afterEach hook and avoid
156+
// leaking reconnect behaviour into downstream specs running in the same
157+
// mocha session.
158+
export const __resetMirrorAuthStateForTests = () => {
159+
mirrorAuthState = 'unknown';
160+
};
161+
125162
/**
126163
* Ensure the V1 MySQL mirror database exists and has up-to-date migrations.
127164
* Idempotent: safe to call from startup and again on every reconnect.
@@ -185,6 +222,9 @@ export const prepareMysqlMirror = async () => {
185222
// Used to gate reconnect-setup retries - only MySQL configs need the setup
186223
// retry path; SQLite test fallbacks never need it.
187224
const isMysqlMirrorConfiguredForReconnect = () => {
225+
if (mysqlConfiguredForReconnectOverride !== null) {
226+
return mysqlConfiguredForReconnectOverride;
227+
}
188228
const CONFIG = getConfig();
189229
return !!(
190230
CONFIG?.MIRROR_DB?.DB_HOST &&
@@ -271,6 +311,20 @@ export const safeMirrorDbHandler = (callback) => {
271311
await reconnectBackfillPromise;
272312
}
273313

314+
// If MySQL is configured but the one-time setup (CREATE DATABASE
315+
// + migrations) has not yet completed, the mirror tables don't
316+
// exist. Running the callback would produce a confusing
317+
// "table doesn't exist" error that gets swallowed by the catch
318+
// below. Skip quietly - the next authenticate success will
319+
// re-enter startReconnectBackfill and retry setup. Symmetric
320+
// with safeMirrorDbHandlerV2.
321+
if (
322+
isMysqlMirrorConfiguredForReconnect() &&
323+
!mirrorSetupSucceeded
324+
) {
325+
return;
326+
}
327+
274328
try {
275329
await callback();
276330
} catch (e) {

‎src/database/v2/index.js‎

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,12 +55,20 @@ const mirrorConfig = mysqlMirrorConfigured ? 'v2Mirror' : 'v2MirrorTest';
5555

5656
loggerV2.info(`[v2]: Mirror DB config selected: ${mirrorConfig} (MySQL configured: ${!!mysqlMirrorConfigured})`);
5757

58+
// Test-only override for isMysqlMirrorConfiguredForReconnectV2(). Tests set
59+
// this to simulate "MySQL is configured" (or not) independently of the actual
60+
// config.yaml. null means "use the real config value".
61+
let mysqlConfiguredForReconnectOverrideV2 = null;
62+
5863
// Returns true if V2.MIRROR_DB is fully configured with MySQL credentials.
5964
// Used to gate reconnect-setup retries - only MySQL configs need the setup
6065
// retry path; SQLite test fallbacks never need it. Reads config on every
6166
// call so the check stays correct across config reloads (symmetric with
6267
// V1's isMysqlMirrorConfiguredForReconnect).
6368
const isMysqlMirrorConfiguredForReconnectV2 = () => {
69+
if (mysqlConfiguredForReconnectOverrideV2 !== null) {
70+
return mysqlConfiguredForReconnectOverrideV2;
71+
}
6472
const c = getConfigV2()?.MIRROR_DB;
6573
return !!(
6674
c?.DB_HOST &&
@@ -73,6 +81,21 @@ const isMysqlMirrorConfiguredForReconnectV2 = () => {
7381

7482
export const sequelizeV2Mirror = new Sequelize(config[mirrorConfig]);
7583

84+
// Snapshot of "is the mirror Sequelize instance actually pointing at MySQL"
85+
// taken at module-load time, alongside sequelizeV2Mirror construction.
86+
//
87+
// mirrorDBEnabledV2() must stay consistent with the backend that
88+
// sequelizeV2Mirror writes to. sequelizeV2Mirror is built ONCE above from
89+
// either the MySQL config (v2Mirror) or the SQLite fallback (v2MirrorTest)
90+
// and is never rebuilt - runtime config changes cannot move an already-open
91+
// Sequelize instance to a different target. If we re-read the live config
92+
// on every mirrorDBEnabledV2() call (the previous behaviour), a runtime
93+
// config change could flip enablement true while writes still go to the
94+
// originally-selected SQLite fallback, silently desyncing the mirror.
95+
// See PR #1585 bugbot finding "Mirror enable check desynchronizes mirror
96+
// target".
97+
const v2MirrorTargetsMysql = !!mysqlMirrorConfigured;
98+
7699
// Test-only override. Tests set this to force mirror-enabled behaviour
77100
// against the SQLite fallback sequelizeV2Mirror so backfill / reconnect
78101
// code paths can be exercised without a live MySQL instance. null means
@@ -107,10 +130,15 @@ export const mirrorDBEnabledV2 = () => {
107130
if (mirrorEnabledTestOverrideV2 !== null) {
108131
return mirrorEnabledTestOverrideV2;
109132
}
110-
// Read config dynamically so any runtime reload is reflected here
111-
// (symmetric with V1's mirrorDBEnabled and with the setupNeverRan gate
112-
// in safeMirrorDbHandlerV2 below).
113-
return isMysqlMirrorConfiguredForReconnectV2();
133+
// Use the module-load snapshot so this check stays consistent with the
134+
// backend that sequelizeV2Mirror was actually constructed against.
135+
// Reading the live config here would allow the check to drift away from
136+
// the Sequelize instance after a runtime config change, causing writes
137+
// intended for MySQL to silently land in the SQLite fallback (or vice
138+
// versa). The setupNeverRan gate in safeMirrorDbHandlerV2 below still
139+
// reads live config for its own purpose (deciding whether to retry the
140+
// reconnect setup path).
141+
return v2MirrorTargetsMysql;
114142
};
115143

116144
// Test-only. DO NOT call from production code. Passing `null` restores the
@@ -128,6 +156,23 @@ export const __setMirrorSetupSucceededForTestsV2 = (value) => {
128156
v2MirrorSetupSucceeded = value;
129157
};
130158

159+
// Test-only. DO NOT call from production code. Forces
160+
// isMysqlMirrorConfiguredForReconnectV2() to return a specific boolean so
161+
// tests can exercise the reconnect setup / skip-callback guards without a
162+
// live MySQL instance or a real MIRROR_DB config. null restores live behaviour.
163+
export const __setMysqlConfiguredForReconnectForTestsV2 = (value) => {
164+
mysqlConfiguredForReconnectOverrideV2 = value;
165+
};
166+
167+
// Test-only. DO NOT call from production code. Resets the internal
168+
// auth-state machine back to 'unknown' so tests that intentionally leave
169+
// the state in 'disconnected' can reset it in an afterEach hook and avoid
170+
// leaking reconnect behaviour into downstream specs running in the same
171+
// mocha session.
172+
export const __resetMirrorAuthStateForTestsV2 = () => {
173+
v2MirrorAuthState = 'unknown';
174+
};
175+
131176
/**
132177
* Validate that V1 and V2 mirror database names are different when both are configured.
133178
* Throws an error if both V1 and V2 MIRROR_DB.DB_NAME are set to the same non-empty value.
@@ -234,6 +279,19 @@ export const safeMirrorDbHandlerV2 = (callback) => {
234279
await v2ReconnectBackfillPromise;
235280
}
236281

282+
// If MySQL is configured but the one-time setup (CREATE DATABASE
283+
// + migrations) has not yet completed, the mirror tables don't
284+
// exist. Running the callback would produce a confusing
285+
// "table doesn't exist" error that gets swallowed by the catch
286+
// below. Skip quietly - the next authenticate success will
287+
// re-enter startV2ReconnectBackfill and retry setup.
288+
if (
289+
isMysqlMirrorConfiguredForReconnectV2() &&
290+
!v2MirrorSetupSucceeded
291+
) {
292+
return;
293+
}
294+
237295
try {
238296
await callback();
239297
} catch (e) {

‎tests/integration/mirror-reconnect.spec.js‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import {
1212
safeMirrorDbHandler,
1313
__setMirrorEnabledForTests,
1414
__setMirrorSetupSucceededForTests,
15+
__setMysqlConfiguredForReconnectForTests,
16+
__resetMirrorAuthStateForTests,
1517
} from '../../src/database/index.js';
1618
import { ProjectMirror } from '../../src/models/projects/projects.model.mirror.js';
1719

@@ -52,6 +54,12 @@ describe('Mirror Reconnect and Orphan Sweep (V1)', function () {
5254
afterEach(async function () {
5355
__setMirrorEnabledForTests(null);
5456
__setMirrorSetupSucceededForTests(false);
57+
__setMysqlConfiguredForReconnectForTests(null);
58+
// Reset auth-state machine so downstream specs running in the same
59+
// mocha session (project.spec.js, unit.spec.js, etc.) don't observe a
60+
// leaked 'disconnected' state and enter the reconnect path on their
61+
// first mirror write.
62+
__resetMirrorAuthStateForTests();
5563
sinon.restore();
5664

5765
// Clean both databases between tests. We clear a minimal set of tables
@@ -217,5 +225,44 @@ describe('Mirror Reconnect and Orphan Sweep (V1)', function () {
217225
expect(mirrorAfter, 'reconnect backfill must recover missing row').to.not
218226
.be.null;
219227
});
228+
229+
// Placed last in this block because it intentionally leaves
230+
// mirrorAuthState in the 'disconnected' state (startReconnectBackfill
231+
// does so on setup failure). Running another reconnect-related test
232+
// after this one would observe that stale state and behave differently.
233+
it('should skip the callback when MySQL is configured but setup has not completed yet', async function () {
234+
// Regression guard for the post-reconnect skip-callback branch in
235+
// safeMirrorDbHandler. When the mirror is "enabled" and authenticate()
236+
// succeeds but the one-time setup (CREATE DATABASE + migrations) has
237+
// not yet completed, running the callback would hit tables that don't
238+
// exist and produce a confusing swallowed error. The handler must
239+
// skip the callback silently in that window. Symmetric with V2.
240+
//
241+
// Forces both prongs of the guard true:
242+
// - isMysqlMirrorConfiguredForReconnect() -> true
243+
// - mirrorSetupSucceeded -> false
244+
__setMysqlConfiguredForReconnectForTests(true);
245+
__setMirrorSetupSucceededForTests(false);
246+
247+
// Happy-path authenticate so the handler reaches the guard without
248+
// going through the disconnected-catch branch. The handler will first
249+
// call startReconnectBackfill (because setupNeverRan is true), which
250+
// calls prepareMysqlMirror. prepareMysqlMirror reads config.yaml
251+
// directly (not via the override above) and, with no real MIRROR_DB
252+
// in the test config, returns false. That leaves mirrorSetupSucceeded
253+
// still false, so the new guard trips.
254+
sinon.stub(sequelizeMirror, 'authenticate').resolves();
255+
256+
let cbRan = false;
257+
await safeMirrorDbHandler(async () => {
258+
cbRan = true;
259+
});
260+
await new Promise((r) => setTimeout(r, 100));
261+
262+
expect(
263+
cbRan,
264+
'callback must not run while MySQL setup is still pending',
265+
).to.equal(false);
266+
});
220267
});
221268
});

‎tests/v2/integration/mirror-reconnect-v2.spec.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import {
1212
safeMirrorDbHandlerV2,
1313
__setMirrorEnabledForTestsV2,
1414
__setMirrorSetupSucceededForTestsV2,
15+
__setMysqlConfiguredForReconnectForTestsV2,
16+
__resetMirrorAuthStateForTestsV2,
1517
} from '../../../src/database/v2/index.js';
1618
import { loggerV2 } from '../../../src/config/logger.js';
1719
import {
@@ -65,6 +67,11 @@ describe('Mirror Reconnect and Orphan Sweep (V2)', function () {
6567
afterEach(async function () {
6668
__setMirrorEnabledForTestsV2(null);
6769
__setMirrorSetupSucceededForTestsV2(false);
70+
__setMysqlConfiguredForReconnectForTestsV2(null);
71+
// Reset auth-state machine so downstream specs running in the same
72+
// mocha session don't observe a leaked 'disconnected' state and enter
73+
// the reconnect path on their first mirror write.
74+
__resetMirrorAuthStateForTestsV2();
6875
sinon.restore();
6976

7077
for (const t of ['program', 'organizations']) {
@@ -266,5 +273,44 @@ describe('Mirror Reconnect and Orphan Sweep (V2)', function () {
266273
loggerSpy.restore();
267274
backfillSpy.restore();
268275
});
276+
277+
// Placed last in this block because it intentionally leaves
278+
// v2MirrorAuthState in the 'disconnected' state (startV2ReconnectBackfill
279+
// does so on setup failure). Running another reconnect-related test
280+
// after this one would observe that stale state and behave differently.
281+
it('should skip the callback when MySQL is configured but setup has not completed yet', async function () {
282+
// Regression guard for the post-reconnect skip-callback branch in
283+
// safeMirrorDbHandlerV2. When the mirror is "enabled" and
284+
// authenticate() succeeds but the one-time setup (CREATE DATABASE +
285+
// migrations) has not yet completed, running the callback would hit
286+
// tables that don't exist and produce a confusing swallowed error.
287+
// The handler must skip the callback silently in that window.
288+
//
289+
// Forces both prongs of the guard true:
290+
// - isMysqlMirrorConfiguredForReconnectV2() -> true
291+
// - v2MirrorSetupSucceeded -> false
292+
__setMysqlConfiguredForReconnectForTestsV2(true);
293+
__setMirrorSetupSucceededForTestsV2(false);
294+
295+
// Happy-path authenticate so the handler reaches the guard without
296+
// going through the disconnected-catch branch. The handler will first
297+
// call startV2ReconnectBackfill (because setupNeverRan is true), which
298+
// in turn calls prepareMysqlMirrorV2. prepareMysqlMirrorV2 reads
299+
// config.yaml directly (not via the override above) and, with no real
300+
// MIRROR_DB in the test config, returns false. That leaves
301+
// v2MirrorSetupSucceeded still false, so the new guard trips.
302+
sinon.stub(sequelizeV2Mirror, 'authenticate').resolves();
303+
304+
let cbRan = false;
305+
await safeMirrorDbHandlerV2(async () => {
306+
cbRan = true;
307+
});
308+
await new Promise((r) => setTimeout(r, 100));
309+
310+
expect(
311+
cbRan,
312+
'callback must not run while MySQL setup is still pending',
313+
).to.equal(false);
314+
});
269315
});
270316
});

0 commit comments

Comments
 (0)