From 8bdf69eb236fc668db68500aba396476eed0565a Mon Sep 17 00:00:00 2001 From: Arvin A <51036481+DeveloperTheExplorer@users.noreply.github.com> Date: Tue, 4 Aug 2026 12:42:20 +0200 Subject: [PATCH] refactor(core): Tidy agent-eval runner and rating-persistence internals (no-changelog) (#35424) --- .../agent-eval-rating.repository.test.ts | 3 +- .../agent-eval-rating.repository.ee.ts | 4 ++ .../agent-eval-runner.service.test.ts | 29 ++++++++++++++- .../agent-evals/agent-eval-runner.service.ts | 37 ++++++++++++++----- packages/cli/vitest.config.integration.ts | 4 ++ 5 files changed, 65 insertions(+), 12 deletions(-) diff --git a/packages/@n8n/db/src/repositories/__tests__/agent-eval-rating.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/agent-eval-rating.repository.test.ts index 5299c75ba5a..58d668734ee 100644 --- a/packages/@n8n/db/src/repositories/__tests__/agent-eval-rating.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/agent-eval-rating.repository.test.ts @@ -1,4 +1,5 @@ import { Container } from '@n8n/di'; +import { In } from '@n8n/typeorm'; import type { Mock } from 'vitest'; import { AgentEvalRating } from '../../entities/agent-eval-rating.ee'; @@ -101,7 +102,7 @@ describe('AgentEvalRatingRepository', () => { expect(qb.where).toHaveBeenCalledWith('result.runId = :runId', { runId: 'run-1' }); // The superseded r-1 is never fetched, only the two winners. expect(entityManager.find.mock.calls[0]?.[1]).toEqual({ - where: { id: expect.objectContaining({ _value: ['r-2', 'r-3'] }) }, + where: { id: In(['r-2', 'r-3']) }, }); expect(latest.map((rating) => rating.id)).toEqual(['r-2', 'r-3']); }); diff --git a/packages/@n8n/db/src/repositories/agent-eval-rating.repository.ee.ts b/packages/@n8n/db/src/repositories/agent-eval-rating.repository.ee.ts index f59739c317a..1bfb9a87112 100644 --- a/packages/@n8n/db/src/repositories/agent-eval-rating.repository.ee.ts +++ b/packages/@n8n/db/src/repositories/agent-eval-rating.repository.ee.ts @@ -41,6 +41,10 @@ export class AgentEvalRatingRepository extends Repository { /** * Newest rating per result in a run. Reduced here, not in SQL: `DISTINCT ON` is * Postgres-only and `MAX(createdAt)` returns both rows on a millisecond tie. + * + * The trailing `rating.id` tiebreak buys reproducibility, not recency: ids are + * random nanoids, so on a same-millisecond tie the winner is arbitrary but at + * least stable across calls. Picking the later one needs a monotonic column. */ async findLatestByRunId(runId: string): Promise { // Ids only — whole rows would load every superseded `correction` just to drop it. diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts index c19a11668e1..f289c0286b1 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval-runner.service.test.ts @@ -384,6 +384,29 @@ describe('AgentEvalRunnerService', () => { 'empty_input', expect.anything(), ); + // Screened out before the queue: an unusable case must never hold a slot. + expect(concurrencyControl.throttle).not.toHaveBeenCalled(); + expect(resultRepository.markAsRunning).not.toHaveBeenCalled(); + }); + + it('still completes the run when recording an empty-input case fails', async () => { + seedFor( + [ + { id: 'row-1', question: ' ' }, + { id: 'row-2', question: 'Q2' }, + ], + { success: 1, error: 1 }, + ); + // This write sits outside `runCase`'s catch, so left unguarded the rejection + // would settle as a dispatch failure and error an otherwise-successful run. + resultRepository.markAsError.mockRejectedValueOnce(new Error('db unavailable')); + evalAgentExecutionService.executeWithLlmMock.mockResolvedValue(successExec() as never); + + const { finished } = await service.startRun('ds-1', 'proj-1', user); + await finished; + + expect(runRepository.markAsCompleted).toHaveBeenCalled(); + expect(runRepository.markAsError).not.toHaveBeenCalled(); }); it('pages through every row when the table exceeds one page', async () => { @@ -682,8 +705,12 @@ describe('AgentEvalRunnerService', () => { await finished; expect(concurrencyControl.throttle).toHaveBeenCalledTimes(2); + // Prefixed so a throttled id is attributable to agent evals in log streaming. expect(concurrencyControl.throttle).toHaveBeenCalledWith( - expect.objectContaining({ mode: 'evaluation' }), + expect.objectContaining({ + mode: 'evaluation', + executionId: expect.stringMatching(/^agent-eval:/), + }), ); expect(concurrencyControl.release).toHaveBeenCalledTimes(2); expect(concurrencyControl.release).toHaveBeenCalledWith({ mode: 'evaluation' }); diff --git a/packages/cli/src/modules/agent-evals/agent-eval-runner.service.ts b/packages/cli/src/modules/agent-evals/agent-eval-runner.service.ts index 4c4b745a8b9..e2a862f5b0c 100644 --- a/packages/cli/src/modules/agent-evals/agent-eval-runner.service.ts +++ b/packages/cli/src/modules/agent-evals/agent-eval-runner.service.ts @@ -263,6 +263,22 @@ export class AgentEvalRunnerService { await this.resultRepository.markAsCancelled(resultRow.id); }; + // An empty input can't produce an execution, so its verdict is recorded + // without taking a queue slot. Self-contained like `runCase`: one unusable + // case must not fail the run just because recording it failed. + const markEmptyInput = async (resultRow: AgentEvalResult) => { + try { + await this.resultRepository.markAsError(resultRow.id, 'empty_input', { + message: 'Case has no value in the mapped input column.', + }); + } catch (error) { + this.logger.error( + `[AgentEvalRunner] Could not record empty input for case ${resultRow.id}`, + { error: error instanceof Error ? error.message : String(error) }, + ); + } + }; + const deadline = this.startRunDeadline(abort); // The stop reads/writes around `runCase` sit outside its safety net, so @@ -278,9 +294,16 @@ export class AgentEvalRunnerService { return; } + // Screened before the slot, after the stop: a cancel still outranks it. + if (resolvedCase.input.trim().length === 0) { + await markEmptyInput(resultRow); + return; + } + // The helper owns release/remove when the run stops while queued; // the finally owns release once a slot is held. - const executionId = `${runId}-case-${index}`; + // Prefixed so the id is attributable in `n8n.execution.throttled`. + const executionId = `agent-eval:${runId}-case-${index}`; if (!(await this.acquireEvaluationSlot(executionId, abort.signal))) { await stopCase(resultRow); return; @@ -486,7 +509,8 @@ export class AgentEvalRunnerService { * Execute one case and persist its result. Returns the case's token usage. * Fully self-contained: a thrown execution or DB error is converted into a * per-case error so one case can never abort the batch or leave its result - * stuck `running`. + * stuck `running`. Assumes a non-empty input — the caller screens those out + * before taking a queue slot. */ private async runCase( resultRow: AgentEvalResult, @@ -494,13 +518,6 @@ export class AgentEvalRunnerService { ctx: { agentId: string; projectId: string; user: User; timeoutMs?: number }, ): Promise { try { - if (resolvedCase.input.trim().length === 0) { - await this.resultRepository.markAsError(resultRow.id, 'empty_input', { - message: 'Case has no value in the mapped input column.', - }); - return undefined; - } - await this.resultRepository.markAsRunning(resultRow.id); const execResult = await this.evalAgentExecutionService.executeWithLlmMock( @@ -568,7 +585,7 @@ export class AgentEvalRunnerService { // Validate the mapping against the live columns so a renamed/deleted column // fails loudly here instead of silently turning every case into an - // "empty input" error at execution time. + // "empty input" error once the run starts. const columnNames = new Set( (await this.dataTableService.getColumns(dataTableId, tableProjectId)).map((c) => c.name), ); diff --git a/packages/cli/vitest.config.integration.ts b/packages/cli/vitest.config.integration.ts index 6de2b9fc5da..1a2ae332441 100644 --- a/packages/cli/vitest.config.integration.ts +++ b/packages/cli/vitest.config.integration.ts @@ -6,5 +6,9 @@ export default mergeConfig(baseConfig, { test: { include: ['test/integration/**/*.test.ts', 'src/**/*.integration.test.ts'], testTimeout: 10_000, + // Loading modules and building the entity schema in `beforeAll` routinely + // runs past Vitest's 10s hook default, which reads as broken coverage + // (whole suite skipped) rather than a slow setup. + hookTimeout: 30_000, }, });