From 8201ad3abe47644fa86f81ea6b32dca90cb225f0 Mon Sep 17 00:00:00 2001 From: andypalmi Date: Tue, 20 Oct 2026 19:13:18 +0200 Subject: [PATCH 1/2] chore: Lower CI test times - Run the forge unit tests with mocha --parallel (4 workers), giving each worker its own Postgres database - Hash passwords with bcrypt cost 4 under test - Share compiled route schemas between app instances in a test process - Stop the test jobs waiting on the TypeScript drift check; the summary job still requires it - Fix tests that the faster runs made flaky: order audit entries by id, compare device group members regardless of order - Claim an MCP refresh token rotation before rotating the token, so two simultaneous refreshes cannot both lose the compare-and-swap --- .github/workflows/tests.yml | 5 -- forge/db/controllers/AccessToken.js | 43 +++++++++++------ package.json | 4 +- test/lib/fast-bcrypt.js | 6 +++ test/lib/fastify-schema-cache.js | 47 +++++++++++++++++++ .../flowforge-test-utils/index.js | 4 ++ .../api/applicationDeviceGroups_spec.js | 4 +- test/unit/forge/routes/auth/index_spec.js | 2 +- 8 files changed, 90 insertions(+), 25 deletions(-) create mode 100644 test/lib/fast-bcrypt.js create mode 100644 test/lib/fastify-schema-cache.js diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2af5198698..68493bf815 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -65,7 +65,6 @@ jobs: name: Backend tests needs: - check-changes - - check-ts-drift runs-on: ubuntu-latest timeout-minutes: 20 steps: @@ -94,7 +93,6 @@ jobs: name: Postgres tests needs: - check-changes - - check-ts-drift runs-on: ubuntu-latest timeout-minutes: 45 services: @@ -138,7 +136,6 @@ jobs: name: UI unit tests needs: - check-changes - - check-ts-drift runs-on: ubuntu-latest steps: - name: Checkout @@ -159,7 +156,6 @@ jobs: name: UI OS tests needs: - check-changes - - check-ts-drift runs-on: ubuntu-latest services: email-service: @@ -205,7 +201,6 @@ jobs: name: UI EE tests needs: - check-changes - - check-ts-drift runs-on: ubuntu-latest services: email-service: diff --git a/forge/db/controllers/AccessToken.js b/forge/db/controllers/AccessToken.js index 9984527a16..9d8bab9aba 100644 --- a/forge/db/controllers/AccessToken.js +++ b/forge/db/controllers/AccessToken.js @@ -1,4 +1,4 @@ -const { Op } = require('sequelize') +const { Op, UniqueConstraintError } = require('sequelize') const mcpToolPermissions = require('../../lib/mcpToolPermissions') const { generateToken, generateNumericToken, sha256, randomPhrase, DEFAULT_TOKEN_SESSION_EXPIRY, DEFAULT_REFRESH_TOKEN_EXPIRY } = require('../utils') @@ -495,26 +495,39 @@ module.exports = { const token = generateToken(32, prefix) const newRefreshToken = generateToken(32, prefix) const expiresAt = capToGrant(Date.now() + DEFAULT_TOKEN_SESSION_EXPIRY, grantExpiresAtMs) - // Compare-and-swap: matches only while the token is current, so of two - // simultaneous refreshes exactly one rotates. - const [rotatedCount] = await app.db.models.AccessToken.update( - { - token, - expiresAt, - refreshToken: newRefreshToken, - refreshTokenExpiresAt: capToGrant(Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY, grantExpiresAtMs) - }, - { where: { refreshToken: existingToken.refreshToken } } - ) - if (rotatedCount > 0) { - await app.db.models.AccessTokenRefreshRotation.create({ + // Claim the rotation by recording it before rotating the row: the unique tokenHash + // index lets exactly one of two simultaneous refreshes insert it, and the record + // already exists when the other one looks it up below. + let rotation = null + try { + rotation = await app.db.models.AccessTokenRefreshRotation.create({ tokenHash: existingToken.refreshToken, rotatedAt: new Date(), AccessTokenId: existingToken.id }) + } catch (err) { + if (!(err instanceof UniqueConstraintError)) { + throw err + } + } + if (rotation) { + const [rotatedCount] = await app.db.models.AccessToken.update( + { + token, + expiresAt, + refreshToken: newRefreshToken, + refreshTokenExpiresAt: capToGrant(Date.now() + DEFAULT_REFRESH_TOKEN_EXPIRY, grantExpiresAtMs) + }, + { where: { refreshToken: existingToken.refreshToken } } + ) + if (rotatedCount === 0) { + // The grant was revoked since it was read. + await rotation.destroy() + return null + } return { token, expiresAt, refreshToken: newRefreshToken } } - // Lost the swap: the presented token is now rotated out, resolve it below. + // Lost the claim: the presented token is now rotated out, resolve it below. } const retired = await app.db.models.AccessToken.byRotatedRefreshToken(refreshToken) diff --git a/package.json b/package.json index 8a31291f9a..8d865aece4 100644 --- a/package.json +++ b/package.json @@ -35,8 +35,8 @@ "lint:colors": "node scripts/lint-colors.js", "test": "npm-run-all --sequential lint test:unit test:system", "test:unit": "npm-run-all --sequential test:unit:forge test:unit:frontend", - "test:unit:forge": "mocha \"test/unit/forge/**/*_spec.js\" --timeout 10000 --node-option=unhandled-rejections=strict", - "test:unit:forge:ci": "mocha \"test/unit/forge/**/*_spec.js\" --bail --timeout 10000 --node-option=unhandled-rejections=strict", + "test:unit:forge": "mocha \"test/unit/forge/**/*_spec.js\" --require test/lib/fast-bcrypt.js --require test/lib/fastify-schema-cache.js --timeout 10000 --node-option=unhandled-rejections=strict", + "test:unit:forge:ci": "mocha \"test/unit/forge/**/*_spec.js\" --parallel --jobs 4 --require test/lib/fast-bcrypt.js --require test/lib/fastify-schema-cache.js --bail --timeout 10000 --node-option=unhandled-rejections=strict", "test:unit:frontend": "vitest run --config ./config/vitest.config.ts", "test:docs": "node test/e2e/docs/valid-links.js ./docs", "test:system": "mocha \"test/system/**/*_spec.js\" --timeout 10000 --node-option=unhandled-rejections=strict", diff --git a/test/lib/fast-bcrypt.js b/test/lib/fast-bcrypt.js new file mode 100644 index 0000000000..fdb7b0ae5e --- /dev/null +++ b/test/lib/fast-bcrypt.js @@ -0,0 +1,6 @@ +// Hashing at the production cost of 10 rounds dominates test CPU time. +// Tests only need valid hashes, so use the minimum cost. +const bcrypt = require('bcrypt') + +const hashSync = bcrypt.hashSync +bcrypt.hashSync = (value) => hashSync(value, 4) diff --git a/test/lib/fastify-schema-cache.js b/test/lib/fastify-schema-cache.js new file mode 100644 index 0000000000..e71d6a9410 --- /dev/null +++ b/test/lib/fastify-schema-cache.js @@ -0,0 +1,47 @@ +// Every test boots many app instances, and each one compiles the same route +// schemas from scratch. Share compiled validators and serializers between the +// instances in a process, keyed on the schema and compiler options. +const { createHash } = require('crypto') +const path = require('path') + +// Use the compilers fastify itself depends on, so behaviour matches the default +const fastifyPath = require.resolve('fastify') +const fromFastify = name => require(require.resolve(name, { paths: [path.dirname(fastifyPath)] })) +const AjvCompiler = fromFastify('@fastify/ajv-compiler') +const SerializerCompiler = fromFastify('@fastify/fast-json-stringify-compiler') + +const hash = value => createHash('sha1').update(JSON.stringify(value) ?? 'undefined').digest('hex') + +function cachedFactory (baseFactory, kind) { + const cache = new Map() + return function (externalSchemas, options) { + const compile = baseFactory(externalSchemas, options) + const prefix = kind + hash(externalSchemas) + hash(options) + return function (routeOptions) { + const key = prefix + hash(routeOptions.schema) + (routeOptions.httpPart || '') + (routeOptions.httpStatus || '') + (routeOptions.contentType || '') + let fn = cache.get(key) + if (!fn) { + fn = compile(routeOptions) + cache.set(key, fn) + } + return fn + } + } +} + +const compilersFactory = { + buildValidator: cachedFactory(AjvCompiler(), 'v'), + buildSerializer: cachedFactory(SerializerCompiler(), 's') +} + +const fastify = require(fastifyPath) +function fastifyWithSchemaCache (options = {}) { + return fastify({ + ...options, + schemaController: { ...options.schemaController, compilersFactory } + }) +} +Object.assign(fastifyWithSchemaCache, fastify) +fastifyWithSchemaCache.default = fastifyWithSchemaCache +fastifyWithSchemaCache.fastify = fastifyWithSchemaCache +require.cache[fastifyPath].exports = fastifyWithSchemaCache diff --git a/test/node_modules/flowforge-test-utils/index.js b/test/node_modules/flowforge-test-utils/index.js index dde513c4c2..444b6c6a87 100644 --- a/test/node_modules/flowforge-test-utils/index.js +++ b/test/node_modules/flowforge-test-utils/index.js @@ -38,6 +38,10 @@ async function setupApp (config = {}) { config.db.user = process.env.FF_TEST_DB_POSTGRES_USER || 'postgres' config.db.password = process.env.FF_TEST_DB_POSTGRES_PASSWORD || 'secret' config.db.database = process.env.FF_TEST_DB_POSTGRES_DATABASE || 'flowforge_test' + // Parallel mocha workers each need their own database + if (process.env.MOCHA_WORKER_ID !== undefined) { + config.db.database += `_w${process.env.MOCHA_WORKER_ID}` + } try { const { Client } = require('pg') diff --git a/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js b/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js index 28cd3f6f80..4571024193 100644 --- a/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js +++ b/test/unit/forge/ee/routes/api/applicationDeviceGroups_spec.js @@ -1077,8 +1077,8 @@ describe('Application Device Groups API', function () { // get the group from DB and verify it contains 2 devices (device1, device3) const updatedDeviceGroup2 = await app.db.models.DeviceGroup.byId(deviceGroup.hashid) updatedDeviceGroup2.should.have.property('Devices').and.have.length(2) - updatedDeviceGroup2.Devices[0].should.have.property('id', device1.id) - updatedDeviceGroup2.Devices[1].should.have.property('id', device3.id) + // DeviceGroup.byId does not order its Devices + updatedDeviceGroup2.Devices.map(d => d.id).sort((a, b) => a - b).should.eql([device1.id, device3.id]) }) it('Owner can set the list of devices in existing group', async function () { diff --git a/test/unit/forge/routes/auth/index_spec.js b/test/unit/forge/routes/auth/index_spec.js index f7a85957f0..6bc822ff33 100644 --- a/test/unit/forge/routes/auth/index_spec.js +++ b/test/unit/forge/routes/auth/index_spec.js @@ -1341,7 +1341,7 @@ describe('Accounts API', async function () { async function getLatestAuditEntry () { return app.db.models.AuditLog.findOne({ - order: [['createdAt', 'DESC']] + order: [['id', 'DESC']] }) } From b47a01459a6330dbd497a3d8cf4c24c49cd7f95f Mon Sep 17 00:00:00 2001 From: andypalmi Date: Wed, 21 Oct 2026 10:33:18 +0200 Subject: [PATCH 2/2] test: Cover the refresh rotation claim branches --- .../forge/db/controllers/AccessToken_spec.js | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/test/unit/forge/db/controllers/AccessToken_spec.js b/test/unit/forge/db/controllers/AccessToken_spec.js index e20620fa89..f1ddaa8490 100644 --- a/test/unit/forge/db/controllers/AccessToken_spec.js +++ b/test/unit/forge/db/controllers/AccessToken_spec.js @@ -1,4 +1,6 @@ -const should = require('should') // eslint-disable-line +const should = require('should') +const sinon = require('sinon') + const { sha256 } = require('../../../../../forge/db/utils') const mcpToolPermissions = require('../../../../../forge/lib/mcpToolPermissions') const setup = require('../setup') @@ -637,6 +639,47 @@ describe('AccessToken controller', function () { live.length.should.be.aboveOrEqual(1) }) + it('re-mints an access token when another refresh already claimed the rotation', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + const row = await app.db.models.AccessToken.byRefreshToken(original.refreshToken) + // Record the claim as a concurrent refresh would, between the lookup and the rotation. + await app.db.models.AccessTokenRefreshRotation.create({ + tokenHash: sha256(original.refreshToken), + rotatedAt: new Date(), + AccessTokenId: row.id + }) + + const result = await app.db.controllers.AccessToken.refreshToken(original.refreshToken) + should.exist(result) + result.should.not.have.property('replay') + should.not.exist(result.refreshToken) + should.exist(await app.db.controllers.AccessToken.getOrExpire(result.token)) + }) + + it('returns null and releases the claim when the grant is revoked mid-refresh', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + sinon.stub(app.db.models.AccessToken, 'update').resolves([0]) + try { + should.not.exist(await app.db.controllers.AccessToken.refreshToken(original.refreshToken)) + } finally { + app.db.models.AccessToken.update.restore() + } + ;(await app.db.models.AccessTokenRefreshRotation.count({ where: { tokenHash: sha256(original.refreshToken) } })).should.equal(0) + }) + + it('rethrows errors other than a lost claim', async function () { + const original = await createToken() + await setRowExpiry(original.refreshToken, { accessMs: -5000 }) + sinon.stub(app.db.models.AccessTokenRefreshRotation, 'create').rejects(new Error('db down')) + try { + await app.db.controllers.AccessToken.refreshToken(original.refreshToken).should.be.rejectedWith('db down') + } finally { + app.db.models.AccessTokenRefreshRotation.create.restore() + } + }) + describe('with a grant expiry', function () { const DAY = 24 * 60 * 60 * 1000