From 279ee1254cdaea04bdb6b9d82293e3117f94b41a Mon Sep 17 00:00:00 2001 From: Jake Turner Date: Wed, 11 Feb 2026 22:09:31 -0800 Subject: [PATCH] fix(Benchmark): improved error reporting and fix sysbench race condition --- admin/app/controllers/benchmark_controller.ts | 4 ++- admin/app/services/benchmark_service.ts | 36 ++++++++++++++++--- admin/docs/release-notes.md | 2 ++ admin/inertia/lib/api.ts | 14 ++++++-- admin/inertia/pages/settings/benchmark.tsx | 9 +++-- 5 files changed, 55 insertions(+), 10 deletions(-) diff --git a/admin/app/controllers/benchmark_controller.ts b/admin/app/controllers/benchmark_controller.ts index 58eaf18..b3e5343 100644 --- a/admin/app/controllers/benchmark_controller.ts +++ b/admin/app/controllers/benchmark_controller.ts @@ -179,7 +179,9 @@ export default class BenchmarkController { percentile: submitResult.percentile, }) } catch (error) { - return response.status(400).send({ + // Pass through the status code from the service if available, otherwise default to 400 + const statusCode = (error as any).statusCode || 400 + return response.status(statusCode).send({ success: false, error: error.message, }) diff --git a/admin/app/services/benchmark_service.ts b/admin/app/services/benchmark_service.ts index b5da37c..e9344cc 100644 --- a/admin/app/services/benchmark_service.ts +++ b/admin/app/services/benchmark_service.ts @@ -26,6 +26,7 @@ import { randomUUID, createHmac } from 'node:crypto' import { DockerService } from './docker_service.js' import { SERVICE_NAMES } from '../../constants/service_names.js' import { BROADCAST_CHANNELS } from '../../constants/broadcast.js' +import Dockerode from 'dockerode' // HMAC secret for signing submissions to the benchmark repository // This provides basic protection against casual API abuse. @@ -186,8 +187,14 @@ export class BenchmarkService { return response.data as RepositorySubmitResponse } catch (error) { - logger.error(`Failed to submit benchmark to repository: ${error.message}`) - throw new Error(`Failed to submit benchmark: ${error.message}`) + const detail = error.response?.data?.error || error.message || 'Unknown error' + const statusCode = error.response?.status + logger.error(`Failed to submit benchmark to repository: ${detail} (Status: ${statusCode})`) + + // Create an error with the status code attached for proper handling upstream + const err: any = new Error(`Failed to submit benchmark: ${detail}`) + err.statusCode = statusCode + throw err } } @@ -707,23 +714,26 @@ export class BenchmarkService { * Run a sysbench command in a Docker container */ private async _runSysbenchCommand(cmd: string[]): Promise { + let container: Dockerode.Container | null = null try { // Create container with TTY to avoid multiplexed output - const container = await this.dockerService.docker.createContainer({ + container = await this.dockerService.docker.createContainer({ Image: SYSBENCH_IMAGE, Cmd: cmd, name: `${SYSBENCH_CONTAINER_NAME}_${Date.now()}`, Tty: true, // Important: prevents multiplexed stdout/stderr headers HostConfig: { - AutoRemove: true, + AutoRemove: false, // Don't auto-remove to avoid race condition with fetching logs }, }) // Start container await container.start() - // Wait for completion and get logs + // Wait for completion await container.wait() + + // Get logs after container has finished const logs = await container.logs({ stdout: true, stderr: true, @@ -734,8 +744,24 @@ export class BenchmarkService { .replace(/[\x00-\x08]/g, '') // Remove control characters .trim() + // Manually remove the container after getting logs + try { + await container.remove() + } catch (removeError) { + // Log but don't fail if removal fails (container might already be gone) + logger.warn(`Failed to remove sysbench container: ${removeError.message}`) + } + return output } catch (error) { + // Clean up container on error if it exists + if (container) { + try { + await container.remove({ force: true }) + } catch (removeError) { + // Ignore removal errors + } + } logger.error(`Sysbench command failed: ${error.message}`) throw new Error(`Sysbench command failed: ${error.message}`) } diff --git a/admin/docs/release-notes.md b/admin/docs/release-notes.md index 6677f71..4609df1 100644 --- a/admin/docs/release-notes.md +++ b/admin/docs/release-notes.md @@ -6,6 +6,8 @@ - **Collections**: Complete overhaul of collection management with dynamic manifests, database tracking of installed resources, and improved UI for managing ZIM files and map assets - **Collections**: Added support for checking if newer versions of installed resources are available based on manifest data ### Bug Fixes +- **Benchmark**: Improved error handling and status code propagation for better user feedback on submission failures +- **Benchmark**: Fix a race condition in the sysbench container management that could lead to benchmark test failures ### Improvements diff --git a/admin/inertia/lib/api.ts b/admin/inertia/lib/api.ts index e6dfe89..a786ee8 100644 --- a/admin/inertia/lib/api.ts +++ b/admin/inertia/lib/api.ts @@ -448,10 +448,20 @@ class API { } async submitBenchmark(benchmark_id: string, anonymous: boolean) { - return catchInternal(async () => { + try { const response = await this.client.post('/benchmark/submit', { benchmark_id, anonymous }) return response.data - })() + } catch (error: any) { + // For 409 Conflict errors, throw a specific error that the UI can handle + if (error.response?.status === 409) { + const err = new Error(error.response?.data?.error || 'This benchmark has already been submitted to the repository') + ;(err as any).status = 409 + throw err + } + // For other errors, extract the message and throw + const errorMessage = error.response?.data?.error || error.message || 'Failed to submit benchmark' + throw new Error(errorMessage) + } } async subscribeToReleaseNotes(email: string) { diff --git a/admin/inertia/pages/settings/benchmark.tsx b/admin/inertia/pages/settings/benchmark.tsx index 30ab4c7..f08a57a 100644 --- a/admin/inertia/pages/settings/benchmark.tsx +++ b/admin/inertia/pages/settings/benchmark.tsx @@ -173,8 +173,13 @@ export default function BenchmarkPage(props: { refetchLatest() queryClient.invalidateQueries({ queryKey: ['benchmark', 'history'] }) }, - onError: (error: Error) => { - setSubmitError(error.message) + onError: (error: any) => { + // Check if this is a 409 Conflict error (already submitted) + if (error.status === 409) { + setSubmitError('A benchmark for this system with the same or higher score has already been submitted.') + } else { + setSubmitError(error.message) + } }, })