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],
   )

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) {
+  if (inFlight) return inFlight
-    rerun = true
+  inFlight = execute(reason)
-    return { ran: false, reason: 'a pass is already running', joined: true }
+  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' }
 }

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)

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')