feat: UNIX socket control plane for porter daemon - #12
Conversation
Bun.serve({fd}) was proposed in oven-sh/bun#2852 but never merged.
Connections open but Bun never dispatches to the handler when using fd.
Switch to direct UNIX socket binding with Bun.serve({unix}).
Systemd manages directory lifecycle via RuntimeDirectory=porter-%i,
giving each template instance its own isolated socket directory.
Also: use unified fetch handler instead of routes (avoids TS fd+routes
type incompatibility), clamp runs limit to 500, and prefer PORTER_SOCKET
env var for socket path resolution in the daemon.
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR migrates the control plane from systemd socket-activation (LISTEN_FDS) to direct UNIX socket binding with per-instance runtime directories. systemd units set PORTER_SOCKET to %t/porter-%i/porter.sock and create %t/porter-%i with RuntimeDirectory/RuntimeDirectoryMode. ControlServer.start() now computes the socket path, removes stale socket files, and calls Bun.serve({ unix: socketPath, routes }). Route handlers use BunRequest and read path params from req.params. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a427414-8c26-417d-9e64-38c30060ad2f
📒 Files selected for processing (4)
docs/control-plane.mdresources/systemd/porter-dev.serviceresources/systemd/porter@.serviceruntime/src/control-server.ts
| if (id && url.pathname === `/api/scheduled-tasks/${id}/runs` && req.method === 'GET') { | ||
| const limit = Number.parseInt(url.searchParams.get('limit') ?? '50', 10); | ||
| return this.handleGetTaskRuns(id, limit); |
There was a problem hiding this comment.
Reject negative limit values instead of normalizing them to the default.
The PR contract says negatives should be rejected, but limit=-1 currently falls through to handleGetTaskRuns() and comes back as 50. That makes bad input look valid and hides caller bugs.
Possible localized fix
if (id && url.pathname === `/api/scheduled-tasks/${id}/runs` && req.method === 'GET') {
const limit = Number.parseInt(url.searchParams.get('limit') ?? '50', 10);
+ if (Number.isFinite(limit) && limit < 0) {
+ return errorJson('limit must be non-negative', 400);
+ }
return this.handleGetTaskRuns(id, limit);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (id && url.pathname === `/api/scheduled-tasks/${id}/runs` && req.method === 'GET') { | |
| const limit = Number.parseInt(url.searchParams.get('limit') ?? '50', 10); | |
| return this.handleGetTaskRuns(id, limit); | |
| if (id && url.pathname === `/api/scheduled-tasks/${id}/runs` && req.method === 'GET') { | |
| const limit = Number.parseInt(url.searchParams.get('limit') ?? '50', 10); | |
| if (Number.isFinite(limit) && limit < 0) { | |
| return errorJson('limit must be non-negative', 400); | |
| } | |
| return this.handleGetTaskRuns(id, limit); |
| const socketPath = | ||
| process.env.PORTER_SOCKET || | ||
| (() => { | ||
| const runtimeDir = process.env.XDG_RUNTIME_DIR; | ||
| const stateDir = process.env.PORTER_STATE_DIR || `${process.env.HOME}/.local/state/porter`; | ||
| const base = runtimeDir ? `${runtimeDir}/porter` : stateDir; | ||
| return `${base}/porter.sock`; | ||
| })(); |
There was a problem hiding this comment.
Preserve the explicit unix override.
start() now ignores ControlServerOptions.unix and always recomputes the socket path from env/fallbacks. Any caller that passes a custom socket path will silently bind somewhere else.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
runtime/src/control-server.ts (2)
68-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate the socket directory before binding.
The fallback branch builds a path under
PORTER_STATE_DIR/HOME, but nothing creates that parent directory first. Outside the systemdRuntimeDirectorycase, first boot turns intoENOENTtheater instead of a control socket.Possible localized fix
+import { mkdir } from 'node:fs/promises'; +import { dirname } from 'node:path'; + async start(): Promise<void> { const socketPath = process.env.PORTER_SOCKET || (() => { const runtimeDir = process.env.XDG_RUNTIME_DIR; const stateDir = process.env.PORTER_STATE_DIR || `${process.env.HOME}/.local/state/porter`; const base = runtimeDir ? `${runtimeDir}/porter` : stateDir; return `${base}/porter.sock`; })(); + + await mkdir(dirname(socketPath), { recursive: true }); try { await Bun.file(socketPath).delete();As per coding guidelines: "Use Node node:fs only for directory operations like readdir/mkdir when not practical via Bun helpers".
Also applies to: 86-87
165-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-object JSON bodies before validation.
req.json()can legally give younull, andvalidateCreateTask(input)then dereferencesinput.id, so a bad client payload becomes a 500. Return 400 unless the parsed body is an object.Possible localized fix
private async handleCreateTask(req: BunRequest<'/api/scheduled-tasks'>): Promise<Response> { let body: unknown; try { body = await req.json(); } catch { return errorJson('invalid JSON body', 400); } + + if (!body || typeof body !== 'object' || Array.isArray(body)) { + return errorJson('JSON body must be an object', 400); + } const input = body as Record<string, unknown>; const validation = validateCreateTask(input); if (validation) return errorJson(validation, 400);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bb57266-1280-4336-b7af-015f43299fe8
📒 Files selected for processing (1)
runtime/src/control-server.ts
1 participant
Summary
Adds a REST API over a UNIX domain socket for runtime management of the porter daemon: scheduled task CRUD, daemon health, and worker pool observability.
API endpoints
CLI
Socket lifecycle
Systemd manages the socket directory via
RuntimeDirectory=porter-%i. Each templateinstance gets its own isolated directory:
The daemon binds the socket directly with
Bun.serve({ unix }).Why not systemd socket activation?
Bun.serve({ fd })was proposed in oven-sh/bun#2852but the PR was closed without merging. Connections open successfully on the
passed fd but Bun never dispatches to the HTTP handler.
Design decisions
nextRunambiguityPOSTcreates session rows so FK constraints passPORTER_SOCKETenv var: set by the systemd service unit, consumed by both daemon and CLI?limiton runs endpoint capped at 500, rejects negatives