diff --git a/server/src/model/teams/teamVoice.db.js b/server/src/model/teams/teamVoice.db.js index 6129142..713f2c6 100644 --- a/server/src/model/teams/teamVoice.db.js +++ b/server/src/model/teams/teamVoice.db.js @@ -24,8 +24,20 @@ const { query } = require('../../utils/db') // enter, with no error anywhere. const DISCORD_PROVIDER = 'discord' +// Deliberately WITHOUT `i.team_id`, and this is not tidiness. +// +// The two queries below join `teams` and already select `t.id AS team_id`, so +// including the integration row's copy produces two result columns with the same +// name — which the `mariadb` driver refuses outright: "Error in results, duplicate +// field name `team_id`". Every caller sees the whole pass fail, and no unit test +// can see it, because they stub this layer. +// +// It would also be the WRONG column even if the driver allowed it: `desiredTeams` +// LEFT JOINs, so `i.team_id` is NULL for exactly the Teams that have no channel +// yet — the create case, where knowing the Team's id matters most. The two queries +// that do not join `teams` ask for it explicitly. const COLUMNS = ` - i.id, i.team_id, i.platform, i.resource, i.external_ref, i.role_ref, + i.id, i.platform, i.resource, i.external_ref, i.role_ref, i.state, i.remove_after, i.last_error, i.synced_at, i.updated_at` /** @@ -112,7 +124,7 @@ async function discordSubjectsFor(teamId) { /** Every row for a platform, with the Team's name — the admin panel's listing. */ async function listForPlatform(platform, resource) { return query( - `SELECT ${COLUMNS}, t.name AS team_name, t.slug AS team_slug, + `SELECT i.team_id, ${COLUMNS}, t.name AS team_name, t.slug AS team_slug, t.display_name_override, t.status AS team_status, t.hidden AS team_hidden, t.member_count, t.linked_count FROM team_integrations i @@ -125,7 +137,7 @@ async function listForPlatform(platform, resource) { async function getForTeam(teamId, platform, resource) { const rows = await query( - `SELECT ${COLUMNS} FROM team_integrations i + `SELECT i.team_id, ${COLUMNS} FROM team_integrations i WHERE i.team_id = ? AND i.platform = ? AND i.resource = ? LIMIT 1`, [Number(teamId), platform, resource], ) diff --git a/server/src/utils/teamVoiceSync.js b/server/src/utils/teamVoiceSync.js index b8b1ecd..0f90aca 100644 --- a/server/src/utils/teamVoiceSync.js +++ b/server/src/utils/teamVoiceSync.js @@ -46,6 +46,9 @@ const DEBOUNCE_MS = 30_000 let running = false let rerun = false +// The promise of the pass currently in flight, so a second caller can await the +// same work rather than be told there is none. +let inFlight = null let lastRunAt = 0 let debounceTimer = null @@ -311,12 +314,36 @@ async function runOnce(reason) { return summary } -/** Run now, awaited, with the lock held. The admin "sync now" button uses this. */ +/** + * Run now, awaited. The admin "Sync now" button uses this. + * + * **A pass already in flight is JOINED, not refused**, the same choice + * `teamSync.reconcileNow` makes and for the same reason: the caller wants "Discord + * now matches", and a pass that started a moment ago delivers exactly that. + * + * Refusing was the first thing written here and it was wrong in a way only the rig + * showed. Saving the settings with voice switched on asks for a pass; an operator + * who then presses Sync now — which is the obvious next thing to do — got + * `ran: false, reason: "a pass is already running"`, and the panel dutifully told + * them **"Nothing was done"** while the pass they had just triggered was busy + * creating their channels. + */ async function passNow(reason = 'manual') { - if (running) { - rerun = true - return { ran: false, reason: 'a pass is already running', joined: true } + if (inFlight) return inFlight + inFlight = execute(reason) + try { + return await inFlight + } finally { + inFlight = null + if (rerun) { + rerun = false + request({ reason: 'continuation' }) + } } +} + +/** The body `passNow` guards. Never throws — see the file header. */ +async function execute(reason) { running = true try { const result = await runOnce(reason) @@ -329,10 +356,6 @@ async function passNow(reason = 'manual') { return { ran: false, reason: err.message } } finally { running = false - if (rerun) { - rerun = false - request({ reason: 'continuation' }) - } } } @@ -387,6 +410,7 @@ function _reset() { stop() running = false rerun = false + inFlight = null lastRunAt = 0 lastPassResult = { at: null, ran: false, reason: 'no pass has run yet' } } diff --git a/server/test/teamVoice.test.js b/server/test/teamVoice.test.js index f723011..996d21a 100644 --- a/server/test/teamVoice.test.js +++ b/server/test/teamVoice.test.js @@ -264,6 +264,61 @@ test('a category ref that is not a channel id is never stored', async () => { await assert.rejects(() => settings.setCategoryRef('../../etc/passwd')) }) +// ── The SQL itself ───────────────────────────────────────────────────────── + +test('no query selects the same result column twice', async () => { + // A defect the live rig found and no stubbed test could: `desiredTeams` and + // `holdersWithoutClaim` both select `t.id AS team_id`, and the shared column + // list used to add `i.team_id` beside it. The `mariadb` driver refuses a result + // set with a repeated field name outright — "Error in results, duplicate field + // name `team_id`" — so every pass failed at its first query, on a code path + // every other test in this file stubs. + // + // The check runs against the INTERPOLATED sql, captured from a fake `query`, + // not against the source text: in the source the shared list is still a + // `${COLUMNS}` placeholder, so a reader — and a first attempt at this test — + // cannot see the duplicate at all. + const db = require('../src/utils/db') + const realQuery = db.query + const seenSql = [] + db.query = async (sql) => { seenSql.push(sql); return [] } + + // Re-require: the module destructures `query` at load time, so patching after + // it is already in the cache would leave it holding the real one. + delete require.cache[require.resolve('../src/model/teams/teamVoice.db.js')] + /* eslint-disable-next-line global-require */ + const freshDb = require('../src/model/teams/teamVoice.db.js') + + try { + await freshDb.desiredTeams({ platform: 'discord', resource: 'voice', minMembers: 5 }) + await freshDb.holdersWithoutClaim({ platform: 'discord', resource: 'voice', minMembers: 5 }) + await freshDb.listForPlatform('discord', 'voice') + await freshDb.getForTeam(1, 'discord', 'voice') + await freshDb.discordSubjectsFor(1) + } finally { + db.query = realQuery + delete require.cache[require.resolve('../src/model/teams/teamVoice.db.js')] + } + + assert.equal(seenSql.length, 5, 'every query in the file should have been captured') + + for (const sql of seenSql) { + const selectList = sql.slice(sql.search(/SELECT/i) + 6, sql.search(/\sFROM\s/i)) + const names = selectList + .split(',') + .map((piece) => piece.trim()) + .filter(Boolean) + .map((piece) => { + const aliased = piece.match(/\sAS\s+(\w+)$/i) + if (aliased) return aliased[1].toLowerCase() + return piece.replace(/^DISTINCT\s+/i, '').replace(/^\w+\./, '').toLowerCase() + }) + const seen = new Set() + const duplicated = names.filter((name) => (seen.has(name) ? true : (seen.add(name), false))) + assert.deepEqual(duplicated, [], `duplicate result column "${duplicated[0]}" in: ${selectList.trim()}`) + } +}) + test('a garbage category ref already in the database reads as unset', async () => { store.set('teams_voice_category_ref', 'nonsense') assert.equal(await settings.categoryRef(), null) diff --git a/server/test/teamVoiceSync.test.js b/server/test/teamVoiceSync.test.js index ede9010..6e3683d 100644 --- a/server/test/teamVoiceSync.test.js +++ b/server/test/teamVoiceSync.test.js @@ -307,6 +307,29 @@ test('a pass that throws is reported, not raised — it hangs off a background t assert.equal(sync.lastPass().ran, false) }) +test('a pass already in flight is JOINED, not refused', async () => { + // Found on the live rig. Saving the settings with voice switched on asks for a + // pass; an operator who then presses "Sync now" — the obvious next thing to do — + // got `ran: false, reason: "a pass is already running"`, and the panel told them + // **"Nothing was done"** while the pass they had just triggered was creating + // their channels. `teamSync.reconcileNow` joins for the same reason: the caller + // wants "Discord now matches", and a pass that started a moment ago delivers it. + let release + const gate = new Promise((resolve) => { release = resolve }) + let passes = 0 + patch(voice, 'plan', async () => { passes += 1; await gate; return plan }) + + const first = sync.passNow('first') + const second = sync.passNow('second') + release() + const [a, b] = await Promise.all([first, second]) + + assert.equal(passes, 1, 'the work was done once') + assert.equal(a.ran, true) + assert.equal(b.ran, true, 'the second caller got the real outcome, not a refusal') + assert.deepEqual(a, b) +}) + test('a pass records what it concluded, for the panel', async () => { plan.provision = [item()] await sync.passNow('test')