feat(api): add container job API for unified plugin container execution#22
Closed
dirkwa wants to merge 5 commits into
Closed
feat(api): add container job API for unified plugin container execution#22dirkwa wants to merge 5 commits into
dirkwa wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Container Jobs API: new public types and server-api exports, runtime detection and container execution utilities, an in-memory job manager with REST routes and OpenAPI docs, server startup integration, configuration settings, and tests. Changes
Sequence DiagramssequenceDiagram
participant Client
participant API as ContainerJobsApi
participant Runtime as RuntimeDetector
participant Image as ImageManager
participant Exec as ContainerExecutor
Client->>API: POST / (run job config)
API->>Runtime: detect/get runtime
API->>API: enforce concurrency, create job (pending)
API->>Image: imageExists(image)
alt image missing
API->>Image: pullImage(image)
Image-->>API: progress events
end
API->>Exec: executeContainer(config)
Exec-->>API: stream stdout/stderr (onProgress)
Exec-->>API: exitCode
API->>API: set status completed/failed, schedule retention cleanup
API-->>Client: return ContainerJobResult
sequenceDiagram
participant App
participant API as ContainerJobsApi
participant Podman
participant Docker
App->>API: start()
API->>Podman: podman --version
alt podman available
Podman-->>API: version → RuntimeInfo(podman)
else podman missing
API->>Docker: docker --version
Docker-->>API: version → RuntimeInfo(docker)
end
API-->>App: store runtime info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dirkwa
force-pushed
the
container-job-api
branch
from
bdd5516 to
93b1149
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/index.ts (1)
106-127:⚠️ Potential issue | 🟠 MajorGate
runContainerJob()on Container Jobs API startup to avoid transient 503s.On Line 111, calls can arrive before Line 126 startup resolves, causing false “no runtime” failures.
Proposed fix
// eslint-disable-next-line `@typescript-eslint/no-explicit-any` const containerJobsApi = new ContainerJobsApi(app as any) + const containerJobsApiStart = containerJobsApi.start() // eslint-disable-next-line `@typescript-eslint/no-explicit-any` ;(app as any).containerJobsApi = containerJobsApi // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - ;(app as any).runContainerJob = (config: ContainerJobConfig) => - containerJobsApi.runJob(config) + ;(app as any).runContainerJob = async (config: ContainerJobConfig) => { + await containerJobsApiStart + return containerJobsApi.runJob(config) + } // eslint-disable-next-line `@typescript-eslint/no-explicit-any` ;(app as any).getContainerRuntime = () => containerJobsApi.getRuntimeInfo() apiList.push('containerjobs') @@ autopilotApi.start(), radarApi.start(), historyApiHttpRegistry.start(), notificationApi.start(), - containerJobsApi.start() + containerJobsApiStart ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/index.ts` around lines 106 - 127, Assign and use the Container Jobs API start promise to gate calls: create a variable like containerJobsStart = containerJobsApi.start() and keep attaching containerJobsApi to app, but replace direct assignments for runContainerJob and getContainerRuntime with wrappers that await containerJobsStart before delegating to containerJobsApi.runJob or containerJobsApi.getRuntimeInfo; e.g., app.runContainerJob = async (config: ContainerJobConfig) => { await containerJobsStart; return containerJobsApi.runJob(config) } and similarly await containerJobsStart in app.getContainerRuntime, ensuring any incoming calls are delayed until containerJobsApi.start() has completed.
🧹 Nitpick comments (1)
test/containerjobs.ts (1)
20-39: Protectprocess.envrestoration withtry/finally.If an assertion fails before Line 35-38, global env state leaks into other tests.
Proposed fix
it('strips systemd socket activation variables', () => { const original = { ...process.env } - process.env.LISTEN_FDS = '1' - process.env.LISTEN_PID = '1234' - process.env.LISTEN_FDNAMES = 'test' - process.env.HOME = '/home/test' - - const env = cleanEnv() - - expect(env.LISTEN_FDS).to.be.undefined - expect(env.LISTEN_PID).to.be.undefined - expect(env.LISTEN_FDNAMES).to.be.undefined - expect(env.HOME).to.equal('/home/test') - - // Restore - delete process.env.LISTEN_FDS - delete process.env.LISTEN_PID - delete process.env.LISTEN_FDNAMES - Object.assign(process.env, original) + try { + process.env.LISTEN_FDS = '1' + process.env.LISTEN_PID = '1234' + process.env.LISTEN_FDNAMES = 'test' + process.env.HOME = '/home/test' + + const env = cleanEnv() + + expect(env.LISTEN_FDS).to.be.undefined + expect(env.LISTEN_PID).to.be.undefined + expect(env.LISTEN_FDNAMES).to.be.undefined + expect(env.HOME).to.equal('/home/test') + } finally { + delete process.env.LISTEN_FDS + delete process.env.LISTEN_PID + delete process.env.LISTEN_FDNAMES + Object.assign(process.env, original) + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/containerjobs.ts` around lines 20 - 39, The test "strips systemd socket activation variables" mutates process.env and restores it only at the end, which can leak if an assertion fails; wrap the setup and assertions in a try/finally so the original env (saved in the original variable) is restored in the finally block and the deletes/Object.assign call always run; update the test block around process.env manipulation and use the existing original variable and cleanup logic in the finally to guarantee restoration after calling cleanEnv() and running expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/containerjobs/index.ts`:
- Around line 222-239: The GET handlers for `${CONTAINER_JOBS_API_PATH}` and
`${CONTAINER_JOBS_API_PATH}/:jobId` currently return job metadata/logs without
any permission check; update both this.app.get handlers to enforce the same
feature permission used by the DELETE route (i.e., reuse the same
authorization/middleware or the same feature flag check present in the DELETE
handler) so reads require the container-jobs feature (or add a read-specific
permission if desired), returning 403 when unauthorized and preserving the
existing 404/JSON responses when authorized.
- Around line 128-144: The job currently reuses config.timeout for both
pullImage(...) and executeContainer(...), doubling the intended timeout window;
decide whether timeout should be a total budget or per-phase and implement
accordingly: if total budget, compute a deadline near job start (e.g., const
deadline = Date.now() + (config.timeout ?? default)) and pass remaining time to
pullImage(this.runtimeInfo, config.image, Math.max(0, deadline - Date.now()))
and to executeContainer by computing the remaining budget before each call; if
per-phase timeouts are desired, add explicit fields (e.g., config.pullTimeout
and config.runTimeout) and use those when calling pullImage and
executeContainer; update references to config.timeout, pullImage(...), and
executeContainer(...) to use the chosen approach so the job doesn't consume
double the intended time.
- Around line 104-115: Snapshot the incoming config (including config.command
and any arrays/objects) into a detached copy before any await and use that copy
to build the job object (use jobId and ContainerJobResult) so you never hold a
reference to the caller's config; likewise, when storing the job in the internal
map and when returning results (return { ...job }), return deep/detached copies
of mutable fields (command and log arrays, nested objects) so callers cannot
mutate internal state or submitted config; update all similar creation/return
sites (the blocks around lines creating job, the subsequent sections at 121-144
and 168) to follow this pattern.
In `@src/api/containerjobs/openApi.ts`:
- Around line 69-83: The OpenAPI DELETE operation for the container jobs
endpoint is missing the 403 response that the runtime can return; update the
delete operation's responses (the 'delete' object in openApi) to include a '403'
entry (e.g., '403': { description: 'Forbidden - access denied' }) so the spec
matches the handler's behavior when access is denied.
In `@src/api/containerjobs/runtime.ts`:
- Around line 87-108: pullImage currently uses execFile (which buffers
stdout/stderr) and doesn't expose the child, causing large pulls to crash and
preventing stop() from cancelling them; change pullImage to spawn the
runtimeBinary(info) with args ['pull', image] (using cleanEnv() for env), stream
the child process stdout/stderr instead of buffering, attach 'close'/'error'
handlers to resolve on exit code 0 and reject otherwise (include collected
stderr up to a reasonable cap in the error), implement timeout by killing the
spawned child on timeout, and return/expose the ChildProcess to the caller in
the same fashion as executeContainer so ContainerJobsApi.stop() can terminate an
in-progress pull.
- Around line 180-184: The handleData handler calls onProgress directly and can
let exceptions escape; wrap the onProgress?.({ stream, data: text }) call in a
try/catch inside handleData (keeping appendToLog(text) before/after as needed)
and in the catch either call the job promise rejection function (the promise's
reject handler used when creating the ChildProcess lifecycle promise) with the
caught error or log a clear warning via your logger so the error becomes a
controlled job failure instead of an uncaught exception; reference: handleData,
onProgress, appendToLog, and the enclosing promise rejection path.
---
Outside diff comments:
In `@src/api/index.ts`:
- Around line 106-127: Assign and use the Container Jobs API start promise to
gate calls: create a variable like containerJobsStart = containerJobsApi.start()
and keep attaching containerJobsApi to app, but replace direct assignments for
runContainerJob and getContainerRuntime with wrappers that await
containerJobsStart before delegating to containerJobsApi.runJob or
containerJobsApi.getRuntimeInfo; e.g., app.runContainerJob = async (config:
ContainerJobConfig) => { await containerJobsStart; return
containerJobsApi.runJob(config) } and similarly await containerJobsStart in
app.getContainerRuntime, ensuring any incoming calls are delayed until
containerJobsApi.start() has completed.
---
Nitpick comments:
In `@test/containerjobs.ts`:
- Around line 20-39: The test "strips systemd socket activation variables"
mutates process.env and restores it only at the end, which can leak if an
assertion fails; wrap the setup and assertions in a try/finally so the original
env (saved in the original variable) is restored in the finally block and the
deletes/Object.assign call always run; update the test block around process.env
manipulation and use the existing original variable and cleanup logic in the
finally to guarantee restoration after calling cleanEnv() and running
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58249e74-5aff-4862-bdaf-722948429658
📒 Files selected for processing (11)
packages/server-api/src/containerjobapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tssrc/api/containerjobs/index.tssrc/api/containerjobs/openApi.tssrc/api/containerjobs/runtime.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tstest/containerjobs.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/api/containerjobs/runtime.ts (2)
180-184:⚠️ Potential issue | 🟠 MajorProtect
onProgresscallback failures from escaping event handlers.Line 183 invokes
onProgressdirectly inside'data'listeners. If the callback throws, it can bypass the job promise lifecycle and surface as an uncaught exception. Catch and route this as a controlled job failure (or at least a guarded warning path).Suggested direction
- const handleData = (stream: 'stdout' | 'stderr') => (data: Buffer) => { + let progressError: Error | null = null + const handleData = (stream: 'stdout' | 'stderr') => (data: Buffer) => { const text = data.toString() appendToLog(log, text) - onProgress?.({ stream, data: text }) + try { + onProgress?.({ stream, data: text }) + } catch (err) { + progressError = err instanceof Error ? err : new Error(String(err)) + child.kill('SIGKILL') + } } @@ - child.on('close', (code) => { + child.on('close', (code) => { if (timeoutId) { clearTimeout(timeoutId) } + if (progressError) { + reject(progressError) + return + } resolve({ exitCode: code ?? 1 }) })Also applies to: 192-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 180 - 184, Wrap any direct calls to the onProgress callback inside a try/catch in the handleData closure (and the other data listeners around lines 192-213) so thrown errors can’t escape the stream event handlers; on catch, log the error with appendToLog(log, ...) and route it to the job failure path (e.g., call the existing job error/reject handler or invoke the centralized failure function used by this runtime) so the job promise is settled predictably instead of allowing an uncaught exception.
87-109:⚠️ Potential issue | 🟠 MajorSwitch
pullImage()fromexecFile()to streamedspawn()and expose the child.Line 94 uses
execFile()forpull, which can fail on verbose pulls due to buffered output limits, and the currentPromise<void>return shape prevents canceling an in-flight pull from job stop logic. This is still a release-impacting reliability gap.Suggested direction
-export function pullImage( +export function pullImage( info: ContainerRuntimeInfo, image: string, timeout: number = DEFAULT_PULL_TIMEOUT -): Promise<void> { +): { promise: Promise<void>; child: ChildProcess } { const binary = runtimeBinary(info) - return new Promise((resolve, reject) => { - execFile( - binary, - ['pull', image], - { timeout, env: cleanEnv() }, - (error, _stdout, stderr) => { - if (error) { - reject( - new Error(`Failed to pull ${image}: ${stderr || error.message}`) - ) - } else { - resolve() - } - } - ) - }) + const child = spawn(binary, ['pull', image], { + stdio: ['ignore', 'pipe', 'pipe'], + env: cleanEnv() + }) + const promise = new Promise<void>((resolve, reject) => { + // collect bounded stderr, enforce timeout, resolve/reject on close/error + }) + return { promise, child } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 87 - 109, Replace the execFile-based pullImage implementation with a spawn-based one: call runtimeBinary(info) and spawn the process with ['pull', image] and env: cleanEnv(), attach streaming listeners for stdout and stderr (pipe data through or log), and create a Promise that resolves on child 'exit' with code 0 and rejects on nonzero exit or 'error' events; ensure you enforce timeout by setting a timer that kills the child and rejects if exceeded (use DEFAULT_PULL_TIMEOUT as default) and return both the Promise and the ChildProcess so callers can cancel in-flight pulls (update pullImage signature to return an object exposing the child and the completion promise). Ensure error messages include stderr and error.message and that the child is properly cleaned up on all error/timeout paths.
🧹 Nitpick comments (1)
src/api/containerjobs/openApi.ts (1)
89-108: Mark conditional and optional fields as nullable in the OpenAPI schema.The OpenAPI schema doesn't align with the TypeScript interface. Several fields marked as optional in the TypeScript type should be marked as nullable in the schema:
error(optional in TypeScript, only present on failed jobs)label(optional in TypeScript)startedAt(optional in TypeScript, only set after job starts)completedAt(optional in TypeScript, only set after job completes)Additionally, add a
requiredarray to explicitly specify which fields are always present.Proposed fix
ContainerJobResult: { type: 'object', + required: ['id', 'status', 'image', 'command', 'exitCode', 'log', 'createdAt', 'runtime'], properties: { id: { type: 'string', format: 'uuid' }, status: { type: 'string', enum: ['pending', 'pulling', 'running', 'completed', 'failed'] }, image: { type: 'string' }, command: { type: 'array', items: { type: 'string' } }, - label: { type: 'string' }, + label: { type: 'string', nullable: true }, exitCode: { type: 'integer', nullable: true }, log: { type: 'array', items: { type: 'string' } }, - error: { type: 'string' }, + error: { type: 'string', nullable: true }, createdAt: { type: 'string', format: 'date-time' }, - startedAt: { type: 'string', format: 'date-time' }, - completedAt: { type: 'string', format: 'date-time' }, + startedAt: { type: 'string', format: 'date-time', nullable: true }, + completedAt: { type: 'string', format: 'date-time', nullable: true }, runtime: { type: 'string', enum: ['podman', 'docker'] } } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/openApi.ts` around lines 89 - 108, The ContainerJobResult OpenAPI schema is missing nullable markers for optional/conditional fields and lacks an explicit required array; update the ContainerJobResult schema so that properties error, label, startedAt, and completedAt include nullable: true, and add a required: [...] array listing always-present fields (e.g., id, status, image, command, exitCode, log, createdAt, runtime — adjust to match your TypeScript interface) to ensure schema matches the TypeScript ContainerJobResult type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/containerjobs/runtime.ts`:
- Around line 71-85: The imageExists function currently calls execFile(binary,
args, { env: cleanEnv() }, ...) without a timeout causing potential hangs;
update imageExists to pass the same timeout option used in pullImage (add a
timeout property to the execFile options) so the external probe aborts when the
runtime CLI/daemon is unresponsive; keep using runtimeBinary(info),
isPodman(info) and cleanEnv() and resolve based on the callback error as before.
---
Duplicate comments:
In `@src/api/containerjobs/runtime.ts`:
- Around line 180-184: Wrap any direct calls to the onProgress callback inside a
try/catch in the handleData closure (and the other data listeners around lines
192-213) so thrown errors can’t escape the stream event handlers; on catch, log
the error with appendToLog(log, ...) and route it to the job failure path (e.g.,
call the existing job error/reject handler or invoke the centralized failure
function used by this runtime) so the job promise is settled predictably instead
of allowing an uncaught exception.
- Around line 87-109: Replace the execFile-based pullImage implementation with a
spawn-based one: call runtimeBinary(info) and spawn the process with ['pull',
image] and env: cleanEnv(), attach streaming listeners for stdout and stderr
(pipe data through or log), and create a Promise that resolves on child 'exit'
with code 0 and rejects on nonzero exit or 'error' events; ensure you enforce
timeout by setting a timer that kills the child and rejects if exceeded (use
DEFAULT_PULL_TIMEOUT as default) and return both the Promise and the
ChildProcess so callers can cancel in-flight pulls (update pullImage signature
to return an object exposing the child and the completion promise). Ensure error
messages include stderr and error.message and that the child is properly cleaned
up on all error/timeout paths.
---
Nitpick comments:
In `@src/api/containerjobs/openApi.ts`:
- Around line 89-108: The ContainerJobResult OpenAPI schema is missing nullable
markers for optional/conditional fields and lacks an explicit required array;
update the ContainerJobResult schema so that properties error, label, startedAt,
and completedAt include nullable: true, and add a required: [...] array listing
always-present fields (e.g., id, status, image, command, exitCode, log,
createdAt, runtime — adjust to match your TypeScript interface) to ensure schema
matches the TypeScript ContainerJobResult type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87091c66-0fc8-4539-a4c8-1d518c0c3959
📒 Files selected for processing (11)
packages/server-api/src/containerjobapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tssrc/api/containerjobs/index.tssrc/api/containerjobs/openApi.tssrc/api/containerjobs/runtime.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tstest/containerjobs.ts
✅ Files skipped from review due to trivial changes (4)
- packages/server-api/src/features.ts
- src/config/config.ts
- packages/server-api/src/containerjobapi.ts
- packages/server-api/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/api/swagger.ts
- packages/server-api/src/serverapi.ts
- src/api/index.ts
- test/containerjobs.ts
- src/api/containerjobs/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/api/containerjobs/runtime.ts (4)
8-9: Consider renamingDEFAULT_PULL_TIMEOUTfor clarity.This constant is used as the default timeout for both image pulls and container execution (line 252). Consider renaming to
DEFAULT_JOB_TIMEOUTorDEFAULT_OPERATION_TIMEOUTto better reflect its usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 8 - 9, Rename the constant DEFAULT_PULL_TIMEOUT to a name that reflects its broader use (e.g., DEFAULT_JOB_TIMEOUT or DEFAULT_OPERATION_TIMEOUT) and update all references to it (including its declaration and usages such as the image pull timeout and container execution timeout in this file, e.g., where DEFAULT_PULL_TIMEOUT is read around the container run/execute logic). Ensure the constant semantic remains the same (same numeric value) and update any related comments/docstrings to match the new name so calls like the image pull and container execution timeout clearly use DEFAULT_JOB_TIMEOUT.
19-35: Consider adding a timeout totryRuntime()for robustness.If the container daemon is unresponsive (e.g., Docker socket exists but daemon is hung),
execFilewill block indefinitely. A short timeout (e.g., 10-15 seconds) would prevent startup from hanging.♻️ Proposed fix
+const RUNTIME_CHECK_TIMEOUT = 15000 + function tryRuntime( binary: string ): Promise<{ available: boolean; version: string; output: string }> { return new Promise((resolve) => { - execFile(binary, ['--version'], { env: cleanEnv() }, (error, stdout) => { + execFile(binary, ['--version'], { env: cleanEnv(), timeout: RUNTIME_CHECK_TIMEOUT }, (error, stdout) => { if (error) { resolve({ available: false, version: '', output: '' }) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 19 - 35, The tryRuntime function can hang if execFile never returns; update tryRuntime to pass a timeout (e.g., 10000–15000 ms) in the execFile options and handle timeout errors so the Promise always resolves (for example treat ETIMEDOUT as available: false). Specifically modify the call to execFile(binary, ['--version'], { env: cleanEnv(), timeout: <ms> }, ...) and in the callback detect timeout/error cases to resolve with { available: false, version: '', output: '' } while keeping the success path returning version/output as before.
169-189: Minor clarity improvement forzFlaglogic.The current logic works correctly, but using
zFlagboth as a suffix (,Z) and as a boolean check (line 184) is confusing. Consider a clearer approach:♻️ Proposed refactor for clarity
export function buildVolumeArgs( config: ContainerJobConfig, info: ContainerRuntimeInfo ): string[] { const args: string[] = [] - const zFlag = isPodman(info) ? ',Z' : '' + const useSELinux = isPodman(info) if (config.inputs) { for (const [containerPath, hostPath] of Object.entries(config.inputs)) { - args.push('-v', `${hostPath}:${containerPath}:ro${zFlag}`) + args.push('-v', `${hostPath}:${containerPath}:ro${useSELinux ? ',Z' : ''}`) } } if (config.outputs) { for (const [containerPath, hostPath] of Object.entries(config.outputs)) { - args.push('-v', `${hostPath}:${containerPath}${zFlag ? ':Z' : ''}`) + args.push('-v', `${hostPath}:${containerPath}${useSELinux ? ':Z' : ''}`) } } return args }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 169 - 189, The zFlag variable in buildVolumeArgs is overloaded as a string (',Z') and then later used as a boolean check, which is confusing; change it to a clear boolean (e.g., isPodmanFlag = isPodman(info)) or use two distinct variables (isPodman and zSuffix) so the logic is explicit: compute a boolean isPodman once via isPodman(info) and compute a zSuffix (':Z' or ',Z' as needed) when building each mount string for config.inputs and config.outputs; update the code paths in buildVolumeArgs that push '-v' entries to use the boolean or the explicit zSuffix so there's no mixed-type usage of zFlag.
137-167: Consider clarifyingPruneResultsemantics.The
imagesRemovedcount is derived from line count, which varies across runtime output formats. ThespaceReclaimedfield contains raw stdout rather than a parsed value. This works but may produce inconsistent results between Podman and Docker.If precise metrics are needed later, consider parsing the
--formatflag output or documenting that these values are approximate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/containerjobs/runtime.ts` around lines 137 - 167, pruneImages currently counts lines and returns raw stdout which yields inconsistent metrics across runtimes; change the execFile call (runtimeBinary/pruneImages) to invoke the image prune command with a deterministic --format (eg. JSON or a stable template) and parse that output into PruneResult so imagesRemoved is an integer and spaceReclaimed is a parsed string (or null) instead of raw stdout; update PruneResult docs/comments accordingly and handle parsing errors by rejecting with a clear message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api/containerjobs/runtime.ts`:
- Around line 8-9: Rename the constant DEFAULT_PULL_TIMEOUT to a name that
reflects its broader use (e.g., DEFAULT_JOB_TIMEOUT or
DEFAULT_OPERATION_TIMEOUT) and update all references to it (including its
declaration and usages such as the image pull timeout and container execution
timeout in this file, e.g., where DEFAULT_PULL_TIMEOUT is read around the
container run/execute logic). Ensure the constant semantic remains the same
(same numeric value) and update any related comments/docstrings to match the new
name so calls like the image pull and container execution timeout clearly use
DEFAULT_JOB_TIMEOUT.
- Around line 19-35: The tryRuntime function can hang if execFile never returns;
update tryRuntime to pass a timeout (e.g., 10000–15000 ms) in the execFile
options and handle timeout errors so the Promise always resolves (for example
treat ETIMEDOUT as available: false). Specifically modify the call to
execFile(binary, ['--version'], { env: cleanEnv(), timeout: <ms> }, ...) and in
the callback detect timeout/error cases to resolve with { available: false,
version: '', output: '' } while keeping the success path returning
version/output as before.
- Around line 169-189: The zFlag variable in buildVolumeArgs is overloaded as a
string (',Z') and then later used as a boolean check, which is confusing; change
it to a clear boolean (e.g., isPodmanFlag = isPodman(info)) or use two distinct
variables (isPodman and zSuffix) so the logic is explicit: compute a boolean
isPodman once via isPodman(info) and compute a zSuffix (':Z' or ',Z' as needed)
when building each mount string for config.inputs and config.outputs; update the
code paths in buildVolumeArgs that push '-v' entries to use the boolean or the
explicit zSuffix so there's no mixed-type usage of zFlag.
- Around line 137-167: pruneImages currently counts lines and returns raw stdout
which yields inconsistent metrics across runtimes; change the execFile call
(runtimeBinary/pruneImages) to invoke the image prune command with a
deterministic --format (eg. JSON or a stable template) and parse that output
into PruneResult so imagesRemoved is an integer and spaceReclaimed is a parsed
string (or null) instead of raw stdout; update PruneResult docs/comments
accordingly and handle parsing errors by rejecting with a clear message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53510c7f-ac9d-40e4-bd35-dbc2ff577d0f
📒 Files selected for processing (11)
packages/server-api/src/containerjobapi.tspackages/server-api/src/features.tspackages/server-api/src/index.tspackages/server-api/src/serverapi.tssrc/api/containerjobs/index.tssrc/api/containerjobs/openApi.tssrc/api/containerjobs/runtime.tssrc/api/index.tssrc/api/swagger.tssrc/config/config.tstest/containerjobs.ts
✅ Files skipped from review due to trivial changes (3)
- packages/server-api/src/index.ts
- src/config/config.ts
- packages/server-api/src/containerjobapi.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/server-api/src/features.ts
- src/api/swagger.ts
- src/api/index.ts
- test/containerjobs.ts
- src/api/containerjobs/openApi.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-api/src/containerjobapi.ts`:
- Line 35: The onProgress callback on the container job API (onProgress?: (data:
ContainerJobProgress) => void) only works for in-process consumers and cannot
deliver real-time updates to REST/HTTP clients; update the JSDoc for the
onProgress property to explicitly state this limitation and recommend using an
HTTP-friendly alternative, and add or wire up an alternative mechanism (e.g.,
Server-Sent Events endpoint, WebSocket support, or a polling/status endpoint
such as a getJobProgress(jobId) API) so HTTP clients can receive progress
updates; reference the onProgress property and the ContainerJobProgress type in
the JSDoc and implement or document the chosen alternative for REST consumers.
- Around line 47-48: The relationship between streaming progress chunks and the
aggregated log is unclear; update the ContainerJobProgress and
ContainerJobResult.log documentation and the code that aggregates progress into
logs (where progress chunks are consumed) to explain exactly how chunks are
transformed (e.g., buffered and split on newline boundaries vs. pushed raw as
entries), whether stdout and stderr are merged or preserved (and how they are
tagged if preserved), and any log size/entry limits, truncation or retention
policy; reference ContainerJobProgress (data: string) and ContainerJobResult.log
(string[]) in the comments and add precise behavior notes in the aggregator
function that converts progress events into the log array.
- Around line 58-59: Document the expected format for the version property and
optionally tighten its type: add a JSDoc comment above the version field
describing the expected format (e.g., "semantic versioning (MAJOR.MINOR.PATCH),
optional pre-release and build metadata") and any constraints, and either (a)
import and use a SemVer/Version type from a library like `semver` or define a
branded type/alias (e.g., type VersionString = string & { __brand?:
'VersionString' }) to make intent explicit, or (b) keep string but make the
format explicit in the comment; refer to the version field and nearby
isPodmanDockerShim? boolean to locate the property in the interface.
- Around line 33-34: The JSDoc for the timeout option is ambiguous; update the
comment for the timeout property to explicitly state the runtime behavior when
the timeout expires: whether runContainerJob() throws or returns a
ContainerJobResult with status: 'failed' and an error message, that the
container is forcibly terminated, and that cleanup/resource release is performed
(or not). Reference the timeout field and the runContainerJob() return type
ContainerJobResult (including status and error message fields) and add a short
sentence describing container termination and cleanup behavior upon timeout so
callers know to expect either an exception or a failed result and that resources
are cleaned up.
- Around line 49-51: Document that the timestamp fields createdAt, startedAt,
and completedAt use ISO 8601 strings by adding JSDoc or inline comments and/or
replacing the plain string annotation with a descriptive type alias (e.g.,
ISO8601String) where these fields are declared so consumers know the expected
format; update the declaration for createdAt, startedAt?, completedAt? and any
related interfaces or types in containerjobapi.ts to reference the
documentation/type alias for consistency.
- Around line 66-72: Define and use a specific error type (e.g.,
ContainerJobError extends Error with a public statusCode: number) and update the
runContainerJob JSDoc to reference that class instead of a plain Error; change
runContainerJob to throw ContainerJobError for infrastructure/precondition
problems (e.g., no runtime -> statusCode 503, concurrency limit -> statusCode
429) and to return a ContainerJobResult with status and exitCode for job-level
failures (non‑zero exit codes) so callers can distinguish thrown HTTP-mapped
errors from completed-but-failed jobs; mention these semantics in the JSDoc and
ensure any code invoking runContainerJob checks for ContainerJobError vs
inspecting ContainerJobResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8b8c569-d363-49f5-b8e6-0517ad8042d6
📒 Files selected for processing (2)
.coderabbit.yamlpackages/server-api/src/containerjobapi.ts
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 9-55: The configuration uses an invalid field
reviews.instructions; replace it with the supported reviews.path_instructions
structure by removing reviews.instructions and adding reviews.path_instructions
as an array of objects where each object includes a path glob (e.g., "*.md" or
specific file patterns) and an instructions string; update the YAML so any
existing instructions text is moved into the appropriate instructions value for
a matching path entry and validate against the schema that
reviews.path_instructions is present and properly formatted.
In `@src/api/containerjobs/index.ts`:
- Around line 272-281: The GET list handler registered at this.app.get for
CONTAINER_JOBS_API_PATH currently returns the live job objects from this.jobs,
exposing mutable fields; update the handler so it returns deep-copied job
representations (same approach used by runJob() and the single-job endpoint)
instead of the original objects—e.g., map Array.from(this.jobs.values()) to a
new array of cloned job objects (copying nested mutable fields like
arrays/objects) and return that, preserving authorization via writeAllowed(req)
before responding.
- Around line 226-230: The saveSettings() method calls
writeSettingsFile(this.app as any, this.app.config.settings, ...) but ignores
the error argument in the callback, so write failures are silently dropped;
update saveSettings() to pass a callback that accepts (err) and handles errors
by logging via debug or this.app.logger (e.g., debug('Error saving settings',
err) or this.app.logger.error(...)) and optionally surface the error if needed,
ensuring the callback references writeSettingsFile, saveSettings, this.app and
this.app.config.settings so the change is easy to locate.
- Around line 263-269: The GET handler currently returns the internal job object
from this.jobs.get(req.params.jobId) directly (and calls res.json(job)),
exposing mutable internal state; mirror runJob()'s behavior by returning a safe
deep copy instead of the original. Update the handler that uses this.jobs and
res.json(job) to clone the job (or at least clone the job.command and job.log
arrays/objects) before calling res.json, ensuring external clients cannot mutate
internal state; reference the existing runJob() copying approach as the pattern
to follow and keep Responses.notFound unchanged for the missing-case branch.
In `@test/containerjobs.ts`:
- Around line 112-122: The test for buildVolumeArgs (using ContainerJobConfig
and podmanInfo) only checks length and presence of '-v' — update it to assert
the actual volume mount strings regardless of object iteration order: construct
the expected mount strings for inputs ('/host/a:/in1' and '/host/b:/in2') and
outputs ('/host/c:/out1') and verify the args array contains these strings
(e.g., by filtering args for '-v' and checking the adjacent entries or by
checking inclusion of each expected string using order-independent assertions)
so the test fails if any specific mount is missing or incorrect.
- Around line 264-268: The test for ContainerJobsApi.stop() only ensures it
doesn't throw; update the test to assert actual cleanup by either exposing a way
to inspect active timers/jobs or injecting a mock timer/child process and
asserting they are cleared/terminated after calling api.stop(); locate
ContainerJobsApi and its stop() method and modify tests around createMockApp()
to inject the mock timer/child or call a new accessor like getActiveJobCount()
(or similar) and assert it becomes zero, or verify the mock timer's
clear/terminate methods were called.
- Around line 32-37: The current finally block deletes a few test vars then runs
Object.assign(process.env, original), which leaves any new env keys added during
the test; change the restoration to fully revert process.env: capture original
as a shallow copy at test start (original = {...process.env}), and in the
finally iterate Object.keys(process.env) to delete any key not present in
original, then iterate Object.keys(original) to set process.env[key] =
original[key] (or delete when original[key] is undefined); update the finally
block replacing the current delete/process.env assignment logic (references:
process.env, original, the finally cleanup) to ensure no orphan keys remain
after tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dbd3730-d9d3-458b-a962-608f10f60f87
📒 Files selected for processing (5)
.coderabbit.yamlpackages/server-api/src/containerjobapi.tssrc/api/containerjobs/index.tssrc/api/containerjobs/runtime.tstest/containerjobs.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/containerjobs/index.ts`:
- Around line 67-77: The stop() method on the containerJobsApi class isn't being
called during server shutdown, causing activeChildren and cleanupTimers to leak;
modify the shutdown sequence so containerJobsApi.stop() is invoked—either add
the containerJobsApi instance (containerJobsApi or app.containerJobsApi) to the
existing shutdown cleanup loop that currently iterates app.interfaces/app.apis,
or explicitly call await app.containerJobsApi.stop() (or
containerJobsApi.stop()) before the server closes in the shutdown handler (the
code around the server shutdown loop that currently omits app.apis). Ensure you
await/handle stop() if it becomes async and keep references to activeChildren
and cleanupTimers cleared after stop() completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 475b75f0-eb80-4377-a9c6-cd0ced659cac
📒 Files selected for processing (2)
.coderabbit.yamlsrc/api/containerjobs/index.ts
dirkwa
force-pushed
the
container-job-api
branch
2 times, most recently
from
66b4a1b to
992a91a
Compare
Define the public API contract for container jobs: ContainerJobConfig, ContainerJobResult, ContainerJobProgress, ContainerRuntimeInfo, and ContainerJobError. Add runContainerJob(), getContainerRuntime(), and listContainerJobs() to the ServerAPI interface. Register containerJobs feature flag.
Detect podman or docker (podman-first), including podman-docker shim detection. Handle image existence checks, pulling with spawn() to avoid maxBuffer crashes, SELinux :Z volume flags, --init for clean container exit, deadline-based timeouts across pull and run phases, and dangling image pruning after pulls.
ContainerJobsApi class with concurrency control, config snapshotting, progress streaming, and rolling log buffer. REST endpoints for runtime info, job listing, job details, job deletion, and image pruning. All routes require containerJobs feature permission. OpenAPI specs for all endpoints including error responses.
Initialize ContainerJobsApi in startApis(), gate runContainerJob() on startup promise, register OpenAPI paths in swagger, add default settings to config, and call stop() during server shutdown to clean up active containers and timers.
Tests for runtime detection (podman/docker/shim), container argument construction (volumes, env, SELinux, timeout), image existence and pulling, API lifecycle (start/stop), REST endpoints with security checks, and error handling.
dirkwa
force-pushed
the
container-job-api
branch
from
33809ae to
9558c61
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a server-level container job API so plugins can run containers (podman/docker) without managing runtimes themselves. Closes SignalK#2477.
app.runContainerJob()for plugins: image pulling, SELinux volume flags, deadline-based timeouts, progress streaming via callback, concurrency control/signalk/v2/api/containerjobs/for runtime info, job listing, job details, deletion, and image pruning — all gated bycontainerJobsfeature permissionPOST /images/pruneendpointTested with
End-to-end tested with signalk-charts-provider-simple
use-server-job-apibranch, which drops ~300 lines of container boilerplate by switching to this API. Successfully downloaded and converted S-57 ENC charts through the full pipeline: download → GDAL export → tippecanoe tiling → chart available in Signal K resources.Commits
ContainerJobConfig,ContainerJobResult,ContainerJobError, feature flag,ServerAPIinterface additionsContainerJobsApiclass, concurrency control, REST endpoints, OpenAPI specs