From b6ce2ac6294d577fc1384d604a3b770dcdfbd6d3 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Sun, 8 Nov 2026 14:36:32 -0500 Subject: [PATCH] test(app): pin the lock-error sink of redisStores, the deadline cap of redisLocks and the hundred-per-page lister A Stryker pass over src/adapters/app/*.ts, src/server/{node,main}.ts and src/platform/index.ts (580 mutants, 96.55 %) left three survivors that change what the App does rather than what it logs, and no test noticed: - src/server/node.ts:48 `redisLocks(client, { sink })` -> `{}`: a renewal or release that fails would be swallowed instead of logged. stores.test.ts now drives a failing EVAL through storesFromEnv and expects the line. - src/adapters/app/redis.ts:83 `deadline - Date.now()` -> `+`: the last wait would no longer be cut to the deadline, so withLock could sleep up to a second past waitMs. The default-wait test now expects the clock to end exactly on the deadline rather than at or after it. - src/adapters/app/cron.ts:37/47 `{ per_page: 100 }` -> `{}`: the lister would page thirty at a time, several times the requests of #641's intent. The paging test now records the per_page of every request. The other survivors are equivalent at their span (Response's default status, a falsy default, platform() falling back to the same value, log text, the token prefix slice, the private-key trim the JWT signer repeats) or need the real-Redis describe that CI runs with REDIS_TEST_URL (the Lua literals). Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- __tests__/adapters/app/cron.test.ts | 7 ++++++- __tests__/adapters/app/redis.test.ts | 5 +++-- __tests__/server/stores.test.ts | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/__tests__/adapters/app/cron.test.ts b/__tests__/adapters/app/cron.test.ts index 5400cdbe..e5ab570a 100644 --- a/__tests__/adapters/app/cron.test.ts +++ b/__tests__/adapters/app/cron.test.ts @@ -188,9 +188,11 @@ describe('appInstallationLister', () => { const api = 'https://ghes.example/api/v3' const { privateKey } = generateKeyPairSync('rsa', { modulusLength: 2048, privateKeyEncoding: { type: 'pkcs1', format: 'pem' }, publicKeyEncoding: { type: 'pkcs1', format: 'pem' } }) - it('lists every repository of every installation over every page, skipping suspended installations and archived repositories', async () => { + it('lists every repository of every installation over every page, a hundred at a time, skipping suspended installations and archived repositories', async () => { + const pageSizes: string[] = [] server.use( http.get(`${api}/app/installations`, ({ request }) => { + pageSizes.push(`installations ${new URL(request.url).searchParams.get('per_page')}`) const p = new URL(request.url).searchParams.get('page') ?? '1' const headers: Record = { 'Content-Type': 'application/json' } if (p === '1') { @@ -203,6 +205,7 @@ describe('appInstallationLister', () => { }), http.post(`${api}/app/installations/:id/access_tokens`, ({ params }) => new Response(JSON.stringify({ token: `tok-${params.id}`, expires_at: new Date(Date.now() + 3_600_000).toISOString(), permissions: {}, repository_selection: 'all' }), { status: 201, headers: { 'Content-Type': 'application/json' } })), http.get(`${api}/installation/repositories`, ({ request }) => { + pageSizes.push(`repositories ${new URL(request.url).searchParams.get('per_page')}`) const token = request.headers.get('authorization') const repos = token === 'token tok-1' ? [{ name: 'api', full_name: 'acme/api', owner: { login: 'acme' }, archived: false }, { name: 'old', full_name: 'acme/old', owner: { login: 'acme' }, archived: true }] @@ -217,6 +220,8 @@ describe('appInstallationLister', () => { } expect(out.map(t => `${t.installationId}:${t.repository.full_name}`)).toEqual(['1:acme/api', '2:globex/core']) + // the largest page GitHub serves: an App on many installations or repositories lists them in the fewest requests + expect(pageSizes).toEqual(['installations 100', 'installations 100', 'repositories 100', 'repositories 100']) }) it.each([ diff --git a/__tests__/adapters/app/redis.test.ts b/__tests__/adapters/app/redis.test.ts index 8781c685..1643de30 100644 --- a/__tests__/adapters/app/redis.test.ts +++ b/__tests__/adapters/app/redis.test.ts @@ -125,7 +125,7 @@ describe('redisLocks', () => { expect(sleep.mock.calls).toEqual([[600], [1000], [1000]]) }) - it('waits five minutes for a held lock by default before giving up', async () => { + it('waits five minutes for a held lock by default before giving up, and never sleeps past that deadline', async () => { const client = fakeRedis() let now = 1_000_000 vi.spyOn(Date, 'now').mockImplementation(() => now) @@ -136,7 +136,8 @@ describe('redisLocks', () => { await expect(redisLocks(client).withLock('k', async () => {})).rejects.toThrow('could not lock k within 300000ms') expect(lockWaitMs).toBe(300_000) - expect(now).toBeGreaterThanOrEqual(1_000_000 + lockWaitMs) + // the last wait is cut to what is left of the deadline, so the wait ends on it, not up to a second after + expect(now).toBe(1_000_000 + lockWaitMs) }) it('renews the lock every third of the ttl while fn runs, and stops once it returns', async () => { diff --git a/__tests__/server/stores.test.ts b/__tests__/server/stores.test.ts index 95775cf3..eae2e5c9 100644 --- a/__tests__/server/stores.test.ts +++ b/__tests__/server/stores.test.ts @@ -67,6 +67,21 @@ describe('storesFromEnv', () => { await stores.close() expect(client.close).toHaveBeenCalled() }) + + it('hands the sink to the redis locks, so a release that fails is logged rather than lost', async () => { + const lines: Record[] = [] + const stores = storesFromEnv({ REDIS_URL: 'redis://x' }, line => lines.push(line)) + await stores.ready + const { createClient } = await import('redis') + const client = vi.mocked(createClient).mock.results.at(-1)!.value as { eval: () => Promise } + client.eval = async () => { + throw new Error('offline') + } + + await expect(stores.locks.withLock('o/r#1', async () => 'handled')).resolves.toBe('handled') + + expect(lines).toContainEqual({ level: 'error', message: 'redis lock o/r#1: release failed: offline' }) + }) }) describe('the server with stores', () => {